[PATCH] D94907: [llvm-nm][ELF] - Make -D display symbol versions.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

As our downstream toolchain doesn't use symbol versioning, I'm not really familiar enough with this to review the details (and don't have the time to read back up on it currently), so you'll need @MaskRay or someone else to give input too.



================
Comment at: llvm/test/tools/llvm-nm/dynamic.test:131
+
+## In the following cases we check that we report a warning when unable to read symbol version.
+## Check that we still print unversioned symbol names.
----------------



================
Comment at: llvm/test/tools/llvm-nm/dynamic.test:134
+
+## Case 1: check we report a warning when we unable to read symbol versions
+## from a broken SHT_GNU_verdef section. In this case its sh_offset
----------------



================
Comment at: llvm/test/tools/llvm-nm/dynamic.test:154
+## Case 2: check we report a warning when we are unable to read a SHT_GNU_versym section entry.
+## In this case it has a wrong size that is not a multiple of its sh_entsize.
+
----------------
Adding "wrong" here felt wrong :-)


================
Comment at: llvm/test/tools/llvm-nm/dynamic.test:161
+## Case 3: check we report a warning when we are unable to get a vesrion for a SHT_GNU_versym section entry.
+## In this case SHT_GNU_versym section refers to a version index 255 which is missing.
+
----------------



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1718-1719
+  std::vector<std::string> Ret;
+  size_t I = 0;
+  for (BasicSymbolRef Sym : Symbols) {
+    ++I;
----------------
Seems like if you're not using `Sym`, this should be an older style `for` loop?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1788
   if (!(MachO && DyldInfoOnly)) {
+    size_t I = (size_t)-1;
     for (BasicSymbolRef Sym : Symbols) {
----------------
Is the case actually necessary? I'm sure I've seen `size_t X = -1` in other places before without warnings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94907