[PATCH] D49016: [llvm-objdump] Add dynamic section printing to private-headers option

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Hmm... I don't like using the dynamic symbol table to lookup the dynamic string table. There is a reason for a specific DT_STRTAB tag. We should use that. Also, this latest version won't work if the section headers have been stripped, but we should be able to dump the dynamic table in this instance, by looking at the DT_* tags in the PT_DYNAMIC segment (if present). Try adding a test for this case (i.e. section headers stripped), and you'll see an error, I suspect... Also, please make sure you have a test that has no section headers or PT_DYNAMIC segment.

In https://reviews.llvm.org/D49016#1165043, @paulsemel wrote:

> Alright.. Again, there was a lot of changes I wanted to do, so sorry if I messed smth @jhenderson.
>  I eventually moved some code from `llvm-readobj` to `libObject`, and I will refactor `llvm-readobj` similar code in another PR !


Could you do this separate PR first, please? Otherwise, we end up with (temporarily) duplicate code, which might be an issue if somebody modifies llvm-readobj in the meantime.



================
Comment at: include/llvm/ADT/STLExtras.h:1041-1045
+template <class T, std::size_t N>
+constexpr size_t size(const T (&array)[N]) noexcept {
+  return N;
+}
+
----------------
paulsemel wrote:
> jhenderson wrote:
> > Be careful that this doesn't get submitted with everything else...
> Sure, I will wait for the other patch to be accepted, and then I'll rebase
Actually, since it's been pointed out, you might as well use array_lengthof in this patch, to remove any dependency.


================
Comment at: include/llvm/Object/ELF.h:533
+  // back on the sections.
+  if (Dyn.empty()) {
+    auto SectionsOrError = sections();
----------------
It occurs to me that this half of the code is currently untested. Could you add a test that shows that if a dynamic segment doesn't exist, we look for and use the .dynamic section.

You should also add a test that shows that if neither exist, we just don't print anything.


================
Comment at: include/llvm/Object/ELF.h:451
+template <class ELFT>
+Expected<typename ELFT::DynRange> ELFFile<ELFT>::dynamicEntries() const {
+  const Elf_Dyn *Dyn = nullptr;
----------------
paulsemel wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > I think this should use the dynamic program header, rather than the sections, because a runnable program doesn't need section headers (but it does not program headers). It'll be faster, because there are generally fewer program headers than sections too. Maybe it could fall back to using the section headers if there is no PT_DYNAMIC segment.
> > > 
> > > Are the functions in this file unit-tested at all? If so, we should add them. If not, we still might want to add some, but it would depend on how straightforward it is to do.
> > I don't think you've addressed this comment: 
> > >Are the functions in this file unit-tested at all? If so, we should add them. If not, we still might want to add some, but it would depend on how straightforward it is to do.
> > 
> > 
> No there is basically two features unit tested in the libObject. Do you really want me to do one ?
Is it possible to easily construct an ELFFile in memory? If not, I suppose not, as we might as well use the lit tests to achieve this.


================
Comment at: test/tools/llvm-objdump/private-headers-dynamic-section.test:27
+# CHECK:  VERSYM               0x00000000000003ec
+# CHECK:  RELACOUNT            0x0000000000000003
----------------
paulsemel wrote:
> jhenderson wrote:
> > Could you also add SHT_RELR cases here, please?
> Are you talking about PLT REL entries ? I've added some anyway !
No, I was referring to SHT_RELR, added recently to LLD (rL336594) and llvm-readobj (rL335922). See also the proposal at https://groups.google.com/forum/#!topic/generic-abi/bX460iggiKg. The important bit is that there are some additional DT_* tags for these sections: DT_RELR, DT_RELRENT, and DT_RELRSZ, and I'd like it if these were tested too.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:44
+template <class ELFT>
+Expected<StringRef> getDynamicStrTab(const ELFFile<ELFT> *o,
+                                     const typename ELFFile<ELFT>::Elf_Dyn *Dyn,
----------------
jhenderson wrote:
> o -> O (or probably better just Elf or similar, since it's not an object file if its got a dynamic section).
Um... This hasn't been done?


Repository:
  rL LLVM

https://reviews.llvm.org/D49016