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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objdump/verneed-wrong-info.test:5
+# We have a SHT_GNU_verneed section with a broken sh_info field
+# that says the section contains more entries that it actually have
+
----------------
grimar wrote:
> that->than, have->has (sorry, did these mistakes/mistypes when suggested it).
> 
> I hope @jhenderson will re-check it though :)
> 
> 
Looks good to me 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:
> jhenderson wrote:
> > Higuoxing wrote:
> > > Seems that these codes are redundant, can we simplify this?
> > I'm not sure I follow what you mean?
> I mean, these functions are similar (they all cast `ELF<32|64><BE|LE>` to `ElfObj`), can we simplify these functions?
> 
> ```
> printELFFileHeader
> printELFDynamicSection
> printELFSymbolVersionRefs
> ```
> 
> 
You're welcome to try some sort of refactor, but do it in another patch.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:292
+             << format("0x%02" PRIx16 " ", (uint16_t)Vernaux->vna_flags)
+             << format("%02u"
+                       " ",
----------------
Use PRIu16, not "u" here.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:326
+
+    if (Shdr.sh_type == ELF::SHT_GNU_verneed)
+      printSymbolVersionDependency<ELFT>(*ContentsOrError, *StrTabOrError);
----------------
grimar wrote:
> You do not need this `if`, this condition is always true now.
You don't need this if as the early continue above means that this if will always be true.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54697