Improve the handling of SVE state as part of threadsuspend #105059
Conversation
|
|
||
| #define XSTATE_MASK_SVE (UI64(1) << (XSTATE_SVE)) | ||
|
|
||
| #define XSTATE_ARM64_SVE_BIT (2) |
There was a problem hiding this comment.
The general CONTEXT struct as is currently used by PAL is supposed to mirror the win32 layout/defines so they can translate 1-to-1
The win32 naming convention here is then XSTATE_ARM64_SVE and it is using bit 2, not bit 1
There was a problem hiding this comment.
The win32 naming convention here is then XSTATE_ARM64_SVE and it is using bit 2, not bit 1
So wrong bit was set for linux in #103801 as well?
There was a problem hiding this comment.
Right.
I don't think it would technically cause problems because we're just emulating the CONTEXT struct and it doesn't actually have to match the Win32 struct 1-to-1 (since its only going to be other pal APIs consuming it)
But it's overall better/simpler to ensure they match so we don't end up with potential conflicts or other issues.
The best option here would be to just consume the native context_t struct from Unix directly (and not do any of this shimming), but that's a more involved PR.
There was a problem hiding this comment.
it doesn't actually have to match the Win32 struct 1-to-1
In fact, it does not match the Win32 struct 1-to-1: #103801 (comment)
The best option here would be to just consume the native context_t struct from Unix directly (and not do any of this shimming), but that's a more involved PR.
+1. The Windows CONTEXT structure is very entrenched throughout CoreCLR VM.
| #define XSTATE_SVE (0) | ||
|
|
||
| #define XSTATE_MASK_SVE (UI64(1) << (XSTATE_SVE)) | ||
| #define XSTATE_ARM64_SVE (2) |
There was a problem hiding this comment.
I think there might need to be an additional set of changes in vm/arm64/asmconstants.h as there is currently a SIZEOF__CONTEXT struct that exists and which is meant to represent the size of the struct on Win32 vs UNIX and I believe it is currently incorrect for Unix (or may need additional handling to ensure its large enough to hold the additional SVE state based on the vector length).
There was a problem hiding this comment.
That shouldn't be related to the Windows handling being added, however, so I've left it to a future PR
There was a problem hiding this comment.
I am not sure how it could be incorrect when there is an assert that it matches the sizeof (T_CONTEXT) in the asmconstants.h.
There was a problem hiding this comment.
The consideration is that SVE registers don’t have a fixed width and so the current logic asserting the size statically is already wrong (it’s assuming SVE registers are exactly 128-bits, when they can be up to 2048 bits)
There was a problem hiding this comment.
That's true, but the constant is to represent the size of the CONTEXT structure and that's fixed for now, so it is correct. I believe that when we'll be adding support for longer SVE registers, we should finally move to the Windows way of separate context and extended context, so the SIZEOF__CONTEXT would still be constant - and it will actually match Windows one after that.
…le on v10.0.26100.0
….0.26100.0 only defines
|
There's some handling under https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/excep.h#L772-L777 for AMD64 where it looks to strip Also some handling in src/coreclr/debug/daccess/dacdbiimplstackwalk.cpp and src/coreclr/debug/inc/common.h that likely needs to be covered for the debugger to work. The Unix handling does need more fixups as well but I think that can wait for a separate PR. This includes ensuring it handles the full register width, that it correctly checks |
| #endif // CONTEXT_ARM64_XSTATE | ||
|
|
||
| #ifndef CONTEXT_XSTATE | ||
| #define CONTEXT_XSTATE CONTEXT_ARM64_XSTATE |
There was a problem hiding this comment.
Windows doesn't define CONTEXT_XSTATE for arm64, it only defines CONTEXT_ARM64_XSTATE. So this is primarily to avoid a lot of other ifdefs elsewhere in the file.
|
@MichalStrehovsky, could you review this PR before the preview 7 snap, 7/22 4pm PST? |
|
cc @kunalspathak for a code review. |
|
A nit - the title of this PR says it is ensuring saving SVE state on Windows, but it also modifies quite a number of Unix only files. It would be nice to rename it. |
tannergooding
changed the title
|
I believe this is just waiting on review from @MichalStrehovsky and otherwise should be good to merge |
Not sure if @MichalStrehovsky might be around until tomorrow. @JulieLeeMSFT - since we already got 2 signoffs, can we merge this in? |
|
Yes, let's merge in. |
|
This missed the snap and will need to be backported, so it will need to go through tactics for approval. |
|
Missed by a minute or so. Please go ahead with a tactics approval. |
|
/backport to release/9.0-preview7 |
|
Started backporting to release/9.0-preview7: https://github.com/dotnet/runtime/actions/runs/10051581416 |
I know next to nothing about thread suspension, @VSadov FYI |
This does not validate that all the handling in
pal/context.cppis correct. There's some known places it isn't handling full width registers and where it may be inconsistent with the Win32 CONTEXT layout.