Revert "Bring a few jithelpers to new unwind plan" #123696
Conversation
This reverts commit 18acc2d.
|
Tagging subscribers to this area: @mangod9 |
There was a problem hiding this comment.
Pull request overview
Reverts #123307 by backing out the prior “new unwind plan” changes around JIT helpers / patchpoints, returning several helpers to older prolog/epilog and unwind mechanisms.
Changes:
- Remove non-AMD64
ClrRestoreNonvolatileContextWorkerimplementations and their asm constants on ARM64/LoongArch64/RISC-V64. - Rework
JIT_PatchpointWorkerWorkerWithPolicyto capture context + unwind viaRtlCaptureContext/virtual unwind instead of building context fromTransitionBlock. - Simplify marked JIT-helper detection/unwind logic and add interpreter-only resume-after-catch unwind entrypoint.
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 | Restricts ClrRestoreNonvolatileContextWorker usage to TARGET_AMD64 and falls back to RtlRestoreContext elsewhere. |
| src/coreclr/vm/riscv64/asmhelpers.S | Removes ClrRestoreNonvolatileContextWorker; minor comment adjustment in JIT_Patchpoint. |
| src/coreclr/vm/riscv64/asmconstants.h | Removes CONTEXT offset/flag constants that supported the removed restore worker. |
| src/coreclr/vm/loongarch64/asmhelpers.S | Removes ClrRestoreNonvolatileContextWorker; minor comment adjustment in JIT_Patchpoint. |
| src/coreclr/vm/loongarch64/asmconstants.h | Removes CONTEXT offset/flag constants that supported the removed restore worker. |
| src/coreclr/vm/jithelpers.cpp | Reworks OSR patchpoint context capture/unwind and stack/frame restoration logic. |
| src/coreclr/vm/frames.h | Removes OSR TransitionBlock-to-CONTEXT helper declaration. |
| src/coreclr/vm/exceptmacros.h | Adds interpreter-only UnwindAndContinueResumeAfterCatch prototype. |
| src/coreclr/vm/excep.cpp | Refactors marked JIT helper detection; updates unwind path; adds interpreter resume-after-catch implementation; adjusts TransitionBlock layout comment/offset. |
| src/coreclr/vm/arm64/asmmacros.h | Removes POP_COOP_PINVOKE_FRAME_WITH_FLOATS_RETURN macro. |
| src/coreclr/vm/arm64/asmhelpers.asm | Removes ClrRestoreNonvolatileContextWorker; minor comment adjustment in JIT_Patchpoint. |
| src/coreclr/vm/arm64/asmhelpers.S | Removes ClrRestoreNonvolatileContextWorker; minor comment adjustment in JIT_Patchpoint. |
| src/coreclr/vm/arm64/asmconstants.h | Removes CONTEXT offset/flag constants that supported the removed restore worker. |
| src/coreclr/vm/amd64/AsmMacros.inc | Changes PUSH_COOP_PINVOKE_FRAME_WITH_FLOATS layout/size and removes the corresponding “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. |
| // Remember original method FP and SP because new method will inherit them. | ||
| UINT_PTR currentSP = GetSP(pFrameContext); | ||
| UINT_PTR currentFP = GetFP(pFrameContext); | ||
|
|
There was a problem hiding this comment.
currentFP is captured from the original managed frame, but it is only written back into the context on TARGET_AMD64 (pFrameContext->Rbp = currentFP). On other architectures (e.g., ARM64/LoongArch64/RISC-V64) the context is left with the FP value produced by the unwind to the caller frame, which can break OSR since the OSR method is supposed to inherit the original method’s frame pointer. Consider restoring the frame pointer for all architectures (e.g., via the existing SetFP helper or per-arch FP field) after the caller unwind, in the same way SP is restored.
| #define CHECK_RANGE(name) \ | ||
| if (GetEEFuncEntryPoint(name) <= uControlPc && uControlPc < GetEEFuncEntryPoint(name##_End)) return true; |
There was a problem hiding this comment.
CHECK_RANGE is defined as a preprocessor macro inside IsIPInMarkedJitHelper but never undefined. Because macros are file-scoped, this leaks the macro name into the rest of excep.cpp and can cause collisions or surprising substitutions later. Add a corresponding #undef CHECK_RANGE after the last use (before leaving the #ifndef FEATURE_PORTABLE_HELPERS block).
| PCODE ControlPCPostAdjustment = GetIP(pContext) - STACKWALK_CONTROLPC_ADJUST_OFFSET; | ||
|
|
||
| // Now we save the address back into the context so that it gets used | ||
| // as the faulting address. | ||
| SetIP(pContext, ControlPCPostAdjustment); |
There was a problem hiding this comment.
The indentation of this block is inconsistent with surrounding code (these lines are indented less than the surrounding #if body), which makes the control flow harder to scan. Re-indent to match the prevailing indentation in this function.
|
Makes sense, let's revert and bring it back once the issue is sorted out. |
|
Fine by me. There is no clear repro. That DotnetFuzzing is also not reproducing it for me (that app doesn't find download directory; application issue..). |
Reverts #123307