[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/relocations-elf.test:148
+
+## Check relocation formatting on 32 bit as well to verify the padding is correct
+# RUN: yaml2obj --docnum=4 %s > %t4
----------------
grimar wrote:
> liadz0rz wrote:
> > grimar wrote:
> > > Missing a full stop.
> > What's full stop?
> The symbol `.` used in writing at the end of a sentence.
I'd change "padding" to "formatting" as it also shows that the offset fieds are of the appropriate size.


================
Comment at: llvm/test/tools/llvm-objdump/relocations-elf.test:191
+- Name: .rela.text
+  Type: SHT_RELA
+  Link: .symtab
----------------
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.


================
Comment at: llvm/test/tools/llvm-objdump/relocations-in-nonreloc.test:9
 # RUN: llvm-objdump -r %t3 | FileCheck %s -DFILE=%t3 --check-prefixes=FMT,REL --implicit-check-not={{.}}
+# RUN: llvm-objdump -r %t3 | FileCheck %s -DFILE=%t3 --strict-whitespace --check-prefixes=REL
 
----------------
I'm not sure you need this additional test case here. I don't think it adds anything above the line above.


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

https://reviews.llvm.org/D72992