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

[Paul Semel via Phabricator via llvm-commits]

paulsemel added inline comments.


================
Comment at: test/tools/llvm-objdump/private-headers-dynamic-section.test:1
+# RUN: llvm-objdump -p %p/Inputs/eh_frame.elf-mipsel | FileCheck %s
+
----------------
jhenderson wrote:
> 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.
Ok.. I gave it a try. The problem is not about building the dynamic section (though it might lead to confusing tests), but more about all the needed parts we need to have to get a relevant test (Program headers, string tables etc..)
What do you think ?


================
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)
----------------
jhenderson wrote:
> 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.
Ok.. I finally added a helper function in libObject, I think it is the right way to do it.
Moreover, DT_NULL is needed by the ELF specification (it must end the Dynamic section), so let's issue an error.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:145
+      break;
+    case ELF::DT_GNU_HASH:
+      Str = "GNU_HASH";
----------------
jhenderson wrote:
> 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.
This was just to initiate this feature, I didn't really know which one to handle. But I will just take every tags in DynamicTags.def


================
Comment at: tools/llvm-objdump/ELFDump.cpp:145
+      break;
+    case ELF::DT_GNU_HASH:
+      Str = "GNU_HASH";
----------------
paulsemel wrote:
> jhenderson wrote:
> > 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.
> This was just to initiate this feature, I didn't really know which one to handle. But I will just take every tags in DynamicTags.def
Ok I'll answer here for this comment and the one above. I decided to take get all the non architecture specific tags for the moment.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:172
+      Expected<StringRef> StrTabOrErr = getDynamicStrTab(o, Dyn, VirtBase);
+      if (StrTabOrErr) {
+        const char *Data = StrTabOrErr.get().data();
----------------
jhenderson wrote:
> What about if there's an error? You should not just ignore it...
Well, I think we should just consume the error and fallback to the hex print (that's what being done here basically - I just need to consume the error)


================
Comment at: tools/llvm-objdump/ELFDump.cpp:227
       outs() << "    PHDR ";
+      VirtBase = Phdr.p_vaddr - Phdr.p_offset;
       break;
----------------
jhenderson wrote:
> 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)```
Your method is not working on my testing binary. The address given in the dynamic section are relative to PHDR, so I do not understand how you want to do do it an other way.
I'll let it like this until I find another solution, but this one is not working :/


Repository:
  rL LLVM

https://reviews.llvm.org/D49016