Page MenuHomePhabricator

[llvm-readobj/llvm-readelf] - Implement GNU style dumper of the SHT_GNU_verneed section.
ClosedPublic

Authored by grimar on May 28 2019, 4:10 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.May 28 2019, 4:10 AM
grimar updated this revision to Diff 201635.May 28 2019, 4:15 AM
  • Fixed helper function name.
MaskRay added inline comments.May 28 2019, 10:32 PM
test/tools/llvm-readobj/elf-verneed-flags.yaml
1 ↗(On Diff #201635)

delete the flags of ?

104 ↗(On Diff #201635)

Probably elaborate a bit, .dynstr is required by .gnu.version_r

tools/llvm-readobj/ELFDumper.cpp
3436 ↗(On Diff #201635)

Nit: " Addr: " (two spaces before "Addr:")

GNU readelf prints 0x prefix for .gnu.version_d and .gnu.version_r but omits the prefix for .gnu.version... I think the inconsistency may be due to a bug. We may stick with one style :)

Another thing: there is no new line between the text of .gnu.version_r and the text of .gnu.version_d.

Another thing: there is no new line between the text of .gnu.version_r and the text of .gnu.version_d.

I guess you mean between .gnu.version and .gnu.version_r? ( .gnu.version_d is implemented in a different patch).
I'll check.

grimar updated this revision to Diff 201865.May 29 2019, 4:32 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.
test/tools/llvm-readobj/elf-verneed-flags.yaml
1 ↗(On Diff #201635)

I am not sure. We check how we dump the versioning sections in elf-versioninfo.test.
But here my intention was to test how flags are dumped (verNeedFlagToString function logic).
I.e. if I remove the flags of it will probably be not so clear that this test is not a partial duplication of the elf-versioninfo.test.

tools/llvm-readobj/ELFDumper.cpp
3436 ↗(On Diff #201635)

GNU output is:

Version symbols section '.gnu.version' contains 6 entries:
 Addr: 0000000000000000  Offset: 0x000280  Link: 7 (.dynsym)
readelf: Warning: Cannot interpret virtual addresses without program headers.
  000:457f              464c               102               901              
  004:   0 (*local*)       0 (*local*)    

Version definition section '.gnu.version_d' contains 3 entries:
  Addr: 0x0000000000000000  Offset: 0x00028c  Link: 8 (.dynstr)
  000000: Rev: 1  Flags: none  Index: 2  Cnt: 1  Name: VERSION1
  0x001c: Rev: 1  Flags: none  Index: 3  Cnt: 2  Name: VERSION2
  0x0038: Parent 1: VERSION1
readelf: Warning: Invalid vd_next field of 0

Version needs section '.gnu.version_r' contains 2 entries:
 Addr: 0x0000000000000000  Offset: 0x0002cc  Link: 8 (.dynstr)
  000000: Version: 1  File: verneed1.so.0  Cnt: 2
  0x0010:   Name: v1  Flags: none  Version: 4
  0x0020:   Name: v2  Flags: none  Version: 5
  0x0030: Version: 1  File: verneed2.so.0  Cnt: 1
  0x0040:   Name: v3  Flags: none  Version: 6

There are following inconsistencies can be observed:

  1. Number of spaces before Addr. For .gnu.version_d there are 2, for other sections - 1.

So having a single space is a bit more consistent with GNU (2:1) :)

I would probably leave a single space, because this line is kind of prolog(header),
it is different from following entries (and hence can be separated).

  1. 0x prefix, yes. As you mentioned, GNU omits it for .gnu.version, but not only that. It also does

it for the first entries. IMO looks ugly and crazy inconsistent. So I am always printing it.

MaskRay accepted this revision.May 29 2019, 6:09 AM
MaskRay added inline comments.
tools/llvm-readobj/ELFDumper.cpp
3436 ↗(On Diff #201635)

1 space looks good to me.

I didn't know the GNU output is so bad until you printed it out.. Pick the style that is aesthetically appealing then :)

000000 -> 0x0000 I believe this is also a GNU readelf bug.

This revision is now accepted and ready to land.May 29 2019, 6:09 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 3:12 AM
MaskRay added inline comments.May 30 2019, 3:20 AM
tools/llvm-readobj/ELFDumper.cpp
3436 ↗(On Diff #201635)