From 8f976528084d17f9d6d1eef2519c961325f9e22f Mon Sep 17 00:00:00 2001 From: vsadov Date: Mon, 25 Jan 2021 18:36:35 -0800 Subject: [PATCH 1/2] port of https://github.com/dotnet/runtime/pull/47212 --- src/coreclr/src/vm/threads.cpp | 26 ++- src/coreclr/src/vm/threads.h | 48 +++++- src/coreclr/src/vm/threadsuspend.cpp | 249 +++++++++++++-------------- 3 files changed, 182 insertions(+), 141 deletions(-) diff --git a/src/coreclr/src/vm/threads.cpp b/src/coreclr/src/vm/threads.cpp index bc09aaf9aaf630..e278d555fb4857 100644 --- a/src/coreclr/src/vm/threads.cpp +++ b/src/coreclr/src/vm/threads.cpp @@ -106,7 +106,10 @@ thread_local int t_ForbidGCLoaderUseCount; uint64_t Thread::dead_threads_non_alloc_bytes = 0; SPTR_IMPL(ThreadStore, ThreadStore, s_pThreadStore); -CONTEXT *ThreadStore::s_pOSContext = NULL; + +CONTEXT* ThreadStore::s_pOSContext = NULL; +BYTE* ThreadStore::s_pOSContextBuffer = NULL; + CLREvent *ThreadStore::s_pWaitForStackCrawlEvent; PTR_ThreadLocalModule ThreadLocalBlock::GetTLMIfExists(ModuleIndex index) @@ -1463,7 +1466,6 @@ Thread::Thread() m_fHasDeadThreadBeenConsideredForGCTrigger = false; m_TraceCallCount = 0; m_ThrewControlForThread = 0; - m_OSContext = NULL; m_ThreadTasks = (ThreadTasks)0; m_pLoadLimiter= NULL; @@ -1497,7 +1499,11 @@ Thread::Thread() NewHolder contextHolder(m_OSContext); m_pSavedRedirectContext = NULL; - NewHolder savedRedirectContextHolder(m_pSavedRedirectContext); + m_pOSContextBuffer = NULL; + +#ifdef _DEBUG + m_RedirectContextInUse = false; +#endif #ifdef FEATURE_COMINTEROP m_pRCWStack = new RCWStackHeader(); @@ -1568,7 +1574,6 @@ Thread::Thread() trackSyncHolder.SuppressRelease(); #endif contextHolder.SuppressRelease(); - savedRedirectContextHolder.SuppressRelease(); #ifdef FEATURE_COMINTEROP m_uliInitializeSpyCookie.QuadPart = 0ul; @@ -2604,11 +2609,18 @@ Thread::~Thread() if (m_OSContext) delete m_OSContext; - if (GetSavedRedirectContext()) + if (m_pOSContextBuffer) { - delete GetSavedRedirectContext(); - SetSavedRedirectContext(NULL); + delete[] m_pOSContextBuffer; + m_pOSContextBuffer = NULL; } + else if (m_pSavedRedirectContext) + { + delete m_pSavedRedirectContext; + } + + MarkRedirectContextInUse(m_pSavedRedirectContext); + m_pSavedRedirectContext = NULL; #ifdef FEATURE_COMINTEROP if (m_pRCWStack) diff --git a/src/coreclr/src/vm/threads.h b/src/coreclr/src/vm/threads.h index 36bf5b30140821..266a1595839793 100644 --- a/src/coreclr/src/vm/threads.h +++ b/src/coreclr/src/vm/threads.h @@ -4103,8 +4103,21 @@ class Thread #endif // _DEBUG private: + // context used during redirection of this thread + // NOTE: there is only one. Since redirection cannot be nested + // if more than one are needed, something is wrong. PTR_CONTEXT m_pSavedRedirectContext; +// in a case when we need the redirection context to include CONTEXT_XSTATE +// this is the buffer that contains the context parts. +// we need the buffer so we could deallocate the whole deal. + BYTE* m_pOSContextBuffer; + +#ifdef _DEBUG + // validate that we use only one context per thread. + bool m_RedirectContextInUse; +#endif + BOOL IsContextSafeToRedirect(T_CONTEXT* pContext); public: @@ -4115,14 +4128,26 @@ class Thread } #ifndef DACCESS_COMPILE - void SetSavedRedirectContext(PT_CONTEXT pCtx) + void MarkRedirectContextInUse(PTR_CONTEXT pCtx) { LIMITED_METHOD_CONTRACT; - m_pSavedRedirectContext = pCtx; - } +#ifdef _DEBUG + _ASSERTE(!m_RedirectContextInUse); + _ASSERTE(pCtx == m_pSavedRedirectContext); + m_RedirectContextInUse = true; #endif + } - void EnsurePreallocatedContext(); + void UnmarkRedirectContextInUse(PTR_CONTEXT pCtx) + { + LIMITED_METHOD_CONTRACT; +#ifdef _DEBUG + _ASSERTE(m_RedirectContextInUse); + _ASSERTE(pCtx == m_pSavedRedirectContext); + m_RedirectContextInUse = false; +#endif + } +#endif //DACCESS_COMPILE ThreadLocalBlock m_ThreadLocalBlock; @@ -4983,12 +5008,21 @@ class ThreadStore #endif private: + static BYTE* s_pOSContextBuffer; static CONTEXT *s_pOSContext; public: - // We can not do any memory allocation after we suspend a thread in order ot - // avoid deadlock situation. + // Pre-allocate an OS context for possible use by a redirected thread and keep in a static variable. + // + // There are two reasons for this pattern: + // - We can not do any memory allocation after we suspend a thread in order to avoid deadlock situation. + // So, when anticipating a need, we must pre-allocate. + // + // - Even though we know the thread we are suspending, we do not want to put the context directly on the + // thread because the thread only _may_ need the context. Often it does not end up needing it, + // then we will keep the context for the next time like this. static void AllocateOSContext(); - static CONTEXT *GrabOSContext(); + // Retrieves and detaches the pre-alocated context + optional containing buffer (when CONTEXT_XSTATE is used) + static CONTEXT* GrabOSContext(BYTE** contextBuffer); private: // Thread abort needs to walk stack to decide if thread abort can proceed. diff --git a/src/coreclr/src/vm/threadsuspend.cpp b/src/coreclr/src/vm/threadsuspend.cpp index 3c3c0784ee6040..094218c585f130 100644 --- a/src/coreclr/src/vm/threadsuspend.cpp +++ b/src/coreclr/src/vm/threadsuspend.cpp @@ -2164,39 +2164,109 @@ void ThreadSuspend::UnlockThreadStore(BOOL bThreadDestroyed, ThreadSuspend::SUSP #endif } +typedef BOOL(WINAPI* PINITIALIZECONTEXT2)(PVOID Buffer, DWORD ContextFlags, PCONTEXT* Context, PDWORD ContextLength, ULONG64 XStateCompactionMask); +PINITIALIZECONTEXT2 pfnInitializeContext2 = NULL; -void ThreadStore::AllocateOSContext() -{ - LIMITED_METHOD_CONTRACT; - _ASSERTE(HoldingThreadStore()); - if (s_pOSContext == NULL -#ifdef _DEBUG - || s_pOSContext == (CONTEXT*)0x1 +#ifdef TARGET_X86 +#define CONTEXT_COMPLETE (CONTEXT_FULL | CONTEXT_FLOATING_POINT | \ + CONTEXT_DEBUG_REGISTERS | CONTEXT_EXTENDED_REGISTERS | CONTEXT_EXCEPTION_REQUEST) +#else +#define CONTEXT_COMPLETE (CONTEXT_FULL | CONTEXT_DEBUG_REGISTERS | CONTEXT_EXCEPTION_REQUEST) #endif - ) + +CONTEXT* AllocateOSContextHelper(BYTE** contextBuffer) +{ + CONTEXT* pOSContext = NULL; + +#if !defined(TARGET_UNIX) && (defined(TARGET_X86) || defined(TARGET_AMD64)) + DWORD context = CONTEXT_COMPLETE; + BOOL supportsAVX = FALSE; + + if (pfnInitializeContext2 == NULL) { - s_pOSContext = new (nothrow) CONTEXT(); + HMODULE hm = GetModuleHandleW(_T("kernel32.dll")); + pfnInitializeContext2 = (PINITIALIZECONTEXT2)GetProcAddress(hm, "InitializeContext2"); } -#ifdef _DEBUG - if (s_pOSContext == NULL) + + // Determine if the processor supports AVX so we could + // retrieve extended registers + DWORD64 FeatureMask = GetEnabledXStateFeatures(); + if ((FeatureMask & XSTATE_MASK_AVX) != 0) { - s_pOSContext = (CONTEXT*)0x1; + context = context | CONTEXT_XSTATE; + supportsAVX = TRUE; } + + // Retrieve contextSize by passing NULL for Buffer + DWORD contextSize = 0; + ULONG64 xStateCompactionMask = XSTATE_MASK_LEGACY | XSTATE_MASK_AVX; + // The initialize call should fail but return contextSize + BOOL success = pfnInitializeContext2 ? + pfnInitializeContext2(NULL, context, NULL, &contextSize, xStateCompactionMask) : + InitializeContext(NULL, context, NULL, &contextSize); + + _ASSERTE(!success && GetLastError() == ERROR_INSUFFICIENT_BUFFER); + + // So now allocate a buffer of that size and call InitializeContext again + BYTE* buffer = new (nothrow)BYTE[contextSize]; + if (buffer != NULL) + { + success = pfnInitializeContext2 ? + pfnInitializeContext2(buffer, context, &pOSContext, &contextSize, xStateCompactionMask) : + InitializeContext(buffer, context, &pOSContext, &contextSize); + + // if AVX is supported set the appropriate features mask in the context + if (success && supportsAVX) + { + // This should not normally fail. + // The system silently ignores any feature specified in the FeatureMask + // which is not enabled on the processor. + success = SetXStateFeaturesMask(pOSContext, XSTATE_MASK_AVX); + } + + if (!success) + { + delete[] buffer; + buffer = NULL; + } + } + + if (!success) + { + pOSContext = NULL; + } + + *contextBuffer = buffer; + +#else + pOSContext = new (nothrow) CONTEXT; + pOSContext->ContextFlags = CONTEXT_COMPLETE; + *contextBuffer = NULL; #endif + + return pOSContext; } -CONTEXT *ThreadStore::GrabOSContext() +void ThreadStore::AllocateOSContext() { LIMITED_METHOD_CONTRACT; _ASSERTE(HoldingThreadStore()); - CONTEXT *pContext = s_pOSContext; - s_pOSContext = NULL; -#ifdef _DEBUG - if (pContext == (CONTEXT*)0x1) + + if (s_pOSContext == NULL) { - pContext = NULL; + s_pOSContext = AllocateOSContextHelper(&s_pOSContextBuffer); } -#endif +} + +CONTEXT* ThreadStore::GrabOSContext(BYTE** contextBuffer) +{ + LIMITED_METHOD_CONTRACT; + _ASSERTE(HoldingThreadStore()); + + CONTEXT* pContext = s_pOSContext; + *contextBuffer = s_pOSContextBuffer; + s_pOSContext = NULL; + s_pOSContextBuffer = NULL; return pContext; } @@ -2702,16 +2772,8 @@ void RedirectedThreadFrame::ExceptionUnwind() Thread* pThread = GetThread(); - if (pThread->GetSavedRedirectContext()) - { - delete m_Regs; - } - else - { - // Save it for future use to avoid repeatedly new'ing - pThread->SetSavedRedirectContext(m_Regs); - } - + // Allow future use to avoid repeatedly new'ing + pThread->UnmarkRedirectContextInUse(m_Regs); m_Regs = NULL; } @@ -2791,15 +2853,9 @@ int RedirectedHandledJITCaseExceptionFilter( ReplaceExceptionContextRecord(pExcepPtrs->ContextRecord, pCtx); DWORD espValue = pCtx->Esp; - if (pThread->GetSavedRedirectContext()) - { - delete pCtx; - } - else - { - // Save it for future use to avoid repeatedly new'ing - pThread->SetSavedRedirectContext(pCtx); - } + + // Allow future use to avoid repeatedly new'ing + pThread->UnmarkRedirectContextInUse(pCtx); ///////////////////////////////////////////////////////////////////////////// // NOTE: Ugly, ugly workaround. @@ -2892,9 +2948,8 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) __try #endif // TARGET_X86 { - // Make sure this thread doesn't reuse the context memory in re-entrancy cases - _ASSERTE(pThread->GetSavedRedirectContext() != NULL); - pThread->SetSavedRedirectContext(NULL); + // Make sure this thread doesn't reuse the context memory. + pThread->MarkRedirectContextInUse(pCtx); // Link in the frame frame.Push(); @@ -2987,19 +3042,8 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) frame.Pop(); { - // Free the context struct if we already have one cached - if (pThread->GetSavedRedirectContext()) - { - CONTEXT* pCtxTemp = (CONTEXT*)_alloca(sizeof(CONTEXT)); - memcpy(pCtxTemp, pCtx, sizeof(CONTEXT)); - delete pCtx; - pCtx = pCtxTemp; - } - else - { - // Save it for future use to avoid repeatedly new'ing - pThread->SetSavedRedirectContext(pCtx); - } + // Allow future use of the context + pThread->UnmarkRedirectContextInUse(pCtx); #if defined(HAVE_GCCOVER) && defined(USE_REDIRECT_FOR_GCSTRESS) // GCCOVER if (pThread->m_fPreemptiveGCDisabledForGCStress) @@ -3102,16 +3146,9 @@ void __stdcall Thread::RedirectedHandledJITCaseForGCStress() // own stack. // -#ifdef TARGET_X86 -#define CONTEXT_COMPLETE (CONTEXT_FULL | CONTEXT_FLOATING_POINT | \ - CONTEXT_DEBUG_REGISTERS | CONTEXT_EXTENDED_REGISTERS | CONTEXT_EXCEPTION_REQUEST) -#else -#define CONTEXT_COMPLETE (CONTEXT_FULL | CONTEXT_DEBUG_REGISTERS | CONTEXT_EXCEPTION_REQUEST) -#endif - BOOL Thread::RedirectThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt) { - CONTRACTL { + CONTRACTL{ NOTHROW; GC_NOTRIGGER; } @@ -3119,56 +3156,29 @@ BOOL Thread::RedirectThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt) _ASSERTE(HandledJITCase()); _ASSERTE(GetThread() != this); + _ASSERTE(ThreadStore::HoldingThreadStore()); //////////////////////////////////////////////////////////////// // Acquire a context structure to save the thread state into - // We need to distinguish between two types of callers: - // - Most callers, including GC, operate while holding the ThreadStore + // All callers, including suspension, operate while holding the ThreadStore // lock. This means that we can pre-allocate a context structure // globally in the ThreadStore and use it in this function. - // - Some callers (currently only YieldTask) cannot take the ThreadStore - // lock. Therefore we always allocate a SavedRedirectContext in the - // Thread constructor. (Since YieldTask currently is the only caller - // that does not hold the ThreadStore lock, we only do this when - // we're hosted.) // Check whether we have a SavedRedirectContext we can reuse: - CONTEXT *pCtx = GetSavedRedirectContext(); + CONTEXT* pCtx = GetSavedRedirectContext(); - // If we've never allocated a context for this thread, do so now + // If we've never assigned a context for this thread, do so now if (!pCtx) { - // If our caller took the ThreadStore lock, then it pre-allocated - // a context in the ThreadStore: - if (ThreadStore::HoldingThreadStore()) - { - pCtx = ThreadStore::GrabOSContext(); - } - - if (!pCtx) - { - // Even when our caller is YieldTask, we can find a NULL - // SavedRedirectContext in this function: Consider the scenario - // where GC is in progress and has already redirected a thread. - // That thread will set its SavedRedirectContext to NULL to enable - // reentrancy. Now assume that the host calls YieldTask for the - // redirected thread. In this case, this function will simply - // fail, but that is fine: The redirected thread will check, - // before it resumes execution, whether it should yield. - return (FALSE); - } - - // Save the pointer for the redirect function - _ASSERTE(GetSavedRedirectContext() == NULL); - SetSavedRedirectContext(pCtx); + pCtx = m_pSavedRedirectContext = ThreadStore::GrabOSContext(&m_pOSContextBuffer); + _ASSERTE(GetSavedRedirectContext() != NULL); } ////////////////////////////////////// // Get and save the thread's context - // Always get complete context - pCtx->ContextFlags = CONTEXT_COMPLETE; + // Always get complete context, pCtx->ContextFlags are set during Initialization BOOL bRes = EEGetThreadContext(this, pCtx); _ASSERTE(bRes && "Failed to GetThreadContext in RedirectThreadAtHandledJITCase - aborting redirect."); @@ -3242,44 +3252,31 @@ BOOL Thread::RedirectCurrentThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt, CONT } CONTRACTL_END; - // REVISIT_TODO need equivalent of this for the current thread - //_ASSERTE(HandledJITCase()); - _ASSERTE(GetThread() == this); _ASSERTE(PreemptiveGCDisabledOther()); _ASSERTE(IsAddrOfRedirectFunc(pTgt)); _ASSERTE(pCurrentThreadCtx); _ASSERTE((pCurrentThreadCtx->ContextFlags & (CONTEXT_COMPLETE - CONTEXT_EXCEPTION_REQUEST)) - == (CONTEXT_COMPLETE - CONTEXT_EXCEPTION_REQUEST)); + == (CONTEXT_COMPLETE - CONTEXT_EXCEPTION_REQUEST)); _ASSERTE(ExecutionManager::IsManagedCode(GetIP(pCurrentThreadCtx))); //////////////////////////////////////////////////////////////// // Allocate a context structure to save the thread state into // Check to see if we've already got memory allocated for this purpose. - CONTEXT *pCtx = GetSavedRedirectContext(); + CONTEXT* pCtx = GetSavedRedirectContext(); - // If we've never allocated a context for this thread, do so now + // If we've never assigned a context for this thread, do so now if (!pCtx) { - pCtx = new (nothrow) CONTEXT(); - - if (!pCtx) - return (FALSE); - - // Save the pointer for the redirect function - _ASSERTE(GetSavedRedirectContext() == NULL); - SetSavedRedirectContext(pCtx); + pCtx = m_pSavedRedirectContext = AllocateOSContextHelper(&m_pOSContextBuffer); + _ASSERTE(GetSavedRedirectContext() != NULL); } ////////////////////////////////////// // Get and save the thread's context - - CopyMemory(pCtx, pCurrentThreadCtx, sizeof(CONTEXT)); - - // Clear any new bits we don't understand (like XSAVE) in case we pass - // this context to RtlRestoreContext (like for gcstress) - pCtx->ContextFlags &= CONTEXT_ALL; + BOOL success = CopyContext(pCtx, pCtx->ContextFlags, pCurrentThreadCtx); + _ASSERTE(success); // Ensure that this flag is set for the next time through the normal path, // RedirectThreadAtHandledJITCase. @@ -3623,11 +3620,6 @@ HRESULT ThreadSuspend::SuspendRuntime(ThreadSuspend::SUSPEND_REASON reason) RetrySuspension: #endif - // We can not allocate memory after we suspend a thread. - // Otherwise, we may deadlock the process, because the thread we just suspended - // might hold locks we would need to acquire while allocating. - ThreadStore::AllocateOSContext(); - #ifdef TIME_SUSPEND DWORD startSuspend = g_SuspendStatistics.GetTime(); #endif @@ -3635,6 +3627,11 @@ HRESULT ThreadSuspend::SuspendRuntime(ThreadSuspend::SUSPEND_REASON reason) // // Suspend the native thread. // + + // We can not allocate memory after we suspend a thread. + // Otherwise, we may deadlock the process, because the thread we just suspended + // might hold locks we would need to acquire while allocating. + ThreadStore::AllocateOSContext(); Thread::SuspendThreadResult str = thread->SuspendThread(); // We should just always build with this TIME_SUSPEND stuff, and report the results via ETW. @@ -4560,10 +4557,6 @@ bool Thread::SysStartSuspendForDebug(AppDomain *pAppDomain) RetrySuspension: #endif // FEATURE_HIJACK && !TARGET_UNIX - // We can not allocate memory after we suspend a thread. - // Otherwise, we may deadlock the process when CLR is hosted. - ThreadStore::AllocateOSContext(); - #ifdef DISABLE_THREADSUSPEND // On platforms that do not support safe thread suspension we have // to rely on the GCPOLL mechanism. @@ -4577,6 +4570,9 @@ bool Thread::SysStartSuspendForDebug(AppDomain *pAppDomain) SuspendThreadResult str = STR_Success; FastInterlockOr(&thread->m_fPreemptiveGCDisabled, 0); #else + // We can not allocate memory after we suspend a thread. + // Otherwise, we may deadlock if suspended thread holds allocator locks. + ThreadStore::AllocateOSContext(); SuspendThreadResult str = thread->SuspendThread(); #endif // DISABLE_THREADSUSPEND @@ -4760,9 +4756,8 @@ bool Thread::SysSweepThreadsForDebug(bool forceSync) RetrySuspension: // We can not allocate memory after we suspend a thread. - // Otherwise, we may deadlock the process when CLR is hosted. + // Otherwise, we may deadlock if the suspended thread holds allocator locks. ThreadStore::AllocateOSContext(); - SuspendThreadResult str = thread->SuspendThread(); if (str == STR_Failure || str == STR_UnstartedOrDead) From 7a30dec04fbf1be9e716037b4c8f54d393e7be35 Mon Sep 17 00:00:00 2001 From: vsadov Date: Mon, 25 Jan 2021 19:23:47 -0800 Subject: [PATCH 2/2] missing change for Unix. --- src/coreclr/src/vm/threadsuspend.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/src/vm/threadsuspend.cpp b/src/coreclr/src/vm/threadsuspend.cpp index 094218c585f130..2575d1085ae0ec 100644 --- a/src/coreclr/src/vm/threadsuspend.cpp +++ b/src/coreclr/src/vm/threadsuspend.cpp @@ -6327,7 +6327,6 @@ void HandleGCSuspensionForInterruptedThread(CONTEXT *interruptedContext) // If the thread is at a GC safe point, push a RedirectedThreadFrame with // the interrupted context and pulse the GC mode so that GC can proceed. FrameWithCookie frame(interruptedContext); - pThread->SetSavedRedirectContext(NULL); frame.Push(pThread);