Page MenuHomePhabricator

RuntimeDyldELF: implement R_AARCH64_PREL64 reloc
ClosedPublic

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

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

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?

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

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

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

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

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.

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
This revision was automatically updated to reflect the committed changes.
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 ......