This is an archive of the discontinued LLVM Phabricator instance.

Differential D28122

RuntimeDyldELF: implement R_AARCH64_PREL64 reloc
ClosedPublic

Authored by evgeny777 on Dec 27 2016, 7:22 AM.

Details

Reviewers
davide
lhames
Commits
rG8e32aebe80ac: RuntimeDyldELF: implement R_AARCH64_PREL64 reloc
rL291558: RuntimeDyldELF: implement R_AARCH64_PREL64 reloc
Summary

Also simplified the whole function a little.

Diff Detail

Repository
rL LLVM

Event Timeline

evgeny777 updated this revision to Diff 82538.Dec 27 2016, 7:22 AM
evgeny777 retitled this revision from to RuntimeDyldELF: implement R_AARCH64_PREL64 reloc.
evgeny777 updated this object.
evgeny777 added reviewers: davide, lhames.
evgeny777 set the repository for this revision to rL LLVM.
evgeny777 added subscribers: ikudrin, grimar, llvm-commits.
Herald added subscribers: rengolin, aemerson. · View Herald TranscriptDec 27 2016, 7:23 AM
davide edited edge metadata.Dec 28 2016, 7:42 AM

Please split this in two independent patches and I'll review both of them, i.e. please separate the functional changes from the NFC ones.

evgeny777 updated this revision to Diff 82594.Dec 28 2016, 8:15 AM
evgeny777 edited edge metadata.
evgeny777 removed rL LLVM as the repository for this revision.

Here is a patch with functional changes only. I'll post NFC changes when this one lands.

davide requested changes to this revision.Dec 28 2016, 8:55 AM
davide edited edge metadata.

If we support BE, then you should probably add a test which exercises this reloc for a big endian target.
Otherwise, just change that to be LE (if that's the case, ideally, we should change that it everywhere for R_AARCH64*)

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
378–380 ↗(On Diff #82594)

is it possible for this to be big-endian?
i.e. shouldn't this always be write64le() as we already do in lld?

This revision now requires changes to proceed.Dec 28 2016, 8:55 AM
evgeny777 added a comment.Dec 28 2016, 9:13 AM

is it possible for this to be big-endian?

Looks like aarch64_be is one of the targets supported by LLVM.
Are you saying that reloc values should still be written as little-endian on aarch64_be? If so who will convert them back to host byte order? CPU?

davide added a comment.Dec 28 2016, 9:14 AM
In D28122#631779, @evgeny777 wrote:

is it possible for this to be big-endian?

Looks like aarch64_be is one of the targets supported by LLVM.
Are you saying that reloc values should still be written as little-endian on aarch64_be? If so who will convert them back to host byte order? CPU?

No, on aarch64-be we should write the bytes in big-endian order. But you should add a test for aarch64_be (for this, and all the other untested BE

davide added a comment.Dec 28 2016, 9:15 AM
In D28122#631780, @davide wrote:
In D28122#631779, @evgeny777 wrote:

is it possible for this to be big-endian?

Looks like aarch64_be is one of the targets supported by LLVM.
Are you saying that reloc values should still be written as little-endian on aarch64_be? If so who will convert them back to host byte order? CPU?

No, on aarch64-be we should write the bytes in big-endian order. But you should add a test for aarch64_be (for this, and all the other untested BE

untested relocations on BE, if any (please add the tests in a separate commit/review to keep the diff readable).

evgeny777 updated this revision to Diff 82606.Dec 28 2016, 9:35 AM
evgeny777 edited edge metadata.

Ah, got it. Let's stick to LE then

davide added a comment.Dec 28 2016, 9:43 AM
In D28122#631780, @davide wrote:
In D28122#631779, @evgeny777 wrote:

is it possible for this to be big-endian?

Looks like aarch64_be is one of the targets supported by LLVM.
Are you saying that reloc values should still be written as little-endian on aarch64_be? If so who will convert them back to host byte order? CPU?

No, on aarch64-be we should write the bytes in big-endian order. But you should add a test for aarch64_be (for this, and all the other untested BE

untested relocations on BE, if any (please add the tests in a separate commit/review to keep the diff readable).

In D28122#631787, @evgeny777 wrote:

Ah, got it. Let's stick to LE then

ehm, that's not quite what I meant, sorry if it wasn't clear enough.What I'm suggesting is adding a test for BE for this relocation, and then audit all the other relocations not tested in aarch64-be configuration and add test for those.

The reason why lld uses write64le is that (I think) it supports only aarch64le so any aarch64be object will be rejected (or it's supposed to).
With your change you end up with a mix of endian-agnostic relocations and a relocation which will assume little-endian independently from the input.
If somebody passes a BE input that could result in a silent misapplication of a relocation, very painful to track down.

evgeny777 updated this revision to Diff 82657.Dec 29 2016, 2:32 AM
evgeny777 edited edge metadata.

I've added BE test for PREL64 and BE/LE tests for PREL32 (feature was there already, but wasn't tested)

So currently supported AArch64 relocs are:
ABS64: LE/BE
ABS32: LE/BE
PREL32: LE/BE
PREL64: LE/BE
CALL26/JUMP26 : LE
MOVW_UABS_G3: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G2_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G1_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G0_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
PREL_PG_HI21: LE (not tested)
ADD_ABS_LO12_NC: LE
LDST32_ABS_LO12_NC: LE (not tested)
LDST64_ABS_LO12_NC: LE (not tested)

Hope this helps

davide added a comment.Jan 2 2017, 6:05 AM
In D28122#632151, @evgeny777 wrote:

I've added BE test for PREL64 and BE/LE tests for PREL32 (feature was there already, but wasn't tested)

So currently supported AArch64 relocs are:
ABS64: LE/BE
ABS32: LE/BE
PREL32: LE/BE
PREL64: LE/BE
CALL26/JUMP26 : LE
MOVW_UABS_G3: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G2_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G1_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
MOVW_UABS_G0_NC: LE (BE test is incorrect: it passes, but checks for invalid values)
PREL_PG_HI21: LE (not tested)
ADD_ABS_LO12_NC: LE
LDST32_ABS_LO12_NC: LE (not tested)
LDST64_ABS_LO12_NC: LE (not tested)

Hope this helps

I think we should really clean up all this technical debt before adding other new relocations/features to the JIT.
I think you may want to send reviews for fixing the tests/bugs in the existing BE relocations, add tests for the missing ones and then we can land this one. Thanks for your cooperation.

evgeny777 added a comment.Jan 9 2017, 4:46 AM

I think you may want to send reviews for fixing the tests/bugs in the existing BE relocations

Sorry I've misinformed you: A64 instructions are always little-endian. The difference occurs only for data relocations: ABS32/64 and PREL32/64. I've added all missing test cases in r291438

evgeny777 updated this revision to Diff 83612.Jan 9 2017, 4:47 AM

Diff updated to reflect latest changes

davide accepted this revision.Jan 9 2017, 8:24 AM
davide edited edge metadata.

LGTM. Lang?

This revision is now accepted and ready to land.Jan 9 2017, 8:24 AM
Closed by commit rL291558: RuntimeDyldELF: implement R_AARCH64_PREL64 reloc (authored by evgeny777). · Explain WhyJan 10 2017, 3:16 AM
This revision was automatically updated to reflect the committed changes.
yuyichao added a subscriber: yuyichao.Jan 10 2017, 2:44 PM
yuyichao added inline comments.
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
353 ↗(On Diff #82594)

Sorry I've misinformed you: A64 instructions are always little-endian. The difference occurs only for data relocations: ABS32/64 and PREL32/64. I've added all missing test cases in r291438

FYI. I included this comment trying to avoid precisely this confusion......

Also FWIW, this was part of https://reviews.llvm.org/D27629 ......

Revision Contents

PathSize
llvm/
trunk/
lib/
ExecutionEngine/
RuntimeDyld/
3 lines
test/
ExecutionEngine/
RuntimeDyld/
AArch64/
4 lines
6 lines

Diff 83790

llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp

Show First 20 Lines • Show All 368 Lines • ▼ Show 20 Lines case ELF::R_AARCH64_ABS64:
break; break;
case ELF::R_AARCH64_PREL32: { case ELF::R_AARCH64_PREL32: {
uint64_t Result = Value + Addend - FinalAddress; uint64_t Result = Value + Addend - FinalAddress;
assert(static_cast<int64_t>(Result) >= INT32_MIN && assert(static_cast<int64_t>(Result) >= INT32_MIN &&
static_cast<int64_t>(Result) <= UINT32_MAX); static_cast<int64_t>(Result) <= UINT32_MAX);
write(isBE, TargetPtr, static_cast<uint32_t>(Result & 0xffffffffU)); write(isBE, TargetPtr, static_cast<uint32_t>(Result & 0xffffffffU));
break; break;
} }
case ELF::R_AARCH64_PREL64:
write(isBE, TargetPtr, Value + Addend - FinalAddress);
break;
case ELF::R_AARCH64_CALL26: // fallthrough case ELF::R_AARCH64_CALL26: // fallthrough
case ELF::R_AARCH64_JUMP26: { case ELF::R_AARCH64_JUMP26: {
// Operation: S+A-P. Set Call or B immediate value to bits fff_fffc of the // Operation: S+A-P. Set Call or B immediate value to bits fff_fffc of the
// calculation. // calculation.
uint64_t BranchImm = Value + Addend - FinalAddress; uint64_t BranchImm = Value + Addend - FinalAddress;
// "Check that -2^27 <= result < 2^27". // "Check that -2^27 <= result < 2^27".
assert(isInt<28>(BranchImm)); assert(isInt<28>(BranchImm));
▲ Show 20 Lines • Show All 1,300 Lines • Show Last 20 Lines

llvm/trunk/test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_BE-relocations.s

Show All 27 Lines# R_AARCH64_MOVW_UABS_G0_NC
.globl k .globl k
.p2align 3 .p2align 3
k: k:
.xword f .xword f
.size k, 8 .size k, 8
r: r:
# R_AARCH64_PREL32: use Q instead of f to fit in 32 bits. # R_AARCH64_PREL32: use Q instead of f to fit in 32 bits.
.word Q - . .word Q - .
# R_AARCH64_PREL64
.p2align 3
.xword f - .
# LE instructions read as BE # LE instructions read as BE
# rtdyld-check: *{4}(g) = 0x6024e0d2 # rtdyld-check: *{4}(g) = 0x6024e0d2
# rtdyld-check: *{4}(g + 4) = 0xe0acc8f2 # rtdyld-check: *{4}(g + 4) = 0xe0acc8f2
# rtdyld-check: *{4}(g + 8) = 0x6035b1f2 # rtdyld-check: *{4}(g + 8) = 0x6035b1f2
# rtdyld-check: *{4}(g + 12) = 0xe0bd99f2 # rtdyld-check: *{4}(g + 12) = 0xe0bd99f2
# rtdyld-check: *{8}k = f # rtdyld-check: *{8}k = f
# rtdyld-check: *{4}r = (Q - r)[31:0] # rtdyld-check: *{4}r = (Q - r)[31:0]
# rtdyld-check: *{8}(r + 8) = f - r - 8

llvm/trunk/test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s

Show First 20 Lines • Show All 41 Lines • ▼ Show 20 Lines# R_AARCH64_ADD_ABS_LO12_NC
.data .data
.globl k .globl k
.p2align 3 .p2align 3
k: k:
.xword f .xword f
.size k, 16 .size k, 16
r: r:
# R_AARCH64_PREL32: use Q instead of f to fit in 32 bits. # R_AARCH64_PREL32: use Q instead of f to fit in 32 bits.
.word Q - . .word Q - .
# R_AARCH64_PREL64
.p2align 3
.xword f - .
# rtdyld-check: *{4}(g) = 0xd2e02460 # rtdyld-check: *{4}(g) = 0xd2e02460
# rtdyld-check: *{4}(g + 4) = 0xf2c8ace0 # rtdyld-check: *{4}(g + 4) = 0xf2c8ace0
# rtdyld-check: *{4}(g + 8) = 0xf2b13560 # rtdyld-check: *{4}(g + 8) = 0xf2b13560
# rtdyld-check: *{4}(g + 12) = 0xf299bde0 # rtdyld-check: *{4}(g + 12) = 0xf299bde0
## Check LDST32_ABS_LO12_NC and LDST64_ABS_LO12_NC ## Check LDST32_ABS_LO12_NC and LDST64_ABS_LO12_NC
# rtdyld-check: (*{4}l)[21:10] = a[11:2] # rtdyld-check: (*{4}l)[21:10] = a[11:2]
# rtdyld-check: (*{4}(l+4))[21:10] = a[11:3] # rtdyld-check: (*{4}(l+4))[21:10] = a[11:3]
## Check ADR_PREL_PG_HI21. Low order bits of immediate value ## Check ADR_PREL_PG_HI21. Low order bits of immediate value
## go to bits 30:29. High order bits go to bits 23:5 ## go to bits 30:29. High order bits go to bits 23:5
# rtdyld-check: (*{4}p)[30:29] = (a - p + 20480)[13:12] # rtdyld-check: (*{4}p)[30:29] = (a - p + 20480)[13:12]
# rtdyld-check: (*{4}p)[23:5] = (a - p + 20480)[32:14] # rtdyld-check: (*{4}p)[23:5] = (a - p + 20480)[32:14]
# rtdyld-check: *{8}k = f # rtdyld-check: *{8}k = f
# rtdyld-check: *{4}r = (Q - r)[31:0] # rtdyld-check: *{4}r = (Q - r)[31:0]
# rtdyld-check: *{8}(r + 8) = f - r - 8
## f & 0xFFF = 0xdef (bits 11:0 of f) ## f & 0xFFF = 0xdef (bits 11:0 of f)
## 0xdef << 10 = 0x37bc00 ## 0xdef << 10 = 0x37bc00
# rtdyld-check: *{4}(a) = 0x9137bc00 # rtdyld-check: *{4}(a) = 0x9137bc00