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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/llvm-readobj/elf-verneed-flags.yaml:1
+## Check how llvm-readobj/llvm-readelf tools dump the flags of SHT_GNU_verneed
+## section entries.
----------------
MaskRay wrote:
> delete `the flags of `?
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`.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3436
   StringRef SymTabName = unwrapOrError(Obj->getSectionName(SymTab));
   OS << " Addr: " << format_hex_no_prefix(Sec->sh_addr, 16)
      << "  Offset: " << format_hex(Sec->sh_offset, 8)
----------------
MaskRay wrote:
> 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 :)
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).

2) `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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62516/new/

https://reviews.llvm.org/D62516