[release/11.0-preview1] Revert "Bring a few jithelpers to new unwind plan (#123307)" #123715
Conversation
…plan (dotnet#123307)" This reverts commit 18acc2d.
|
Backport of #123696 to release/11.0-preview1 |
|
Tagging subscribers to this area: @mangod9 |
|
@akoeplinger Is it fine to submit this via dotnet/runtime or does it need to be submitted via dotnet/dotnet directly? |
There was a problem hiding this comment.
Pull request overview
Reverts commit 18acc2d8f708b72420de1410c954d2984312faa2 (“Bring a few jithelpers to new unwind plan”), restoring prior unwind/context-handling behavior for several JIT helpers and architectures.
Changes:
- Removes
ClrRestoreNonvolatileContextWorkerimplementations and related asm constants for ARM64/LoongArch64/RISC-V64. - Reworks OSR patchpoint transition context setup to use captured/unwound context rather than building a
CONTEXTfrom theTransitionBlock. - Simplifies marked-JIT-helper detection/unwind path and removes now-unused prolog/epilog helper macros.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/threads.cpp | Limits ClrRestoreNonvolatileContextWorker usage to AMD64 and falls back to RtlRestoreContext elsewhere. |
| src/coreclr/vm/riscv64/asmhelpers.S | Removes RISC-V64 ClrRestoreNonvolatileContextWorker and tweaks patchpoint comment. |
| src/coreclr/vm/riscv64/asmconstants.h | Removes CONTEXT offset/flag constants no longer needed after worker removal. |
| src/coreclr/vm/loongarch64/asmhelpers.S | Removes LoongArch64 ClrRestoreNonvolatileContextWorker and tweaks patchpoint comment. |
| src/coreclr/vm/loongarch64/asmconstants.h | Removes CONTEXT offset/flag constants no longer needed after worker removal. |
| src/coreclr/vm/jithelpers.cpp | Changes OSR patchpoint transition to capture/unwind context (instead of TransitionBlock-based construction). |
| src/coreclr/vm/frames.h | Removes SoftwareExceptionFrame::UpdateContextForOSRTransition declaration. |
| src/coreclr/vm/exceptmacros.h | Adds UnwindAndContinueResumeAfterCatch declaration for interpreter builds. |
| src/coreclr/vm/excep.cpp | Refactors marked-JIT-helper identification and unwind logic; adds UnwindAndContinueResumeAfterCatch implementation. |
| src/coreclr/vm/arm64/asmmacros.h | Removes ARM64 epilog macro that restored FP callee-saved regs and returned. |
| src/coreclr/vm/arm64/asmhelpers.asm | Removes ARM64 ClrRestoreNonvolatileContextWorker; patchpoint comment tweak. |
| src/coreclr/vm/arm64/asmhelpers.S | Removes ARM64 ClrRestoreNonvolatileContextWorker; patchpoint comment tweak. |
| src/coreclr/vm/arm64/asmconstants.h | Removes CONTEXT offset/flag constants no longer needed after worker removal. |
| src/coreclr/vm/amd64/AsmMacros.inc | Adjusts PUSH_COOP_PINVOKE_FRAME_WITH_FLOATS layout and removes its “return” epilog macro. |
| src/coreclr/vm/amd64/AsmHelpers.asm | Switches JIT_Patchpoint to PROLOG_WITH_TRANSITION_BLOCK / EPILOG_WITH_TRANSITION_BLOCK_RETURN. |
| src/coreclr/pal/inc/unixasmmacrosriscv64.inc | Removes POP_COOP_PINVOKE_FRAME_WITH_FLOATS_RETURN macro. |
| src/coreclr/pal/inc/unixasmmacrosloongarch64.inc | Removes POP_COOP_PINVOKE_FRAME_WITH_FLOATS_RETURN macro. |
| src/coreclr/pal/inc/unixasmmacrosarm64.inc | Removes POP_COOP_PINVOKE_FRAME_WITH_FLOATS_RETURN macro. |
| pFrameContext->Rbp = currentFP; | ||
| #endif // TARGET_AMD64 | ||
|
|
||
| SetSP(pFrameContext, currentSP); |
There was a problem hiding this comment.
In this OSR transition setup, the context is unwound with RtlVirtualUnwind and then SP is overwritten back to the original method’s SP. On non-AMD64 targets, FP is captured (currentFP) but never restored, so after RtlVirtualUnwind the FP register in pFrameContext will still reflect the caller frame while SP is set to the callee frame. This mismatch can break stack walking and any FP-based addressing in the OSR method. Consider restoring FP for the non-AMD64 targets as well (via SetFP/GetFP helpers), or remove the FP capture/comment if FP is intentionally not part of the transition contract.
| SetSP(pFrameContext, currentSP); | |
| SetSP(pFrameContext, currentSP); | |
| #if !defined(TARGET_AMD64) | |
| SetFP(pFrameContext, currentFP); | |
| #endif |
| #if defined(TARGET_AMD64) && defined(_DEBUG) | ||
| CHECK_WRITE_BARRIER_RANGE(JIT_WriteBarrier_Debug) | ||
| CHECK_RANGE(JIT_WriteBarrier_Debug) | ||
| #endif |
There was a problem hiding this comment.
CHECK_RANGE is defined as a preprocessor macro inside IsIPInMarkedJitHelper but never undefined. This leaks a very generic macro name into the rest of the translation unit and can cause accidental collisions later in the file or with future includes/changes. Please #undef CHECK_RANGE after the last use (mirroring the prior #undef CHECK_WRITE_BARRIER_RANGE pattern).
| #endif | |
| #endif | |
| #undef CHECK_RANGE |
|
|
||
| #if defined(TARGET_AMD64) | ||
| // If calls push the return address, we need to simulate that here, so the OSR | ||
| // method sees the "expected" SP misalgnment on entry. |
There was a problem hiding this comment.
Minor typo in comment: “misalgnment” -> “misalignment”.
| // method sees the "expected" SP misalgnment on entry. | |
| // method sees the "expected" SP misalignment on entry. |
|
@jkotas it's fine to do it in runtime right now |
|
/ba-g infrastructure timeouts |
jkotas
merged commit ed49df7
into
dotnet:release/11.0-preview1
2 participants
Backport of #123696 to release/11.0-preview1
/cc @janvorli
Customer Impact
Very frequent intermittent runtime crashes caused by buggy CPU context manipulations.
Regression
Regression introduced by #123307, merged on Thu Jan 22, 2026.
Testing
Default CI run.
Risk
Low, clean revert of the offending PR.