From 7eb5ef2c946d04300e3bc3ddc7470950bac01162 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 19 Dec 2024 10:46:56 -0500 Subject: [PATCH] [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 --- src/coreclr/vm/profilingenumerators.cpp | 4 +- src/coreclr/vm/proftoeeinterfaceimpl.cpp | 8 +- src/tests/profiler/native/CMakeLists.txt | 3 +- src/tests/profiler/native/classfactory.cpp | 5 + .../enumthreadsprofiler.cpp | 104 ++++++++++++++++++ .../enumthreadsprofiler/enumthreadsprofiler.h | 34 ++++++ src/tests/profiler/unittest/enumthreads.cs | 56 ++++++++++ .../profiler/unittest/enumthreads.csproj | 21 ++++ 8 files changed, 229 insertions(+), 6 deletions(-) create mode 100644 src/tests/profiler/native/enumthreadsprofiler/enumthreadsprofiler.cpp create mode 100644 src/tests/profiler/native/enumthreadsprofiler/enumthreadsprofiler.h create mode 100644 src/tests/profiler/unittest/enumthreads.cs create mode 100644 src/tests/profiler/unittest/enumthreads.csproj diff --git a/src/coreclr/vm/profilingenumerators.cpp b/src/coreclr/vm/profilingenumerators.cpp index c55d40ffbeed9..ac924f5032c7e 100644 --- a/src/coreclr/vm/profilingenumerators.cpp +++ b/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; diff --git a/src/coreclr/vm/proftoeeinterfaceimpl.cpp b/src/coreclr/vm/proftoeeinterfaceimpl.cpp index e99273beb5d13..77fa9989cfac2 100644 --- a/src/coreclr/vm/proftoeeinterfaceimpl.cpp +++ b/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; diff --git a/src/tests/profiler/native/CMakeLists.txt b/src/tests/profiler/native/CMakeLists.txt index 1279874df07c3..4333a8b269828 100644 --- a/src/tests/profiler/native/CMakeLists.txt +++ b/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 diff --git a/src/tests/profiler/native/classfactory.cpp b/src/tests/profiler/native/classfactory.cpp index bb27152f8581f..677b57fef95ef 100644 --- a/src/tests/profiler/native/classfactory.cpp +++ b/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"); diff --git a/src/tests/profiler/native/enumthreadsprofiler/enumthreadsprofiler.cpp b/src/tests/profiler/native/enumthreadsprofiler/enumthreadsprofiler.cpp new file mode 100644 index 0000000000000..de6caeaef7100 --- /dev/null +++ b/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); +} diff --git a/src/tests/profiler/native/enumthreadsprofiler/enumthreadsprofiler.h b/src/tests/profiler/native/enumthreadsprofiler/enumthreadsprofiler.h new file mode 100644 index 0000000000000..df71610843259 --- /dev/null +++ b/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 _gcStarts; + std::atomic _gcFinishes; + std::atomic _profilerEnumThreadsCompleted; + std::atomic _failures; +}; diff --git a/src/tests/profiler/unittest/enumthreads.cs b/src/tests/profiler/unittest/enumthreads.cs new file mode 100644 index 0000000000000..95562decc3f55 --- /dev/null +++ b/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; + } + } +} diff --git a/src/tests/profiler/unittest/enumthreads.csproj b/src/tests/profiler/unittest/enumthreads.csproj new file mode 100644 index 0000000000000..d51dcb692abfe --- /dev/null +++ b/src/tests/profiler/unittest/enumthreads.csproj @@ -0,0 +1,21 @@ + + + .NETCoreApp + exe + true + true + + true + + true + + + + + + + +