[llvm-readobj] - Teach llvm-readobj to print dependencies of SHT_GNU_verdef and refactor dumping method.

[grimar]

Summary

This patch changes single method of llvm-readobj.
It teaches SHT_GNU_verdef dumper to print version dependencies,
also it removes few fields from output that can be dumped with other keys
and slightly refactors code.
Testcase was also modified to match the changes.
Change is required for testcases of upcoming lld patches.

[davide]

tools/llvm-readobj/ELFDumper.cpp

571 ↗

(On Diff #61354)

How many dependencies can you have?
IIRC you can have only one immediate parent/dependency so it's probably easier/more reasonable to only print that, no?
If you can have multiple dependencies, instead, can you please add a test exercising that behaviour?

[• rafael]

tools/llvm-readobj/ELFDumper.cpp

570 ↗

(On Diff #61354)

The "spec" used the name "predecessor versions". Maybe this should be "Predecessors". I assume that if you have

foo {
} bar;

bar {
} zed;

it will chain them?

If that is the case, maybe it would be better to print just the first predecessor?

In any case, I agree with Davide, we need a test :-)

[ruiu]

test/tools/llvm-readobj/elf-versioninfo.test

58 ↗

(On Diff #61354)

Is it a common practice for llvm-dump* tools to print out an empty set {} if an element is missing? An alternative way is to not print out Dependencies {} if a version definition has no dependencies.

[grimar]

test/tools/llvm-readobj/elf-versioninfo.test

58 ↗

(On Diff #61354)

I think it is common, for example we often have relocations empty in testcases:

Relocations [
]

I am going to change this in according to my answers to Rafael and Davide.

tools/llvm-readobj/ELFDumper.cpp

570 ↗

(On Diff #61354)

No, it will not chain. Foo will have [foo, bar], where bar is dependency. Bar will have [bar, zed].
About terminology, I used sun docs: https://docs.oracle.com/cd/E19120-01/open.solaris/819-0690/chapter6-61174/index.html, it says "version dependencies".

And like I wrote in my answer to Davide, I was unable to get more than one "dependency", so looks "predecessor" word is more correct here.
So I plan to use it and print predecessor without a loop.

571 ↗

(On Diff #61354)

I did not found how to get more that one dependency here. I also think that it can be only one now. But spec says: "At least one version dependency must exist. Additional version dependencies can be present, the number being indicated by the vn_cnt value.".
So initially I implemented it in a more generic way, basing on what it says.
I am ok to print only one since it looks to be the only case and it is more "predecessor" than "dependency".

[grimar]

tools/llvm-readobj/ELFDumper.cpp

570 ↗

(On Diff #61354)

Ah, I was mean: "yes, it will chain" zed<-bar<-foo, but will not add zed as a "dependency" for bar.

[grimar]

tools/llvm-readobj/ELFDumper.cpp

570 ↗

(On Diff #61354)

*for foo
sorry :)

[grimar]

• Addressed review comments.

[• rafael]

LGTM with nits.

test/tools/llvm-readobj/elf-versioninfo.test

71 ↗

(On Diff #61513)

Count is redundant, no? It is always 1 or 2 and being 2 is visible by Predecessor being present.

tools/llvm-readobj/ELFDumper.cpp

572 ↗

(On Diff #61513)

Produce an error if is > 2.

[grimar]

tools/llvm-readobj/ELFDumper.cpp

572 ↗

(On Diff #61513)

I do that above :)

[davide]

lg, thanks.

[Higuoxing]

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

569

Hi, I am curious about why the Parents/Predecessor more than one. According to https://akkadia.org/drepper/symbol-versioning

vda_next byte offset to the next Elfxx_Verdaux entry. The first entry (pointed to by the Elfxx_Verdef entry, contains the actual defined name. The second and all later entries name predecessor versions.

Feel free to ignore me :) just out of curiosity

[Higuoxing]

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

569

Besides, gnu-readelf could print multiple parents

[Higuoxing]

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

569

Sorry, I am curious about why the Parents/Predecessor *cannot be* more than one.

You could use my patch to make a simple test, if needed

https://reviews.llvm.org/D54867

feel free to ignore me :)

[grimar]

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

569

Hi! You can find some answers in the comments for this patch, starting from this one:
https://reviews.llvm.org/D21552?id=61354#inline-182645

In short - I was unable to produce the output that has more than one predecessor,
so having the code for that was unuseful and we stopped on this version of code that time.
It still seems reasonable nowadays, I think.

[Higuoxing]

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

569

Oh, sorry for bothering, thank you very much!

llvm/trunk/test/tools/llvm-readobj/elf-versioninfo.test

 CHECK-NEXT:     }
 CHECK-NEXT:     Symbol {
 CHECK-NEXT:       Version: 3
 CHECK-NEXT:       Name: VERSION2@@VERSION2
 CHECK-NEXT:     }
 CHECK-NEXT:   ]
 CHECK-NEXT: }
 
-CHECK: Version definition {
-CHECK-NEXT:   Section Name: .gnu.version_d (70)
-CHECK-NEXT:   Address: 0x25C
-CHECK-NEXT:   Offset: 0x25C
-CHECK-NEXT:   Link: 2
-CHECK-NEXT:   Entries [
-CHECK-NEXT:     Entry {
-CHECK-NEXT:       Offset: 0x0
-CHECK-NEXT:       Rev: 1
-CHECK-NEXT:       Flags: 1
-CHECK-NEXT:       Index: 1
-CHECK-NEXT:       Cnt: 1
-CHECK-NEXT:       Hash: 430712
-CHECK-NEXT:       Name: blah
-CHECK-NEXT:     }
-CHECK-NEXT:     Entry {
-CHECK-NEXT:       Offset: 0x1C
-CHECK-NEXT:       Rev: 1
-CHECK-NEXT:       Flags: 0
-CHECK-NEXT:       Index: 2
-CHECK-NEXT:       Cnt: 1
-CHECK-NEXT:       Hash: 175630257
-CHECK-NEXT:       Name: VERSION1
-CHECK-NEXT:     }
-CHECK-NEXT:     Entry {
-CHECK-NEXT:       Offset: 0x38
-CHECK-NEXT:       Rev: 1
-CHECK-NEXT:       Flags: 0
-CHECK-NEXT:       Index: 3
-CHECK-NEXT:       Cnt: 2
-CHECK-NEXT:       Hash: 175630258
-CHECK-NEXT:       Name: VERSION2
-CHECK-NEXT:     }
-CHECK-NEXT:   ]
+CHECK:      SHT_GNU_verdef {
+CHECK-NEXT:   Definition {
+CHECK-NEXT:     Version: 1
+CHECK-NEXT:     Flags: Base (0x1)
+CHECK-NEXT:     Index: 1
+CHECK-NEXT:     Hash: 430712
+CHECK-NEXT:     Name: blah
+CHECK-NEXT:   }
+CHECK-NEXT:   Definition {
+CHECK-NEXT:     Version: 1
+CHECK-NEXT:     Flags: 0x0
+CHECK-NEXT:     Index: 2
+CHECK-NEXT:     Hash: 175630257
+CHECK-NEXT:     Name: VERSION1
+CHECK-NEXT:   }
+CHECK-NEXT:   Definition {
+CHECK-NEXT:     Version: 1
+CHECK-NEXT:     Flags: 0x0
+CHECK-NEXT:     Index: 3
+CHECK-NEXT:     Hash: 175630258
+CHECK-NEXT:     Name: VERSION2
+CHECK-NEXT:     Predecessor: VERSION1
+CHECK-NEXT:   }
 CHECK-NEXT: }
 
 RUN: llvm-readobj -V %p/Inputs/verneed.elf-x86-64 | FileCheck %s --check-prefix=VERNEED
 
 VERNEED:       SHT_GNU_verneed {
 VERNEED-NEXT:   Dependency {
 VERNEED-NEXT:     Version: 1
 VERNEED-NEXT:     Count: 2

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

static const EnumEntry<unsigned> SymVersionFlags[] = {
     {"Weak", "WEAK", VER_FLG_WEAK},
     {"Info", "INFO", VER_FLG_INFO}};
 
 template <typename ELFO, class ELFT>
 static void printVersionDefinitionSection(ELFDumper<ELFT> *Dumper,
                                           const ELFO *Obj,
                                           const typename ELFO::Elf_Shdr *Sec,
                                           ScopedPrinter &W) {
-  DictScope SD(W, "Version definition");
+  typedef typename ELFO::Elf_Verdef VerDef;
+  typedef typename ELFO::Elf_Verdaux VerdAux;
+
+  DictScope SD(W, "SHT_GNU_verdef");
   if (!Sec)
     return;
-  StringRef Name = unwrapOrError(Obj->getSectionName(Sec));
-  W.printNumber("Section Name", Name, Sec->sh_name);
-  W.printHex("Address", Sec->sh_addr);
-  W.printHex("Offset", Sec->sh_offset);
-  W.printNumber("Link", Sec->sh_link);
 
-  unsigned verdef_entries = 0;
   // The number of entries in the section SHT_GNU_verdef
   // is determined by DT_VERDEFNUM tag.
+  unsigned VerDefsNum = 0;
   for (const typename ELFO::Elf_Dyn &Dyn : Dumper->dynamic_table()) {
     if (Dyn.d_tag == DT_VERDEFNUM)
-      verdef_entries = Dyn.d_un.d_val;
+      VerDefsNum = Dyn.d_un.d_val;
   }
   const uint8_t *SecStartAddress =
       (const uint8_t *)Obj->base() + Sec->sh_offset;
   const uint8_t *SecEndAddress = SecStartAddress + Sec->sh_size;
   const uint8_t *P = SecStartAddress;
   const typename ELFO::Elf_Shdr *StrTab =
       unwrapOrError(Obj->getSection(Sec->sh_link));
 
-  ListScope Entries(W, "Entries");
-  for (unsigned i = 0; i < verdef_entries; ++i) {
-    if (P + sizeof(typename ELFO::Elf_Verdef) > SecEndAddress)
+  while (VerDefsNum--) {
+    if (P + sizeof(VerDef) > SecEndAddress)
       report_fatal_error("invalid offset in the section");
-    auto *VD = reinterpret_cast<const typename ELFO::Elf_Verdef *>(P);
-    DictScope Entry(W, "Entry");
-    W.printHex("Offset", (uintptr_t)P - (uintptr_t)SecStartAddress);
-    W.printNumber("Rev", VD->vd_version);
-    // FIXME: print something more readable.
-    W.printNumber("Flags", VD->vd_flags);
+
+    auto *VD = reinterpret_cast<const VerDef *>(P);
+    DictScope Def(W, "Definition");
+    W.printNumber("Version", VD->vd_version);
+    W.printEnum("Flags", VD->vd_flags, makeArrayRef(SymVersionFlags));
     W.printNumber("Index", VD->vd_ndx);
-    W.printNumber("Cnt", VD->vd_cnt);
     W.printNumber("Hash", VD->vd_hash);
     W.printString("Name",
                   StringRef((const char *)(Obj->base() + StrTab->sh_offset +
                                            VD->getAux()->vda_name)));
+    if (!VD->vd_cnt)
+      report_fatal_error("at least one definition string must exist");
+    if (VD->vd_cnt > 2)
+      report_fatal_error("more than one predecessor is not expected");
+
+    if (VD->vd_cnt == 2) {
+      const uint8_t *PAux = P + VD->vd_aux + VD->getAux()->vda_next;
+      const VerdAux *Aux = reinterpret_cast<const VerdAux *>(PAux);
+      W.printString("Predecessor",
+                    StringRef((const char *)(Obj->base() + StrTab->sh_offset +
+                                             Aux->vda_name)));
+    }
+
     P += VD->vd_next;
   }
 }
 
 template <typename ELFO, class ELFT>
 static void printVersionDependencySection(ELFDumper<ELFT> *Dumper,
                                           const ELFO *Obj,
                                           const typename ELFO::Elf_Shdr *Sec,