[PATCH] D133751: [llvm-objdump] Change printSymbolVersionDependency to use ELFFile API

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

Looks good, thanks!



================
Comment at: llvm/test/tools/llvm-objdump/ELF/verneed-invalid.test:2
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump -p %t 2>&1 | FileCheck %s --check-prefix=BROKEN-AUX -DFILE=%t
+
----------------
MaskRay wrote:
> jhenderson wrote:
> > Similarly, this test seems unrelated to the commit message?
> I need a file to test `reportWarning(toString(V.takeError()), FileName);`
> 
> The change is like rewriting an existing functionality. Personally I think the needed tests should be like as if we do not have the functionality yet.
> The change is like rewriting an existing functionality. Personally I think the needed tests should be like as if we do not have the functionality yet.

Agreed. If anything, I'd have added this as a prerequisite patch, but it's fine as-is. Thanks!


================
Comment at: llvm/tools/llvm-objdump/ELFDump.cpp:352-356
     ArrayRef<uint8_t> Contents =
         unwrapOrError(Elf.getSectionContents(Shdr), FileName);
     const typename ELFT::Shdr *StrTabSec =
         unwrapOrError(Elf.getSection(Shdr.sh_link), FileName);
     StringRef StrTab = unwrapOrError(Elf.getStringTable(*StrTabSec), FileName);
----------------
MaskRay wrote:
> jhenderson wrote:
> > These variables are only used in the `else` part now. Could we move them?
> They can but they aren't so related to this change. We can rewrite printSymbolVersionDefinition to use the llvm/lib/Object API as well then these variables can go away with that change.
Sounds reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133751