[release/9.0-staging] [Profiler] Avoid Recursive ThreadStoreLock in Profiling Thread Enumerator (#110665)

* [Profiler] Avoid Recursive ThreadStoreLock

Profiling Enumerators look to acquire the ThreadStoreLock.
In release config, re-acquiring the ThreadStoreLock and
releasing it in ProfilerThreadEnum::Init will cause
problems if the callback invoking EnumThread has logic
that depends on the ThreadStoreLock being held.
To avoid recursively acquiring the ThreadStoreLock,
expand the condition when the profiling thread enumerator
shouldn't acquire the ThreadStoreLock.

* [Profiler] Change order to set fProfilerRequestedRuntimeSuspend

There was a potential race condition when setting the flag
before suspending and resetting the flag after restarting.
For example, if the thread restarting runtime is preempted
right after resuming runtime, the flag could remain unset
by the time another thread looks to suspend runtime, which
would see that the flag as set.

* [Profiler][Tests] Add unit test for EnumThreads during suspension

* [Profiler][Tests] Fixup EnumThreads test

---------

Co-authored-by: mdh1418 <[email protected]>

Author: github-actions[bot]

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>