From 0ccc1c08f43d47439d3b9e4785e025b17185fcdb Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 23 Feb 2022 19:03:01 -0800 Subject: [PATCH 1/6] Do not copy XState other than AVX --- src/coreclr/vm/threadsuspend.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 3c6f46b4932bbe..4afcb4557507c3 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -3029,9 +3029,22 @@ BOOL Thread::RedirectCurrentThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt, CONT ////////////////////////////////////// // Get and save the thread's context - BOOL success = CopyContext(pCtx, pCtx->ContextFlags, pCurrentThreadCtx); + + // this method is called for GC stress in managed code. + // if current context has XState features, we are only interested in AVX + DWORD64 srcFeatures = 0; + BOOL success = GetXStateFeaturesMask(pCurrentThreadCtx, &srcFeatures); + if (srcFeatures & XSTATE_MASK_AVX) + { + success &= SetXStateFeaturesMask(pCurrentThreadCtx, XSTATE_MASK_AVX); + } + + success &= CopyContext(pCtx, pCtx->ContextFlags, pCurrentThreadCtx); _ASSERTE(success); + if (!success) + return FALSE; + // Ensure that this flag is set for the next time through the normal path, // RedirectThreadAtHandledJITCase. pCtx->ContextFlags |= CONTEXT_EXCEPTION_REQUEST; From 95204eaf8cacaca7d1640fd6305d768a055c8a3f Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 23 Feb 2022 19:43:31 -0800 Subject: [PATCH 2/6] #if defined(TARGET_X86) || defined(TARGET_AMD64) --- src/coreclr/vm/threadsuspend.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 4afcb4557507c3..d9866245424401 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -3029,15 +3029,18 @@ BOOL Thread::RedirectCurrentThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt, CONT ////////////////////////////////////// // Get and save the thread's context + BOOL success = true; +#if defined(TARGET_X86) || defined(TARGET_AMD64) // this method is called for GC stress in managed code. // if current context has XState features, we are only interested in AVX DWORD64 srcFeatures = 0; - BOOL success = GetXStateFeaturesMask(pCurrentThreadCtx, &srcFeatures); + success = GetXStateFeaturesMask(pCurrentThreadCtx, &srcFeatures); if (srcFeatures & XSTATE_MASK_AVX) { success &= SetXStateFeaturesMask(pCurrentThreadCtx, XSTATE_MASK_AVX); } +#endif //defined(TARGET_X86) || defined(TARGET_AMD64) success &= CopyContext(pCtx, pCtx->ContextFlags, pCurrentThreadCtx); _ASSERTE(success); From 7951248f6617958441d4c3e3baaa1f08b4a8b5a2 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 24 Feb 2022 06:57:55 -0800 Subject: [PATCH 3/6] mask XState unconditionally --- src/coreclr/vm/threadsuspend.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index d9866245424401..80eadb7a34dcb2 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -3035,11 +3035,8 @@ BOOL Thread::RedirectCurrentThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt, CONT // this method is called for GC stress in managed code. // if current context has XState features, we are only interested in AVX DWORD64 srcFeatures = 0; - success = GetXStateFeaturesMask(pCurrentThreadCtx, &srcFeatures); - if (srcFeatures & XSTATE_MASK_AVX) - { - success &= SetXStateFeaturesMask(pCurrentThreadCtx, XSTATE_MASK_AVX); - } + success &= GetXStateFeaturesMask(pCurrentThreadCtx, &srcFeatures); + success &= SetXStateFeaturesMask(pCurrentThreadCtx, srcFeatures & XSTATE_MASK_AVX); #endif //defined(TARGET_X86) || defined(TARGET_AMD64) success &= CopyContext(pCtx, pCtx->ContextFlags, pCurrentThreadCtx); From d531d3df37c6a5504ec156a692b8b190336a4881 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 24 Feb 2022 09:22:40 -0800 Subject: [PATCH 4/6] Ensure XSTATE_MASK_AVX is set before calling EEGetThreadContext --- src/coreclr/vm/threadsuspend.cpp | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 80eadb7a34dcb2..3252b3efc236c3 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -1997,15 +1997,6 @@ CONTEXT* AllocateOSContextHelper(BYTE** contextBuffer) 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; @@ -2912,9 +2903,23 @@ BOOL Thread::RedirectThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt) ////////////////////////////////////// // Get and save the thread's context + BOOL bRes = true; // Always get complete context, pCtx->ContextFlags are set during Initialization - BOOL bRes = EEGetThreadContext(this, pCtx); + +#if defined(TARGET_X86) || defined(TARGET_AMD64) + // Scenarios like GC stress may indirectly disable XState features in the pCtx + // depending on the state at the time of GC stress interrupt. + // + // Make sure that AVX feature mask is set, if supported. + // + // This should not normally fail. + // The system silently ignores any feature specified in the FeatureMask + // which is not enabled on the processor. + bRes &= SetXStateFeaturesMask(pCtx, XSTATE_MASK_AVX); +#endif //defined(TARGET_X86) || defined(TARGET_AMD64) + + bRes &= EEGetThreadContext(this, pCtx); _ASSERTE(bRes && "Failed to GetThreadContext in RedirectThreadAtHandledJITCase - aborting redirect."); if (!bRes) @@ -3032,8 +3037,11 @@ BOOL Thread::RedirectCurrentThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt, CONT BOOL success = true; #if defined(TARGET_X86) || defined(TARGET_AMD64) - // this method is called for GC stress in managed code. - // if current context has XState features, we are only interested in AVX + // This method is called for GC stress interrupts in managed code. + // The current context may have various XState features, depending on what is used/dirty, + // but only AVX feature may contain live data. (that could change in the future) + // Besides pCtx may not have space to store other features. + // So we will mask out everything but AVX. DWORD64 srcFeatures = 0; success &= GetXStateFeaturesMask(pCurrentThreadCtx, &srcFeatures); success &= SetXStateFeaturesMask(pCurrentThreadCtx, srcFeatures & XSTATE_MASK_AVX); From c552ae206699ae74451f61d534fd8491d2b9143d Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 24 Feb 2022 12:48:48 -0800 Subject: [PATCH 5/6] redundant supportsAVX, more clear comment --- src/coreclr/vm/threadsuspend.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 3252b3efc236c3..0b10ee1a0eeb74 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -1960,7 +1960,6 @@ CONTEXT* AllocateOSContextHelper(BYTE** contextBuffer) #if !defined(TARGET_UNIX) && (defined(TARGET_X86) || defined(TARGET_AMD64)) DWORD context = CONTEXT_COMPLETE; - BOOL supportsAVX = FALSE; if (pfnInitializeContext2 == NULL) { @@ -1974,7 +1973,6 @@ CONTEXT* AllocateOSContextHelper(BYTE** contextBuffer) if ((FeatureMask & XSTATE_MASK_AVX) != 0) { context = context | CONTEXT_XSTATE; - supportsAVX = TRUE; } // Retrieve contextSize by passing NULL for Buffer @@ -3039,7 +3037,7 @@ BOOL Thread::RedirectCurrentThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt, CONT #if defined(TARGET_X86) || defined(TARGET_AMD64) // This method is called for GC stress interrupts in managed code. // The current context may have various XState features, depending on what is used/dirty, - // but only AVX feature may contain live data. (that could change in the future) + // but only AVX feature may contain live data. (that could change with new features in JIT) // Besides pCtx may not have space to store other features. // So we will mask out everything but AVX. DWORD64 srcFeatures = 0; From ef7bcdd0d3f6a89e8b1d213150bc63cd44955c44 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 24 Feb 2022 15:10:03 -0800 Subject: [PATCH 6/6] PR feedback --- src/coreclr/vm/threadsuspend.cpp | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 0b10ee1a0eeb74..61dbae0ca95603 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -3032,7 +3032,7 @@ BOOL Thread::RedirectCurrentThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt, CONT ////////////////////////////////////// // Get and save the thread's context - BOOL success = true; + BOOL success = TRUE; #if defined(TARGET_X86) || defined(TARGET_AMD64) // This method is called for GC stress interrupts in managed code. @@ -3041,13 +3041,24 @@ BOOL Thread::RedirectCurrentThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt, CONT // Besides pCtx may not have space to store other features. // So we will mask out everything but AVX. DWORD64 srcFeatures = 0; - success &= GetXStateFeaturesMask(pCurrentThreadCtx, &srcFeatures); - success &= SetXStateFeaturesMask(pCurrentThreadCtx, srcFeatures & XSTATE_MASK_AVX); + success = GetXStateFeaturesMask(pCurrentThreadCtx, &srcFeatures); + _ASSERTE(success); + if (!success) + return FALSE; + + // Get may return 0 if no XState is set, which Set would not accept. + if (srcFeatures != 0) + { + success = SetXStateFeaturesMask(pCurrentThreadCtx, srcFeatures & XSTATE_MASK_AVX); + _ASSERTE(success); + if (!success) + return FALSE; + } + #endif //defined(TARGET_X86) || defined(TARGET_AMD64) - success &= CopyContext(pCtx, pCtx->ContextFlags, pCurrentThreadCtx); + success = CopyContext(pCtx, pCtx->ContextFlags, pCurrentThreadCtx); _ASSERTE(success); - if (!success) return FALSE;