[PATCH] D75671: [llvm-readobj][llvm-readelf][test] - Add a test to check how we dump relocation addends.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-addends.test:1
+## Check how llvm-readobj and llvm-readelf tools dumps addends of rellocations. 
+
----------------
dumps -> dump
rellocations -> relocations


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-addends.test:3
+
+# RUN: yaml2obj --docnum=1 -D ENCODE=LSB -DTYPE=SHT_RELA %s -o %t.le64.rela
+# RUN: llvm-readobj -r %t.le64.rela | FileCheck %s --check-prefix=LLVM-RELA64 --match-full-lines
----------------
Inconsistent spacing for -D option (should be -DENCODE=LSB or -D TYPE=SHT_RELA). Same throughout.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-addends.test:8-9
+
+# LLVM-RELA64:      Relocations [
+# LLVM-RELA64-NEXT:   Section (1) .foo {
+# LLVM-RELA64-NEXT:     0x0 R_X86_64_NONE - 0x0
----------------
This test is about addend printing, so we probably don't need the scoping/header lines for context. Same goes throughout. You also probably don't need --match-full-lines.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-addends.test:10
+# LLVM-RELA64-NEXT:   Section (1) .foo {
+# LLVM-RELA64-NEXT:     0x0 R_X86_64_NONE - 0x0
+# LLVM-RELA64-NEXT:     0x0 R_X86_64_NONE - 0x1
----------------
Here and on all the lines, you probably want `R_X86_64_NONE - 0x0{{$}}` instead of --match-full-lines, as you don't need the offset for the test.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-addends.test:24-25
+
+## FIXME: We either should not dump an addend or should read it from a
+##        destination location for a SHT_REL case.
+
----------------
Unless you have a patch to imminently fix this, I'd recommend filing a bug. Same goes for the other FIXMEs below (and indeed all FIXMEs in general).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-addends.test:33
+
+## Check what llvm-readelf prints for the same objects.
+
----------------
I think I'd find it slightly easier to read this test if identical readelf and readobj test cases are grouped together. In other words:

```
# RUN: llvm-readobj <little endian, rela>
# RUN: llvm-readelf <little endian, rela>

# checks for little endian rela

# RUN: llvm-readobj <big endian, rela>
# RUN: llvm-readelf <big endian, rela>

# checks for big endian rela

# RUN: llvm-readobj <little endian, rel>
# RUN: llvm-readelf <little endian, rel>

# checks for little endian rel

# RUN: llvm-readobj <big endian, rel>
# RUN: llvm-readelf <big endian, rel>

# checks for big endian rel
```


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-addends.test:38
+
+# GNU-RELA64:      Relocation section '.foo' at offset 0x40 contains 6 entries:
+# GNU-RELA64-NEXT: Offset            Info             Type          Symbol's Value  Symbol's Name + Addend
----------------
Don't need this line.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-addends.test:39
+# GNU-RELA64:      Relocation section '.foo' at offset 0x40 contains 6 entries:
+# GNU-RELA64-NEXT: Offset            Info             Type          Symbol's Value  Symbol's Name + Addend
+# GNU-RELA64-NEXT: 0000000000000000  0000000000000000 R_X86_64_NONE                                 0
----------------
You only really need the Type to Addend columns in this line.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-addends.test:82
+        Type:   R_X86_64_NONE
+## Addend == an arbitrary negative number.
+      - Addend: 0xFFFFFFFFFFFFCFC7 ## -12345
----------------
Any particular reason you have an arbitrary negative number, but not an arbitrary positive one?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-addends.test:86
+
+## Starting from here we check ELFCLASS32 objects.
+
----------------
Is there anything actually stopping us using R_X86_64 for both 64 and 32 bit cases, and therefore allowing us to use the same YAML throughout?


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

https://reviews.llvm.org/D75671