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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:16
+# LLVM-INVALID-LINK-EMPTY:
+# LLVM-INVALID-LINK-NEXT:  warning: '[[FILE]]': invalid section linked to SHT_GNU_versym section with index 1: invalid section index: 25
+# LLVM-INVALID-LINK-NEXT:  ]
----------------
25 -> 255


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:31
+
+## Check that we report a warning when the sh_link field of a SHT_GNU_versym section does not referenc
+## a dynamic symbol table table section.
----------------
referenc -> reference


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:32
+## Check that we report a warning when the sh_link field of a SHT_GNU_versym section does not referenc
+## a dynamic symbol table table section.
+
----------------
Delete the extra "table"


================
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*)
----------------
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.


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:95
+
+## Check we report a warning when SHT_GNU_versym section is not correctly aligned in memory
+
----------------
Nit: missing full stop.


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:123
+
+## Check we report a warning when SHT_GNU_versym section has an invalid entry size.
+
----------------
Here and elsewhere "... when a SHT_GNU_versym..."


================
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:
----------------
What's the trailing "()" for here?


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:151
+
+## Check we report a warning when the number of version entries does not match the number of symbols in the symbols table associated.
+
----------------
in the associated symbol table.


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:183
+
+## Check we can dump a SHT_GNU_versym section when it is linked to a custom dynamic symbols
+## table that is not called ".dynsym".
----------------
dynamic symbols -> dynamic symbol


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:373-374
 
+// For a given section returns a linked symbol table and
+// a string table associated.
+template <class ELFT>
----------------
// Returns the linked symbol table and associated string table for a given section.


================
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 =
----------------
ExpectedType should be an `Elf_Word`, I think.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:451
+        Twine(SymTabOrErr->first.size()) +
+        ") in the symbols table with index " + Twine(Sec->sh_link)));
+
----------------
symbols table -> 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");
----------------
Should this be `for (size_t I = 0, S = Syms.size(); I < S; ++I)`?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5928
+    W.printString("Name", this->dumper()->getFullSymbolName(
+                              &Syms[I], StrTable, true /* IsDynamic */));
   }
----------------
`true /* IsDynamic */` -> `/*IsDynamic=*/true`


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

https://reviews.llvm.org/D71054