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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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

> Hmm no.. I'm pretty sure it is a bad idea to do a PR that is completely relying on this one. I just duplicated the code, so I don't see any problem even if someone modifies this area in llvm-readobj. I will just refactor no worries :)


Okay. Please make sure in the commit comment to explain that this is a copy of the llvm-readobj code, and that a subsequent refactor is still to come.



================
Comment at: include/llvm/Object/ELF.h:421-422
+template <class ELFT>
+const char *ELFFile<ELFT>::getDynamicTagAsString(unsigned Arch,
+                                                 uint64_t Type) const {
+#define DYNAMIC_TAG(n, v)
----------------
This strikes me as a rather large function to have in a header file. What do you think about moving the body into ELF.cpp source file? It looks like there is precedence for this already. I also noticed that the source provides a "STRINGIFY_ENUM_CASE" macro that does what you want.


================
Comment at: include/llvm/Object/ELF.h:513
+template <class ELFT>
+Expected<typename ELFT::DynRange> ELFFile<ELFT>::dynamicEntries() const {
+  ArrayRef<Elf_Dyn> Dyn;
----------------
Again, this should be moved to the source 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
+
----------------
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.


================
Comment at: test/tools/llvm-objdump/private-headers-no-dynamic-segment.test:14
+    Flags:           [ SHF_ALLOC, SHF_WRITE ]
+    Content: 0c00000000000000a0060000000000000d0000000000000024090000000000001900000000000000a80d2000000000001b0000000000000010000000000000001a00000000000000b80d2000000000001c000000000000000800000000000000f5feff6f0000000098020000000000000500000000000000c8030000000000000600000000000000c0020000000000000a000000000000002f010000000000000b0000000000000018000000000000001500000000000000000000000000000003000000000000000010200000000000020000000000000048000000000000001400000000000000070000000000000017000000000000005806000000000000070000000000000050050000000000000800000000000000080100000000000009000000000000001800000000000000fbffff6f000000000000000800000000feffff6f000000001005000000000000ffffff6f000000000200000000000000f0ffff6f00000000f804000000000000f9ffff6f0000000004000000000000002300000000000000140000000000000024000000000000002143658700000000250000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000000
+
----------------
Now that you've done this, feel free to keep it, but I think you can simplify it down a bit and not test all the tags here.


================
Comment at: test/tools/llvm-objdump/private-headers-no-dynamic.test:18
+
+# CHECK: Dynamic Section:
----------------
Maybe worth doing a `CHECK-NOT: DT_` or similar to show that the tool isn't trying to dump garbage in this cae.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:32-35
+  for (const typename ELFO::Elf_Shdr &Sec : *SectionsOrError) {
+    if (Sec.sh_type == ELF::SHT_DYNSYM)
+      return Elf->getStringTableForSymtab(Sec);
+  }
----------------
I don't think we want this loop as the first choice. Making it the fallback would match the behaviour earlier of `dynamicEntries()` (i.e. use program headers first, and only fallback on sections if that doesn't work.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:42
+
+  for (const typename ELFO::Elf_Phdr &Phdr : *ProgramHeaderOrError) {
+    if (Phdr.p_type == ELF::PT_LOAD) {
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D49016