Skip to content

Conversation

@kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Apr 24, 2024

Description

Backport of #94787 to release/8.0-staging

Before this change, when an icall was made from the gsharedvt with a constrained static virtual method, the this argument wasn't handled properly, leading to a SIGSEGV error. Additionally, nullable boxed value types were not dereferenced correctly. This PR expands the functionality of mono_gsharedvt_constrained_call to manage static call arguments and avoid dereferencing sharedvt reference arguments if they are nullable value types.

Customer Impact

The issue was reported on Apple mobile platforms in #94467 and #101427. It should fix #101427 and resolve tests failures on Apple mobile platforms.

This issue also fixes a regression introduced in .NET 8.0.2 that prevents Blazor WebAssembly applications that use MudBlazor from being AOTed. Fixes #100527

Testing

This PR introduces a runtime test, similar to the one implemented for .NET 9 (#101489). This fix is verified manually and by the existing System.Collections.Immutable.Tests on Apple mobile platforms.

Risk

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

This backport is for a servicing release 8.0.3.

  • 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.

…llable value types

This PR extends mono_gsharedvt_constrained_call to handle static MONO_GSHAREDVT_CONSTRAINT_CALL_TYPE_REF calls. If the cmethod is a static method, this_arg should be NULL. Also, it skips dereferencing sharedvt ref arguments if they are nullable value types.
@kotlarmilos kotlarmilos added this to the 8.0.x milestone Apr 24, 2024
@kotlarmilos kotlarmilos self-assigned this Apr 24, 2024
@dotnet-policy-service
Copy link
Contributor

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

@ivanpovazan ivanpovazan self-requested a review April 24, 2024 11:59
@lambdageek
Copy link
Member

We already had part of the fix in #97850; the difference is that that one was for non-virtual static methods. Also it didn't include the handling for nullable types

@lambdageek
Copy link
Member

lambdageek commented Apr 24, 2024

I think this might be related to #100527, too. Still investigating, but I think #97850 without this other stuff caused a regression in wasm AOT... Reverting #97850 resolves that issue. Going to see if taking the fixes here is sufficient as well

Update nope, this isn't enough for the blazor issue. I think I'm not picking up the AOT compiler changes. This might still be a good fix for Blazor...

Update2: yep, this also fixes #100527

@kotlarmilos
Copy link
Member Author

The failures shouldn't be related. The System.Collections.Immutable.Tests are now passing on Apple mobile platforms.

@kotlarmilos kotlarmilos added the Servicing-consider Issue for next servicing release review label Apr 25, 2024
@kotlarmilos kotlarmilos changed the title [release/8.0-staging] Extend mono_gsharedvt_constrained_call for static calls and handle nullable value types [release/8.0-staging] Extend mono_gsharedvt_constrained_call for static virtual calls Apr 25, 2024
@kotlarmilos kotlarmilos added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 26, 2024
@kotlarmilos kotlarmilos merged commit 8ff4dd4 into release/8.0-staging Apr 30, 2024
@kotlarmilos kotlarmilos deleted the backport/pr-94787-to-release/8.0-staging branch April 30, 2024 08:57
@github-actions github-actions bot locked and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Codegen-JIT-mono Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants