From 78904b87e521c6a5db9421f6b3d6e14c73d33db1 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 22 Aug 2022 16:11:06 -0400 Subject: [PATCH 1/9] do we need to inflate here? it seems like we always get the right class --- src/mono/mono/metadata/class-setup-vtable.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/mono/mono/metadata/class-setup-vtable.c b/src/mono/mono/metadata/class-setup-vtable.c index 2c29e30d05a05b..5ab2d554e351b8 100644 --- a/src/mono/mono/metadata/class-setup-vtable.c +++ b/src/mono/mono/metadata/class-setup-vtable.c @@ -1025,12 +1025,14 @@ apply_override (MonoClass *klass, MonoClass *override_class, MonoMethod **vtable * This is needed so the mono_class_is_assignable_from_internal () calls in the * conflict resolution work. */ - if (mono_class_is_ginst (override_class)) { + g_assert (override_class == override->klass); + if (mono_class_is_ginst (override_class) && override->klass != override_class) { override = mono_class_inflate_generic_method_checked (override, &mono_class_get_generic_class (override_class)->context, error); mono_error_assert_ok (error); } - if (mono_class_is_ginst (prev_override_class)) { + g_assert (prev_override->klass == prev_override_class); + if (mono_class_is_ginst (prev_override_class) && prev_override->klass != prev_override_class) { prev_override = mono_class_inflate_generic_method_checked (prev_override, &mono_class_get_generic_class (prev_override_class)->context, error); mono_error_assert_ok (error); } From 270196eeb3f90157963ba3f7483256410953e4fa Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 23 Aug 2022 14:21:10 -0400 Subject: [PATCH 2/9] use member access, not type punning --- src/mono/mono/metadata/class-setup-vtable.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/class-setup-vtable.c b/src/mono/mono/metadata/class-setup-vtable.c index 5ab2d554e351b8..e680508495cb14 100644 --- a/src/mono/mono/metadata/class-setup-vtable.c +++ b/src/mono/mono/metadata/class-setup-vtable.c @@ -932,7 +932,8 @@ mono_class_setup_vtable_full (MonoClass *klass, GList *in_setup) context = mono_class_get_context (klass); type_token = mono_class_get_generic_class (klass)->container_class->type_token; } else { - context = (MonoGenericContext *) mono_class_try_get_generic_container (klass); //FIXME is this a case of a try? + MonoGenericContainer *container = mono_class_try_get_generic_container (klass); //FIXME is this a case of a try? + context = container ? &container->context : NULL; type_token = klass->type_token; } From ba3701340dfab3fe6abd64588f643a5a0d406473 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 23 Aug 2022 15:26:39 -0400 Subject: [PATCH 3/9] Add regression test for #70190 --- .../regressions/github61244.cs | 41 ++++++++++++++++++- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github61244.cs b/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github61244.cs index ae08369177388d..66783d12a7a164 100644 --- a/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github61244.cs +++ b/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github61244.cs @@ -14,11 +14,19 @@ // derived interface contexts, but the order is changed (or different.) // When this occurs the generic info is incorrect for the inflated method. +// TestClass2 tests a regression due to the fix for the previous +// regression that caused Mono to incorrectly instantiate generic +// interfaces that appeared in the MethodImpl table + class Program { static int Main(string[] args) { - return new TestClass().DoTest(); + int result = new TestClass().DoTest(); + if (result != 100) + return result; + result = new TestClass2().DoTest(); + return result; } } @@ -78,4 +86,33 @@ public int DoTest () Console.WriteLine("Passed => 100"); return 100; } -} \ No newline at end of file +} + +public interface IA +{ + public int Foo(); +} + +public interface IB : IA +{ + int IA.Foo() { return 104; } +} + +public interface IC : IB

+{ + int IA.Foo() { return 105; } +} + +public class C : IC +{ + int IA.Foo() { return 100; } +} + +public class TestClass2 +{ + public int DoTest() + { + IA c = new C(); + return c.Foo(); + } +} From 55564f90c01c180318d900c16da06a74303e2348 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 23 Aug 2022 15:27:33 -0400 Subject: [PATCH 4/9] Assert code from #64102 is unreachable In https://github.com/dotnet/runtime/pull/64102#discussion_r790019545 we concluded that this branch is never taken. --- src/mono/mono/metadata/class-setup-vtable.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/mono/mono/metadata/class-setup-vtable.c b/src/mono/mono/metadata/class-setup-vtable.c index e680508495cb14..f5bf175f712584 100644 --- a/src/mono/mono/metadata/class-setup-vtable.c +++ b/src/mono/mono/metadata/class-setup-vtable.c @@ -1775,8 +1775,9 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o 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); + // there used to be code here to inflate decl, but in https://github.com/dotnet/runtime/pull/64102#discussion_r790019545 we + // think that this does not correspond to any real code. + g_assert_not_reached (); } if (!apply_override (klass, ic, vtable, decl, override, &override_map, &override_class_map, &conflict_map)) goto fail; From 42263276dcf375b6ab073bc3be6db11afcec87be Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 23 Aug 2022 15:28:35 -0400 Subject: [PATCH 5/9] Assert that overrides are already inflated how we expect --- src/mono/mono/metadata/class-setup-vtable.c | 32 +++++++++------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/mono/mono/metadata/class-setup-vtable.c b/src/mono/mono/metadata/class-setup-vtable.c index f5bf175f712584..9a228be1c83159 100644 --- a/src/mono/mono/metadata/class-setup-vtable.c +++ b/src/mono/mono/metadata/class-setup-vtable.c @@ -1011,6 +1011,8 @@ apply_override (MonoClass *klass, MonoClass *override_class, MonoMethod **vtable MonoMethod *prev_override = (MonoMethod*)g_hash_table_lookup (map, decl); MonoClass *prev_override_class = (MonoClass*)g_hash_table_lookup (class_map, decl); + g_assert (override_class == override->klass); + g_hash_table_insert (map, decl, override); g_hash_table_insert (class_map, decl, override_class); @@ -1018,25 +1020,7 @@ apply_override (MonoClass *klass, MonoClass *override_class, MonoMethod **vtable if (prev_override) { ERROR_DECL (error); - /* - * The override methods are part of the generic definition, need to inflate them so their - * parent class becomes the actual interface/class containing the override, i.e. - * IFace in: - * class Foo : IFace - * This is needed so the mono_class_is_assignable_from_internal () calls in the - * conflict resolution work. - */ - g_assert (override_class == override->klass); - if (mono_class_is_ginst (override_class) && override->klass != override_class) { - override = mono_class_inflate_generic_method_checked (override, &mono_class_get_generic_class (override_class)->context, error); - mono_error_assert_ok (error); - } - g_assert (prev_override->klass == prev_override_class); - if (mono_class_is_ginst (prev_override_class) && prev_override->klass != prev_override_class) { - prev_override = mono_class_inflate_generic_method_checked (prev_override, &mono_class_get_generic_class (prev_override_class)->context, error); - mono_error_assert_ok (error); - } if (!*conflict_map) *conflict_map = g_hash_table_new (mono_aligned_addr_hash, NULL); @@ -1790,6 +1774,18 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o MonoMethod *decl = overrides [i*2]; MonoMethod *override = overrides [i*2 + 1]; if (MONO_CLASS_IS_INTERFACE_INTERNAL (decl->klass)) { + /* + * We expect override methods that are part of a generic definition, to have + * their parent class be the actual interface/class containing the override, + * i.e. + * + * IFace in: + * class Foo : IFace + * + * This is needed so the mono_class_is_assignable_from_internal () calls in the + * conflict resolution work. + */ + g_assert (override->klass == klass); if (!apply_override (klass, klass, vtable, decl, override, &override_map, &override_class_map, &conflict_map)) goto fail; } From 54d68802deed2cade8b938da6bb83a3506ec3ff9 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 23 Aug 2022 15:28:57 -0400 Subject: [PATCH 6/9] try to enable some disabled DIM tests --- src/tests/issues.targets | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index 7f6f58cff1ad06..b9955801d679bc 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -2022,10 +2022,10 @@ These tests are not supposed to be run with mono. - + needs triage - + needs triage From 34d4348f66b741da593d4d7d4554df04b75be3c0 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 23 Aug 2022 18:15:33 -0400 Subject: [PATCH 7/9] remove unused var --- src/mono/mono/metadata/class-setup-vtable.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/mono/mono/metadata/class-setup-vtable.c b/src/mono/mono/metadata/class-setup-vtable.c index 9a228be1c83159..01f19f46180f4a 100644 --- a/src/mono/mono/metadata/class-setup-vtable.c +++ b/src/mono/mono/metadata/class-setup-vtable.c @@ -1018,8 +1018,6 @@ apply_override (MonoClass *klass, MonoClass *override_class, MonoMethod **vtable /* Collect potentially conflicting overrides which are introduced by default interface methods */ if (prev_override) { - ERROR_DECL (error); - g_assert (prev_override->klass == prev_override_class); if (!*conflict_map) From cae9bd44bb3822596a1c461c816b533dffaf7200 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 24 Aug 2022 00:07:56 -0400 Subject: [PATCH 8/9] Don't assert - code is reachable, there's just nothing to do --- src/mono/mono/metadata/class-setup-vtable.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/mono/mono/metadata/class-setup-vtable.c b/src/mono/mono/metadata/class-setup-vtable.c index 01f19f46180f4a..bc949189a3c125 100644 --- a/src/mono/mono/metadata/class-setup-vtable.c +++ b/src/mono/mono/metadata/class-setup-vtable.c @@ -1756,11 +1756,9 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o MonoMethod *override = iface_overrides [i*2 + 1]; 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) { - // there used to be code here to inflate decl, but in https://github.com/dotnet/runtime/pull/64102#discussion_r790019545 we - // think that this does not correspond to any real code. - g_assert_not_reached (); - } + } + // there used to be code here to inflate decl if decl->is_inflated, but in https://github.com/dotnet/runtime/pull/64102#discussion_r790019545 we + // think that this does not correspond to any real code. if (!apply_override (klass, ic, vtable, decl, override, &override_map, &override_class_map, &conflict_map)) goto fail; } From 2f75323b75f868db601b8def8d6e43e29790cffb Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 24 Aug 2022 09:39:18 -0400 Subject: [PATCH 9/9] Add link to issue for failing tests --- src/tests/issues.targets | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index b9955801d679bc..24b9f51aabd2da 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -2022,11 +2022,11 @@ These tests are not supposed to be run with mono. - - needs triage + + https://github.com/dotnet/runtime/issues/36113 - - needs triage + + https://github.com/dotnet/runtime/issues/36113 needs triage