Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Remove TSNC_UnsafeSkipEnterCooperative
  • Loading branch information
janvorli committed Mar 26, 2024
commit 6c0033c70025a0a85a0ee920e849a018029b2210
3 changes: 0 additions & 3 deletions src/coreclr/vm/ceemain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1329,9 +1329,6 @@ void STDMETHODCALLTYPE EEShutDownHelper(BOOL fIsDllUnloading)
{
g_fEEShutDown |= ShutDown_Phase2;

// Shutdown finalizer before we suspend all background threads. Otherwise we
// never get to finalize anything.

// <TODO>@TODO: This does things which shouldn't occur in part 2. Namely,
// calling managed dll main callbacks (AppDomain::SignalProcessDetach), and
// RemoveAppDomainFromIPC.
Copy link
Member

@jkotas jkotas Mar 26, 2024

Choose a reason for hiding this comment

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

I am looking at what we do below and there seems to be more instances of questionable cleanup:

  • CLRRemoveVectoredHandlers
  • StubManager::TerminateStubManagers - it should just flush the debug-only log, but it should not be destroying the Crsts
  • Interpreter::Terminate

Copy link
Member Author

Choose a reason for hiding this comment

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

The Disassembler::StaticClose(); frees the disassembler shared library, it also sounds like something we should not do.

Copy link
Member Author

Choose a reason for hiding this comment

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

StubManager::TerminateStubManagers - it actually doesn't do anything interesting. The whole log is just a SString instance that is never accessed in any way - there is a StubManager::DbgGetLog method, but nothing calls it.

Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/eehash.inl
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ BOOL EEHashTableBase<KeyType, Helper, bDefaultCopyIsDeep>::DeleteValue(KeyType p
_ASSERTE (OwnLock());

Thread *pThread = GetThreadNULLOk();
GCX_MAYBE_COOP_NO_THREAD_BROKEN(pThread ? !(pThread->m_StateNC & Thread::TSNC_UnsafeSkipEnterCooperative) : FALSE);
GCX_MAYBE_COOP_NO_THREAD_BROKEN(pThread != NULL);

_ASSERTE(m_pVolatileBucketTable->m_dwNumBuckets != 0);

Expand Down Expand Up @@ -850,7 +850,7 @@ BOOL EEHashTableBase<KeyType, Helper, bDefaultCopyIsDeep>::
_ASSERTE_IMPL(OwnLock());

Thread *pThread = GetThreadNULLOk();
GCX_MAYBE_COOP_NO_THREAD_BROKEN(pThread ? !(pThread->m_StateNC & Thread::TSNC_UnsafeSkipEnterCooperative) : FALSE);
GCX_MAYBE_COOP_NO_THREAD_BROKEN(pThread != NULL);

_ASSERTE(pIter->m_pTable == (void *) this);

Expand Down
24 changes: 1 addition & 23 deletions src/coreclr/vm/runtimecallablewrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1227,16 +1227,6 @@ HRESULT RCWCleanupList::ReleaseRCWListInCorrectCtx(LPVOID pData)

LPVOID pCurrCtxCookie = GetCurrentCtxCookie();

// If we are releasing our IP's as a result of shutdown, we MUST not transition
// into cooperative GC mode. This "fix" will prevent us from doing so.
if (g_fEEShutDown & ShutDown_Finalize2)
{
Thread *pThread = GetThreadNULLOk();
if (pThread && !FinalizerThread::IsCurrentThreadFinalizer())
pThread->SetThreadStateNC(Thread::TSNC_UnsafeSkipEnterCooperative);
}


// Make sure we're in the right context / apartment.
// Also - if we've already transitioned once, we don't want to do so again.
// If the cookie exists in multiple MTA apartments, and the STA has gone away
Expand Down Expand Up @@ -1268,14 +1258,6 @@ HRESULT RCWCleanupList::ReleaseRCWListInCorrectCtx(LPVOID pData)
}
}

// Reset the bit indicating we cannot transition into cooperative GC mode.
if (g_fEEShutDown & ShutDown_Finalize2)
{
Thread *pThread = GetThreadNULLOk();
if (pThread && !FinalizerThread::IsCurrentThreadFinalizer())
pThread->ResetThreadStateNC(Thread::TSNC_UnsafeSkipEnterCooperative);
}

return S_OK;
}

Expand Down Expand Up @@ -1559,7 +1541,6 @@ void RCW::RemoveMemoryPressure()
NOTHROW;
GC_TRIGGERS;
MODE_PREEMPTIVE;
PRECONDITION((GetThread()->m_StateNC & Thread::TSNC_UnsafeSkipEnterCooperative) == 0);
}
CONTRACTL_END;

Expand Down Expand Up @@ -1771,7 +1752,6 @@ void RCW::Cleanup()
// if the wrapper is still in the cache. Also, if we can't switch to coop mode,
// we're guaranteed to have already decoupled the RCW from its object.
#ifdef _DEBUG
if (!(GetThread()->m_StateNC & Thread::TSNC_UnsafeSkipEnterCooperative))
{
GCX_COOP();

Expand All @@ -1789,9 +1769,7 @@ void RCW::Cleanup()
ReleaseAllInterfacesCallBack(this);

// Remove the memory pressure caused by this RCW (if present)
// If we're in a shutdown situation, we can ignore the memory pressure.
if ((GetThread()->m_StateNC & Thread::TSNC_UnsafeSkipEnterCooperative) == 0)
RemoveMemoryPressure();
RemoveMemoryPressure();
}

#ifdef _DEBUG
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/threads.h
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ class Thread
// unused = 0x00000040,
TSNC_CLRCreatedThread = 0x00000080, // The thread was created through Thread::CreateNewThread
TSNC_ExistInThreadStore = 0x00000100, // For dtor to know if it needs to be removed from ThreadStore
TSNC_UnsafeSkipEnterCooperative = 0x00000200, // This is a "fix" for deadlocks caused when cleaning up COM
// unused = 0x00000200,
TSNC_OwnsSpinLock = 0x00000400, // The thread owns a spinlock.
TSNC_PreparingAbort = 0x00000800, // Preparing abort. This avoids recursive HandleThreadAbort call.
TSNC_OSAlertableWait = 0x00001000, // Preparing abort. This avoids recursive HandleThreadAbort call.
Expand Down
3 changes: 0 additions & 3 deletions src/coreclr/vm/threadsuspend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2099,9 +2099,6 @@ void Thread::RareDisablePreemptiveGC()
goto Exit;
}

// This should NEVER be called if the TSNC_UnsafeSkipEnterCooperative bit is set!
_ASSERTE(!(m_StateNC & TSNC_UnsafeSkipEnterCooperative) && "DisablePreemptiveGC called while the TSNC_UnsafeSkipEnterCooperative bit is set");

// Holding a spin lock in preemp mode and switch to coop mode could cause other threads spinning
// waiting for GC
_ASSERTE ((m_StateNC & Thread::TSNC_OwnsSpinLock) == 0);
Expand Down