From 752091cb35e6f3ba333b1133d9f1ff45d5b666d7 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 24 Feb 2022 19:00:40 -0800 Subject: [PATCH 1/9] RestoreContextSimulated --- src/coreclr/vm/threads.h | 8 ++ src/coreclr/vm/threadsuspend.cpp | 128 ++++++++++--------------------- 2 files changed, 48 insertions(+), 88 deletions(-) diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index f51f75445b3995..f49b7b54eeda60 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -3076,6 +3076,14 @@ class Thread static void __stdcall RedirectedHandledJITCaseForGCStress(); #endif // defined(HAVE_GCCOVER) && USE_REDIRECT_FOR_GCSTRESS +#ifdef TARGET_X86 + // RtlRestoreContext is available on x86, but relatively recently. + // RestoreContextSimulated uses SEH machinery for a similar result on legacy OS-es. + // This function should not be used on new OS-es as the pattern is not + // guranteed to continue working in the future. + static void __stdcall RestoreContextSimulated(Thread* pThread, CONTEXT* pCtx, void* pFrame); +#endif + friend void CPFH_AdjustContextForThreadSuspensionRace(T_CONTEXT *pContext, Thread *pThread); #endif // FEATURE_HIJACK && !TARGET_UNIX diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index d73329268db63c..fed29341e488cc 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -2515,18 +2515,17 @@ void RedirectedThreadFrame::ExceptionUnwind() int RedirectedHandledJITCaseExceptionFilter( PEXCEPTION_POINTERS pExcepPtrs, // Exception data RedirectedThreadFrame *pFrame, // Frame on stack - BOOL fDone, // Whether redirect completed without exception CONTEXT *pCtx) // Saved context { // !!! Do not use a non-static contract here. // !!! Contract may insert an exception handling record. // !!! This function assumes that GetCurrentSEHRecord() returns the exception record set up in - // !!! Thread::RedirectedHandledJITCase + // !!! Thread::RestoreContextSimulated // // !!! Do not use an object with dtor, since it injects a fs:0 entry. STATIC_CONTRACT_NOTHROW; STATIC_CONTRACT_GC_TRIGGERS; - STATIC_CONTRACT_MODE_ANY; + STATIC_CONTRACT_MODE_COOPERATIVE; if (pExcepPtrs->ExceptionRecord->ExceptionCode == STATUS_STACK_OVERFLOW) { @@ -2535,38 +2534,8 @@ int RedirectedHandledJITCaseExceptionFilter( // Get the thread handle Thread *pThread = GetThread(); - - STRESS_LOG2(LF_SYNC, LL_INFO100, "In RedirectedHandledJITCaseExceptionFilter fDone = %d pFrame = %p\n", fDone, pFrame); - - // If we get here via COM+ exception, gc-mode is unknown. We need it to - // be cooperative for this function. - GCX_COOP_NO_DTOR(); - - // If the exception was due to the called client, then we need to figure out if it - // is an exception that can be eaten or if it needs to be handled elsewhere. - if (!fDone) - { - if (pExcepPtrs->ExceptionRecord->ExceptionFlags & EXCEPTION_NONCONTINUABLE) - { - return (EXCEPTION_CONTINUE_SEARCH); - } - - // Get the latest thrown object - OBJECTREF throwable = CLRException::GetThrowableFromExceptionRecord(pExcepPtrs->ExceptionRecord); - - // If this is an uncatchable exception, then let the exception be handled elsewhere - if (IsUncatchable(&throwable)) - { - pThread->EnablePreemptiveGC(); - return (EXCEPTION_CONTINUE_SEARCH); - } - } -#ifdef _DEBUG - else - { - _ASSERTE(pExcepPtrs->ExceptionRecord->ExceptionCode == EXCEPTION_HIJACK); - } -#endif + STRESS_LOG1(LF_SYNC, LL_INFO100, "In RedirectedHandledJITCaseExceptionFilter pFrame = %p\n", pFrame); + _ASSERTE(pExcepPtrs->ExceptionRecord->ExceptionCode == EXCEPTION_HIJACK); // Unlink the frame in preparation for resuming in managed code pFrame->Pop(); @@ -2642,6 +2611,38 @@ extern "C" PCONTEXT __stdcall GetCurrentSavedRedirectContext() return pContext; } +#ifdef TARGET_X86 + +void __stdcall Thread::RestoreContextSimulated(Thread* pThread, CONTEXT* pCtx, void* pFrame) +{ + pThread->HandleThreadAbort(); // Might throw an exception. + + // A counter to avoid a nasty case where an + // up-stack filter throws another exception + // causing our filter to be run again for + // some unrelated exception. + int filter_count = 0; + + __try + { + // Save the instruction pointer where we redirected last. This does not race with the check + // against this variable in HandledJitCase because the GC will not attempt to redirect the + // thread until the instruction pointer of this thread is back in managed code. + pThread->m_LastRedirectIP = GetIP(pCtx); + pThread->m_SpinCount = 0; + + RaiseException(EXCEPTION_HIJACK, 0, 0, NULL); + } + __except (++filter_count == 1 + ? RedirectedHandledJITCaseExceptionFilter(GetExceptionInformation(), (RedirectedThreadFrame*)pFrame, pCtx) + : EXCEPTION_CONTINUE_SEARCH) + { + _ASSERTE(!"Reached body of __except in Thread::RedirectedHandledJITCase"); + } +} + +#endif // TARGET_X86 + void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) { STATIC_CONTRACT_THROWS; @@ -2665,17 +2666,6 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) STRESS_LOG5(LF_SYNC, LL_INFO1000, "In RedirectedHandledJITcase reason 0x%x pFrame = %p pc = %p sp = %p fp = %p", reason, &frame, GetIP(pCtx), GetSP(pCtx), GetFP(pCtx)); -#ifdef TARGET_X86 - // This will indicate to the exception filter whether or not the exception is caused - // by us or the client. - BOOL fDone = FALSE; - int filter_count = 0; // A counter to avoid a nasty case where an - // up-stack filter throws another exception - // causing our filter to be run again for - // some unrelated exception. - - __try -#endif // TARGET_X86 { // Make sure this thread doesn't reuse the context memory. pThread->MarkRedirectContextInUse(pCtx); @@ -2692,42 +2682,13 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) else #endif // HAVE_GCCOVER && USE_REDIRECT_FOR_GCSTRESS { - // Enable PGC before calling out to the client to allow runtime suspend to finish - GCX_PREEMP_NO_DTOR(); - - // Notify the interface of the pending suspension - switch (reason) { - case RedirectReason_GCSuspension: - break; - case RedirectReason_DebugSuspension: - break; - case RedirectReason_UserSuspension: - // Do nothing; - break; - default: - _ASSERTE(!"Invalid redirect reason"); - break; - } - - // Disable preemptive GC so we can unlink the frame - GCX_PREEMP_NO_DTOR_END(); + _ASSERTE(reason == RedirectReason_GCSuspension || + reason == RedirectReason_DebugSuspension || + reason == RedirectReason_UserSuspension); } #ifdef TARGET_X86 - pThread->HandleThreadAbort(); // Might throw an exception. - - // Indicate that the call to the service went without an exception, and that - // we're raising our own exception to resume the thread to where it was - // redirected from - fDone = TRUE; - - // Save the instruction pointer where we redirected last. This does not race with the check - // against this variable in HandledJitCase because the GC will not attempt to redirect the - // thread until the instruction pointer of this thread is back in managed code. - pThread->m_LastRedirectIP = GetIP(pCtx); - pThread->m_SpinCount = 0; - - RaiseException(EXCEPTION_HIJACK, 0, 0, NULL); + RestoreContextSimulated(pThread, pCtx, &frame); #else // TARGET_X86 @@ -2790,15 +2751,6 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) } #endif // TARGET_X86 } -#ifdef TARGET_X86 - __except (++filter_count == 1 - ? RedirectedHandledJITCaseExceptionFilter(GetExceptionInformation(), &frame, fDone, pCtx) - : EXCEPTION_CONTINUE_SEARCH) - { - _ASSERTE(!"Reached body of __except in Thread::RedirectedHandledJITCase"); - } - -#endif // TARGET_X86 } //**************************************************************************************** From 7fbba05501bd24158bd6544991e92e18bb43a164 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 24 Feb 2022 20:14:26 -0800 Subject: [PATCH 2/9] probe for RtlRestoreContext --- src/coreclr/vm/threadsuspend.cpp | 77 +++++++++++++++++++------------- 1 file changed, 46 insertions(+), 31 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index fed29341e488cc..8ee395e2509688 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -1948,6 +1948,9 @@ typedef BOOL(WINAPI* PINITIALIZECONTEXT2)(PVOID Buffer, DWORD ContextFlags, PCON PINITIALIZECONTEXT2 pfnInitializeContext2 = NULL; #ifdef TARGET_X86 +typedef VOID(__cdecl* PRTLRESTORECONTEXT)(PCONTEXT ContextRecord, struct _EXCEPTION_RECORD* ExceptionRecord); +PRTLRESTORECONTEXT pfnRtlRestoreContext = NULL; + #define CONTEXT_COMPLETE (CONTEXT_FULL | CONTEXT_FLOATING_POINT | \ CONTEXT_DEBUG_REGISTERS | CONTEXT_EXTENDED_REGISTERS | CONTEXT_EXCEPTION_REQUEST) #else @@ -1965,6 +1968,13 @@ CONTEXT* AllocateOSContextHelper(BYTE** contextBuffer) { HMODULE hm = GetModuleHandleW(_T("kernel32.dll")); pfnInitializeContext2 = (PINITIALIZECONTEXT2)GetProcAddress(hm, "InitializeContext2"); + +#ifdef TARGET_X86 + if (pfnRtlRestoreContext == NULL) + { + pfnRtlRestoreContext = (PRTLRESTORECONTEXT)GetProcAddress(hm, "RtlRestoreContext"); + } +#endif //TARGET_X86 } // Determine if the processor supports AVX so we could @@ -2688,50 +2698,53 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) } #ifdef TARGET_X86 - RestoreContextSimulated(pThread, pCtx, &frame); - -#else // TARGET_X86 + if (!pfnRtlRestoreContext) + { + RestoreContextSimulated(pThread, pCtx, &frame); + } + else +#endif // TARGET_X86 + { #if defined(HAVE_GCCOVER) && defined(USE_REDIRECT_FOR_GCSTRESS) // GCCOVER - // - // If GCStress interrupts an IL stub or inlined p/invoke while it's running in preemptive mode, it switches the mode to - // cooperative - but we will resume to preemptive below. We should not trigger an abort in that case, as it will fail - // due to the GC mode. - // - if (!pThread->m_fPreemptiveGCDisabledForGCStress) + // + // If GCStress interrupts an IL stub or inlined p/invoke while it's running in preemptive mode, it switches the mode to + // cooperative - but we will resume to preemptive below. We should not trigger an abort in that case, as it will fail + // due to the GC mode. + // + if (!pThread->m_fPreemptiveGCDisabledForGCStress) #endif - { - - UINT_PTR uAbortAddr; - UINT_PTR uResumePC = (UINT_PTR)GetIP(pCtx); - CopyOSContext(pThread->m_OSContext, pCtx); - uAbortAddr = (UINT_PTR)COMPlusCheckForAbort(); - if (uAbortAddr) { - LOG((LF_EH, LL_INFO100, "thread abort in progress, resuming thread under control... (handled jit case)\n")); - CONSISTENCY_CHECK(CheckPointer(pCtx)); + UINT_PTR uAbortAddr; + UINT_PTR uResumePC = (UINT_PTR)GetIP(pCtx); + CopyOSContext(pThread->m_OSContext, pCtx); + uAbortAddr = (UINT_PTR)COMPlusCheckForAbort(); + if (uAbortAddr) + { + LOG((LF_EH, LL_INFO100, "thread abort in progress, resuming thread under control... (handled jit case)\n")); + + CONSISTENCY_CHECK(CheckPointer(pCtx)); - STRESS_LOG1(LF_EH, LL_INFO10, "resume under control: ip: %p (handled jit case)\n", uResumePC); + STRESS_LOG1(LF_EH, LL_INFO10, "resume under control: ip: %p (handled jit case)\n", uResumePC); - SetIP(pThread->m_OSContext, uResumePC); + SetIP(pThread->m_OSContext, uResumePC); #if defined(TARGET_ARM) - // Save the original resume PC in Lr - pCtx->Lr = uResumePC; + // Save the original resume PC in Lr + pCtx->Lr = uResumePC; - // Since we have set a new IP, we have to clear conditional execution flags too. - ClearITState(pThread->m_OSContext); + // Since we have set a new IP, we have to clear conditional execution flags too. + ClearITState(pThread->m_OSContext); #endif // TARGET_ARM - SetIP(pCtx, uAbortAddr); + SetIP(pCtx, uAbortAddr); + } } - } - // Unlink the frame in preparation for resuming in managed code - frame.Pop(); + // Unlink the frame in preparation for resuming in managed code + frame.Pop(); - { // Allow future use of the context pThread->UnmarkRedirectContextInUse(pCtx); @@ -2744,12 +2757,14 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) #endif LOG((LF_SYNC, LL_INFO1000, "Resuming execution with RtlRestoreContext\n")); - SetLastError(dwLastError); // END_PRESERVE_LAST_ERROR +#ifdef TARGET_X86 + pfnRtlRestoreContext(pCtx, NULL); +#else RtlRestoreContext(pCtx, NULL); +#endif } -#endif // TARGET_X86 } } From 50c5ed6e7f3346e9cb35ac9f563babd7eafa0337 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 24 Feb 2022 20:54:53 -0800 Subject: [PATCH 3/9] ntdll.dll --- src/coreclr/vm/threadsuspend.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 8ee395e2509688..debe7c8540dfb1 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -1968,14 +1968,15 @@ CONTEXT* AllocateOSContextHelper(BYTE** contextBuffer) { HMODULE hm = GetModuleHandleW(_T("kernel32.dll")); pfnInitializeContext2 = (PINITIALIZECONTEXT2)GetProcAddress(hm, "InitializeContext2"); + } #ifdef TARGET_X86 - if (pfnRtlRestoreContext == NULL) - { - pfnRtlRestoreContext = (PRTLRESTORECONTEXT)GetProcAddress(hm, "RtlRestoreContext"); - } -#endif //TARGET_X86 + if (pfnRtlRestoreContext == NULL) + { + HMODULE hm = GetModuleHandleW(_T("ntdll.dll")); + pfnRtlRestoreContext = (PRTLRESTORECONTEXT)GetProcAddress(hm, "RtlRestoreContext"); } +#endif //TARGET_X86 // Determine if the processor supports AVX so we could // retrieve extended registers From b176a8ec49fe3a293dde463bde84236d359b70af Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 25 Feb 2022 11:49:07 -0800 Subject: [PATCH 4/9] restore self-trap sequence --- src/coreclr/vm/threadsuspend.cpp | 131 +++++++++++++++++-------------- 1 file changed, 72 insertions(+), 59 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index debe7c8540dfb1..d4cb0ab4e8fb6b 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -2677,95 +2677,108 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) STRESS_LOG5(LF_SYNC, LL_INFO1000, "In RedirectedHandledJITcase reason 0x%x pFrame = %p pc = %p sp = %p fp = %p", reason, &frame, GetIP(pCtx), GetSP(pCtx), GetFP(pCtx)); - { - // Make sure this thread doesn't reuse the context memory. - pThread->MarkRedirectContextInUse(pCtx); + // Make sure this thread doesn't reuse the context memory. + pThread->MarkRedirectContextInUse(pCtx); - // Link in the frame - frame.Push(); + // Link in the frame + frame.Push(); #if defined(HAVE_GCCOVER) && defined(USE_REDIRECT_FOR_GCSTRESS) // GCCOVER - if (reason == RedirectReason_GCStress) - { - _ASSERTE(pThread->PreemptiveGCDisabledOther()); - DoGcStress(frame.GetContext(), NULL); - } - else + if (reason == RedirectReason_GCStress) + { + _ASSERTE(pThread->PreemptiveGCDisabledOther()); + DoGcStress(frame.GetContext(), NULL); + } + else #endif // HAVE_GCCOVER && USE_REDIRECT_FOR_GCSTRESS - { - _ASSERTE(reason == RedirectReason_GCSuspension || - reason == RedirectReason_DebugSuspension || - reason == RedirectReason_UserSuspension); - } + { + _ASSERTE(reason == RedirectReason_GCSuspension || + reason == RedirectReason_DebugSuspension || + reason == RedirectReason_UserSuspension); + + // Actual self-suspension. + // Leave and reenter COOP mode to be trapped on the way back. + GCX_PREEMP_NO_DTOR(); + GCX_PREEMP_NO_DTOR_END(); + } + + // Once we get here the suspension is over! + // We will restore the state as it was at the point of redirection + // and continue normal execution. #ifdef TARGET_X86 - if (!pfnRtlRestoreContext) - { - RestoreContextSimulated(pThread, pCtx, &frame); - } - else + if (!pfnRtlRestoreContext) + { + RestoreContextSimulated(pThread, pCtx, &frame); + + // we never return to the caller. + __UNREACHABLE(); + } + else #endif // TARGET_X86 - { + { #if defined(HAVE_GCCOVER) && defined(USE_REDIRECT_FOR_GCSTRESS) // GCCOVER - // - // If GCStress interrupts an IL stub or inlined p/invoke while it's running in preemptive mode, it switches the mode to - // cooperative - but we will resume to preemptive below. We should not trigger an abort in that case, as it will fail - // due to the GC mode. - // - if (!pThread->m_fPreemptiveGCDisabledForGCStress) + // + // If GCStress interrupts an IL stub or inlined p/invoke while it's running in preemptive mode, it switches the mode to + // cooperative - but we will resume to preemptive below. We should not trigger an abort in that case, as it will fail + // due to the GC mode. + // + if (!pThread->m_fPreemptiveGCDisabledForGCStress) #endif - { + { - UINT_PTR uAbortAddr; - UINT_PTR uResumePC = (UINT_PTR)GetIP(pCtx); - CopyOSContext(pThread->m_OSContext, pCtx); - uAbortAddr = (UINT_PTR)COMPlusCheckForAbort(); - if (uAbortAddr) - { - LOG((LF_EH, LL_INFO100, "thread abort in progress, resuming thread under control... (handled jit case)\n")); + UINT_PTR uAbortAddr; + UINT_PTR uResumePC = (UINT_PTR)GetIP(pCtx); + CopyOSContext(pThread->m_OSContext, pCtx); + uAbortAddr = (UINT_PTR)COMPlusCheckForAbort(); + if (uAbortAddr) + { + LOG((LF_EH, LL_INFO100, "thread abort in progress, resuming thread under control... (handled jit case)\n")); - CONSISTENCY_CHECK(CheckPointer(pCtx)); + CONSISTENCY_CHECK(CheckPointer(pCtx)); - STRESS_LOG1(LF_EH, LL_INFO10, "resume under control: ip: %p (handled jit case)\n", uResumePC); + STRESS_LOG1(LF_EH, LL_INFO10, "resume under control: ip: %p (handled jit case)\n", uResumePC); - SetIP(pThread->m_OSContext, uResumePC); + SetIP(pThread->m_OSContext, uResumePC); #if defined(TARGET_ARM) - // Save the original resume PC in Lr - pCtx->Lr = uResumePC; + // Save the original resume PC in Lr + pCtx->Lr = uResumePC; - // Since we have set a new IP, we have to clear conditional execution flags too. - ClearITState(pThread->m_OSContext); + // Since we have set a new IP, we have to clear conditional execution flags too. + ClearITState(pThread->m_OSContext); #endif // TARGET_ARM - SetIP(pCtx, uAbortAddr); - } + SetIP(pCtx, uAbortAddr); } + } - // Unlink the frame in preparation for resuming in managed code - frame.Pop(); + // Unlink the frame in preparation for resuming in managed code + frame.Pop(); - // Allow future use of the context - pThread->UnmarkRedirectContextInUse(pCtx); + // Allow future use of the context + pThread->UnmarkRedirectContextInUse(pCtx); #if defined(HAVE_GCCOVER) && defined(USE_REDIRECT_FOR_GCSTRESS) // GCCOVER - if (pThread->m_fPreemptiveGCDisabledForGCStress) - { - pThread->EnablePreemptiveGC(); - pThread->m_fPreemptiveGCDisabledForGCStress = false; - } + if (pThread->m_fPreemptiveGCDisabledForGCStress) + { + pThread->EnablePreemptiveGC(); + pThread->m_fPreemptiveGCDisabledForGCStress = false; + } #endif - LOG((LF_SYNC, LL_INFO1000, "Resuming execution with RtlRestoreContext\n")); - SetLastError(dwLastError); // END_PRESERVE_LAST_ERROR + LOG((LF_SYNC, LL_INFO1000, "Resuming execution with RtlRestoreContext\n")); + SetLastError(dwLastError); // END_PRESERVE_LAST_ERROR #ifdef TARGET_X86 - pfnRtlRestoreContext(pCtx, NULL); + pfnRtlRestoreContext(pCtx, NULL); #else - RtlRestoreContext(pCtx, NULL); + RtlRestoreContext(pCtx, NULL); #endif - } + + // we never return to the caller. + __UNREACHABLE(); } } From de513591b81bf349030c35a1af8a1f05313d5b35 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 25 Feb 2022 11:52:15 -0800 Subject: [PATCH 5/9] PR feedback --- src/coreclr/vm/threads.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index f49b7b54eeda60..48676551179749 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -3081,7 +3081,7 @@ class Thread // RestoreContextSimulated uses SEH machinery for a similar result on legacy OS-es. // This function should not be used on new OS-es as the pattern is not // guranteed to continue working in the future. - static void __stdcall RestoreContextSimulated(Thread* pThread, CONTEXT* pCtx, void* pFrame); + static void RestoreContextSimulated(Thread* pThread, CONTEXT* pCtx, void* pFrame); #endif friend void CPFH_AdjustContextForThreadSuspensionRace(T_CONTEXT *pContext, Thread *pThread); From d4e1f5235cb2158fb3b6f77009e430a7ba459103 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 25 Feb 2022 15:40:09 -0800 Subject: [PATCH 6/9] Clarify CopyContext in RedirectedHandledJITCaseExceptionFilter --- src/coreclr/vm/threadsuspend.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index d4cb0ab4e8fb6b..5159865eb8bb8f 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -2551,12 +2551,12 @@ int RedirectedHandledJITCaseExceptionFilter( // Unlink the frame in preparation for resuming in managed code pFrame->Pop(); - // Copy the saved context record into the EH context; - // NB: cannot use ReplaceExceptionContextRecord here. - // these contexts may contain extended registers and may have different format - // for reasons such as alignment or context compaction + // Copy everything in the saved context record into the EH context. + // Historically the EH context has enough space for every enabled context feature. + // That may not hold for the future features beyond AVX, but this codepath is + // supposed to be used only on OSes that do not have RtlRestoreContext. CONTEXT* pTarget = pExcepPtrs->ContextRecord; - if (!CopyContext(pTarget, pTarget->ContextFlags, pCtx)) + if (!CopyContext(pTarget, pCtx->ContextFlags, pCtx)) { STRESS_LOG1(LF_SYNC, LL_ERROR, "ERROR: Could not set context record, lastError = 0x%x\n", GetLastError()); EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE); From 4e4245dcd09a68a1a60fc592e369a05edccb7cd9 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 1 Mar 2022 16:23:59 -0800 Subject: [PATCH 7/9] simpler indentation. --- src/coreclr/vm/threadsuspend.cpp | 79 +++++++++++++++----------------- 1 file changed, 38 insertions(+), 41 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 5159865eb8bb8f..c9d4265ebd6773 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -2714,72 +2714,69 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) // we never return to the caller. __UNREACHABLE(); } - else #endif // TARGET_X86 - { #if defined(HAVE_GCCOVER) && defined(USE_REDIRECT_FOR_GCSTRESS) // GCCOVER - // - // If GCStress interrupts an IL stub or inlined p/invoke while it's running in preemptive mode, it switches the mode to - // cooperative - but we will resume to preemptive below. We should not trigger an abort in that case, as it will fail - // due to the GC mode. - // - if (!pThread->m_fPreemptiveGCDisabledForGCStress) + // + // If GCStress interrupts an IL stub or inlined p/invoke while it's running in preemptive mode, it switches the mode to + // cooperative - but we will resume to preemptive below. We should not trigger an abort in that case, as it will fail + // due to the GC mode. + // + if (!pThread->m_fPreemptiveGCDisabledForGCStress) #endif - { + { - UINT_PTR uAbortAddr; - UINT_PTR uResumePC = (UINT_PTR)GetIP(pCtx); - CopyOSContext(pThread->m_OSContext, pCtx); - uAbortAddr = (UINT_PTR)COMPlusCheckForAbort(); - if (uAbortAddr) - { - LOG((LF_EH, LL_INFO100, "thread abort in progress, resuming thread under control... (handled jit case)\n")); + UINT_PTR uAbortAddr; + UINT_PTR uResumePC = (UINT_PTR)GetIP(pCtx); + CopyOSContext(pThread->m_OSContext, pCtx); + uAbortAddr = (UINT_PTR)COMPlusCheckForAbort(); + if (uAbortAddr) + { + LOG((LF_EH, LL_INFO100, "thread abort in progress, resuming thread under control... (handled jit case)\n")); - CONSISTENCY_CHECK(CheckPointer(pCtx)); + CONSISTENCY_CHECK(CheckPointer(pCtx)); - STRESS_LOG1(LF_EH, LL_INFO10, "resume under control: ip: %p (handled jit case)\n", uResumePC); + STRESS_LOG1(LF_EH, LL_INFO10, "resume under control: ip: %p (handled jit case)\n", uResumePC); - SetIP(pThread->m_OSContext, uResumePC); + SetIP(pThread->m_OSContext, uResumePC); #if defined(TARGET_ARM) - // Save the original resume PC in Lr - pCtx->Lr = uResumePC; + // Save the original resume PC in Lr + pCtx->Lr = uResumePC; - // Since we have set a new IP, we have to clear conditional execution flags too. - ClearITState(pThread->m_OSContext); + // Since we have set a new IP, we have to clear conditional execution flags too. + ClearITState(pThread->m_OSContext); #endif // TARGET_ARM - SetIP(pCtx, uAbortAddr); - } + SetIP(pCtx, uAbortAddr); } + } - // Unlink the frame in preparation for resuming in managed code - frame.Pop(); + // Unlink the frame in preparation for resuming in managed code + frame.Pop(); - // Allow future use of the context - pThread->UnmarkRedirectContextInUse(pCtx); + // Allow future use of the context + pThread->UnmarkRedirectContextInUse(pCtx); #if defined(HAVE_GCCOVER) && defined(USE_REDIRECT_FOR_GCSTRESS) // GCCOVER - if (pThread->m_fPreemptiveGCDisabledForGCStress) - { - pThread->EnablePreemptiveGC(); - pThread->m_fPreemptiveGCDisabledForGCStress = false; - } + if (pThread->m_fPreemptiveGCDisabledForGCStress) + { + pThread->EnablePreemptiveGC(); + pThread->m_fPreemptiveGCDisabledForGCStress = false; + } #endif - LOG((LF_SYNC, LL_INFO1000, "Resuming execution with RtlRestoreContext\n")); - SetLastError(dwLastError); // END_PRESERVE_LAST_ERROR + LOG((LF_SYNC, LL_INFO1000, "Resuming execution with RtlRestoreContext\n")); + SetLastError(dwLastError); // END_PRESERVE_LAST_ERROR #ifdef TARGET_X86 - pfnRtlRestoreContext(pCtx, NULL); + pfnRtlRestoreContext(pCtx, NULL); #else - RtlRestoreContext(pCtx, NULL); + RtlRestoreContext(pCtx, NULL); #endif - // we never return to the caller. - __UNREACHABLE(); - } + // we never return to the caller. + __UNREACHABLE(); } //**************************************************************************************** From 6fcc73e1a2366c4d2969470159b0011e5fe0cfd9 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 1 Mar 2022 18:16:54 -0800 Subject: [PATCH 8/9] restore last error on the legacy path. --- src/coreclr/vm/threads.h | 2 +- src/coreclr/vm/threadsuspend.cpp | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 48676551179749..9b12f507182f5b 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -3081,7 +3081,7 @@ class Thread // RestoreContextSimulated uses SEH machinery for a similar result on legacy OS-es. // This function should not be used on new OS-es as the pattern is not // guranteed to continue working in the future. - static void RestoreContextSimulated(Thread* pThread, CONTEXT* pCtx, void* pFrame); + static void RestoreContextSimulated(Thread* pThread, CONTEXT* pCtx, void* pFrame, DWORD dwLastError); #endif friend void CPFH_AdjustContextForThreadSuspensionRace(T_CONTEXT *pContext, Thread *pThread); diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index c9d4265ebd6773..bddd177b1a3f14 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -2526,7 +2526,8 @@ void RedirectedThreadFrame::ExceptionUnwind() int RedirectedHandledJITCaseExceptionFilter( PEXCEPTION_POINTERS pExcepPtrs, // Exception data RedirectedThreadFrame *pFrame, // Frame on stack - CONTEXT *pCtx) // Saved context + CONTEXT *pCtx, // Saved context + DWORD dwLastError) // saved last error { // !!! Do not use a non-static contract here. // !!! Contract may insert an exception handling record. @@ -2590,6 +2591,9 @@ int RedirectedHandledJITCaseExceptionFilter( // Register the special OS handler as the top handler with the OS SetCurrentSEHRecord(pCurSEH); + // restore last error + SetLastError(dwLastError); + // Resume execution at point where thread was originally redirected return (EXCEPTION_CONTINUE_EXECUTION); } @@ -2624,7 +2628,7 @@ extern "C" PCONTEXT __stdcall GetCurrentSavedRedirectContext() #ifdef TARGET_X86 -void __stdcall Thread::RestoreContextSimulated(Thread* pThread, CONTEXT* pCtx, void* pFrame) +void Thread::RestoreContextSimulated(Thread* pThread, CONTEXT* pCtx, void* pFrame, DWORD dwLastError) { pThread->HandleThreadAbort(); // Might throw an exception. @@ -2645,7 +2649,7 @@ void __stdcall Thread::RestoreContextSimulated(Thread* pThread, CONTEXT* pCtx, RaiseException(EXCEPTION_HIJACK, 0, 0, NULL); } __except (++filter_count == 1 - ? RedirectedHandledJITCaseExceptionFilter(GetExceptionInformation(), (RedirectedThreadFrame*)pFrame, pCtx) + ? RedirectedHandledJITCaseExceptionFilter(GetExceptionInformation(), (RedirectedThreadFrame*)pFrame, pCtx, dwLastError) : EXCEPTION_CONTINUE_SEARCH) { _ASSERTE(!"Reached body of __except in Thread::RedirectedHandledJITCase"); @@ -2709,7 +2713,7 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) #ifdef TARGET_X86 if (!pfnRtlRestoreContext) { - RestoreContextSimulated(pThread, pCtx, &frame); + RestoreContextSimulated(pThread, pCtx, &frame, dwLastError); // we never return to the caller. __UNREACHABLE(); From bfee187318a1cd29df2f78f9eef14ddd0a4e7f43 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Tue, 1 Mar 2022 19:19:31 -0700 Subject: [PATCH 9/9] Update src/coreclr/vm/threads.h --- src/coreclr/vm/threads.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 9b12f507182f5b..26fbd48550cc4b 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -3080,7 +3080,7 @@ class Thread // RtlRestoreContext is available on x86, but relatively recently. // RestoreContextSimulated uses SEH machinery for a similar result on legacy OS-es. // This function should not be used on new OS-es as the pattern is not - // guranteed to continue working in the future. + // guaranteed to continue working in the future. static void RestoreContextSimulated(Thread* pThread, CONTEXT* pCtx, void* pFrame, DWORD dwLastError); #endif