[mono] Extend mono_gsharedvt_constrained_call for static calls and handle nullable value types #94787

kotlarmilos · 2023-11-15T16:14:38Z

Description

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.

Fixes #94467

…haredvt ref arguments if it is a nullable vtype.

ghost · 2023-11-15T16:14:50Z

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

Issue Details

Description

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.

Fixes #94467

Author:	kotlarmilos
Assignees:	kotlarmilos
Labels:	`area-Codegen-AOT-mono`, `os-ios`
Milestone:	9.0.0

kotlarmilos · 2023-11-15T16:15:13Z

src/mono/mono/mini/jit-icalls.c

@@ -1482,7 +1483,9 @@ mono_gsharedvt_constrained_call (gpointer mp, MonoMethod *cmethod, MonoClass *kl
 		g_assert (fsig->param_count < 16);
 		memcpy (new_args, args, fsig->param_count * sizeof (gpointer));
 		for (int i = 0; i < fsig->param_count; ++i) {
-			if (deref_args [i])
+			// If the argument is a ref or nullable non-vtype, deref it


@vargaz Please confirm this is correct.

Isn't this information already known at the callsite in handle_constrained_gsharedvt_call and can be passed through deref_args? Also isn't deref_args supposed to be set only for valuetype args, why the check below?

Is there a testcase which fails for this case ?

The case we want handle here is handler.Test<bool?>(true);.

Isn't this information already known at the callsite in handle_constrained_gsharedvt_call and can be passed through deref_args?

Yes, this information already known at the callsite:

if (mini_is_gsharedvt_type (fsig->params [i])) { MonoInst *is_deref; int deref_arg_reg; ins = mini_emit_get_gsharedvt_info_klass (cfg, mono_class_from_mono_type_internal (fsig->params [i]), MONO_RGCTX_INFO_CLASS_BOX_TYPE); deref_arg_reg = alloc_preg (cfg); /* deref_arg = BOX_TYPE != MONO_GSHAREDVT_BOX_TYPE_VTYPE */ EMIT_NEW_BIALU_IMM (cfg, is_deref, OP_ISUB_IMM, deref_arg_reg, ins->dreg, 1); MONO_EMIT_NEW_STORE_MEMBASE (cfg, OP_STOREI1_MEMBASE_REG, is_gsharedvt_ins->dreg, i, is_deref->dreg); }

Also isn't deref_args supposed to be set only for valuetype args, why the check below?

It appears that an argument is being dereferenced if the BOX_TYPE != MONO_GSHAREDVT_BOX_TYPE_VTYPE. However, if the argument is a nullable value type, deref_args [i] is set because mono_class_is_nullable is checked first:

if (MONO_TYPE_IS_REFERENCE (m_class_get_byval_arg (impl_class))) return GUINT_TO_POINTER (MONO_GSHAREDVT_BOX_TYPE_REF); else if (mono_class_is_nullable (impl_class)) return GUINT_TO_POINTER (MONO_GSHAREDVT_BOX_TYPE_NULLABLE); else return GUINT_TO_POINTER (MONO_GSHAREDVT_BOX_TYPE_VTYPE);

Then, during runtime, a nullable value type is dereferenced, leading to a runtime crash. Perhaps this check (if not nullable vtype) can be moved to the handle_constrained_gsharedvt_call, which might result in larger codegen.

Is there a testcase which fails for this case ?

The fullAOT tests are not currently running on the CI. Once the job is enabled, we will check if this case is already covered. If it is not covered, we will add a runtime test. You can currently verify this by using the following sample: #94467 (comment).

Would suggest this fix instead:

diff --git a/src/mono/mono/mini/jit-icalls.c b/src/mono/mono/mini/jit-icalls.c index 21f3252c814..442cb9a65aa 100644 --- a/src/mono/mono/mini/jit-icalls.c +++ b/src/mono/mono/mini/jit-icalls.c @@ -1482,7 +1482,7 @@ mono_gsharedvt_constrained_call (gpointer mp, MonoMethod *cmethod, MonoClass *kl g_assert (fsig->param_count < 16); memcpy (new_args, args, fsig->param_count * sizeof (gpointer)); for (int i = 0; i < fsig->param_count; ++i) { - if (deref_args [i]) + if (deref_args [i] != MONO_GSHAREDVT_BOX_TYPE_VTYPE && deref_args [i] != MONO_GSHAREDVT_BOX_TYPE_NULLABLE) new_args [i] = *(gpointer*)new_args [i]; } args = new_args; diff --git a/src/mono/mono/mini/method-to-ir.c b/src/mono/mono/mini/method-to-ir.c index 99a11126b62..1f359e31cc6 100644 --- a/src/mono/mono/mini/method-to-ir.c +++ b/src/mono/mono/mini/method-to-ir.c @@ -3896,13 +3896,8 @@ handle_constrained_gsharedvt_call (MonoCompile *cfg, MonoMethod *cmethod, MonoMe int addr_reg; if (mini_is_gsharedvt_type (fsig->params [i])) { - MonoInst *is_deref; - int deref_arg_reg; ins = mini_emit_get_gsharedvt_info_klass (cfg, mono_class_from_mono_type_internal (fsig->params [i]), MONO_RGCTX_INFO_CLASS_BOX_TYPE); - deref_arg_reg = alloc_preg (cfg); - /* deref_arg = BOX_TYPE != MONO_GSHAREDVT_BOX_TYPE_VTYPE */ - EMIT_NEW_BIALU_IMM (cfg, is_deref, OP_ISUB_IMM, deref_arg_reg, ins->dreg, 1); - MONO_EMIT_NEW_STORE_MEMBASE (cfg, OP_STOREI1_MEMBASE_REG, is_gsharedvt_ins->dreg, i, is_deref->dreg); + MONO_EMIT_NEW_STORE_MEMBASE (cfg, OP_STOREI1_MEMBASE_REG, is_gsharedvt_ins->dreg, i, ins->dreg); } else if (has_gsharedvt) { MONO_EMIT_NEW_STORE_MEMBASE_IMM (cfg, OP_STOREI1_MEMBASE_IMM, is_gsharedvt_ins->dreg, i, 0); }

kotlarmilos · 2023-11-23T09:10:29Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-11-23T09:10:53Z

Azure Pipelines successfully started running 1 pipeline(s).

kotlarmilos · 2023-12-06T15:59:17Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-12-06T15:59:42Z

Azure Pipelines successfully started running 1 pipeline(s).

kotlarmilos · 2023-12-08T11:30:37Z

/backport to release/8.0-staging

github-actions · 2023-12-08T11:30:52Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7140806171

vargaz · 2023-12-08T11:33:24Z

Would be nice to have some CI tests before a backport.

kotlarmilos · 2023-12-08T11:39:20Z

Agreed. Postponing this backport until we enable fullAOT CI tests: #92057.

kotlarmilos · 2024-04-24T11:29:57Z

/backport to release/8.0-staging

github-actions · 2024-04-24T11:30:13Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8815796126

github-actions · 2024-04-24T11:31:45Z

@kotlarmilos backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: [mono] Don't use this_arg in static constrained calls. Don't deref gsharedvt ref arguments if it is a nullable vtype.
Using index info to reconstruct a base tree...
M	src/mono/mono/mini/jit-icalls.c
Falling back to patching base and 3-way merge...
Auto-merging src/mono/mono/mini/jit-icalls.c
CONFLICT (content): Merge conflict in src/mono/mono/mini/jit-icalls.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 [mono] Don't use this_arg in static constrained calls. Don't deref gsharedvt ref arguments if it is a nullable vtype.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-04-24T11:31:45Z

@kotlarmilos an error occurred while backporting to release/8.0-staging, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

[mono] Don't use this_arg in static constrained calls. Don't deref gs…

e057939

…haredvt ref arguments if it is a nullable vtype.

kotlarmilos added area-Codegen-AOT-mono os-ios Apple iOS labels Nov 15, 2023

kotlarmilos added this to the 9.0.0 milestone Nov 15, 2023

kotlarmilos self-assigned this Nov 15, 2023

kotlarmilos requested review from vargaz, lambdageek and SamMonoRT as code owners November 15, 2023 16:14

kotlarmilos commented Nov 15, 2023

View reviewed changes

kotlarmilos requested a review from BrzVlad November 23, 2023 09:11

kotlarmilos added 3 commits November 23, 2023 10:13

[mono] Fix lint formatting

094150d

Merge branch 'dotnet:main' into bugfix/static-gsharedvt-methods

841438c

Remove additional deref register

e9f806a

vargaz approved these changes Dec 6, 2023

View reviewed changes

This was referenced Dec 6, 2023

[wasm][debugger] Debugger tests getting randomly aborted on CI #89409

Open

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

runfoapp bot mentioned this pull request Dec 6, 2023

system.text.regularexpressions.tests work item #72834

Open

kotlarmilos merged commit 5b0f743 into dotnet:main Dec 7, 2023
152 of 163 checks passed

github-actions bot mentioned this pull request Dec 8, 2023

[release/8.0-staging] [mono] Extend mono_gsharedvt_constrained_call for static calls and handle nullable value types #95784

Closed

kotlarmilos added the blocked Issue/PR is blocked on something - see comments label Dec 8, 2023

github-actions bot locked and limited conversation to collaborators Jan 8, 2024

ivanpovazan mentioned this pull request Apr 23, 2024

[mono][ios] ToFrozenSet crashes on iOS devices with 8.0.3 release #101427

Closed

kotlarmilos mentioned this pull request Apr 24, 2024

Add runtime test for static gsharedvt methods #101489

Merged

github-actions bot unlocked this conversation Apr 24, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono] Extend mono_gsharedvt_constrained_call for static calls and handle nullable value types #94787

[mono] Extend mono_gsharedvt_constrained_call for static calls and handle nullable value types #94787

kotlarmilos commented Nov 15, 2023

ghost commented Nov 15, 2023

Description

kotlarmilos Nov 15, 2023

BrzVlad Nov 27, 2023 •

edited

Loading

vargaz Nov 27, 2023

kotlarmilos Dec 4, 2023

vargaz Dec 6, 2023

kotlarmilos commented Nov 23, 2023

azure-pipelines bot commented Nov 23, 2023

kotlarmilos commented Dec 6, 2023

azure-pipelines bot commented Dec 6, 2023

kotlarmilos commented Dec 8, 2023

github-actions bot commented Dec 8, 2023

vargaz commented Dec 8, 2023

kotlarmilos commented Dec 8, 2023

kotlarmilos commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

[mono] Extend mono_gsharedvt_constrained_call for static calls and handle nullable value types #94787

[mono] Extend mono_gsharedvt_constrained_call for static calls and handle nullable value types #94787

Conversation

kotlarmilos commented Nov 15, 2023

Description

ghost commented Nov 15, 2023

Description

kotlarmilos Nov 15, 2023

Choose a reason for hiding this comment

BrzVlad Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

vargaz Nov 27, 2023

Choose a reason for hiding this comment

kotlarmilos Dec 4, 2023

Choose a reason for hiding this comment

vargaz Dec 6, 2023

Choose a reason for hiding this comment

kotlarmilos commented Nov 23, 2023

azure-pipelines bot commented Nov 23, 2023

kotlarmilos commented Dec 6, 2023

azure-pipelines bot commented Dec 6, 2023

kotlarmilos commented Dec 8, 2023

github-actions bot commented Dec 8, 2023

vargaz commented Dec 8, 2023

kotlarmilos commented Dec 8, 2023

kotlarmilos commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

BrzVlad Nov 27, 2023 •

edited

Loading