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

NativeAOT/win-x86: Enable FEATURE_EH_CALLFINALLY_THUNKS #99718

Merged
merged 3 commits into from
Mar 28, 2024

Conversation

filipnavara
Copy link
Member

Fixes #99687

Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 13, 2024
@filipnavara
Copy link
Member Author

cc @dotnet/jit-contrib

@filipnavara
Copy link
Member Author

I run into some issues that may be related to this. For example, Invariants.Tests compiled without this change passes. With this change the stack pointer is trashed early in exception handling and causes ret to end up reading wrong location from stack after the catch funclet executed. I'm investigating.

@filipnavara filipnavara marked this pull request as draft March 16, 2024 13:04
@filipnavara
Copy link
Member Author

Turns out the issue is unrelated to this change. It's simply lack of EECodeManager::GetResumeSp equivalent on the NativeAOT side (#99688 (comment)). That is a separate issue that needs to be fixed.

@filipnavara filipnavara marked this pull request as ready for review March 16, 2024 14:27
@AndyAyersMS
Copy link
Member

@BruceForstall perhaps you should take a look?

@filipnavara
Copy link
Member Author

This is hopefully the last missing piece for the win-x86 NativeAOT support.

@BruceForstall BruceForstall self-requested a review March 26, 2024 00:00
Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

Looks ok to me.

Note that it is changing exception handling behavior of the existing Linux/x86 support. We don't build or test that (that I know of), and comments here indicate Linux/x86 is already broken for other reasons, so perhaps this adds yet another thing that either improves the existing state or adds to the backlog. In any case, it makes sense that x86 with funclets and x64 with funclets are on the same plan.

@BruceForstall BruceForstall merged commit f51d705 into dotnet:main Mar 28, 2024
110 checks passed
@filipnavara filipnavara deleted the win-x86-callfinally branch March 28, 2024 22:11
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NativeAot] win-x86: Funclets getting double called
4 participants