[release/9.0-staging] [Profiler] Avoid Recursive ThreadStoreLock in Profiling Thread Enumerator

src/coreclr/vm/profilingenumerators.cpp

@@ -556,9 +556,11 @@ HRESULT ProfilerThreadEnum::Init()
     }
     CONTRACTL_END;
 
+    // If EnumThreads is called from a profiler callback where the runtime is already suspended,
+    // don't recursively acquire/release the ThreadStore Lock.
     // If a profiler has requested that the runtime suspend to do stack snapshots, it
     // will be holding the ThreadStore lock already
-    ThreadStoreLockHolder tsLock(!g_profControlBlock.fProfilerRequestedRuntimeSuspend);
+    ThreadStoreLockHolder tsLock(!ThreadStore::HoldingThreadStore() && !g_profControlBlock.fProfilerRequestedRuntimeSuspend);
 
     Thread * pThread = NULL;
 

src/coreclr/vm/proftoeeinterfaceimpl.cpp

@@ -6873,8 +6873,8 @@ HRESULT ProfToEEInterfaceImpl::SuspendRuntime()
         return CORPROF_E_SUSPENSION_IN_PROGRESS;
     }
 
-    g_profControlBlock.fProfilerRequestedRuntimeSuspend = TRUE;
     ThreadSuspend::SuspendEE(ThreadSuspend::SUSPEND_REASON::SUSPEND_FOR_PROFILER);
+    g_profControlBlock.fProfilerRequestedRuntimeSuspend = TRUE;
     return S_OK;
 }
 
@@ -6912,8 +6912,8 @@ HRESULT ProfToEEInterfaceImpl::ResumeRuntime()
         return CORPROF_E_UNSUPPORTED_CALL_SEQUENCE;
     }
 
-    ThreadSuspend::RestartEE(FALSE /* bFinishedGC */, TRUE /* SuspendSucceeded */);
     g_profControlBlock.fProfilerRequestedRuntimeSuspend = FALSE;
+    ThreadSuspend::RestartEE(FALSE /* bFinishedGC */, TRUE /* SuspendSucceeded */);
     return S_OK;
 }
 
@@ -7705,8 +7705,8 @@ HRESULT ProfToEEInterfaceImpl::EnumerateGCHeapObjects(ObjectCallback callback, v
         // 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);
+        g_profControlBlock.fProfilerRequestedRuntimeSuspend = TRUE;
         ownEESuspension = TRUE;
     }
 
@@ -7738,8 +7738,8 @@ HRESULT ProfToEEInterfaceImpl::EnumerateGCHeapObjects(ObjectCallback callback, v
 
     if (ownEESuspension)
     {
-        ThreadSuspend::RestartEE(FALSE /* bFinishedGC */, TRUE /* SuspendSucceeded */);
         g_profControlBlock.fProfilerRequestedRuntimeSuspend = FALSE;
+        ThreadSuspend::RestartEE(FALSE /* bFinishedGC */, TRUE /* SuspendSucceeded */);
     }
 
     return hr;

src/tests/profiler/native/CMakeLists.txt

@@ -5,11 +5,11 @@ project(Profiler)
 set(SOURCES
     assemblyprofiler/assemblyprofiler.cpp
     eltprofiler/slowpatheltprofiler.cpp
+    enumthreadsprofiler/enumthreadsprofiler.cpp
     eventpipeprofiler/eventpipereadingprofiler.cpp
     eventpipeprofiler/eventpipewritingprofiler.cpp
     eventpipeprofiler/eventpipemetadatareader.cpp
     gcallocateprofiler/gcallocateprofiler.cpp
-    nongcheap/nongcheap.cpp
     gcbasicprofiler/gcbasicprofiler.cpp
     gcheapenumerationprofiler/gcheapenumerationprofiler.cpp
     gcheapenumerationprofiler/gcheapenumerationprofiler.def
@@ -20,6 +20,7 @@ set(SOURCES
     metadatagetdispenser/metadatagetdispenser.cpp
     moduleload/moduleload.cpp
     multiple/multiple.cpp
+    nongcheap/nongcheap.cpp
     nullprofiler/nullprofiler.cpp
     rejitprofiler/rejitprofiler.cpp
     rejitprofiler/ilrewriter.cpp

src/tests/profiler/native/classfactory.cpp

@@ -3,6 +3,7 @@
 
 #include "classfactory.h"
 #include "eltprofiler/slowpatheltprofiler.h"
+#include "enumthreadsprofiler/enumthreadsprofiler.h"
 #include "eventpipeprofiler/eventpipereadingprofiler.h"
 #include "eventpipeprofiler/eventpipewritingprofiler.h"
 #include "getappdomainstaticaddress/getappdomainstaticaddress.h"
@@ -144,6 +145,10 @@ HRESULT STDMETHODCALLTYPE ClassFactory::CreateInstance(IUnknown *pUnkOuter, REFI
     {
         profiler = new GCHeapEnumerationProfiler();
     }
+    else if (clsid == EnumThreadsProfiler::GetClsid())
+    {
+        profiler = new EnumThreadsProfiler();
+    }
     else
     {
         printf("No profiler found in ClassFactory::CreateInstance. Did you add your profiler to the list?\n");

src/tests/profiler/native/enumthreadsprofiler/enumthreadsprofiler.cpp

@@ -0,0 +1,104 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+#include "enumthreadsprofiler.h"
+
+GUID EnumThreadsProfiler::GetClsid()
+{
+    // {0742962D-2ED3-44B0-BA84-06B1EF0A0A0B}
+    GUID clsid = { 0x0742962d, 0x2ed3, 0x44b0,{ 0xba, 0x84, 0x06, 0xb1, 0xef, 0x0a, 0x0a, 0x0b } };
+	return clsid;
+}
+
+HRESULT EnumThreadsProfiler::Initialize(IUnknown* pICorProfilerInfoUnk)
+{
+    Profiler::Initialize(pICorProfilerInfoUnk);
+    printf("EnumThreadsProfiler::Initialize\n");
+
+    HRESULT hr = S_OK;
+    if (FAILED(hr = pCorProfilerInfo->SetEventMask2(COR_PRF_MONITOR_GC | COR_PRF_MONITOR_SUSPENDS, COR_PRF_HIGH_MONITOR_NONE)))
+    {
+        printf("FAIL: ICorProfilerInfo::SetEventMask2() failed hr=0x%x", hr);
+        IncrementFailures();
+    }
+
+    return hr;
+}
+
+HRESULT STDMETHODCALLTYPE EnumThreadsProfiler::GarbageCollectionStarted(int cGenerations, BOOL generationCollected[], COR_PRF_GC_REASON reason)
+{
+    SHUTDOWNGUARD();
+
+    printf("EnumThreadsProfiler::GarbageCollectionStarted\n");
+    _gcStarts.fetch_add(1, std::memory_order_relaxed);
+    if (_gcStarts < _gcFinishes)
+    {
+        IncrementFailures();
+        printf("EnumThreadsProfiler::GarbageCollectionStarted: FAIL: Expected GCStart >= GCFinish. Start=%d, Finish=%d\n", (int)_gcStarts, (int)_gcFinishes);
+    }
+
+    return S_OK;
+}
+
+HRESULT STDMETHODCALLTYPE EnumThreadsProfiler::GarbageCollectionFinished()
+{
+    SHUTDOWNGUARD();
+
+    printf("EnumThreadsProfiler::GarbageCollectionFinished\n");
+    _gcFinishes.fetch_add(1, std::memory_order_relaxed);
+    if (_gcStarts < _gcFinishes)
+    {
+        IncrementFailures();
+        printf("EnumThreadsProfiler::GarbageCollectionFinished: FAIL: Expected GCStart >= GCFinish. Start=%d, Finish=%d\n", (int)_gcStarts, (int)_gcFinishes);
+    }
+
+    return S_OK;
+}
+
+HRESULT STDMETHODCALLTYPE EnumThreadsProfiler::RuntimeSuspendFinished()
+{
+    SHUTDOWNGUARD();
+
+    printf("EnumThreadsProfiler::RuntimeSuspendFinished\n");
+
+    ICorProfilerThreadEnum* threadEnum = nullptr;
+    HRESULT enumThreadsHR = pCorProfilerInfo->EnumThreads(&threadEnum);
+    printf("Finished enumerating threads\n");
+    _profilerEnumThreadsCompleted.fetch_add(1, std::memory_order_relaxed);
+    threadEnum->Release();
+    return enumThreadsHR;
+}
+
+HRESULT EnumThreadsProfiler::Shutdown()
+{
+    Profiler::Shutdown();
+
+    if (_gcStarts == 0)
+    {
+        printf("EnumThreadsProfiler::Shutdown: FAIL: Expected GarbageCollectionStarted to be called\n");
+    }
+    else if (_gcFinishes == 0)
+    {
+        printf("EnumThreadsProfiler::Shutdown: FAIL: Expected GarbageCollectionFinished to be called\n");
+    }
+    else if (_profilerEnumThreadsCompleted == 0)
+    {
+        printf("EnumThreadsProfiler::Shutdown: FAIL: Expected RuntimeSuspendFinished to be called and EnumThreads completed\n");
+    }
+    else if(_failures == 0)
+    {
+        printf("PROFILER TEST PASSES\n");
+    }
+    else
+    {
+        // failures were printed earlier when _failures was incremented
+    }
+    fflush(stdout);
+
+    return S_OK;
+}
+
+void EnumThreadsProfiler::IncrementFailures()
+{
+    _failures.fetch_add(1, std::memory_order_relaxed);
+}

src/tests/profiler/native/enumthreadsprofiler/enumthreadsprofiler.h

@@ -0,0 +1,34 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+#pragma once
+
+#include "../profiler.h"
+
+class EnumThreadsProfiler : public Profiler
+{
+public:
+    EnumThreadsProfiler() : Profiler(),
+        _gcStarts(0),
+        _gcFinishes(0),
+        _profilerEnumThreadsCompleted(0),
+        _failures(0)
+    {}
+
+    // Profiler callbacks override
+	static GUID GetClsid();
+    virtual HRESULT STDMETHODCALLTYPE Initialize(IUnknown* pICorProfilerInfoUnk);
+    virtual HRESULT STDMETHODCALLTYPE GarbageCollectionStarted(int cGenerations, BOOL generationCollected[], COR_PRF_GC_REASON reason);
+    virtual HRESULT STDMETHODCALLTYPE GarbageCollectionFinished();
+    virtual HRESULT STDMETHODCALLTYPE RuntimeSuspendFinished();
+    virtual HRESULT STDMETHODCALLTYPE Shutdown();
+
+    // Helper methods
+    void IncrementFailures();
+
+private:
+    std::atomic<int> _gcStarts;
+    std::atomic<int> _gcFinishes;
+    std::atomic<int> _profilerEnumThreadsCompleted;
+    std::atomic<int> _failures;
+};

src/tests/profiler/unittest/enumthreads.cs

@@ -0,0 +1,56 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+
+namespace Profiler.Tests
+{
+    class EnumThreadsTests
+    {
+        static readonly Guid EnumThreadsProfilerGuid = new Guid("0742962D-2ED3-44B0-BA84-06B1EF0A0A0B");
+
+        public static int EnumerateThreadsWithNonProfilerRequestedRuntimeSuspension()
+        {
+            GC.Collect();
+            return 100;
+        }
+
+        public static int Main(string[] args)
+        {
+            if (args.Length > 0 && args[0].Equals("RunTest", StringComparison.OrdinalIgnoreCase))
+            {
+                switch (args[1])
+                {
+                    case nameof(EnumerateThreadsWithNonProfilerRequestedRuntimeSuspension):
+                        return EnumerateThreadsWithNonProfilerRequestedRuntimeSuspension();
+                    default:
+                        return 102;
+                }
+            }
+
+            if (!RunProfilerTest(nameof(EnumerateThreadsWithNonProfilerRequestedRuntimeSuspension)))
+            {
+                return 101;
+            }
+
+            return 100;
+        }
+
+        private static bool RunProfilerTest(string testName)
+        {
+            try
+            {
+                return ProfilerTestRunner.Run(profileePath: System.Reflection.Assembly.GetExecutingAssembly().Location,
+                                              testName: "EnumThreads",
+                                              profilerClsid: EnumThreadsProfilerGuid,
+                                              profileeArguments: testName
+                                              ) == 100;
+            }
+            catch (Exception ex)
+            {
+                Console.WriteLine(ex);
+            }
+            return false;
+        }
+    }
+}

src/tests/profiler/unittest/enumthreads.csproj

@@ -0,0 +1,21 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <TargetFrameworkIdentifier>.NETCoreApp</TargetFrameworkIdentifier>
+    <OutputType>exe</OutputType>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <Optimize>true</Optimize>
+    <!-- This test provides no interesting scenarios for GCStress -->
+    <GCStressIncompatible>true</GCStressIncompatible>
+    <!-- The test launches a secondary process and process launch creates
+    an infinite event loop in the SocketAsyncEngine on Linux. Since
+    runincontext loads even framework assemblies into the unloadable
+    context, locals in this loop prevent unloading -->
+    <UnloadabilityIncompatible>true</UnloadabilityIncompatible>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+    <ProjectReference Include="$(TestSourceDir)Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" />
+    <ProjectReference Include="../common/profiler_common.csproj" />
+    <CMakeProjectReference Include="$(MSBuildThisFileDirectory)/../native/CMakeLists.txt" />
+  </ItemGroup>
+</Project>