Skip to content

Move FLS init before EventPipeAdapter::FinishInitialize and the first SetupThread() #115546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 50 additions & 31 deletions src/coreclr/vm/ceemain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -839,36 +839,19 @@ void EEStartupHelper()

#ifdef PROFILING_SUPPORTED
// Initialize the profiling services.
// This must happen before Thread::HasStarted() that fires profiler notifications is called on the finalizer thread.
hr = ProfilingAPIUtility::InitializeProfiling();

_ASSERTE(SUCCEEDED(hr));
IfFailGo(hr);
#endif // PROFILING_SUPPORTED

InitializeExceptionHandling();

//
// Install our global exception filter
//
if (!InstallUnhandledExceptionFilter())
{
IfFailGo(E_FAIL);
}

// throws on error
SetupThread();

#ifdef DEBUGGING_SUPPORTED
// Notify debugger once the first thread is created to finish initialization.
if (g_pDebugInterface != NULL)
{
g_pDebugInterface->StartupPhase2(GetThread());
Copy link
Member

@jkotas jkotas May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HasStarted on the finalizer thread will send ThreadCreated notifications to the debugger. On non-Windows, are there going to be issues if this debugger notification on the finalizer thread is sent to debugger before this StartupPhase2 finishes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetupThread also calls ThreadCreated and the EE initializing thread calls ThreadCreated before StartupPhase2 is called.

Both SetupThread and HasStarted will check for if (CORDebuggerAttached()) before sending the notification though.

The functional part of ThreadCreated (apart from notifications) is to check if debugger wants threads to stop, which cannot happen if debugger is not yet officially attached.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there could be a race where finalizer thread checks if debugger is attached (sees that it is not) and then debugger asks threads to stop.

There is no such race for the thread that initializes the EE.

}
#endif

// This isn't done as part of InitializeGarbageCollector() above because
// debugger must be initialized before creating EE thread objects
#ifdef TARGET_WINDOWS
// Create the finalizer thread on windows earlier, as we will need to wait for
// the completion of its initialization part that initializes COM as that has to be done
// before the first Thread is attached. Thus we want to give the thread a bit more time.
FinalizerThread::FinalizerThreadCreate();
#endif

InitPreStubManager();

Expand All @@ -882,8 +865,6 @@ void EEStartupHelper()
// of the JIT helpers.
InitJITHelpers1();

SyncBlockCache::Attach();

// Set up the sync block
SyncBlockCache::Start();

Expand All @@ -898,6 +879,48 @@ void EEStartupHelper()

IfFailGo(hr);

InitializeExceptionHandling();

//
// Install our global exception filter
//
if (!InstallUnhandledExceptionFilter())
{
IfFailGo(E_FAIL);
}

#ifdef TARGET_WINDOWS
// g_pGCHeap->Initialize() above could take nontrivial time, so by now the finalizer thread
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we end up creating the finalizer thread too soon on non-Windows?

For example, can the finalizer Thread be registered in thread store before ProfilingAPIUtility::InitializeProfiling and miss profiler notifications that are done as part of the registration?

Copy link
Member Author

@VSadov VSadov May 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If InitializeProfiling does not need Thread object for the current thread, it can be moved earlier.

The comments inside InitializeProfiling say that there are asserts somewhere that finalizer thread has started. So I wonder - are there tests for profiler attach? (or maybe I misunderstood the comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments inside InitializeProfiling say that there are asserts somewhere that finalizer thread has started.

These comments refer to the old profiler attach implementation. I am deleting them in #115680

So I wonder - are there tests for profiler attach?

Yes, look for "Attach" under https://github.com/dotnet/runtime/tree/main/src/tests/profiler. (The tests won't hit every subtle race condition though.)

Copy link
Member Author

@VSadov VSadov May 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the profiler initializtion earlier. If it needs to attach a Thread, then it would hit an assert in OsAttachThread. There is an assert there now to catch these cases deterministically.

I was mostly wondering if we test scenarios beyond "profiler is not involved at all".

Copy link
Member Author

@VSadov VSadov May 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With default test set there are no failures, so based on that we could say profiler initializtion does not create/attach Thread objects - directly or indirectly, then it is all good.

If the profiler tests you've pointed to run in default test set, it may be good enough. If we need to run some other leg - like outer loop, perhaps makes sense to do that. Maybe makes sense to do that regardless.

// should have initialized FLS slot for thread cleanup notifications.
// And ensured that COM is initialized (must happen before allocating FLS slot).
// Make sure that this was done before we start creating Thread objects
// Ex: The call to SetupThread below will create and attach a Thread object.
// Event pipe might also do that.
FinalizerThread::WaitForFinalizerThreadStart();
#endif

// throws on error
_ASSERTE(GetThreadNULLOk() == NULL);
SetupThread();

#ifdef DEBUGGING_SUPPORTED
// Notify debugger once the first thread is created to finish initialization.
if (g_pDebugInterface != NULL)
{
g_pDebugInterface->StartupPhase2(GetThread());
}
#endif

#ifndef TARGET_WINDOWS
// This isn't done as part of InitializeGarbageCollector() above because
// debugger must be initialized before creating EE thread objects
FinalizerThread::FinalizerThreadCreate();
#else
// On windows the finalizer thread is already partially created and is waiting
// right before doing HasStarted(). We will release it now.
FinalizerThread::EnableFinalization();
#endif

#ifdef FEATURE_PERFTRACING
// Finish setting up rest of EventPipe - specifically enable SampleProfiler if it was requested at startup.
// SampleProfiler needs to cooperate with the GC which hasn't fully finished setting up in the first part of the
Expand Down Expand Up @@ -957,12 +980,6 @@ void EEStartupHelper()
g_MiniMetaDataBuffMaxSize, MEM_COMMIT, PAGE_READWRITE);
#endif // FEATURE_MINIMETADATA_IN_TRIAGEDUMPS

#ifdef TARGET_WINDOWS
// By now finalizer thread should have initialized FLS slot for thread cleanup notifications.
// And ensured that COM is initialized (must happen before allocating FLS slot).
// Make sure that this was done.
FinalizerThread::WaitForFinalizerThreadStart();
#endif
g_fEEStarted = TRUE;
g_EEStartupStatus = S_OK;
hr = S_OK;
Expand Down Expand Up @@ -1764,6 +1781,8 @@ void InitFlsSlot()
// thread - thread to attach
static void OsAttachThread(void* thread)
{
_ASSERTE(g_flsIndex != FLS_OUT_OF_INDEXES);

if (t_flsState == FLS_STATE_INVOKED)
{
_ASSERTE_ALL_BUILDS(!"Attempt to execute managed code after the .NET runtime thread state has been destroyed.");
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/vm/finalizerthread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,7 @@ DWORD WINAPI FinalizerThread::FinalizerThreadStart(void *args)

// handshake with EE initialization, as now we can attach Thread objects to native threads.
hEventFinalizerDone->Set();
WaitForFinalizerEvent (hEventFinalizer);
#endif

s_FinalizerThreadOK = GetFinalizerThread()->HasStarted();
Expand Down
7 changes: 0 additions & 7 deletions src/coreclr/vm/syncblk.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -591,13 +591,6 @@ void SyncBlockCache::CleanupSyncBlocks()
} EE_END_FINALLY;
}

// create the sync block cache
/* static */
void SyncBlockCache::Attach()
{
LIMITED_METHOD_CONTRACT;
}

// create the sync block cache
/* static */
void SyncBlockCache::Start()
Expand Down
Loading