From a8ba3216a5783b4f3c5c1bf8cf92dabea46b9c43 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Thu, 20 Jun 2024 05:09:35 -0700 Subject: [PATCH 01/21] Skip exact interface matches when an interface has variance Search for interface matches starting from the end instead of the beginning of the list --- src/mono/mono/metadata/class.c | 47 ++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 043338c14899d9..3a7d1ed84518b5 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -1946,10 +1946,16 @@ 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); + // const char *iname = mono_type_get_name_full (m_class_get_byval_arg (itf), MONO_TYPE_NAME_FORMAT_FULL_NAME); + 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) { + // g_print ("exact match for %s on %s\n", iname, m_class_get_name (klass)); + if (!has_variance) + return exact_match; + } int klass_interface_offsets_count = m_class_get_interface_offsets_count (klass); @@ -1957,10 +1963,17 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo MonoClass *gtd = mono_class_get_generic_type_definition (itf); int found = -1; - for (i = 0; i < klass_interface_offsets_count; i++) { + for (i = klass_interface_offsets_count - 1; i >= 0; i--) { if (mono_class_is_variant_compatible (itf, m_class_get_interfaces_packed (klass) [i], FALSE)) { + /* + g_print ( + "is_variant_compatible (%s, %s, FALSE) == true\n", + iname, + mono_type_get_name_full (m_class_get_byval_arg (m_class_get_interfaces_packed (klass) [i]), MONO_TYPE_NAME_FORMAT_FULL_NAME) + ); + */ found = i; - *non_exact_match = TRUE; + *non_exact_match = (i != exact_match); break; } @@ -1968,10 +1981,17 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo if (found != -1) return m_class_get_interface_offsets_packed (klass) [found]; - for (i = 0; i < klass_interface_offsets_count; i++) { + for (i = klass_interface_offsets_count - 1; i >= 0; i--) { if (mono_class_get_generic_type_definition (m_class_get_interfaces_packed (klass) [i]) == gtd) { + /* + g_print ( + "gtd_of (%s) == gtd_of (%s)\n", + mono_type_get_name_full (m_class_get_byval_arg (m_class_get_interfaces_packed (klass) [i]), MONO_TYPE_NAME_FORMAT_FULL_NAME), + iname + ); + */ found = i; - *non_exact_match = TRUE; + *non_exact_match = (i != exact_match); break; } } @@ -1982,12 +2002,19 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo return m_class_get_interface_offsets_packed (klass) [found]; } - if (!mono_class_has_variant_generic_params (itf)) + if (!has_variance) return -1; - for (i = 0; i < klass_interface_offsets_count; i++) { + for (i = klass_interface_offsets_count - 1; i >= 0; i--) { if (mono_class_is_variant_compatible (itf, m_class_get_interfaces_packed (klass) [i], FALSE)) { - *non_exact_match = TRUE; + /* + g_print ( + "is_variant_compatible (%s, %s, FALSE) == true\n", + iname, + mono_type_get_name_full (m_class_get_byval_arg (m_class_get_interfaces_packed (klass) [i]), MONO_TYPE_NAME_FORMAT_FULL_NAME) + ); + */ + *non_exact_match = (i != exact_match); return m_class_get_interface_offsets_packed (klass) [i]; } } From 5c1716e5a5eb6ce58436a230cb17d40cf151774d Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 24 Jun 2024 11:28:47 -0700 Subject: [PATCH 02/21] When searching for a variant interface, use a special 'variant search table' that contains only variant interfaces in the correct search order --- .../mono/metadata/class-private-definition.h | 4 + src/mono/mono/metadata/class.c | 83 +++++++++++++++---- 2 files changed, 70 insertions(+), 17 deletions(-) diff --git a/src/mono/mono/metadata/class-private-definition.h b/src/mono/mono/metadata/class-private-definition.h index 1e0c01ab336b3b..7648e0dc0519ba 100644 --- a/src/mono/mono/metadata/class-private-definition.h +++ b/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; + + void *variant_search_table; + guint variant_search_table_length; }; struct _MonoClassDef { diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 3a7d1ed84518b5..3458267d5f7a0b 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -1933,6 +1933,60 @@ mono_class_interface_offset (MonoClass *klass, MonoClass *itf) return -1; } +typedef struct VarianceSearchEntry { + MonoClass *klass; + int interface_offset; +} VarianceSearchEntry; + +static void +build_variance_search_table (MonoClass *klass) { + guint buf_size = m_class_get_interface_offsets_count (klass), buf_count = 0; + VarianceSearchEntry *buf = g_alloca (buf_size * sizeof(VarianceSearchEntry)); + memset (buf, 0, buf_size * sizeof(VarianceSearchEntry)); + + MonoClass *current = klass; + while (current) { + // g_print ("%s.%s:\n", m_class_get_name_space (current), m_class_get_name (current)); + MonoClass **ifaces = m_class_get_interfaces (current); + for (guint i = 0, c = m_class_get_interface_count (current); i < c; i++) { + MonoClass *iface = ifaces [i]; + if (!mono_class_has_variant_generic_params (iface)) + continue; + + // FIXME: Avoid adding duplicates. + // g_print ("-> %s.%s\n", m_class_get_name_space (iface), m_class_get_name (iface)); + g_assert (buf_count < buf_size); + buf[buf_count].klass = iface; + buf[buf_count].interface_offset = mono_class_interface_offset (klass, iface); + buf_count++; + } + + current = current->parent; + } + + if (buf_count) { + guint bytes = buf_count * sizeof(VarianceSearchEntry); + klass->variant_search_table = g_malloc (bytes); + memcpy (klass->variant_search_table, buf, bytes); + } else + klass->variant_search_table = NULL; + klass->variant_search_table_length = buf_count; + klass->variant_search_table_inited = TRUE; +} + +static void +get_variance_search_table (MonoClass *klass, VarianceSearchEntry **table, guint *table_size) { + g_assert (klass); + g_assert (table); + g_assert (table_size); + + if (!klass->variant_search_table_inited) + build_variance_search_table (klass); + + *table = (VarianceSearchEntry *)klass->variant_search_table; + *table_size = klass->variant_search_table_length; +} + /** * mono_class_interface_offset_with_variance: * @@ -1959,11 +2013,11 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo 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 = klass_interface_offsets_count - 1; i >= 0; i--) { + for (i = 0; i < klass_interface_offsets_count; i++) { if (mono_class_is_variant_compatible (itf, m_class_get_interfaces_packed (klass) [i], FALSE)) { /* g_print ( @@ -1981,7 +2035,7 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo if (found != -1) return m_class_get_interface_offsets_packed (klass) [found]; - for (i = klass_interface_offsets_count - 1; i >= 0; i--) { + for (i = 0; i < klass_interface_offsets_count; i++) { if (mono_class_get_generic_type_definition (m_class_get_interfaces_packed (klass) [i]) == gtd) { /* g_print ( @@ -2000,22 +2054,17 @@ 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) { + VarianceSearchEntry *vst; + guint vst_count; + get_variance_search_table (klass, &vst, &vst_count); - if (!has_variance) - return -1; + for (guint i = 0; i < vst_count; i++) { + if (!mono_class_is_variant_compatible (itf, vst [i].klass, FALSE)) + continue; - for (i = klass_interface_offsets_count - 1; i >= 0; i--) { - if (mono_class_is_variant_compatible (itf, m_class_get_interfaces_packed (klass) [i], FALSE)) { - /* - g_print ( - "is_variant_compatible (%s, %s, FALSE) == true\n", - iname, - mono_type_get_name_full (m_class_get_byval_arg (m_class_get_interfaces_packed (klass) [i]), MONO_TYPE_NAME_FORMAT_FULL_NAME) - ); - */ - *non_exact_match = (i != exact_match); - return m_class_get_interface_offsets_packed (klass) [i]; + *non_exact_match = (vst [i].interface_offset != exact_match); + return vst [i].interface_offset; } } From d2ab21fa01d63f3b8d2432467080c6fdd5565b21 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 24 Jun 2024 11:48:18 -0700 Subject: [PATCH 03/21] Checkpoint: Maybe fix CI --- src/mono/mono/metadata/class-init.c | 49 ++++++++++++++++ .../mono/metadata/class-private-definition.h | 2 +- src/mono/mono/metadata/class.c | 58 +------------------ .../mono/metadata/details/class-functions.h | 3 + .../mono/metadata/details/class-types.h | 5 ++ 5 files changed, 60 insertions(+), 57 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 0f5643e663f0f4..b2cfac8bfce516 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4189,6 +4189,55 @@ mono_class_set_runtime_vtable (MonoClass *klass, MonoVTable *vtable) klass->runtime_vtable = vtable; } +static void +build_variance_search_table (MonoClass *klass) { + int buf_size = m_class_get_interface_offsets_count (klass), buf_count = 0; + MonoVarianceSearchEntry *buf = g_alloca (buf_size * sizeof(MonoVarianceSearchEntry)); + memset (buf, 0, buf_size * sizeof(MonoVarianceSearchEntry)); + + MonoClass *current = klass; + while (current) { + // g_print ("%s.%s:\n", m_class_get_name_space (current), m_class_get_name (current)); + MonoClass **ifaces = m_class_get_interfaces (current); + for (guint i = 0, c = m_class_get_interface_count (current); i < c; i++) { + MonoClass *iface = ifaces [i]; + if (!mono_class_has_variant_generic_params (iface)) + continue; + + // FIXME: Avoid adding duplicates. + // g_print ("-> %s.%s\n", m_class_get_name_space (iface), m_class_get_name (iface)); + g_assert (buf_count < buf_size); + buf[buf_count].klass = iface; + buf[buf_count].interface_offset = mono_class_interface_offset (klass, iface); + buf_count++; + } + + current = current->parent; + } + + if (buf_count) { + guint bytes = buf_count * sizeof(MonoVarianceSearchEntry); + klass->variant_search_table = g_malloc (bytes); + memcpy (klass->variant_search_table, buf, bytes); + } else + klass->variant_search_table = NULL; + klass->variant_search_table_length = buf_count; + klass->variant_search_table_inited = TRUE; +} + +void +mono_class_get_variance_search_table (MonoClass *klass, MonoVarianceSearchEntry **table, int *table_size) { + g_assert (klass); + g_assert (table); + g_assert (table_size); + + if (!klass->variant_search_table_inited) + build_variance_search_table (klass); + + *table = (MonoVarianceSearchEntry *)klass->variant_search_table; + *table_size = klass->variant_search_table_length; +} + /** * mono_classes_init: * diff --git a/src/mono/mono/metadata/class-private-definition.h b/src/mono/mono/metadata/class-private-definition.h index 7648e0dc0519ba..0bf00cef18948f 100644 --- a/src/mono/mono/metadata/class-private-definition.h +++ b/src/mono/mono/metadata/class-private-definition.h @@ -130,7 +130,7 @@ struct _MonoClass { MonoPropertyBag infrequent_data; void *variant_search_table; - guint variant_search_table_length; + int variant_search_table_length; }; struct _MonoClassDef { diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 3458267d5f7a0b..e5460612353134 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -1933,60 +1933,6 @@ mono_class_interface_offset (MonoClass *klass, MonoClass *itf) return -1; } -typedef struct VarianceSearchEntry { - MonoClass *klass; - int interface_offset; -} VarianceSearchEntry; - -static void -build_variance_search_table (MonoClass *klass) { - guint buf_size = m_class_get_interface_offsets_count (klass), buf_count = 0; - VarianceSearchEntry *buf = g_alloca (buf_size * sizeof(VarianceSearchEntry)); - memset (buf, 0, buf_size * sizeof(VarianceSearchEntry)); - - MonoClass *current = klass; - while (current) { - // g_print ("%s.%s:\n", m_class_get_name_space (current), m_class_get_name (current)); - MonoClass **ifaces = m_class_get_interfaces (current); - for (guint i = 0, c = m_class_get_interface_count (current); i < c; i++) { - MonoClass *iface = ifaces [i]; - if (!mono_class_has_variant_generic_params (iface)) - continue; - - // FIXME: Avoid adding duplicates. - // g_print ("-> %s.%s\n", m_class_get_name_space (iface), m_class_get_name (iface)); - g_assert (buf_count < buf_size); - buf[buf_count].klass = iface; - buf[buf_count].interface_offset = mono_class_interface_offset (klass, iface); - buf_count++; - } - - current = current->parent; - } - - if (buf_count) { - guint bytes = buf_count * sizeof(VarianceSearchEntry); - klass->variant_search_table = g_malloc (bytes); - memcpy (klass->variant_search_table, buf, bytes); - } else - klass->variant_search_table = NULL; - klass->variant_search_table_length = buf_count; - klass->variant_search_table_inited = TRUE; -} - -static void -get_variance_search_table (MonoClass *klass, VarianceSearchEntry **table, guint *table_size) { - g_assert (klass); - g_assert (table); - g_assert (table_size); - - if (!klass->variant_search_table_inited) - build_variance_search_table (klass); - - *table = (VarianceSearchEntry *)klass->variant_search_table; - *table_size = klass->variant_search_table_length; -} - /** * mono_class_interface_offset_with_variance: * @@ -2055,9 +2001,9 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo return m_class_get_interface_offsets_packed (klass) [found]; } else if (has_variance) { - VarianceSearchEntry *vst; + MonoVarianceSearchEntry *vst; guint vst_count; - get_variance_search_table (klass, &vst, &vst_count); + mono_class_get_variance_search_table (klass, &vst, &vst_count); for (guint i = 0; i < vst_count; i++) { if (!mono_class_is_variant_compatible (itf, vst [i].klass, FALSE)) diff --git a/src/native/public/mono/metadata/details/class-functions.h b/src/native/public/mono/metadata/details/class-functions.h index 3d71d168d41e37..66874749841efc 100644 --- a/src/native/public/mono/metadata/details/class-functions.h +++ b/src/native/public/mono/metadata/details/class-functions.h @@ -131,6 +131,9 @@ MONO_API_FUNCTION(MONO_RT_EXTERNAL_ONLY mono_bool, mono_class_is_delegate, (Mono MONO_API_FUNCTION(MONO_RT_EXTERNAL_ONLY mono_bool, mono_class_implements_interface, (MonoClass* klass, MonoClass* iface)) +void +mono_class_get_variance_search_table (MonoClass *klass, MonoVarianceSearchEntry **table, int *table_size); + /* MonoClassField accessors */ MONO_API_FUNCTION(const char*, mono_field_get_name, (MonoClassField *field)) diff --git a/src/native/public/mono/metadata/details/class-types.h b/src/native/public/mono/metadata/details/class-types.h index 716025ed4b8dab..685be0fdaf7065 100644 --- a/src/native/public/mono/metadata/details/class-types.h +++ b/src/native/public/mono/metadata/details/class-types.h @@ -17,6 +17,11 @@ typedef struct _MonoClassField MonoClassField; typedef struct _MonoProperty MonoProperty; typedef struct _MonoEvent MonoEvent; +typedef struct MonoVarianceSearchEntry { + MonoClass *klass; + int interface_offset; +} MonoVarianceSearchEntry; + typedef enum { MONO_TYPE_NAME_FORMAT_IL, MONO_TYPE_NAME_FORMAT_REFLECTION, From 45c88da7ff147d39ca11d1c9672e2845eb45a5c6 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 24 Jun 2024 13:32:43 -0700 Subject: [PATCH 04/21] Fix type mismatch --- src/mono/mono/metadata/class.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index e5460612353134..f62ae636917c5c 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -2002,10 +2002,10 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo return m_class_get_interface_offsets_packed (klass) [found]; } else if (has_variance) { MonoVarianceSearchEntry *vst; - guint vst_count; + int vst_count; mono_class_get_variance_search_table (klass, &vst, &vst_count); - for (guint i = 0; i < vst_count; i++) { + for (int i = 0; i < vst_count; i++) { if (!mono_class_is_variant_compatible (itf, vst [i].klass, FALSE)) continue; From 67ec69e7ecf85f8807322ae9fc93f88ad20d4c16 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 24 Jun 2024 14:06:07 -0700 Subject: [PATCH 05/21] Fix wasm build --- src/mono/mono/metadata/class.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index f62ae636917c5c..ffc4e2a6b7600f 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -2005,7 +2005,7 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo int vst_count; mono_class_get_variance_search_table (klass, &vst, &vst_count); - for (int i = 0; i < vst_count; i++) { + for (i = 0; i < vst_count; i++) { if (!mono_class_is_variant_compatible (itf, vst [i].klass, FALSE)) continue; From 59e7b6b657682470dceec678a09dc504c19750cf Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 24 Jun 2024 16:19:48 -0700 Subject: [PATCH 06/21] Fix variant fallthrough behavior --- src/mono/mono/metadata/class.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index ffc4e2a6b7600f..4163a45f59664d 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -2012,6 +2012,8 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo *non_exact_match = (vst [i].interface_offset != exact_match); return vst [i].interface_offset; } + + return exact_match; } return -1; From 02903010c472a1953a023c925f6335ec9904a4c5 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 25 Jun 2024 09:59:29 -0700 Subject: [PATCH 07/21] Add missing interfaces to set of array special interfaces in mono Don't dereference random memory if get_virtual_method fails in interp --- src/mono/mono/metadata/class-init.c | 9 ++++++++- src/mono/mono/mini/interp/interp.c | 10 +++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index b2cfac8bfce516..1be61509f2fb76 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/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; } } diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index 3728c995e9dd96..731f40113b96f8 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -619,7 +619,15 @@ 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); + if (ioffset < 0) + g_print ( + "interface_offset_with_variance failure for %s.%s and %s.%s\n", + m_class_get_name_space(vtable->klass), m_class_get_name(vtable->klass), + m_class_get_name_space(m->klass), m_class_get_name(m->klass) + ); + g_assert (ioffset >= 0); + slot += ioffset; } MonoMethod *virtual_method = m_class_get_vtable (vtable->klass) [slot]; From 88a844037328c16cbebdb922cfdfa189e089fc4c Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 25 Jun 2024 12:50:38 -0700 Subject: [PATCH 08/21] Fixes for variance search table in cases of complex recursion --- src/mono/mono/metadata/class-init.c | 94 +++++++++++++++++++++++------ src/mono/mono/mini/interp/interp.c | 21 +++++-- 2 files changed, 93 insertions(+), 22 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 1be61509f2fb76..53d89f5ed1e72b 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4196,40 +4196,98 @@ mono_class_set_runtime_vtable (MonoClass *klass, MonoVTable *vtable) klass->runtime_vtable = vtable; } +static int +index_of_class (MonoClass *needle, MonoVarianceSearchEntry *haystack, int haystack_size) { + for (int i = 0; i < haystack_size; i++) + if (haystack[i].klass == needle) + return i; + + return -1; +} + static void -build_variance_search_table (MonoClass *klass) { - int buf_size = m_class_get_interface_offsets_count (klass), buf_count = 0; - MonoVarianceSearchEntry *buf = g_alloca (buf_size * sizeof(MonoVarianceSearchEntry)); - memset (buf, 0, buf_size * sizeof(MonoVarianceSearchEntry)); +concat_variance_search_table (MonoClass *klass, MonoVarianceSearchEntry *buf, int buf_size, int *buf_count, MonoClass *current) { + MonoVarianceSearchEntry *table; + int table_size; + mono_class_get_variance_search_table (current, &table, &table_size); + if (!table_size || !table) + return; + + for (int i = 0; i < table_size; i++) { + MonoClass *iface = table[i].klass; + if (index_of_class (iface, buf, *buf_count) >= 0) + continue; + + g_assert (*buf_count < buf_size); + buf[*buf_count].klass = table[i].klass; + buf[*buf_count].interface_offset = mono_class_interface_offset (klass, table[i].klass); + (*buf_count) += 1; + } +} + +static void +build_variance_search_table_inner (MonoClass *klass, MonoVarianceSearchEntry *buf, int buf_size, int *buf_count, MonoClass *current) { + guint c = m_class_get_interface_count (current); + if (c) { + /* + char *cname = mono_type_get_full_name (current); + g_print ("+ %s:\n", cname); + */ - MonoClass *current = klass; - while (current) { - // g_print ("%s.%s:\n", m_class_get_name_space (current), m_class_get_name (current)); MonoClass **ifaces = m_class_get_interfaces (current); - for (guint i = 0, c = m_class_get_interface_count (current); i < c; i++) { + for (guint i = 0; i < c; i++) { MonoClass *iface = ifaces [i]; if (!mono_class_has_variant_generic_params (iface)) continue; - // FIXME: Avoid adding duplicates. - // g_print ("-> %s.%s\n", m_class_get_name_space (iface), m_class_get_name (iface)); - g_assert (buf_count < buf_size); - buf[buf_count].klass = iface; - buf[buf_count].interface_offset = mono_class_interface_offset (klass, iface); - buf_count++; + // Avoid adding duplicates. + if (index_of_class (iface, buf, *buf_count) >= 0) + continue; + + /* + char *iname = mono_type_get_full_name (iface); + g_print ("-> %s\n", iname); + g_free (iname); + */ + g_assert (*buf_count < buf_size); + buf[*buf_count].klass = iface; + buf[*buf_count].interface_offset = mono_class_interface_offset (klass, iface); + (*buf_count) += 1; } - current = current->parent; + for (guint i = 0; i < c; i++) + concat_variance_search_table (klass, buf, buf_size, buf_count, ifaces [i]); + + /* + g_print ("- %s:\n", cname); + g_free (cname); + */ } + if (current->parent) + concat_variance_search_table (klass, buf, buf_size, buf_count, current->parent); +} + +static void +build_variance_search_table (MonoClass *klass) { + // FIXME: Is there a way to deterministically compute the right capacity? + int buf_size = 512, buf_count = 0; + MonoVarianceSearchEntry *buf = g_alloca (buf_size * sizeof(MonoVarianceSearchEntry)); + memset (buf, 0, buf_size * sizeof(MonoVarianceSearchEntry)); + + klass->variant_search_table = NULL; + klass->variant_search_table_length = 0; + // Break recursion + klass->variant_search_table_inited = TRUE; + + build_variance_search_table_inner (klass, buf, buf_size, &buf_count, klass); + if (buf_count) { guint bytes = buf_count * sizeof(MonoVarianceSearchEntry); klass->variant_search_table = g_malloc (bytes); memcpy (klass->variant_search_table, buf, bytes); - } else - klass->variant_search_table = NULL; + } klass->variant_search_table_length = buf_count; - klass->variant_search_table_inited = TRUE; } void diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index 731f40113b96f8..83a363cfec0a83 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -620,12 +620,25 @@ get_virtual_method (InterpMethod *imethod, MonoVTable *vtable) /* TODO: interface offset lookup is slow, go through IMT instead */ gboolean non_exact_match; int ioffset = mono_class_interface_offset_with_variance (vtable->klass, m->klass, &non_exact_match); - if (ioffset < 0) + if (ioffset < 0) { + char *vtname = mono_type_get_full_name (vtable->klass), + *mname = mono_type_get_full_name (m->klass); g_print ( - "interface_offset_with_variance failure for %s.%s and %s.%s\n", - m_class_get_name_space(vtable->klass), m_class_get_name(vtable->klass), - m_class_get_name_space(m->klass), m_class_get_name(m->klass) + "interface_offset_with_variance failure for %s and %s\n", + vtname, mname ); + MonoVarianceSearchEntry *table; + int table_size; + mono_class_get_variance_search_table (vtable->klass, &table, &table_size); + for (int i = 0; i < table_size; i++) { + char *tyname = mono_type_get_full_name (table[i].klass); + g_print ( + "#%d: %s\n", + i, tyname + ); + g_free (tyname); + } + } g_assert (ioffset >= 0); slot += ioffset; } From adcf3b315455e024e989f48a5710981eff43299f Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Thu, 27 Jun 2024 13:13:04 -0700 Subject: [PATCH 09/21] Don't copy base class interfaces into the variance table, since it makes two-pass hard Try to match the spec more closely by doing two passes per class --- src/mono/mono/metadata/class-init.c | 21 ++++++++----------- src/mono/mono/metadata/class.c | 32 +++++++++++++++++++++++------ 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 53d89f5ed1e72b..c1291f6ff314f8 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4237,9 +4237,6 @@ build_variance_search_table_inner (MonoClass *klass, MonoVarianceSearchEntry *bu MonoClass **ifaces = m_class_get_interfaces (current); for (guint i = 0; i < c; i++) { MonoClass *iface = ifaces [i]; - if (!mono_class_has_variant_generic_params (iface)) - continue; - // Avoid adding duplicates. if (index_of_class (iface, buf, *buf_count) >= 0) continue; @@ -4249,23 +4246,21 @@ build_variance_search_table_inner (MonoClass *klass, MonoVarianceSearchEntry *bu g_print ("-> %s\n", iname); g_free (iname); */ - g_assert (*buf_count < buf_size); - buf[*buf_count].klass = iface; - buf[*buf_count].interface_offset = mono_class_interface_offset (klass, iface); - (*buf_count) += 1; - } + if (mono_class_has_variant_generic_params (iface)) { + g_assert (*buf_count < buf_size); + buf[*buf_count].klass = iface; + buf[*buf_count].interface_offset = mono_class_interface_offset (klass, iface); + (*buf_count) += 1; + } - for (guint i = 0; i < c; i++) - concat_variance_search_table (klass, buf, buf_size, buf_count, ifaces [i]); + concat_variance_search_table (klass, buf, buf_size, buf_count, iface); + } /* g_print ("- %s:\n", cname); g_free (cname); */ } - - if (current->parent) - concat_variance_search_table (klass, buf, buf_size, buf_count, current->parent); } static void diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 4163a45f59664d..adf80f83ca221d 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -2003,16 +2003,36 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo } else if (has_variance) { MonoVarianceSearchEntry *vst; int vst_count; - mono_class_get_variance_search_table (klass, &vst, &vst_count); + MonoClass *current = klass; - for (i = 0; i < vst_count; i++) { - if (!mono_class_is_variant_compatible (itf, vst [i].klass, FALSE)) - continue; + // 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 + for (i = 0; i < vst_count; i++) { + if (itf != vst [i].klass) + continue; + + *non_exact_match = FALSE; + return vst [i].interface_offset; + } + + // Variance pass + for (i = 0; i < vst_count; i++) { + if (!mono_class_is_variant_compatible (itf, vst [i].klass, FALSE)) + continue; + + *non_exact_match = (vst [i].interface_offset != exact_match); + return vst [i].interface_offset; + } - *non_exact_match = (vst [i].interface_offset != exact_match); - return vst [i].interface_offset; + // Now check base class if present + current = current->parent; } + // If the variance search failed to find a match, fall back on the one from mono_class_interface_offset + *non_exact_match = (exact_match < 0); return exact_match; } From c65f1e018a51bd742873a968ce3a5a100ba03051 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Thu, 27 Jun 2024 14:39:19 -0700 Subject: [PATCH 10/21] Fix build --- src/mono/mono/metadata/class.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index adf80f83ca221d..9d7919af8622e3 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -2028,7 +2028,7 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo } // Now check base class if present - current = current->parent; + current = m_class_get_parent (current); } // If the variance search failed to find a match, fall back on the one from mono_class_interface_offset From 95043186d3c94bc452dcc6feedf872a1df4db129 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Fri, 28 Jun 2024 13:42:32 -0700 Subject: [PATCH 11/21] Ensure interfaces table is initialized before building the variance search table --- src/mono/mono/metadata/class-init.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index c1291f6ff314f8..1121e6979657a9 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4227,6 +4227,11 @@ concat_variance_search_table (MonoClass *klass, MonoVarianceSearchEntry *buf, in static void build_variance_search_table_inner (MonoClass *klass, MonoVarianceSearchEntry *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); + } guint c = m_class_get_interface_count (current); if (c) { /* From 526471494b4ac4e622bbb5469fe222f7813ef76b Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Fri, 28 Jun 2024 17:09:13 -0700 Subject: [PATCH 12/21] Put variance search table construction behind the loader lock --- src/mono/mono/metadata/class-init.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 1121e6979657a9..38e09b41b4ecf3 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4268,26 +4268,26 @@ build_variance_search_table_inner (MonoClass *klass, MonoVarianceSearchEntry *bu } } +// 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 = 512, buf_count = 0; - MonoVarianceSearchEntry *buf = g_alloca (buf_size * sizeof(MonoVarianceSearchEntry)); + MonoVarianceSearchEntry *buf = g_alloca (buf_size * sizeof(MonoVarianceSearchEntry)), + *result = NULL; memset (buf, 0, buf_size * sizeof(MonoVarianceSearchEntry)); - - klass->variant_search_table = NULL; - klass->variant_search_table_length = 0; - // Break recursion - klass->variant_search_table_inited = TRUE; - build_variance_search_table_inner (klass, buf, buf_size, &buf_count, klass); if (buf_count) { guint bytes = buf_count * sizeof(MonoVarianceSearchEntry); - klass->variant_search_table = g_malloc (bytes); - memcpy (klass->variant_search_table, buf, bytes); + result = g_malloc (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 @@ -4296,8 +4296,12 @@ mono_class_get_variance_search_table (MonoClass *klass, MonoVarianceSearchEntry g_assert (table); g_assert (table_size); - if (!klass->variant_search_table_inited) - build_variance_search_table (klass); + if (!klass->variant_search_table_inited) { + mono_loader_lock (); + if (!klass->variant_search_table_inited) + build_variance_search_table (klass); + mono_loader_unlock (); + } *table = (MonoVarianceSearchEntry *)klass->variant_search_table; *table_size = klass->variant_search_table_length; From f7324cd41e906b0a3b02032c09f4988be18e7b96 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 1 Jul 2024 16:53:15 -0700 Subject: [PATCH 13/21] Cleanup and simplification --- src/mono/mono/metadata/class-init.c | 45 ++++++------------- src/mono/mono/metadata/class.c | 21 +++++---- src/mono/mono/mini/interp/interp.c | 4 +- .../mono/metadata/details/class-functions.h | 3 +- 4 files changed, 30 insertions(+), 43 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 38e09b41b4ecf3..a6112b2b8b72eb 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4197,36 +4197,35 @@ mono_class_set_runtime_vtable (MonoClass *klass, MonoVTable *vtable) } static int -index_of_class (MonoClass *needle, MonoVarianceSearchEntry *haystack, int haystack_size) { +index_of_class (MonoClass *needle, MonoClass **haystack, int haystack_size) { for (int i = 0; i < haystack_size; i++) - if (haystack[i].klass == needle) + if (haystack[i] == needle) return i; return -1; } static void -concat_variance_search_table (MonoClass *klass, MonoVarianceSearchEntry *buf, int buf_size, int *buf_count, MonoClass *current) { - MonoVarianceSearchEntry *table; +concat_variance_search_table (MonoClass *klass, MonoClass **buf, int buf_size, int *buf_count, MonoClass *current) { + MonoClass **table; int table_size; mono_class_get_variance_search_table (current, &table, &table_size); if (!table_size || !table) return; for (int i = 0; i < table_size; i++) { - MonoClass *iface = table[i].klass; + MonoClass *iface = table[i]; if (index_of_class (iface, buf, *buf_count) >= 0) continue; g_assert (*buf_count < buf_size); - buf[*buf_count].klass = table[i].klass; - buf[*buf_count].interface_offset = mono_class_interface_offset (klass, table[i].klass); + buf[*buf_count] = table[i]; (*buf_count) += 1; } } static void -build_variance_search_table_inner (MonoClass *klass, MonoVarianceSearchEntry *buf, int buf_size, int *buf_count, MonoClass *current) { +build_variance_search_table_inner (MonoClass *klass, MonoClass **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); @@ -4234,11 +4233,6 @@ build_variance_search_table_inner (MonoClass *klass, MonoVarianceSearchEntry *bu } guint c = m_class_get_interface_count (current); if (c) { - /* - char *cname = mono_type_get_full_name (current); - g_print ("+ %s:\n", cname); - */ - MonoClass **ifaces = m_class_get_interfaces (current); for (guint i = 0; i < c; i++) { MonoClass *iface = ifaces [i]; @@ -4246,25 +4240,14 @@ build_variance_search_table_inner (MonoClass *klass, MonoVarianceSearchEntry *bu if (index_of_class (iface, buf, *buf_count) >= 0) continue; - /* - char *iname = mono_type_get_full_name (iface); - g_print ("-> %s\n", iname); - g_free (iname); - */ if (mono_class_has_variant_generic_params (iface)) { g_assert (*buf_count < buf_size); - buf[*buf_count].klass = iface; - buf[*buf_count].interface_offset = mono_class_interface_offset (klass, iface); + buf[*buf_count] = iface; (*buf_count) += 1; } concat_variance_search_table (klass, buf, buf_size, buf_count, iface); } - - /* - g_print ("- %s:\n", cname); - g_free (cname); - */ } } @@ -4273,13 +4256,13 @@ static void build_variance_search_table (MonoClass *klass) { // FIXME: Is there a way to deterministically compute the right capacity? int buf_size = 512, buf_count = 0; - MonoVarianceSearchEntry *buf = g_alloca (buf_size * sizeof(MonoVarianceSearchEntry)), - *result = NULL; - memset (buf, 0, buf_size * sizeof(MonoVarianceSearchEntry)); + MonoClass **buf = g_alloca (buf_size * sizeof(MonoClass *)), + **result = NULL; + memset (buf, 0, buf_size * sizeof(MonoClass *)); build_variance_search_table_inner (klass, buf, buf_size, &buf_count, klass); if (buf_count) { - guint bytes = buf_count * sizeof(MonoVarianceSearchEntry); + guint bytes = buf_count * sizeof(MonoClass *); result = g_malloc (bytes); memcpy (result, buf, bytes); } @@ -4291,7 +4274,7 @@ build_variance_search_table (MonoClass *klass) { } void -mono_class_get_variance_search_table (MonoClass *klass, MonoVarianceSearchEntry **table, int *table_size) { +mono_class_get_variance_search_table (MonoClass *klass, MonoClass ***table, int *table_size) { g_assert (klass); g_assert (table); g_assert (table_size); @@ -4303,7 +4286,7 @@ mono_class_get_variance_search_table (MonoClass *klass, MonoVarianceSearchEntry mono_loader_unlock (); } - *table = (MonoVarianceSearchEntry *)klass->variant_search_table; + *table = (MonoClass **)klass->variant_search_table; *table_size = klass->variant_search_table_length; } diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 9d7919af8622e3..e972346d2154a6 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -2001,7 +2001,7 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo return m_class_get_interface_offsets_packed (klass) [found]; } else if (has_variance) { - MonoVarianceSearchEntry *vst; + MonoClass **vst; int vst_count; MonoClass *current = klass; @@ -2009,22 +2009,27 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo while (current) { mono_class_get_variance_search_table (current, &vst, &vst_count); - // Exact match pass + // 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].klass) + if (itf != vst [i]) continue; *non_exact_match = FALSE; - return vst [i].interface_offset; + return exact_match; } - // Variance pass + // 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].klass, FALSE)) + if (!mono_class_is_variant_compatible (itf, vst [i], FALSE)) continue; - *non_exact_match = (vst [i].interface_offset != exact_match); - return vst [i].interface_offset; + 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 diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index 83a363cfec0a83..e8b02fbd30703e 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -627,11 +627,11 @@ get_virtual_method (InterpMethod *imethod, MonoVTable *vtable) "interface_offset_with_variance failure for %s and %s\n", vtname, mname ); - MonoVarianceSearchEntry *table; + MonoClass **table; int table_size; mono_class_get_variance_search_table (vtable->klass, &table, &table_size); for (int i = 0; i < table_size; i++) { - char *tyname = mono_type_get_full_name (table[i].klass); + char *tyname = mono_type_get_full_name (table[i]); g_print ( "#%d: %s\n", i, tyname diff --git a/src/native/public/mono/metadata/details/class-functions.h b/src/native/public/mono/metadata/details/class-functions.h index 66874749841efc..45534cf416a53f 100644 --- a/src/native/public/mono/metadata/details/class-functions.h +++ b/src/native/public/mono/metadata/details/class-functions.h @@ -131,8 +131,7 @@ MONO_API_FUNCTION(MONO_RT_EXTERNAL_ONLY mono_bool, mono_class_is_delegate, (Mono MONO_API_FUNCTION(MONO_RT_EXTERNAL_ONLY mono_bool, mono_class_implements_interface, (MonoClass* klass, MonoClass* iface)) -void -mono_class_get_variance_search_table (MonoClass *klass, MonoVarianceSearchEntry **table, int *table_size); +MONO_API_FUNCTION(void, mono_class_get_variance_search_table, (MonoClass *klass, MonoClass ***table, int *table_size)); /* MonoClassField accessors */ MONO_API_FUNCTION(const char*, mono_field_get_name, (MonoClassField *field)) From c7dfe4b35bf6bed98f7d2b0a97b71da64a617479 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 1 Jul 2024 19:21:37 -0700 Subject: [PATCH 14/21] Cleanup --- src/mono/mono/metadata/class-init.c | 2 +- .../mono/metadata/class-private-definition.h | 2 +- src/mono/mono/metadata/class.c | 18 +----------------- src/mono/mono/mini/interp/interp.c | 19 ------------------- 4 files changed, 3 insertions(+), 38 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index a6112b2b8b72eb..452e4c99acaa95 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4286,7 +4286,7 @@ mono_class_get_variance_search_table (MonoClass *klass, MonoClass ***table, int mono_loader_unlock (); } - *table = (MonoClass **)klass->variant_search_table; + *table = klass->variant_search_table; *table_size = klass->variant_search_table_length; } diff --git a/src/mono/mono/metadata/class-private-definition.h b/src/mono/mono/metadata/class-private-definition.h index 0bf00cef18948f..e4b04b0284b815 100644 --- a/src/mono/mono/metadata/class-private-definition.h +++ b/src/mono/mono/metadata/class-private-definition.h @@ -129,7 +129,7 @@ struct _MonoClass { /* Infrequently used items. See class-accessors.c: InfrequentDataKind for what goes into here. */ MonoPropertyBag infrequent_data; - void *variant_search_table; + MonoClass **variant_search_table; int variant_search_table_length; }; diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index e972346d2154a6..192153d1b0cc8d 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -1946,13 +1946,11 @@ mono_class_interface_offset (MonoClass *klass, MonoClass *itf) int mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gboolean *non_exact_match) { - // const char *iname = mono_type_get_name_full (m_class_get_byval_arg (itf), MONO_TYPE_NAME_FORMAT_FULL_NAME); 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 (exact_match >= 0) { - // g_print ("exact match for %s on %s\n", iname, m_class_get_name (klass)); if (!has_variance) return exact_match; } @@ -1965,13 +1963,6 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo for (i = 0; i < klass_interface_offsets_count; i++) { if (mono_class_is_variant_compatible (itf, m_class_get_interfaces_packed (klass) [i], FALSE)) { - /* - g_print ( - "is_variant_compatible (%s, %s, FALSE) == true\n", - iname, - mono_type_get_name_full (m_class_get_byval_arg (m_class_get_interfaces_packed (klass) [i]), MONO_TYPE_NAME_FORMAT_FULL_NAME) - ); - */ found = i; *non_exact_match = (i != exact_match); break; @@ -1983,13 +1974,6 @@ 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) { - /* - g_print ( - "gtd_of (%s) == gtd_of (%s)\n", - mono_type_get_name_full (m_class_get_byval_arg (m_class_get_interfaces_packed (klass) [i]), MONO_TYPE_NAME_FORMAT_FULL_NAME), - iname - ); - */ found = i; *non_exact_match = (i != exact_match); break; @@ -2036,7 +2020,7 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo current = m_class_get_parent (current); } - // If the variance search failed to find a match, fall back on the one from mono_class_interface_offset + // 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; } diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index e8b02fbd30703e..e50d64cdbbe3d3 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -620,25 +620,6 @@ get_virtual_method (InterpMethod *imethod, MonoVTable *vtable) /* TODO: interface offset lookup is slow, go through IMT instead */ gboolean non_exact_match; int ioffset = mono_class_interface_offset_with_variance (vtable->klass, m->klass, &non_exact_match); - if (ioffset < 0) { - char *vtname = mono_type_get_full_name (vtable->klass), - *mname = mono_type_get_full_name (m->klass); - g_print ( - "interface_offset_with_variance failure for %s and %s\n", - vtname, mname - ); - MonoClass **table; - int table_size; - mono_class_get_variance_search_table (vtable->klass, &table, &table_size); - for (int i = 0; i < table_size; i++) { - char *tyname = mono_type_get_full_name (table[i]); - g_print ( - "#%d: %s\n", - i, tyname - ); - g_free (tyname); - } - } g_assert (ioffset >= 0); slot += ioffset; } From 8bd5929a6bba827cba14327bd2a43fbc9d1df381 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 2 Jul 2024 11:19:08 -0700 Subject: [PATCH 15/21] Enable some variance tests on mono --- src/tests/issues.targets | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index 5da277d04e87b4..cfe678875b64bb 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -1755,9 +1755,6 @@ needs triage - - needs triage - needs triage From 235ed52b0c3ea0077906e3a35f6f2ba74b8b6553 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 2 Jul 2024 12:08:02 -0700 Subject: [PATCH 16/21] Add regression test --- .../Regressions/coreclr/103365/103365.cs | 51 +++++++++++++++++++ .../Regressions/coreclr/103365/103365.csproj | 8 +++ 2 files changed, 59 insertions(+) create mode 100644 src/tests/Regressions/coreclr/103365/103365.cs create mode 100644 src/tests/Regressions/coreclr/103365/103365.csproj diff --git a/src/tests/Regressions/coreclr/103365/103365.cs b/src/tests/Regressions/coreclr/103365/103365.cs new file mode 100644 index 00000000000000..9a5628aa6a5dc2 --- /dev/null +++ b/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 Android. Running on Windows yields the expected behavior. +*/ + +using System; +using System.Collections; +using System.Collections.Generic; +using System.Runtime.CompilerServices; +using Xunit; + +public interface IBaseInterface +{ + string explicitDeclaration(); +} + +public class BasicBaseClass : IBaseInterface +{ + string className = "BasicBaseClass"; + string IBaseInterface.explicitDeclaration() + { + return className; + } +} + +public class BasicDerivedClass : BasicBaseClass, IBaseInterface +{ + string className = "BasicDerivedClass"; + + string IBaseInterface.explicitDeclaration() + { + return className; + } +} + +public static class Test_Issue103365 +{ + [Fact] + public static void Main () + { + var instances = new IBaseInterface[2]; + instances[0] = new BasicBaseClass(); + instances[1] = new BasicDerivedClass(); + Assert.Equal("BasicBaseClass", instances[0].explicitDeclaration()); + Assert.Equal("BasicDerivedClass", instances[1].explicitDeclaration()); + } +} + diff --git a/src/tests/Regressions/coreclr/103365/103365.csproj b/src/tests/Regressions/coreclr/103365/103365.csproj new file mode 100644 index 00000000000000..ba30d94c716f74 --- /dev/null +++ b/src/tests/Regressions/coreclr/103365/103365.csproj @@ -0,0 +1,8 @@ + + + + + + + + From 2292affcd056f6c47c062fe73d7d145c7189e318 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 2 Jul 2024 12:27:28 -0700 Subject: [PATCH 17/21] Update reason why variance test is disabled --- src/tests/issues.targets | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index cfe678875b64bb..0282e5821dba7a 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -1755,6 +1755,9 @@ needs triage + + ldtoken in default interface methods does not appear to work in minijit or interp + needs triage From e5fdca99ad5f87cd0b7d969509fcd4ab16f97e0a Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 9 Jul 2024 12:06:25 -0700 Subject: [PATCH 18/21] Cleanup --- src/native/public/mono/metadata/details/class-types.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/native/public/mono/metadata/details/class-types.h b/src/native/public/mono/metadata/details/class-types.h index 685be0fdaf7065..716025ed4b8dab 100644 --- a/src/native/public/mono/metadata/details/class-types.h +++ b/src/native/public/mono/metadata/details/class-types.h @@ -17,11 +17,6 @@ typedef struct _MonoClassField MonoClassField; typedef struct _MonoProperty MonoProperty; typedef struct _MonoEvent MonoEvent; -typedef struct MonoVarianceSearchEntry { - MonoClass *klass; - int interface_offset; -} MonoVarianceSearchEntry; - typedef enum { MONO_TYPE_NAME_FORMAT_IL, MONO_TYPE_NAME_FORMAT_REFLECTION, From ff1138cf746f20b60c7cc86bf7318fcaefea5f73 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 9 Jul 2024 18:59:42 -0700 Subject: [PATCH 19/21] Address PR feedback --- src/mono/mono/metadata/class-init.c | 6 +++--- src/mono/mono/metadata/class-internals.h | 3 +++ src/native/public/mono/metadata/details/class-functions.h | 2 -- src/tests/Regressions/coreclr/103365/103365.cs | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 452e4c99acaa95..3d405a61992e8e 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4255,9 +4255,9 @@ build_variance_search_table_inner (MonoClass *klass, MonoClass **buf, int buf_si static void build_variance_search_table (MonoClass *klass) { // FIXME: Is there a way to deterministically compute the right capacity? - int buf_size = 512, buf_count = 0; - MonoClass **buf = g_alloca (buf_size * sizeof(MonoClass *)), - **result = NULL; + 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, klass); diff --git a/src/mono/mono/metadata/class-internals.h b/src/mono/mono/metadata/class-internals.h index a6a908e8bab10f..7044e59262db65 100644 --- a/src/mono/mono/metadata/class-internals.h +++ b/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. diff --git a/src/native/public/mono/metadata/details/class-functions.h b/src/native/public/mono/metadata/details/class-functions.h index 45534cf416a53f..3d71d168d41e37 100644 --- a/src/native/public/mono/metadata/details/class-functions.h +++ b/src/native/public/mono/metadata/details/class-functions.h @@ -131,8 +131,6 @@ MONO_API_FUNCTION(MONO_RT_EXTERNAL_ONLY mono_bool, mono_class_is_delegate, (Mono MONO_API_FUNCTION(MONO_RT_EXTERNAL_ONLY mono_bool, mono_class_implements_interface, (MonoClass* klass, MonoClass* iface)) -MONO_API_FUNCTION(void, mono_class_get_variance_search_table, (MonoClass *klass, MonoClass ***table, int *table_size)); - /* MonoClassField accessors */ MONO_API_FUNCTION(const char*, mono_field_get_name, (MonoClassField *field)) diff --git a/src/tests/Regressions/coreclr/103365/103365.cs b/src/tests/Regressions/coreclr/103365/103365.cs index 9a5628aa6a5dc2..867ff68128ab34 100644 --- a/src/tests/Regressions/coreclr/103365/103365.cs +++ b/src/tests/Regressions/coreclr/103365/103365.cs @@ -3,7 +3,7 @@ /* 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 Android. Running on Windows yields the expected behavior. +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; From 90b90e525eef4537afa82f86d8fa8a9b45733454 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 9 Jul 2024 19:27:04 -0700 Subject: [PATCH 20/21] Address PR feedback Don't build search tables for interfaces --- src/mono/mono/metadata/class-init.c | 33 ++++++++++------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 3d405a61992e8e..3a48f51c2efe67 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4205,25 +4205,6 @@ index_of_class (MonoClass *needle, MonoClass **haystack, int haystack_size) { return -1; } -static void -concat_variance_search_table (MonoClass *klass, MonoClass **buf, int buf_size, int *buf_count, MonoClass *current) { - MonoClass **table; - int table_size; - mono_class_get_variance_search_table (current, &table, &table_size); - if (!table_size || !table) - return; - - for (int i = 0; i < table_size; i++) { - MonoClass *iface = table[i]; - if (index_of_class (iface, buf, *buf_count) >= 0) - continue; - - g_assert (*buf_count < buf_size); - buf[*buf_count] = table[i]; - (*buf_count) += 1; - } -} - static void build_variance_search_table_inner (MonoClass *klass, MonoClass **buf, int buf_size, int *buf_count, MonoClass *current) { if (!m_class_is_interfaces_inited (current)) { @@ -4236,7 +4217,7 @@ build_variance_search_table_inner (MonoClass *klass, MonoClass **buf, int buf_si MonoClass **ifaces = m_class_get_interfaces (current); for (guint i = 0; i < c; i++) { MonoClass *iface = ifaces [i]; - // Avoid adding duplicates. + // Avoid adding duplicates or recursing into them. if (index_of_class (iface, buf, *buf_count) >= 0) continue; @@ -4246,7 +4227,7 @@ build_variance_search_table_inner (MonoClass *klass, MonoClass **buf, int buf_si (*buf_count) += 1; } - concat_variance_search_table (klass, buf, buf_size, buf_count, iface); + build_variance_search_table_inner (klass, buf, buf_size, buf_count, iface); } } } @@ -4263,7 +4244,7 @@ build_variance_search_table (MonoClass *klass) { if (buf_count) { guint bytes = buf_count * sizeof(MonoClass *); - result = g_malloc (bytes); + result = mono_mem_manager_alloc (m_class_get_mem_manager (klass), bytes); memcpy (result, buf, bytes); } klass->variant_search_table_length = buf_count; @@ -4279,6 +4260,14 @@ mono_class_get_variance_search_table (MonoClass *klass, MonoClass ***table, int 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) From c01da2d254fbc0834a382bc5b82b2418f9cf9dfa Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Thu, 11 Jul 2024 12:18:58 -0700 Subject: [PATCH 21/21] Address PR feedback --- src/mono/mono/metadata/class-init.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 3a48f51c2efe67..94536ed5abbf32 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4206,15 +4206,15 @@ index_of_class (MonoClass *needle, MonoClass **haystack, int haystack_size) { } static void -build_variance_search_table_inner (MonoClass *klass, MonoClass **buf, int buf_size, int *buf_count, MonoClass *current) { - if (!m_class_is_interfaces_inited (current)) { +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 (current, error); + mono_class_setup_interfaces (klass, error); return_if_nok (error); } - guint c = m_class_get_interface_count (current); + guint c = m_class_get_interface_count (klass); if (c) { - MonoClass **ifaces = m_class_get_interfaces (current); + 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. @@ -4227,7 +4227,7 @@ build_variance_search_table_inner (MonoClass *klass, MonoClass **buf, int buf_si (*buf_count) += 1; } - build_variance_search_table_inner (klass, buf, buf_size, buf_count, iface); + build_variance_search_table_inner (iface, buf, buf_size, buf_count); } } } @@ -4240,7 +4240,7 @@ build_variance_search_table (MonoClass *klass) { 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, klass); + build_variance_search_table_inner (klass, buf, buf_size, &buf_count); if (buf_count) { guint bytes = buf_count * sizeof(MonoClass *);