Fix IP adjustment for interpreter EH #117055
Conversation
The exception handling stack frame iterator adjusts IP it returns for all cases when the IP represents a return address. This is to make sure that if the last instruction in a try regios is a call, it is still covered by the try region (the return address would be right after it). For jit/aoted code, throwing a software exceptions results in a call. Only hardware exceptions like division by zero etc report the address of the failing instruction. In the interpreter, when an exception is thrown, the IP address is always the address of the instruction that has triggered the exception. So we should not adjust the IP. This change fixes that by marking exception throwing frame as "faulting", which ensures the adjustment doesn't occur. That information is recorded in the InterpreterFrame when an exception is thrown by the interpreter code and cleared when the execution resumes after catch.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the IP adjustment for interpreter exception handling by marking interpreter frames as faulting when an exception is thrown, ensuring the IP remains unadjusted during interpreter exceptions. Key changes include:
- Setting faulting flags in stackwalk.cpp when an exception context is detected.
- Updating interpexec.cpp to mark interpreter frames as faulting during exception throw and rethrow.
- Introducing a new faulting flag and related setter in InterpreterFrame (frames.h and frames.cpp) and applying it in excep.cpp.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/stackwalk.cpp | Added faulting flag logic to set exception active context status. |
| src/coreclr/vm/interpexec.cpp | Marked interpreter frames as faulting for throw and rethrow paths. |
| src/coreclr/vm/frames.h | Added the m_isFaulting field and SetIsFaulting method to InterpreterFrame. |
| src/coreclr/vm/frames.cpp | Updated context setting to reflect the faulting status. |
| src/coreclr/vm/excep.cpp | Ensured interpreter frames are marked as faulting during unwind. |
Comments suppressed due to low confidence (1)
src/coreclr/vm/stackwalk.cpp:2863
- Consider adding a brief comment explaining why checking for CONTEXT_EXCEPTION_ACTIVE here leads to setting isInterrupted and hasFaulted. This will help maintainability by clarifying the relationship between the context flag and the interpreter frame state.
if (pRD->pCurrentContext->ContextFlags & CONTEXT_EXCEPTION_ACTIVE)
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
src/coreclr/vm/excep.cpp
Outdated
| @@ -7360,6 +7360,12 @@ VOID DECLSPEC_NORETURN UnwindAndContinueRethrowHelperAfterCatch(Frame* pEntryFra | |||
| } | |||
| else | |||
| { | |||
| #ifdef FEATURE_INTERPRETER | |||
| if ((pEntryFrame != NULL) && (pEntryFrame->GetFrameIdentifier() == FrameIdentifier::InterpreterFrame)) | |||
There was a problem hiding this comment.
Does both of us overlooking 'pEntryFrame can be NULL' in this case indicate that we need a comment in a key spot to make it clear in the future that it's potentially null?
There was a problem hiding this comment.
I've actually just pushed another fix, the NULL was not a correct fix. The correct check is FRAME_TOP. The Thread::GetFrame() that is a source of this pEntryFrame returns that when there are no frames on the thread.
| pContext->ContextFlags = CONTEXT_FULL; | ||
| if (m_isFaulting) | ||
| { | ||
| pContext->ContextFlags |= CONTEXT_EXCEPTION_ACTIVE; |
There was a problem hiding this comment.
How does exactly setting this flag end up preventing the ip adjustment ? Seems not obvious from searching in the code.
There was a problem hiding this comment.
The EH doesn't do adjustment for frames that are marked as faulting. In the JIT/AOT case, these are frames of hardware exceptions where the IP is the IP of the faulting instruction. For the interpreter, the throwing IR opcodes behave the same way. Setting this flag results in setting the m_crawl.isFaulting and m_crawl.isInterrupted to true. The SfiInit and SfiNext perform the IP adjustment only when the isFaulting is not set.
E.g. for the SfiInit, you can see it here:
runtime/src/coreclr/vm/exceptionhandling.cpp
Lines 3863 to 3866 in 50be35a
| @@ -1737,12 +1737,14 @@ void InterpExecMethod(InterpreterFrame *pInterpreterFrame, InterpMethodContextFr | |||
| { | |||
| throwable = LOCAL_VAR(ip[1], OBJECTREF); | |||
| } | |||
| pInterpreterFrame->SetIsFaulting(true); | |||
There was a problem hiding this comment.
Shouldn't this flag also be set for the other sources of exception throwing in the interp method execution ? Conv ovf opcodes for example
There was a problem hiding this comment.
No, these are all handled in the UnwindAndContinueRethrowHelperAfterCatch
There was a problem hiding this comment.
That one is called in the UNINSTALL_UNWIND_AND_CONTINUE_HANDLER macro in the InterpExecMethod.
The exception handling stack frame iterator adjusts IP it returns for all cases when the IP represents a return address. This is to make sure that if the last instruction in a try regios is a call, it is still covered by the try region (the return address would be right after it). For jit/aoted code, throwing a software exceptions results in a call. Only hardware exceptions like division by zero etc report the address of the failing instruction.
In the interpreter, when an exception is thrown, the IP address is always the address of the instruction that has triggered the exception. So we should not adjust the IP.
This change fixes that by marking exception throwing frame as "faulting", which ensures the adjustment doesn't occur. That information is recorded in the InterpreterFrame when an exception is thrown by the interpreter code and cleared when the execution resumes after catch.