Page MenuHomePhabricator

Fix R_AARCH64_MOVW_UABS_G3 relocation
ClosedPublic

Authored by yuyichao on Dec 8 2016, 7:53 PM.

Details

Summary

The relocation is missing mask so an address that has non-zero bits in 47:43 may overwrite the register number. (Frequently shows up as target register changed to xzr....)

Diff Detail

Repository
rL LLVM

Event Timeline

yuyichao updated this revision to Diff 80860.Dec 8 2016, 7:53 PM
yuyichao retitled this revision from to Fix R_AARCH64_MOVW_UABS_G3 relocation.
yuyichao updated this object.
yuyichao added a reviewer: t.p.northover.
yuyichao added a subscriber: llvm-commits.
yuyichao updated this revision to Diff 80866.Dec 8 2016, 9:00 PM
yuyichao updated this object.

Tests added and endianess issue fixed.

yuyichao added inline comments.Dec 8 2016, 9:09 PM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
364 ↗(On Diff #80866)

Now that the diff is much bigger I should point out that this line is the actual bug fix (adding mask). Other changes are necessary so that the added test can be run without causing trouble on big endian systems.

yuyichao updated this revision to Diff 81225.Dec 13 2016, 6:23 AM

Use target endian for data relocation.

davide added a subscriber: davide.Dec 14 2016, 3:53 PM
davide added inline comments.
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
466–467 ↗(On Diff #81225)

(10 - 2) -> 8

480–481 ↗(On Diff #81225)

any reason why (10 - 3) is not spelled 7 (here and everywhere else)

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s
1–2 ↗(On Diff #81225)

We need tests for both little and big endian, no?

9–15 ↗(On Diff #81225)

Can you add comments pointing out the relocations emitted?

yuyichao added inline comments.Dec 14 2016, 4:02 PM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
480–481 ↗(On Diff #81225)

I believe it's because these are the last bits of the source (3) and the destination (10). I'm fine with either but I think the 10 - 3 is reasonably clear what it is doing along with the comment above each of these.

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s
1–2 ↗(On Diff #81225)

Sure. I don't have hardware to actually check if it's actually the correct output but I'll just reference clang/gcc output in be mode.

9–15 ↗(On Diff #81225)

Sorry, what exactly do you mean? The relocations are explicitly specified in the code (#:abs_g3: etc.). Do you mean to write the name like R_AARCH64_MOVW_UABS_G3? Or something else?

yuyichao added inline comments.Dec 14 2016, 4:20 PM
test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s
9–15 ↗(On Diff #81225)

Or do you mean the disassemble of the relocated instructions?

I don't have additional comments but please wait for Tim/Lang to sign this off.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
480–481 ↗(On Diff #81225)

Fine.

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s
9–15 ↗(On Diff #81225)

The former (spelling the as R_AARCH64_FOO)

(and add the required tests please)

(and add the required tests please)

Sure.

test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_relocations.s
9–15 ↗(On Diff #81225)

OK. Will do.

yuyichao updated this revision to Diff 81522.Dec 14 2016, 7:54 PM

Tests added. Also discovered and had to fix two other bugs since the arch detection was wrong for aarch64_be ELF file....

lhames accepted this revision.Dec 15 2016, 9:31 AM
lhames edited edge metadata.

LGTM. Thanks Yichao. :)

This revision is now accepted and ready to land.Dec 15 2016, 9:31 AM
This revision was automatically updated to reflect the committed changes.