[llvm-objdump] Add `Version References` dumper

[Higuoxing]

Summary

Add symbol version dumper for #30241

[Higuoxing]

[llvm-objdump] Add Version References dumper (PR30241)

formatted

[Higuoxing]

tools/llvm-objdump/ELFDump.cpp

239–259 ↗

(On Diff #174605)

Seems that these codes are redundant, can we simplify this?

[Higuoxing]

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

Rename some variables

[Higuoxing]

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

[jhenderson]

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.

test/tools/llvm-objdump/private-headers-symbol-version.test

8 ↗

(On Diff #174605)

Nit: Spurious blank line?

tools/llvm-objdump/ELFDump.cpp

171 ↗

(On Diff #174737)

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

179–181 ↗

(On Diff #174737)

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.

187 ↗

(On Diff #174737)

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

195 ↗

(On Diff #174737)

Rather than having this big if statement, do

if (GnuVerDepSec != nullptr)
  return;

202 ↗

(On Diff #174737)

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.

208 ↗

(On Diff #174737)

unsigned is probably the wrong type. Use the type of vn_cnt or just the largest uint*_t type needed.

197–198 ↗

(On Diff #174716)

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.

162 ↗

(On Diff #174605)

printSymbolVersionRef -> printSymbolVersionRefs or printSymbolVersionReferences

164 ↗

(On Diff #174605)

Why not use using here?

239–259 ↗

(On Diff #174605)

I'm not sure I follow what you mean?

tools/llvm-objdump/llvm-objdump.cpp

2224 ↗

(On Diff #174716)

put the return on a new line, since printELFSymbolVersionRef doesn't return anything.

[Higuoxing]

In D54697#1304044, @jhenderson wrote:

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.

Sure, I will add them

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

https://refspecs.linuxfoundation.org/LSB_2.1.0/LSB-Core-generic/LSB-Core-generic/symverdefs.html
https://refspecs.linuxfoundation.org/LSB_2.1.0/LSB-Core-generic/LSB-Core-generic/symverrqmts.html

PS. Thanks for your kind reviewing, I will cover them one by one :)

[Higuoxing]

Updating D54697: [llvm-objdump] Add Version References dumper (PR30241)
Partially refactored some codes

[Higuoxing]

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

delete #include "llvm/Object/ELFTypes.h"

[Higuoxing]

tools/llvm-objdump/ELFDump.cpp

164 ↗

(On Diff #174605)

I use typedef here, just like what they do in other functions.

239–259 ↗

(On Diff #174605)

I mean, these functions are similar (they all cast ELF<32|64><BE|LE> to ElfObj), can we simplify these functions?

printELFFileHeader
printELFDynamicSection
printELFSymbolVersionRefs

[Higuoxing]

tools/llvm-objdump/ELFDump.cpp

187 ↗

(On Diff #174737)

I change it to VerNeedSec

[Higuoxing]

I want to use YAML2Obj to generate the test files, but there are something wrong with that tool. So, I have to write a patch for that. This patch have to be suspended for some time, until that one landed ...

[Higuoxing]

Add dumper for symbol version definitions.

[jhenderson]

Hi @Higuoxing. Is this ready for review again? It is currently marked as WIP/changes planned.

[Higuoxing]

In D54697#1332797, @jhenderson wrote:

Hi @Higuoxing. Is this ready for review again? It is currently marked as WIP/changes planned.

This works, but there are some little refactors needed. Besides, I cannot give enough test cases right now ...

[Higuoxing]

(WIP) This is a backup.

[Higuoxing]

Sorry for not responding on this patch for 2 months. Could anyone review this? Thanks a lot.

[Higuoxing]

update

[Higuoxing]

Updating D54697: [llvm-objdump] Add Version References dumper

[jhenderson]

I'll take a look at this next week, if nobody else does in the meantime.

[Higuoxing]

Updating D54697: [llvm-objdump] Add Version References dumper

[grimar]

I did not see this patch earlier and so worked on something very close to this for some time. I stopped my work
and shared the ideas/code I have in the comments here :)

tools/llvm-objdump/ELFDump.cpp

288 ↗

(On Diff #187942)

If you move this loop to printSymbolVersionInfo and change the printSymbolVersionDependency signature
to printSymbolVersionDependency(const typename ELFT::Shdr &Shdr, ArrayRef<uint8_t> Contents)
that probably will be better, because for printSymbolVersionDefinition implementation you would also
need to iterate over all sections and take the content. That can be done in a single place.

I mean the code in printSymbolVersionInfo can be like:

template <class ELFT>
static void printSymbolVersionInfo(const ELFFile<ELFT> *Elf) {
  typedef typename ELFT::Shdr Elf_Shdr;

  auto SecOrErr = Elf->sections();
  if (!SecOrErr)
    report_fatal_error(errorToErrorCode(SecOrErr.takeError()).message());

  for (const Elf_Shdr &Shdr : *SecOrErr) {
    if (Shdr.sh_type != ELF::SHT_GNU_verdef &&
        Shdr.sh_type != ELF::SHT_GNU_verneed)
      continue;

    auto ContentsOrErr = Elf->getSectionContents(&Shdr);
    if (!ContentsOrErr)
      report_fatal_error(errorToErrorCode(ContentsOrErr.takeError()).message());

    if (Shdr.sh_type == ELF::SHT_GNU_verdef)
      printSymbolVersionDefinition<ELFT>(Shdr, *ContentsOrErr);
    else if (Shdr.sh_type == ELF::SHT_GNU_verneed)
      printSymbolVersionDependency<ELFT>(Shdr, *ContentsOrErr);
  }
}

294 ↗

(On Diff #187942)

Actually, you do not need this I think.
You can simply iterate over Elf_Verneed entries until vn_next != 0.

300 ↗

(On Diff #187942)

I would use getSectionContents (see sample above), because it does more checks.

316 ↗

(On Diff #187942)

The same, I think, you can simply iterate until vna_next != 0.

[grimar]

test/tools/llvm-objdump/verneed-wrong-info.test

5 ↗

(On Diff #187942)

The case looks simple, so I would remove the link and just describe it.
i.e. something like:

"We have a SHT_GNU_verneed section with a broken sh_info field that says the section
contains more entries that it actually have"

[Higuoxing]

tools/llvm-objdump/ELFDump.cpp

288 ↗

(On Diff #187942)

Oh, thank you @grimar , I am wandering that whether the dumping order of verneed and verdef sections is important? Sometimes, the verneed section is before verdef and sometimes verneed is after verdef.

[Higuoxing]

I found that elf2yaml codes could be used here, so I just do some copy and paste work.

I don't know if we need some checks on sh_info field and DT_VERNEED, just like what gnu-objdump do.

[grimar]

test/tools/llvm-objdump/verneed-wrong-info.test

5 ↗

(On Diff #188074)

that->than, have->has (sorry, did these mistakes/mistypes when suggested it).

I hope @jhenderson will re-check it though :)

tools/llvm-objdump/ELFDump.cpp

303 ↗

(On Diff #188074)

Shouldn't you be able to get Filename from Elf?

313 ↗

(On Diff #188074)

If you're not going to implement SHT_GNU_verneed in this patch then I think you should
get rid of Shdr.sh_type != ELF::SHT_GNU_verneed.

288 ↗

(On Diff #187942)

I did not notice that, but if so then I guess GNU objdump just print them in the same order as they are in a section table probably.
i.e. the same is done in this patch.

[grimar]

tools/llvm-objdump/ELFDump.cpp

313 ↗

(On Diff #188074)

I meant "if you're not going to implement ELF::SHT_GNU_verdef" of course..

[Higuoxing]

Updating D54697: [llvm-objdump] Add Version References dumper

[Higuoxing]

tools/llvm-objdump/ELFDump.cpp

303 ↗

(On Diff #188074)

Sorry, I cannot find a getFileName() method in Elf or something else like that.

313 ↗

(On Diff #188074)

Thanks for reviewing, I would like to implement it in next patch :)

[grimar]

This looks fine, thanks! (with a minor nit addressed)
Let's see if anyone else has comments.

tools/llvm-objdump/ELFDump.cpp

326 ↗

(On Diff #188105)

You do not need this if, this condition is always true now.

303 ↗

(On Diff #188074)

Yeah, I see now. Nevermind, I looked at the wrong class I think.

[jhenderson]

test/tools/llvm-objdump/verneed-wrong-info.test

5 ↗

(On Diff #188074)

Looks good to me here :)

tools/llvm-objdump/ELFDump.cpp

292 ↗

(On Diff #188105)

Use PRIu16, not "u" here.

326 ↗

(On Diff #188105)

You don't need this if as the early continue above means that this if will always be true.

239–259 ↗

(On Diff #174605)

You're welcome to try some sort of refactor, but do it in another patch.

[Higuoxing]

Updating D54697: [llvm-objdump] Add Version References dumper

[Higuoxing]

Thanks a lot!

[jhenderson]

LGTM, but please wait for @grimar to confirm he is happy.

[grimar]

LGTM.

llvm/trunk/test/tools/llvm-objdump/verneed-elf.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objdump -p %t | FileCheck %s
+
+# CHECK:      Version References:
+# CHECK-NEXT:   required from dso.so.0:
+# CHECK-NEXT:     0x000004d2 0x0a 03 v1
+# CHECK-NEXT:     0x0000162e 0x0b 04 v2
+# CHECK-NEXT:   required from dso.so.1:
+# CHECK-NEXT:     0x000011d7 0x0c 02 v3
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x0000000000201000
+Sections:
+  - Name:            .gnu.version_r
+    Type:            SHT_GNU_verneed
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x0000000000200250
+    Link:            .dynstr
+    AddressAlign:    0x0000000000000004
+    Info:            0x0000000000000002
+    Dependencies:
+      - Version:         1
+        File:            dso.so.0
+        Entries:
+          - Name:            v1
+            Hash:            1234
+            Flags:           10
+            Other:           3
+          - Name:            v2
+            Hash:            5678
+            Flags:           11
+            Other:           4
+      - Version:         1
+        File:            dso.so.1
+        Entries:
+          - Name:            v3
+            Hash:            4567
+            Flags:           12
+            Other:           2
+DynamicSymbols:
+  Global:
+    - Name:            f1

llvm/trunk/test/tools/llvm-objdump/verneed-wrong-info.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objdump -p %t 2>&1 | FileCheck %s
+
+# We have a SHT_GNU_verneed section with a broken sh_info field
+# that says the section contains more entries than it actually has.
+
+# CHECK:      Version References:
+# CHECK-NEXT:   required from dso.so.0:
+# CHECK-NEXT:     0x000004d2 0x0a 03 v1
+# CHECK-NEXT:     0x0000162e 0x0b 04 v2
+# CHECK-NEXT:   required from dso.so.1:
+# CHECK-NEXT:     0x000011d7 0x0c 02 v3
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x0000000000201000
+Sections:
+  - Name:            .gnu.version_r
+    Type:            SHT_GNU_verneed
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x0000000000200250
+    Link:            .dynstr
+    AddressAlign:    0x0000000000000004
+    Info:            0x0000000000009999
+    Dependencies:
+      - Version:         1
+        File:            dso.so.0
+        Entries:
+          - Name:            v1
+            Hash:            1234
+            Flags:           10
+            Other:           3
+          - Name:            v2
+            Hash:            5678
+            Flags:           11
+            Other:           4
+      - Version:         1
+        File:            dso.so.1
+        Entries:
+          - Name:            v3
+            Hash:            4567
+            Flags:           12
+            Other:           2
+DynamicSymbols:
+  Global:
+    - Name:            f1

llvm/trunk/tools/llvm-objdump/ELFDump.cpp

outs() << "off    " << format(Fmt, (uint64_t)Phdr.p_offset) << "vaddr "
            << "memsz " << format(Fmt, (uint64_t)Phdr.p_memsz) << "flags "
            << ((Phdr.p_flags & ELF::PF_R) ? "r" : "-")
            << ((Phdr.p_flags & ELF::PF_W) ? "w" : "-")
            << ((Phdr.p_flags & ELF::PF_X) ? "x" : "-") << "\n";
   }
   outs() << "\n";
 }
 
+template <class ELFT>
+void printSymbolVersionDependency(ArrayRef<uint8_t> Contents,
+                                  StringRef StrTab) {
+  typedef ELFFile<ELFT> ELFO;
+  typedef typename ELFO::Elf_Verneed Elf_Verneed;
+  typedef typename ELFO::Elf_Vernaux Elf_Vernaux;
+
+  outs() << "Version References:\n";
+
+  const uint8_t *Buf = Contents.data();
+  while (Buf) {
+    const Elf_Verneed *Verneed = reinterpret_cast<const Elf_Verneed *>(Buf);
+    outs() << "  required from "
+           << StringRef(StrTab.drop_front(Verneed->vn_file).data()) << ":\n";
+
+    const uint8_t *BufAux = Buf + Verneed->vn_aux;
+    while (BufAux) {
+      const Elf_Vernaux *Vernaux =
+          reinterpret_cast<const Elf_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;
+  }
+}
+
+template <class ELFT>
+void printSymbolVersionInfo(const ELFFile<ELFT> *Elf, StringRef FileName) {
+  typedef typename ELFT::Shdr Elf_Shdr;
+
+  auto SectionsOrError = Elf->sections();
+  if (!SectionsOrError)
+    report_error(FileName, SectionsOrError.takeError());
+
+  for (const Elf_Shdr &Shdr : *SectionsOrError) {
+    if (Shdr.sh_type != ELF::SHT_GNU_verneed)
+      continue;
+
+    auto ContentsOrError = Elf->getSectionContents(&Shdr);
+    if (!ContentsOrError)
+      report_error(FileName, ContentsOrError.takeError());
+
+    auto StrTabSecOrError = Elf->getSection(Shdr.sh_link);
+    if (!StrTabSecOrError)
+      report_error(FileName, StrTabSecOrError.takeError());
+
+    auto StrTabOrError = Elf->getStringTable(*StrTabSecOrError);
+    if (!StrTabOrError)
+      report_error(FileName, StrTabOrError.takeError());
+
+    printSymbolVersionDependency<ELFT>(*ContentsOrError, *StrTabOrError);
+    // TODO: Implement symbol version definitions dumper.
+  }
+}
+
 void llvm::printELFFileHeader(const object::ObjectFile *Obj) {
   if (const auto *ELFObj = dyn_cast<ELF32LEObjectFile>(Obj))
     printProgramHeaders(ELFObj->getELFFile());
   else if (const auto *ELFObj = dyn_cast<ELF32BEObjectFile>(Obj))
     printProgramHeaders(ELFObj->getELFFile());
   else if (const auto *ELFObj = dyn_cast<ELF64LEObjectFile>(Obj))
     printProgramHeaders(ELFObj->getELFFile());
   else if (const auto *ELFObj = dyn_cast<ELF64BEObjectFile>(Obj))
     printProgramHeaders(ELFObj->getELFFile());
 }
 
 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());
   else if (const auto *ELFObj = dyn_cast<ELF64BEObjectFile>(Obj))
     printDynamicSection(ELFObj->getELFFile(), Obj->getFileName());
 }
+
+void llvm::printELFSymbolVersionInfo(const object::ObjectFile *Obj) {
+  if (const auto *ELFObj = dyn_cast<ELF32LEObjectFile>(Obj))
+    printSymbolVersionInfo(ELFObj->getELFFile(), Obj->getFileName());
+  else if (const auto *ELFObj = dyn_cast<ELF32BEObjectFile>(Obj))
+    printSymbolVersionInfo(ELFObj->getELFFile(), Obj->getFileName());
+  else if (const auto *ELFObj = dyn_cast<ELF64LEObjectFile>(Obj))
+    printSymbolVersionInfo(ELFObj->getELFFile(), Obj->getFileName());
+  else if (const auto *ELFObj = dyn_cast<ELF64BEObjectFile>(Obj))
+    printSymbolVersionInfo(ELFObj->getELFFile(), Obj->getFileName());
+}

llvm/trunk/tools/llvm-objdump/llvm-objdump.h

 void printMachOUnwindInfo(const object::MachOObjectFile *O);
 void printMachOExportsTrie(const object::MachOObjectFile *O);
 void printMachORebaseTable(object::MachOObjectFile *O);
 void printMachOBindTable(object::MachOObjectFile *O);
 void printMachOLazyBindTable(object::MachOObjectFile *O);
 void printMachOWeakBindTable(object::MachOObjectFile *O);
 void printELFFileHeader(const object::ObjectFile *O);
 void printELFDynamicSection(const object::ObjectFile *Obj);
+void printELFSymbolVersionInfo(const object::ObjectFile *Obj);
 void printCOFFFileHeader(const object::ObjectFile *O);
 void printCOFFSymbolTable(const object::COFFImportFile *I);
 void printCOFFSymbolTable(const object::COFFObjectFile *O);
 void printMachOFileHeader(const object::ObjectFile *O);
 void printMachOLoadCommands(const object::ObjectFile *O);
 void printWasmFileHeader(const object::ObjectFile *O);
 void printExportsTrie(const object::ObjectFile *O);
 void printRebaseTable(object::ObjectFile *O);

llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp

FaultMapParser FMP(FaultMapContents.bytes_begin(),
                      FaultMapContents.bytes_end());
 
   outs() << FMP;
 }
 
 static void printPrivateFileHeaders(const ObjectFile *O, bool OnlyFirst) {
   if (O->isELF()) {
     printELFFileHeader(O);
-    return printELFDynamicSection(O);
+    printELFDynamicSection(O);
+    printELFSymbolVersionInfo(O);
+    return;
   }
   if (O->isCOFF())
     return printCOFFFileHeader(O);
   if (O->isWasm())
     return printWasmFileHeader(O);
   if (O->isMachO()) {
     printMachOFileHeader(O);
     if (!OnlyFirst)