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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:1041
     if (Verneed->vn_file < StrTab.size())
-      VN.File = std::string(StrTab.drop_front(Verneed->vn_file));
+      VN.File = std::string(StrTab.data() + Verneed->vn_file);
     else
----------------
jhenderson wrote:
> Should this be a separate patch?
Without this change llvm-objdump -p output may contain `\0` separated strings, which are incorrect.

The only other use `llvm/tools/llvm-readobj/ELFDumper.cpp:4706` avoids the problem by using `VN.File.data()`.


================
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
+
----------------
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.


================
Comment at: llvm/tools/llvm-objdump/ELFDump.cpp:301
 
-    const uint8_t *BufAux = Buf + Verneed->vn_aux;
-    while (BufAux) {
-      auto *Vernaux = reinterpret_cast<const typename ELFT::Vernaux *>(BufAux);
-      outs() << "    "
-             << format("0x%08" PRIx32 " ", (uint32_t)Vernaux->vna_hash)
-             << format("0x%02" PRIx16 " ", (uint16_t)Vernaux->vna_flags)
-             << format("%02" PRIu16 " ", (uint16_t)Vernaux->vna_other)
-             << StringRef(StrTab.drop_front(Vernaux->vna_name).data()) << '\n';
-      BufAux = Vernaux->vna_next ? BufAux + Vernaux->vna_next : nullptr;
-    }
-    Buf = Verneed->vn_next ? Buf + Verneed->vn_next : nullptr;
+  raw_fd_ostream &OS = outs();
+  for (const VerNeed &VN : *V) {
----------------
jhenderson wrote:
> Nit: this local variable seems a little bit unnecessary?
It avoids repeated outs() call at the cost of one extra line. I think it is worthwhile.


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133751