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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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. |
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. |
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). |
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! |