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 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
3 changes: 0 additions & 3 deletions src/coreclr/vm/ceemain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1332,9 +1332,6 @@ void STDMETHODCALLTYPE EEShutDownHelper(BOOL fIsDllUnloading)
// Shutdown finalizer before we suspend all background threads. Otherwise we
// never get to finalize anything.

jkotas marked this conversation as resolved.
Show resolved Hide resolved
// 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 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
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
35 changes: 0 additions & 35 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
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
2 changes: 1 addition & 1 deletion src/coreclr/vm/runtimecallablewrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1790,7 +1790,7 @@ void RCW::Cleanup()

// 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)
if ((GetThread()->m_StateNC & Thread::TSNC_UnsafeSkipEnterCooperative) == 0)
jkotas marked this conversation as resolved.
Show resolved Hide resolved
RemoveMemoryPressure();
}

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/stdinterfaces_wrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ namespace
LIMITED_METHOD_CONTRACT;
// If we are finalizing all alive objects, or after this stage, we do not allow
// a thread to enter EE.
return !((g_fEEShutDown & ShutDown_Finalize2) || g_fForbidEnterEE);
return !(g_fEEShutDown & ShutDown_Finalize2);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/threads.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7767,7 +7767,7 @@ OBJECTREF Thread::GetCulture(BOOL bUICulture)

// This is the case when we're building CoreLib and haven't yet created
// the system assembly.
if (SystemDomain::System()->SystemAssembly()==NULL || g_fForbidEnterEE) {
if (SystemDomain::System()->SystemAssembly()==NULL) {
return NULL;
}

Expand Down
2 changes: 0 additions & 2 deletions src/coreclr/vm/vars.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,7 @@ GVAL_IMPL(SIZE_T, g_runtimeVirtualSize);

#ifndef DACCESS_COMPILE

Volatile<LONG> g_fForbidEnterEE = false;
bool g_fManagedAttach = false;
bool g_fNoExceptions = false;

DWORD g_FinalizerWaiterStatus = 0;

Expand Down
2 changes: 0 additions & 2 deletions src/coreclr/vm/vars.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -480,13 +480,11 @@ EXTERN BOOL g_fComStarted;
GVAL_DECL(DWORD, g_fEEShutDown);
EXTERN DWORD g_fFastExitProcess;
EXTERN BOOL g_fFatalErrorOccurredOnGCThread;
EXTERN Volatile<LONG> g_fForbidEnterEE;
GVAL_DECL(bool, g_fProcessDetach);
#ifdef FEATURE_METADATA_UPDATER
GVAL_DECL(bool, g_metadataUpdatesApplied);
#endif
EXTERN bool g_fManagedAttach;
EXTERN bool g_fNoExceptions;

// Indicates whether we're executing shut down as a result of DllMain
// (DLL_PROCESS_DETACH). See comments at code:EEShutDown for details.
Expand Down
Loading