[mono] Extend mono_gsharedvt_constrained_call JIT icall to handle static virtual methods

[kotlarmilos]

This PR extends mono_gsharedvt_constrained_call JIT icall to handle static virtual methods. If cmethod is a static virtual method, this arg should be null.

Contributes to #90732

[ghost]

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

Issue Details

---

This PR extends mono_gsharedvt_constrained_call JIT icall to handle static virtual methods. If cmethod is a static virtual method, the receiver's type is null. In these scenarios, the call cannot be made through constrained_gsharedvt_call_setup, and the type is retrieved from vtable of a class.

Hopefully fixes #90732

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

[LeVladIonescu]

Can you also add the testcase here?

[kotlarmilos]

Good idea. We will add a test case once we confirm this is the right fix.

[BrzVlad]

So this code was a special case for call to Object.GetType () by returning vt->type. Your change just makes every static virtual call do the same thing instead of running the actual method !?

[kotlarmilos]

Yes, here is the reasoning behind this change. Before the optimization of constrained calls from gshared methods in #79339, the type was retrieved as vt = mono_class_vtable_checked (klass, error);, in the same way as it is done here. After that, in #65126 arguments of static virtual methods are handled in the same way as for Object.GetType ():

runtime/src/mono/mono/mini/method-to-ir.c

Lines 3857 to 3858 in 5d00fc5

	/* !fsig->hasthis is for the wrapper for the Object.GetType () icall or static virtual methods */
	if ((fsig->hasthis || m_method_is_static (cmethod)) && fsig->param_count) {

Additionally, there is a note that the mp is null in case of static virtual methods, which is why the constrained_gsharedvt_call_setup fails:

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

Lines 1423 to 1433 in 5d00fc5

	/*
	* mono_gsharedvt_constrained_call:
	*
	* Make a call to CMETHOD using the receiver MP, which is assumed to be of type KLASS. ARGS contains
	* the arguments to the method in the format used by mono_runtime_invoke_checked ().
	* MP is NULL if CMETHOD is a static virtual method.
	*/
	MonoObject*
	mono_gsharedvt_constrained_call (gpointer mp, MonoMethod *cmethod, MonoClass *klass,
	MonoGsharedvtConstrainedCallInfo *info, guint8 *deref_args, gpointer *args)
	{

My assumption was that it shouldn't reach the constrained_gsharedvt_call_setup in this case.

[kotlarmilos]

After offline discussion, the above ^ was a wild assumption :) The static methods should be handled in constrained_gsharedvt_call_setup as @BrzVlad suggested.

[BrzVlad]

As far as my understanding goes, when you get below to constrained_gsharedvt_call_setup, you have the constrained class in klass (which should implement the method to be called) and the static virtual method to be called in cmethod (double check this). This is enough information to resolve the actual implementation of cmethod in klass, just need find out why the code crashes.

[kotlarmilos]

As far as my understanding goes, when you get below to constrained_gsharedvt_call_setup, you have the constrained class in klass (which should implement the method to be called) and the static virtual method to be called in cmethod (double check this). This is enough information to resolve the actual implementation of cmethod in klass, just need find out why the code crashes.

Thanks for feedback! It should be already handled here:

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

Lines 1377 to 1390 in 5d00fc5

	/* Lookup the virtual method */
	mono_class_setup_vtable (klass);
	g_assert (m_class_get_vtable (klass));
	vt_slot = mono_method_get_vtable_slot (cmethod);
	if (mono_class_is_interface (cmethod->klass)) {
	iface_offset = mono_class_interface_offset (klass, cmethod->klass);
	g_assert (iface_offset != -1);
	vt_slot += iface_offset;
	}
	m = m_class_get_vtable (klass) [vt_slot];
	if (cmethod->is_inflated) {
	m = mono_class_inflate_generic_method_full_checked (m, NULL, mono_method_get_context (cmethod), error);
	return_val_if_nok (error, NULL);
	}

Do you think we can avoid this_obj assignment if receiver's type (mp) is null?

[kotlarmilos]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BrzVlad]

This test seems quite trivial. Would it even fail on CI ? Or is the failure dependent on compilation flags that we don't use on CI.

[kotlarmilos]

The issue occurred in full AOT mode only. This test is used locally to reproduce and fix the issue, which is similar to the customer reported issue. Do you think it should be expanded?

[BrzVlad]

If you disable your fix will this test fail on CI currently ?

[kotlarmilos]

Before adding a new test, we should verify if it is not already covered on the CI. This will be done once the AOT job is fixed. The customer issue won't be closed until we verify that this case has test coverage.

[kotlarmilos]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/5964549560