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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I noticed that llvm-readobj already implements a lot of stuff that would be helpful for this - specifically, it already prevents a mechanism for getting the dynamic tags. I don't think it makes sense to reinvent the wheel, so maybe you should consider finding a way to share that code between the two. It would probably need to be in the llvmObject library.



================
Comment at: test/tools/llvm-objdump/private-headers-dynamic-section.test:1
+# RUN: llvm-objdump -p %p/Inputs/eh_frame.elf-mipsel | FileCheck %s
+
----------------
As with other tests, I'd rather not use a pre-canned binary here, especially one that isn't directly associated with this test. Please use yaml2obj.


================
Comment at: test/tools/llvm-objdump/private-headers-dynamic-section.test:4
+# CHECK: Dynamic Section:
+# CHECK:   NEEDED            libstdc++.so.6
+# CHECK:   NEEDED            libm.so.6
----------------
Assuming yaml2obj is able to do this, I'd just have one of each tag we need to print (perhaps 2 for NEEDED, so that we show our string table look ups work properly). If it isn't, I'd look at extending yaml2obj in preference to messing about trying to get a binary with everything.

We also need to have one of each tag we support printing of, plus one tag that isn't interpreted at this point. In particular, we need both DT_REL and DT_RELA-style tags printing, but there are plenty of other untested tags too. I don't know if yaml2obj will allow all of these in a single file, so you might need multiple test cases.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:28
+                                     uint64_t VirtBase) {
+  for (; Dyn->d_tag != ELF::DT_NULL; Dyn++) {
+    if (Dyn->d_tag == ELF::DT_STRTAB)
----------------
What happens if you have a malformed segment with no DT_NULL tag?

You should probably use the program header's p_filesz field as an additional check to avoid overrunning the end of the array. If we hit p_filesz and not DT_NULL, we should probably warn. Assuming you can get yaml2obj to create such a section, I'd also expect a test case for that.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:33
+  }
+  return createError("string table entry not present");
+}
----------------
I'd be a little bit more specific and say "DT_STRTAB entry..."


================
Comment at: tools/llvm-objdump/ELFDump.cpp:41
+  outs() << "Dynamic Section:\n";
+  for (; Dyn->d_tag != ELF::DT_NULL; Dyn++) {
+    char TagValue[11];
----------------
See my earlier comment about DT_NULL not being present.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:42-43
+  for (; Dyn->d_tag != ELF::DT_NULL; Dyn++) {
+    char TagValue[11];
+    sprintf(TagValue, "0x%x", static_cast<unsigned int>(Dyn->d_tag));
+    const char *Str = nullptr;
----------------
I'm not sure we should do this upfront. I think it should be on an as-needed basis. I also think that there's something in the llvm support library that turns an integer into a hex string - see `utohexstr` etc.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:44
+    sprintf(TagValue, "0x%x", static_cast<unsigned int>(Dyn->d_tag));
+    const char *Str = nullptr;
+    switch (Dyn->d_tag) {
----------------
You can leave this unitialised, or better yet use a StringRef.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:143
+    case ELF::DT_LOOS:
+      Str = "LOOS";
+      break;
----------------
DT_LOOS shouldn't be printed like this. It's a boundary marker, so would actually be some platform specific value (e.g. DT_GNU_MY_MAGIC_TAG).

Similar comments go for DT_HIGHOS, DT_LOPROC, and DT_HIGHPROC, although I see you haven't got them in the list.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:145
+      break;
+    case ELF::DT_GNU_HASH:
+      Str = "GNU_HASH";
----------------
What made you pick just this set of platform specific values?

I'd actually suggest that you should have printing for all dynamic tags listed in llvm/BinaryFormat/DynamicTags.def.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:167
+    }
+    outs() << format("  %-18s", Str);
+    const char *Fmt =
----------------
Why 18? (Please add a comment).


================
Comment at: tools/llvm-objdump/ELFDump.cpp:172
+      Expected<StringRef> StrTabOrErr = getDynamicStrTab(o, Dyn, VirtBase);
+      if (StrTabOrErr) {
+        const char *Data = StrTabOrErr.get().data();
----------------
What about if there's an error? You should not just ignore it...


================
Comment at: tools/llvm-objdump/ELFDump.cpp:182
+
 template <class ELFT> void printProgramHeaders(const ELFFile<ELFT> *o) {
   typedef ELFFile<ELFT> ELFO;
----------------
I don't like the fact that this "print" function has side effects of looking up the dynamic segment. I'd rather have an extra loop in `printDynamicSection` do the lookup (which should be simple, because we only care about one kind of segment).

I'm also not sure that `printProgramHeaders` should also print the contents of the dynamic segment. I think that feels like a separate operation (i.e. I could imagine a case where you want to just print the dynamic segment, but without the rest of the program headers). I think that implies that the two should be separate unrelated functions, perhaps with a single third function `printPrivateHeaders` or similar that prints both.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:227
       outs() << "    PHDR ";
+      VirtBase = Phdr.p_vaddr - Phdr.p_offset;
       break;
----------------
This is not a reliable way of looking up the base address of an ELF, because PT_PHDR is optional. I'm actually not really sure what the best way is (you could use the PT_LOAD segments as well), but fortune has it that it's unnecessary, since the dynamic segment itself has an address: you can use that to calculate the relative offset between the segment start and the dynamic string table, and use that relative offset to find the string table section. I think the following calculation would work, though you should probably check it:

```StrtabOffset = DynamicSegment->p_offset - (DynamicSegment->p_vaddr -
                  StrtabTag->d_un.d_val)```


Repository:
  rL LLVM

https://reviews.llvm.org/D49016