[mono] Issue #103365 follow-up PR

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

@@ -4197,75 +4197,101 @@ mono_class_set_runtime_vtable (MonoClass *klass, MonoVTable *vtable)
 }
 
 static int
-index_of_class (MonoClass *needle, MonoClass **haystack, int haystack_size) {
+index_of_class (MonoClass *needle, MonoVarianceSearchTableEntry *haystack, int haystack_size) {
 	for (int i = 0; i < haystack_size; i++)
-		if (haystack[i] == needle)
+		if (haystack[i].klass == 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);
+append_variance_entry (MonoClass *klass, MonoVarianceSearchTableEntry *buf, int buf_size, int *buf_count, MonoClass *value) {
+	int index = *buf_count;
+	(*buf_count) += 1;
+	g_assert (index < buf_size);
+	buf[index].klass = value;
+	buf[index].offset = value ? mono_class_interface_offset (klass, value) : 0;
+}
+
+static gboolean
+build_variance_search_table_inner (MonoClass *klass, MonoVarianceSearchTableEntry *buf, int buf_size, int *buf_count, MonoClass *current, gboolean terminator) {
+	// We have to track separately whether the buffer contains any actual interfaces, since we're appending
+	//  null terminators between levels of the inheritance hierarchy
+	gboolean result = FALSE;
+
+	while (current) {
+		if (!m_class_is_interfaces_inited (current)) {
+			ERROR_DECL (error);
+			mono_class_setup_interfaces (current, error);
+			return_val_if_nok (error, FALSE);
+		}
+
+		guint c = m_class_get_interface_count (current);
+		MonoClass **ifaces = m_class_get_interfaces (current);
 		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;
+				append_variance_entry (klass, buf, buf_size, buf_count, iface);
+				result = TRUE;
 			}
 
-			build_variance_search_table_inner (iface, buf, buf_size, buf_count);
+			if (build_variance_search_table_inner (klass, buf, buf_size, buf_count, iface, FALSE))
+				result = TRUE;
+		}
+
+		current = current->parent;
+
+		if (terminator) {
+			// HACK: Don't append another NULL if we already have one, it's unnecessary
+			if (*buf_count && buf[(*buf_count) - 1].klass != NULL)
+				append_variance_entry (klass, buf, buf_size, buf_count, NULL);
 		}
 	}
+
+	return result;
 }
 
+typedef struct VarianceSearchTable {
+	int count;
+	MonoVarianceSearchTableEntry entries[1]; // a total of count items, at least 1
+} VarianceSearchTable;
+
 // 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);
+	int buf_size = m_class_get_interface_offsets_count (klass) + klass->idepth + 1, buf_count = 0;
+	MonoVarianceSearchTableEntry *buf = g_alloca (buf_size * sizeof(MonoVarianceSearchTableEntry));
+	VarianceSearchTable *result = NULL;
+	memset (buf, 0, buf_size * sizeof(MonoVarianceSearchTableEntry));
+	gboolean any_items = build_variance_search_table_inner (klass, buf, buf_size, &buf_count, klass, TRUE);
+
+	if (any_items) {
+		guint bytes = (buf_count * sizeof(MonoVarianceSearchTableEntry));
+		result = (VarianceSearchTable *)mono_mem_manager_alloc (m_class_get_mem_manager (klass), bytes + sizeof(VarianceSearchTable));
+		result->count = buf_count;
+		memcpy (result->entries, 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) {
+MonoVarianceSearchTableEntry *
+mono_class_get_variance_search_table (MonoClass *klass, 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;
+		return NULL;
 	}
 
 	if (!klass->variant_search_table_inited) {
@@ -4275,8 +4301,14 @@ mono_class_get_variance_search_table (MonoClass *klass, MonoClass ***table, int
 		mono_loader_unlock ();
 	}
 
-	*table = klass->variant_search_table;
-	*table_size = klass->variant_search_table_length;
+	VarianceSearchTable *vst = klass->variant_search_table;
+	if (vst) {
+		*table_size = vst->count;
+		return vst->entries;
+	} else {
+		*table_size = 0;
+		return NULL;
+	}
 }
 
 /**

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

@@ -1465,8 +1465,13 @@ 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);
+typedef struct _MonoVarianceSearchTableEntry {
+    MonoClass *klass;
+    guint16 offset;
+} MonoVarianceSearchTableEntry;
+
+MonoVarianceSearchTableEntry *
+mono_class_get_variance_search_table (MonoClass *klass, int *table_size);
 
 // There are many ways to do on-demand initialization.
 //   Some allow multiple concurrent initializations. Some do not.

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

@@ -129,8 +129,7 @@ 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;
+	void *variant_search_table;
 };
 
 struct _MonoClassDef {

src/mono/mono/metadata/class.c

@@ -1985,39 +1985,49 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo
 
 		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);
-
+		int vst_count, offset = 0;
+		MonoVarianceSearchTableEntry *vst = mono_class_get_variance_search_table (klass, &vst_count);
+
+		// The variance search table is a buffer containing all interfaces with in/out params in the type's inheritance
+		//  hierarchy, with a NULL separator between each level of the hierarchy. This allows us to skip recursing down
+		//  the whole chain and avoid performing duplicate compatibility checks, since duplicates are stripped from the
+		//  buffer. To comply with the spec, we do an exact-match pass and then a variance pass for each level in the
+		//  hierarchy, then move on to the next level and do two passes for that one.
+		while (offset < 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])
+			for (i = offset; i < vst_count; i++) {
+				// If we hit a null, that's the next level in the inheritance hierarchy, so stop there
+				if (vst [i].klass == NULL)
+					break;
+
+				if (itf != vst [i].klass)
 					continue;
 
 				*non_exact_match = FALSE;
+				g_assert (vst [i].offset == exact_match);
 				return exact_match;
 			}
 
 			// 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))
+			for (i = offset; i < vst_count; i++) {
+				// If we hit a null, that's the next level in the inheritance hierarchy, so bump offset past it
+				if (vst [i].klass == NULL) {
+					offset = i + 1;
+					break;
+				}
+
+				if (!mono_class_is_variant_compatible (itf, vst [i].klass, FALSE))
 					continue;
 
-				int inexact_match = mono_class_interface_offset (klass, vst[i]);
-				g_assert (inexact_match != exact_match);
-				*non_exact_match = TRUE;
+				int inexact_match = vst [i].offset;
+				// FIXME: Is it correct that this is possible?
+				// g_assert (inexact_match != exact_match);
+				*non_exact_match = inexact_match != exact_match;
 				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).

src/mono/mono/metadata/loader.c

@@ -1303,6 +1303,7 @@ get_method_constrained (MonoImage *image, MonoMethod *method, MonoClass *constra
 		g_assert (itf_slot >= 0);
 		gboolean variant = FALSE;
 		int itf_base = mono_class_interface_offset_with_variance (constrained_class, base_class, &variant);
+		g_assert (itf_base >= 0);
 		vtable_slot = itf_slot + itf_base;
 	}
 	g_assert (vtable_slot >= 0);

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

@@ -625,6 +625,7 @@ get_virtual_method (InterpMethod *imethod, MonoVTable *vtable)
 	}
 
 	MonoMethod *virtual_method = m_class_get_vtable (vtable->klass) [slot];
+	g_assert (virtual_method);
 	if (m->is_inflated && mono_method_get_context (m)->method_inst) {
 		MonoGenericContext context = { NULL, NULL };
 
@@ -647,6 +648,32 @@ get_virtual_method (InterpMethod *imethod, MonoVTable *vtable)
 		virtual_method = mono_marshal_get_synchronized_wrapper (virtual_method);
 	}
 
+	// Basic sanity check since a call might crash if we have the wrong method somehow
+
+	MonoMethodSignature *m_sig, *virtual_method_sig;
+	{
+		ERROR_DECL (error);
+		m_sig = mono_method_signature_checked (m, error);
+		mono_error_cleanup (error);
+	}
+	{
+		ERROR_DECL (error);
+		virtual_method_sig = mono_method_signature_checked (virtual_method, error);
+		mono_error_cleanup (error);
+	}
+	gboolean ok = m_sig &&
+		virtual_method_sig &&
+		(m_sig->param_count == virtual_method_sig->param_count) &&
+		(m_sig->hasthis == virtual_method_sig->hasthis);
+	if (!ok) {
+		g_print (
+			"get_virtual_method signature mismatch for %s.%s and %s.%s\n",
+			m_class_get_name (m->klass), m->name,
+			m_class_get_name (virtual_method->klass), virtual_method->name
+		);
+		g_assert (ok);
+	}
+
 	InterpMethod *virtual_imethod = mono_interp_get_imethod (virtual_method);
 	return virtual_imethod;
 }
@@ -7479,7 +7506,7 @@ MINT_IN_CASE(MINT_BRTRUE_I8_SP) ZEROP_SP(gint64, !=); MINT_IN_BREAK;
 			if (local_cmethod->is_generic || mono_class_is_gtd (local_cmethod->klass)) {
 				MonoException *ex = mono_exception_from_name_msg (mono_defaults.corlib, "System", "InvalidOperationException", "");
 				THROW_EX (ex, ip);
-			}			
+			}
 
 			// FIXME push/pop LMF
 			if (G_UNLIKELY (mono_method_has_unmanaged_callers_only_attribute (local_cmethod))) {