[PATCH] D71118: [llvm-readelf/llvm-readobj] - Improved the error reporting in a few method related to versioning.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/elf-verdef-invalid.test:266
 
-## Check we error out when trying to print version symbols, but SHT_GNU_verdef is invalid due to any reason.
+## Check we report out when trying to print version symbols, but SHT_GNU_verdef is invalid due to any reason.
 
----------------
"we report when..."

This line is quite long. Maybe it should be split.


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:222
+
+## Version index in a SHT_GNU_versym sections overflows the version map.
+## Check we report it when trying to dump dynamic symbols.
----------------
sections -> section


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:226
+# RUN: yaml2obj %s --docnum=8 -o %t8
+# RUN: llvm-readobj -dt %t8 2>&1 | FileCheck -DFILE=%t8 --check-prefix=VERSION-OVERFLOW-LLVM %s
+# RUN: llvm-readelf --dyn-syms %t8 2>&1 | FileCheck -DFILE=%t8 --check-prefix=VERSION-OVERFLOW-GNU %s
----------------
I'd use `--dyn-syms` here instead of `-dt`, since there's no reason for it to be inconsistent with the line below.


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:256
+    Type:    SHT_GNU_versym
+    Entries: [ 0xFF, 0xFE ]
+DynamicSymbols:
----------------
Would it be worth an extra 0xFF entry to show that the warning gets uniqued?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4126
+  std::vector<StringRef> Versions;
+  for (size_t I = 0; I < VerTable.size(); ++I) {
+    unsigned Ndx = VerTable[I].vs_index;
----------------
`for (size_t I = 0, S = VerTable.size(); I < S; ++I)`




================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4136
+        this->dumper()->getSymbolVersionByIndex(Ndx, IsDefault);
+    if (!NameOrErr || NameOrErr->empty()) {
+      if (!NameOrErr) {
----------------
I'm not sure it's right to say that an empty name is corrupt?


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

https://reviews.llvm.org/D71118