[PATCH] D72992: [llvm-objdump] - Add column headers for relocation printing

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/WebAssembly/relocations.test:12
 
-; CHECK:      RELOCATION RECORDS FOR [CODE]:
-; CHECK-NEXT: 00000006 R_WASM_TYPE_INDEX_LEB 1+0
+; CHECK:RELOCATION RECORDS FOR [CODE]:
+; CHECK-NEXT:OFFSET   TYPE                     VALUE
----------------
As a slight readability suggestion, consider doing the following:

`;      CHECK:RELOCATION RECORDS FOR [CODE]:`

This allows for the ':' characters and therefore the actual checked-for text to line up.

Same applies a few lines below.



================
Comment at: llvm/test/tools/llvm-objdump/relocations-elf.test:191
+- Name: .rela.text
+  Type: SHT_RELA
+  Link: .symtab
----------------
jhenderson wrote:
> grimar wrote:
> > grimar wrote:
> > > It is strange to see `SHT_RELA` section in a `ELFCLASS32` object.
> > > What do you want to check with it? I see it is what the test case above did, but I do not know why it does that :)
> > (I mean it has `SHT_REL`, what is unnatural for `ELFCLASS64` in the same way like `SHT_RELA` is unnatural for `ELFCLASS32`). 
> I'd keep both SHT_REL and SHT_RELA sections. The gABI doesn't limit these sections to architectures of one or other size (although some processor supplements do), so we should show we can handle all situations. I know at least one system I worked on used SHT_REL originally and then later swapped to SHT_RELA.
@grimar, are you happy with my suggestion of keeping both?


================
Comment at: llvm/test/tools/llvm-objdump/relocations-elf.test:7
 
-# CHECK:      RELOCATION RECORDS FOR [.text]:
-# CHECK-NEXT: 0000000000000001 R_X86_64_32 glob1
-# CHECK-NEXT: 0000000000000001 R_X86_64_32S glob2
-# CHECK-NEXT: 0000000000000002 R_X86_64_64 loc1
-# CHECK-NEXT: 0000000000000001 R_X86_64_32 glob1+0x1
-# CHECK-NEXT: 0000000000000001 R_X86_64_32S glob2+0x2
-# CHECK-NEXT: 0000000000000002 R_X86_64_64 loc1+0x3
+# CHECK:RELOCATION RECORDS FOR [.text]:
+# CHECK-NEXT:OFFSET           TYPE                     VALUE
----------------
Same comment as the WASM test.


================
Comment at: llvm/test/tools/llvm-objdump/relocations-elf.test:150
+# RUN: yaml2obj --docnum=4 %s > %t4
+# RUN: llvm-objdump -r %t4 | FileCheck %s --check-prefix=32ELF --strict-whitespace --match-full-lines
+
----------------
`ELF32` is the canonical naming style for 32-bit ELF (and ELF64 for 64-bit ELF). Please use that, not `32ELF`, as @grimar originally asked for.


================
Comment at: llvm/test/tools/llvm-objdump/relocations-elf.test:152
+
+# 32ELF:RELOCATION RECORDS FOR [.text]:
+# 32ELF-NEXT:OFFSET   TYPE                     VALUE
----------------
Same comment as other tests.


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

https://reviews.llvm.org/D72992