Skip to content

Commit 5c4510a

Browse files
vargazvargaz
authored
[runtime] Implement support for conflict detection for Default Interface Methods. (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.
1 parent 284c5af commit 5c4510a

File tree

9 files changed

+240
-29
lines changed

9 files changed

+240
-29
lines changed

‎mono/metadata/class-accessors.c‎

Lines changed: 47 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ typedef enum {
2121
PROP_EVENT_INFO = 6, /* MonoClassEventInfo* */
2222
PROP_FIELD_DEF_VALUES = 7, /* MonoFieldDefaultValue* */
2323
PROP_DECLSEC_FLAGS = 8, /* guint32 */
24-
PROP_WEAK_BITMAP = 9
24+
PROP_WEAK_BITMAP = 9,
25+
PROP_DIM_CONFLICTS = 10 /* GSList of MonoMethod* */
2526
} InfrequentDataKind;
2627

2728
/* Accessors based on class kind*/
@@ -105,7 +106,6 @@ mono_class_try_get_generic_container (MonoClass *klass)
105106
return NULL;
106107
}
107108

108-
109109
void
110110
mono_class_set_generic_container (MonoClass *klass, MonoGenericContainer *container)
111111
{
@@ -424,6 +424,51 @@ mono_class_get_weak_bitmap (MonoClass *klass, int *nbits)
424424
return prop->bits;
425425
}
426426

427+
gboolean
428+
mono_class_has_dim_conflicts (MonoClass *klass)
429+
{
430+
if (klass->has_dim_conflicts)
431+
return TRUE;
432+
433+
if (mono_class_is_ginst (klass)) {
434+
MonoClass *gklass = mono_class_get_generic_class (klass)->container_class;
435+
436+
return gklass->has_dim_conflicts;
437+
}
438+
439+
return FALSE;
440+
}
441+
442+
typedef struct {
443+
MonoPropertyBagItem head;
444+
445+
GSList *data;
446+
} DimConflictData;
447+
448+
void
449+
mono_class_set_dim_conflicts (MonoClass *klass, GSList *conflicts)
450+
{
451+
DimConflictData *info = mono_class_alloc (klass, sizeof (DimConflictData));
452+
info->data = conflicts;
453+
454+
g_assert (!mono_class_is_ginst (klass));
455+
456+
info->head.tag = PROP_DIM_CONFLICTS;
457+
mono_property_bag_add (&klass->infrequent_data, info);
458+
}
459+
460+
GSList*
461+
mono_class_get_dim_conflicts (MonoClass *klass)
462+
{
463+
if (mono_class_is_ginst (klass))
464+
return mono_class_get_dim_conflicts (mono_class_get_generic_class (klass)->container_class);
465+
466+
DimConflictData *info = mono_property_bag_get (&klass->infrequent_data, PROP_DIM_CONFLICTS);
467+
468+
g_assert (info);
469+
return info->data;
470+
}
471+
427472
#ifdef MONO_CLASS_DEF_PRIVATE
428473
#define MONO_CLASS_GETTER(funcname, rettype, optref, argtype, fieldname) rettype funcname (argtype *klass) { return optref klass-> fieldname ; }
429474
#include "class-getters.h"

‎mono/metadata/class-getters.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ MONO_CLASS_GETTER(m_class_is_has_finalize_inited, gboolean, , MonoClass, has_fin
4848
MONO_CLASS_GETTER(m_class_is_fields_inited, gboolean, , MonoClass, fields_inited)
4949
MONO_CLASS_GETTER(m_class_has_failure, gboolean, , MonoClass, has_failure)
5050
MONO_CLASS_GETTER(m_class_has_weak_fields, gboolean, , MonoClass, has_weak_fields)
51+
MONO_CLASS_GETTER(m_class_has_dim_conflicts, gboolean, , MonoClass, has_dim_conflicts)
5152
MONO_CLASS_GETTER(m_class_get_parent, MonoClass *, , MonoClass, parent)
5253
MONO_CLASS_GETTER(m_class_get_nested_in, MonoClass *, , MonoClass, nested_in)
5354
MONO_CLASS_GETTER(m_class_get_image, MonoImage *, , MonoClass, image)

‎mono/metadata/class-init.c‎

Lines changed: 146 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2498,7 +2498,8 @@ mono_class_need_stelemref_method (MonoClass *klass)
24982498
}
24992499

25002500
static int
2501-
apply_override (MonoClass *klass, MonoMethod **vtable, MonoMethod *decl, MonoMethod *override)
2501+
apply_override (MonoClass *klass, MonoClass *override_class, MonoMethod **vtable, MonoMethod *decl, MonoMethod *override,
2502+
GHashTable **override_map, GHashTable **override_class_map, GHashTable **conflict_map)
25022503
{
25032504
int dslot;
25042505
dslot = mono_method_get_vtable_slot (decl);
@@ -2517,12 +2518,138 @@ apply_override (MonoClass *klass, MonoMethod **vtable, MonoMethod *decl, MonoMet
25172518
vtable [dslot]->slot = dslot;
25182519
}
25192520

2520-
if (mono_security_core_clr_enabled ())
2521-
mono_security_core_clr_check_override (klass, vtable [dslot], decl);
2521+
if (!*override_map) {
2522+
*override_map = g_hash_table_new (mono_aligned_addr_hash, NULL);
2523+
*override_class_map = g_hash_table_new (mono_aligned_addr_hash, NULL);
2524+
}
2525+
GHashTable *map = *override_map;
2526+
GHashTable *class_map = *override_class_map;
2527+
2528+
MonoMethod *prev_override = g_hash_table_lookup (map, decl);
2529+
MonoClass *prev_override_class = g_hash_table_lookup (class_map, decl);
2530+
2531+
g_hash_table_insert (map, decl, override);
2532+
g_hash_table_insert (class_map, decl, override_class);
2533+
2534+
/* Collect potentially conflicting overrides which are introduced by default interface methods */
2535+
if (prev_override) {
2536+
ERROR_DECL (error);
2537+
2538+
/*
2539+
* The override methods are part of the generic definition, need to inflate them so their
2540+
* parent class becomes the actual interface/class containing the override, i.e.
2541+
* IFace<T> in:
2542+
* class Foo<T> : IFace<T>
2543+
* This is needed so the mono_class_is_assignable_from () calls in the
2544+
* conflict resolution work.
2545+
*/
2546+
if (mono_class_is_ginst (override_class)) {
2547+
override = mono_class_inflate_generic_method_checked (override, &mono_class_get_generic_class (override_class)->context, error);
2548+
mono_error_assert_ok (error);
2549+
}
2550+
2551+
if (mono_class_is_ginst (prev_override_class)) {
2552+
prev_override = mono_class_inflate_generic_method_checked (prev_override, &mono_class_get_generic_class (prev_override_class)->context, error);
2553+
mono_error_assert_ok (error);
2554+
}
2555+
2556+
if (!*conflict_map)
2557+
*conflict_map = g_hash_table_new (mono_aligned_addr_hash, NULL);
2558+
GHashTable *cmap = *conflict_map;
2559+
GSList *entries = g_hash_table_lookup (cmap, decl);
2560+
if (!(decl->flags & METHOD_ATTRIBUTE_ABSTRACT))
2561+
entries = g_slist_prepend (entries, decl);
2562+
entries = g_slist_prepend (entries, prev_override);
2563+
entries = g_slist_prepend (entries, override);
2564+
2565+
g_hash_table_insert (cmap, decl, entries);
2566+
}
25222567

25232568
return TRUE;
25242569
}
25252570

2571+
static void
2572+
handle_dim_conflicts (MonoMethod **vtable, MonoClass *klass, GHashTable *conflict_map)
2573+
{
2574+
GHashTableIter iter;
2575+
MonoMethod *decl;
2576+
GSList *entries, *l, *l2;
2577+
GSList *dim_conflicts = NULL;
2578+
2579+
g_hash_table_iter_init (&iter, conflict_map);
2580+
while (g_hash_table_iter_next (&iter, (gpointer*)&decl, (gpointer*)&entries)) {
2581+
/*
2582+
* Iterate over the candidate methods, remove ones whose class is less concrete than the
2583+
* class of another one.
2584+
*/
2585+
/* This is O(n^2), but that shouldn't be a problem in practice */
2586+
for (l = entries; l; l = l->next) {
2587+
for (l2 = entries; l2; l2 = l2->next) {
2588+
MonoMethod *m1 = l->data;
2589+
MonoMethod *m2 = l2->data;
2590+
if (!m1 || !m2 || m1 == m2)
2591+
continue;
2592+
if (mono_class_is_assignable_from (m1->klass, m2->klass))
2593+
l->data = NULL;
2594+
else if (mono_class_is_assignable_from (m2->klass, m1->klass))
2595+
l2->data = NULL;
2596+
}
2597+
}
2598+
int nentries = 0;
2599+
MonoMethod *impl = NULL;
2600+
for (l = entries; l; l = l->next) {
2601+
if (l->data) {
2602+
nentries ++;
2603+
impl = l->data;
2604+
}
2605+
}
2606+
if (nentries > 1) {
2607+
/* If more than one method is left, we have a conflict */
2608+
if (decl->is_inflated)
2609+
decl = ((MonoMethodInflated*)decl)->declaring;
2610+
dim_conflicts = g_slist_prepend (dim_conflicts, decl);
2611+
/*
2612+
for (l = entries; l; l = l->next) {
2613+
if (l->data)
2614+
printf ("%s %s %s\n", mono_class_full_name (klass), mono_method_full_name (decl, TRUE), mono_method_full_name (l->data, TRUE));
2615+
}
2616+
*/
2617+
} else {
2618+
/*
2619+
* Use the implementing method computed above instead of the already
2620+
* computed one, which depends on interface ordering.
2621+
*/
2622+
int ic_offset = mono_class_interface_offset (klass, decl->klass);
2623+
int im_slot = ic_offset + decl->slot;
2624+
vtable [im_slot] = impl;
2625+
}
2626+
g_slist_free (entries);
2627+
}
2628+
if (dim_conflicts) {
2629+
mono_loader_lock ();
2630+
klass->has_dim_conflicts = 1;
2631+
mono_loader_unlock ();
2632+
2633+
/*
2634+
* Exceptions are thrown at method call time and only for the methods which have
2635+
* conflicts, so just save them in the class.
2636+
*/
2637+
2638+
/* Make a copy of the list from the class mempool */
2639+
GSList *conflicts = mono_class_alloc0 (klass, g_slist_length (dim_conflicts) * sizeof (GSList));
2640+
int i = 0;
2641+
for (l = dim_conflicts; l; l = l->next) {
2642+
conflicts [i].data = l->data;
2643+
conflicts [i].next = &conflicts [i + 1];
2644+
i ++;
2645+
}
2646+
conflicts [i - 1].next = NULL;
2647+
2648+
mono_class_set_dim_conflicts (klass, conflicts);
2649+
g_slist_free (dim_conflicts);
2650+
}
2651+
}
2652+
25262653
static void
25272654
print_unimplemented_interface_method_info (MonoClass *klass, MonoClass *ic, MonoMethod *im, int im_slot, MonoMethod **overrides, int onum)
25282655
{
@@ -2664,6 +2791,8 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
26642791
guint32 max_iid;
26652792
GPtrArray *ifaces = NULL;
26662793
GHashTable *override_map = NULL;
2794+
GHashTable *override_class_map = NULL;
2795+
GHashTable *conflict_map = NULL;
26672796
MonoMethod *cm;
26682797
#if (DEBUG_INTERFACE_VTABLE_CODE|TRACE_INTERFACE_VTABLE_CODE)
26692798
int first_non_interface_slot;
@@ -2746,6 +2875,10 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
27462875
mono_memory_barrier ();
27472876
klass->vtable = tmp;
27482877

2878+
mono_loader_lock ();
2879+
klass->has_dim_conflicts = gklass->has_dim_conflicts;
2880+
mono_loader_unlock ();
2881+
27492882
/* Have to set method->slot for abstract virtual methods */
27502883
if (klass->methods && gklass->methods) {
27512884
int mcount = mono_class_get_method_count (klass);
@@ -2823,12 +2956,8 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
28232956
for (int i = 0; i < iface_onum; i++) {
28242957
MonoMethod *decl = iface_overrides [i*2];
28252958
MonoMethod *override = iface_overrides [i*2 + 1];
2826-
if (!apply_override (klass, vtable, decl, override))
2959+
if (!apply_override (klass, ic, vtable, decl, override, &override_map, &override_class_map, &conflict_map))
28272960
goto fail;
2828-
2829-
if (!override_map)
2830-
override_map = g_hash_table_new (mono_aligned_addr_hash, NULL);
2831-
g_hash_table_insert (override_map, decl, override);
28322961
}
28332962
g_free (iface_overrides);
28342963
}
@@ -2838,12 +2967,8 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
28382967
MonoMethod *decl = overrides [i*2];
28392968
MonoMethod *override = overrides [i*2 + 1];
28402969
if (MONO_CLASS_IS_INTERFACE (decl->klass)) {
2841-
if (!apply_override (klass, vtable, decl, override))
2970+
if (!apply_override (klass, klass, vtable, decl, override, &override_map, &override_class_map, &conflict_map))
28422971
goto fail;
2843-
2844-
if (!override_map)
2845-
override_map = g_hash_table_new (mono_aligned_addr_hash, NULL);
2846-
g_hash_table_insert (override_map, decl, override);
28472972
}
28482973
}
28492974

@@ -3108,6 +3233,14 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
31083233
override_map = NULL;
31093234
}
31103235

3236+
if (override_class_map)
3237+
g_hash_table_destroy (override_class_map);
3238+
3239+
if (conflict_map) {
3240+
handle_dim_conflicts (vtable, klass, conflict_map);
3241+
g_hash_table_destroy (conflict_map);
3242+
}
3243+
31113244
g_slist_free (virt_methods);
31123245
virt_methods = NULL;
31133246

‎mono/metadata/class-internals.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1374,6 +1374,15 @@ mono_class_set_weak_bitmap (MonoClass *klass, int nbits, gsize *bits);
13741374
gsize*
13751375
mono_class_get_weak_bitmap (MonoClass *klass, int *nbits);
13761376

1377+
gboolean
1378+
mono_class_has_dim_conflicts (MonoClass *klass);
1379+
1380+
void
1381+
mono_class_set_dim_conflicts (MonoClass *klass, GSList *conflicts);
1382+
1383+
GSList*
1384+
mono_class_get_dim_conflicts (MonoClass *klass);
1385+
13771386
MonoMethod *
13781387
mono_class_get_method_from_name_checked (MonoClass *klass, const char *name, int param_count, int flags, MonoError *error);
13791388

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ struct _MonoClass {
8080
guint fields_inited : 1; /* setup_fields () has finished */
8181
guint has_failure : 1; /* See mono_class_get_exception_data () for a MonoErrorBoxed with the details */
8282
guint has_weak_fields : 1; /* class has weak reference fields */
83+
guint has_dim_conflicts : 1; /* Class has conflicting default interface methods */
8384

8485
MonoClass *parent;
8586
MonoClass *nested_in;

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

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2914,16 +2914,6 @@ emit_get_rgctx (MonoCompile *cfg, int context_used)
29142914
mrgctx_loc = mono_get_vtable_var (cfg);
29152915
EMIT_NEW_TEMPLOAD (cfg, mrgctx_var, mrgctx_loc->inst_c0);
29162916

2917-
return mrgctx_var;
2918-
} else if (MONO_CLASS_IS_INTERFACE (cfg->method->klass)) {
2919-
MonoInst *mrgctx_loc, *mrgctx_var;
2920-
2921-
/* Default interface methods need an mrgctx since the vtabke at runtime points at an implementing class */
2922-
mrgctx_loc = mono_get_vtable_var (cfg);
2923-
EMIT_NEW_TEMPLOAD (cfg, mrgctx_var, mrgctx_loc->inst_c0);
2924-
2925-
g_assert (mono_method_needs_static_rgctx_invoke (cfg->method, TRUE));
2926-
29272917
return mrgctx_var;
29282918
} else if (method->flags & METHOD_ATTRIBUTE_STATIC || method->klass->valuetype) {
29292919
MonoInst *vtable_loc, *vtable_var;

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2894,6 +2894,10 @@ mono_method_is_generic_sharable_full (MonoMethod *method, gboolean allow_type_va
28942894
if (!mono_method_is_generic_impl (method))
28952895
return FALSE;
28962896

2897+
if (MONO_CLASS_IS_INTERFACE (method->klass))
2898+
/* Default Interface Methods don't work yet with gshared */
2899+
return FALSE;
2900+
28972901
/*
28982902
if (!mono_debug_count ())
28992903
allow_partial = FALSE;

‎mono/mini/mini-trampolines.c‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -574,6 +574,31 @@ common_call_trampoline (mgreg_t *regs, guint8 *code, MonoMethod *m, MonoVTable *
574574
vtable_slot = mini_resolve_imt_method (vt, vtable_slot, imt_method, &impl_method, &addr, &need_rgctx_tramp, &variant_iface, error);
575575
return_val_if_nok (error, NULL);
576576

577+
if (mono_class_has_dim_conflicts (vt->klass)) {
578+
GSList *conflicts = mono_class_get_dim_conflicts (vt->klass);
579+
GSList *l;
580+
MonoMethod *decl = imt_method;
581+
582+
if (decl->is_inflated)
583+
decl = mono_method_get_declaring_generic_method (decl);
584+
585+
gboolean in_conflict = FALSE;
586+
for (l = conflicts; l; l = l->next) {
587+
if (decl == l->data) {
588+
in_conflict = TRUE;
589+
break;
590+
}
591+
}
592+
if (in_conflict) {
593+
char *class_name = mono_class_full_name (vt->klass);
594+
char *method_name = mono_method_full_name (decl, TRUE);
595+
mono_error_set_not_supported (error, "Interface method '%s' in class '%s' has multiple candidate implementations.", method_name, class_name);
596+
g_free (class_name);
597+
g_free (method_name);
598+
return NULL;
599+
}
600+
}
601+
577602
/* We must handle magic interfaces on rank 1 arrays of ref types as if they were variant */
578603
if (!variant_iface && vt->klass->rank == 1 && !vt->klass->element_class->valuetype && imt_method->klass->is_array_special_interface)
579604
variant_iface = imt_method;

0 commit comments

Comments
 (0)