[PATCH] D54697: [llvm-objdump] Add `Version References` dumper (PR30241)

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Could you add tests for the cases where a) there is no DT_VERNEEDNUM tag, and b) where it has a value of 0, please.

Could you also point me at some documentation describing the VerNeed structs etc.



================
Comment at: test/tools/llvm-objdump/private-headers-symbol-version.test:8
+CHECK-NEXT:     0x09691a75 0x00 0x02 GLIBC_2.2.5
+
----------------
Nit: Spurious blank line?


================
Comment at: tools/llvm-objdump/ELFDump.cpp:162
+template <class ELFT>
+void printSymbolVersionRef(const ELFFile<ELFT> *Elf, StringRef Filename) {
+
----------------
printSymbolVersionRef -> printSymbolVersionRefs or printSymbolVersionReferences


================
Comment at: tools/llvm-objdump/ELFDump.cpp:164
+
+  typedef ELFFile<ELFT> ELFO;
+  using VerNeed = typename ELFO::Elf_Verneed;
----------------
Why not use `using` here?


================
Comment at: tools/llvm-objdump/ELFDump.cpp:239-259
 void llvm::printELFDynamicSection(const object::ObjectFile *Obj) {
   if (const auto *ELFObj = dyn_cast<ELF32LEObjectFile>(Obj))
     printDynamicSection(ELFObj->getELFFile(), Obj->getFileName());
   else if (const auto *ELFObj = dyn_cast<ELF32BEObjectFile>(Obj))
     printDynamicSection(ELFObj->getELFFile(), Obj->getFileName());
   else if (const auto *ELFObj = dyn_cast<ELF64LEObjectFile>(Obj))
     printDynamicSection(ELFObj->getELFFile(), Obj->getFileName());
----------------
Higuoxing wrote:
> Seems that these codes are redundant, can we simplify this?
I'm not sure I follow what you mean?


================
Comment at: tools/llvm-objdump/ELFDump.cpp:197-198
+  if (GNUVerDepSec) {
+    const uint8_t *VNeed =
+        (const uint8_t *)Elf->base() + GNUVerDepSec->sh_offset;
+    auto SymTabOrErr = Elf->getSection(GNUVerDepSec->sh_link);
----------------
I'm trying to understand the structure of the VerNeed stuff. Is it a variable size block, and hence the need to iterate over bytes rather than struct instances?

Also, prefer reinterpret_cast instead of C-style, here and elsewhere in this function.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:171
+
+  unsigned VerNeedNum = 0;
+  for (const auto &Dyn : *DynamicEntriesOrError) {
----------------
d_val's type is different in 32 bit and 64-bit ELF, so this should be a type large enough to hold either version (i.e. uin64_t).


================
Comment at: tools/llvm-objdump/ELFDump.cpp:179-181
+  if (VerNeedNum == 0)
+    return;
+  outs() << "Version References:\n";
----------------
Does GNU objdump print the header if there is a DT_VERNEEDNUM tag with a value of 0? I feel like it would be reasonable to do so in our case.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:187
+
+  const typename ELFO::Elf_Shdr *GNUVerDepSec = nullptr;
+  for (const typename ELFO::Elf_Shdr &Sec : *SectionsOrError) {
----------------
You can use `ELFT::Elf_Shdr` here and elsewhere directly, and avoid the need for the `typename`.

I'm also not sure what "GNUVerDepSec" means, so that probably needs expanding a bit more (especially the "Dep" part).


================
Comment at: tools/llvm-objdump/ELFDump.cpp:195
+
+  if (GNUVerDepSec) {
+    const uint8_t *VNeed =
----------------
Rather than having this big if statement, do

```
if (GnuVerDepSec != nullptr)
  return;
```


================
Comment at: tools/llvm-objdump/ELFDump.cpp:202
+    const typename ELFO::Elf_Shdr *SymTab = *SymTabOrErr;
+    for (unsigned I = 0; I < VerNeedNum; ++I) {
+      const auto *Need = reinterpret_cast<const VerNeed *>(VNeed);
----------------
unsigned -> uint64_t to match my earlier comment.

Maybe worth replacing `I` with VerNeedIndex or something, and `J` below similarly, just to disambiguate them a bit more.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:208
+             << ":\n";
+      unsigned VernAuxNum = Need->vn_cnt;
+      const uint8_t *VAux = VNeed + Need->vn_aux;
----------------
unsigned is probably the wrong type. Use the type of vn_cnt or just the largest uint*_t type needed.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2224
+    printELFDynamicSection(o);
+    return printELFSymbolVersionRef(o);
   }
----------------
put the return on a new line, since `printELFSymbolVersionRef ` doesn't return anything.


Repository:
  rL LLVM

https://reviews.llvm.org/D54697