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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

I think you can only add `--strict-whitespaces` to `relocations-elf.test`. No need to touch and change another tests probably,
it is enough to test the paddings in one place once and not spread this check everywhere (if there is no valid reason).



================
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
----------------
Missing a full stop.


================
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=32BITPADDING --strict-whitespace
+
----------------
I'd `32BITPADDING` -> `ELF32`


================
Comment at: llvm/test/tools/llvm-objdump/relocations-elf.test:153
+# 32BITPADDING:      RELOCATION RECORDS FOR [.text]:
+# 32BITPADDING-NEXT: OFFSET   TYPE                     VALUE
+# 32BITPADDING-NEXT: 00000001 R_386_NONE               glob1
----------------
I think you either need to use `--match-full-lines` or use `{{^}}` which checks the begining of the line:
```
# 32BITPADDING: {{^}}OFFSET   TYPE                     VALUE
```
because currently you do not check the paddings at the begining of the lines.


================
Comment at: llvm/test/tools/llvm-objdump/relocations-elf.test:167
+  Machine: EM_386
+
+Sections:
----------------
I see that you've basing on the YAML from above, but we usually do not
leave such empty lines in YAMLs (see another tests), so please remove.


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


================
Comment at: llvm/test/tools/llvm-objdump/relocations-elf.test:211
+  - Name: loc1
+  - Name: loc2
+  - Name:    glob1
----------------
It seems enough to have one local and one global symbol here.
(the binding does not affect the output anyways it seems).


================
Comment at: llvm/test/tools/llvm-objdump/relocations-elf.test:218
+  - Name:    glob2
+    Binding: STB_GLOBAL
----------------
It is generally a good practice to minimize the YAML used.
This one contains excessive parts.
Also, we often format it to improve readability.

I think you can use something like this:


```
--- !ELF
FileHeader:
  Class:   ELFCLASS32
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_386
Sections:
- Name: .text
  Type: SHT_PROGBITS
- Name: .rel.text
  Type: SHT_REL
  Info: .text
  Relocations:
    - Offset: 0x1
      Symbol: global
      Type:   R_386_PC32
    - Offset: 0x2
      Symbol: loc
      Type:   R_386_32
Symbols:
  - Name:    loc
  - Name:    global
    Binding: STB_GLOBAL
```


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

https://reviews.llvm.org/D72992