[release/9.0-staging] [mono][interp] Fix various leaks, primarily around dynamic methods (#120600)

* [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

* [mono][interp] Fix attempt to free tiering data when tiering is disabled (#119294)

* [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

* [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).

* [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.

Author: BrzVlad

src/mono/mono/metadata/loader.c

@@ -36,6 +36,7 @@
 #include <mono/metadata/loader-internals.h>
 #include <mono/metadata/class-init.h>
 #include <mono/metadata/class-internals.h>
+#include <mono/metadata/components.h>
 #include <mono/metadata/debug-helpers.h>
 #include <mono/metadata/reflection.h>
 #include <mono/metadata/profiler-private.h>
@@ -78,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)
 {
@@ -103,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;
 	}
 }
@@ -1371,8 +1379,14 @@ 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 ()))
+	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
+	if (mono_component_event_pipe ()->component.available () ||
+			mono_component_debugger ()->component.available ())
 		return;
 
 	if (method->signature) {

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;

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) {

src/mono/mono/mini/interp/tiering.c

@@ -128,6 +128,58 @@ 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
+	// 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)
 {

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);
 

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 ());
@@ -9424,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;
@@ -9609,12 +9612,10 @@ 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;
 
 	if (td->ref_slots) {
@@ -9688,14 +9689,17 @@ 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);
 	mono_interp_pgo_generate_end ();
 	if (td->retry_compilation) {
 		retry_compilation = TRUE;
 		retry_with_inlining = td->retry_with_inlining;
 		goto retry;
 	}
+
+	return td->imethod_items;
 }
 
 gboolean
@@ -9841,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);
 
@@ -9862,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;
@@ -9871,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 ();

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;