This is an archive of the discontinued LLVM Phabricator instance.

RuntimeDyldELF: add support for R_AARCH64_ADD_ABS_LO12_NC
ClosedPublic

Authored by evgeny777 on Dec 26 2016, 5:27 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

evgeny777 updated this revision to Diff 82496.Dec 26 2016, 5:27 AM
evgeny777 retitled this revision from to RuntimeDyldELF: add support for R_AARCH64_ADD_ABS_LO12_NC.
evgeny777 updated this object.
evgeny777 added reviewers: davide, lhames.
evgeny777 set the repository for this revision to rL LLVM.
evgeny777 added subscribers: grimar, ikudrin, llvm-commits.
davide edited edge metadata.Dec 26 2016, 7:20 AM

The code looks correct, but I left a comment inline.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
460–464 ↗(On Diff #82496)

This is correct, but while you're here, I'd rather see if you can have a common helper for all the relocations updating immediates in AArch64 instructions (e.g., ldr, str) as we do in lld (see or32AArch64Imm).

evgeny777 updated this revision to Diff 82510.Dec 26 2016, 9:04 AM
evgeny777 edited edge metadata.
evgeny777 removed rL LLVM as the repository for this revision.

Addressed review comments.
Now lld and RuntimeDyldELF uses two identical sets of helper functions, so it probably makes sense to move them to header file and provide patch for lld.

This looks much better now. Please split in two patches. The first one which does the refactoring, the second one which adds the new functionality. @lhames , what do you think?

Minor comment inline.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
42–45 ↗(On Diff #82510)

Prefer the ternary operator here:
return (isBE) ? write<T, support::big>(...) : write<T, support:little>(...);

Addressed review comments.
Now lld and RuntimeDyldELF uses two identical sets of helper functions, so it probably makes sense to move them to header file and provide patch for lld.

Maybe. I have no opinions on the topic (bluntly, I don't care), but there have been discussions recently about sharing code between the JIT and the static linker. You may want to check with the involved parties before going on, but I think it makes sense to do this as a follow-up.

So, can it be pushed as is?

So, can it be pushed as is?

Yes, but please split the NFC changes from the functionality changes first.

evgeny777 added inline comments.Dec 26 2016, 9:57 AM
lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
42–45 ↗(On Diff #82510)

Ok, but it's actually void.

Do you propose to push extra reloc first and than refactoring as a subsequent patch?

Do you propose to push extra reloc first and than refactoring as a subsequent patch?

Either way.

This revision was automatically updated to reflect the committed changes.