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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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

if (!apply_override (klass, ic, vtable, decl, override, &override_map, &override_class_map, &conflict_map))
goto fail;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;

// In GH issue 61244 the mono runtime aborted when inflating the default
// interface method because the context used was from the base interface.

// The OneArgBaseInterface portion of this test handles the original bug
// where the base interface has less generic arguments than the derived
// interface and the runtime aborts.

// The SecondInterface portion of this test handles an additional scenario
// where the number of generic arguments are the same in the base and
// derived interface contexts, but the order is changed (or different.)
// When this occurs the generic info is incorrect for the inflated method.

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