Use CopyContext to restore saved context on X86 #65490

VSadov · 2022-02-17T06:37:38Z

jkotas · 2022-02-17T07:18:25Z

Any chance we can switch to RtlRestoreContext on x86 and make it more similar to how other platforms work?

adamsitnik · 2022-02-17T09:50:14Z

@VSadov I have built your PR and I can't repro the issue anymore (AMD, x86, Win11) 👍

filipnavara · 2022-02-17T11:15:35Z

I've backported the fix on top of release/6.0 branch and built it locally. I can also confirm that the issue no longer reproduces.

janvorli · 2022-02-17T12:27:03Z

src/coreclr/vm/threadsuspend.cpp

+    //
+    // REVIEW: CopyContext may fail. in theory. How should we handle that? FailFast?
+    CONTEXT* pTarget = pExcepPtrs->ContextRecord;
+    CopyContext(pTarget, pTarget->ContextFlags, pCtx);


It seems it would make sense to modify the ReplaceExceptionContextRecord to use CopyContext instead.

I was not sure if ReplaceExceptionContextRecord can be used outside of Windows. Can it?

It's not readily obvious but it's always guarded by HOST_WINDOWS and/or !TARGET_UNIX so it looks Windows-only. CopyContext is available on Windows 7 SP1+ which is the minimum target at the moment so that should be fine too.

I also wonder if ReplaceExceptionContextRecord needs to handle extended context at all if suspension resume switches completely to the RtlRestoreContext plan.

I was not sure if ReplaceExceptionContextRecord can be used outside of Windows. Can it?

I have tried moving the fix into ReplaceExceptionContextRecord and that did not build on Linux, so that answers this.

VSadov · 2022-02-17T17:17:35Z

Any chance we can switch to RtlRestoreContext on x86 and make it more similar to how other platforms work?

I think it is worth trying. The reason for current approach could be compatibility with no longer interesting platforms. (Win95?)

Edit:
The actual reason is that RtlRestoreContext became available on x86 relatively recently and I am not yet sure how far back the support goes - i.e. is it available on Windows7 SP1 ?

filipnavara · 2022-02-17T17:50:55Z

Any chance we can switch to RtlRestoreContext on x86 and make it more similar to how other platforms work?

Seems reasonable going forward but I assume that this will need servicing for 6.0 and it looks like too big of a change for backporting?

VSadov · 2022-02-17T22:43:01Z

I have looked into unifying on use of RtlRestoreContext and it is promising, but not completely straightforward.

The level of API that we are targeting does not include RtlRestoreContext on x86. We would have to dynamically query/bind to it and use if present. We may also have to keep the old codepath, unless we find some other pattern that is guaranteed to work on all platforms that we support.

I think we might want to take the current fix first though - to unblock clean CI, since the issue causes random failures.

VSadov · 2022-02-17T23:25:57Z

CC: @davidwrighton

davidwrighton · 2022-02-18T01:34:21Z

src/coreclr/vm/threadsuspend.cpp

+    //     these contexts may contain extended registers and may have different format
+    //     for reasons such as alignment or context compaction
+    //
+    // REVIEW: CopyContext may fail. in theory. How should we handle that? FailFast?


Yes, FailFast is appropriate for failure here.

However, be careful calling new functions from this function. If they have destructors and the compiler chooses to inline them, the runtime will start crashing.

I was thinking of code like:

if (!CopyContext(pTarget, pTarget->ContextFlags, pCtx)) { STRESS_LOG1(LF_SYNC, LL_ERROR, "ERROR: Could not set context record, lastError = 0x%x\n", GetLastError()); ThrowLastError(); }

Would that fit here?

It is highly unlikely that copy would fail, but it'd be nice to have some diagnostics if that start happenning.

davidwrighton

As a quick fix to unblock testing, I approve of this, but I'm not confident this is the right long term fix. IMO, we should change to RtlRestoreContext on operating systems that support it, and keep the existing horrifying logic for the older OS. Then of course, once we have that, we should backport the fix to servicing.

davidwrighton · 2022-02-18T02:24:49Z

PR feedback update looks good to me.

jkotas · 2022-02-18T05:00:55Z

src/coreclr/vm/threadsuspend.cpp

+    if (!CopyContext(pTarget, pTarget->ContextFlags, pCtx))
+    {
+        STRESS_LOG1(LF_SYNC, LL_ERROR, "ERROR: Could not set context record, lastError = 0x%x\n", GetLastError());
+        ThrowLastError();


Suggested change

ThrowLastError();

EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE);

The caller does not expect exceptions. This needs to be failfast. Throwing exception is likely going to lead to hard to diagnose crash otherwise.

jkotas · 2022-02-18T05:02:04Z

src/coreclr/vm/threadsuspend.cpp

@@ -2509,6 +2509,18 @@ void RedirectedThreadFrame::ExceptionUnwind()
 #ifndef TARGET_UNIX

 #ifdef TARGET_X86
+
+// a helper to make sure these calls are not inlined into a filter function.
+// in particular we do not want a possibility of inlined destructor calls.


This should not be a concern once you change it to EEPOLICY_HANDLE_FATAL_ERROR. EEPOLICY_HANDLE_FATAL_ERROR is non-inlineable.

jkotas · 2022-02-18T05:03:40Z

As a quick fix to unblock testing, I approve of this, but I'm not confident this is the right long term fix.

+1. It is likely that the OS treatment of context will evolving over time that is going to break the exception filter hack again.

VSadov · 2022-02-18T05:06:07Z

It is likely that the OS treatment of context will evolving over time that is going to break the exception filter hack again.

Agree on this as well.

VSadov · 2022-02-18T07:07:48Z

Thanks!!!

* Use CopyContext to restore saved context on X86 * PR feedback * more 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 <danmose@microsoft.com> Co-authored-by: Dan Moseley <danmose@microsoft.com>

* 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 <danmose@microsoft.com> Co-authored-by: Vladimir Sadov <vsadov@microsoft.com> Co-authored-by: Dan Moseley <danmose@microsoft.com>

Use CopyContext to restore saved context on X86

c6156f4

ghost assigned VSadov Feb 17, 2022

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

janvorli reviewed Feb 17, 2022

View reviewed changes

VSadov marked this pull request as ready for review February 17, 2022 22:47

VSadov force-pushed the suspX86 branch from 06a249c to c6156f4 Compare February 17, 2022 23:13

davidwrighton reviewed Feb 18, 2022

View reviewed changes

davidwrighton approved these changes Feb 18, 2022

View reviewed changes

PR feedback

0e124a3

jkotas reviewed Feb 18, 2022

View reviewed changes

more PR feedback

bae2f45

VSadov merged commit 26a1c7e into dotnet:main Feb 18, 2022

VSadov deleted the suspX86 branch February 18, 2022 07:09

VSadov added a commit to VSadov/runtime that referenced this pull request Mar 2, 2022

Use CopyContext to restore saved context on X86 (dotnet#65490)

9933a8b

* Use CopyContext to restore saved context on X86 * PR feedback * more PR feedback

agocke pushed a commit to agocke/runtime that referenced this pull request Mar 8, 2022

Use CopyContext to restore saved context on X86 (dotnet#65490)

35932d9

* Use CopyContext to restore saved context on X86 * PR feedback * more PR feedback

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CopyContext to restore saved context on X86 #65490

Use CopyContext to restore saved context on X86 #65490

VSadov commented Feb 17, 2022 •

edited

Loading

jkotas commented Feb 17, 2022

adamsitnik commented Feb 17, 2022

filipnavara commented Feb 17, 2022

janvorli Feb 17, 2022

VSadov Feb 17, 2022

filipnavara Feb 17, 2022 •

edited

Loading

VSadov Feb 17, 2022

VSadov Feb 17, 2022 •

edited

Loading

VSadov commented Feb 17, 2022 •

edited

Loading

filipnavara commented Feb 17, 2022

VSadov commented Feb 17, 2022

VSadov commented Feb 17, 2022

davidwrighton Feb 18, 2022

davidwrighton Feb 18, 2022

VSadov Feb 18, 2022 •

edited

Loading

davidwrighton left a comment

davidwrighton commented Feb 18, 2022

jkotas Feb 18, 2022

jkotas Feb 18, 2022

jkotas commented Feb 18, 2022

VSadov commented Feb 18, 2022

VSadov commented Feb 18, 2022

	ThrowLastError();
	EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE);

Use CopyContext to restore saved context on X86 #65490

Use CopyContext to restore saved context on X86 #65490

Conversation

VSadov commented Feb 17, 2022 • edited Loading

jkotas commented Feb 17, 2022

adamsitnik commented Feb 17, 2022

filipnavara commented Feb 17, 2022

janvorli Feb 17, 2022

Choose a reason for hiding this comment

VSadov Feb 17, 2022

Choose a reason for hiding this comment

filipnavara Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

VSadov Feb 17, 2022

Choose a reason for hiding this comment

VSadov Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

VSadov commented Feb 17, 2022 • edited Loading

filipnavara commented Feb 17, 2022

VSadov commented Feb 17, 2022

VSadov commented Feb 17, 2022

davidwrighton Feb 18, 2022

Choose a reason for hiding this comment

davidwrighton Feb 18, 2022

Choose a reason for hiding this comment

VSadov Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

davidwrighton left a comment

Choose a reason for hiding this comment

davidwrighton commented Feb 18, 2022

jkotas Feb 18, 2022

Choose a reason for hiding this comment

jkotas Feb 18, 2022

Choose a reason for hiding this comment

jkotas commented Feb 18, 2022

VSadov commented Feb 18, 2022

VSadov commented Feb 18, 2022

VSadov commented Feb 17, 2022 •

edited

Loading

filipnavara Feb 17, 2022 •

edited

Loading

VSadov Feb 17, 2022 •

edited

Loading

VSadov commented Feb 17, 2022 •

edited

Loading

VSadov Feb 18, 2022 •

edited

Loading