Skip to content

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

Merged
lambdageek merged 3 commits into
dotnet:mainfrom
Unity-Technologies:bugfix/issue-61244
Feb 7, 2022
Merged

vtable setup fix for generic default interface methods in mono runtime#64102
lambdageek merged 3 commits into
dotnet:mainfrom
Unity-Technologies:bugfix/issue-61244

Conversation

@bholmes

@bholmesbholmes commented Jan 21, 2022

Copy link
Copy Markdown
Contributor

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.
@ghostghost added area-TypeSystem-coreclr community-contribution Indicates that the PR has been added by a community member labels 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
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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

@lambdageeklambdageekFeb 1, 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.

We'll do a separate PR for net7 with

} elseif (decl->is_inflated) {
g_assert_not_reached ();
}

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

@lambdageek

Copy link
Copy Markdown
Member

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

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

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@lambdageek

Copy link
Copy Markdown
Member

/azp run sync-runtime-to-mono

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

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

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:

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

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

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

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

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

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.)

@bholmesbholmesFeb 2, 2022

Copy link
Copy Markdown
ContributorAuthor

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
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!

@bholmes

Copy link
Copy Markdown
ContributorAuthor

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

@lambdageek

Copy link
Copy Markdown
Member

/backport to release/6.0

@github-actions

Copy link
Copy Markdown
Contributor

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

@lambdageek

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

Thanks @bholmes !

@lambdageek
lambdageek merged commit 3e4acf4 into dotnet:mainFeb 7, 2022
@ghostghost locked as resolved and limited conversation to collaborators Mar 10, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-meta-monocommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

3 participants

@bholmes@lambdageek@MichalStrehovsky