Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/9.0] Fix FP state restore on macOS exception forwarding #110163

Merged
merged 5 commits into from
Dec 10, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 25, 2024

Backport of #109458 to release/9.0

/cc @janvorli

Customer Impact

[x] Customer reported
[ ] Found internally

Original issue: #109423
Attempt to load Java VM into .NET process on macOS x64 always crashes due to a problem in hardware exception forwarding from coreclr's exception handling port to Java's exception handling port (or any other registered one).

Regression

[x] Yes
[ ] No

Regression in .NET 9 Preview 6 due to a change to fix AVX512 support on macOS for debugging.

Testing

Testing using a repro provided by the customer and coreclr tests

Risk

Low, fixes an obviously incorrect floating point context restoration in case of hardware exception in external non-.NET code.

dotnet-maestro bot and others added 3 commits October 28, 2024 18:57
…28.2 (#109323)

Microsoft.SourceBuild.Intermediate.emsdk , Microsoft.NET.Workload.Emscripten.Current.Manifest-9.0.100 , Microsoft.NET.Workload.Emscripten.Current.Manifest-9.0.100.Transport
 From Version 9.0.0-rtm.24519.2 -> To Version 9.0.0-rtm.24528.2

Dependency coherency updates

runtime.linux-arm64.Microsoft.NETCore.Runtime.JIT.Tools,runtime.linux-x64.Microsoft.NETCore.Runtime.JIT.Tools,runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.JIT.Tools,runtime.linux-musl-x64.Microsoft.NETCore.Runtime.JIT.Tools,runtime.win-arm64.Microsoft.NETCore.Runtime.JIT.Tools,runtime.win-x64.Microsoft.NETCore.Runtime.JIT.Tools,runtime.osx-arm64.Microsoft.NETCore.Runtime.JIT.Tools,runtime.osx-x64.Microsoft.NETCore.Runtime.JIT.Tools,runtime.linux-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk,runtime.linux-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools,runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk,runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools,runtime.linux-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk,runtime.linux-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools,runtime.linux-musl-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk,runtime.linux-musl-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools,runtime.win-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk,runtime.win-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools,runtime.osx-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk,runtime.osx-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools,runtime.osx-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk,runtime.osx-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools
 From Version 19.1.0-alpha.1.24510.5 -> To Version 19.1.0-alpha.1.24519.2 (parent: Microsoft.NET.Workload.Emscripten.Current.Manifest-9.0.100.Transport

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
The change that enabled AVX512 support in the past has introduced a subtle
issue in restoring context for forwarding hardware exceptions that occur
in 3rd party non-managed code. In that case, the restored floating point
state is garbled.

The problem is due to the fact that we pass a x86_avx512_state context to
the thread_set_state. That context contains a header field describing the
format of the context (it can be AVX, AVX512, 32 or 64 bit, ...). That is
then followed by the actual context structure. This style of context is
identified e.g. by x86_AVX_STATE flavor. The header field contains the
specific flavor, which would be x86_AVX_STATE64 or x86_AVX512_STATE64.
The thread_set_state uses the flavor to detect whether the context passed
to it is this combined one or just x86_AVX_STATE64 or x86_AVX512_STATE64
which doesn't have the header field.
The issue was that while we were passing in the combined context, we were
passing in the flavor extracted from its header. So the thread_set_state
used the header as part of the context. That resulted e.g. in xmm register
contents being shifted by 8 bytes, thus garbling the state.
@janvorli janvorli self-assigned this Nov 25, 2024
@janvorli janvorli added this to the 9.0.x milestone Nov 25, 2024
@janvorli janvorli added the Servicing-consider Issue for next servicing release review label Nov 25, 2024
@janvorli janvorli requested a review from VSadov November 25, 2024 20:50
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

lgtm. please get a code review. we will take for consideration in 9.0.x

@VSadov
Copy link
Member

VSadov commented Nov 25, 2024

LGTM. Thanks!

@janvorli janvorli changed the base branch from release/9.0 to release/9.0-staging December 3, 2024 20:06
@janvorli
Copy link
Member

janvorli commented Dec 4, 2024

There are three failing tests on ios, the failure in all cases is unrelated to this change:

Failed to install the application
                 App is not signed

It is #110395

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Dec 4, 2024
@jeffschwMSFT jeffschwMSFT modified the milestones: 9.0.x, 9.0.2 Dec 4, 2024
@janvorli janvorli merged commit 83db46b into release/9.0-staging Dec 10, 2024
91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants