[PATCH] D54697: [llvm-objdump] Add `Version References` dumper

[George Rimar via Phabricator via llvm-commits]

grimar 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
+
----------------
that->than, have->has (sorry, did these mistakes/mistypes when suggested it).

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




================
Comment at: tools/llvm-objdump/ELFDump.cpp:288
+    }
+  }
+
----------------
Higuoxing wrote:
> grimar wrote:
> > 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);
> >   }
> > }
> > ```
> > 
> > 
> 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. 
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.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:303
+template <class ELFT>
+void printSymbolVersionInfo(const ELFFile<ELFT> *Elf, StringRef FileName) {
+  typedef typename ELFT::Shdr Elf_Shdr;
----------------
Shouldn't you be able to get `Filename` from `Elf`?


================
Comment at: tools/llvm-objdump/ELFDump.cpp:313
+        Shdr.sh_type != ELF::SHT_GNU_verneed)
+      continue;
+
----------------
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`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54697