[PATCH] D71054: [llvm-readobj][llvm-readelf] - Refactor parsing of the SHT_GNU_versym section.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:72
+# INVALID-STRING-TABLE-GNU-EMPTY: 
+# INVALID-STRING-TABLE-GNU-NEXT:   warning: '[[FILE]]': can't get a string table for the symbol table linked to SHT_GNU_versym section with index 1: invalid string table linked to SHT_DYNSYM section with index 5: invalid sh_type for string table section [index 2]: expected SHT_STRTAB, but got SHT_NULL
+# INVALID-STRING-TABLE-GNU-NEXT:   000:   0 (*local*)
----------------
jhenderson wrote:
> I'm not asking for a change, but I'm noting that this warning message is getting a bit silly in length! I don't have any better suggestion though without losing context.
Yeah, but this is a result of a multiple messages concatenations. It is kind of by design.


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:130
+# INVALID-ENT-SIZE-GNU:       Version symbols section '.gnu.version' contains 1 entries:
+# INVALID-ENT-SIZE-GNU-NEXT:   Addr: 0000000000000000  Offset: 0x000040  Link: 0 ()
+# INVALID-ENT-SIZE-GNU-EMPTY:
----------------
jhenderson wrote:
> What's the trailing "()" for here?
It usually contains a section name. E.g.:

`Addr: 0000000000000000  Offset: 0x000040  Link: 4 (.dynsym)`

Here the sh_link==0 hence we see no name. Doesn't seem to be a bug problem? (since having sh_link==0 is ubnormal).



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:378
+getLinkAsSymtab(const ELFFile<ELFT> *Obj, const typename ELFT::Shdr *Sec,
+                   unsigned SecNdx, unsigned ExpectedType) {
+  Expected<const typename ELFT::Shdr *> SymtabOrErr =
----------------
jhenderson wrote:
> ExpectedType should be an `Elf_Word`, I think.
Probably not. It is expected to accept an enum value which is unsigned:
(https://github.com/llvm-mirror/llvm/blob/master/include/llvm/BinaryFormat/ELF.h#L815)

```
enum : unsigned {
...
  SHT_DYNSYM = 11,                      // Symbol table.
```


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5924
 
-  // Same number of entries in the dynamic symbol table (DT_SYMTAB).
-  for (const Elf_Sym &Sym : Dumper->dynamic_symbols()) {
+  for (size_t I = 0; I < Syms.size(); ++I) {
     DictScope S(W, "Symbol");
----------------
jhenderson wrote:
> Should this be `for (size_t I = 0, S = Syms.size(); I < S; ++I)`?
Done.


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

https://reviews.llvm.org/D71054