[runtime] Implement support for conflict detection for Default Interf…

…ace Methods. (mono/mono#6897)

Detect conflicts caused by diamond inheritance during vtable construction, and throw an exception when a conflicting
methods is called.
Disable generic sharing for default interface methods, it doesn't work yet, probably
because these methods are called on classes, but the gshared information is associated with the interface.
Enable dim-diamondshape.exe test.

Commit migrated from mono/mono@5c4510a

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

@@ -21,7 +21,8 @@ typedef enum {
 	PROP_EVENT_INFO = 6, /* MonoClassEventInfo* */
 	PROP_FIELD_DEF_VALUES = 7, /* MonoFieldDefaultValue* */
 	PROP_DECLSEC_FLAGS = 8, /* guint32 */
-	PROP_WEAK_BITMAP = 9
+	PROP_WEAK_BITMAP = 9,
+	PROP_DIM_CONFLICTS = 10 /* GSList of MonoMethod* */
 }  InfrequentDataKind;
 
 /* Accessors based on class kind*/
@@ -105,7 +106,6 @@ mono_class_try_get_generic_container (MonoClass *klass)
 	return NULL;
 }
 
-
 void
 mono_class_set_generic_container (MonoClass *klass, MonoGenericContainer *container)
 {
@@ -424,6 +424,51 @@ mono_class_get_weak_bitmap (MonoClass *klass, int *nbits)
 	return prop->bits;
 }
 
+gboolean
+mono_class_has_dim_conflicts (MonoClass *klass)
+{
+	if (klass->has_dim_conflicts)
+		return TRUE;
+
+	if (mono_class_is_ginst (klass)) {
+		MonoClass *gklass = mono_class_get_generic_class (klass)->container_class;
+
+		return gklass->has_dim_conflicts;
+	}
+
+	return FALSE;
+}
+
+typedef struct {
+	MonoPropertyBagItem head;
+
+	GSList *data;
+} DimConflictData;
+
+void
+mono_class_set_dim_conflicts (MonoClass *klass, GSList *conflicts)
+{
+	DimConflictData *info = mono_class_alloc (klass, sizeof (DimConflictData));
+	info->data = conflicts;
+
+	g_assert (!mono_class_is_ginst (klass));
+
+	info->head.tag = PROP_DIM_CONFLICTS;
+	mono_property_bag_add (&klass->infrequent_data, info);
+}
+
+GSList*
+mono_class_get_dim_conflicts (MonoClass *klass)
+{
+	if (mono_class_is_ginst (klass))
+		return mono_class_get_dim_conflicts (mono_class_get_generic_class (klass)->container_class);
+
+	DimConflictData *info = mono_property_bag_get (&klass->infrequent_data, PROP_DIM_CONFLICTS);
+
+	g_assert (info);
+	return info->data;
+}
+
 #ifdef MONO_CLASS_DEF_PRIVATE
 #define MONO_CLASS_GETTER(funcname, rettype, optref, argtype, fieldname) rettype funcname (argtype *klass) { return optref klass-> fieldname ; }
 #include "class-getters.h"

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

@@ -48,6 +48,7 @@ MONO_CLASS_GETTER(m_class_is_has_finalize_inited, gboolean, , MonoClass, has_fin
 MONO_CLASS_GETTER(m_class_is_fields_inited, gboolean, , MonoClass, fields_inited)
 MONO_CLASS_GETTER(m_class_has_failure, gboolean, , MonoClass, has_failure)
 MONO_CLASS_GETTER(m_class_has_weak_fields, gboolean, , MonoClass, has_weak_fields)
+MONO_CLASS_GETTER(m_class_has_dim_conflicts, gboolean, , MonoClass, has_dim_conflicts)
 MONO_CLASS_GETTER(m_class_get_parent, MonoClass *, , MonoClass, parent)
 MONO_CLASS_GETTER(m_class_get_nested_in, MonoClass *, ,  MonoClass, nested_in)
 MONO_CLASS_GETTER(m_class_get_image, MonoImage *, , MonoClass, image)

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

@@ -2498,7 +2498,8 @@ mono_class_need_stelemref_method (MonoClass *klass)
 }
 
 static int
-apply_override (MonoClass *klass, MonoMethod **vtable, MonoMethod *decl, MonoMethod *override)
+apply_override (MonoClass *klass, MonoClass *override_class, MonoMethod **vtable, MonoMethod *decl, MonoMethod *override,
+				GHashTable **override_map, GHashTable **override_class_map, GHashTable **conflict_map)
 {
 	int dslot;
 	dslot = mono_method_get_vtable_slot (decl);
@@ -2517,12 +2518,138 @@ apply_override (MonoClass *klass, MonoMethod **vtable, MonoMethod *decl, MonoMet
 		vtable [dslot]->slot = dslot;
 	}
 
-	if (mono_security_core_clr_enabled ())
-		mono_security_core_clr_check_override (klass, vtable [dslot], decl);
+	if (!*override_map) {
+		*override_map = g_hash_table_new (mono_aligned_addr_hash, NULL);
+		*override_class_map = g_hash_table_new (mono_aligned_addr_hash, NULL);
+	}
+	GHashTable *map = *override_map;
+	GHashTable *class_map = *override_class_map;
+
+	MonoMethod *prev_override = g_hash_table_lookup (map, decl);
+	MonoClass *prev_override_class = g_hash_table_lookup (class_map, decl);
+
+	g_hash_table_insert (map, decl, override);
+	g_hash_table_insert (class_map, decl, override_class);
+
+	/* Collect potentially conflicting overrides which are introduced by default interface methods */
+	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<T> in:
+		 * class Foo<T> : IFace<T>
+		 * This is needed so the mono_class_is_assignable_from () calls in the
+		 * conflict resolution work.
+		 */
+		if (mono_class_is_ginst (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)) {
+			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);
+		GHashTable *cmap = *conflict_map;
+		GSList *entries = g_hash_table_lookup (cmap, decl);
+		if (!(decl->flags & METHOD_ATTRIBUTE_ABSTRACT))
+			entries = g_slist_prepend (entries, decl);
+		entries = g_slist_prepend (entries, prev_override);
+		entries = g_slist_prepend (entries, override);
+
+		g_hash_table_insert (cmap, decl, entries);
+	}
 
 	return TRUE;
 }
 
+static void
+handle_dim_conflicts (MonoMethod **vtable, MonoClass *klass, GHashTable *conflict_map)
+{
+	GHashTableIter iter;
+	MonoMethod *decl;
+	GSList *entries, *l, *l2;
+	GSList *dim_conflicts = NULL;
+
+	g_hash_table_iter_init (&iter, conflict_map);
+	while (g_hash_table_iter_next (&iter, (gpointer*)&decl, (gpointer*)&entries)) {
+		/*
+		 * Iterate over the candidate methods, remove ones whose class is less concrete than the
+		 * class of another one.
+		 */
+		/* This is O(n^2), but that shouldn't be a problem in practice */
+		for (l = entries; l; l = l->next) {
+			for (l2 = entries; l2; l2 = l2->next) {
+				MonoMethod *m1 = l->data;
+				MonoMethod *m2 = l2->data;
+				if (!m1 || !m2 || m1 == m2)
+					continue;
+				if (mono_class_is_assignable_from (m1->klass, m2->klass))
+					l->data = NULL;
+				else if (mono_class_is_assignable_from (m2->klass, m1->klass))
+					l2->data = NULL;
+			}
+		}
+		int nentries = 0;
+		MonoMethod *impl = NULL;
+		for (l = entries; l; l = l->next) {
+			if (l->data) {
+				nentries ++;
+				impl = l->data;
+			}
+		}
+		if (nentries > 1) {
+			/* If more than one method is left, we have a conflict */
+			if (decl->is_inflated)
+				decl = ((MonoMethodInflated*)decl)->declaring;
+			dim_conflicts = g_slist_prepend (dim_conflicts, decl);
+			/*
+			  for (l = entries; l; l = l->next) {
+			  if (l->data)
+			  printf ("%s %s %s\n", mono_class_full_name (klass), mono_method_full_name (decl, TRUE), mono_method_full_name (l->data, TRUE));
+			  }
+			*/
+		} else {
+			/*
+			 * Use the implementing method computed above instead of the already
+			 * computed one, which depends on interface ordering.
+			 */
+			int ic_offset = mono_class_interface_offset (klass, decl->klass);
+			int im_slot = ic_offset + decl->slot;
+			vtable [im_slot] = impl;
+		}
+		g_slist_free (entries);
+	}
+	if (dim_conflicts) {
+		mono_loader_lock ();
+		klass->has_dim_conflicts = 1;
+		mono_loader_unlock ();
+
+		/*
+		 * Exceptions are thrown at method call time and only for the methods which have
+		 * conflicts, so just save them in the class.
+		 */
+
+		/* Make a copy of the list from the class mempool */
+		GSList *conflicts = mono_class_alloc0 (klass, g_slist_length (dim_conflicts) * sizeof (GSList));
+		int i = 0;
+		for (l = dim_conflicts; l; l = l->next) {
+			conflicts [i].data = l->data;
+			conflicts [i].next = &conflicts [i + 1];
+			i ++;
+		}
+		conflicts [i - 1].next = NULL;
+
+		mono_class_set_dim_conflicts (klass, conflicts);
+		g_slist_free (dim_conflicts);
+	}
+}
+
 static void
 print_unimplemented_interface_method_info (MonoClass *klass, MonoClass *ic, MonoMethod *im, int im_slot, MonoMethod **overrides, int onum)
 {
@@ -2664,6 +2791,8 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
 	guint32 max_iid;
 	GPtrArray *ifaces = NULL;
 	GHashTable *override_map = NULL;
+	GHashTable *override_class_map = NULL;
+	GHashTable *conflict_map = NULL;
 	MonoMethod *cm;
 #if (DEBUG_INTERFACE_VTABLE_CODE|TRACE_INTERFACE_VTABLE_CODE)
 	int first_non_interface_slot;
@@ -2746,6 +2875,10 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
 		mono_memory_barrier ();
 		klass->vtable = tmp;
 
+		mono_loader_lock ();
+		klass->has_dim_conflicts = gklass->has_dim_conflicts;
+		mono_loader_unlock ();
+
 		/* Have to set method->slot for abstract virtual methods */
 		if (klass->methods && gklass->methods) {
 			int mcount = mono_class_get_method_count (klass);
@@ -2823,12 +2956,8 @@ 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 (!apply_override (klass, vtable, decl, override))
+			if (!apply_override (klass, ic, vtable, decl, override, &override_map, &override_class_map, &conflict_map))
 				goto fail;
-
-			if (!override_map)
-				override_map = g_hash_table_new (mono_aligned_addr_hash, NULL);
-			g_hash_table_insert (override_map, decl, override);
 		}
 		g_free (iface_overrides);
 	}
@@ -2838,12 +2967,8 @@ 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 (decl->klass)) {
-			if (!apply_override (klass, vtable, decl, override))
+			if (!apply_override (klass, klass, vtable, decl, override, &override_map, &override_class_map, &conflict_map))
 				goto fail;
-
-			if (!override_map)
-				override_map = g_hash_table_new (mono_aligned_addr_hash, NULL);
-			g_hash_table_insert (override_map, decl, override);
 		}
 	}
 
@@ -3108,6 +3233,14 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
 		override_map = NULL;
 	}
 
+	if (override_class_map)
+		g_hash_table_destroy (override_class_map);
+
+	if (conflict_map) {
+		handle_dim_conflicts (vtable, klass, conflict_map);
+		g_hash_table_destroy (conflict_map);
+	}
+
 	g_slist_free (virt_methods);
 	virt_methods = NULL;
 

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

@@ -1374,6 +1374,15 @@ mono_class_set_weak_bitmap (MonoClass *klass, int nbits, gsize *bits);
 gsize*
 mono_class_get_weak_bitmap (MonoClass *klass, int *nbits);
 
+gboolean
+mono_class_has_dim_conflicts (MonoClass *klass);
+
+void
+mono_class_set_dim_conflicts (MonoClass *klass, GSList *conflicts);
+
+GSList*
+mono_class_get_dim_conflicts (MonoClass *klass);
+
 MonoMethod *
 mono_class_get_method_from_name_checked (MonoClass *klass, const char *name, int param_count, int flags, MonoError *error);
 

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

@@ -80,6 +80,7 @@ struct _MonoClass {
 	guint fields_inited : 1; /* setup_fields () has finished */
 	guint has_failure : 1; /* See mono_class_get_exception_data () for a MonoErrorBoxed with the details */
 	guint has_weak_fields : 1; /* class has weak reference fields */
+	guint has_dim_conflicts : 1; /* Class has conflicting default interface methods */
 
 	MonoClass  *parent;
 	MonoClass  *nested_in;

src/mono/mono/mini/method-to-ir.c

@@ -2914,16 +2914,6 @@ emit_get_rgctx (MonoCompile *cfg, int context_used)
 		mrgctx_loc = mono_get_vtable_var (cfg);
 		EMIT_NEW_TEMPLOAD (cfg, mrgctx_var, mrgctx_loc->inst_c0);
 
-		return mrgctx_var;
-	} else if (MONO_CLASS_IS_INTERFACE (cfg->method->klass)) {
-		MonoInst *mrgctx_loc, *mrgctx_var;
-
-		/* Default interface methods need an mrgctx since the vtabke at runtime points at an implementing class */
-		mrgctx_loc = mono_get_vtable_var (cfg);
-		EMIT_NEW_TEMPLOAD (cfg, mrgctx_var, mrgctx_loc->inst_c0);
-
-		g_assert (mono_method_needs_static_rgctx_invoke (cfg->method, TRUE));
-
 		return mrgctx_var;
 	} else if (method->flags & METHOD_ATTRIBUTE_STATIC || method->klass->valuetype) {
 		MonoInst *vtable_loc, *vtable_var;

src/mono/mono/mini/mini-generic-sharing.c

@@ -2894,6 +2894,10 @@ mono_method_is_generic_sharable_full (MonoMethod *method, gboolean allow_type_va
 	if (!mono_method_is_generic_impl (method))
 		return FALSE;
 
+	if (MONO_CLASS_IS_INTERFACE (method->klass))
+		/* Default Interface Methods don't work yet with gshared */
+		return FALSE;
+
 	/*
 	if (!mono_debug_count ())
 		allow_partial = FALSE;

src/mono/mono/mini/mini-trampolines.c

@@ -574,6 +574,31 @@ common_call_trampoline (mgreg_t *regs, guint8 *code, MonoMethod *m, MonoVTable *
 			vtable_slot = mini_resolve_imt_method (vt, vtable_slot, imt_method, &impl_method, &addr, &need_rgctx_tramp, &variant_iface, error);
 			return_val_if_nok (error, NULL);
 
+			if (mono_class_has_dim_conflicts (vt->klass)) {
+				GSList *conflicts = mono_class_get_dim_conflicts (vt->klass);
+				GSList *l;
+				MonoMethod *decl = imt_method;
+
+				if (decl->is_inflated)
+					decl = mono_method_get_declaring_generic_method (decl);
+
+				gboolean in_conflict = FALSE;
+				for (l = conflicts; l; l = l->next) {
+					if (decl == l->data) {
+						in_conflict = TRUE;
+						break;
+					}
+				}
+				if (in_conflict) {
+					char *class_name = mono_class_full_name (vt->klass);
+					char *method_name = mono_method_full_name (decl, TRUE);
+					mono_error_set_not_supported (error, "Interface method '%s' in class '%s' has multiple candidate implementations.", method_name, class_name);
+					g_free (class_name);
+					g_free (method_name);
+					return NULL;
+				}
+			}
+
 			/* We must handle magic interfaces on rank 1 arrays of ref types as if they were variant */
 			if (!variant_iface && vt->klass->rank == 1 && !vt->klass->element_class->valuetype && imt_method->klass->is_array_special_interface)
 				variant_iface = imt_method;