From 1cf6774c2825201c851371614088f7cc12f6fd38 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 15 Jul 2024 22:05:31 -0700 Subject: [PATCH 1/7] Reduce size of every MonoClass instance by 4 bytes --- src/mono/mono/metadata/class-init.c | 25 +++++++++++++------ .../mono/metadata/class-private-definition.h | 3 +-- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 94536ed5abbf3..3d036b70b5e88 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4232,22 +4232,27 @@ build_variance_search_table_inner (MonoClass *klass, MonoClass **buf, int buf_si } } +typedef struct VarianceSearchTable { + int count; + MonoClass *klasses[1]; // a total of count items, at least 1 +} VarianceSearchTable; + // Only call this with the loader lock held static void build_variance_search_table (MonoClass *klass) { // FIXME: Is there a way to deterministically compute the right capacity? int buf_size = m_class_get_interface_offsets_count (klass), buf_count = 0; MonoClass **buf = g_alloca (buf_size * sizeof(MonoClass *)); - MonoClass **result = NULL; + VarianceSearchTable *result = NULL; memset (buf, 0, buf_size * sizeof(MonoClass *)); build_variance_search_table_inner (klass, buf, buf_size, &buf_count); if (buf_count) { - guint bytes = buf_count * sizeof(MonoClass *); - result = mono_mem_manager_alloc (m_class_get_mem_manager (klass), bytes); - memcpy (result, buf, bytes); + guint bytes = (buf_count * sizeof(MonoClass *)) + sizeof(VarianceSearchTable); + result = (VarianceSearchTable *)mono_mem_manager_alloc (m_class_get_mem_manager (klass), bytes); + result->count = buf_count; + memcpy (result->klasses, 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 (); @@ -4275,8 +4280,14 @@ mono_class_get_variance_search_table (MonoClass *klass, MonoClass ***table, int mono_loader_unlock (); } - *table = klass->variant_search_table; - *table_size = klass->variant_search_table_length; + VarianceSearchTable *vst = klass->variant_search_table; + if (vst) { + *table = vst->klasses; + *table_size = vst->count; + } else { + *table = NULL; + *table_size = 0; + } } /** diff --git a/src/mono/mono/metadata/class-private-definition.h b/src/mono/mono/metadata/class-private-definition.h index e4b04b0284b81..829ac16d12903 100644 --- a/src/mono/mono/metadata/class-private-definition.h +++ b/src/mono/mono/metadata/class-private-definition.h @@ -129,8 +129,7 @@ struct _MonoClass { /* Infrequently used items. See class-accessors.c: InfrequentDataKind for what goes into here. */ MonoPropertyBag infrequent_data; - MonoClass **variant_search_table; - int variant_search_table_length; + void *variant_search_table; }; struct _MonoClassDef { From dcc4fa169f1a06891005f6ee2df1d9a7ad4a15b9 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 16 Jul 2024 18:50:36 -0700 Subject: [PATCH 2/7] Checkpoint: Non-recursive variance search with cached offsets --- src/mono/mono/metadata/class-init.c | 42 +++++++++++-------- src/mono/mono/metadata/class-internals.h | 2 +- src/mono/mono/metadata/class.c | 42 +++++++++++++------ .../public/mono/metadata/object-forward.h | 5 +++ 4 files changed, 60 insertions(+), 31 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 3d036b70b5e88..4afaf19cb22ec 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4197,24 +4197,25 @@ mono_class_set_runtime_vtable (MonoClass *klass, MonoVTable *vtable) } static int -index_of_class (MonoClass *needle, MonoClass **haystack, int haystack_size) { +index_of_class (MonoClass *needle, MonoVarianceSearchTableEntry *haystack, int haystack_size) { for (int i = 0; i < haystack_size; i++) - if (haystack[i] == needle) + if (haystack[i].klass == needle) return i; return -1; } static void -build_variance_search_table_inner (MonoClass *klass, MonoClass **buf, int buf_size, int *buf_count) { - if (!m_class_is_interfaces_inited (klass)) { +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 (klass, error); + mono_class_setup_interfaces (current, error); return_if_nok (error); } - guint c = m_class_get_interface_count (klass); + + guint c = m_class_get_interface_count (current); if (c) { - MonoClass **ifaces = m_class_get_interfaces (klass); + MonoClass **ifaces = m_class_get_interfaces (current); for (guint i = 0; i < c; i++) { MonoClass *iface = ifaces [i]; // Avoid adding duplicates or recursing into them. @@ -4222,36 +4223,41 @@ build_variance_search_table_inner (MonoClass *klass, MonoClass **buf, int buf_si continue; if (mono_class_has_variant_generic_params (iface)) { - g_assert (*buf_count < buf_size); - buf[*buf_count] = 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); } - build_variance_search_table_inner (iface, buf, buf_size, buf_count); + build_variance_search_table_inner (klass, buf, buf_size, buf_count, iface); } } + + if (current->parent) + build_variance_search_table_inner (klass, buf, buf_size, buf_count, current->parent); } typedef struct VarianceSearchTable { int count; - MonoClass *klasses[1]; // a total of count items, at least 1 + MonoVarianceSearchTableEntry entries[1]; // a total of count items, at least 1 } VarianceSearchTable; // Only call this with the loader lock held static void build_variance_search_table (MonoClass *klass) { - // FIXME: Is there a way to deterministically compute the right capacity? int buf_size = m_class_get_interface_offsets_count (klass), buf_count = 0; - MonoClass **buf = g_alloca (buf_size * sizeof(MonoClass *)); + 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); + build_variance_search_table_inner (klass, buf, buf_size, &buf_count, klass); if (buf_count) { - guint bytes = (buf_count * sizeof(MonoClass *)) + sizeof(VarianceSearchTable); + 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; - memcpy (result->klasses, buf, bytes); + memcpy (result->entries, buf, bytes); } klass->variant_search_table = result; // Ensure we do not set the inited flag until we've stored the result pointer @@ -4260,7 +4266,7 @@ build_variance_search_table (MonoClass *klass) { } void -mono_class_get_variance_search_table (MonoClass *klass, MonoClass ***table, int *table_size) { +mono_class_get_variance_search_table (MonoClass *klass, MonoVarianceSearchTableEntry **table, int *table_size) { g_assert (klass); g_assert (table); g_assert (table_size); @@ -4282,7 +4288,7 @@ mono_class_get_variance_search_table (MonoClass *klass, MonoClass ***table, int VarianceSearchTable *vst = klass->variant_search_table; if (vst) { - *table = vst->klasses; + *table = vst->entries; *table_size = vst->count; } else { *table = NULL; diff --git a/src/mono/mono/metadata/class-internals.h b/src/mono/mono/metadata/class-internals.h index 7044e59262db6..8daaeb0a6ba4e 100644 --- a/src/mono/mono/metadata/class-internals.h +++ b/src/mono/mono/metadata/class-internals.h @@ -1466,7 +1466,7 @@ gboolean mono_method_has_unmanaged_callers_only_attribute (MonoMethod *method); void -mono_class_get_variance_search_table (MonoClass *klass, MonoClass ***table, int *table_size); +mono_class_get_variance_search_table (MonoClass *klass, MonoVarianceSearchTableEntry **table, int *table_size); // There are many ways to do on-demand initialization. // Some allow multiple concurrent initializations. Some do not. diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 5e789d2af9638..13004d9884894 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -1985,39 +1985,57 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo return m_class_get_interface_offsets_packed (klass) [found]; } else if (has_variance) { - MonoClass **vst; + MonoVarianceSearchTableEntry *vst; int vst_count; - MonoClass *current = klass; + mono_class_get_variance_search_table (klass, &vst, &vst_count); + int depth = vst_count ? vst[0].depth : 0, i = 0, j; - // Perform two passes per class, then check the base class - while (current) { - mono_class_get_variance_search_table (current, &vst, &vst_count); + while (depth) { + // g_print ("depth==%d, i==%d, count==%d\n", depth, i, vst_count); // Exact match pass: Is there an exact match at this level of the type hierarchy? // If so, we can use the interface_offset we computed earlier, since we're walking from most derived to least. - for (i = 0; i < vst_count; i++) { - if (itf != vst [i]) + for (j = i; j < vst_count; j++) { + if (vst [j].depth != depth) + 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) continue; *non_exact_match = FALSE; + g_assert (vst [j].offset == exact_match); return exact_match; } // Inexact match (variance) pass: // Is any interface at this level of the type hierarchy variantly compatible with the desired interface? // If so, select the first compatible one we find. - for (i = 0; i < vst_count; i++) { - if (!mono_class_is_variant_compatible (itf, vst [i], FALSE)) + for (j = i; j < vst_count; j++) { + if (vst [j].depth < depth) { + i = j; + 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)) continue; - int inexact_match = mono_class_interface_offset (klass, vst[i]); + int inexact_match = vst [j].offset; g_assert (inexact_match != exact_match); *non_exact_match = TRUE; return inexact_match; } - // Now check base class if present - current = m_class_get_parent (current); + depth--; } // If the variance search failed to find a match, return the exact match search result (probably -1). diff --git a/src/native/public/mono/metadata/object-forward.h b/src/native/public/mono/metadata/object-forward.h index d8cca573e52ce..dce451bb0b389 100644 --- a/src/native/public/mono/metadata/object-forward.h +++ b/src/native/public/mono/metadata/object-forward.h @@ -19,4 +19,9 @@ 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__ */ From 9e2dc6910924e99f4d03ad125dd096285e6ee726 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 16 Jul 2024 19:22:05 -0700 Subject: [PATCH 3/7] Fix CI --- src/mono/mono/metadata/class.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 13004d9884894..4229570edf01e 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -1988,7 +1988,8 @@ mono_class_interface_offset_with_variance (MonoClass *klass, MonoClass *itf, gbo MonoVarianceSearchTableEntry *vst; int vst_count; mono_class_get_variance_search_table (klass, &vst, &vst_count); - int depth = vst_count ? vst[0].depth : 0, i = 0, j; + 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); From a65f23360ced205816f131b1b10b991d16352b0c Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Wed, 17 Jul 2024 18:16:03 -0700 Subject: [PATCH 4/7] Address PR feedback --- src/mono/mono/metadata/class-init.c | 71 +++++++++++-------- src/mono/mono/metadata/class-internals.h | 9 ++- src/mono/mono/metadata/class.c | 55 ++++++-------- .../public/mono/metadata/object-forward.h | 5 -- 4 files changed, 73 insertions(+), 67 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 4afaf19cb22ec..bc3559b4f3568 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/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; } } diff --git a/src/mono/mono/metadata/class-internals.h b/src/mono/mono/metadata/class-internals.h index 8daaeb0a6ba4e..d2a54413ad919 100644 --- a/src/mono/mono/metadata/class-internals.h +++ b/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. diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 4229570edf01e..834cf8688cd3e 100644 --- a/src/mono/mono/metadata/class.c +++ b/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). diff --git a/src/native/public/mono/metadata/object-forward.h b/src/native/public/mono/metadata/object-forward.h index dce451bb0b389..d8cca573e52ce 100644 --- a/src/native/public/mono/metadata/object-forward.h +++ b/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__ */ From d5315b4859b3e64593aacc5663eb397346a6f073 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Thu, 18 Jul 2024 19:55:27 -0700 Subject: [PATCH 5/7] Sanity checks --- src/mono/mono/metadata/loader.c | 1 + src/mono/mono/mini/interp/interp.c | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/loader.c b/src/mono/mono/metadata/loader.c index 467fe0e668c07..921fe29e2743e 100644 --- a/src/mono/mono/metadata/loader.c +++ b/src/mono/mono/metadata/loader.c @@ -1303,6 +1303,7 @@ get_method_constrained (MonoImage *image, MonoMethod *method, MonoClass *constra g_assert (itf_slot >= 0); gboolean variant = FALSE; int itf_base = mono_class_interface_offset_with_variance (constrained_class, base_class, &variant); + g_assert (itf_base >= 0); vtable_slot = itf_slot + itf_base; } g_assert (vtable_slot >= 0); diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index 1e31ba4cdca65..4e16a84bf9751 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -647,6 +647,10 @@ get_virtual_method (InterpMethod *imethod, MonoVTable *vtable) virtual_method = mono_marshal_get_synchronized_wrapper (virtual_method); } + // Basic sanity check since a call might crash if we have the wrong method somehow + g_assert (m->signature->param_count == virtual_method->signature->param_count); + g_assert (m->signature->hasthis == virtual_method->signature->hasthis); + InterpMethod *virtual_imethod = mono_interp_get_imethod (virtual_method); return virtual_imethod; } @@ -7479,7 +7483,7 @@ MINT_IN_CASE(MINT_BRTRUE_I8_SP) ZEROP_SP(gint64, !=); MINT_IN_BREAK; if (local_cmethod->is_generic || mono_class_is_gtd (local_cmethod->klass)) { MonoException *ex = mono_exception_from_name_msg (mono_defaults.corlib, "System", "InvalidOperationException", ""); THROW_EX (ex, ip); - } + } // FIXME push/pop LMF if (G_UNLIKELY (mono_method_has_unmanaged_callers_only_attribute (local_cmethod))) { From fcfbe2890c0a855bf9bf825fb4af3d38f9aaa644 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Fri, 19 Jul 2024 09:40:03 -0700 Subject: [PATCH 6/7] Fix heap corruption Address PR feedback More assertions --- src/mono/mono/metadata/class-init.c | 4 ++-- src/mono/mono/metadata/class-internals.h | 2 +- src/mono/mono/mini/interp/interp.c | 27 ++++++++++++++++++++++-- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index bc3559b4f3568..76459536c2497 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -4271,8 +4271,8 @@ build_variance_search_table (MonoClass *klass) { gboolean any_items = build_variance_search_table_inner (klass, buf, buf_size, &buf_count, klass, TRUE); 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); + guint bytes = (buf_count * sizeof(MonoVarianceSearchTableEntry)); + result = (VarianceSearchTable *)mono_mem_manager_alloc (m_class_get_mem_manager (klass), bytes + sizeof(VarianceSearchTable)); result->count = buf_count; memcpy (result->entries, buf, bytes); } diff --git a/src/mono/mono/metadata/class-internals.h b/src/mono/mono/metadata/class-internals.h index d2a54413ad919..fc967bb0adb5f 100644 --- a/src/mono/mono/metadata/class-internals.h +++ b/src/mono/mono/metadata/class-internals.h @@ -1466,8 +1466,8 @@ gboolean mono_method_has_unmanaged_callers_only_attribute (MonoMethod *method); typedef struct _MonoVarianceSearchTableEntry { - int depth, offset; MonoClass *klass; + guint16 offset; } MonoVarianceSearchTableEntry; MonoVarianceSearchTableEntry * diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index 4e16a84bf9751..f4c9e64106eba 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -625,6 +625,7 @@ get_virtual_method (InterpMethod *imethod, MonoVTable *vtable) } MonoMethod *virtual_method = m_class_get_vtable (vtable->klass) [slot]; + g_assert (virtual_method); if (m->is_inflated && mono_method_get_context (m)->method_inst) { MonoGenericContext context = { NULL, NULL }; @@ -648,8 +649,30 @@ get_virtual_method (InterpMethod *imethod, MonoVTable *vtable) } // Basic sanity check since a call might crash if we have the wrong method somehow - g_assert (m->signature->param_count == virtual_method->signature->param_count); - g_assert (m->signature->hasthis == virtual_method->signature->hasthis); + + MonoMethodSignature *m_sig, *virtual_method_sig; + { + ERROR_DECL (error); + m_sig = mono_method_signature_checked (m, error); + mono_error_cleanup (error); + } + { + ERROR_DECL (error); + virtual_method_sig = mono_method_signature_checked (virtual_method, error); + mono_error_cleanup (error); + } + gboolean ok = m_sig && + virtual_method_sig && + (m_sig->param_count == virtual_method_sig->param_count) && + (m_sig->hasthis == virtual_method_sig->hasthis); + if (!ok) { + g_print ( + "get_virtual_method signature mismatch for %s.%s and %s.%s\n", + m_class_get_name (m->klass), m->name, + m_class_get_name (virtual_method->klass), virtual_method->name + ); + g_assert (ok); + } InterpMethod *virtual_imethod = mono_interp_get_imethod (virtual_method); return virtual_imethod; From 0b951f7cea37c1c684fb9258ad344181ce1ce37b Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Fri, 19 Jul 2024 13:43:52 -0700 Subject: [PATCH 7/7] Remove diagnostic check --- src/mono/mono/mini/interp/interp.c | 27 +-------------------------- 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index f4c9e64106eba..61e1ff55d2f9b 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -626,6 +626,7 @@ get_virtual_method (InterpMethod *imethod, MonoVTable *vtable) MonoMethod *virtual_method = m_class_get_vtable (vtable->klass) [slot]; g_assert (virtual_method); + if (m->is_inflated && mono_method_get_context (m)->method_inst) { MonoGenericContext context = { NULL, NULL }; @@ -648,32 +649,6 @@ get_virtual_method (InterpMethod *imethod, MonoVTable *vtable) virtual_method = mono_marshal_get_synchronized_wrapper (virtual_method); } - // Basic sanity check since a call might crash if we have the wrong method somehow - - MonoMethodSignature *m_sig, *virtual_method_sig; - { - ERROR_DECL (error); - m_sig = mono_method_signature_checked (m, error); - mono_error_cleanup (error); - } - { - ERROR_DECL (error); - virtual_method_sig = mono_method_signature_checked (virtual_method, error); - mono_error_cleanup (error); - } - gboolean ok = m_sig && - virtual_method_sig && - (m_sig->param_count == virtual_method_sig->param_count) && - (m_sig->hasthis == virtual_method_sig->hasthis); - if (!ok) { - g_print ( - "get_virtual_method signature mismatch for %s.%s and %s.%s\n", - m_class_get_name (m->klass), m->name, - m_class_get_name (virtual_method->klass), virtual_method->name - ); - g_assert (ok); - } - InterpMethod *virtual_imethod = mono_interp_get_imethod (virtual_method); return virtual_imethod; }