Repository navigation
[RISC-V] Fix jump stubs and emit direct calls PC-relative - #135610
pcshrosbree wants to merge 4 commits into
Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
Bot
added
the
community-contribution
|
Azure Pipelines: Successfully started running 7 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
8c9c79f to
34f1465
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The architecture-specific relocation, retry, and register behavior has not been executed on RISC-V hardware.
0 open findings
What changed in this PR
Fixes RISC-V jump-stub generation and makes direct calls consistently PC-relative.
Changes:
- Corrects jump-stub allocation and uses
t6without corruptingra. - Emits direct calls as relocated
auipc+jalrpairs. - Adds retry accounting and ForceRelocs coverage.
Holistic assessment: The approach matches established architectures, but requires RISC-V hardware validation.
| File | Description |
|---|---|
ForceRelocsJumpStubs.csproj |
Configures relocation stress testing. |
ForceRelocsJumpStubs.cs |
Exercises representative call shapes. |
riscv64/cgencpu.h |
Corrects jump-stub encoding. |
riscv64/asmhelpers.S |
Documents t6 scratch usage. |
jitinterface.cpp |
Fixes allocation and reserve accounting. |
lsrariscv64.cpp |
Removes obsolete target register allocation. |
lower.cpp |
Updates direct-call lowering documentation. |
emitriscv64.cpp |
Always emits relocated PC-relative calls. |
codegenriscv64.cpp |
Configures direct calls without target registers. |
🧠 Review effort: Balanced
…ra out of it The inner `PCODE jumpStubAddr` in the RISC-V case of CEEJitInfo::recordRelocation shadowed the outer one, so the stub that ExecutionManager::jumpStub returned was dropped, the method was recompiled with rel32 off, and later compilations in the process got no rel32 hint and emitted their direct calls in the absolute form. The stub itself used ra as its scratch register, so a callee reached through it would have returned to its own entry. Use t6, which no call contract passes across a call: t0/t1 carry the profiler-enter arguments, t2 the secret stub parameter and the dispatch token, t3/t4 the write barrier arguments and t5 the indirection cell. The profiler helpers' entry comment listed t6 as an input; it is overwritten before any read, so correct the comment.
28bf47b to
33ff0b3
Compare
|
Rebased onto main after #135411. The only conflict was the |
| // A direct call: auipc + jalr with a relocation. No register is needed for the target. | ||
| params.callType = EC_FUNC_TOKEN; | ||
| params.ireg = internalRegisters.GetSingle(call); | ||
| params.ireg = params.isJump ? rsGetRsvdReg() : REG_RA; |
There was a problem hiding this comment.
I remember we wanted to phase out rsGetRsvdReg, on RISC-V it's just another temp register which currently LSRA can't allocate. Would be great not to add its uses.
There was a problem hiding this comment.
Done in 9d2a4ee. The direct call no longer sets ireg. The emitter does not read it for EC_FUNC_TOKEN: it picks the auipc/jalr registers itself (ra for a call, t2 for a tail call). So the assert at the top of emitIns_Call now requires ireg only for EC_INDIR_R, and the codegen assert moved into the indirect branch.
The same params.isJump ? rsGetRsvdReg() : REG_RA line is on main in three other places (genCodeForReturnTrap, the direct-call path of genCallInstruction and genJumpToThrowHlpBlk_la), from #119203. I can drop those in a follow-up, or here if you prefer.
Note
This reply was prepared with AI assistance (Anthropic Claude, checked by OpenAI Codex) at my request. I reviewed it before posting.
…ump stubs emitIns_Call consulted the relocation hint for direct calls and took an absolute li + jalr form when the hint said no. amd64 and arm64 always record a relocation for a direct call and rely on the VM to insert a jump stub when the target is out of reach; do the same here now that the RISC-V stub works. IsAddressInRange stays for the data-address emitters. This removes the retry's relocation-free fallback: before, a failed stub allocation recompiled the method with rel32 off and every call in the absolute form, so the second attempt always converged. Now the retry asks for stubs again with m_reserveForJumpStubs raised, as amd64 and arm64 do, and g_fAllowRel32 only affects data addressing. A contained constant call target no longer needs a register, so drop the internal register LSRA defined for it and the codegen read of it, and fix the lowering comment that described the old li + jalr form. Codegen leaves ireg unset for this direct call, since the emitter chooses the auipc and jalr registers itself, and emitIns_Call requires ireg only for an indirect call.
Before this PR, ForceRelocs alone could not reach the RISC-V jump stub: the first pass presets m_fJumpStubOverflow, and the retry emitted direct calls as li + jalr with no relocation. Now that the JIT records a relocation for every direct call (previous commit), the ForceRelocs address scatter in ClrVirtualAlloc, which applies on RISC-V because W^X is off there by default, can put helper and cross-heap targets out of range on the retry, so those calls go through jump stubs. Add src/tests/baseservices/jumpstubs, which runs four call shapes (allocation and write barrier, a CoreLib call, a throw caught by the caller, a plain managed call) with DOTNET_ForceRelocs=1 and tiered compilation off.
recordRelocation added to m_reserveForJumpStubs only when it also tried to allocate a stub, so once m_fJumpStubOverflow was set for a compilation, by a failed allocation or by the ForceRelocs preset, the remaining out-of-range calls in that pass were left out of the estimate the retry reserves with; with the preset the estimate was empty. arm64 keeps counting after the flag is set, and so does amd64 once rel32 is restricted to code targets. Move the estimate out of the !m_fJumpStubOverflow guard; the allocation stays behind it. A data relocation out of range still only forces the retry, where the JIT addresses data absolutely, so it is still not counted.
33ff0b3 to
80d2714
Compare
Fixes #135607.
The RISC-V jump stub path in
CEEJitInfo::recordRelocationallocated a stub and then dropped it: the innerPCODE jumpStubAddrshadowed the outer one (since #119203), so every out-of-range call relocation setm_fJumpStubOverflow, the method was recompiled withg_fAllowRel32 = FALSE, and later compilations in the process got no rel32 hint and emitted their direct calls in the absoluteli+jalrform.The stub itself was also wrong.
emitBackToBackJumpusedraas its scratch register (auipc ra; ld ra, 16(ra); jalr x0, ra), so a callee reached through it would have returned to its own entry. It now usest6: of the temporaries,t0/t1carry the profiler-enter arguments,t2the secret stub parameter and the dispatch token,t3/t4the write barrier arguments andt5the indirection cell;t6isREG_OPT_RSVDin the JIT and the write barrier already declares it trashed.Four commits, so that the first one stands on its own:
t6instead ofra.auipc+jalrwith a relocation, as on amd64 (genCodeAddrNeedsRelocis unconditionally true there) and arm64 (the emitter always records the relocation forbl); the VM inserts a jump stub when the target is out of reach.getRelocTypeHintandIsAddressInRangestay for thePCREL_I/PCREL_Sdata relocations, which is all that disablingg_fAllowRel32affects from now on. The internal register LSRA reserved for a contained constant call target goes with the old form. This commit widens what goes through stubs from "the hint was wrong" to "every far call", in the same change that makes the stubs work for the first time; if you would rather take it after a hardware run of the first commit, drop commits 2 and 3 and I will open them separately.src/tests/baseservices/jumpstubsruns four call shapes (an allocation stored through the write barrier, a CoreLib call, a throw caught by the caller, and a plain managed call) withDOTNET_ForceRelocs=1. Before this PR, ForceRelocs alone could not reach the RISC-V stub: the first pass presetsm_fJumpStubOverflow, and the retry emitted direct calls asli+jalrwith no relocation. Now that every direct call records a relocation (commit 2), the ForceRelocs address scatter inClrVirtualAlloc, which applies on RISC-V because W^X is off there by default, can put helper and cross-heap targets out of range on the retry, so those calls go through stubs.m_fJumpStubOverflowpreset (ForceRelocs) or hit a failed allocation stopped counting, so the retry used an incomplete estimate.Validation: cross-built
clr.runtimefor linux-riscv64 (Checked) in theazurelinux-4.0-net11.0-cross-riscv64image; built and ran the new test on linux-x64 (Checked) underDOTNET_ForceRelocs=1, to check the test itself. On x64 underDOTNET_ForceRelocs=1, turning W^X off (DOTNET_EnableWriteXorExecute=0, the RISC-V default) was measured to raise the jump stubs created during the test from 13 to 20 (0 without ForceRelocs) and to place the write-barrier copy and the precodes in windows more than 2 GiB from the jitted code. Nothing here was run on RISC-V. I have no hardware, and the linux_riscv64 job in this repository only builds. The intended check is the new test on a Checked RISC-V runtime: with the assignment fix, the JIT change andDOTNET_ForceRelocs=1, restoring the oldra-based encoder should make it crash or hang, and this change should make it pass.Note
This PR was prepared with AI assistance (Anthropic Claude, checked by OpenAI Codex) at my request. I reviewed the change before opening it.