Fix interpreter ResumeAfterCatch on arm64 Unix #122504
Conversation
The DispatchManagedException and DispatchRethrownManagedException were marked as NORETURN. That caused a problem when the C++ compiler has optimized the invocation of such methods so that it has removed its epilog and the return address was in a completely different function. The Regressions/coreclr/GitHub_88128/test88128 was failing due to this om macOS arm64. I have removed all the NORETURN markings from these methods to fix it.
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
There was a problem hiding this comment.
Pull request overview
This PR fixes an interpreter issue on arm64 Unix where ResumeAfterCatch was incorrectly restoring non-volatile registers. The problem occurred because DispatchManagedException and DispatchRethrownManagedException were marked as DECLSPEC_NORETURN, which allowed the C++ compiler to optimize away epilogs, causing the return address to point to a different function. This broke the native unwind loop.
Key changes:
- Removed
DECLSPEC_NORETURNmarking from all exception dispatch functions - Removed
UNREACHABLE()calls after invocations of these dispatch functions - Added
UNREACHABLE()to one location where it's still needed since the parent function remains NORETURN
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/interpexec.cpp | Removed UNREACHABLE() after DispatchManagedException and DispatchRethrownManagedException calls, replaced one with break statement |
| src/coreclr/vm/exceptmacros.h | Removed DECLSPEC_NORETURN from DispatchManagedException declaration for TARGET_UNIX |
| src/coreclr/vm/exceptionhandling.h | Removed DECLSPEC_NORETURN from all dispatch exception function declarations |
| src/coreclr/vm/exceptionhandling.cpp | Removed DECLSPEC_NORETURN from function definitions and removed/commented out UNREACHABLE() calls |
| src/coreclr/vm/excep.cpp | Added UNREACHABLE() after DispatchManagedException call in UnwindAndContinueRethrowHelperAfterCatch (still NORETURN) |
| CallCatchFunclet(pExInfo->m_exception, (BYTE *)pCatchHandler, pFrameIter->m_crawl.GetRegisterSet(), pExInfo); | ||
| // CallCatchFunclet will resume after the catch and never return here. | ||
| UNREACHABLE(); | ||
| //UNREACHABLE(); |
There was a problem hiding this comment.
The commented-out UNREACHABLE() should be removed rather than left as a comment. Since this PR is specifically about removing NORETURN markings and UNREACHABLE() calls due to the compiler optimization issue, leaving this commented out is inconsistent with the other changes.
| //UNREACHABLE(); |
There was a problem hiding this comment.
To better understand code flow, I believe it would be better to preserve all the UNREACHABLE statements as a comment, also explaining that this is a workaround for some unwinding issue.
2 participants
The
DispatchManagedExceptionandDispatchRethrownManagedExceptionwere marked asNORETURN. That caused a problem when the C++ compiler has optimized the invocation of such methods so that it has removed its epilog and the return address was in a completely different function. That has broken the native unwind loop in theInterpreterCodeManager::ResumeAfterCatch, as it restored some non-volatile registers incorrectly.The Regressions/coreclr/GitHub_88128/test88128 was failing due to this om macOS arm64.
I have removed all the
NORETURNmarkings from these methods to fix it.