Skip to content
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

Remove preventing EH and entering EE at shutdown #100293

Merged
merged 4 commits into from
Mar 27, 2024
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
33 changes: 0 additions & 33 deletions src/coreclr/vm/ceemain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1329,12 +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.

// No longer process exceptions
g_fNoExceptions = true;

// <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 All @@ -1351,32 +1345,9 @@ void STDMETHODCALLTYPE EEShutDownHelper(BOOL fIsDllUnloading)
TerminateDebugger();
#endif // DEBUGGING_SUPPORTED

StubManager::TerminateStubManagers();

#ifdef FEATURE_INTERPRETER
Interpreter::Terminate();
#endif // FEATURE_INTERPRETER

//@TODO: find the right place for this
VirtualCallStubManager::UninitStatic();

// Unregister our vectored exception and continue handlers from the OS.
// This will ensure that if any other DLL unload (after ours) has an exception,
// we wont attempt to process that exception (which could lead to various
// issues including AV in the runtime).
//
// This should be done:
//
// 1) As the last action during the shutdown so that any unexpected AVs
// in the runtime during shutdown do result in FailFast in VEH.
//
// 2) Only when the runtime is processing DLL_PROCESS_DETACH.
CLRRemoveVectoredHandlers();

#if USE_DISASSEMBLER
Disassembler::StaticClose();
#endif // USE_DISASSEMBLER

WriteJitHelperCountToSTRESSLOG();

STRESS_LOG0(LF_STARTUP, LL_INFO10, "EEShutdown shutting down logging");
Expand Down Expand Up @@ -1700,10 +1671,6 @@ BOOL STDMETHODCALLTYPE EEDllMain( // TRUE on success, FALSE on error.
LOG((LF_STARTUP, INFO3, "EEShutDown invoked from EEDllMain"));
EEShutDown(TRUE); // shut down EE if it was started up
}
else
{
CLRRemoveVectoredHandlers();
}
break;
}
}
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/vm/crst.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ extern DWORD g_fEEShutDown;
#endif
// Total count of Crst lock of the type (Shutdown) that are currently in use
extern Volatile<LONG> g_ShutdownCrstUsageCount;
extern Volatile<LONG> g_fForbidEnterEE;

// The CRST.
class CrstBase
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
9 changes: 0 additions & 9 deletions src/coreclr/vm/eepolicy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ void SafeExitProcess(UINT exitCode, ShutdownCompleteAction sca = SCA_ExitProcess
// other DLLs call Release() on us in their detach [dangerous!], etc.
GCX_PREEMP_NO_DTOR();

InterlockedExchange((LONG*)&g_fForbidEnterEE, TRUE);

// Note that for free and retail builds StressLog must also be enabled
if (g_pConfig && g_pConfig->StressLog())
{
Expand All @@ -59,13 +57,6 @@ void SafeExitProcess(UINT exitCode, ShutdownCompleteAction sca = SCA_ExitProcess
}
}

// Turn off exception processing, because if some other random DLL has a
// fault in DLL_PROCESS_DETACH, we could get called for exception handling.
// Since we've turned off part of the runtime, we can't, for instance,
// properly execute the GC that handling an exception might trigger.
g_fNoExceptions = true;
LOG((LF_EH, LL_INFO10, "SafeExitProcess: turning off exceptions\n"));

if (sca == SCA_TerminateProcessWhenShutdownComplete)
{
// disabled because if we fault in this code path we will trigger our Watson code
Expand Down
64 changes: 0 additions & 64 deletions src/coreclr/vm/excep.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4585,13 +4585,6 @@ LONG InternalUnhandledExceptionFilter_Worker(
STRESS_LOG2(LF_EH, LL_INFO10, "In InternalUnhandledExceptionFilter_Worker, Exception = %x, sp = %p\n",
pExceptionInfo->ExceptionRecord->ExceptionCode, GetCurrentSP());

// If we can't enter the EE, done.
if (g_fForbidEnterEE)
{
LOG((LF_EH, LL_INFO100, "InternalUnhandledExceptionFilter_Worker: g_fForbidEnterEE is TRUE\n"));
return EXCEPTION_CONTINUE_SEARCH;
}

// We don't do anything when this is called from an unmanaged thread.
Thread *pThread = GetThreadNULLOk();

Expand Down Expand Up @@ -4623,18 +4616,6 @@ LONG InternalUnhandledExceptionFilter_Worker(
}
#endif

// This shouldn't be possible, but MSVC re-installs us... for now, just bail if this happens.
if (g_fNoExceptions)
{
return EXCEPTION_CONTINUE_SEARCH;
}

// Are we looking at a stack overflow here?
if ((pThread != NULL) && !pThread->DetermineIfGuardPagePresent())
{
g_fForbidEnterEE = true;
}

#ifdef DEBUGGING_SUPPORTED

// Mark that this exception has gone unhandled. At the moment only the debugger will
Expand Down Expand Up @@ -5533,8 +5514,6 @@ static LONG ThreadBaseExceptionFilter_Worker(PEXCEPTION_POINTERS pExceptionInfo,
ThreadBaseExceptionFilterParam *pParam = (ThreadBaseExceptionFilterParam *) pvParam;
UnhandledExceptionLocation location = pParam->location;

_ASSERTE(!g_fNoExceptions);

Thread* pThread = GetThread();

#ifdef _DEBUG
Expand Down Expand Up @@ -6737,14 +6716,6 @@ VEH_ACTION WINAPI CLRVectoredExceptionHandlerPhase3(PEXCEPTION_POINTERS pExcepti

VEH_ACTION WINAPI CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo)
{
// It is not safe to execute code inside VM after we shutdown EE. One example is DisablePreemptiveGC
// will block forever.
if (g_fForbidEnterEE)
{
return VEH_CONTINUE_SEARCH;
}


//
// DO NOT USE CONTRACTS HERE AS THIS ROUTINE MAY NEVER RETURN. You can use
// static contracts, but currently this is all WRAPPER_NO_CONTRACT.
Expand Down Expand Up @@ -7434,12 +7405,6 @@ LONG WINAPI CLRVectoredExceptionHandlerShim(PEXCEPTION_POINTERS pExceptionInfo)
// WARNING WARNING WARNING WARNING WARNING WARNING WARNING
//

// If exceptions (or runtime) have been disabled, then simply return.
if (g_fForbidEnterEE || g_fNoExceptions)
{
return EXCEPTION_CONTINUE_SEARCH;
}

#ifdef FEATURE_EH_FUNCLETS
pExceptionInfo->ContextRecord->ContextFlags |= CONTEXT_EXCEPTION_ACTIVE;
#endif // FEATURE_EH_FUNCLETS
Expand Down Expand Up @@ -7688,35 +7653,6 @@ void CLRAddVectoredHandlers(void)
#endif // !TARGET_UNIX
}

// This function removes the vectored exception and continue handler registration
// from the OS.
void CLRRemoveVectoredHandlers(void)
{
CONTRACTL
{
NOTHROW;
GC_NOTRIGGER;
MODE_ANY;
}
CONTRACTL_END;
#ifndef TARGET_UNIX

// Unregister the vectored exception handler if one is registered (and we can).
if (g_hVectoredExceptionHandler != NULL)
{
// Unregister the vectored exception handler
if (RemoveVectoredExceptionHandler(g_hVectoredExceptionHandler) == FALSE)
{
LOG((LF_EH, LL_INFO100, "CLRRemoveVectoredHandlers: RemoveVectoredExceptionHandler() failed.\n"));
}
else
{
LOG((LF_EH, LL_INFO100, "CLRRemoveVectoredHandlers: RemoveVectoredExceptionHandler() succeeded.\n"));
}
}
#endif // !TARGET_UNIX
}

//
// This does the work of the Unwind and Continue Hanlder inside the catch clause of that handler. The stack has not
// been unwound when this is called. Keep that in mind when deciding where to put new code :)
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/vm/excep.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ struct EE_ILEXCEPTION_CLAUSE;

void InitializeExceptionHandling();
void CLRAddVectoredHandlers(void);
void CLRRemoveVectoredHandlers(void);
void TerminateExceptionHandling();

// Prototypes
Expand Down
3 changes: 0 additions & 3 deletions src/coreclr/vm/i386/excepx86.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1571,9 +1571,6 @@ EXCEPTION_HANDLER_IMPL(COMPlusFrameHandler)

_ASSERTE((pContext == NULL) || ((pContext->ContextFlags & CONTEXT_CONTROL) == CONTEXT_CONTROL));

if (g_fNoExceptions)
return ExceptionContinueSearch; // No EH during EE shutdown.

// Check if the exception represents a GCStress Marker. If it does,
// we shouldnt record its entry in the TLS as such exceptions are
// continuable and can confuse the VM to treat them as CSE,
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 && !g_fForbidEnterEE)
RemoveMemoryPressure();
RemoveMemoryPressure();
}

#ifdef _DEBUG
Expand Down
Loading
Loading