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

vtable setup fix for generic default interface methods in mono runtime #64102

Merged
merged 3 commits into from
Feb 7, 2022

Conversation

bholmes
Copy link
Contributor

@bholmes bholmes commented Jan 21, 2022

When processing the overrides from interface default methods we should
check if the interface class is a generic type definition first and
inflate with the interface class context.

Test case included.

Fixes #61244

When processing the overrides from interface default methods we should
check if the interface class is a generic type definition first and
inflate with the interface class context.

Test case included.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 21, 2022
Comment on lines +1747 to 1750
} else if (decl->is_inflated) {
override = mono_class_inflate_generic_method_checked (override, mono_method_get_context (decl), error);
mono_error_assert_ok (error);
} else if (mono_class_is_gtd (override->klass)) {
override = mono_class_inflate_generic_method_full_checked (override, ic, mono_class_get_context (ic), error);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not certain that we need the decl->is_inflated case at all. I can not think of a scenario where this is needed. If we can find the testcase I would like to add that to the tests in this PR as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did change the code locally to assert if we hit the decl->is_inflated case and override changed. I then ran all the tests and I do think this this assert was hit and I got the same results. Depending on how well dims are covered with our test code, this code block may really be unnecessary.

} else if (decl->is_inflated) {
	MonoMethod *toverride = mono_class_inflate_generic_method_checked (override, mono_method_get_context (decl), error);
	g_assert(toverride == override);
	override = toverride;
	mono_error_assert_ok (error);
}

Copy link
Member

@lambdageek lambdageek Feb 1, 2022

Choose a reason for hiding this comment

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

We'll do a separate PR for net7 with

} else if (decl->is_inflated) {
	g_assert_not_reached ();
}

we'll backport the current version to mono/mono and net6

@lambdageek
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lambdageek lambdageek self-assigned this Feb 1, 2022
@lambdageek

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@lambdageek
Copy link
Member

/azp run sync-runtime-to-mono

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -0,0 +1,25 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

Before adding new tests, it might be worth checking whether this is already covered by the any of the existing default interface methods tests that are disabled on Mono. There's a ton of them and don't have an issue number:

<ExcludeList Include="$(XunitTestBinBase)/Loader/classloader/DefaultInterfaceMethods/reabstraction/**">
<Issue>needs triage</Issue>
</ExcludeList>

<ExcludeList Include="$(XunitTestBinBase)/reflection/DefaultInterfaceMethods/Emit/**">
<Issue>needs triage</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/reflection/DefaultInterfaceMethods/InvokeConsumer/**">
<Issue>needs triage</Issue>
</ExcludeList>

<ExcludeList Include="$(XunitTestBinBase)/Regressions/coreclr/15241/genericcontext/**">
<Issue>needs triage</Issue>
</ExcludeList>

<ExcludeList Include="$(XunitTestBinBase)/Regressions/coreclr/16123/ambiguousconstraint/**">
<Issue>needs triage</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/Regressions/coreclr/16355/variance/**">
<Issue>needs triage</Issue>
</ExcludeList>

<ExcludeList Include="$(XunitTestBinBase)/Regressions/coreclr/22021/consumer/**">
<Issue>needs triage</Issue>
</ExcludeList>

Copy link
Member

Choose a reason for hiding this comment

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

(This test look pretty low value because it doesn't try to do a dispatch or check whether we have a good generic context.)

Copy link
Contributor Author

@bholmes bholmes Feb 2, 2022

Choose a reason for hiding this comment

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

I did check and none of the existing tests you list cover this case. If you want a more interesting case I can push the following...

// The .NET Foundation licenses this file to you under the MIT license.

using System;

class Program
{
    static int Main(string[] args)
    {
        return new TestClass().DoTest();
    }
}

public interface OneArgBaseInterface<T1>
{
    int SomeFunc1(T1 someParam1, Type someParam1Type);
}

public interface TwoArgBaseInterface<T1, T2>
{
    int SomeFunc1(T1 someParam1, T2 someParam2, Type someParam1Type, Type someParam2Type);
}

public interface SecondInterface<TParam2Type, TParam1Type> :  OneArgBaseInterface<TParam1Type>, TwoArgBaseInterface<TParam1Type, TParam2Type>
{
    int OneArgBaseInterface<TParam1Type>.SomeFunc1(TParam1Type someParam1, Type someParam1Type)
    {
        if (typeof(TParam1Type) != someParam1Type)
        {
            Console.WriteLine("Failed => 101");
            return 101;
        }

        return 100;
    }

    int TwoArgBaseInterface<TParam1Type, TParam2Type>.SomeFunc1(TParam1Type someParam1, TParam2Type someParam2, Type someParam1Type, Type someParam2Type)
    {
        if (typeof(TParam1Type) != someParam1Type)
        {
            Console.WriteLine("Failed => 102");
            return 102;
        }

        if (typeof(TParam2Type) != someParam2Type)
        {
            Console.WriteLine("Failed => 103");
            return 103;
        }

        return 100;
    }
}

public class TestClass : SecondInterface<int, string>
{
    public int DoTest ()
    {
        int ret = (this as OneArgBaseInterface<string>).SomeFunc1("test string", typeof(string));
        if (ret != 100)
            return ret;

        ret = (this as TwoArgBaseInterface<string, int>).SomeFunc1("test string", 0, typeof(string), typeof(int));
        if (ret != 100)
            return ret;

        Console.WriteLine("Passed => 100");
        return 100;
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

👍 this test looks much more thorough! Thanks!

@bholmes
Copy link
Contributor Author

bholmes commented Feb 3, 2022

@lambdageek pushed a test update. Will need to wait for all the tests to run again.

@lambdageek
Copy link
Member

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2022

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1790707501

@lambdageek
Copy link
Member

@bholmes Could you merge dotnet/runtime main into this branch. There are some CI infrastructure changes that are preventing the minijit and monointerpreter lanes from running.

@lambdageek
Copy link
Member

Thanks @bholmes !

@lambdageek lambdageek merged commit 3e4acf4 into dotnet:main Feb 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-meta-mono community-contribution Indicates that the PR has been added by a community member
Projects
None yet
3 participants