From cb87aaebb321edd9ed74786305953314acfefda3 Mon Sep 17 00:00:00 2001 From: Steve MacLean Date: Wed, 6 Mar 2019 02:40:50 -0500 Subject: [PATCH] Cleanup old code (#23053) Remove IsContinuableException declaration & fix comment Clean up ThreadBaseRedirectingFilter Clean up comment Cleanup dead code --- src/inc/clrconfigvalues.h | 3 --- src/inc/switches.h | 7 ------ src/vm/eeconfig.cpp | 14 ----------- src/vm/eeconfig.h | 11 --------- src/vm/eepolicy.cpp | 2 +- src/vm/excep.cpp | 4 +-- src/vm/excep.h | 2 -- src/vm/threads.cpp | 63 ++++++++++++++++++----------------------------- 8 files changed, 27 insertions(+), 79 deletions(-) diff --git a/src/inc/clrconfigvalues.h b/src/inc/clrconfigvalues.h index ecf43f0..201a123 100644 --- a/src/inc/clrconfigvalues.h +++ b/src/inc/clrconfigvalues.h @@ -102,7 +102,6 @@ /// /// AppDomain /// -CONFIG_DWORD_INFO(INTERNAL_ADBreakOnCannotUnload, W("ADBreakOnCannotUnload"), 0, "Used to troubleshoot failures to unload appdomain (e.g. someone sitting in unmanged code). In some cases by the time we throw the appropriate exception the thread has moved from the offending call. This setting allows in an instrumented build to stop exactly at the right moment.") RETAIL_CONFIG_DWORD_INFO_DIRECT_ACCESS(UNSUPPORTED_AddRejitNops, W("AddRejitNops"), "Control for the profiler rejit feature infrastructure") CONFIG_DWORD_INFO(INTERNAL_ADDumpSB, W("ADDumpSB"), 0, "Not used") CONFIG_DWORD_INFO(INTERNAL_ADForceSB, W("ADForceSB"), 0, "Forces sync block creation for all objects") @@ -110,8 +109,6 @@ CONFIG_DWORD_INFO(INTERNAL_ADLogMemory, W("ADLogMemory"), 0, "Superseded by test CONFIG_DWORD_INFO(INTERNAL_ADTakeDHSnapShot, W("ADTakeDHSnapShot"), 0, "Superseded by test hooks") CONFIG_DWORD_INFO(INTERNAL_ADTakeSnapShot, W("ADTakeSnapShot"), 0, "Superseded by test hooks") CONFIG_DWORD_INFO_DIRECT_ACCESS(INTERNAL_EnableFullDebug, W("EnableFullDebug"), "Heavy-weight checking for AD boundary violations (AD leaks)") -RETAIL_CONFIG_DWORD_INFO(EXTERNAL_ADULazyMemoryRelease, W("ADULazyMemoryRelease"), 1, "On by default. Turned off in cases when people try to catch memory leaks, in which case AD unload should be immediately followed by GC)") -RETAIL_CONFIG_DWORD_INFO_DIRECT_ACCESS(EXTERNAL_ADURetryCount, W("ADURetryCount"), "Controls timeout of AD unload. Used for workarounds when machine is too slow, there are network issues etc.") // For the proposal and discussion on why finalizers are not run on shutdown by default anymore in CoreCLR, see the API review: // https://github.com/dotnet/corefx/issues/5205 diff --git a/src/inc/switches.h b/src/inc/switches.h index e47c78c..6003460 100644 --- a/src/inc/switches.h +++ b/src/inc/switches.h @@ -47,13 +47,6 @@ #if 0 #define APPDOMAIN_STATE - #define BREAK_ON_UNLOAD - #define AD_LOG_MEMORY - #define AD_NO_UNLOAD - #define AD_SNAPSHOT - #define BREAK_META_ACCESS - #define AD_BREAK_ON_CANNOT_UNLOAD - #define BREAK_ON_CLSLOAD // Enable to track details of EESuspension #define TIME_SUSPEND diff --git a/src/vm/eeconfig.cpp b/src/vm/eeconfig.cpp index a3a636d..a44f439 100644 --- a/src/vm/eeconfig.cpp +++ b/src/vm/eeconfig.cpp @@ -310,9 +310,6 @@ HRESULT EEConfig::Init() szZapBBInstr = NULL; szZapBBInstrDir = NULL; - fAppDomainUnload = true; - dwADURetryCount=1000; - #ifdef _DEBUG // interop logging m_pTraceIUnknown = NULL; @@ -1097,17 +1094,6 @@ HRESULT EEConfig::sync() fExpandAllOnLoad = (GetConfigDWORD_DontUse_(CLRConfig::INTERNAL_ExpandAllOnLoad, fExpandAllOnLoad) != 0); #endif //_DEBUG - -#ifdef AD_NO_UNLOAD - fAppDomainUnload = (CLRConfig::GetConfigValue(CLRConfig::INTERNAL_AppDomainNoUnload) == 0); -#endif - dwADURetryCount=GetConfigDWORD_DontUse_(CLRConfig::EXTERNAL_ADURetryCount, dwADURetryCount); - if (dwADURetryCount==(DWORD)-1) - { - _ASSERTE(!"Reserved value"); - dwADURetryCount=(DWORD)-2; - } - #ifdef ENABLE_STARTUP_DELAY { //I want this string in decimal diff --git a/src/vm/eeconfig.h b/src/vm/eeconfig.h index 20dde06..a63204f 100644 --- a/src/vm/eeconfig.h +++ b/src/vm/eeconfig.h @@ -505,13 +505,6 @@ public: return fProbeForStackOverflow; } - inline bool AppDomainUnload() const - {LIMITED_METHOD_CONTRACT; return fAppDomainUnload; } - - inline DWORD AppDomainUnloadRetryCount() const - {LIMITED_METHOD_CONTRACT; return dwADURetryCount; } - - #ifdef _DEBUG inline bool AppDomainLeaks() const { @@ -866,10 +859,6 @@ private: //---------------------------------------------------------------- unsigned int DoubleArrayToLargeObjectHeapThreshold; // double arrays of more than this number of elems go in large object heap #endif - bool fAppDomainUnload; // Enable appdomain unloading - - DWORD dwADURetryCount; - #ifdef _DEBUG bool fExpandAllOnLoad; // True if we want to load all types/jit all methods in an assembly // at load time. diff --git a/src/vm/eepolicy.cpp b/src/vm/eepolicy.cpp index 22d196b..ff61677 100644 --- a/src/vm/eepolicy.cpp +++ b/src/vm/eepolicy.cpp @@ -504,7 +504,7 @@ void SafeExitProcess(UINT exitCode, BOOL fAbort = FALSE, ShutdownCompleteAction if (sca == SCA_ExitProcessWhenShutdownComplete) { // disabled because if we fault in this code path we will trigger our - // Watson code via EntryPointFilter which is THROWS (see Dev11 317016) + // Watson code CONTRACT_VIOLATION(ThrowsViolation); #ifdef FEATURE_PAL diff --git a/src/vm/excep.cpp b/src/vm/excep.cpp index 2d5a160..f0b282b 100644 --- a/src/vm/excep.cpp +++ b/src/vm/excep.cpp @@ -5771,7 +5771,7 @@ static LONG ThreadBaseExceptionFilter_Worker(PEXCEPTION_POINTERS pExceptionInfo, { // No, don't swallow unhandled exceptions... - // ...except if the exception is of a type that is always swallowed (ThreadAbort, AppDomainUnload)... + // ...except if the exception is of a type that is always swallowed (ThreadAbort, ...) if (ExceptionIsAlwaysSwallowed(pExceptionInfo)) { // ...return EXCEPTION_EXECUTE_HANDLER to swallow the exception anyway. return EXCEPTION_EXECUTE_HANDLER; @@ -6562,7 +6562,7 @@ LONG FilterAccessViolation(PEXCEPTION_POINTERS pExceptionPointers, LPVOID lpvPar } /* - * IsContinuableException + * IsInterceptableException * * Returns whether this is an exception the EE knows how to intercept and continue from. * diff --git a/src/vm/excep.h b/src/vm/excep.h index 3568d24..cc4e97f 100644 --- a/src/vm/excep.h +++ b/src/vm/excep.h @@ -732,8 +732,6 @@ BOOL IsInFirstFrameOfHandler(Thread *pThread, //========================================================================== LONG FilterAccessViolation(PEXCEPTION_POINTERS pExceptionPointers, LPVOID lpvParam); -bool IsContinuableException(Thread *pThread); - bool IsInterceptableException(Thread *pThread); #ifdef DEBUGGING_SUPPORTED diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index 5370775..bffd81e 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -7802,8 +7802,6 @@ static LONG ThreadBaseRedirectingFilter(PEXCEPTION_POINTERS pExceptionInfo, LPVO STATIC_CONTRACT_GC_TRIGGERS; STATIC_CONTRACT_MODE_ANY; - LONG (*ptrFilter) (PEXCEPTION_POINTERS, PVOID); - TryParam * pRealParam = reinterpret_cast(pParam); ManagedThreadCallState * _pCallState = pRealParam->m_pCallState; @@ -7811,12 +7809,10 @@ static LONG ThreadBaseRedirectingFilter(PEXCEPTION_POINTERS pExceptionInfo, LPVO // This will invoke the swallowing filter. If that returns EXCEPTION_CONTINUE_SEARCH, // it will trigger unhandled exception processing. - ptrFilter = ThreadBaseExceptionAppDomainFilter; - - // WARNING - ptrFilter may not return + // WARNING - ThreadBaseExceptionAppDomainFilter may not return // This occurs when the debugger decides to intercept an exception and catch it in a frame closer // to the leaf than the one executing this filter - ret = (*ptrFilter) (pExceptionInfo, _pCallState); + ret = ThreadBaseExceptionAppDomainFilter(pExceptionInfo, _pCallState); // Although EXCEPTION_EXECUTE_HANDLER can also be returned in cases corresponding to // unhandled exceptions, all of those cases have already notified the debugger of an unhandled @@ -7834,43 +7830,32 @@ static LONG ThreadBaseRedirectingFilter(PEXCEPTION_POINTERS pExceptionInfo, LPVO Thread *pCurThread = GetThread(); _ASSERTE(pCurThread); + // + // In the default domain, when an exception goes unhandled on a managed thread whose threadbase is in the VM (e.g. explicitly spawned threads, + // ThreadPool threads, finalizer thread, etc), CLR can end up in the unhandled exception processing path twice. + // + // The first attempt to perform UE processing happens at the managed thread base (via this function). When it completes, + // we will set TSNC_ProcessedUnhandledException state against the thread to indicate that we have perform the unhandled exception processing. + // + // On CoreSys CoreCLR, the host can ask CoreCLR to run all code in the default domain. As a result, when we return from the first attempt to perform UE + // processing, the call could return back with EXCEPTION_EXECUTE_HANDLER since, like desktop CoreCLR is instructed by SL host to swallow all unhandled exceptions, + // CoreSys CoreCLR can also be instructed by its Phone host to swallow all unhandled exceptions. As a result, the exception dispatch will never continue to go upstack + // to the native threadbase in the OS kernel and thus, there will never be a second attempt to perform UE processing. Hence, we dont, and shouldnt, need to set + // TSNC_ProcessedUnhandledException state against the thread if we are in SingleAppDomain mode and have been asked to swallow the exception. + // + // If we continue to set TSNC_ProcessedUnhandledException and a ThreadPool Thread A has an exception go unhandled, we will swallow it correctly for the first time. + // The next time Thread A has an exception go unhandled, our UEF will see TSNC_ProcessedUnhandledException set and assume (incorrectly) UE processing has happened and + // will fail to honor the host policy (e.g. swallow unhandled exception). Thus, the 2nd unhandled exception may end up crashing the app when it should not. + // + if (ret != EXCEPTION_EXECUTE_HANDLER) { LOG((LF_EH, LL_INFO100, "ThreadBaseRedirectingFilter: setting TSNC_ProcessedUnhandledException\n")); + // Since we have already done unhandled exception processing for it, we dont want it + // to happen again if our UEF gets invoked upon returning back to the OS. // - // In the default domain, when an exception goes unhandled on a managed thread whose threadbase is in the VM (e.g. explicitly spawned threads, - // ThreadPool threads, finalizer thread, etc), CLR can end up in the unhandled exception processing path twice. - // - // The first attempt to perform UE processing happens at the managed thread base (via this function). When it completes, - // we will set TSNC_ProcessedUnhandledException state against the thread to indicate that we have perform the unhandled exception processing. - // - // On the desktop CLR, after the first attempt, we will return back to the OS with EXCEPTION_CONTINUE_SEARCH as unhandled exceptions cannot be swallowed. When the exception reaches - // the native threadbase in the OS kernel, the OS will invoke the UEF registered for the process. This can result in CLR's UEF (COMUnhandledExceptionFilter) - // getting invoked that will attempt to perform UE processing yet again for the same thread. To avoid this duplicate processing, we check the presence of - // TSNC_ProcessedUnhandledException state on the thread and if present, we simply return back to the OS. - // - // On desktop CoreCLR, we will only do UE processing once (at the managed threadbase) since no thread is created in default domain - all are created and executed in non-default domain. - // As a result, we go via completely different codepath that prevents duplication of UE processing from happening, especially since desktop CoreCLR is targetted for SL and SL - // always passes us a flag to swallow unhandled exceptions. - // - // On CoreSys CoreCLR, the host can ask CoreCLR to run all code in the default domain. As a result, when we return from the first attempt to perform UE - // processing, the call could return back with EXCEPTION_EXECUTE_HANDLER since, like desktop CoreCLR is instructed by SL host to swallow all unhandled exceptions, - // CoreSys CoreCLR can also be instructed by its Phone host to swallow all unhandled exceptions. As a result, the exception dispatch will never continue to go upstack - // to the native threadbase in the OS kernel and thus, there will never be a second attempt to perform UE processing. Hence, we dont, and shouldnt, need to set - // TSNC_ProcessedUnhandledException state against the thread if we are in SingleAppDomain mode and have been asked to swallow the exception. - // - // If we continue to set TSNC_ProcessedUnhandledException and a ThreadPool Thread A has an exception go unhandled, we will swallow it correctly for the first time. - // The next time Thread A has an exception go unhandled, our UEF will see TSNC_ProcessedUnhandledException set and assume (incorrectly) UE processing has happened and - // will fail to honor the host policy (e.g. swallow unhandled exception). Thus, the 2nd unhandled exception may end up crashing the app when it should not. - // - if (ret != EXCEPTION_EXECUTE_HANDLER) - { - // Since we have already done unhandled exception processing for it, we dont want it - // to happen again if our UEF gets invoked upon returning back to the OS. - // - // Set the flag to indicate so. - pCurThread->SetThreadStateNC(Thread::TSNC_ProcessedUnhandledException); - } + // Set the flag to indicate so. + pCurThread->SetThreadStateNC(Thread::TSNC_ProcessedUnhandledException); } return ret; -- 2.7.4