Address PR feedback

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

@@ -4206,15 +4206,28 @@ index_of_class (MonoClass *needle, MonoVarianceSearchTableEntry *haystack, int h
 }
 
 static void
-build_variance_search_table_inner (MonoClass *klass, MonoVarianceSearchTableEntry *buf, int buf_size, int *buf_count, MonoClass *current) {
-	if (!m_class_is_interfaces_inited (current)) {
-		ERROR_DECL (error);
-		mono_class_setup_interfaces (current, error);
-		return_if_nok (error);
-	}
+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);
-	if (c) {
+		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];
@@ -4223,20 +4236,24 @@ build_variance_search_table_inner (MonoClass *klass, MonoVarianceSearchTableEntr
 				continue;
 
 			if (mono_class_has_variant_generic_params (iface)) {
-				int index = *buf_count;
-				(*buf_count) += 1;
-				g_assert (index < buf_size);
-				buf[index].klass = iface;
-				buf[index].depth = current->idepth;
-				buf[index].offset = mono_class_interface_offset (klass, iface);
+				append_variance_entry (klass, buf, buf_size, buf_count, iface);
+				result = TRUE;
 			}
 
-			build_variance_search_table_inner (klass, buf, buf_size, buf_count, iface);
+			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);
 		}
 	}
 
-	if (current->parent)
-		build_variance_search_table_inner (klass, buf, buf_size, buf_count, current->parent);
+	return result;
 }
 
 typedef struct VarianceSearchTable {
@@ -4247,13 +4264,13 @@ typedef struct VarianceSearchTable {
 // Only call this with the loader lock held
 static void
 build_variance_search_table (MonoClass *klass) {
-	int buf_size = m_class_get_interface_offsets_count (klass), buf_count = 0;
+	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(MonoClass *));
-	build_variance_search_table_inner (klass, buf, buf_size, &buf_count, klass);
+	memset (buf, 0, buf_size * sizeof(MonoVarianceSearchTableEntry));
+	gboolean any_items = build_variance_search_table_inner (klass, buf, buf_size, &buf_count, klass, TRUE);
 
-	if (buf_count) {
+	if (any_items) {
 		guint bytes = ((buf_count - 1) * sizeof(MonoVarianceSearchTableEntry)) + sizeof(VarianceSearchTable);
 		result = (VarianceSearchTable *)mono_mem_manager_alloc (m_class_get_mem_manager (klass), bytes);
 		result->count = buf_count;
@@ -4265,18 +4282,16 @@ build_variance_search_table (MonoClass *klass) {
 	klass->variant_search_table_inited = TRUE;
 }
 
-void
-mono_class_get_variance_search_table (MonoClass *klass, MonoVarianceSearchTableEntry **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) {
@@ -4288,11 +4303,11 @@ mono_class_get_variance_search_table (MonoClass *klass, MonoVarianceSearchTableE
 
 	VarianceSearchTable *vst = klass->variant_search_table;
 	if (vst) {
-		*table = vst->entries;
 		*table_size = vst->count;
+		return vst->entries;
 	} else {
-		*table = NULL;
 		*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, MonoVarianceSearchTableEntry **table, int *table_size);
+typedef struct _MonoVarianceSearchTableEntry {
+    int depth, offset;
+    MonoClass *klass;
+} 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.c

@@ -1985,58 +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) {
-		MonoVarianceSearchTableEntry *vst;
-		int vst_count;
-		mono_class_get_variance_search_table (klass, &vst, &vst_count);
-		int depth = vst_count ? vst[0].depth : 0, j;
-		i = 0;
-
-		while (depth) {
-			// g_print ("depth==%d, i==%d, count==%d\n", depth, i, 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 (j = i; j < vst_count; j++) {
-				if (vst [j].depth != depth)
+			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;
 
-				/*
-				char * cname = mono_type_get_full_name (vst [j].klass);
-				g_print ("  #%d: depth==%d %s\n", j, vst [j].depth, cname);
-				g_free (cname);
-				*/
-				if (itf != vst [j].klass)
+				if (itf != vst [i].klass)
 					continue;
 
 				*non_exact_match = FALSE;
-				g_assert (vst [j].offset == exact_match);
+				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 (j = i; j < vst_count; j++) {
-				if (vst [j].depth < depth) {
-					i = j;
+			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;
 				}
 
-				/*
-				char * cname = mono_type_get_full_name (vst [j].klass);
-				g_print ("  #%d: depth==%d %s\n", j, vst [j].depth, cname);
-				g_free (cname);
-				*/
-				if (!mono_class_is_variant_compatible (itf, vst [j].klass, FALSE))
+				if (!mono_class_is_variant_compatible (itf, vst [i].klass, FALSE))
 					continue;
 
-				int inexact_match = vst [j].offset;
-				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;
 			}
-
-			depth--;
 		}
 
 		// If the variance search failed to find a match, return the exact match search result (probably -1).

src/native/public/mono/metadata/object-forward.h

@@ -19,9 +19,4 @@ typedef struct _MonoException MONO_RT_MANAGED_ATTR MonoException;
 typedef struct _MonoReflectionAssembly MONO_RT_MANAGED_ATTR MonoReflectionAssembly;
 typedef struct _MonoReflectionTypeBuilder MONO_RT_MANAGED_ATTR MonoReflectionTypeBuilder;
 
-typedef struct _MonoVarianceSearchTableEntry {
-    int depth, offset;
-    MonoClass *klass;
-} MonoVarianceSearchTableEntry;
-
 #endif /* __MONO_OBJECT_FORWARD_H__ */