[mono][gc] Remove reliance on regs/stack scanning in some cases (dotn…

…et#101478) * [mono][sgen] Allocated gchandle for this object when invoking finalizers We were assuming the object is kept alive from the stack/regs, which is not reliable on wasm. * [mono][metadata] Allocate handle while operating with newly allocated RuntimeType * Enable test suite
BrzVlad · May 9, 2024 · 9be6287 · 9be6287
1 parent 9926d60
commit 9be6287
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 28 deletions.
diff --git a/src/libraries/tests.proj b/src/libraries/tests.proj
@@ -395,8 +395,6 @@
     <!-- Issue: https://github.com/dotnet/runtime/issues/95795 -->
     <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Runtime\tests\System.Globalization.Tests\Hybrid\System.Globalization.Hybrid.WASM.Tests.csproj" />
     <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Runtime\tests\System.Globalization.Calendars.Tests\Hybrid\System.Globalization.Calendars.Hybrid.WASM.Tests.csproj" />
-    <!-- Issue: https://github.com/dotnet/runtime/issues/100932 -->
-    <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Runtime\tests\System.Reflection.Tests\System.Reflection.Tests.csproj" />
   </ItemGroup>
 
   <!-- Aggressive Trimming related failures -->

diff --git a/src/mono/mono/metadata/reflection.c b/src/mono/mono/metadata/reflection.c
@@ -440,6 +440,48 @@ mono_type_get_object (MonoDomain *domain, MonoType *type)
 	return ret;
 }
 
+/* LOCKING: assumes the loader lock is taken */
+static MonoReflectionType*
+mono_type_get_object_checked_alloc_helper (MonoType *type, MonoMemoryManager *memory_manager, MonoDomain *domain, MonoError *error)
+{
+	HANDLE_FUNCTION_ENTER ();
+
+	MonoReflectionType *res, *cached;
+	/* This is stored in vtables/JITted code so it has to be pinned */
+	MonoReflectionTypeHandle res_handle = MONO_HANDLE_CAST (MonoReflectionType, mono_object_new_pinned_handle (mono_defaults.runtimetype_class, error));
+	goto_if_nok (error, exit);
+
+	res = MONO_HANDLE_RAW (res_handle);
+
+	res->type = type;
+	if (memory_manager->collectible) {
+		MonoObject *loader_alloc = mono_gchandle_get_target_internal (mono_mem_manager_get_loader_alloc (memory_manager));
+		g_assert (loader_alloc);
+		MONO_OBJECT_SETREF_INTERNAL (res, m_keepalive, loader_alloc);
+	}
+
+	mono_mem_manager_lock (memory_manager);
+	if (memory_manager->collectible)
+		cached = (MonoReflectionType *)mono_weak_hash_table_lookup (memory_manager->weak_type_hash, type);
+	else
+		cached = (MonoReflectionType *)mono_g_hash_table_lookup (memory_manager->type_hash, type);
+	if (cached) {
+		res = cached;
+		MONO_HANDLE_ASSIGN_RAW (res_handle, res);
+	} else {
+		if (memory_manager->collectible)
+			mono_weak_hash_table_insert (memory_manager->weak_type_hash, type, res);
+		else
+			mono_g_hash_table_insert_internal (memory_manager->type_hash, type, res);
+		if (type->type == MONO_TYPE_VOID && !m_type_is_byref (type))
+			domain->typeof_void = (MonoObject*)res;
+	}
+	mono_mem_manager_unlock (memory_manager);
+
+exit:
+	HANDLE_FUNCTION_RETURN_OBJ (res_handle);
+}
+
 MonoReflectionType*
 mono_type_get_object_checked (MonoType *type, MonoError *error)
 {
@@ -554,33 +596,9 @@ mono_type_get_object_checked (MonoType *type, MonoError *error)
 		res = &mono_class_get_ref_info_raw (klass)->type; /* FIXME use handles */
 		goto leave;
 	}
-	/* This is stored in vtables/JITted code so it has to be pinned */
-	res = (MonoReflectionType *)mono_object_new_pinned (mono_defaults.runtimetype_class, error);
-	goto_if_nok (error, leave);
 
-	res->type = type;
-	if (memory_manager->collectible) {
-		MonoObject *loader_alloc = mono_gchandle_get_target_internal (mono_mem_manager_get_loader_alloc (memory_manager));
-		g_assert (loader_alloc);
-		MONO_OBJECT_SETREF_INTERNAL (res, m_keepalive, loader_alloc);
-	}
 
-	mono_mem_manager_lock (memory_manager);
-	if (memory_manager->collectible)
-		cached = (MonoReflectionType *)mono_weak_hash_table_lookup (memory_manager->weak_type_hash, type);
-	else
-		cached = (MonoReflectionType *)mono_g_hash_table_lookup (memory_manager->type_hash, type);
-	if (cached) {
-		res = cached;
-	} else {
-		if (memory_manager->collectible)
-			mono_weak_hash_table_insert (memory_manager->weak_type_hash, type, res);
-		else
-			mono_g_hash_table_insert_internal (memory_manager->type_hash, type, res);
-		if (type->type == MONO_TYPE_VOID && !m_type_is_byref (type))
-			domain->typeof_void = (MonoObject*)res;
-	}
-	mono_mem_manager_unlock (memory_manager);
+	res = mono_type_get_object_checked_alloc_helper (type, memory_manager, domain, error);
 
 leave:
 	mono_loader_unlock ();

diff --git a/src/mono/mono/sgen/sgen-gc.c b/src/mono/mono/sgen/sgen-gc.c
@@ -2848,6 +2848,9 @@ sgen_gc_invoke_finalizers (void)
 
 	g_assert (!pending_unqueued_finalizer);
 
+	gboolean gchandle_allocated = FALSE;
+	guint32 gchandle = 0;
+
 	/* FIXME: batch to reduce lock contention */
 	while (sgen_have_pending_finalizers ()) {
 		GCObject *obj;
@@ -2878,8 +2881,16 @@ sgen_gc_invoke_finalizers (void)
 		if (!obj)
 			break;
 
+		// We explicitly pin the object via a gchandle so we don't rely on the ref being
+		// present on stack/regs which is not scannable on WASM.
+		if (!gchandle_allocated) {
+			gchandle = sgen_gchandle_new (obj, TRUE);
+			gchandle_allocated = TRUE;
+		} else {
+			sgen_gchandle_set_target (gchandle, obj);
+		}
+
 		count++;
-		/* the object is on the stack so it is pinned */
 		/*g_print ("Calling finalizer for object: %p (%s)\n", obj, sgen_client_object_safe_name (obj));*/
 		sgen_client_run_finalize (obj);
 	}
@@ -2889,6 +2900,9 @@ sgen_gc_invoke_finalizers (void)
 		pending_unqueued_finalizer = FALSE;
 	}
 
+	if (gchandle_allocated)
+		sgen_gchandle_free (gchandle);
+
 	return count;
 }