[PATCH] D72992: [llvm-objdump] - Add column headers for relocation printing
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 22 00:51:34 PST 2020
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
More information about the llvm-commits
mailing list