Bug 24249

Summary: Section offsets not monotonically increasing
Product: dwz Reporter: Tom de Vries <vries>
Component: defaultAssignee: Nobody <nobody>
Status: RESOLVED FIXED    
Severity: normal CC: dwz, jakub, mark
Priority: P2    
Version: unspecified   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed:
Attachments: dwz-0.12-ignore-nobits.patch
Tentative patch

Description Tom de Vries 2019-02-20 09:44:11 UTC
For a hello world source file hello.c, using the gold linker does not cause trouble for dwz:
...
$ gcc hello.c -g -fuse-ld=gold
$ dwz a.out 
...

And adding the .gdb_index section using gdb-add-index also works fine with dwz:
...
$ gcc hello.c -g -fuse-ld=gold
$ gdb-add-index a.out
$ dwz a.out 
...

But, I get this error when using the gold linker to generate the .gdb_index section:
...
$ gcc hello.c -g -fuse-ld=gold -Wl,--gdb-index
$ dwz a.out 
dwz: Section offsets in a.out not monotonically increasing
...

Inspecting the section offsets around .gdb_index:
...
$ readelf -S a.out | grep '[[]' | egrep -C1  'Offset|\.gdb_index'
  [Nr] Name              Type             Address           Offset
  [ 0]                   NULL             0000000000000000  00000000
--
  [31] .debug_str        PROGBITS         0000000000000000  00001b2f
  [32] .gdb_index        PROGBITS         0000000000000000  000034b0
  [33] .debug_ranges     PROGBITS         0000000000000000  000021f0
...
indeed shows that the offsets are not monotonically increasing.
Comment 1 Tom de Vries 2019-02-20 09:48:03 UTC
In related PR21191 - "objcopy --only-keep-debug creates non-monotonically increasing section offsets", it's also dwz that complains.
Comment 2 Jakub Jelinek 2019-02-20 09:50:06 UTC
That would be a gold bug then.
Comment 3 Tom de Vries 2019-02-20 09:56:01 UTC
Created attachment 11629 [details]
dwz-0.12-ignore-nobits.patch

Using this ( https://build.opensuse.org/package/view_file/openSUSE:Factory/dwz/dwz-0.12-ignore-nobits.patch ) patch, we get:
...
$ gcc hello.c -g -fuse-ld=gold -Wl,--gdb-index
$ dwz a.out 
$ readelf -S a.out | grep '[[]' | egrep -C1  'Offset|\.gdb_index'
  [Nr] Name              Type             Address           Offset
  [ 0]                   NULL             0000000000000000  00000000
--
  [31] .debug_str        PROGBITS         0000000000000000  00001a11
  [32] .gdb_index        PROGBITS         0000000000000000  00003390
  [33] .debug_ranges     PROGBITS         0000000000000000  000020d0
...

So, the patch allows dwz to handle the input. Note that the section offsets in the dwz output are still not monotonically increasing.
Comment 4 Tom de Vries 2019-02-20 10:18:49 UTC
(In reply to Jakub Jelinek from comment #2)
> That would be a gold bug then.

I read here ( rbc#1366145 - "dwz applied to a dts-compiled binary complains about section offsets not monotonically increasing",  https://bugzilla.redhat.com/show_bug.cgi?id=1366145 ):
...
 The .shstrtab section would appear in the middle of the section header table, but at the end of the actual file. This meant that the file offsets of the entries in the section header table were no longer in a monotonically increasing order. Whilst this property is not a requirement of the ELF standard, it is expected by a variety of other tools which process ELF binaries.
...

I suppose this is the rationale behind classifying this as gold bug.
Comment 5 Jakub Jelinek 2019-02-20 10:34:40 UTC
Pedantically, ELF doesn't require that.  But I view it as a QoI thing, there is no reason for a sane ELF producer to just put sections randomly in .shstrtab, sorting them by increasing .sh_offset is most reasonable.  And, both prelink and dwz and other tools just choose not to optimize if the producer didn't do a reasonable job.
Comment 6 Tom de Vries 2019-02-20 10:43:33 UTC
(In reply to Jakub Jelinek from comment #2)
> That would be a gold bug then.

Refiled as PR24250 - "[gold] Section offsets not monotonically increasing with --gdb-index"
Comment 7 Tom de Vries 2019-02-20 17:13:51 UTC
Created attachment 11635 [details]
Tentative patch

(In reply to Jakub Jelinek from comment #5)
> Pedantically, ELF doesn't require that.  But I view it as a QoI thing, there
> is no reason for a sane ELF producer to just put sections randomly in
> .shstrtab, sorting them by increasing .sh_offset is most reasonable.  And,
> both prelink and dwz and other tools just choose not to optimize if the
> producer didn't do a reasonable job.

I think it's reasonable to draw the line somewhere, so I'd understand it if this gets marked resolved-wontfix.

OTOH, I'm not happy with not having a workaround (the ability to run say an elf sanitizer or some such to fix this trivial problem to allow dwz to run on the sanitized elf).

This tentative patch fixes up the bad-quality input in fdopen_dso in a fairly orthogonal way, by sorting dso->shdr and dso->scn according to sh_offset, and updating dso->ehdr.e_shstrndx.
...
$ gcc hello.c -g -fuse-ld=gold -Wl,--gdb-index
$ dwz a.out
dwz: Section offsets in a.out not monotonically increasing, fixing up
$ echo $?
0
$ readelf -S a.out
Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  ...
  [37] .shstrtab         STRTAB           0000000000000000  0000284f
       000000000000017f  0000000000000000           0     0     1
  [38] .gdb_index        PROGBITS         0000000000000000  00003390
       00000000000022f3  0000000000000000           0     0     4
...
Comment 8 Tom de Vries 2019-02-22 10:41:26 UTC
(In reply to Tom de Vries from comment #7)
> This tentative patch fixes up the bad-quality input in fdopen_dso in a
> fairly orthogonal way, by sorting dso->shdr and dso->scn according to
> sh_offset, and updating dso->ehdr.e_shstrndx.

Review remark from Micha: If this patch changes order of the section header table, then it should also update the section index member (st_shndx) of symbol table entries.
Comment 9 Jakub Jelinek 2019-02-22 10:45:41 UTC
I still have concerns, e.g. if we process *.debug files stripped into file by objcopy or eu-strip, won't this shdr reordering then break the matching between original binary and *.debug file?
On the other side, this code has been written primarily for prelink, when dwz only modifies debug sections perhaps it could away with relying on this or only care (and reshuffle) the non-allocated sections that way and ignore others.
Comment 10 Tom de Vries 2019-02-22 12:42:35 UTC
(In reply to Jakub Jelinek from comment #9)
> I still have concerns,

FWIW, dwz-0.12-ignore-nobits.patch handles this PR without reordering of the section table, so from that point of view it's the easier solution.

> e.g. if we process *.debug files stripped into file
> by objcopy or eu-strip, won't this shdr reordering then break the matching
> between original binary and *.debug file?

Indeed the ordering of entries in the section table in the original binary and the .debug file will not be the same.

I'm not aware though of a tool that relies on this ordering to be the same.

> On the other side, this code has been written primarily for prelink, when
> dwz only modifies debug sections perhaps it could away with relying on this

Sorry, not sure what you mean here.

> or only care (and reshuffle) the non-allocated sections that way and ignore
> others.

Agreed, that would work for this example.


I start to wonder now whether a mix of the two posted approaches (dwz-0.12-ignore-nobits.patch and the tentative patch) would be the best solution:
1. resort the section header based on offset (ignoring nobits offsets, see PR24251)
2. recalculate the offsets base on the new sizes (in write_dso)
3. unsort the sections back into the original order.
This approach would have the same result as dwz-0.12-ignore-nobits.patch, and avoid the problem of having to rewrite symbol tables, but would be less intrusive to write_dso than dwz-0.12-ignore-nobits.patch. WDYT?
Comment 11 Mark Wielaard 2019-02-22 13:40:16 UTC
(In reply to Tom de Vries from comment #10)
> (In reply to Jakub Jelinek from comment #9)
> > e.g. if we process *.debug files stripped into file
> > by objcopy or eu-strip, won't this shdr reordering then break the matching
> > between original binary and *.debug file?
> 
> Indeed the ordering of entries in the section table in the original binary
> and the .debug file will not be the same.
> 
> I'm not aware though of a tool that relies on this ordering to be the same.

I think eu-unstrip does deal with differences in ordering, but you might want to test whether it still works correctly.
Comment 12 Tom de Vries 2019-03-12 17:09:03 UTC
patch posted: https://sourceware.org/ml/dwz/2019-q1/msg00108.html