This is an archive of the discontinued LLVM Phabricator instance.

Differential D75671

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

Authored by grimar on Mar 5 2020, 3:54 AM.

Details

Reviewers
jhenderson
MaskRay
• espindola
Commits
rG63778bc653a2: [llvm-readobj][llvm-readelf][test] - Add a test to check how we dump relocation…
Summary

Seems we do not test how we print relocation addends well.
And the behavior of dumpers does not seem to be ideal here
(and llvm-readelf does not match GNU as the test case shows).

This patch adds a test case to document the current behavior.

Depends on D75527 and D75608.

Diff Detail

Event Timeline

grimar created this revision.Mar 5 2020, 3:54 AM
Herald added a reviewer: • espindola. · View Herald TranscriptMar 5 2020, 3:54 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: rupprecht, emaste. · View Herald Transcript
grimar updated this revision to Diff 248431.Mar 5 2020, 3:56 AM
  • Removed unused "Offset: 0x0"
grimar mentioned this in D75527: [yaml2obj] - Add `ELFYAML::YAMLIntUInt` to fix how we parse a relocation `Addend` key..Mar 5 2020, 6:34 AM
grimar updated this revision to Diff 248472.Mar 5 2020, 6:48 AM
  • Fixed comments and formatted the test case.
jhenderson added inline comments.Mar 6 2020, 1:34 AM
llvm/test/tools/llvm-readobj/ELF/reloc-addends.test
2

dumps -> dump
rellocations -> relocations

4

Inconsistent spacing for -D option (should be -DENCODE=LSB or -D TYPE=SHT_RELA). Same throughout.

9–10

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.

11

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.

25–26

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).

34

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
39

Don't need this line.

40

You only really need the Type to Addend columns in this line.

83

Any particular reason you have an arbitrary negative number, but not an arbitrary positive one?

87

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?

grimar updated this revision to Diff 251041.Mar 18 2020, 4:53 AM
grimar marked 13 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/reloc-addends.test
25–26

I've added a link to the existent bug here and reported a new bug and added a link for a FIXME below.

83

Nope. I just do not like the negative numbers representation I think, my brain assumes them are "special".
I.e added the arbitrary positive number case.

87

32 and 64 bit cases not only have different relocations, but also the different size of r_addend.
I`d probably prefer not to mix them in this test case.

grimar marked an inline comment as done.Mar 18 2020, 4:54 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/reloc-addends.test
83

I.e -> I've

jhenderson accepted this revision.Mar 20 2020, 2:38 AM

LGTM, with a couple of nits.

llvm/test/tools/llvm-readobj/ELF/reloc-addends.test
13

Missing {{$}}?

17

I'd probably delete this blank line, to show that this is tied to the GNU block (mostly for consistency with my comment in the next section).

35–37

The FIXME is tied to the LLVM case, right? If so, delete the blank line after it. At the moment, it looks like it applies to both cases.

96

Same comment as above re. the blank line after this FIXME.

116

Same comment as above re. this FIXME.

This revision is now accepted and ready to land.Mar 20 2020, 2:38 AM
Closed by commit rG63778bc653a2: [llvm-readobj][llvm-readelf][test] - Add a test to check how we dump relocation… (authored by grimar). · Explain WhyMar 20 2020, 3:46 AM
This revision was automatically updated to reflect the committed changes.
grimar marked 5 inline comments as done.

Revision Contents

PathSize
llvm/
test/
tools/
llvm-readobj/
ELF/
159 lines

Diff 251041

llvm/test/tools/llvm-readobj/ELF/reloc-addends.test

  • This file was added.
## Check how llvm-readobj and llvm-readelf tools dump addends of relocations.
jhendersonUnsubmitted
Done
ReplyInline Actions

dumps -> dump
rellocations -> relocations

jhenderson: dumps -> dump rellocations -> relocations
# RUN: yaml2obj --docnum=1 -DENCODE=LSB -DTYPE=SHT_RELA %s -o %t.le64.rela
# RUN: llvm-readobj -r %t.le64.rela | FileCheck %s --check-prefix=LLVM-RELA64
jhendersonUnsubmitted
Done
ReplyInline Actions

Inconsistent spacing for -D option (should be -DENCODE=LSB or -D TYPE=SHT_RELA). Same throughout.

jhenderson: Inconsistent spacing for -D option (should be -DENCODE=LSB or -D TYPE=SHT_RELA). Same…
# RUN: llvm-readelf -r %t.le64.rela | FileCheck %s --check-prefix=GNU-RELA64
# LLVM-RELA64: R_X86_64_NONE - 0x0{{$}}
# LLVM-RELA64-NEXT: R_X86_64_NONE - 0x1{{$}}
# LLVM-RELA64-NEXT: R_X86_64_NONE - 0xFFFFFFFFFFFFFFFF{{$}}
# LLVM-RELA64-NEXT: R_X86_64_NONE - 0x7FFFFFFFFFFFFFFF{{$}}
jhendersonUnsubmitted
Done
ReplyInline Actions

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.

jhenderson: This test is about addend printing, so we probably don't need the scoping/header lines for…
# LLVM-RELA64-NEXT: R_X86_64_NONE - 0x8000000000000000{{$}}
jhendersonUnsubmitted
Done
ReplyInline Actions

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.

jhenderson: Here and on all the lines, you probably want `R_X86_64_NONE - 0x0{{$}}` instead of --match-full…
# LLVM-RELA64-NEXT: R_X86_64_NONE - 0xFFFFFFFFFFFFCFC7{{$}}
# LLVM-RELA64-NEXT: R_X86_64_NONE - 0x12345
jhendersonUnsubmitted
Done
ReplyInline Actions

Missing {{$}}?

jhenderson: Missing `{{$}}`?
## FIXME: GNU readelf prints addends differently.
## See https://bugs.llvm.org/show_bug.cgi?id=45235.
jhendersonUnsubmitted
Done
ReplyInline Actions

I'd probably delete this blank line, to show that this is tied to the GNU block (mostly for consistency with my comment in the next section).

jhenderson: I'd probably delete this blank line, to show that this is tied to the GNU block (mostly for…
# GNU-RELA64: Type Symbol's Value Symbol's Name + Addend
# GNU-RELA64-NEXT: R_X86_64_NONE 0{{$}}
# GNU-RELA64-NEXT: R_X86_64_NONE 1{{$}}
# GNU-RELA64-NEXT: R_X86_64_NONE ffffffffffffffff{{$}}
# GNU-RELA64-NEXT: R_X86_64_NONE 7fffffffffffffff{{$}}
# GNU-RELA64-NEXT: R_X86_64_NONE 8000000000000000{{$}}
# GNU-RELA64-NEXT: R_X86_64_NONE ffffffffffffcfc7{{$}}
# GNU-RELA64-NEXT: R_X86_64_NONE 12345{{$}}
jhendersonUnsubmitted
Done
ReplyInline Actions

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).

jhenderson: Unless you have a patch to imminently fix this, I'd recommend filing a bug. Same goes for the…
grimarAuthorUnsubmitted
Done
ReplyInline Actions

I've added a link to the existent bug here and reported a new bug and added a link for a FIXME below.

grimar: I've added a link to the existent bug here and reported a new bug and added a link for a FIXME…
# RUN: yaml2obj --docnum=1 -DENCODE=MSB -DTYPE=SHT_RELA %s -o %t.be64.rela
# RUN: llvm-readobj -r %t.be64.rela | FileCheck %s --check-prefix=LLVM-RELA64
# RUN: llvm-readelf -r %t.be64.rela | FileCheck %s --check-prefix=GNU-RELA64
# RUN: yaml2obj --docnum=1 -DENCODE=LSB -DTYPE=SHT_REL %s -o %t.le64.rel
# RUN: llvm-readobj -r %t.le64.rel | FileCheck %s --check-prefix=LLVM-REL64
# RUN: llvm-readelf -r %t.le64.rel | FileCheck %s --check-prefix=GNU-REL64
jhendersonUnsubmitted
Done
ReplyInline Actions

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
jhenderson: I think I'd find it slightly easier to read this test if identical readelf and readobj test…
## FIXME: We either should not dump an addend or should read it from a
## destination location for a SHT_REL case.
## See https://bugs.llvm.org/show_bug.cgi?id=44257.
jhendersonUnsubmitted
Done
ReplyInline Actions

The FIXME is tied to the LLVM case, right? If so, delete the blank line after it. At the moment, it looks like it applies to both cases.

jhenderson: The FIXME is tied to the LLVM case, right? If so, delete the blank line after it. At the moment…
# LLVM-REL64-COUNT-7: R_X86_64_NONE - 0x0{{$}}
jhendersonUnsubmitted
Done
ReplyInline Actions

Don't need this line.

jhenderson: Don't need this line.
# LLVM-REL64-NOT: R_
jhendersonUnsubmitted
Done
ReplyInline Actions

You only really need the Type to Addend columns in this line.

jhenderson: You only really need the Type to Addend columns in this line.
# GNU-REL64: Type Symbol's Value Symbol's Name{{$}}
# GNU-REL64-COUNT-7: R_X86_64_NONE {{$}}
# GNU-REL64-NOT: R_
# RUN: yaml2obj --docnum=1 -DENCODE=MSB -DTYPE=SHT_REL %s -o %t.be64.rel
# RUN: llvm-readobj -r %t.be64.rel | FileCheck %s --check-prefix=LLVM-REL64
# RUN: llvm-readelf -r %t.be64.rel | FileCheck %s --check-prefix=GNU-REL64
--- !ELF
FileHeader:
Class: ELFCLASS64
Data: ELFDATA2[[ENCODE]]
Type: ET_EXEC
Machine: EM_X86_64
Sections:
- Name: .foo
Type: [[TYPE]]
Relocations:
## Addend == 0.
- Addend: 0x0
Type: R_X86_64_NONE
## Addend == first positive int64/uint64 == 1.
- Addend: 0x1
Type: R_X86_64_NONE
## Addend == first negative int64 == -1.
- Addend: 0xffffffffffffffff
Type: R_X86_64_NONE
## Addend == max possible int64 == 0x7FFFFFFFFFFFFFFF.
- Addend: 0x7FFFFFFFFFFFFFFF
Type: R_X86_64_NONE
## Addend == min possible int64 == 0x8000000000000000.
- Addend: 0x8000000000000000
Type: R_X86_64_NONE
## Addend == an arbitrary negative number.
- Addend: 0xFFFFFFFFFFFFCFC7 ## -12345
Type: R_X86_64_NONE
## Addend == an arbitrary positive number.
- Addend: 0x12345
Type: R_X86_64_NONE
## Starting from here we check ELFCLASS32 objects.
jhendersonUnsubmitted
Done
ReplyInline Actions

Any particular reason you have an arbitrary negative number, but not an arbitrary positive one?

jhenderson: Any particular reason you have an arbitrary negative number, but not an arbitrary positive one?
grimarAuthorUnsubmitted
Done
ReplyInline Actions

Nope. I just do not like the negative numbers representation I think, my brain assumes them are "special".
I.e added the arbitrary positive number case.

grimar: Nope. I just do not like the negative numbers representation I think, my brain assumes them are…
grimarAuthorUnsubmitted
Done
ReplyInline Actions

I.e -> I've

grimar: I.e -> I've
# RUN: yaml2obj --docnum=2 -DENCODE=LSB -DTYPE=SHT_RELA %s -o %t.le32.rela
# RUN: llvm-readobj -r %t.le32.rela | FileCheck %s --check-prefix=LLVM-RELA32
# RUN: llvm-readelf -r %t.le32.rela | FileCheck %s --check-prefix=GNU-RELA32
jhendersonUnsubmitted
Done
ReplyInline Actions

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?

jhenderson: Is there anything actually stopping us using R_X86_64 for both 64 and 32 bit cases, and…
grimarAuthorUnsubmitted
Done
ReplyInline Actions

32 and 64 bit cases not only have different relocations, but also the different size of r_addend.
I`d probably prefer not to mix them in this test case.

grimar: 32 and 64 bit cases not only have different relocations, but also the different size of…
# LLVM-RELA32: R_386_NONE - 0x0{{$}}
# LLVM-RELA32-NEXT: R_386_NONE - 0x1{{$}}
# LLVM-RELA32-NEXT: R_386_NONE - 0xFFFFFFFF{{$}}
# LLVM-RELA32-NEXT: R_386_NONE - 0x7FFFFFFF{{$}}
# LLVM-RELA32-NEXT: R_386_NONE - 0x80000000{{$}}
# LLVM-RELA32-NEXT: R_386_NONE - 0xFFFFCFC7{{$}}
## FIXME: GNU readelf prints addends differently.
## See https://bugs.llvm.org/show_bug.cgi?id=45235.
jhendersonUnsubmitted
Done
ReplyInline Actions

Same comment as above re. the blank line after this FIXME.

jhenderson: Same comment as above re. the blank line after this FIXME.
# GNU-RELA32: Type Sym. Value Symbol's Name + Addend
# GNU-RELA32-NEXT: R_386_NONE 0{{$}}
# GNU-RELA32-NEXT: R_386_NONE 1{{$}}
# GNU-RELA32-NEXT: R_386_NONE ffffffffffffffff{{$}}
# GNU-RELA32-NEXT: R_386_NONE 7fffffff{{$}}
# GNU-RELA32-NEXT: R_386_NONE ffffffff80000000{{$}}
# GNU-RELA32-NEXT: R_386_NONE ffffffffffffcfc7{{$}}
# RUN: yaml2obj --docnum=2 -DENCODE=MSB -DTYPE=SHT_RELA %s -o %t.be32.rela
# RUN: llvm-readobj -r %t.be32.rela | FileCheck %s --check-prefix=LLVM-RELA32
# RUN: llvm-readelf -r %t.be32.rela | FileCheck %s --check-prefix=GNU-RELA32
# RUN: yaml2obj --docnum=2 -DENCODE=LSB -DTYPE=SHT_REL %s -o %t.le32.rel
# RUN: llvm-readobj -r %t.le32.rel | FileCheck %s --check-prefix=LLVM-REL32
# RUN: llvm-readelf -r %t.le32.rel | FileCheck %s --check-prefix=GNU-REL32
## FIXME: We either should not dump an addend or should read it from a
## destination location for a SHT_REL case.
## See https://bugs.llvm.org/show_bug.cgi?id=44257.
jhendersonUnsubmitted
Done
ReplyInline Actions

Same comment as above re. this FIXME.

jhenderson: Same comment as above re. this FIXME.
# LLVM-REL32-COUNT-7: R_386_NONE - 0x0{{$}}
# LLVM-REL32-NOT: R_
# GNU-REL32: Type
# GNU-REL32-COUNT-7: R_386_NONE {{$}}
# GNU-REL32-NOT: R_
# RUN: yaml2obj --docnum=2 -DENCODE=MSB -DTYPE=SHT_REL %s -o %t.be32.rel
# RUN: llvm-readobj -r %t.be32.rel | FileCheck %s --check-prefix=LLVM-REL32
# RUN: llvm-readelf -r %t.be32.rel | FileCheck %s --check-prefix=GNU-REL32
--- !ELF
FileHeader:
Class: ELFCLASS32
Data: ELFDATA2[[ENCODE]]
Type: ET_EXEC
Machine: EM_386
Sections:
- Name: .foo
Type: [[TYPE]]
Relocations:
## Addend == 0.
- Addend: 0x0
Type: R_386_NONE
## Addend == first positive int32/uint32 == 1.
- Addend: 0x1
Type: R_386_NONE
## Addend == first negative int32 == -1.
- Addend: 0xffffffff
Type: R_386_NONE
## Addend == max possible int32 == 0x7FFFFFFF.
- Addend: 0x7FFFFFFF
Type: R_386_NONE
## Addend == min possible int32 == 0x80000000.
- Addend: 0x80000000
Type: R_386_NONE
## Addend == an arbitrary negative number.
- Addend: 0xFFFFCFC7 ## -12345
Type: R_386_NONE
## Addend == an arbitrary positive number.
- Addend: 0x12345
Type: R_386_NONE