[mono] Skip exact interface matches when an interface has variance (#103757)

* When searching for a variant interface, use a special 'variant search table' that contains only variant interfaces in the correct search order, and perform the search by doing two passes per class
* Add missing interfaces to set of array special interfaces in mono
* Don't dereference random memory if get_virtual_method fails in interp
* Add regression test
* Update reason why variance test is disabled

Author: kg

src/mono/mono/metadata/class-init.c

@@ -3286,7 +3286,14 @@ mono_class_setup_interface_id_nolock (MonoClass *klass)
 	    * 	a != b ==> true
 		*/
 		const char *name = m_class_get_name (klass);
-		if (!strcmp (name, "IList`1") || !strcmp (name, "ICollection`1") || !strcmp (name, "IEnumerable`1") || !strcmp (name, "IEnumerator`1"))
+		if (
+			!strcmp (name, "IList`1") ||
+			!strcmp (name, "IReadOnlyList`1") ||
+			!strcmp (name, "ICollection`1") ||
+			!strcmp (name, "IReadOnlyCollection`1") ||
+			!strcmp (name, "IEnumerable`1") ||
+			!strcmp (name, "IEnumerator`1")
+		)
 			klass->is_array_special_interface = 1;
 	}
 }
@@ -4189,6 +4196,89 @@ mono_class_set_runtime_vtable (MonoClass *klass, MonoVTable *vtable)
 	klass->runtime_vtable = vtable;
 }
 
+static int
+index_of_class (MonoClass *needle, MonoClass **haystack, int haystack_size) {
+	for (int i = 0; i < haystack_size; i++)
+		if (haystack[i] == needle)
+			return i;
+
+	return -1;
+}
+
+static void
+build_variance_search_table_inner (MonoClass *klass, MonoClass **buf, int buf_size, int *buf_count) {
+	if (!m_class_is_interfaces_inited (klass)) {
+		ERROR_DECL (error);
+		mono_class_setup_interfaces (klass, error);
+		return_if_nok (error);
+	}
+	guint c = m_class_get_interface_count (klass);
+	if (c) {
+		MonoClass **ifaces = m_class_get_interfaces (klass);
+		for (guint i = 0; i < c; i++) {
+			MonoClass *iface = ifaces [i];
+			// Avoid adding duplicates or recursing into them.
+			if (index_of_class (iface, buf, *buf_count) >= 0)
+				continue;
+
+			if (mono_class_has_variant_generic_params (iface)) {
+				g_assert (*buf_count < buf_size);
+				buf[*buf_count] = iface;
+				(*buf_count) += 1;
+			}
+
+			build_variance_search_table_inner (iface, buf, buf_size, buf_count);
+		}
+	}
+}
+
+// Only call this with the loader lock held
+static void
+build_variance_search_table (MonoClass *klass) {
+	// FIXME: Is there a way to deterministically compute the right capacity?
+	int buf_size = m_class_get_interface_offsets_count (klass), buf_count = 0;
+	MonoClass **buf = g_alloca (buf_size * sizeof(MonoClass *));
+	MonoClass **result = NULL;
+	memset (buf, 0, buf_size * sizeof(MonoClass *));
+	build_variance_search_table_inner (klass, buf, buf_size, &buf_count);
+
+	if (buf_count) {
+		guint bytes = buf_count * sizeof(MonoClass *);
+		result = mono_mem_manager_alloc (m_class_get_mem_manager (klass), bytes);
+		memcpy (result, buf, bytes);
+	}
+	klass->variant_search_table_length = buf_count;
+	klass->variant_search_table = result;
+	// Ensure we do not set the inited flag until we've stored the result pointer
+	mono_memory_barrier ();
+	klass->variant_search_table_inited = TRUE;
+}
+
+void
+mono_class_get_variance_search_table (MonoClass *klass, MonoClass ***table, int *table_size) {
+	g_assert (klass);
+	g_assert (table);
+	g_assert (table_size);
+
+	// We will never do a variance search to locate a given interface on an interface, only on
+	//  a fully-defined type or generic instance
+	if (m_class_is_interface (klass)) {
+		*table = NULL;
+		*table_size = 0;
+		return;
+	}
+
+	if (!klass->variant_search_table_inited) {
+		mono_loader_lock ();
+		if (!klass->variant_search_table_inited)
+			build_variance_search_table (klass);
+		mono_loader_unlock ();
+	}
+
+	*table = klass->variant_search_table;
+	*table_size = klass->variant_search_table_length;
+}
+
 /**
  * mono_classes_init:
  *

src/mono/mono/metadata/class-internals.h

@@ -1465,6 +1465,9 @@ mono_class_has_default_constructor (MonoClass *klass, gboolean public_only);
 gboolean
 mono_method_has_unmanaged_callers_only_attribute (MonoMethod *method);
 
+void
+mono_class_get_variance_search_table (MonoClass *klass, MonoClass ***table, int *table_size);
+
 // There are many ways to do on-demand initialization.
 //   Some allow multiple concurrent initializations. Some do not.
 //   Some allow multiple concurrent writes to the global. Some do not.

src/mono/mono/metadata/class-private-definition.h

@@ -80,6 +80,7 @@ struct _MonoClass {
 	guint has_deferred_failure : 1;
 	/* next byte*/
 	guint is_exception_class : 1; /* is System.Exception or derived from it */
+	guint variant_search_table_inited : 1;
 
 	MonoClass  *parent;
 	MonoClass  *nested_in;
@@ -127,6 +128,9 @@ struct _MonoClass {
 
 	/* Infrequently used items. See class-accessors.c: InfrequentDataKind for what goes into here. */
 	MonoPropertyBag infrequent_data;
+
+	MonoClass **variant_search_table;
+	int variant_search_table_length;
 };
 
 struct _MonoClassDef {

src/mono/mono/metadata/class.c

@@ -1946,21 +1946,25 @@ mono_class_interface_offset (MonoClass *klass, MonoClass *itf)
 int
 mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gboolean *non_exact_match)
 {
-	int i = mono_class_interface_offset (klass, itf);
+	gboolean has_variance = mono_class_has_variant_generic_params (itf);
+	int exact_match = mono_class_interface_offset (klass, itf), i = -1;
 	*non_exact_match = FALSE;
-	if (i >= 0)
-		return i;
+
+	if (exact_match >= 0) {
+		if (!has_variance)
+			return exact_match;
+	}
 
 	int klass_interface_offsets_count = m_class_get_interface_offsets_count (klass);
 
-	if (m_class_is_array_special_interface  (itf) && m_class_get_rank (klass) < 2) {
+	if (m_class_is_array_special_interface (itf) && m_class_get_rank (klass) < 2) {
 		MonoClass *gtd = mono_class_get_generic_type_definition (itf);
 		int found = -1;
 
 		for (i = 0; i < klass_interface_offsets_count; i++) {
 			if (mono_class_is_variant_compatible (itf, m_class_get_interfaces_packed (klass) [i], FALSE)) {
 				found = i;
-				*non_exact_match = TRUE;
+				*non_exact_match = (i != exact_match);
 				break;
 			}
 
@@ -1971,7 +1975,7 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo
 		for (i = 0; i < klass_interface_offsets_count; i++) {
 			if (mono_class_get_generic_type_definition (m_class_get_interfaces_packed (klass) [i]) == gtd) {
 				found = i;
-				*non_exact_match = TRUE;
+				*non_exact_match = (i != exact_match);
 				break;
 			}
 		}
@@ -1980,16 +1984,45 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo
 			return -1;
 
 		return m_class_get_interface_offsets_packed (klass) [found];
-	}
+	} else if (has_variance) {
+		MonoClass **vst;
+		int vst_count;
+		MonoClass *current = klass;
+
+		// Perform two passes per class, then check the base class
+		while (current) {
+			mono_class_get_variance_search_table (current, &vst, &vst_count);
+
+			// Exact match pass: Is there an exact match at this level of the type hierarchy?
+			// If so, we can use the interface_offset we computed earlier, since we're walking from most derived to least.
+			for (i = 0; i < vst_count; i++) {
+				if (itf != vst [i])
+					continue;
+
+				*non_exact_match = FALSE;
+				return exact_match;
+			}
 
-	if (!mono_class_has_variant_generic_params (itf))
-		return -1;
+			// Inexact match (variance) pass:
+			// Is any interface at this level of the type hierarchy variantly compatible with the desired interface?
+			// If so, select the first compatible one we find.
+			for (i = 0; i < vst_count; i++) {
+				if (!mono_class_is_variant_compatible (itf, vst [i], FALSE))
+					continue;
 
-	for (i = 0; i < klass_interface_offsets_count; i++) {
-		if (mono_class_is_variant_compatible (itf, m_class_get_interfaces_packed (klass) [i], FALSE)) {
-			*non_exact_match = TRUE;
-			return m_class_get_interface_offsets_packed (klass) [i];
+				int inexact_match = mono_class_interface_offset (klass, vst[i]);
+				g_assert (inexact_match != exact_match);
+				*non_exact_match = TRUE;
+				return inexact_match;
+			}
+
+			// Now check base class if present
+			current = m_class_get_parent (current);
 		}
+
+		// If the variance search failed to find a match, return the exact match search result (probably -1).
+		*non_exact_match = (exact_match < 0);
+		return exact_match;
 	}
 
 	return -1;

src/mono/mono/mini/interp/interp.c

@@ -619,7 +619,9 @@ get_virtual_method (InterpMethod *imethod, MonoVTable *vtable)
 		g_assert (vtable->klass != m->klass);
 		/* TODO: interface offset lookup is slow, go through IMT instead */
 		gboolean non_exact_match;
-		slot += mono_class_interface_offset_with_variance (vtable->klass, m->klass, &non_exact_match);
+		int ioffset = mono_class_interface_offset_with_variance (vtable->klass, m->klass, &non_exact_match);
+		g_assert (ioffset >= 0);
+		slot += ioffset;
 	}
 
 	MonoMethod *virtual_method = m_class_get_vtable (vtable->klass) [slot];

src/tests/Regressions/coreclr/103365/103365.cs

@@ -0,0 +1,51 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+/*
+https://github.com/dotnet/runtime/issues/103365
+When using an interface with a generic out type, an explicit implementation, and a derived class, the base classes implementation is called instead of the derived class when running on Mono; CoreCLR has the expected behavior.
+*/
+
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Runtime.CompilerServices;
+using Xunit;
+
+public interface IBaseInterface<out T>
+{
+    string explicitDeclaration();
+}
+
+public class BasicBaseClass : IBaseInterface<BasicBaseClass>
+{
+    string className = "BasicBaseClass";
+    string IBaseInterface<BasicBaseClass>.explicitDeclaration()
+    {
+        return className;
+    }
+}
+
+public class BasicDerivedClass : BasicBaseClass, IBaseInterface<BasicDerivedClass>
+{
+    string className = "BasicDerivedClass";
+
+    string IBaseInterface<BasicDerivedClass>.explicitDeclaration()
+    {
+        return className;
+    }
+}
+
+public static class Test_Issue103365
+{
+    [Fact]
+  	public static void Main ()
+	{
+        var instances = new IBaseInterface<BasicBaseClass>[2];
+        instances[0] = new BasicBaseClass();
+        instances[1] = new BasicDerivedClass();
+        Assert.Equal("BasicBaseClass", instances[0].explicitDeclaration());
+        Assert.Equal("BasicDerivedClass", instances[1].explicitDeclaration());
+  	}
+}
+

src/tests/Regressions/coreclr/103365/103365.csproj

@@ -0,0 +1,8 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <ItemGroup>
+    <Compile Include="103365.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <ProjectReference Include="$(TestSourceDir)Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" />
+  </ItemGroup>
+</Project>

src/tests/issues.targets

@@ -1756,7 +1756,7 @@
             <Issue>needs triage</Issue>
         </ExcludeList>
         <ExcludeList Include="$(XunitTestBinBase)/Regressions/coreclr/16355/variance/**">
-            <Issue>needs triage</Issue>
+            <Issue>ldtoken in default interface methods does not appear to work in minijit or interp</Issue>
         </ExcludeList>
         <ExcludeList Include="$(XunitTestBinBase)/Regressions/coreclr/20616/UnicodeBug/**">
             <Issue>needs triage</Issue>