Skip to content

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

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.

Sign up for GitHub

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

Merged
lambdageek merged 3 commits into from
Feb 7, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f4fbe1
vtable setup fix for generic default interface methods in mono runtime
bholmes Jan 21, 2022
ce9b198
Update test to ensure that the correct context is used
bholmes Feb 3, 2022
295c8ac
Merge remote-tracking branch 'dotnet/main' into bugfix/issue-61244
bholmes Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/mono/mono/metadata/class-setup-vtable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1742,11 +1742,11 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
for (int i = 0; i < iface_onum; i++) {
MonoMethod *decl = iface_overrides [i*2];
MonoMethod *override = iface_overrides [i*2 + 1];
if (decl->is_inflated) {
if (mono_class_is_gtd (override->klass)) {
override = mono_class_inflate_generic_method_full_checked (override, ic, mono_class_get_context (ic), error);
} 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);
}
Comment on lines +1747 to 1750

@bholmes bholmes Jan 21, 2022

Copy link
Copy Markdown
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.

@bholmes bholmes Jan 21, 2022

Copy link
Copy Markdown
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);
}

@lambdageek lambdageek Feb 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

if (!apply_override (klass, ic, vtable, decl, override, &override_map, &override_class_map, &conflict_map))
goto fail;
Expand Down
25 changes: 25 additions & 0 deletions src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github61244.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Licensed to the .NET Foundation under one or more agreements.

@MichalStrehovsky MichalStrehovsky Feb 2, 2022

Copy link
Copy Markdown
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:

runtime/src/tests/issues.targets

Lines 1848 to 1850 in 6527b7f

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

runtime/src/tests/issues.targets

Lines 1913 to 1918 in 6527b7f

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

runtime/src/tests/issues.targets

Lines 1928 to 1930 in 6527b7f

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

runtime/src/tests/issues.targets

Lines 1934 to 1939 in 6527b7f

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

runtime/src/tests/issues.targets

Lines 1943 to 1945 in 6527b7f

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

@MichalStrehovsky MichalStrehovsky Feb 2, 2022

Copy link
Copy Markdown
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.)

@bholmes bholmes Feb 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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;
    }
}

@MichalStrehovsky MichalStrehovsky Feb 3, 2022

Copy link
Copy Markdown
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!

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

using System;

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

public interface FirstInterface<T>
{
void SomeFunc1(T someParam);
}

public interface SecondInterface<T1, T2> : FirstInterface<T1>
{
void FirstInterface<T1>.SomeFunc1(T1 someParam) => Console.WriteLine ("Test");
}

public class TestClass : SecondInterface<int, string>{}
9 changes: 9 additions & 0 deletions src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github61244.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<OutputType>Exe</OutputType>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>