Address feedback

Author: mdh1418

src/coreclr/vm/proftoeeinterfaceimpl.cpp

@@ -6855,6 +6855,13 @@ HRESULT ProfToEEInterfaceImpl::SuspendRuntime()
         (LF_CORPROF,
         LL_INFO1000,
         "**PROF: SuspendRuntime\n"));
+    if (!IsCalledAsynchronously() && !AreCallbackStateFlagsSet(COR_PRF_CALLBACKSTATE_IN_TRIGGERS_SCOPE))
+    {
+        LOG((LF_CORPROF,
+             LL_ERROR,
+             "**PROF: ERROR: Returning CORPROF_E_UNSUPPORTED_CALL_SEQUENCE due to illegal asynchronous profiler call\n"));
+        return CORPROF_E_UNSUPPORTED_CALL_SEQUENCE;
+    }
 
     if (!g_fEEStarted)
     {
@@ -6887,6 +6894,13 @@ HRESULT ProfToEEInterfaceImpl::ResumeRuntime()
         (LF_CORPROF,
         LL_INFO1000,
         "**PROF: ResumeRuntime\n"));
+    if (!IsCalledAsynchronously() && !AreCallbackStateFlagsSet(COR_PRF_CALLBACKSTATE_IN_TRIGGERS_SCOPE))
+    {
+        LOG((LF_CORPROF,
+             LL_ERROR,
+             "**PROF: ERROR: Returning CORPROF_E_UNSUPPORTED_CALL_SEQUENCE due to illegal asynchronous profiler call\n"));
+        return CORPROF_E_UNSUPPORTED_CALL_SEQUENCE;
+    }
 
     if (!g_fEEStarted)
     {
@@ -7644,6 +7658,13 @@ HRESULT ProfToEEInterfaceImpl::EnumerateGCHeapObjects(ObjectCallback callback, v
         (LF_CORPROF,
         LL_INFO1000,
         "**PROF: EnumerateGCHeapObjects.\n"));
+    if (!IsCalledAsynchronously() && !AreCallbackStateFlagsSet(COR_PRF_CALLBACKSTATE_IN_TRIGGERS_SCOPE))
+    {
+        LOG((LF_CORPROF,
+             LL_ERROR,
+             "**PROF: ERROR: Returning CORPROF_E_UNSUPPORTED_CALL_SEQUENCE due to illegal asynchronous profiler call\n"));
+        return CORPROF_E_UNSUPPORTED_CALL_SEQUENCE;
+    }
 
     if (callback == nullptr)
     {
@@ -7655,22 +7676,38 @@ HRESULT ProfToEEInterfaceImpl::EnumerateGCHeapObjects(ObjectCallback callback, v
         return CORPROF_E_RUNTIME_UNINITIALIZED;
     }
 
-    bool ownEESuspension = FALSE;
-    if (!ThreadSuspend::SysIsSuspendInProgress() && (ThreadSuspend::GetSuspensionThread() == 0))
+    bool ownEESuspension = false;
+    bool suspendedByThisThread = (ThreadSuspend::GetSuspensionThread() == GetThreadNULLOk());
+    if (suspendedByThisThread && !g_profControlBlock.fProfilerRequestedRuntimeSuspend)
     {
-        g_profControlBlock.fProfilerRequestedRuntimeSuspend = TRUE;
-        ThreadSuspend::SuspendEE(ThreadSuspend::SUSPEND_REASON::SUSPEND_FOR_PROFILER);
-        ownEESuspension = TRUE;
+        // This thread is responsible for suspending the runtime so we can't block
+        // waiting for the runtime to resume. However it wasn't the profiler call that did
+        // the suspend so we don't know what state the GC heap is in. Other threads also might
+        // be modifying it concurrently. In the future more analysis or coordination with other
+        // suspenders might let us narrow the scope of this error condition, but we have no need
+        // to do this now.
+        return CORPROF_E_SUSPENSION_IN_PROGRESS;
     }
-    else if (!g_profControlBlock.fProfilerRequestedRuntimeSuspend)
+    else if (suspendedByThisThread && g_profControlBlock.fProfilerRequestedRuntimeSuspend)
     {
-        return CORPROF_E_SUSPENSION_IN_PROGRESS;
+        // This thread previously invoked ICorProfiler::SuspendRuntime(). Our caller
+        // has the responsibility to resume the runtime no earlier than when this API returns
+        // and to preserve the GC heap in a consistent state. We should avoid invoking
+        // SuspendEE/ResumeEE again in this function because those APIs do not support
+        // re-entrant suspends.
     }
     else
     {
-        // The profiler requested a runtime suspension before invoking this API,
-        // so the responsibility of resuming the runtime is outside the scope of this API.
-        // Given that the profiler owns the suspension, walking the GC Heap is safe.
+        _ASSERTE(!suspendedByThisThread);
+        // Its possible some background threads are suspending and resuming the runtime
+        // concurrently. We need to suspend the runtime on this thread to be certain the heap
+        // stays in a walkable state for the duration that we need it to. Our call to
+        // SuspendEE() may race with other threads by design and this thread may block
+        // arbitrarily long inside SuspendEE() for other threads to complete their own
+        // suspensions.
+        g_profControlBlock.fProfilerRequestedRuntimeSuspend = TRUE;
+        ThreadSuspend::SuspendEE(ThreadSuspend::SUSPEND_REASON::SUSPEND_FOR_PROFILER);
+        ownEESuspension = TRUE;
     }
 
     // Suspending EE ensures safe object inspection. We permit the GC Heap walk callback to

src/tests/profiler/gcheapenumeration/gcheapenumeration.cs

@@ -15,18 +15,15 @@ class GCHeapEnumerationTests
         static List<object> _rootObjects = new List<object>();
 
         [DllImport("Profiler")]
-        private static extern void EnumerateGCHeapObjects();
-
-        [DllImport("Profiler")]
-        private static extern void EnumerateHeapObjectsInBackgroundThread();
+        private static extern void EnumerateGCHeapObjectsWithoutProfilerRequestedRuntimeSuspension();
 
         [DllImport("Profiler")]
         private static extern void EnumerateGCHeapObjectsWithinProfilerRequestedRuntimeSuspension();
 
         public static int EnumerateGCHeapObjectsSingleThreadNoPriorSuspension()
         {
             _rootObjects.Add(new CustomGCHeapObject());
-            EnumerateGCHeapObjects();
+            EnumerateGCHeapObjectsWithoutProfilerRequestedRuntimeSuspension();
             return 100;
         }
 
@@ -37,39 +34,15 @@ public static int EnumerateGCHeapObjectsSingleThreadWithinProfilerRequestedRunti
             return 100;
         }
 
-        public static int EnumerateGCHeapObjectsInBackgroundThreadWithRuntimeSuspension()
+        // Test invoking ProfToEEInterfaceImpl::EnumerateGCHeapObjects during non-profiler requested runtime suspension, e.g. during GC
+        // ProfToEEInterfaceImpl::EnumerateGCHeapObjects should be invoked by the GarbageCollectionStarted callback
+        public static int EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension()
         {
             _rootObjects.Add(new CustomGCHeapObject());
-            EnumerateHeapObjectsInBackgroundThread();
             GC.Collect();
             return 100;
         }
 
-        public static int EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension()
-        {
-            ManualResetEvent startEvent = new ManualResetEvent(false);
-            Thread gcThread = new Thread(() =>
-            {
-                startEvent.WaitOne();
-                GC.Collect();
-            });
-            gcThread.Name = "GC.Collect";
-            gcThread.Start();
-
-            Thread enumerateThread = new Thread(() =>
-            {
-                startEvent.WaitOne();
-                Thread.Sleep(1000);
-                _rootObjects.Add(new CustomGCHeapObject());
-                EnumerateGCHeapObjects();
-            });
-            enumerateThread.Name = "EnumerateGCHeapObjects";
-            enumerateThread.Start();
-
-            startEvent.Set();
-            return 100;
-        }
-
         public static int Main(string[] args)
         {
             if (args.Length > 0 && args[0].Equals("RunTest", StringComparison.OrdinalIgnoreCase))
@@ -82,38 +55,30 @@ public static int Main(string[] args)
                     case nameof(EnumerateGCHeapObjectsSingleThreadWithinProfilerRequestedRuntimeSuspension):
                         return EnumerateGCHeapObjectsSingleThreadWithinProfilerRequestedRuntimeSuspension();
 
-                    case nameof(EnumerateGCHeapObjectsInBackgroundThreadWithRuntimeSuspension):
-                        return EnumerateGCHeapObjectsInBackgroundThreadWithRuntimeSuspension();
-
                     case nameof(EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension):
                         return EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension();
                 }
             }
 
-            if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsSingleThreadNoPriorSuspension), "FALSE"))
+            if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsSingleThreadNoPriorSuspension), false))
             {
                 return 101;
             }
 
-            if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsSingleThreadWithinProfilerRequestedRuntimeSuspension), "FALSE"))
+            if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsSingleThreadWithinProfilerRequestedRuntimeSuspension), false))
             {
                 return 102;
             }
 
-            if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsInBackgroundThreadWithRuntimeSuspension), "TRUE"))
+            if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension), true))
             {
                 return 103;
             }
 
-            if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension), "FALSE"))
-            {
-                return 104;
-            }
-
             return 100;
         }
 
-        private static bool RunProfilerTest(string testName, string shouldSetMonitorGCEventMask)
+        private static bool RunProfilerTest(string testName, bool shouldSetMonitorGCEventMask)
         {
             try
             {
@@ -123,7 +88,7 @@ private static bool RunProfilerTest(string testName, string shouldSetMonitorGCEv
                                               profileeArguments: testName,
                                               envVars: new Dictionary<string, string>
                                               {
-                                                {ShouldSetMonitorGCEventMaskEnvVar, shouldSetMonitorGCEventMask},
+                                                {ShouldSetMonitorGCEventMaskEnvVar, shouldSetMonitorGCEventMask ? "TRUE" : "FALSE"},
                                               }
                                               ) == 100;
             }