Do not copy XState other than AVX when redirecting for GC stress #65825

VSadov · 2022-02-24T03:08:52Z

GC stress mechanism relies on exceptions thrown from random locations in managed code.

The context that OS passes for the exception location may contain whatever XState that happens to be dirty. However only AVX matters in managed code.
Besides, the redirection context that stores the state may not have space to save other XState features.

To avoid dealing with the state that we do not expect, we will indicate via the feature mask that only AVX state is valid in the provided context.

Fixes: #65776

VSadov · 2022-02-24T03:12:37Z

@janvorli - could you check that the fix actually resolves the issue? I could not reproduce it. I think the issue requires your other changes.

janvorli · 2022-02-24T09:35:29Z

It doesn't work, the CopyContext still fails with this change.

janvorli · 2022-02-24T09:48:57Z

~~It seems that the SetXStateFeatureMask is an "or" operation. I've called GetXStateFeatureMask after the SetXStateFeatureMask you've added and the mask was the same as before the set (0xA3 in my case).~~
I was wrong, the reason why it doesn't work is different. The mask that the GetXStateFeaturesMask returned - the 0xA3 - doesn't contain XSTATE_MASK_AVX. This mask contains:
XSTATE_LEGACY_FLOATING_POINT
XSTATE_LEGACY_SSE
XSTATE_AVX512_KMASK
XSTATE_AVX512_ZMM

VSadov · 2022-02-24T14:59:06Z

Ah, yes, since it is picking whatever is dirty, it is possible that there is XState, but without AVX. I need to apply the mask unconditionally.

janvorli · 2022-02-24T15:48:36Z

But what would it copy then when the source context doesn't contain AVX? And it would drop the AVX512 state possibly resulting in floating point state corruption, wouldn't it? It still seems to me that allocating the context with everything is a better way.

VSadov · 2022-02-24T17:30:51Z

Since IP is in managed code, only AVX could have live data. If that is not present, we will only copy the legacy part.

The AVX2 vs. AVX512 is confusing. I keep mixing them up. I believe managed code may use AVX2 (i.e. AVX256), but that is included in AVX feature in XState, but not AVX512.

When AVX512 is supported we will have to make the context larger and start saving/restoring AVX512 feature as well.

janvorli · 2022-02-24T18:40:31Z

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


With this removed, the code above that sets the supportAVX based on the FeatureMask can be removed too.

Right, even before supportAVX was redundant, since SetXStateFeaturesMask performs the same check - to filter out what is not supported.

janvorli · 2022-02-24T20:59:38Z

I can confirm that the current state of the change works.

janvorli · 2022-02-24T21:21:12Z

src/coreclr/vm/threadsuspend.cpp

+    // 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);


I only have one nit - I would prefer failures / asserts on each of the failure instead of combining the results together.

GetXStateFeaturesMask and SetXStateFeaturesMask basically cannot fail. They are very tolerant to the input.
To get failures you'd need something like pass context flags for a wrong architecture (like arm64). As-is that would not compile.

Realistically only CopyContext is an API with complex requirements and validation that could fail here due to some mistake.
I think in this particular case individual asserts/returns would not add a lot of value.

I've just found those would fail also when passed context without any XSTATE present. It seems like something that could possibly happen for the usage in Thread::RedirectCurrentThreadAtHandledJITCase where we pass it a general context that we get from the vectored exception handler. So it seems that it would be nice to actually make that work in that case.

Hmm, my read on that was that Set(Get) would be a noop, but I think you are right, if no XState is set it may fail. Arguably a bug in the validation of Set not accepting 0.

Yes, we should make this case work.

janvorli

LGTM, thank you!

VSadov · 2022-02-25T01:22:56Z

linux DAC build failures happen in other PRs as well.

…net#65825) * Do not copy XState other than AVX * #if defined(TARGET_X86) || defined(TARGET_AMD64) * mask XState unconditionally * Ensure XSTATE_MASK_AVX is set before calling EEGetThreadContext * redundant supportsAVX, more clear comment * PR feedback

…#66120) * Use CopyContext to restore saved context on X86 (#65490) * Use CopyContext to restore saved context on X86 * PR feedback * more PR feedback * Do not copy XState other than AVX when redirecting for GC stress (#65825) * Do not copy XState other than AVX * #if defined(TARGET_X86) || defined(TARGET_AMD64) * mask XState unconditionally * Ensure XSTATE_MASK_AVX is set before calling EEGetThreadContext * redundant supportsAVX, more clear comment * PR feedback * null-check the redirect context before using. (#65910) * null-check the redirect context before using. * tweak the comment * do not allocate context if InitializeContext has unexpected results. * EE Suspension on x86 should use RtlRestoreContext when available (#65878) * RestoreContextSimulated * probe for RtlRestoreContext * ntdll.dll * restore self-trap sequence * PR feedback * Clarify CopyContext in RedirectedHandledJITCaseExceptionFilter * simpler indentation. * restore last error on the legacy path. * Update src/coreclr/vm/threads.h Co-authored-by: Dan Moseley <[email protected]> Co-authored-by: Dan Moseley <[email protected]>

* Use CopyContext to restore saved context on X86 (#65490) * Use CopyContext to restore saved context on X86 * PR feedback * more PR feedback * Do not copy XState other than AVX when redirecting for GC stress (#65825) * Do not copy XState other than AVX * #if defined(TARGET_X86) || defined(TARGET_AMD64) * mask XState unconditionally * Ensure XSTATE_MASK_AVX is set before calling EEGetThreadContext * redundant supportsAVX, more clear comment * PR feedback * null-check the redirect context before using. (#65910) * null-check the redirect context before using. * tweak the comment * do not allocate context if InitializeContext has unexpected results. * EE Suspension on x86 should use RtlRestoreContext when available (#65878) * RestoreContextSimulated * probe for RtlRestoreContext * ntdll.dll * restore self-trap sequence * PR feedback * Clarify CopyContext in RedirectedHandledJITCaseExceptionFilter * simpler indentation. * restore last error on the legacy path. * Update src/coreclr/vm/threads.h Co-authored-by: Dan Moseley <[email protected]> Co-authored-by: Vladimir Sadov <[email protected]> Co-authored-by: Dan Moseley <[email protected]>

Do not copy XState other than AVX

0ccc1c0

dotnet-issue-labeler bot added the area-VM-coreclr label Feb 24, 2022

ghost assigned VSadov Feb 24, 2022

#if defined(TARGET_X86) || defined(TARGET_AMD64)

95204ea

mask XState unconditionally

7951248

Ensure XSTATE_MASK_AVX is set before calling EEGetThreadContext

d531d3d

janvorli reviewed Feb 24, 2022

View reviewed changes

redundant supportsAVX, more clear comment

c552ae2

janvorli approved these changes Feb 24, 2022

View reviewed changes

PR feedback

ef7bcdd

janvorli approved these changes Feb 24, 2022

View reviewed changes

VSadov merged commit 1857d04 into dotnet:main Feb 25, 2022

VSadov deleted the gcCov branch February 25, 2022 01:23

agocke mentioned this pull request Feb 28, 2022

Fatal error. Internal CLR error. (0x80131506) #65890

Closed

ghost locked as resolved and limited conversation to collaborators Mar 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not copy XState other than AVX when redirecting for GC stress #65825

Do not copy XState other than AVX when redirecting for GC stress #65825

VSadov commented Feb 24, 2022 •

edited

Loading

VSadov commented Feb 24, 2022

janvorli commented Feb 24, 2022

janvorli commented Feb 24, 2022 •

edited

Loading

VSadov commented Feb 24, 2022

janvorli commented Feb 24, 2022

VSadov commented Feb 24, 2022 •

edited

Loading

janvorli Feb 24, 2022

VSadov Feb 24, 2022

janvorli commented Feb 24, 2022

janvorli Feb 24, 2022

VSadov Feb 24, 2022 •

edited

Loading

janvorli Feb 24, 2022

VSadov Feb 24, 2022

janvorli left a comment

VSadov commented Feb 25, 2022

Do not copy XState other than AVX when redirecting for GC stress #65825

Do not copy XState other than AVX when redirecting for GC stress #65825

Conversation

VSadov commented Feb 24, 2022 • edited Loading

VSadov commented Feb 24, 2022

janvorli commented Feb 24, 2022

janvorli commented Feb 24, 2022 • edited Loading

VSadov commented Feb 24, 2022

janvorli commented Feb 24, 2022

VSadov commented Feb 24, 2022 • edited Loading

janvorli Feb 24, 2022

Choose a reason for hiding this comment

VSadov Feb 24, 2022

Choose a reason for hiding this comment

janvorli commented Feb 24, 2022

janvorli Feb 24, 2022

Choose a reason for hiding this comment

VSadov Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

janvorli Feb 24, 2022

Choose a reason for hiding this comment

VSadov Feb 24, 2022

Choose a reason for hiding this comment

janvorli left a comment

Choose a reason for hiding this comment

VSadov commented Feb 25, 2022

VSadov commented Feb 24, 2022 •

edited

Loading

janvorli commented Feb 24, 2022 •

edited

Loading

VSadov commented Feb 24, 2022 •

edited

Loading

VSadov Feb 24, 2022 •

edited

Loading