From 7107d2c2e38fe18c02acdb06e0d9b0cf473bf60e Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Fri, 29 Aug 2025 19:38:56 +0300 Subject: [PATCH 1/5] [mono][interp] Fix various leaks, primarily around dynamic methods (#119176) * [mono][interp] Fix leaking of compilation data For dynamic methods, imethod_alloc0 allocates from the dynamic method's mempool which is freed when the method is collected. We were previously allocating only from the global memory manager. * [mono][interp] Stop leaking data items Previously we were registering imethod locations from these data items in order for them to be patched once a method is tiered up. Because of this registering, the data item had to always be around. We now free the data item for dynamic methods and also we deregister the patch locations when freeing such a method. * [mono][interp] Free headers allocated for inlined methods We add them to a list for later freeing. This uses the same pattern as jit. * [mono][interp] Skip interp free for methods not yet compiled --- src/mono/mono/mini/interp/interp-internals.h | 1 + src/mono/mono/mini/interp/interp.c | 11 +++-- src/mono/mono/mini/interp/tiering.c | 50 ++++++++++++++++++++ src/mono/mono/mini/interp/tiering.h | 3 ++ src/mono/mono/mini/interp/transform.c | 14 ++++-- src/mono/mono/mini/interp/transform.h | 1 + 6 files changed, 72 insertions(+), 8 deletions(-) diff --git a/src/mono/mono/mini/interp/interp-internals.h b/src/mono/mono/mini/interp/interp-internals.h index 5fe04bf314c8c7..41407a2fd67235 100644 --- a/src/mono/mono/mini/interp/interp-internals.h +++ b/src/mono/mono/mini/interp/interp-internals.h @@ -148,6 +148,7 @@ struct InterpMethod { /* locals_size is equal to the offset of the param_area */ guint32 locals_size; guint32 alloca_size; + int n_data_items; int num_clauses; // clauses int transformed; // boolean unsigned int param_count; diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index 011d99a9625dda..91966a0b18e782 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -3565,12 +3565,17 @@ interp_free_method (MonoMethod *method) jit_mm_lock (jit_mm); -#if HOST_BROWSER InterpMethod *imethod = (InterpMethod*)mono_internal_hash_table_lookup (&jit_mm->interp_code_hash, method); - mono_jiterp_free_method_data (method, imethod); + if (imethod) { +#if HOST_BROWSER + mono_jiterp_free_method_data (method, imethod); #endif - mono_internal_hash_table_remove (&jit_mm->interp_code_hash, method); + mono_interp_clear_data_items_patch_sites (imethod->data_items, imethod->n_data_items); + + mono_internal_hash_table_remove (&jit_mm->interp_code_hash, method); + } + jit_mm_unlock (jit_mm); if (dmethod->mp) { diff --git a/src/mono/mono/mini/interp/tiering.c b/src/mono/mono/mini/interp/tiering.c index a49b591e9d090e..6d332acf76bd17 100644 --- a/src/mono/mono/mini/interp/tiering.c +++ b/src/mono/mono/mini/interp/tiering.c @@ -128,6 +128,56 @@ register_imethod_data_item (gpointer data, gpointer user_data) } } +void +mono_interp_clear_data_items_patch_sites (gpointer *data_items, int n_data_items) +{ + // data_items is part of the memory of a dynamic method that is being freed. + // slots within this memory can be registered as patch sites for other imethods + // We conservatively assume each slot could be an imethod slot, then look it up + // in imethod to patch_sites hashtable. If we find it in the hashtable, we remove + // the slot from the patch site list. + mono_os_mutex_lock (&tiering_mutex); + + for (int i = 0; i < n_data_items; i++) { + GSList *sites; + gpointer *slot = data_items + i; + gpointer imethod_candidate = *slot; + + if (dn_simdhash_ptr_ptr_try_get_value (patch_sites_table, imethod_candidate, (void **)&sites)) { + GSList *prev = NULL; + + // Remove slot from sites list + if (sites->data == slot) { + // If the slot is found in the first element we will also need to update the hash table since + // the list head changes + if (!sites->next) { + g_slist_free_1 (sites); + dn_simdhash_ptr_ptr_try_remove (patch_sites_table, imethod_candidate); + } else { + prev = sites; + sites = sites->next; + g_slist_free_1 (prev); + dn_simdhash_ptr_ptr_try_replace_value (patch_sites_table, imethod_candidate, sites); + } + } else { + prev = sites; + sites = sites->next; + while (sites != NULL) { + if (sites->data == slot) { + prev->next = sites->next; + g_slist_free_1 (sites); + // duplicates not allowed + break; + } + prev = sites; + sites = sites->next; + } + } + } + } + mono_os_mutex_unlock (&tiering_mutex); +} + void mono_interp_register_imethod_data_items (gpointer *data_items, GSList *indexes) { diff --git a/src/mono/mono/mini/interp/tiering.h b/src/mono/mono/mini/interp/tiering.h index dbd7da87ecd4d9..c50e4988b43972 100644 --- a/src/mono/mono/mini/interp/tiering.h +++ b/src/mono/mono/mini/interp/tiering.h @@ -14,6 +14,9 @@ mono_interp_tiering_enabled (void); void mono_interp_register_imethod_data_items (gpointer *data_items, GSList *indexes); +void +mono_interp_clear_data_items_patch_sites (gpointer *data_items, int n_data_items); + void mono_interp_register_imethod_patch_site (gpointer *imethod_ptr); diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index a74e9baafb300b..e9ac3620e5b9fd 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -3176,6 +3176,8 @@ interp_inline_newobj (TransformData *td, MonoMethod *target_method, MonoMethodSi if (!interp_inline_method (td, target_method, mheader, error)) goto fail; + td->headers_to_free = g_slist_prepend_mempool (td->mempool, td->headers_to_free, mheader); + push_var (td, dreg); return TRUE; fail: @@ -3760,6 +3762,7 @@ interp_transform_call (TransformData *td, MonoMethod *method, MonoMethod *target return_val_if_nok (error, FALSE); if (interp_inline_method (td, target_method, mheader, error)) { + td->headers_to_free = g_slist_prepend_mempool (td->mempool, td->headers_to_free, mheader); td->ip += 5; goto done; } @@ -4510,7 +4513,7 @@ interp_method_compute_offsets (TransformData *td, InterpMethod *imethod, MonoMet // 64 vars * 72 bytes = 4608 bytes. Many methods need less than this int target_vars_capacity = num_locals + 64; - imethod->local_offsets = (guint32*)g_malloc (num_il_locals * sizeof(guint32)); + imethod->local_offsets = (guint32*)imethod_alloc0 (td, num_il_locals * sizeof(guint32)); td->vars = (InterpVar*)g_malloc0 (target_vars_capacity * sizeof (InterpVar)); td->vars_size = num_locals; td->vars_capacity = target_vars_capacity; @@ -4608,7 +4611,7 @@ interp_method_compute_offsets (TransformData *td, InterpMethod *imethod, MonoMet } #endif - imethod->clause_data_offsets = (guint32*)g_malloc (header->num_clauses * sizeof (guint32)); + imethod->clause_data_offsets = (guint32*)imethod_alloc0 (td, header->num_clauses * sizeof (guint32)); td->clause_vars = (int*)mono_mempool_alloc (td->mempool, sizeof (int) * header->num_clauses); for (guint i = 0; i < header->num_clauses; i++) { int var = interp_create_var (td, mono_get_object_type ()); @@ -9609,10 +9612,9 @@ generate (MonoMethod *method, MonoMethodHeader *header, InterpMethod *rtm, MonoG rtm->alloca_size = td->total_locals_size + td->max_stack_size; g_assert ((rtm->alloca_size % MINT_STACK_ALIGNMENT) == 0); rtm->locals_size = td->param_area_offset; - // FIXME: Can't allocate this using imethod_alloc0 as its registered with mono_interp_register_imethod_data_items () - //rtm->data_items = (gpointer*)imethod_alloc0 (td, td->n_data_items * sizeof (td->data_items [0])); - rtm->data_items = (gpointer*)mono_mem_manager_alloc0 (td->mem_manager, td->n_data_items * sizeof (td->data_items [0])); + rtm->data_items = (gpointer*)imethod_alloc0 (td, td->n_data_items * sizeof (td->data_items [0])); memcpy (rtm->data_items, td->data_items, td->n_data_items * sizeof (td->data_items [0])); + rtm->n_data_items = td->n_data_items; mono_interp_register_imethod_data_items (rtm->data_items, td->imethod_items); rtm->patchpoint_data = td->patchpoint_data; @@ -9689,6 +9691,8 @@ generate (MonoMethod *method, MonoMethodHeader *header, InterpMethod *rtm, MonoG if (td->line_numbers) g_array_free (td->line_numbers, TRUE); g_slist_free (td->imethod_items); + for (GSList *l = td->headers_to_free; l; l = l->next) + mono_metadata_free_mh ((MonoMethodHeader *)l->data); mono_mempool_destroy (td->mempool); mono_interp_pgo_generate_end (); if (td->retry_compilation) { diff --git a/src/mono/mono/mini/interp/transform.h b/src/mono/mono/mini/interp/transform.h index 0379cf1682f6a3..d851f51762e338 100644 --- a/src/mono/mono/mini/interp/transform.h +++ b/src/mono/mono/mini/interp/transform.h @@ -314,6 +314,7 @@ typedef struct // FIXME: ptr_u32 dn_simdhash_ptr_ptr_t *data_hash; GSList *imethod_items; + GSList *headers_to_free; #ifdef ENABLE_EXPERIMENT_TIERED // FIXME: ptr_u32 dn_simdhash_ptr_ptr_t *patchsite_hash; From 118620d197d0870d095be09097a7a22365e3fc56 Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Tue, 9 Sep 2025 17:56:24 +0300 Subject: [PATCH 2/5] [mono][interp] Fix attempt to free tiering data when tiering is disabled (#119294) --- src/mono/mono/mini/interp/tiering.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/mono/mono/mini/interp/tiering.c b/src/mono/mono/mini/interp/tiering.c index 6d332acf76bd17..54708d4f4a0510 100644 --- a/src/mono/mono/mini/interp/tiering.c +++ b/src/mono/mono/mini/interp/tiering.c @@ -131,6 +131,8 @@ register_imethod_data_item (gpointer data, gpointer user_data) void mono_interp_clear_data_items_patch_sites (gpointer *data_items, int n_data_items) { + if (!enable_tiering) + return; // data_items is part of the memory of a dynamic method that is being freed. // slots within this memory can be registered as patch sites for other imethods // We conservatively assume each slot could be an imethod slot, then look it up From 73785c3aa33340a0bafc42b749cedbe76fbf1225 Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Mon, 22 Sep 2025 15:14:39 +0300 Subject: [PATCH 3/5] [mono] Actually free dynamic methods, even if we have profiler attached (#119749) * [mono] Actually free dynamic methods, even if we have profiler attached Ever since the early days of mono, freeing dynamic methods was disabled if a profiler was attached. The reason for this was probably that the profiler might store `MonoMethod` instances in its own data, leading to problems if we free those instances. Looking at the profilers nowadays it is not clear where patterns like this would happen. Profilers that do store methods (like aot, coverage), don't process wrapper methods, so we should be safe since dynamic methods have the MONO_WRAPPER_DYNAMIC_METHOD wrapper type. The problem is that, nowadays, we can always have a profiler attached even if no actual profiling happens. macios for example always calls mono_profiler_install. I belive actual callbacks can be added later as necessary. * Disable freeing dynamic methods only when eventpipe or debugger are enabled --- src/mono/mono/metadata/loader.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/mono/mono/metadata/loader.c b/src/mono/mono/metadata/loader.c index 921fe29e2743e1..58ccd6afd15a70 100644 --- a/src/mono/mono/metadata/loader.c +++ b/src/mono/mono/metadata/loader.c @@ -36,6 +36,7 @@ #include #include #include +#include #include #include #include @@ -1371,8 +1372,11 @@ mono_free_method (MonoMethod *method) MONO_PROFILER_RAISE (method_free, (method)); - /* FIXME: This hack will go away when the profiler will support freeing methods */ - if (G_UNLIKELY (mono_profiler_installed ())) + // EventPipe might require information about methods to be stored throughout + // entire app execution, so stack traces can be resolved at a later time. + // Same for debugger, we are being overly conservative + if (mono_component_event_pipe ()->component.available () || + mono_component_debugger ()->component.available ()) return; if (method->signature) { From 34df24510fd3fb794860fd373415cbc24bd00a22 Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Wed, 24 Sep 2025 16:16:32 +0300 Subject: [PATCH 4/5] [mono][interp] Fix race when registering patch point data items (#119990) The code of a compiled method can use data items that contain InterpMethod* pointers. Because at the moment when a method is compiled these referenced interp methods weren't yet tiered up, we will register these data item locations to the tiering backend so that, when they do get tiered up, the locations get updated with the new InterpMethod* reference. At this moment of compilation time, we could race with other compilers of the same InterpMethod so we could end up registering redundant data_items (only the data_items for the method winning the race will actually end up being used by the executing interpreter code). Normally this is harmless, we just patch some data that never gets used. The problems start when we free interpreter methods. When the interp method is freed, we need to clear the patch_sites_table of any locations pointing into this method's data items. We do a search starting from the data items of this method. Because we have no way to reference the wrongly registered data items, these entries will stay alive in the table. As the memory gets reused, these entries will point into other runtime code, and when the get patched, it will result in corruption of memory. We fix this issue by registering the patch sites only for the method winning the compilation race (the one that first sets the transformed flag). --- src/mono/mono/mini/interp/transform.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index e9ac3620e5b9fd..10fbe877db711c 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -9427,7 +9427,7 @@ get_native_offset (TransformData *td, int il_offset) } } -static void +static GSList* generate (MonoMethod *method, MonoMethodHeader *header, InterpMethod *rtm, MonoGenericContext *generic_context, MonoError *error) { TransformData transform_data; @@ -9616,7 +9616,6 @@ generate (MonoMethod *method, MonoMethodHeader *header, InterpMethod *rtm, MonoG memcpy (rtm->data_items, td->data_items, td->n_data_items * sizeof (td->data_items [0])); rtm->n_data_items = td->n_data_items; - mono_interp_register_imethod_data_items (rtm->data_items, td->imethod_items); rtm->patchpoint_data = td->patchpoint_data; if (td->ref_slots) { @@ -9690,7 +9689,6 @@ generate (MonoMethod *method, MonoMethodHeader *header, InterpMethod *rtm, MonoG g_ptr_array_free (td->seq_points, TRUE); if (td->line_numbers) g_array_free (td->line_numbers, TRUE); - g_slist_free (td->imethod_items); for (GSList *l = td->headers_to_free; l; l = l->next) mono_metadata_free_mh ((MonoMethodHeader *)l->data); mono_mempool_destroy (td->mempool); @@ -9700,6 +9698,8 @@ generate (MonoMethod *method, MonoMethodHeader *header, InterpMethod *rtm, MonoG retry_with_inlining = td->retry_with_inlining; goto retry; } + + return td->imethod_items; } gboolean @@ -9845,7 +9845,8 @@ mono_interp_transform_method (InterpMethod *imethod, ThreadContext *context, Mon memcpy (&tmp_imethod, imethod, sizeof (InterpMethod)); imethod = &tmp_imethod; - MONO_TIME_TRACK (mono_interp_stats.transform_time, generate (method, header, imethod, generic_context, error)); + GSList *imethod_data_items; + MONO_TIME_TRACK (mono_interp_stats.transform_time, imethod_data_items = generate (method, header, imethod, generic_context, error)); mono_metadata_free_mh (header); @@ -9866,6 +9867,8 @@ mono_interp_transform_method (InterpMethod *imethod, ThreadContext *context, Mon mono_interp_stats.methods_transformed++; mono_atomic_fetch_add_i32 (&mono_jit_stats.methods_with_interp, 1); + mono_interp_register_imethod_data_items (imethod->data_items, imethod_data_items); + // FIXME Publishing of seq points seems to be racy with tiereing. We can have both tiered and untiered method // running at the same time. We could therefore get the optimized imethod seq points for the unoptimized method. gpointer seq_points = NULL; @@ -9875,6 +9878,8 @@ mono_interp_transform_method (InterpMethod *imethod, ThreadContext *context, Mon } jit_mm_unlock (jit_mm); + g_slist_free (imethod_data_items); + if (mono_stats_method_desc && mono_method_desc_full_match (mono_stats_method_desc, imethod->method)) { g_printf ("Printing runtime stats at method: %s\n", mono_method_get_full_name (imethod->method)); mono_runtime_print_stats (); From 3491f747d408b370d2fe82d1635d5c2df25e4139 Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Fri, 10 Oct 2025 09:29:16 +0300 Subject: [PATCH 5/5] [mono] Enable dynfree of methods only if env var is passed If the MONO_ENABLE_DYNMETHOD_FREE is not passed, the behavior is not changed at all in order to mitigate potential risk. The old behavior was that if we have a profiler installed we don't free dynamic methods, which was always the case on maui. --- src/mono/mono/metadata/loader.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/mono/mono/metadata/loader.c b/src/mono/mono/metadata/loader.c index 58ccd6afd15a70..ea4195287e79b9 100644 --- a/src/mono/mono/metadata/loader.c +++ b/src/mono/mono/metadata/loader.c @@ -79,6 +79,8 @@ static gint32 memberref_sig_cache_size; static gint32 methods_size; static gint32 signatures_size; +static gboolean mono_enable_dynfree = FALSE; + void mono_loader_init (void) { @@ -104,6 +106,11 @@ mono_loader_init (void) mono_counters_register ("MonoMethodSignature size", MONO_COUNTER_METADATA | MONO_COUNTER_INT, &signatures_size); + char *env_opt = g_getenv ("MONO_ENABLE_DYNMETHOD_FREE"); + if (env_opt && env_opt [0] == '1') + mono_enable_dynfree = TRUE; + g_free (env_opt); + inited = TRUE; } } @@ -1372,6 +1379,9 @@ mono_free_method (MonoMethod *method) MONO_PROFILER_RAISE (method_free, (method)); + if (G_UNLIKELY (mono_profiler_installed () && !mono_enable_dynfree)) + return; + // EventPipe might require information about methods to be stored throughout // entire app execution, so stack traces can be resolved at a later time. // Same for debugger, we are being overly conservative