From 8570bc96372649a3823ae7c7b6d2ac5eb38794df Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Sun, 15 Mar 2020 20:46:49 -0700 Subject: [PATCH 1/5] GC coverage can now occur in scenarios where a thread doesn't exist. Properly handle this case and assert some invariant. --- src/coreclr/src/vm/gccover.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/coreclr/src/vm/gccover.cpp b/src/coreclr/src/vm/gccover.cpp index c5801e259fec76..2490ee89c3597a 100644 --- a/src/coreclr/src/vm/gccover.cpp +++ b/src/coreclr/src/vm/gccover.cpp @@ -1400,7 +1400,15 @@ BOOL OnGcCoverageInterrupt(PCONTEXT regs) } Thread* pThread = GetThread(); - _ASSERTE(pThread); + if (!pThread) + { + // No thread at the moment so we aren't doing coverage for this function. + // This should only occur for methods with the NativeCallableAttribute, + // where the caller could be coming from a thread unknown to the CLR and + // we haven't created a thread yet - see PreStubWorker_Premptive(). + _ASSERTE(pMD->HasNativeCallableAttribute()); + return (FALSE); + } #if defined(USE_REDIRECT_FOR_GCSTRESS) && !defined(TARGET_UNIX) // If we're unable to redirect, then we simply won't test GC at this @@ -1452,6 +1460,7 @@ void DoGcStress (PCONTEXT regs, NativeCodeVersion nativeCodeVersion) DWORD offset = codeInfo.GetRelOffset(); Thread *pThread = GetThread(); + _ASSERTE(pThread); if (!IsGcCoverageInterruptInstruction(instrPtr)) { From 3564b03b02dfc47a4a739bdfa0ae99e394e46f95 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Sun, 15 Mar 2020 21:23:49 -0700 Subject: [PATCH 2/5] Update gccover.cpp Misspelled function referenced in comment. --- src/coreclr/src/vm/gccover.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/vm/gccover.cpp b/src/coreclr/src/vm/gccover.cpp index 2490ee89c3597a..15c78751931c24 100644 --- a/src/coreclr/src/vm/gccover.cpp +++ b/src/coreclr/src/vm/gccover.cpp @@ -1404,8 +1404,8 @@ BOOL OnGcCoverageInterrupt(PCONTEXT regs) { // No thread at the moment so we aren't doing coverage for this function. // This should only occur for methods with the NativeCallableAttribute, - // where the caller could be coming from a thread unknown to the CLR and - // we haven't created a thread yet - see PreStubWorker_Premptive(). + // where the call could be coming from a thread unknown to the CLR and + // we haven't created a thread yet - see PreStubWorker_Preemptive(). _ASSERTE(pMD->HasNativeCallableAttribute()); return (FALSE); } From 33378a7ab15face3ef8e510a5c07a5d513ea0590 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Sun, 15 Mar 2020 21:57:16 -0700 Subject: [PATCH 3/5] Remove MethodDesc from GC coverage. --- src/coreclr/src/vm/gccover.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/src/vm/gccover.cpp b/src/coreclr/src/vm/gccover.cpp index 15c78751931c24..96cd53ce2c3f05 100644 --- a/src/coreclr/src/vm/gccover.cpp +++ b/src/coreclr/src/vm/gccover.cpp @@ -1407,7 +1407,8 @@ BOOL OnGcCoverageInterrupt(PCONTEXT regs) // where the call could be coming from a thread unknown to the CLR and // we haven't created a thread yet - see PreStubWorker_Preemptive(). _ASSERTE(pMD->HasNativeCallableAttribute()); - return (FALSE); + RemoveGcCoverageInterrupt(instrPtr, savedInstrPtr); + return TRUE; } #if defined(USE_REDIRECT_FOR_GCSTRESS) && !defined(TARGET_UNIX) From d6989bd50c5791c0f043c48bc136c360aa30f4d1 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Mon, 16 Mar 2020 16:34:28 -0700 Subject: [PATCH 4/5] Code review and fix some asserts when running the Debug version of tests. --- src/coreclr/src/vm/excep.cpp | 6 ++++++ src/coreclr/src/vm/jitinterface.cpp | 7 ++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/coreclr/src/vm/excep.cpp b/src/coreclr/src/vm/excep.cpp index 35a39f359cecc8..792a2d7a155750 100644 --- a/src/coreclr/src/vm/excep.cpp +++ b/src/coreclr/src/vm/excep.cpp @@ -7979,6 +7979,7 @@ LONG WINAPI CLRVectoredExceptionHandlerShim(PEXCEPTION_POINTERS pExceptionInfo) // // 1) We have a valid Thread object (implies exception on managed thread) // 2) Not a valid Thread object but the IP is in the execution engine (implies native thread within EE faulted) + // 3) The exception occurred in a GC marked location when no thread exists (i.e. reverse P/Invoke with NativeCallableAttribute). if (pThread || fExceptionInEE) { if (!bIsGCMarker) @@ -8066,6 +8067,11 @@ LONG WINAPI CLRVectoredExceptionHandlerShim(PEXCEPTION_POINTERS pExceptionInfo) #endif // FEATURE_EH_FUNCLETS } + else if (bIsGCMarker) + { + _ASSERTE(pThread == NULL); + result = EXCEPTION_CONTINUE_EXECUTION; + } SetLastError(dwLastError); diff --git a/src/coreclr/src/vm/jitinterface.cpp b/src/coreclr/src/vm/jitinterface.cpp index 2c7b37d6518dbd..66de648e9ab3fd 100644 --- a/src/coreclr/src/vm/jitinterface.cpp +++ b/src/coreclr/src/vm/jitinterface.cpp @@ -12678,9 +12678,10 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, flags = GetCompileFlags(ftn, flags, &methodInfo); - // If the reverse P/Invoke flag is used, we aren't going to support - // any tiered compilation. - if (flags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_REVERSE_PINVOKE)) + // Clearing all tier flags and mark as optimized if the reverse P/Invoke + // flag is used and the function is eligible. + if (flags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_REVERSE_PINVOKE) + && ftn->IsEligibleForTieredCompilation()) { _ASSERTE(config->GetCallerGCMode() != CallerGCMode::Coop); From ee53fec33c00d936706b54cb5004a405f552f6d3 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Mon, 16 Mar 2020 20:22:17 -0700 Subject: [PATCH 5/5] Limit JIT flag manipulation to FEATURE_TIERED_COMPILATION --- src/coreclr/src/vm/jitinterface.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/vm/jitinterface.cpp b/src/coreclr/src/vm/jitinterface.cpp index 66de648e9ab3fd..3ec424eab68312 100644 --- a/src/coreclr/src/vm/jitinterface.cpp +++ b/src/coreclr/src/vm/jitinterface.cpp @@ -12678,6 +12678,7 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, flags = GetCompileFlags(ftn, flags, &methodInfo); +#ifdef FEATURE_TIERED_COMPILATION // Clearing all tier flags and mark as optimized if the reverse P/Invoke // flag is used and the function is eligible. if (flags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_REVERSE_PINVOKE) @@ -12689,10 +12690,9 @@ PCODE UnsafeJitFunction(PrepareCodeConfig* config, flags.Clear(CORJIT_FLAGS::CORJIT_FLAG_TIER0); flags.Clear(CORJIT_FLAGS::CORJIT_FLAG_TIER1); -#ifdef FEATURE_TIERED_COMPILATION config->SetJitSwitchedToOptimized(); -#endif // FEATURE_TIERED_COMPILATION } +#endif // FEATURE_TIERED_COMPILATION #ifdef _DEBUG if (!flags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_SKIP_VERIFICATION))