EE Suspension on x86 should use RtlRestoreContext when available #65878

VSadov · 2022-02-25T04:55:56Z

More reliable fix for: #65292

Check dynamically for RtlRestoreContext on x86 and if available (i.e. on Win11), use the same codepath as on x64.
Otherwise fallback to preexisting mechanism where a context is patched in an exception filter.

src/coreclr/vm/threadsuspend.cpp

jkotas · 2022-02-25T16:01:55Z

src/coreclr/vm/threads.h

You should not need __stdcall here

VSadov · 2022-02-25T23:51:09Z

@tommcdon @hoyosjs - this change may indirectly affect ThreadAbort scenario on Win11/x86.
In some cases it will start using the same code as on x64, so it is not something entirely new, but it is a change. I do not think we have a lot of coverage for thread abort scenario here.

Is this something that may need extra testing on the debugger side?

VSadov · 2022-03-01T22:42:06Z

I have run the diagnostics tests with this change a few times (on Windows11 with x86 configuration). I did not notice any differences from the baseline run without changes.

I think we can assume the change does not break/regress anything in the debugger scenarios.

VSadov · 2022-03-01T23:11:48Z

rebased onto recent main to force stuck tests to run.

jkotas · 2022-03-01T23:31:37Z

src/coreclr/vm/threadsuspend.cpp

-        RaiseException(EXCEPTION_HIJACK, 0, 0, NULL);
+    if (!pfnRtlRestoreContext)
+    {
+        RestoreContextSimulated(pThread, pCtx, &frame);


Do we have a problem on this path with preserving last error?

It looks like we did not try to restore last error on x86 even before this change.
I am actually not sure when the last error could be lost, assuming nothing fails.

I can move the last error restore to the point right after the GCX_PREEMP/GCX_PREEMP_END, but do we need that at all?

For example, I think STRESS_LOG1 may modify the last error in some cases.

I think it would be best to restore the last error right before returning EXCEPTION_CONTINUE_EXECUTION in the filter.

Yeah. It is not too hard to restore right before it continues.

Also the docs on GetLastError say "some functions set the last-error code to 0 on success and others do not."

jkotas · 2022-03-01T23:32:38Z

src/coreclr/vm/threadsuspend.cpp

+    else
+#endif // TARGET_X86
+    {


Suggested change

else

#endif // TARGET_X86

{

#endif // TARGET_X86

Nit: I would unindent the rest to make this easier to follow

Right. Since it is unreachable.

jkotas

LGTM otherwise. Thanks!

src/coreclr/vm/threads.h

VSadov · 2022-03-02T04:21:41Z

Thanks!!

tommcdon · 2022-03-02T17:30:37Z

@tommcdon @hoyosjs - this change may indirectly affect ThreadAbort scenario on Win11/x86. In some cases it will start using the same code as on x64, so it is not something entirely new, but it is a change. I do not think we have a lot of coverage for thread abort scenario here.

Is this something that may need extra testing on the debugger side?

Thanks for letting us know @VSadov! We will pay special attention to Debugger::FuncEvalAbort scenarios.

…net#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]>

…#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]>

ghost assigned VSadov Feb 25, 2022

ghost added the area-VM-coreclr label Feb 25, 2022

jkotas reviewed Feb 25, 2022

View reviewed changes

src/coreclr/vm/threadsuspend.cpp Outdated Show resolved Hide resolved

jkotas reviewed Feb 25, 2022

View reviewed changes

src/coreclr/vm/threads.h Outdated

Copy link

Member

jkotas Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not need __stdcall here

VSadov marked this pull request as ready for review February 25, 2022 23:54

VSadov force-pushed the x86ctx branch from a387f23 to 6c58f37 Compare March 1, 2022 17:35

VSadov requested review from jkotas and mangod9 March 1, 2022 22:44

VSadov added 6 commits March 1, 2022 15:10

RestoreContextSimulated

752091c

probe for RtlRestoreContext

7fbba05

ntdll.dll

50c5ed6

restore self-trap sequence

b176a8e

PR feedback

de51359

Clarify CopyContext in RedirectedHandledJITCaseExceptionFilter

d4e1f52

VSadov force-pushed the x86ctx branch from 6c58f37 to d4e1f52 Compare March 1, 2022 23:10

jkotas reviewed Mar 1, 2022

View reviewed changes

jkotas approved these changes Mar 1, 2022

View reviewed changes

VSadov added 2 commits March 1, 2022 16:23

simpler indentation.

4e4245d

restore last error on the legacy path.

6fcc73e

danmoseley reviewed Mar 2, 2022

View reviewed changes

src/coreclr/vm/threads.h Outdated Show resolved Hide resolved

Update src/coreclr/vm/threads.h

bfee187

VSadov merged commit defd20c into dotnet:main Mar 2, 2022

VSadov deleted the x86ctx branch March 2, 2022 04:22

ghost locked as resolved and limited conversation to collaborators Apr 2, 2022

EE Suspension on x86 should use RtlRestoreContext when available #65878

EE Suspension on x86 should use RtlRestoreContext when available #65878

Uh oh!

Conversation

VSadov commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Mar 1, 2022

Uh oh!

VSadov commented Mar 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VSadov commented Mar 2, 2022

Uh oh!

tommcdon commented Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

VSadov commented Feb 25, 2022 •

edited

Loading

VSadov commented Feb 25, 2022 •

edited

Loading