[PATCH] D58615: [llvm-objdump] Add `Version Definitions` dumper

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objdump/verdef-many-entries-elf.test:4
+
+# CHECK:      Version definitions:
+# CHECK-NEXT:  1 0x01 0x075bcd15 VERSION_1
----------------
By default FileCheck treats all consecutive whitespace as a single space, both in input and check patterns, unless you specify `--strict-whitespace`. I don't think you need to go down this approach, so I don't think you need a test for so many entries (but it's up to you).


================
Comment at: tools/llvm-objdump/ELFDump.cpp:312
+  uint32_t VerdefIndex = 1;
+  // sh_info contains the number of entries of verdef section.
+  // To make the index column have consistent width, we should
----------------
"entries of verdef section" -> "entries in the SHT_GNU_verdef" section. You have also manually formatted the paragraph, with line breaks that are too early, rather than letting clang-format do it for you.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:316
+  // We assume the entries of verdef is no greater than 99.
+  uint16_t VerdefIndexWidth = (Shdr.sh_info > 9) ? 2 : 1;
+  while (Buf) {
----------------
You are going along the right lines here, by using sh_info, but I don't think you should make any assumptions about the maximum number of entries, except that it won't be greater than 2^32 - 1. Better would be to simply convert sh_info to a string and use string.size() to get the required width.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:329-330
+      if (VerdauxIndex)
+        outs() << ((Shdr.sh_info > 9) ? "                   "
+                                      : "                  ");
+      outs() << StringRef(StrTab.drop_front(Verdaux->vda_name).data()) << '\n';
----------------
Rather than hard-coding the number of spaces here, use `std::string(size, ' ')` to create a string of spaces. You can base the size on `VerdefIndexWidth`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58615