[mono] Crash in common_call_trampoline due to inconsistent rgctx mode

[uweigand]

Description

When running the Microsoft.CodeAnalysis.CSharp.UnitTests.Symbols.DefaultInterfaceImplementationTests.VarianceSafety_15 test case out of the roslyn unit test suite, mono crashes with:

* Assertion at /home/uweigand/dotnet-s390x/runtime/src/mono/mono/mini/mini-trampolines.c:615, condition `actual_method->klass == klass' not met

Configuration

.NET 6 Preview7 re-built for the linux-s390x target (using the mono runtime by default), running natively.

Regression?

Yes, I haven't seen this crash with the Preview6 build (or earlier).

Other information

The klass variable is set by this code:

                if (m->is_inflated && mono_method_get_context (m)->method_inst) {
                        MonoMethodRuntimeGenericContext *mrgctx = (MonoMethodRuntimeGenericContext*)mono_arch_find_static_call_vtable (regs, code);

                        klass = mrgctx->class_vtable->klass;
                        method_inst = mrgctx->method_inst;
                } else if ((m->flags & METHOD_ATTRIBUTE_STATIC) || m_class_is_valuetype (m->klass)) {
                        MonoVTable *vtable = mono_arch_find_static_call_vtable (regs, code);

                        klass = vtable->klass;
                } else {
[...]

Note that in this particular instance, the pointer returned by mono_arch_find_static_call_vtable (regs, code) is actually a MonoMethodRuntimeGenericContext, not a MonoVTable. But the code above branches into the second "if" block instead of the first, and therefore reads a clobbered pointer from vtable->klass.

The reason why the second branch is chosen is that mono_method_get_context (m)->method_inst is nullptr at this point.

However, looking at the code that sets up the rgctx register, e.g. in mini_get_rgctx_access_for_method or check_method_sharing, we see that the MRGCTX access method is used even in cases where method_inst is null, if the method is a "default method":

MonoRgctxAccess
mini_get_rgctx_access_for_method (MonoMethod *method)
{
        /* gshared dim methods use an mrgctx */
        if (mini_method_is_default_method (method))
                return MONO_RGCTX_ACCESS_MRGCTX;

        if (mono_method_get_context (method)->method_inst)
                return MONO_RGCTX_ACCESS_MRGCTX;

        if (method->flags & METHOD_ATTRIBUTE_STATIC || m_class_is_valuetype (method->klass))
                return MONO_RGCTX_ACCESS_VTABLE;

        return MONO_RGCTX_ACCESS_THIS;
}

This is exactly what happens in the current case: the method in question is marked as "default method". Changing the above code in common_call_trampoline to expect a MGRTX access in that case as well fixes the crash (and makes the roslyn test case pass).

I'll submit a PR implementing this change shortly.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

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

Issue Details

---

Description

When running the Microsoft.CodeAnalysis.CSharp.UnitTests.Symbols.DefaultInterfaceImplementationTests.VarianceSafety_15 test case out of the roslyn unit test suite, mono crashes with:

* Assertion at /home/uweigand/dotnet-s390x/runtime/src/mono/mono/mini/mini-trampolines.c:615, condition `actual_method->klass == klass' not met

Configuration

.NET 6 Preview7 re-built for the linux-s390x target (using the mono runtime by default), running natively.

Regression?

Yes, I haven't seen this crash with the Preview6 build (or earlier).

Other information

The klass variable is set by this code:

                if (m->is_inflated && mono_method_get_context (m)->method_inst) {
                        MonoMethodRuntimeGenericContext *mrgctx = (MonoMethodRuntimeGenericContext*)mono_arch_find_static_call_vtable (regs, code);

                        klass = mrgctx->class_vtable->klass;
                        method_inst = mrgctx->method_inst;
                } else if ((m->flags & METHOD_ATTRIBUTE_STATIC) || m_class_is_valuetype (m->klass)) {
                        MonoVTable *vtable = mono_arch_find_static_call_vtable (regs, code);

                        klass = vtable->klass;
                } else {
[...]

Note that in this particular instance, the pointer returned by mono_arch_find_static_call_vtable (regs, code) is actually a MonoMethodRuntimeGenericContext, not a MonoVTable. But the code above branches into the second "if" block instead of the first, and therefore reads a clobbered pointer from vtable->klass.

The reason why the second branch is chosen is that mono_method_get_context (m)->method_inst is nullptr at this point.

However, looking at the code that sets up the rgctx register, e.g. in mini_get_rgctx_access_for_method or check_method_sharing, we see that the MRGCTX access method is used even in cases where method_inst is null, if the method is a "default method":

MonoRgctxAccess
mini_get_rgctx_access_for_method (MonoMethod *method)
{
        /* gshared dim methods use an mrgctx */
        if (mini_method_is_default_method (method))
                return MONO_RGCTX_ACCESS_MRGCTX;

        if (mono_method_get_context (method)->method_inst)
                return MONO_RGCTX_ACCESS_MRGCTX;

        if (method->flags & METHOD_ATTRIBUTE_STATIC || m_class_is_valuetype (method->klass))
                return MONO_RGCTX_ACCESS_VTABLE;

        return MONO_RGCTX_ACCESS_THIS;
}

This is exactly what happens in the current case: the method in question is marked as "default method". Changing the above code in common_call_trampoline to expect a MGRTX access in that case as well fixes the crash (and makes the roslyn test case pass).

I'll submit a PR implementing this change shortly.

Author:	uweigand
Assignees:	-
Labels:	untriaged, area-VM-meta-mono, in pr
Milestone:	-