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/8.0] [mono] Extend mono_gsharedvt_constrained_call JIT icall to handle static virtual methods #91059

Merged
merged 5 commits into from
Aug 29, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 24, 2023

Description

Backport of #90875 to release/8.0

This PR extends mono_gsharedvt_constrained_call JIT icall to handle static virtual methods. Prior to this change, when a JIT icall is made from gsharedvt with a constrained static virtual method, the this is not handled properly, resulting with SIGSEGV. This PR adds support for handling static virtual methods, making this argument NULL.

Customer Impact

The issue was discovered by a customer using an iOS Xamarin app. The fix should help resolving #90732.

Testing

Manual testing was performed, ensuring that the JIT icalls can be made from gsharedvt with a constrained static virtual method.

Risk

Low risk. This change extends functionality of the mono_gsharedvt_constrained_call JIT icall to handle static virtual methods.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

The change doesn't affect code that ships in a NuGet package.

@kotlarmilos
Copy link
Member

Before merging, we should verify if it resolves the customer issue reported in #90732.

@kotlarmilos kotlarmilos added this to the 8.0.0 milestone Aug 24, 2023
@kotlarmilos kotlarmilos self-assigned this Aug 24, 2023
@ghost
Copy link

ghost commented Aug 24, 2023

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #90875 to release/8.0

/cc @kotlarmilos

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-Codegen-AOT-mono, os-ios

Milestone: -

@carlossanlop carlossanlop added Servicing-consider Issue for next servicing release review blocked Issue/PR is blocked on something - see comments labels Aug 24, 2023
@carlossanlop
Copy link
Member

Before merging, we should verify if it resolves the customer issue reported in #90732.

Thanks for the heads-up. I added the blocked label until we get that confirmation.

@SamMonoRT @marek-safar in the meanwhile, do you approve of this change?

@SamMonoRT
Copy link
Member

Requesting a review from @lambdageek too (should be on Monday though).

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Aug 28, 2023
@carlossanlop
Copy link
Member

Signed-off by both @lambdageek and @marek-safar .

There are two mono failures. Are they related to this PR or can I merge, @kotlarmilos / @SamMonoRT ?

@SamMonoRT
Copy link
Member

please don't merge today. CI failures are unrelated, but validating manual testing to be sure of fix to customer issue. I'll message once done tomorrow am

@carlossanlop
Copy link
Member

Ok no problem. Please add the blocked label to any PRs you'd like me to avoid merging once it's approved and the CI is looking good.

Copy link
Member

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

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

Manually verified that the reported failure has been fixed by building/running the sample on ios-arm64 device.

@SamMonoRT SamMonoRT removed the blocked Issue/PR is blocked on something - see comments label Aug 29, 2023
@SamMonoRT
Copy link
Member

cc @carlossanlop - this is ready to be merged. Thank you.

@carlossanlop carlossanlop merged commit 2ec0bc7 into release/8.0 Aug 29, 2023
@carlossanlop carlossanlop deleted the backport/pr-90875-to-release/8.0 branch August 29, 2023 17:34
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Codegen-AOT-mono os-ios Apple iOS Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants