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.

Author: janvorli

src/coreclr/inc/eetwain.h

@@ -105,6 +105,8 @@ enum ICodeManagerFlags
                                 // any untracked slots
 
     LightUnwind     =   0x0100, // Unwind just enough to get return addresses
+    ReportFPBasedSlotsOnly
+                    =   0x0200,
 };
 
 //*****************************************************************************

src/coreclr/inc/gcinfodecoder.h

@@ -679,7 +679,11 @@ class GcInfoDecoder
         if(slotIndex < slotDecoder.GetNumRegisters())
         {
             UINT32 regNum = pSlot->Slot.RegisterNumber;
-            if( reportScratchSlots || !IsScratchRegister( regNum, pRD ) )
+            if( (reportScratchSlots || !IsScratchRegister( regNum, pRD )) 
+#ifndef NATIVEAOT
+                && ((inputFlags & ReportFPBasedSlotsOnly) == 0)
+#endif                
+            )
             {
                 ReportRegisterToGC(
                             regNum,

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;

src/coreclr/vm/eetwain.cpp

@@ -1349,6 +1349,14 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY     pContext,
         GC_NOTRIGGER;
     } CONTRACTL_END;
 
+#ifdef FEATURE_EH_FUNCLETS
+    if (flags & ParentOfFuncletStackFrame)
+    {
+        STRESS_LOG0((LF_GCROOTS, LL_INFO100, "Not reporting this frame because it was already reported via another funclet.\n"));
+        return true;
+    }
+#endif // FEATURE_EH_FUNCLETS
+
     PTR_CBYTE methodStart = PTR_CBYTE(pCodeInfo->GetSavedMethodCode());
     unsigned  curOffs = pCodeInfo->GetRelOffset();
     GCInfoToken gcInfoToken = pCodeInfo->GetGCInfoToken();

src/coreclr/vm/exinfo.cpp

@@ -326,7 +326,9 @@ ExInfo::ExInfo(Thread *pThread, EXCEPTION_RECORD *pExceptionRecord, CONTEXT *pEx
     m_propagateExceptionContext(NULL),
 #endif // HOST_UNIX
     m_CurrentClause({}),
-    m_pMDToReportFunctionLeave(NULL)
+    m_pMDToReportFunctionLeave(NULL),
+    //m_lastFuncletReportedSlotsCount(0)
+    m_lastReportedFunclet({0, 0, 0})
 {
     m_StackTraceInfo.AllocateStackTrace();
     pThread->GetExceptionState()->m_pCurrentTracker = this;

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,10 @@ struct ExInfo : public ExceptionTrackerBase
     REGDISPLAY     m_regDisplay;
     // Initial explicit frame for stack walking
     Frame         *m_pInitialFrame;
+    // List of stack slots reported at the previous GC pass for a funclet
+    // size_t         m_lastFuncletReportedSlots[32];
+    // int            m_lastFuncletReportedSlotsCount;
+    LastReportedFuncletInfo m_lastReportedFunclet;
 
 #if defined(TARGET_UNIX)
     void TakeExceptionPointersOwnership(PAL_SEHException* ex);

src/coreclr/vm/gcenv.ee.common.cpp

@@ -3,6 +3,7 @@
 
 #include "common.h"
 #include "gcenv.h"
+#include <exinfo.h>
 
 #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
@@ -222,7 +225,52 @@ StackWalkAction GcStackCrawlCallBack(CrawlFrame* pCF, VOID* pData)
     fReportGCReferences = pCF->ShouldCrawlframeReportGCReferences();
 #endif // defined(FEATURE_EH_FUNCLETS)
 
-    if (fReportGCReferences)
+    Thread *pThread = pCF->GetThread();
+    ExInfo *pExInfo = (ExInfo *)pThread->GetExceptionState()->GetCurrentExceptionTracker();
+
+    if (pCF->ShouldSaveReportedSlots())
+    {
+        STRESS_LOG3(LF_GCROOTS, LL_INFO1000, "Saving location in frame method 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->ShouldParentToFuncletReportSavedSlots())
+    {
+        STRESS_LOG4(LF_GCROOTS, LL_INFO1000, "Reporting saved slots in 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 if (fReportGCReferences)
     {
         if (pCF->IsFrameless())
         {
@@ -297,7 +345,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).

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,

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,7 @@ void StackFrameIterator::ResetCrawlFrame()
     m_crawl.isFilterFuncletCached = false;
     m_crawl.fShouldParentToFuncletSkipReportingGCReferences = false;
     m_crawl.fShouldParentFrameUseUnwindTargetPCforGCReporting = false;
+    m_crawl.fShouldParentToFuncletReportSavedSlots = false;
 #endif // FEATURE_EH_FUNCLETS
 
     m_crawl.pThread = this->m_pThread;
@@ -1631,10 +1633,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 +1654,8 @@ StackWalkAction StackFrameIterator::Filter(void)
             }
         }
 
+        m_crawl.fShouldParentToFuncletReportSavedSlots = false;
+
         // by default, there is no funclet for the current frame
         // that reported GC references
         m_crawl.fShouldParentToFuncletSkipReportingGCReferences = false;
@@ -1659,6 +1664,8 @@ StackWalkAction StackFrameIterator::Filter(void)
         // CrawlFrame
         m_crawl.fShouldCrawlframeReportGCReferences = true;
 
+        m_crawl.fShouldSaveReportedSlots = 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 +1862,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");
                                             }
                                         }
                                     }
@@ -1871,7 +1878,7 @@ StackWalkAction StackFrameIterator::Filter(void)
                             {
                                 // Get a reference to the funclet's parent frame.
                                 m_sfFuncletParent = ExceptionTracker::FindParentStackFrameForStackWalk(&m_crawl, true);
-                                _ASSERTE(!m_fFuncletNotSeen);
+                                //_ASSERTE(!m_fFuncletNotSeen);
 
                                 if (m_sfFuncletParent.IsNull())
                                 {
@@ -1899,7 +1906,15 @@ StackWalkAction StackFrameIterator::Filter(void)
 
                                         if (g_isNewExceptionHandlingEnabled)
                                         {
-                                            if (!ExecutionManager::IsManagedCode(GetIP(m_crawl.GetRegisterSet()->pCallerContext)))
+                                            // TODO: need to do this for the first funclet of the current exception handling only
+                                            if (!m_fFoundFirstFunclet &&  pExInfo > (void*)GetRegdisplaySP(m_crawl.GetRegisterSet()))
+                                            {
+                                                _ASSERTE(pExInfo != NULL);
+                                                m_crawl.fShouldSaveReportedSlots = true;
+                                                m_fFoundFirstFunclet = true;
+                                            }
+
+                                            if (!ExceptionTracker::HasFrameBeenUnwoundByAnyActiveException(&m_crawl) && !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.
@@ -2117,29 +2132,30 @@ StackWalkAction StackFrameIterator::Filter(void)
                                                     m_crawl.ehClauseForCatch.HandlerEndPC);                                                
                                             }
                                         }
-                                        else if (!m_crawl.IsFunclet())
+                                        else 
                                         {
-                                            // 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
-                                            // we might have a catch handler below us that might contain GC roots.
-                                            m_fDidFuncletReportGCReferences = true;
-                                            STRESS_LOG0(LF_GCROOTS, LL_INFO100,
-                                                "STACKWALK: Reached parent of funclet which didn't report GC roots is not a funclet, resetting m_fDidFuncletReportGCReferences to true\n");
+                                            if (!m_crawl.IsFunclet())
+                                            {
+                                                if (m_fFuncletNotSeen)
+                                                {
+                                                    // We need to report this parent frame
+                                                    //shouldSkipReporting = false;
+                                                    m_crawl.fShouldParentToFuncletReportSavedSlots = 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
+                                                // we might have a catch handler below us that might contain GC roots.
+                                                m_fDidFuncletReportGCReferences = true;
+                                                STRESS_LOG0(LF_GCROOTS, LL_INFO100,
+                                                    "STACKWALK: Reached parent of funclet which didn't report GC roots is not a funclet, resetting m_fDidFuncletReportGCReferences to true\n");
+                                            }
                                         }
 
                                         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
                                         {

src/coreclr/vm/stackwalk.h

@@ -417,6 +417,18 @@ class CrawlFrame
         return fShouldParentFrameUseUnwindTargetPCforGCReporting;
     }
 
+    bool ShouldParentToFuncletReportSavedSlots()
+    {
+        LIMITED_METHOD_CONTRACT;
+        return fShouldParentToFuncletReportSavedSlots;
+    }
+
+    bool ShouldSaveReportedSlots()
+    {
+        LIMITED_METHOD_CONTRACT;
+        return fShouldSaveReportedSlots;
+    }
+
     const EE_ILEXCEPTION_CLAUSE& GetEHClauseForCatch()
     {
         return ehClauseForCatch;
@@ -469,6 +481,8 @@ class CrawlFrame
     bool              fShouldParentToFuncletSkipReportingGCReferences;
     bool              fShouldCrawlframeReportGCReferences;
     bool              fShouldParentFrameUseUnwindTargetPCforGCReporting;
+    bool              fShouldSaveReportedSlots;
+    bool              fShouldParentToFuncletReportSavedSlots;
     EE_ILEXCEPTION_CLAUSE ehClauseForCatch;
 #endif //FEATURE_EH_FUNCLETS
     Thread*           pThread;
@@ -701,7 +715,7 @@ class StackFrameIterator
 
         if (!ResetOnlyIntermediaryState)
         {
-            m_fFuncletNotSeen = false;
+            //m_fFuncletNotSeen = false;
             m_sfFuncletParent = StackFrame();
             m_fProcessNonFilterFunclet = false;
         }
@@ -754,6 +768,9 @@ class StackFrameIterator
     bool          m_movedPastFirstExInfo;
     // Indicates that no funclet was seen during the current stack walk yet
     bool          m_fFuncletNotSeen;
+
+    bool          m_fFoundFirstFunclet;
+
 #if defined(RECORD_RESUMABLE_FRAME_SP)
     LPVOID m_pvResumableFrameTargetSP;
 #endif // RECORD_RESUMABLE_FRAME_SP