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

[mono] Fix SwiftError variable offset calculation #103043

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

kotlarmilos
Copy link
Member

Description

This PR attempts to fix CryptoKit test failure encountered in #102583. The current approach always allocates a local var for SwiftSelf and doesn't set correct offset when the argument is passed via the stack to the wrapper.

The idea is to allocate a new stack var for SwiftError only when it is passed to the wrapper via regs. If SwiftError is passed to the wrapper via the stack, the existing stack var could be used, instead of a local variable.

In the wrapper prologue, if SwiftError is passed to the wrapper via regs, its value should be preserved in a local variable.

This is determined by checking ainfo->offset which is set in get_call_info. On amd64, if SwiftError is passed via regs, inreg is set to FALSE and is handled in the common block for all reg arguments. Reverse pinvoke wrappers should always allocate the local variable, as the pinvoke flag in get_call_info is 1, whereas for direct pinvoke, it is 0.

Validation

These changes should be verified using the Interop/Swift runtime tests and the Security.Cryptography library tests.

@kotlarmilos kotlarmilos added this to the 9.0.0 milestone Jun 4, 2024
@kotlarmilos kotlarmilos self-assigned this Jun 4, 2024
@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link
Contributor

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

@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

@matouskozak @jkurdek Please take a look at these changes.

Copy link
Member

@matouskozak matouskozak left a comment

Choose a reason for hiding this comment

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

The changes LGTM! and overall the code looks much cleaner to me know.

Copy link
Member

@jkurdek jkurdek left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@matouskozak matouskozak merged commit 193b826 into dotnet:main Jun 17, 2024
152 of 160 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants