From 725bcfa52770a890d28a32cfba84e7c4f8ec7926 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 17 Sep 2025 00:03:28 +0200 Subject: [PATCH] Fix several issues with interpreter and EH This change fixes few issues I have found while investigating coreclr tests failures when running interpreted * The GC info for reverse pinvoke stubs didn't have the revPInvokeOffset set * When SfiNext reached native marker state, it was doing one more unwind to get to the frame function state. That is benign for non-interpreter scenarios, as it doesn't change the regdisplay, but it is useless due to that. But for interpreter, it can actually incorrectly move to interpreted code. The fix is to not to do that unwind and just return. * The StackFrameIterator::SkipTo that is called when SfiNext collides with the previous EH was missing copying of the first argument register that can hold the InterpreterFrame pointer. * When SfiNext moved out of the last interpreted code frame and the caller of the interpreted code was not managed code (e.g. the CallDescrWorker), the StackFrameIterator's regdisplay was not updated to that context. --- src/coreclr/interpreter/compiler.cpp | 5 +++ src/coreclr/vm/exceptionhandling.cpp | 47 ++++++++++++++++++++-------- src/coreclr/vm/stackwalk.cpp | 12 +++++++ 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 602e89ebd61625..8340434c86ed68 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -1280,6 +1280,11 @@ void InterpCompiler::BuildGCInfo(InterpMethod *pInterpMethod) gcInfoEncoder->SetStackBaseRegister(0); gcInfoEncoder->DefineInterruptibleRange(0, ConvertOffset(m_methodCodeSize)); + if (pInterpMethod->unmanagedCallersOnly) + { + gcInfoEncoder->SetReversePInvokeFrameSlot(0); + } + GENERIC_CONTEXTPARAM_TYPE paramType; switch (m_methodInfo->options & CORINFO_GENERICS_CTXT_MASK) diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 3fd89787003e93..e905879090a2f7 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -3901,7 +3901,7 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid QCALL_CONTRACT; StackWalkAction retVal = SWA_FAILED; - CLR_BOOL isPropagatingToNativeCode = FALSE; + CLR_BOOL success = FALSE; Thread* pThread = GET_THREAD(); ExInfo* pTopExInfo = (ExInfo*)pThread->GetExceptionState()->GetCurrentExceptionTracker(); @@ -3936,24 +3936,40 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid // Move the stack frame iterator to the InterpreterFrame and extract the IP of the real caller of the interpreted code. retVal = pThis->Next(); _ASSERTE(retVal != SWA_FAILED); + _ASSERTE(pThis->GetFrameState() == StackFrameIterator::SFITER_FRAME_FUNCTION); _ASSERTE(pThis->m_crawl.GetFrame()->GetFrameIdentifier() == FrameIdentifier::InterpreterFrame); + InterpreterFrame *pInterpreterFrame = (InterpreterFrame *)pThis->m_crawl.GetFrame(); // Move to the caller of the interpreted code retVal = pThis->Next(); _ASSERTE(retVal != SWA_FAILED); + if (pThis->GetFrameState() != StackFrameIterator::SFITER_FRAMELESS_METHOD) + { + // The caller of the interpreted code is not managed. + if (pInterpreterFrame->GetReturnAddress() != 0) + { + // The caller is not InterpreterCodeManager::CallFunclet, so we can update the regdisplay + // (The CallFunclet has no TransitionFrame to update from) + pInterpreterFrame->UpdateRegDisplay(pThis->m_crawl.GetRegisterSet(), /* updateFloats */ true); + } + } } #endif // FEATURE_INTERPRETER // Check for reverse pinvoke or CallDescrWorkerInternal. - if (pThis->GetFrameState() == StackFrameIterator::SFITER_NATIVE_MARKER_FRAME) + if ((pThis->GetFrameState() == StackFrameIterator::SFITER_NATIVE_MARKER_FRAME) +#ifdef FEATURE_INTERPRETER + || (pThis->GetFrameState() == StackFrameIterator::SFITER_DONE) +#endif // FEATURE_INTERPRETER + ) { EECodeInfo codeInfo(preUnwindControlPC); #ifdef USE_GC_INFO_DECODER GcInfoDecoder gcInfoDecoder(codeInfo.GetGCInfoToken(), DECODE_REVERSE_PINVOKE_VAR); - isPropagatingToNativeCode = gcInfoDecoder.GetReversePInvokeFrameStackSlot() != NO_REVERSE_PINVOKE_FRAME; + CLR_BOOL isPropagatingToNativeCode = gcInfoDecoder.GetReversePInvokeFrameStackSlot() != NO_REVERSE_PINVOKE_FRAME; #else // USE_GC_INFO_DECODER hdrInfo *hdrInfoBody; codeInfo.DecodeGCHdrInfo(&hdrInfoBody); - isPropagatingToNativeCode = hdrInfoBody->revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET; + CLR_BOOL isPropagatingToNativeCode = hdrInfoBody->revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET; #endif // USE_GC_INFO_DECODER bool isPropagatingToExternalNativeCode = false; @@ -4036,10 +4052,13 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid } } - // Unwind to the caller of the managed code - retVal = pThis->Next(); - _ASSERTE(retVal != SWA_FAILED); - _ASSERTE(pThis->GetFrameState() != StackFrameIterator::SFITER_SKIPPED_FRAME_FUNCTION); + *pfIsExceptionIntercepted = FALSE; + + if (fUnwoundReversePInvoke) + { + *fUnwoundReversePInvoke = TRUE; + } + goto Exit; } } @@ -4050,7 +4069,6 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid if (pThis->GetFrameState() == StackFrameIterator::SFITER_DONE) { EH_LOG((LL_INFO100, "SfiNext (pass=%d): no more managed frames found on stack", pTopExInfo->m_passNumber)); - retVal = SWA_FAILED; goto Exit; } @@ -4137,6 +4155,10 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid { EH_LOG((LL_INFO100, "SfiNext (pass=%d): failed to get next frame", pTopExInfo->m_passNumber)); } + else + { + success = TRUE; + } if (!isCollided) { @@ -4146,7 +4168,7 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid Exit:; END_QCALL; - if (retVal != SWA_FAILED) + if (success) { TADDR controlPC = pThis->m_crawl.GetRegisterSet()->ControlPC; if (!pThis->m_crawl.HasFaulted() && !pThis->m_crawl.IsIPadjusted()) @@ -4159,7 +4181,7 @@ Exit:; if (fUnwoundReversePInvoke) { - *fUnwoundReversePInvoke = isPropagatingToNativeCode; + *fUnwoundReversePInvoke = FALSE; } if (pThis->GetFrameState() == StackFrameIterator::SFITER_FRAMELESS_METHOD) @@ -4168,10 +4190,9 @@ Exit:; pTopExInfo->m_passNumber, controlPC, GetRegdisplaySP(pThis->m_crawl.GetRegisterSet()), pThis->m_crawl.GetFunction()->m_pszDebugClassName, pThis->m_crawl.GetFunction()->m_pszDebugMethodName)); } - return TRUE; } - return FALSE; + return success; } namespace AsmOffsetsAsserts diff --git a/src/coreclr/vm/stackwalk.cpp b/src/coreclr/vm/stackwalk.cpp index dd2e03badb3007..4c713e37d7b928 100644 --- a/src/coreclr/vm/stackwalk.cpp +++ b/src/coreclr/vm/stackwalk.cpp @@ -1515,6 +1515,11 @@ void StackFrameIterator::SkipTo(StackFrameIterator *pOtherStackFrameIterator) *pRD->pCurrentContextPointers = *pOtherRD->pCurrentContextPointers; SetIP(pRD->pCurrentContext, GetIP(pOtherRD->pCurrentContext)); SetSP(pRD->pCurrentContext, GetSP(pOtherRD->pCurrentContext)); +#ifdef FEATURE_INTERPRETER + // May contain InterpreterFrame address + SetFirstArgReg(pRD->pCurrentContext, GetFirstArgReg(pOtherRD->pCurrentContext)); +#endif + #if defined(TARGET_AMD64) && defined(TARGET_WINDOWS) pRD->SSP = pOtherRD->SSP; #endif @@ -1533,6 +1538,10 @@ void StackFrameIterator::SkipTo(StackFrameIterator *pOtherStackFrameIterator) *pRD->pCallerContextPointers = *pOtherRD->pCallerContextPointers; SetIP(pRD->pCallerContext, GetIP(pOtherRD->pCallerContext)); SetSP(pRD->pCallerContext, GetSP(pOtherRD->pCallerContext)); +#ifdef FEATURE_INTERPRETER + // May contain InterpreterFrame address + SetFirstArgReg(pRD->pCallerContext, GetFirstArgReg(pOtherRD->pCallerContext)); +#endif #define CALLEE_SAVED_REGISTER(regname) pRD->pCallerContext->regname = (pRD->pCallerContextPointers->regname == NULL) ? pOtherRD->pCallerContext->regname : *pRD->pCallerContextPointers->regname; ENUM_CALLEE_SAVED_REGISTERS(); @@ -2851,6 +2860,7 @@ void StackFrameIterator::ProcessCurrentFrame(void) // Switch to walking the underlying interpreted frames. // Save the registers the interpreter frames walking reuses so that we can restore them // after we are done with the interpreter frames. + LOG((LF_GCROOTS, LL_INFO10000, "STACKWALK: Switching to interpreted frames for InterpreterFrame %p, saving SP=%p, IP=%p\n", m_crawl.pFrame, GetIP(pRD->pCurrentContext), GetSP(pRD->pCurrentContext))); m_interpExecMethodIP = GetIP(pRD->pCurrentContext); m_interpExecMethodSP = GetSP(pRD->pCurrentContext); m_interpExecMethodFP = GetFP(pRD->pCurrentContext); @@ -2870,6 +2880,8 @@ void StackFrameIterator::ProcessCurrentFrame(void) { // We have finished walking the interpreted frames. Process the InterpreterFrame itself. // Restore the registers to the values they had before we started walking the interpreter frames. + LOG((LF_GCROOTS, LL_INFO10000, "STACKWALK: Completed walking of interpreted frames for InterpreterFrame %p, restoring SP=%p, IP=%p\n", m_crawl.pFrame, m_interpExecMethodSP, m_interpExecMethodIP)); + _ASSERTE(dac_cast(m_crawl.pFrame) == GetFirstArgReg(pRD->pCurrentContext)); SetIP(pRD->pCurrentContext, m_interpExecMethodIP); SetSP(pRD->pCurrentContext, m_interpExecMethodSP); SetFP(pRD->pCurrentContext, m_interpExecMethodFP);