From 90c3cbbb8e672e952c9381d919bd8e2d5a3524ac Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 5 Mar 2024 10:58:29 +0100 Subject: [PATCH 1/5] Testing only - enable the new EH for excercising it --- src/coreclr/inc/clrconfigvalues.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/inc/clrconfigvalues.h b/src/coreclr/inc/clrconfigvalues.h index d9571f0776456f..45674a77c35396 100644 --- a/src/coreclr/inc/clrconfigvalues.h +++ b/src/coreclr/inc/clrconfigvalues.h @@ -259,7 +259,7 @@ RETAIL_CONFIG_DWORD_INFO(UNSUPPORTED_legacyCorruptedStateExceptionsPolicy, W("le CONFIG_DWORD_INFO(INTERNAL_SuppressLostExceptionTypeAssert, W("SuppressLostExceptionTypeAssert"), 0, "") RETAIL_CONFIG_DWORD_INFO(INTERNAL_UseEntryPointFilter, W("UseEntryPointFilter"), 0, "") RETAIL_CONFIG_DWORD_INFO(INTERNAL_Corhost_Swallow_Uncaught_Exceptions, W("Corhost_Swallow_Uncaught_Exceptions"), 0, "") -RETAIL_CONFIG_DWORD_INFO(EXTERNAL_LegacyExceptionHandling, W("LegacyExceptionHandling"), 1, "Enable legacy exception handling."); +RETAIL_CONFIG_DWORD_INFO(EXTERNAL_LegacyExceptionHandling, W("LegacyExceptionHandling"), 0, "Enable legacy exception handling."); /// From 0d71a2f2d00dd4b13a6138cc6aa713717cb491bd Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 4 Mar 2024 21:47:22 +0100 Subject: [PATCH 2/5] Fix stack walking for new EH There is an edge case when the StackFrameIterator was incorrectly setting the fShouldParentFrameUseUnwindTargetPCforGCReporting, which resulted in a 100% reproducible GC hole in the baseservices\exceptions\unittests\ThrowInFinallyNestedInTry with GC stress C and tiered compilation off. The fix is to delete code that is already present in an "if" branch above the change and that should really be executed only when the funclet parent is the caller of the actual handler frame. --- src/coreclr/inc/eetwain.h | 3 ++ src/coreclr/vm/amd64/cgencpu.h | 8 ++++- src/coreclr/vm/exinfo.cpp | 3 +- src/coreclr/vm/exinfo.h | 9 +++++ src/coreclr/vm/gcenv.ee.common.cpp | 57 ++++++++++++++++++++++++++++-- src/coreclr/vm/gcinfodecoder.cpp | 18 +++++++++- src/coreclr/vm/stackwalk.cpp | 46 ++++++++++++++++-------- src/coreclr/vm/stackwalk.h | 18 +++++++++- 8 files changed, 142 insertions(+), 20 deletions(-) diff --git a/src/coreclr/inc/eetwain.h b/src/coreclr/inc/eetwain.h index 71729a9182f3a1..deefe5b0ce8deb 100644 --- a/src/coreclr/inc/eetwain.h +++ b/src/coreclr/inc/eetwain.h @@ -105,6 +105,9 @@ enum ICodeManagerFlags // any untracked slots LightUnwind = 0x0100, // Unwind just enough to get return addresses + ReportFPBasedSlotsOnly + = 0x0200, // EnumGCRefs/EnumerateLiveSlots should only include + // slots that are based on the frame pointer }; //***************************************************************************** diff --git a/src/coreclr/vm/amd64/cgencpu.h b/src/coreclr/vm/amd64/cgencpu.h index 5dcaf12dfb8120..b3f038321e3bfe 100644 --- a/src/coreclr/vm/amd64/cgencpu.h +++ b/src/coreclr/vm/amd64/cgencpu.h @@ -429,7 +429,13 @@ inline void SetSSP(CONTEXT *context, DWORD64 ssp) } #endif // !DACCESS_COMPILE -#define SetFP(context, ebp) +inline void SetFP(CONTEXT *context, TADDR rbp) +{ + LIMITED_METHOD_DAC_CONTRACT; + + context->Rbp = (DWORD64)rbp; +} + inline TADDR GetFP(const CONTEXT * context) { LIMITED_METHOD_CONTRACT; diff --git a/src/coreclr/vm/exinfo.cpp b/src/coreclr/vm/exinfo.cpp index 7728d9bed3fef0..8731a20ba585b2 100644 --- a/src/coreclr/vm/exinfo.cpp +++ b/src/coreclr/vm/exinfo.cpp @@ -326,7 +326,8 @@ ExInfo::ExInfo(Thread *pThread, EXCEPTION_RECORD *pExceptionRecord, CONTEXT *pEx m_propagateExceptionContext(NULL), #endif // HOST_UNIX m_CurrentClause({}), - m_pMDToReportFunctionLeave(NULL) + m_pMDToReportFunctionLeave(NULL), + m_lastReportedFunclet({0, 0, 0}) { m_StackTraceInfo.AllocateStackTrace(); pThread->GetExceptionState()->m_pCurrentTracker = this; diff --git a/src/coreclr/vm/exinfo.h b/src/coreclr/vm/exinfo.h index db6851fc4d5a16..fc9f134dfeb493 100644 --- a/src/coreclr/vm/exinfo.h +++ b/src/coreclr/vm/exinfo.h @@ -196,6 +196,13 @@ enum class ExKind : uint8_t struct PAL_SEHException; +struct LastReportedFuncletInfo +{ + PCODE IP; + TADDR FP; + uint32_t Flags; +}; + struct ExInfo : public ExceptionTrackerBase { ExInfo(Thread *pThread, EXCEPTION_RECORD *pExceptionRecord, CONTEXT *pExceptionContext, ExKind exceptionKind); @@ -269,6 +276,8 @@ struct ExInfo : public ExceptionTrackerBase REGDISPLAY m_regDisplay; // Initial explicit frame for stack walking Frame *m_pInitialFrame; + // Info on the last reported funclet used to report references in the parent frame + LastReportedFuncletInfo m_lastReportedFunclet; #if defined(TARGET_UNIX) void TakeExceptionPointersOwnership(PAL_SEHException* ex); diff --git a/src/coreclr/vm/gcenv.ee.common.cpp b/src/coreclr/vm/gcenv.ee.common.cpp index 79921a1b69b66b..09aa48e4c48e91 100644 --- a/src/coreclr/vm/gcenv.ee.common.cpp +++ b/src/coreclr/vm/gcenv.ee.common.cpp @@ -3,6 +3,7 @@ #include "common.h" #include "gcenv.h" +#include #if defined(FEATURE_EH_FUNCLETS) @@ -148,6 +149,8 @@ void GcEnumObject(LPVOID pData, OBJECTREF *pObj, uint32_t flags) Object ** ppObj = (Object **)pObj; GCCONTEXT * pCtx = (GCCONTEXT *) pData; + STRESS_LOG3(LF_GCROOTS, LL_INFO1000, "GcEnumObject at slot %p, object %p, pinned=%d\n", pObj, *ppObj, (flags & GC_CALL_PINNED) ? 1 : 0); + // Since we may be asynchronously walking another thread's stack, // check (frequently) for stack-buffer-overrun corruptions after // any long operation @@ -220,8 +223,54 @@ StackWalkAction GcStackCrawlCallBack(CrawlFrame* pCF, VOID* pData) // We may have unwound this crawlFrame and thus, shouldn't report the invalid // references it may contain. fReportGCReferences = pCF->ShouldCrawlframeReportGCReferences(); -#endif // defined(FEATURE_EH_FUNCLETS) + Thread *pThread = pCF->GetThread(); + ExInfo *pExInfo = (ExInfo *)pThread->GetExceptionState()->GetCurrentExceptionTracker(); + + if (pCF->ShouldSaveFuncletInfo()) + { + STRESS_LOG3(LF_GCROOTS, LL_INFO1000, "Saving info on funclet at SP: %p, PC: %p, FP: %p\n", + GetRegdisplaySP(pCF->GetRegisterSet()), GetControlPC(pCF->GetRegisterSet()), GetFP(pCF->GetRegisterSet()->pCurrentContext)); + + _ASSERTE(pExInfo); + REGDISPLAY *pRD = pCF->GetRegisterSet(); + pExInfo->m_lastReportedFunclet.IP = GetControlPC(pRD); + pExInfo->m_lastReportedFunclet.FP = GetFP(pRD->pCurrentContext); + pExInfo->m_lastReportedFunclet.Flags = pCF->GetCodeManagerFlags(); + } + + if (pCF->ShouldParentToFuncletReportSavedFuncletSlots()) + { + STRESS_LOG4(LF_GCROOTS, LL_INFO1000, "Reporting slots in funclet parent frame method at SP: %p, PC: %p using original FP: %p, PC: %p\n", + GetRegdisplaySP(pCF->GetRegisterSet()), GetControlPC(pCF->GetRegisterSet()), pExInfo->m_lastReportedFunclet.FP, pExInfo->m_lastReportedFunclet.IP); + + _ASSERTE(!pCF->ShouldParentToFuncletUseUnwindTargetLocationForGCReporting()); + _ASSERTE(pExInfo); + + ICodeManager * pCM = pCF->GetCodeManager(); + _ASSERTE(pCM != NULL); + + CONTEXT context = {}; + REGDISPLAY partialRD; + SetIP(&context, pExInfo->m_lastReportedFunclet.IP); + SetFP(&context, pExInfo->m_lastReportedFunclet.FP); + SetSP(&context, 0); + + context.ContextFlags = CONTEXT_CONTROL | CONTEXT_INTEGER; + FillRegDisplay(&partialRD, &context); + + EECodeInfo codeInfo(pExInfo->m_lastReportedFunclet.IP); + _ASSERTE(codeInfo.IsValid()); + + pCM->EnumGcRefs(&partialRD, + &codeInfo, + pExInfo->m_lastReportedFunclet.Flags | ReportFPBasedSlotsOnly, + GcEnumObject, + pData, + NO_OVERRIDE_OFFSET); + } + else +#endif // defined(FEATURE_EH_FUNCLETS) if (fReportGCReferences) { if (pCF->IsFrameless()) @@ -297,7 +346,11 @@ StackWalkAction GcStackCrawlCallBack(CrawlFrame* pCF, VOID* pData) pFrame->GcScanRoots( gcctx->f, gcctx->sc); } } - + else + { + STRESS_LOG2(LF_GCROOTS, LL_INFO1000, "Skipping GC scanning in frame method at SP: %p, PC: %p\n", + GetRegdisplaySP(pCF->GetRegisterSet()), GetControlPC(pCF->GetRegisterSet())); + } // If we're executing a LCG dynamic method then we must promote the associated resolver to ensure it // doesn't get collected and yank the method code out from under us). diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index 855aa24f627f6b..5eebf7f0202bcb 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -680,7 +680,9 @@ bool GcInfoDecoder::EnumerateLiveSlots( // previously visited a child funclet if (WantsReportOnlyLeaf() && (inputFlags & ParentOfFuncletStackFrame)) { - LOG((LF_GCROOTS, LL_INFO100000, "Not reporting this frame because it was already reported via another funclet.\n")); +#ifndef NATIVEAOT + STRESS_LOG0(LF_GCROOTS, LL_INFO100, "Not reporting this frame because it was already reported via another funclet.\n"); +#endif return true; } @@ -1510,6 +1512,13 @@ void GcInfoDecoder::ReportRegisterToGC( // AMD64 { GCINFODECODER_CONTRACT; +#ifndef NATIVEAOT + if (flags & ReportFPBasedSlotsOnly) + { + return; + } +#endif + _ASSERTE(regNum >= 0 && regNum <= 16); _ASSERTE(regNum != 4); // rsp @@ -2205,6 +2214,13 @@ void GcInfoDecoder::ReportStackSlotToGC( OBJECTREF* pObjRef = GetStackSlot(spOffset, spBase, pRD); _ASSERTE(IS_ALIGNED(pObjRef, sizeof(OBJECTREF*))); +#ifndef NATIVEAOT + if ((flags & ReportFPBasedSlotsOnly) && (GC_FRAMEREG_REL != spBase)) + { + return; + } +#endif + #ifdef _DEBUG LOG((LF_GCROOTS, LL_INFO1000, /* Part One */ "Reporting %s" FMT_STK, diff --git a/src/coreclr/vm/stackwalk.cpp b/src/coreclr/vm/stackwalk.cpp index 0103bb709a475b..03b7b1c829e470 100644 --- a/src/coreclr/vm/stackwalk.cpp +++ b/src/coreclr/vm/stackwalk.cpp @@ -1098,6 +1098,7 @@ void StackFrameIterator::CommonCtor(Thread * pThread, PTR_Frame pFrame, ULONG32 m_forceReportingWhileSkipping = ForceGCReportingStage::Off; m_movedPastFirstExInfo = false; m_fFuncletNotSeen = false; + m_fFoundFirstFunclet = false; #if defined(RECORD_RESUMABLE_FRAME_SP) m_pvResumableFrameTargetSP = NULL; #endif @@ -1460,6 +1461,8 @@ void StackFrameIterator::ResetCrawlFrame() m_crawl.isFilterFuncletCached = false; m_crawl.fShouldParentToFuncletSkipReportingGCReferences = false; m_crawl.fShouldParentFrameUseUnwindTargetPCforGCReporting = false; + m_crawl.fShouldSaveFuncletInfo = false; + m_crawl.fShouldParentToFuncletReportSavedFuncletSlots = false; #endif // FEATURE_EH_FUNCLETS m_crawl.pThread = this->m_pThread; @@ -1631,10 +1634,11 @@ StackWalkAction StackFrameIterator::Filter(void) { if (!m_movedPastFirstExInfo) { - if ((pExInfo->m_passNumber == 2) && !pExInfo->m_csfEnclosingClause.IsNull() && m_sfFuncletParent.IsNull()) + if ((pExInfo->m_passNumber == 2) && !pExInfo->m_csfEnclosingClause.IsNull() && m_sfFuncletParent.IsNull() && pExInfo->m_lastReportedFunclet.IP != 0) { // We are in the 2nd pass and we have already called an exceptionally called - // a finally funclet, but we have not seen any funclet on the call stack yet. + // finally funclet and reported that to GC in a previous GC run. But we have + // not seen any funclet on the call stack yet. // Simulate that we have actualy seen a finally funclet during this pass and // that it didn't report GC references to ensure that the references will be // reported by the parent correctly. @@ -1651,6 +1655,8 @@ StackWalkAction StackFrameIterator::Filter(void) } } + m_crawl.fShouldParentToFuncletReportSavedFuncletSlots = false; + // by default, there is no funclet for the current frame // that reported GC references m_crawl.fShouldParentToFuncletSkipReportingGCReferences = false; @@ -1659,6 +1665,8 @@ StackWalkAction StackFrameIterator::Filter(void) // CrawlFrame m_crawl.fShouldCrawlframeReportGCReferences = true; + m_crawl.fShouldSaveFuncletInfo = false; + // By default, assume that parent frame is going to report GC references from // the actual location reported by the stack walk. m_crawl.fShouldParentFrameUseUnwindTargetPCforGCReporting = false; @@ -1855,7 +1863,7 @@ StackWalkAction StackFrameIterator::Filter(void) // Initiate force reporting of references in the new managed exception handling code frames. // These frames are still alive when we are in a finally funclet. m_forceReportingWhileSkipping = ForceGCReportingStage::LookForManagedFrame; - STRESS_LOG0(LF_GCROOTS, LL_INFO100, "STACKWALK: Setting m_forceReportingWhileSkipping = ForceGCReportingStage::LookForManagedFrame\n"); + STRESS_LOG0(LF_GCROOTS, LL_INFO100, "STACKWALK: Setting m_forceReportingWhileSkipping = ForceGCReportingStage::LookForManagedFrame while processing filter funclet\n"); } } } @@ -1873,10 +1881,11 @@ StackWalkAction StackFrameIterator::Filter(void) m_sfFuncletParent = ExceptionTracker::FindParentStackFrameForStackWalk(&m_crawl, true); _ASSERTE(!m_fFuncletNotSeen); + bool fFrameWasUnwound = ExceptionTracker::HasFrameBeenUnwoundByAnyActiveException(&m_crawl); if (m_sfFuncletParent.IsNull()) { // This can only happen if the funclet (and its parent) have been unwound. - _ASSERTE(ExceptionTracker::HasFrameBeenUnwoundByAnyActiveException(&m_crawl)); + _ASSERTE(fFrameWasUnwound); } else { @@ -1899,7 +1908,17 @@ StackWalkAction StackFrameIterator::Filter(void) if (g_isNewExceptionHandlingEnabled) { - if (!ExecutionManager::IsManagedCode(GetIP(m_crawl.GetRegisterSet()->pCallerContext))) + if (!m_fFoundFirstFunclet && pExInfo > (void*)GetRegdisplaySP(m_crawl.GetRegisterSet())) + { + // For the first funclet we encounter below the topmost ExInfo, we instruct the GC scanning of the frame + // to save information on the funclet so that we can use it to report references in the parent frame if + // no such funclet is found in future GC scans for the same exception. + _ASSERTE(pExInfo != NULL); + m_crawl.fShouldSaveFuncletInfo = true; + m_fFoundFirstFunclet = true; + } + + if (!fFrameWasUnwound && !ExecutionManager::IsManagedCode(GetIP(m_crawl.GetRegisterSet()->pCallerContext))) { // Initiate force reporting of references in the new managed exception handling code frames. // These frames are still alive when we are in a finally funclet. @@ -2119,6 +2138,14 @@ StackWalkAction StackFrameIterator::Filter(void) } else if (!m_crawl.IsFunclet()) { + if (m_fFuncletNotSeen) + { + // We have reached a real parent of a funclet that would be on the stack if GC didn't + // kick in between the calls to funclets in the second pass. We instruct GC to report + // roots using the info of the saved funclet we've seen during a previous GC. + m_crawl.fShouldParentToFuncletReportSavedFuncletSlots = true; + m_fFuncletNotSeen = false; + } // we've reached the parent and it's not handling an exception, it's also not // a funclet so reset our state. note that we cannot reset the state when the // parent is a funclet since the leaf funclet didn't report any references and @@ -2131,15 +2158,6 @@ StackWalkAction StackFrameIterator::Filter(void) if (g_isNewExceptionHandlingEnabled) { _ASSERTE(!ExceptionTracker::HasFrameBeenUnwoundByAnyActiveException(&m_crawl)); - if (m_fFuncletNotSeen && m_crawl.IsFunclet()) - { - _ASSERTE(!m_fProcessIntermediaryNonFilterFunclet); - _ASSERTE(m_crawl.fShouldCrawlframeReportGCReferences); - m_fDidFuncletReportGCReferences = true; - shouldSkipReporting = false; - m_crawl.fShouldParentFrameUseUnwindTargetPCforGCReporting = true; - m_crawl.ehClauseForCatch = pExInfo->m_ClauseForCatch; - } } else { diff --git a/src/coreclr/vm/stackwalk.h b/src/coreclr/vm/stackwalk.h index 5e7afd8035fb98..19fcebc2ec8188 100644 --- a/src/coreclr/vm/stackwalk.h +++ b/src/coreclr/vm/stackwalk.h @@ -417,6 +417,18 @@ class CrawlFrame return fShouldParentFrameUseUnwindTargetPCforGCReporting; } + bool ShouldParentToFuncletReportSavedFuncletSlots() + { + LIMITED_METHOD_CONTRACT; + return fShouldParentToFuncletReportSavedFuncletSlots; + } + + bool ShouldSaveFuncletInfo() + { + LIMITED_METHOD_CONTRACT; + return fShouldSaveFuncletInfo; + } + const EE_ILEXCEPTION_CLAUSE& GetEHClauseForCatch() { return ehClauseForCatch; @@ -469,6 +481,8 @@ class CrawlFrame bool fShouldParentToFuncletSkipReportingGCReferences; bool fShouldCrawlframeReportGCReferences; bool fShouldParentFrameUseUnwindTargetPCforGCReporting; + bool fShouldSaveFuncletInfo; + bool fShouldParentToFuncletReportSavedFuncletSlots; EE_ILEXCEPTION_CLAUSE ehClauseForCatch; #endif //FEATURE_EH_FUNCLETS Thread* pThread; @@ -701,7 +715,6 @@ class StackFrameIterator if (!ResetOnlyIntermediaryState) { - m_fFuncletNotSeen = false; m_sfFuncletParent = StackFrame(); m_fProcessNonFilterFunclet = false; } @@ -754,6 +767,9 @@ class StackFrameIterator bool m_movedPastFirstExInfo; // Indicates that no funclet was seen during the current stack walk yet bool m_fFuncletNotSeen; + // Indicates that the stack walk has moved past a funclet + bool m_fFoundFirstFunclet; + #if defined(RECORD_RESUMABLE_FRAME_SP) LPVOID m_pvResumableFrameTargetSP; #endif // RECORD_RESUMABLE_FRAME_SP From fcbfa2c7006064a15e30da48bfdc8a88d62929a5 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 14 Mar 2024 13:47:50 +0100 Subject: [PATCH 3/5] Cleanup, reflecting feedback and fixing one last issue --- src/coreclr/inc/gcinfodecoder.h | 8 ++++++-- src/coreclr/vm/exceptionhandling.cpp | 11 +++++++++++ src/coreclr/vm/gcenv.ee.common.cpp | 2 -- src/coreclr/vm/gcinfodecoder.cpp | 18 +----------------- 4 files changed, 18 insertions(+), 21 deletions(-) diff --git a/src/coreclr/inc/gcinfodecoder.h b/src/coreclr/inc/gcinfodecoder.h index eb60728af5b1f7..a85aa2a6abb73b 100644 --- a/src/coreclr/inc/gcinfodecoder.h +++ b/src/coreclr/inc/gcinfodecoder.h @@ -186,6 +186,7 @@ enum ICodeManagerFlags NoReportUntracked = 0x0080, // EnumGCRefs/EnumerateLiveSlots should *not* include // any untracked slots + ReportFPBasedSlotsOnly = 0, // Unused by nativeaot, the gc info decoder checks that. Set to 0 to let the compiler optimize out the related code. }; #endif // !_strike_h @@ -675,11 +676,12 @@ class GcInfoDecoder { _ASSERTE(slotIndex < slotDecoder.GetNumSlots()); const GcSlotDesc* pSlot = slotDecoder.GetSlotDesc(slotIndex); + bool reportFpBasedSlotsOnly = (inputFlags & ReportFPBasedSlotsOnly); if(slotIndex < slotDecoder.GetNumRegisters()) { UINT32 regNum = pSlot->Slot.RegisterNumber; - if( reportScratchSlots || !IsScratchRegister( regNum, pRD ) ) + if( ( reportScratchSlots || !IsScratchRegister( regNum, pRD ) ) && !reportFpBasedSlotsOnly ) { ReportRegisterToGC( regNum, @@ -699,7 +701,9 @@ class GcInfoDecoder { INT32 spOffset = pSlot->Slot.Stack.SpOffset; GcStackSlotBase spBase = pSlot->Slot.Stack.Base; - if( reportScratchSlots || !IsScratchStackSlot(spOffset, spBase, pRD) ) + + if( ( reportScratchSlots || !IsScratchStackSlot(spOffset, spBase, pRD) ) && + ( !reportFpBasedSlotsOnly || (GC_FRAMEREG_REL == spBase ) ) ) { ReportStackSlotToGC( spOffset, diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index a6118ef56bca14..298968db2a7ef4 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -8254,6 +8254,17 @@ extern "C" bool QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStackwalk } } } + else // pass number 2 + { + if (pThis->GetFrameState() == StackFrameIterator::SFITER_SKIPPED_FRAME_FUNCTION) + { + // Update context pointers using the skipped frame. This is needed when exception handling continues + // from ProcessCLRExceptionNew, since the RtlUnwind doesn't maintain context pointers. + Frame *pSkippedFrame = pThis->m_crawl.GetFrame(); + _ASSERTE(pSkippedFrame->NeedsUpdateRegDisplay() && pSkippedFrame->GetReturnAddress() == GetControlPC(pThis->m_crawl.GetRegisterSet())); + pSkippedFrame->UpdateRegDisplay(pThis->m_crawl.GetRegisterSet()); + } + } StackWalkAction retVal = pThis->Next(); result = (retVal != SWA_FAILED); } diff --git a/src/coreclr/vm/gcenv.ee.common.cpp b/src/coreclr/vm/gcenv.ee.common.cpp index 09aa48e4c48e91..20c27d209f6e4e 100644 --- a/src/coreclr/vm/gcenv.ee.common.cpp +++ b/src/coreclr/vm/gcenv.ee.common.cpp @@ -149,8 +149,6 @@ void GcEnumObject(LPVOID pData, OBJECTREF *pObj, uint32_t flags) Object ** ppObj = (Object **)pObj; GCCONTEXT * pCtx = (GCCONTEXT *) pData; - STRESS_LOG3(LF_GCROOTS, LL_INFO1000, "GcEnumObject at slot %p, object %p, pinned=%d\n", pObj, *ppObj, (flags & GC_CALL_PINNED) ? 1 : 0); - // Since we may be asynchronously walking another thread's stack, // check (frequently) for stack-buffer-overrun corruptions after // any long operation diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index 5eebf7f0202bcb..855aa24f627f6b 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -680,9 +680,7 @@ bool GcInfoDecoder::EnumerateLiveSlots( // previously visited a child funclet if (WantsReportOnlyLeaf() && (inputFlags & ParentOfFuncletStackFrame)) { -#ifndef NATIVEAOT - STRESS_LOG0(LF_GCROOTS, LL_INFO100, "Not reporting this frame because it was already reported via another funclet.\n"); -#endif + LOG((LF_GCROOTS, LL_INFO100000, "Not reporting this frame because it was already reported via another funclet.\n")); return true; } @@ -1512,13 +1510,6 @@ void GcInfoDecoder::ReportRegisterToGC( // AMD64 { GCINFODECODER_CONTRACT; -#ifndef NATIVEAOT - if (flags & ReportFPBasedSlotsOnly) - { - return; - } -#endif - _ASSERTE(regNum >= 0 && regNum <= 16); _ASSERTE(regNum != 4); // rsp @@ -2214,13 +2205,6 @@ void GcInfoDecoder::ReportStackSlotToGC( OBJECTREF* pObjRef = GetStackSlot(spOffset, spBase, pRD); _ASSERTE(IS_ALIGNED(pObjRef, sizeof(OBJECTREF*))); -#ifndef NATIVEAOT - if ((flags & ReportFPBasedSlotsOnly) && (GC_FRAMEREG_REL != spBase)) - { - return; - } -#endif - #ifdef _DEBUG LOG((LF_GCROOTS, LL_INFO1000, /* Part One */ "Reporting %s" FMT_STK, From 659481083841824e45b13202d3aad0d223b0beb0 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 14 Mar 2024 21:22:52 +0100 Subject: [PATCH 4/5] Reflect PR feedback and fix asserts --- src/coreclr/inc/gcinfodecoder.h | 4 +++- src/coreclr/vm/exceptionhandling.cpp | 2 +- src/coreclr/vm/stackwalk.cpp | 1 - 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coreclr/inc/gcinfodecoder.h b/src/coreclr/inc/gcinfodecoder.h index a85aa2a6abb73b..e3ddecae8fa7d5 100644 --- a/src/coreclr/inc/gcinfodecoder.h +++ b/src/coreclr/inc/gcinfodecoder.h @@ -186,7 +186,9 @@ enum ICodeManagerFlags NoReportUntracked = 0x0080, // EnumGCRefs/EnumerateLiveSlots should *not* include // any untracked slots - ReportFPBasedSlotsOnly = 0, // Unused by nativeaot, the gc info decoder checks that. Set to 0 to let the compiler optimize out the related code. + ReportFPBasedSlotsOnly + = 0x0200, // EnumGCRefs/EnumerateLiveSlots should only include + // slots that are based on the frame pointer }; #endif // !_strike_h diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 298968db2a7ef4..0180cd32872145 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -8261,7 +8261,7 @@ extern "C" bool QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStackwalk // Update context pointers using the skipped frame. This is needed when exception handling continues // from ProcessCLRExceptionNew, since the RtlUnwind doesn't maintain context pointers. Frame *pSkippedFrame = pThis->m_crawl.GetFrame(); - _ASSERTE(pSkippedFrame->NeedsUpdateRegDisplay() && pSkippedFrame->GetReturnAddress() == GetControlPC(pThis->m_crawl.GetRegisterSet())); + _ASSERTE(pSkippedFrame->NeedsUpdateRegDisplay()); pSkippedFrame->UpdateRegDisplay(pThis->m_crawl.GetRegisterSet()); } } diff --git a/src/coreclr/vm/stackwalk.cpp b/src/coreclr/vm/stackwalk.cpp index 03b7b1c829e470..841a781ec17388 100644 --- a/src/coreclr/vm/stackwalk.cpp +++ b/src/coreclr/vm/stackwalk.cpp @@ -1879,7 +1879,6 @@ StackWalkAction StackFrameIterator::Filter(void) { // Get a reference to the funclet's parent frame. m_sfFuncletParent = ExceptionTracker::FindParentStackFrameForStackWalk(&m_crawl, true); - _ASSERTE(!m_fFuncletNotSeen); bool fFrameWasUnwound = ExceptionTracker::HasFrameBeenUnwoundByAnyActiveException(&m_crawl); if (m_sfFuncletParent.IsNull()) From 49a2b8b544b343a71d5911bfdb13248a6edee9dd Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 15 Mar 2024 01:06:57 +0100 Subject: [PATCH 5/5] Filter out InlinedCallFrame from updating the pointers InlinedCallFrame needs to be excluded from updating the context pointers, as it restores PC / SP to a different location than the callsite of the pinvoke and it doesn't update the context pointers anyways. --- src/coreclr/vm/exceptionhandling.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 0180cd32872145..7a92fa7666f10e 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -8260,9 +8260,14 @@ extern "C" bool QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStackwalk { // Update context pointers using the skipped frame. This is needed when exception handling continues // from ProcessCLRExceptionNew, since the RtlUnwind doesn't maintain context pointers. + // We explicitly don't do that for inlined frames as it would modify the PC/SP to point to + // a slightly different location in the managed code calling the pinvoke and the inlined + // call frame doesn't update the context pointers anyways. Frame *pSkippedFrame = pThis->m_crawl.GetFrame(); - _ASSERTE(pSkippedFrame->NeedsUpdateRegDisplay()); - pSkippedFrame->UpdateRegDisplay(pThis->m_crawl.GetRegisterSet()); + if (pSkippedFrame->NeedsUpdateRegDisplay() && (pSkippedFrame->GetVTablePtr() != InlinedCallFrame::GetMethodFrameVPtr())) + { + pSkippedFrame->UpdateRegDisplay(pThis->m_crawl.GetRegisterSet()); + } } } StackWalkAction retVal = pThis->Next();