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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/Object/ELF.h:513
+template <class ELFT>
+Expected<typename ELFT::DynRange> ELFFile<ELFT>::dynamicEntries() const {
+  ArrayRef<Elf_Dyn> Dyn;
----------------
jhenderson wrote:
> Again, this should be moved to the source file.
You've marked this as done but `dynamicEntries` is still in the header file...


================
Comment at: test/tools/llvm-objdump/private-headers-dynamic-section.test:1
+# RUN: llvm-objdump -p %p/Inputs/private-headers-x86_64.elf | FileCheck %s
+
----------------
jhenderson wrote:
> One more test suggestion: use llvm-objcopy to remove the section headers for this test input and then run the same dump. It should produce the same output.
Sorry, I guess I wasn't clear. I was expecting these to be in the same test file. So the run commands would look something like:


```
# RUN: llvm-objdump -p %p/Inputs/private-headers-x86_64.elf | FileCheck %s
# RUN: llvm-objcopy <insert appropriate arguments here> %p/Inputs/private-headers-x86_64.elf %t-stripped.elf
# RUN: llvm-objdump -p %t-stripped.elf | FileCheck %s
```

This means that you can show that the output is identical with and without the section headers.


================
Comment at: test/tools/llvm-objdump/private-headers-no-dynamic.test:18
+
+# CHECK: Dynamic Section:
----------------
paulsemel wrote:
> jhenderson wrote:
> > Maybe worth doing a `CHECK-NOT: DT_` or similar to show that the tool isn't trying to dump garbage in this cae.
> I though about it, but there is no pattern I can check.. (as I am not printing the DT_ thing)
This is the last bit of the output right? If so, you can do:

CHECK-NOT: {{.}} or similar.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:42
+
+  for (const typename ELFO::Elf_Phdr &Phdr : *ProgramHeaderOrError) {
+    if (Phdr.p_type == ELF::PT_LOAD) {
----------------
paulsemel wrote:
> jhenderson wrote:
> > I just realised that you're reinventing the wheel again here. llvm-readobj has the same function to do the same sort of thing. Take a look at `toMappedAddr` in `parseDynamicTable` in the ElfDumper class. It probably is worth sharing this again, so as to not repeat the code, and risk getting it slightly wrong.
> I move this toMappedAddr function to libObj, as I see a lot of use cases for it :)
Yes, I agree with this.


Repository:
  rL LLVM

https://reviews.llvm.org/D49016