[PATCH] D109016: [llvm-objdump] Fix 'llvm-objdump -dr' for executables with relocations

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/exec-relocations.test:1
+## Check that 'llvm-objdump -dr' correctly prints relocations in executables.
+
----------------
I think this test needs moving into llvm-objdump/X86, as it disassembles X86 and therefore requires that target. You can maybe rename it `elf-disassemble-relocs-exec.test` to mirror the existing `elf-disassemble-relocs.test` found therein.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/exec-relocations.test:17
+  Machine:         EM_X86_64
+  Entry:           0x4004A0
+ProgramHeaders:
----------------
You don't need a specific entry point to be defined for this test. This can be omitted.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/exec-relocations.test:19
+ProgramHeaders:
+  - Type:            PT_PHDR
+    Flags:           [ PF_R ]
----------------
You don't need this program header. It shouldn't directly impact the disassembly + relocations.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/exec-relocations.test:28
+    VAddr:           0x400000
+    Align:           0x200000
+Sections:
----------------
There's no need for this alignment value. Just omit it.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/exec-relocations.test:33
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x400586
+    AddressAlign:    0x10
----------------
If memory serves me rightly, yaml2obj automatically sets the Address values, based on the program header address value the section is in. I might be misremembering though, so maybe best do some experimentation where one or both sections are missing the address, and see what the output section addresses look like.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/exec-relocations.test:35
+    AddressAlign:    0x10
+    Content:         554889E5BF28064000E8FCFEFFFFB8000000005DC3
+  - Name:            .rodata
----------------
You should be able to simplify this content, so only a small amount is needed (specifically for the two instructions being checked, plus maybe a simple nop instruction before and after, to show relocations aren't printed in the wrong place).


================
Comment at: llvm/test/tools/llvm-objdump/ELF/exec-relocations.test:45
+    Flags:           [ SHF_INFO_LINK ]
+    Link:            .symtab
+    AddressAlign:    0x8
----------------
You can omit this. yaml2obj sets relocation section Link fields to .symtab by default.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/exec-relocations.test:54
+      - Offset:          0x400590
+        Symbol:          'puts'
+        Type:            R_X86_64_PLT32
----------------
No need to quote the name.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/exec-relocations.test:62
+    Value:           0x400628
+  - Name:            'puts'
+    Type:            STT_FUNC
----------------
You don't need to quote the name.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109016/new/

https://reviews.llvm.org/D109016