[PATCH] D54697: [llvm-objdump] Add `Version References` dumper
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 25 01:51:40 PST 2019
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
More information about the llvm-commits
mailing list