This is an archive of the discontinued LLVM Phabricator instance.

[lld][ELF][ARM] Add veneer generation to branch instructions
ClosedPublic

Authored by denis-protivensky on Feb 9 2015, 6:53 AM.

Details

Summary

When calling ARM code from Thumb and vice versa, a veneer that switches instruction set should be generated.
Added veneer generation for ARM_CALL, ARM_JUMP24 & analogous Thumb instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

denis-protivensky retitled this revision from to [lld][ELF][ARM] Add veneer generation to branch instructions.
denis-protivensky updated this object.
denis-protivensky edited the test plan for this revision. (Show Details)
denis-protivensky added a project: lld.
denis-protivensky added a subscriber: Unknown Object (MLST).
shankarke accepted this revision.Feb 9 2015, 9:24 AM
shankarke added a reviewer: shankarke.
shankarke added a subscriber: shankarke.

Other than the below comment, it LGTM.

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
33–113 ↗(On Diff #19579)

Can you move this into an anonymous namespace.

This revision is now accepted and ready to land.Feb 9 2015, 9:24 AM
ruiu accepted this revision.Feb 9 2015, 10:30 AM
ruiu edited edge metadata.

LGTM

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
40 ↗(On Diff #19579)

The address is four byte right? I'd move this 0x00 and the comment to end of the previous line.

wnewton accepted this revision.Feb 9 2015, 5:44 PM
wnewton edited edge metadata.
wnewton added inline comments.
lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
157 ↗(On Diff #19579)

It seems like it might be simpler not to pass R_*_CALL relocations into this function at all then we could remove the handling here and in the switch statement below (but retain a comment about ARMv5).

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
33–113 ↗(On Diff #19579)

Yep, missed that.

40 ↗(On Diff #19579)

Okay.

157 ↗(On Diff #19579)

Will, I'm not sure I understand the idea. I take value of relocation from the reference object, and I cannot but pass it into the function as it's used in couple more places.
If I change comparison here and the switch down the code, I'll need to pass couple of flags into the function: isThumbCode, isCall and maybe more.
Could you please explain your thought in more detail?

wnewton added inline comments.Feb 10 2015, 12:25 AM
lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
157 ↗(On Diff #19579)

Here we just return if we find kindValue is R_ARM_CALL or R_ARM_THM_CALL but then we test kindValue again lower down in the switch where these values can now never occur. handleVeneer is only called in one place and in that switch we explicitly call it for R_ARM_CALL and R_ARM_THM_CALL even though handleVeneer seems to be an effective noo-op in those cases.

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
157 ↗(On Diff #19579)

Okay, I got what confused you. My intention was to form fully-working skeleton code with all checks needed. Further, when architecture parsing option is added, it'll be simpler to just walk through all TODO records and add the corresponding checks (and the tests, of course).
Do you find these stubs excessive?

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
157 ↗(On Diff #19579)

I thought a bit more about what you said, and I agree that these checks for ARM_*_CALL should be removed. At least because they're not covered with tests and do not bring any new functionality yet. I'll leave a TODO mark instead as you proposed. Thanks!

This revision was automatically updated to reflect the committed changes.