Merge pull request #3232 from dotnet-bot/from-tfs

Merge changes from TFS

10 files changed, 126 insertions, 23 deletions

diff --git a/src/Native/Runtime/DebuggerHook.cpp b/src/Native/Runtime/DebuggerHook.cpp
index a6f65831d..688eb69f6 100644
--- a/src/Native/Runtime/DebuggerHook.cpp
+++ b/src/Native/Runtime/DebuggerHook.cpp
@@ -11,6 +11,8 @@ GVAL_IMPL_INIT(UInt32, g_numGcProtectionRequests, 0);
 
 #ifndef DACCESS_COMPILE
 
+/* static */ DebuggerProtectedBufferList* DebuggerHook::s_debuggerProtectedBuffers = nullptr;
+
 // TODO: Tab to space, overall, just to make sure I will actually do it :)
 // TODO: This structure needs to match with DBI
 struct FuncEvalParameterCommand
@@ -54,6 +56,8 @@ struct GcProtectionRequest
             {
                 // If the request requires extra memory, allocate for it
                 requests[i].address = (uint64_t)new (nothrow) uint8_t[requests[i].size];
+
+                // The debugger will handle the case when address is nullptr (we have to break our promise)
             }
         }
 
@@ -66,7 +70,19 @@ struct GcProtectionRequest
         {
             if (requests[i].type == 1)
             {
-                // What shall I do?
+                // TODO: Release them when there should be gone
+                DebuggerProtectedBufferList* tail = DebuggerHook::s_debuggerProtectedBuffers;
+                s_debuggerProtectedBuffers = new (std::nothrow) DebuggerProtectedBufferList();
+                if (s_debuggerProtectedBuffers == nullptr)
+                {
+                    // TODO: We cannot handle the debugger request to protect a buffer (we have to break our promise)
+                }
+                else
+                {
+                    s_debuggerProtectedBuffers->address = requests[i].address;
+                    s_debuggerProtectedBuffers->size = requests[i].size;
+                    s_debuggerProtectedBuffers->next = tail;
+                }
             }
         }
 
diff --git a/src/Native/Runtime/DebuggerHook.h b/src/Native/Runtime/DebuggerHook.h
index 26feedd06..fb8d0090c 100644
--- a/src/Native/Runtime/DebuggerHook.h
+++ b/src/Native/Runtime/DebuggerHook.h
@@ -17,10 +17,18 @@
 
 #ifndef DACCESS_COMPILE
 
+struct DebuggerProtectedBufferList
+{
+    UInt64 address;
+    UInt16 size;
+    struct DebuggerProtectedBufferList* next;
+};
+
 class DebuggerHook
 {
 public:
-	static void OnBeforeGcCollection();
+    static void OnBeforeGcCollection();
+    static DebuggerProtectedBufferList* s_debuggerProtectedBuffers;
 };
 
 #endif //!DACCESS_COMPILE
diff --git a/src/Native/Runtime/StackFrameIterator.cpp b/src/Native/Runtime/StackFrameIterator.cpp
index 979ef48d9..8c7d4f1d2 100644
--- a/src/Native/Runtime/StackFrameIterator.cpp
+++ b/src/Native/Runtime/StackFrameIterator.cpp
@@ -87,12 +87,13 @@ GVAL_IMPL_INIT(PTR_VOID, g_RhpRethrow2Addr, &RhpRethrow2);
 #define FAILFAST_OR_DAC_FAIL_UNCONDITIONALLY(msg) { ASSERT_UNCONDITIONALLY(msg); RhFailFast(); }
 #endif
 
-
 PTR_PInvokeTransitionFrame GetPInvokeTransitionFrame(PTR_VOID pTransitionFrame)
 {
     return static_cast<PTR_PInvokeTransitionFrame>(pTransitionFrame);
 }
 
+// TODO: Remove the assumption that there is only 1 func eval in progress
+GVAL_IMPL_INIT(UInt64, g_debuggermagic, 0);
 
 StackFrameIterator::StackFrameIterator(Thread * pThreadToWalk, PTR_VOID pInitialTransitionFrame)
 {
@@ -124,6 +125,7 @@ void StackFrameIterator::EnterInitialInvalidState(Thread * pThreadToWalk)
     m_HijackedReturnValueKind = GCRK_Unknown;
     m_pConservativeStackRangeLowerBound = NULL;
     m_pConservativeStackRangeUpperBound = NULL;
+    m_ShouldSkipRegularGcReporting = false;
     m_pendingFuncletFramePointer = NULL;
     m_pNextExInfo = pThreadToWalk->GetCurExInfo();
     m_ControlPC = 0;
@@ -1098,6 +1100,7 @@ UnwindOutOfCurrentManagedFrame:
 
     PTR_VOID pPreviousTransitionFrame;
     FAILFAST_OR_DAC_FAIL(GetCodeManager()->UnwindStackFrame(&m_methodInfo, &m_RegDisplay, &pPreviousTransitionFrame));
+    
     bool doingFuncletUnwind = GetCodeManager()->IsFunclet(&m_methodInfo);
 
     if (pPreviousTransitionFrame != NULL)
@@ -1357,8 +1360,23 @@ void StackFrameIterator::PrepareToYieldFrame()
 
     ASSERT(m_pInstance->FindCodeManagerByAddress(m_ControlPC));
 
+    bool atDebuggerHijackSite = (this->m_ControlPC == (PTR_VOID)(TADDR)g_debuggermagic);
+
+    if (atDebuggerHijackSite)
+    {
+        FAILFAST_OR_DAC_FAIL_MSG(m_pConservativeStackRangeLowerBound != NULL,
+            "Debugger hijack unwind is missing the required conservative range from a preceding transition thunk.");
+    }
+
     if (m_dwFlags & ApplyReturnAddressAdjustment)
+    {
+        FAILFAST_OR_DAC_FAIL_MSG(!atDebuggerHijackSite,
+            "EH stack walk is attempting to propagate an exception across a debugger hijack site.");
+
         m_ControlPC = AdjustReturnAddressBackward(m_ControlPC);
+    }
+
+    m_ShouldSkipRegularGcReporting = false;
 
     // Each time a managed frame is yielded, configure the iterator to explicitly indicate
     // whether or not unwinding to the current frame has revealed a stack range that must be
@@ -1383,8 +1401,45 @@ void StackFrameIterator::PrepareToYieldFrame()
         // that the upper bound computation never mutates m_RegDisplay.
         CalculateCurrentMethodState();
         ASSERT(IsValid());
-        UIntNative rawUpperBound = GetCodeManager()->GetConservativeUpperBoundForOutgoingArgs(&m_methodInfo, &m_RegDisplay);
-        m_pConservativeStackRangeUpperBound = (PTR_UIntNative)rawUpperBound;
+
+        if (!atDebuggerHijackSite)
+        {
+            UIntNative rawUpperBound = GetCodeManager()->GetConservativeUpperBoundForOutgoingArgs(&m_methodInfo, &m_RegDisplay);
+            m_pConservativeStackRangeUpperBound = (PTR_UIntNative)rawUpperBound;
+        }
+        else
+        {
+            // Debugger hijack points differ from all other unwind cases in that they are not
+            // guaranteed to be GC safe points, which implies that regular GC reporting will not
+            // protect the GC references that the function was using (in registers and/or in local
+            // stack slots) at the time of the hijack.
+            //
+            // GC references held in registers at the time of the hijack are reported by the
+            // debugger and therefore do not need to be handled here.  (The debugger does this by
+            // conservatively reporting the entire CONTEXT record which lists the full register
+            // set that was observed when the thread was stopped at the hijack point.)
+            //
+            // This code is therefore only responsible for reporting the GC references that were
+            // stored on the stack at the time of the hijack.  Conceptually, this is done by
+            // conservatively reporting the entire stack frame.  Since debugger hijack unwind
+            // always occurs via a UniversalTransitionThunk, the conservative lower bound
+            // published by the thunk can be used as a workable lower bound for the entire stack
+            // frame.
+            //
+            // Computing a workable upper bound is more difficult, especially because the stack
+            // frame of a funclet can contain FP-relative locals which reside arbitrarily far up
+            // the stack compared to the current SP.  The top of the thread's stack is currently
+            // used as an extremely conservative upper bound as away to cover all cases without
+            // introducing more stack walker complexity.
+
+            PTR_VOID pStackLow;
+            PTR_VOID pStackHigh;
+#ifndef DACCESS_COMPILE
+            m_pThread->GetStackBounds(&pStackLow, &pStackHigh);
+#endif
+            m_pConservativeStackRangeUpperBound = (PTR_UIntNative)pStackHigh;
+            m_ShouldSkipRegularGcReporting = true;
+        }
 
         ASSERT(m_pConservativeStackRangeLowerBound != NULL);
         ASSERT(m_pConservativeStackRangeUpperBound != NULL);
@@ -1541,7 +1596,7 @@ PTR_VOID StackFrameIterator::AdjustReturnAddressBackward(PTR_VOID controlPC)
 
 // static
 StackFrameIterator::ReturnAddressCategory StackFrameIterator::CategorizeUnadjustedReturnAddress(PTR_VOID returnAddress)
-{
+{    
 #if defined(USE_PORTABLE_HELPERS) // @TODO: CORERT: no portable thunks are defined
 
     return InManagedCode;
@@ -1584,6 +1639,11 @@ StackFrameIterator::ReturnAddressCategory StackFrameIterator::CategorizeUnadjust
 #endif // defined(USE_PORTABLE_HELPERS)
 }
 
+bool StackFrameIterator::ShouldSkipRegularGcReporting()
+{
+    return m_ShouldSkipRegularGcReporting;
+}
+
 #ifndef DACCESS_COMPILE
 
 COOP_PINVOKE_HELPER(Boolean, RhpSfiInit, (StackFrameIterator* pThis, PAL_LIMITED_CONTEXT* pStackwalkCtx, Boolean instructionFault))
diff --git a/src/Native/Runtime/StackFrameIterator.h b/src/Native/Runtime/StackFrameIterator.h
index 214a7aed6..343b52e84 100644
--- a/src/Native/Runtime/StackFrameIterator.h
+++ b/src/Native/Runtime/StackFrameIterator.h
@@ -56,6 +56,10 @@ public:
     bool HasStackRangeToReportConservatively();
     void GetStackRangeToReportConservatively(PTR_RtuObjectRef * ppLowerBound, PTR_RtuObjectRef * ppUpperBound);
 
+    // Debugger Hijacked frame looks very much like a usual managed frame except when the 
+    // frame must be reported conservatively, and when that happens, regular GC reporting should be skipped
+    bool ShouldSkipRegularGcReporting();
+
 private:
     // The invoke of a funclet is a bit special and requires an assembly thunk, but we don't want to break the
     // stackwalk due to this.  So this routine will unwind through the assembly thunks used to invoke funclets.
@@ -187,5 +191,6 @@ protected:
     PreservedRegPtrs    m_funcletPtrs;  // @TODO: Placing the 'scratch space' in the StackFrameIterator is not
                                         // preferred because not all StackFrameIterators require this storage 
                                         // space.  However, the implementation simpler by doing it this way.
+    bool                m_ShouldSkipRegularGcReporting;
 };
 
diff --git a/src/Native/Runtime/amd64/AsmOffsetsCpu.h b/src/Native/Runtime/amd64/AsmOffsetsCpu.h
index 5dd0c02ff..1bc09bad0 100644
--- a/src/Native/Runtime/amd64/AsmOffsetsCpu.h
+++ b/src/Native/Runtime/amd64/AsmOffsetsCpu.h
@@ -9,7 +9,7 @@
 // NOTE: the offsets MUST be in hex notation WITHOUT the 0x prefix
 
 #ifndef UNIX_AMD64_ABI
-PLAT_ASM_SIZEOF(250, ExInfo)
+PLAT_ASM_SIZEOF(260, ExInfo)
 PLAT_ASM_OFFSET(0, ExInfo, m_pPrevExInfo)
 PLAT_ASM_OFFSET(8, ExInfo, m_pExContext)
 PLAT_ASM_OFFSET(10, ExInfo, m_exception)
@@ -17,7 +17,7 @@ PLAT_ASM_OFFSET(18, ExInfo, m_kind)
 PLAT_ASM_OFFSET(19, ExInfo, m_passNumber)
 PLAT_ASM_OFFSET(1c, ExInfo, m_idxCurClause)
 PLAT_ASM_OFFSET(20, ExInfo, m_frameIter)
-PLAT_ASM_OFFSET(240, ExInfo, m_notifyDebuggerSP)
+PLAT_ASM_OFFSET(250, ExInfo, m_notifyDebuggerSP)
 
 PLAT_ASM_OFFSET(0, PInvokeTransitionFrame, m_RIP)
 PLAT_ASM_OFFSET(8, PInvokeTransitionFrame, m_FramePointer)
@@ -25,7 +25,7 @@ PLAT_ASM_OFFSET(10, PInvokeTransitionFrame, m_pThread)
 PLAT_ASM_OFFSET(18, PInvokeTransitionFrame, m_dwFlags)
 PLAT_ASM_OFFSET(20, PInvokeTransitionFrame, m_PreservedRegs)
 
-PLAT_ASM_SIZEOF(220, StackFrameIterator)
+PLAT_ASM_SIZEOF(230, StackFrameIterator)
 PLAT_ASM_OFFSET(10, StackFrameIterator, m_FramePointer)
 PLAT_ASM_OFFSET(18, StackFrameIterator, m_ControlPC)
 PLAT_ASM_OFFSET(20, StackFrameIterator, m_RegDisplay)
diff --git a/src/Native/Runtime/arm/AsmOffsetsCpu.h b/src/Native/Runtime/arm/AsmOffsetsCpu.h
index 1c1d1978c..baf8da385 100644
--- a/src/Native/Runtime/arm/AsmOffsetsCpu.h
+++ b/src/Native/Runtime/arm/AsmOffsetsCpu.h
@@ -8,7 +8,7 @@
 //
 // NOTE: the offsets MUST be in hex notation WITHOUT the 0x prefix
 
-PLAT_ASM_SIZEOF(130, ExInfo)
+PLAT_ASM_SIZEOF(138, ExInfo)
 PLAT_ASM_OFFSET(0, ExInfo, m_pPrevExInfo)
 PLAT_ASM_OFFSET(4, ExInfo, m_pExContext)
 PLAT_ASM_OFFSET(8, ExInfo, m_exception)
@@ -16,7 +16,7 @@ PLAT_ASM_OFFSET(0c, ExInfo, m_kind)
 PLAT_ASM_OFFSET(0d, ExInfo, m_passNumber)
 PLAT_ASM_OFFSET(10, ExInfo, m_idxCurClause)
 PLAT_ASM_OFFSET(18, ExInfo, m_frameIter)
-PLAT_ASM_OFFSET(128, ExInfo, m_notifyDebuggerSP)
+PLAT_ASM_OFFSET(130, ExInfo, m_notifyDebuggerSP)
 
 PLAT_ASM_OFFSET(4, PInvokeTransitionFrame, m_RIP)
 PLAT_ASM_OFFSET(8, PInvokeTransitionFrame, m_FramePointer)
@@ -24,7 +24,7 @@ PLAT_ASM_OFFSET(0c, PInvokeTransitionFrame, m_pThread)
 PLAT_ASM_OFFSET(10, PInvokeTransitionFrame, m_dwFlags)
 PLAT_ASM_OFFSET(14, PInvokeTransitionFrame, m_PreservedRegs)
 
-PLAT_ASM_SIZEOF(110, StackFrameIterator)
+PLAT_ASM_SIZEOF(118, StackFrameIterator)
 PLAT_ASM_OFFSET(08, StackFrameIterator, m_FramePointer)
 PLAT_ASM_OFFSET(0c, StackFrameIterator, m_ControlPC)
 PLAT_ASM_OFFSET(10, StackFrameIterator, m_RegDisplay)
diff --git a/src/Native/Runtime/gcrhenv.cpp b/src/Native/Runtime/gcrhenv.cpp
index 782572acc..53f83eb7a 100644
--- a/src/Native/Runtime/gcrhenv.cpp
+++ b/src/Native/Runtime/gcrhenv.cpp
@@ -72,6 +72,7 @@
 
 GPTR_IMPL(EEType, g_pFreeObjectEEType);
 
+#include "DebuggerHook.h"
 
 #ifndef DACCESS_COMPILE
 
@@ -920,6 +921,8 @@ void GCToEEInterface::RestartEE(bool /*bFinishedGC*/)
 
 void GCToEEInterface::GcStartWork(int condemned, int /*max_gen*/)
 {
+    DebuggerHook::OnBeforeGcCollection();
+    
     // Invoke any registered callouts for the start of the collection.
     RestrictedCallouts::InvokeGcCallouts(GCRC_StartCollection, condemned);
 }
diff --git a/src/Native/Runtime/gcrhscan.cpp b/src/Native/Runtime/gcrhscan.cpp
index 1216a99bc..31ac4d7a5 100644
--- a/src/Native/Runtime/gcrhscan.cpp
+++ b/src/Native/Runtime/gcrhscan.cpp
@@ -36,6 +36,8 @@
 
 #ifndef DACCESS_COMPILE
 
+void GcEnumObjectsConservatively(PTR_PTR_Object ppLowerBound, PTR_PTR_Object ppUpperBound, EnumGcRefCallbackFunc * fnGcEnumRef, EnumGcRefScanContext * pSc);
+
 void EnumAllStaticGCRefs(EnumGcRefCallbackFunc * fn, EnumGcRefScanContext * sc)
 {
     GetRuntimeInstance()->EnumAllStaticGCRefs(reinterpret_cast<void*>(fn), sc);
@@ -47,7 +49,13 @@ void EnumAllStaticGCRefs(EnumGcRefCallbackFunc * fn, EnumGcRefScanContext * sc)
  
 void GCToEEInterface::GcScanRoots(EnumGcRefCallbackFunc * fn,  int condemned, int max_gen, EnumGcRefScanContext * sc)
 {
-    DebuggerHook::OnBeforeGcCollection();
+    DebuggerProtectedBufferList* cursor = DebuggerHook::s_debuggerProtectedBuffers;
+    while (cursor != nullptr)
+    {
+        GcEnumObjectsConservatively((PTR_PTR_Object)cursor->address, (PTR_PTR_Object)(cursor->address + cursor->size), fn, sc);
+        cursor = cursor->next;
+    }
+
     // STRESS_LOG1(LF_GCROOTS, LL_INFO10, "GCScan: Phase = %s\n", sc->promotion ? "promote" : "relocate");
 
     FOREACH_THREAD(pThread)
diff --git a/src/Native/Runtime/i386/AsmOffsetsCpu.h b/src/Native/Runtime/i386/AsmOffsetsCpu.h
index a85192760..d7f70f4f2 100644
--- a/src/Native/Runtime/i386/AsmOffsetsCpu.h
+++ b/src/Native/Runtime/i386/AsmOffsetsCpu.h
@@ -8,7 +8,7 @@
 //
 // NOTE: the offsets MUST be in hex notation WITHOUT the 0x prefix
 
-PLAT_ASM_SIZEOF(b8, ExInfo)
+PLAT_ASM_SIZEOF(bc, ExInfo)
 PLAT_ASM_OFFSET(0, ExInfo, m_pPrevExInfo)
 PLAT_ASM_OFFSET(4, ExInfo, m_pExContext)
 PLAT_ASM_OFFSET(8, ExInfo, m_exception)
@@ -16,7 +16,7 @@ PLAT_ASM_OFFSET(0c, ExInfo, m_kind)
 PLAT_ASM_OFFSET(0d, ExInfo, m_passNumber)
 PLAT_ASM_OFFSET(10, ExInfo, m_idxCurClause)
 PLAT_ASM_OFFSET(14, ExInfo, m_frameIter)
-PLAT_ASM_OFFSET(b4, ExInfo, m_notifyDebuggerSP)
+PLAT_ASM_OFFSET(b8, ExInfo, m_notifyDebuggerSP)
 
 PLAT_ASM_OFFSET(0, PInvokeTransitionFrame, m_RIP)
 PLAT_ASM_OFFSET(4, PInvokeTransitionFrame, m_FramePointer)
@@ -24,7 +24,7 @@ PLAT_ASM_OFFSET(8, PInvokeTransitionFrame, m_pThread)
 PLAT_ASM_OFFSET(0c, PInvokeTransitionFrame, m_dwFlags)
 PLAT_ASM_OFFSET(10, PInvokeTransitionFrame, m_PreservedRegs)
 
-PLAT_ASM_SIZEOF(a0, StackFrameIterator)
+PLAT_ASM_SIZEOF(a4, StackFrameIterator)
 PLAT_ASM_OFFSET(08, StackFrameIterator, m_FramePointer)
 PLAT_ASM_OFFSET(0c, StackFrameIterator, m_ControlPC)
 PLAT_ASM_OFFSET(10, StackFrameIterator, m_RegDisplay)
diff --git a/src/Native/Runtime/thread.cpp b/src/Native/Runtime/thread.cpp
index 62a458cbd..db1668383 100644
--- a/src/Native/Runtime/thread.cpp
+++ b/src/Native/Runtime/thread.cpp
@@ -481,13 +481,16 @@ void Thread::GcScanRootsWorker(void * pfnEnumCallback, void * pvCallbackData, St
             frameIterator.CalculateCurrentMethodState();
         
             STRESS_LOG1(LF_GCROOTS, LL_INFO1000, "Scanning method %pK\n", (void*)frameIterator.GetRegisterSet()->IP);
-       
-            RedhawkGCInterface::EnumGcRefs(frameIterator.GetCodeManager(),
-                                           frameIterator.GetMethodInfo(), 
-                                           frameIterator.GetEffectiveSafePointAddress(),
-                                           frameIterator.GetRegisterSet(),
-                                           pfnEnumCallback,
-                                           pvCallbackData);
+
+            if (!frameIterator.ShouldSkipRegularGcReporting())
+            {
+                RedhawkGCInterface::EnumGcRefs(frameIterator.GetCodeManager(),
+                                               frameIterator.GetMethodInfo(), 
+                                               frameIterator.GetEffectiveSafePointAddress(),
+                                               frameIterator.GetRegisterSet(),
+                                               pfnEnumCallback,
+                                               pvCallbackData);
+            }
         
             // Each enumerated frame (including the first one) may have an associated stack range we need to
             // report conservatively (every pointer aligned value that looks like it might be a GC reference is