From 3999c8b67b9be11124d12cd66f25ddb993cdf95e Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Wed, 16 Oct 2019 13:47:18 -0700 Subject: [PATCH 1/3] Protect against a rare invalid lock acquision attempt during etw processing during abrupt shutdown Targeted and partial fix for https://github.com/dotnet/coreclr/issues/27129 - This is not a generic fix for the issue above, it is only a very targeted fix for an issue seen (a new issue introduced in 3.x). For a generic fix and more details, see the fix in 5.0: https://github.com/dotnet/coreclr/pull/27238. - This change avoids taking a lock during process detach - a point in time when all other threads have already been abruptly shut down by the OS and locks may have been orphaned. - The issue leads to a hang during shutdown when ETW tracing is enabled and the .NET process being traced begins the shutdown sequence at an unfortunate time - this is a probably rare timing issue. It would take the shutdown sequence to begin at just the point when a thread holds a particular lock and is terminated by the OS while holding the lock, then the OS sends the process detach event to the CLR, work during which then tries to acquire the lock and cannot because it is orphaned. - The generic fix has broader consequences and is unlikely to be a reasonable change to make so late in the cycle, such a change needs some bake time and feedback. Hence this targeted fix for 3.x. --- src/vm/ceemain.cpp | 9 ++++----- src/vm/prestub.cpp | 8 +++++++- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/vm/ceemain.cpp b/src/vm/ceemain.cpp index 1c5d4f885246..8a5bb4f4e697 100644 --- a/src/vm/ceemain.cpp +++ b/src/vm/ceemain.cpp @@ -1323,6 +1323,10 @@ void STDMETHODCALLTYPE EEShutDownHelper(BOOL fIsDllUnloading) if (fIsDllUnloading) { + // The process is detaching, so set the global state. + // This is used to get around FreeLibrary problems. + g_fProcessDetach = true; + ETW::EnumerationLog::ProcessShutdown(); } @@ -1339,11 +1343,6 @@ void STDMETHODCALLTYPE EEShutDownHelper(BOOL fIsDllUnloading) Thread * pThisThread = GetThread(); #endif - // If the process is detaching then set the global state. - // This is used to get around FreeLibrary problems. - if(fIsDllUnloading) - g_fProcessDetach = true; - if (IsDbgHelperSpecialThread()) { // Our debugger helper thread does not allow Thread object to be set up. diff --git a/src/vm/prestub.cpp b/src/vm/prestub.cpp index b76a018daf21..7ea3519e31e5 100644 --- a/src/vm/prestub.cpp +++ b/src/vm/prestub.cpp @@ -1197,7 +1197,13 @@ PrepareCodeConfig::JitOptimizationTier PrepareCodeConfig::GetJitOptimizationTier _ASSERTE(methodDesc != nullptr); _ASSERTE(config == nullptr || methodDesc == config->GetMethodDesc()); - if (config != nullptr) + // The code inside this block may take one or more locks. If process detach has already occurred, then other threads would + // have already been abruptly terminated, which may have left locks orphaned, so it would not be feasible to attempt taking + // a lock on process detach. This function is called from ETW-related code on shutdown where unfortunately it processes and + // sends rundown events upon process detach. If process detach has already occurred, then resort to best-effort behavior + // that may return inaccurate information. This is a temporary and targeted fix for a clear problem, and should not be used + // long-term. + if (config != nullptr && !g_fProcessDetach) { if (config->JitSwitchedToMinOpt()) { From ef16c6df9e77e35d8706b46b4af8cc18d4aa4bae Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 18 Oct 2019 07:30:43 -0700 Subject: [PATCH 2/3] Report tier as unknown when it cannot be determined --- src/vm/prestub.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vm/prestub.cpp b/src/vm/prestub.cpp index 7ea3519e31e5..dea9832ece90 100644 --- a/src/vm/prestub.cpp +++ b/src/vm/prestub.cpp @@ -1236,7 +1236,7 @@ PrepareCodeConfig::JitOptimizationTier PrepareCodeConfig::GetJitOptimizationTier #endif } - return methodDesc->IsJitOptimizationDisabled() ? JitOptimizationTier::MinOptJitted : JitOptimizationTier::Optimized; + return methodDesc->IsJitOptimizationDisabled() ? JitOptimizationTier::MinOptJitted : JitOptimizationTier::Unknown; } const char *PrepareCodeConfig::GetJitOptimizationTierStr(PrepareCodeConfig *config, MethodDesc *methodDesc) From 24730c5963747d5a220ed376b27ad41d46c8e118 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 18 Oct 2019 21:37:15 -0700 Subject: [PATCH 3/3] Return unknown only on process detach --- src/vm/prestub.cpp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/vm/prestub.cpp b/src/vm/prestub.cpp index dea9832ece90..a8d3e4247ad1 100644 --- a/src/vm/prestub.cpp +++ b/src/vm/prestub.cpp @@ -1197,13 +1197,18 @@ PrepareCodeConfig::JitOptimizationTier PrepareCodeConfig::GetJitOptimizationTier _ASSERTE(methodDesc != nullptr); _ASSERTE(config == nullptr || methodDesc == config->GetMethodDesc()); - // The code inside this block may take one or more locks. If process detach has already occurred, then other threads would - // have already been abruptly terminated, which may have left locks orphaned, so it would not be feasible to attempt taking - // a lock on process detach. This function is called from ETW-related code on shutdown where unfortunately it processes and - // sends rundown events upon process detach. If process detach has already occurred, then resort to best-effort behavior - // that may return inaccurate information. This is a temporary and targeted fix for a clear problem, and should not be used + // The code below may take one or more locks. If process detach has already occurred, then other threads would have already + // been abruptly terminated, which may have left locks orphaned, so it would not be feasible to attempt taking a lock on + // process detach. This function is called from ETW-related code on shutdown where unfortunately it processes and sends + // rundown events upon process detach. If process detach has already occurred, then resort to best-effort behavior that may + // return inaccurate information. This is a temporary and targeted fix for a clear problem, and should not be used // long-term. - if (config != nullptr && !g_fProcessDetach) + if (g_fProcessDetach) + { + return JitOptimizationTier::Unknown; + } + + if (config != nullptr) { if (config->JitSwitchedToMinOpt()) { @@ -1236,7 +1241,7 @@ PrepareCodeConfig::JitOptimizationTier PrepareCodeConfig::GetJitOptimizationTier #endif } - return methodDesc->IsJitOptimizationDisabled() ? JitOptimizationTier::MinOptJitted : JitOptimizationTier::Unknown; + return methodDesc->IsJitOptimizationDisabled() ? JitOptimizationTier::MinOptJitted : JitOptimizationTier::Optimized; } const char *PrepareCodeConfig::GetJitOptimizationTierStr(PrepareCodeConfig *config, MethodDesc *methodDesc)