ARM: support R_ARM_THM_JUMP19 relocation in THM stubs#1127
ARM: support R_ARM_THM_JUMP19 relocation in THM stubs#1127sai18022001 wants to merge 2 commits intoqualcomm:mainfrom
Conversation
| case llvm::ELF::R_ARM_THM_CALL: | ||
| case llvm::ELF::R_ARM_THM_JUMP19: { | ||
| if (m_Target->isJ1J2BranchEncoding()) | ||
| return llvm::isInt<THM2_MAX_BRANCH_BITS>(Offset); |
There was a problem hiding this comment.
JUMP19 is 21 bits, not 23 or 25.
There was a problem hiding this comment.
Will fix R_ARM_THM_JUMP19 to use isInt<21> instead of reusing THM2_MAX_BRANCH_BITS
| case llvm::ELF::R_ARM_THM_CALL: | ||
| case llvm::ELF::R_ARM_THM_JUMP19: { | ||
| if (m_Target->isJ1J2BranchEncoding()) | ||
| return llvm::isInt<THM2_MAX_BRANCH_BITS>(Offset); |
| @@ -0,0 +1,13 @@ | |||
| #---THMJump19Veneer.test--------- Executable --------------------------# | |||
There was a problem hiding this comment.
We should test the veneer case (both thm-thm and thm-arm) too. I think this test does not produce veneers since everything appears to be in range.
There was a problem hiding this comment.
Understood, will add test cases that force veneer creation by placing caller and target out of range, covering both THMtoTHM and THMtoARM cases.
| @@ -0,0 +1,5 @@ | |||
| extern int my_func(int x); | |||
There was a problem hiding this comment.
The bug will reproduce without this file. You can do --entry=my_func. I think link.ld can also be dropped since this particular issue should reproduce without a linker script, but you will probably need one when testing the stub cases. Then this test could be a single .s file (test.s) with no Inputs dir.
There was a problem hiding this comment.
Good point, will simplify to a single .s file with --entry=my_func and drop main.c and link.ld for the basic case.
There was a problem hiding this comment.
After looking at existing tests in the repo, they all use an Inputs/ directory for source files. I'll follow the same convention and keep the Inputs/ dir, but will simplify to a single .s file with --entry=my_func, dropping main.c. The linker script will still be needed to force out-of-range for the veneer test cases.
There was a problem hiding this comment.
There are many examples of single-input assembly tests in eld, such as ARM/standalone/Relocs/arm-adr-addend.s for example.
There was a problem hiding this comment.
Thanks for the example. I've adopted the single .s file pattern with RUN: and CHECK: as inline comments. However, I tried using --section-start to place sections far apart (to force out-of-range for veneer creation) but it doesn't seem to be honored by ELD without a linker script , both sections end up placed sequentially regardless. So I'll need an Inputs/ dir for the linker script for the veneer test cases.
Do you have a preferred way to force out-of-range section placement without a linker script in ELD?
There was a problem hiding this comment.
The veneer/stub case should probably be a separate test that will most likely need a linker script.
| // supported yet. For non-PLT targets (plain labels in Thumb code | ||
| // without .thumb_func), proceed with the relocation. | ||
| if (pReloc.symInfo()->reserved() & Relocator::ReservePLT) { | ||
| DiagEngine->raise(Diag::unsupport_cond_branch_reloc) << (int)pReloc.type(); |
There was a problem hiding this comment.
Probably this needs to be tested at the time PLT is reserved ?
The diagnostics will need to always point to the input object file, section and symbol.
Please add a test for this.
| // This stub cannot be used for ARM target. | ||
| if ((pTargetValue & 0x1) == 0) | ||
| if ((pTargetValue & 0x1) == 0 && | ||
| pReloc->type() != llvm::ELF::R_ARM_THM_JUMP19) |
There was a problem hiding this comment.
What if R_ARM_THM_JUMP24 ?
There was a problem hiding this comment.
I've updated THMToTHMStub::isNeeded to also exempt R_ARM_THM_JUMP24 alongside R_ARM_THM_JUMP19 when T=0:
if ((pTargetValue & 0x1) == 0 &&
pReloc->type() != llvm::ELF::R_ARM_THM_JUMP19 &&
pReloc->type() != llvm::ELF::R_ARM_THM_JUMP24)
return false;
| RUN: %clang %clangopts %p/Inputs/main.c -o %t.main.o -c --target=arm-none-eabi -mcpu=cortex-m0plus -mfloat-abi=soft | ||
| RUN: %link %linkopts %t.test.o %t.main.o -o %t.out -T %p/Inputs/link.ld -m armelf --entry=main -static 2>&1 | ||
| RUN: %readelf -s %t.out | %filecheck %s | ||
| #CHECK: plain_entry |
There was a problem hiding this comment.
Need test for veneers to make sure proper veneers are generated
There was a problem hiding this comment.
Still working on this with @quic-areg. I'm trying to force out-of-range section placement to trigger veneer creation, but --section-start doesn't seem to be honored by ELD without a linker script. Will add the veneer test cases once I figure out the right approach for section placement.
There was a problem hiding this comment.
What I had in mind is to have two tests: one testing the original case mentioned in the ticket (which you already have, and does not need a linker script) and another testing veneers, which will most likely need a linker script (I mentioned this in a separate comment).
--section-startdoesn't seem to be honored by ELD without a linker script
Do you have an example? It should work without one AFAIK.
There was a problem hiding this comment.
@quic-seaswara @quic-areg
Thanks for clarifying! I tested it but the map shows .text.target placed at 0xa (sequentially after .text.caller) instead of 0x200000:
.text.caller 0x0 0xa
.text.target 0xa 0x4
Command used:
ld.eld test.o --entry=my_func -static --section-start=.text.caller=0x0 --section-start=.text.target=0x200000 Map=test.map
Am I using it incorrectly, or is there something else I should try?
There was a problem hiding this comment.
@quic-seaswara @quic-areg Thanks for clarifying! I tested it but the map shows
.text.targetplaced at0xa(sequentially after.text.caller) instead of0x200000:.text.caller 0x0 0xa .text.target 0xa 0x4Command used:
ld.eld test.o --entry=my_func -static --section-start=.text.caller=0x0 --section-start=.text.target=0x200000 Map=test.mapAm I using it incorrectly, or is there something else I should try?
Yes, you are using incorrectly, the --section-start linker command line option will work only on output sections, not input sections.
Fixes: qualcomm#1005 Signed-off-by: Sai Sanjay Chikne <saichikne180201@gmail.com>
Signed-off-by: Sai Sanjay Chikne <saichikne180201@gmail.com>
4573840 to
6d37e02
Compare
Fixes #1005
Problem
R_ARM_THM_JUMP19(conditional branch) failed for static ARM targets like Cortex-M0plus / Cortex-M33 with:This happened because the stub factory did not recognize
R_ARM_THM_JUMP19, and the relocator unconditionally rejected T=0 targets even for non-PLT symbols (plain Thumb labels without.thumb_func).Changes
R_ARM_THM_JUMP19toTHMToTHMStubandTHMToARMStubusing correct 21-bit range validation (isInt<21>)scanGlobalReloc)ARMRelocator.cppto allowT=0targets forJUMP19when the symbol is a non-PLT Thumb labelTest
Added tests in
test/ARM/standalone/THMJump19Veneer/:thm-jump19-basic.s,thm-jump19-veneer.s,thm-jump19-plt-diag.s