Bug 12921 - sh_offset for SHT_NOBITS sections
Summary: sh_offset for SHT_NOBITS sections
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.22
: P2 normal
Target Milestone: ---
Assignee: Alan Modra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-22 16:23 UTC by Jakub Jelinek
Modified: 2011-06-24 14:03 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
possible fix (2.07 KB, patch)
2011-06-23 15:31 UTC, Alan Modra
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2011-06-22 16:23:21 UTC
A recent change in ld broke prelink.

E>g. on the attached testcase:

struct A { char a; struct A *b; int *c; };
static struct A local = { 77, &local, 0 };
int vbss[16384] __attribute__((aligned (4096)));
int vdata __attribute__((aligned (4096))) = 5;

asm (".text; .balign 4096; vtext:; .previous");

int main()
{
  asm volatile ("" : : "m" (local), "m" (vbss[0]), "m" (vdata));
  return 0;
}

we used to emit (e.g. in 2.21.0.51.0.6, but like since forever):
  [24] .data             PROGBITS        0000000000603000 003000 001028 00  WA  0   0 4096
  [25] .bss              NOBITS          0000000000605000 004028 011000 00  WA  0   0 4096
  [26] .comment          PROGBITS        0000000000000000 004028 000058 01  MS  0   0  1
  [27] .shstrtab         STRTAB          0000000000000000 004080 0000ee 00      0   0  1

but now (2.21.52.0.1) we emit:
  [24] .data             PROGBITS        0000000000603000 003000 002004 00  WA  0   0 4096
  [25] .bss              NOBITS          0000000000606000 006000 011000 00  WA  0   0 4096
  [26] .comment          PROGBITS        0000000000000000 005004 000059 01  MS  0   0  1
  [27] .shstrtab         STRTAB          0000000000000000 00505d 0000ee 00      0   0  1

The problem is that the sh_offset for .bss is no longer smaller or equal to the next section's sh_offset, but, what's more important, is that it is even way after the end of the file on disk.
When elfutils libelf tries to write this during prelink --undo, it will write 0x6000 bytes of file instead of just 0x505d+0xee the original file had.
Can this change be please reverted?
Comment 1 Alan Modra 2011-06-23 00:27:06 UTC
This is most likely http://sourceware.org/ml/binutils/2011-05/msg00299.html
Why on earth does prelink write to a SHT_NOBITS section?

Incidentally, we've put bss sh_offset potentially past end of file for a very long time.  The recent change just made it visible in cases where the bss section got its own PT_LOAD header (due to alignment).
Comment 2 Alan Modra 2011-06-23 00:29:02 UTC
No, that last comment was wrong.  It wasn't sh_offset but header p_offset that could be past end of file.
Comment 3 Jakub Jelinek 2011-06-23 07:32:34 UTC
The intention of prelink --undo is that it reverts the binary/shared library to the original, bitwise, content.  During prelinking the binary/shared library usually grows, and on undo prelink feeds the saved ElfXX_Shdr/ElfXX_Phdr etc. content to libelf to restore the file.  And, apparently sh_offset beyond end of file mean at least for elfutils libelf that it pads with zeros up to that spot.

Of course one could probably tweak libelf (haven't tried the other implementation how it behaves nor looked how hard would it be), but the question is why is such an sh_offset desirable.  When the section has no content in the file (SHT_NOBITS or zero sized section), why can't sh_offset simply be what it used to (either equal to sh_offset of the next section, or of last section + its in the file size)?
Comment 4 H.J. Lu 2011-06-23 13:41:29 UTC
(In reply to comment #3)
> The intention of prelink --undo is that it reverts the binary/shared library to
> the original, bitwise, content.  During prelinking the binary/shared library
> usually grows, and on undo prelink feeds the saved ElfXX_Shdr/ElfXX_Phdr etc.
> content to libelf to restore the file.  And, apparently sh_offset beyond end of
> file mean at least for elfutils libelf that it pads with zeros up to that spot.
> 

It is too bad that we didn't have a testcase binutils so that we would
have prevented it from happening.  When this bug is fixed, we should also
add a testcase.
Comment 5 Jakub Jelinek 2011-06-23 13:55:28 UTC
Sure, at least it got caught by prelink testsuite.
Comment 6 Alan Modra 2011-06-23 15:08:03 UTC
Actually older binutils do give cases of sh_offset past end of file. ld/testsuite/ld-elf/tbss3.s assembled and linked with older binutils gives .tbss with sh_offset past end of file.  The same file s/tbss/bss/ and correct .bss section flags and you'll see the same thing with .bss.

Now, if I revert the elf.c part of http://sourceware.org/ml/binutils/2011-05/msg00299.html you'll have what you want for your testcase, but then in the tbss3.s case .tbss will have sh_offset of zero and PT_TLS segment will have p_offset of zero.  Not that there is anything really wrong with a zero for sh_offset of SHT_NOBITS sections or zero p_offset for zero p_filesz headers.  In fact that would be the logical value to choose for a section/header that doesn't have contents in the file.  I wonder what would break if we did that for all such sections and headers?  Probably quite a lot, which is why I was trying to set sh_offset to as unsurprising a value as I thought possible.  I chose such that sh_addr % sh_align == sh_offset % sh_align to make the tbss3 testcase give the same sh_offset and p_offset as older linkers.
Comment 7 Alan Modra 2011-06-23 15:31:32 UTC
Created attachment 5813 [details]
possible fix

I'll run the testsuite overnight with my big list of targets to see how this goes.
Comment 8 H.J. Lu 2011-06-23 15:52:37 UTC
(In reply to comment #7)
> Created attachment 5813 [details]
> possible fix
> 
> I'll run the testsuite overnight with my big list of targets to see how this
> goes.

We also need a testcase for this bug.
Comment 9 cvs-commit@gcc.gnu.org 2011-06-24 03:36:44 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	amodra@sourceware.org	2011-06-24 03:36:40

Modified files:
	bfd            : ChangeLog elf.c 

Log message:
	PR ld/12921
	* elf.c (assign_file_positions_for_load_sections): Don't align
	sh_offset for all SHT_NOBITS sections here, just .tbss sections
	that don't get a PT_LOAD.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/bfd/ChangeLog.diff?cvsroot=src&r1=1.5399&r2=1.5400
http://sourceware.org/cgi-bin/cvsweb.cgi/src/bfd/elf.c.diff?cvsroot=src&r1=1.541&r2=1.542
Comment 10 cvs-commit@gcc.gnu.org 2011-06-24 03:39:35 UTC
CVSROOT:	/cvs/src
Module name:	src
Branch: 	binutils-2_21-branch
Changes by:	amodra@sourceware.org	2011-06-24 03:39:32

Modified files:
	bfd            : ChangeLog elf.c 

Log message:
	PR ld/12921
	* elf.c (assign_file_positions_for_load_sections): Don't align
	sh_offset for all SHT_NOBITS sections here, just .tbss sections
	that don't get a PT_LOAD.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/bfd/ChangeLog.diff?cvsroot=src&only_with_tag=binutils-2_21-branch&r1=1.5180.2.36&r2=1.5180.2.37
http://sourceware.org/cgi-bin/cvsweb.cgi/src/bfd/elf.c.diff?cvsroot=src&only_with_tag=binutils-2_21-branch&r1=1.524.2.3&r2=1.524.2.4
Comment 11 Alan Modra 2011-06-24 03:59:40 UTC
Fixed