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,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
 };
 
 //*****************************************************************************

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/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;

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);

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
@@ -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).

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,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
                                         {

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