[threads] Switch foreign threads to GC Safe in mono_thread_detach (#42758)

* [test] Invoke from foreign threads then try to GC and shutdown

   Even if the non-Mono threads that once called Mono are looping or deadlocked,
   they shouldn't prevent us from doing a GC (in hybrid suspend mode) or from
   shutting down if they detached from the runtime.

* [tests] update .gitignore

   Ignore AOT build artifacts in subdirectories of mono/tests/, too

* [threads] Make mono_thread_attach external only

   Runtime should use mono_thread_internal_attach

* [threads] Mark mono_thread_detach external only; switch to GC Safe

   Runtime threads should call mono_thread_internal_detach

   Addresses mono/mono#20290 and mono/mono#20283

   If a foreign thread (that was created outside the runtime) calls
   mono_thread_detach, leave it in a preemptively-suspendable state, since we
   can't expect it to coop suspend.

   Conversely in mono_thread_attach (external only), ensure that we always leave
   the thread in GC Unsafe (aka RUNNING) state, for cases like
   
       while (cond) {
         t = mono_thread_attach (domain);
         <...>
         mono_thread_detach (t);
       }

* Tests fixup

   Delete test that invokes the runtime in a loop forever.
   This is just exercising a race between mono_thread_attach and the
   runtime shutdown.  Instead update the invoke_foreign_thread test to
   loop a few times to check that attach/detach loops are ok.

  In the deadlock test, wait for the foreign thread to finish calling
  the runtime before the test thread returns from native back to managed
   to avoid a race between shutdown and the invoke.


* [interp] Set context to null when freeing

   If a foreign thread runs this loop

       for (...) {
         mono_thread_attach;
         mono_runtime_invoke;
         mono_thread_detach;
      }

   on the second iteration it will get a ThreadContext that was already freed
   during the detach.  Set the TLS variable to null before freeing the context.


* [threads] Switch to GC Unsafe before creating managed thread object

   For a re-attaching move the thread state transition to happen earlier so that
   create_internal_thread_object (which does a managed allocation) is always done
   in GC Unsafe mode.


* fixup eventpipe to use mono_thread_internal_{attach,detach}

Co-authored-by: lambdageek <[email protected]>
Co-authored-by: Aleksey Kliger <[email protected]>

Author: monojenkins

src/mono/mono/eventpipe/ep-rt-mono.h

@@ -1528,7 +1528,7 @@ ep_rt_thread_setup (bool background_thread)
 #ifdef EP_RT_MONO_USE_STATIC_RUNTIME
 	// NOTE, under netcore, only root domain exists.
 	if (!mono_thread_current ()) {
-		MonoThread *thread = mono_thread_attach (mono_get_root_domain ());
+		MonoThread *thread = mono_thread_internal_attach (mono_get_root_domain ());
 		if (background_thread && thread) {
 			mono_thread_set_state (thread, ThreadState_Background);
 			mono_thread_info_set_flags (MONO_THREAD_INFO_FLAGS_NO_SAMPLE);
@@ -1547,7 +1547,7 @@ ep_rt_thread_teardown (void)
 #ifdef EP_RT_MONO_USE_STATIC_RUNTIME
 	MonoThread *current_thread = mono_thread_current ();
 	if (current_thread)
-		mono_thread_detach (current_thread);
+		mono_thread_internal_detach (current_thread);
 #else
 	ep_rt_mono_func_table_get ()->ep_rt_mono_thread_detach ();
 #endif

src/mono/mono/metadata/appdomain.c

@@ -349,7 +349,7 @@ mono_runtime_init_checked (MonoDomain *domain, MonoThreadStartCB start_cb, MonoT
 		domain->setup = MONO_HANDLE_RAW (setup);
 	}
 
-	mono_thread_attach (domain);
+	mono_thread_internal_attach (domain);
 
 #if defined(ENABLE_PERFTRACING) && !defined(DISABLE_EVENTPIPE)
 	ds_server_init ();

src/mono/mono/metadata/cominterop.c

@@ -2859,7 +2859,7 @@ cominterop_ccw_queryinterface_impl (MonoCCWInterface* ccwe, const guint8* riid,
 		*ppv = NULL;
 
 	if (!mono_domain_get ())
-		mono_thread_attach (mono_get_root_domain ());
+		mono_thread_attach_external_native_thread (mono_get_root_domain (), FALSE);
 
 	/* handle IUnknown special */
 	if (cominterop_class_guid_equal (riid, mono_class_get_iunknown_class ())) {
@@ -2990,7 +2990,7 @@ cominterop_ccw_get_ids_of_names_impl (MonoCCWInterface* ccwe, gpointer riid,
 	klass = mono_object_class (object);
 
 	if (!mono_domain_get ())
-		 mono_thread_attach (mono_get_root_domain ());
+		mono_thread_attach_external_native_thread (mono_get_root_domain (), FALSE);
 
 	for (i=0; i < cNames; i++) {
 		methodname = mono_unicode_to_external (rgszNames[i]);

src/mono/mono/metadata/icall-eventpipe.c

@@ -113,7 +113,7 @@ eventpipe_thread_attach (gboolean background_thread)
 
 	// NOTE, under netcore, only root domain exists.
 	if (!mono_thread_current ()) {
-		thread = mono_thread_attach (mono_get_root_domain ());
+		thread = mono_thread_internal_attach (mono_get_root_domain ());
 		if (background_thread) {
 			mono_thread_set_state (thread, ThreadState_Background);
 			mono_thread_info_set_flags (MONO_THREAD_INFO_FLAGS_NO_SAMPLE);
@@ -129,7 +129,7 @@ eventpipe_thread_detach (void)
 {
 	MonoThread *current_thread = mono_thread_current ();
 	if (current_thread)
-		mono_thread_detach (current_thread);
+		mono_thread_internal_detach (current_thread);
 }
 
 void

src/mono/mono/metadata/threads-types.h

@@ -425,6 +425,16 @@ void mono_threads_add_joinable_thread (gpointer tid);
 void mono_threads_join_threads (void);
 void mono_thread_join (gpointer tid);
 
+MONO_PROFILER_API MonoThread*
+mono_thread_internal_attach (MonoDomain *domain);
+
+MONO_PROFILER_API void
+mono_thread_internal_detach (MonoThread *thread);
+
+MonoThread *
+mono_thread_attach_external_native_thread (MonoDomain *domain, gboolean background);
+
+
 MONO_API gpointer
 mono_threads_attach_coop (MonoDomain *domain, gpointer *dummy);
 

src/mono/mono/metadata/threads.c

@@ -702,6 +702,8 @@ init_internal_thread_object (MonoInternalThread *thread)
 static MonoInternalThread*
 create_internal_thread_object (void)
 {
+	MONO_REQ_GC_UNSAFE_MODE;
+
 	ERROR_DECL (error);
 	MonoInternalThread *thread;
 	MonoVTable *vt;
@@ -1525,6 +1527,58 @@ mono_thread_create_checked (MonoDomain *domain, gpointer func, gpointer arg, Mon
  */
 MonoThread *
 mono_thread_attach (MonoDomain *domain)
+{
+	return mono_thread_attach_external_native_thread (domain, FALSE);
+}
+
+/**
+ * mono_thread_attach_external_native_thread:
+ *
+ * Attach the current thread (that was created outside the runtime or managed
+ * code) to the runtime.  If \p background is TRUE, set the IsBackground
+ * property on the thread.
+ *
+ * COOP: On return, the thread is in GC Unsafe mode
+ */
+MonoThread *
+mono_thread_attach_external_native_thread (MonoDomain *domain, gboolean background)
+{
+	MonoThread *thread = mono_thread_internal_attach (domain);
+
+	if (background)
+		mono_thread_set_state (mono_thread_internal_current (), ThreadState_Background);
+
+#if 0
+	/* Can't do this - would break embedders who do their own GC thread
+	 * state transitions.  Also while the conversion of MONO_API entry
+	 * points to do a transition to GC Unsafe is not complete, doing a
+	 * transition here potentially means running runtime code while in GC
+	 * Safe mode.
+	 */
+	if (mono_threads_is_blocking_transition_enabled ()) {
+		/* mono_jit_thread_attach and mono_thread_attach are external-only and
+		 * not called by the runtime on any of our own threads.  So if we get
+		 * here, the thread is running native code - leave it in GC Safe mode
+		 * and leave it to the n2m invoke wrappers or MONO_API entry points to
+		 * switch to GC Unsafe.
+		 */
+		MONO_STACKDATA (stackdata);
+		mono_threads_enter_gc_safe_region_unbalanced_internal (&stackdata);
+	}
+#endif
+	return thread;
+}
+
+/**
+ * mono_thread_internal_attach:
+ *
+ * Attach the current thread to the runtime.  The thread was created on behalf
+ * of the runtime and the runtime is responsible for it.
+ *
+ * COOP: On return, the thread is in GC Unsafe mode
+ */
+MonoThread *
+mono_thread_internal_attach (MonoDomain *domain)
 {
 	MonoInternalThread *internal;
 	MonoThread *thread;
@@ -1538,7 +1592,34 @@ mono_thread_attach (MonoDomain *domain)
 		return mono_thread_current ();
 	}
 
-	info = mono_thread_info_attach ();
+	if (G_UNLIKELY ((info = mono_thread_info_current_unchecked ()))) {
+		/* 
+		 * We are not attached currently, but we were earlier.  Ensure the thread is in GC Unsafe mode.
+		 * Have to do this before creating the managed thread object.
+		 *
+		 */
+		if (mono_threads_is_blocking_transition_enabled ()) {
+			/*
+			 * Ensure the thread is in RUNNING state.
+			 * If the thread is doing something like this
+			 *
+			 * while (cond) {
+			 *   t = mono_thread_attach (domain);
+			 *   <...>
+			 *   mono_thread_detach (t);
+			 * }
+			 *
+			 * The call to mono_thread_detach will put it in GC Safe
+			 * (blocking, preemptive suspend) mode, so the next time we
+			 * come back to attach, we need to switch to GC Unsafe
+			 * (running, cooperative suspend) mode.
+			 */
+			MONO_STACKDATA (stackdata);
+			mono_threads_enter_gc_unsafe_region_unbalanced_internal (&stackdata);
+		}
+	} else {
+		info = mono_thread_info_attach ();
+	}
 	g_assert (info);
 
 	tid=mono_native_thread_id_get ();
@@ -1596,12 +1677,39 @@ mono_threads_attach_tools_thread (void)
 
 /**
  * mono_thread_detach:
+ *
+ * COOP: On return, the thread is in GC Safe mode
  */
 void
 mono_thread_detach (MonoThread *thread)
 {
-	if (thread)
-		mono_thread_detach_internal (thread->internal_thread);
+	if (!thread)
+		return;
+	mono_thread_internal_detach (thread);
+	/*
+	 * If the thread wasn't created by the runtime, leave it in GC
+	 * Safe mode.  Under hybrid and coop suspend, we don't want to
+	 * wait for it to cooperatively suspend.
+	 */
+	if (mono_threads_is_blocking_transition_enabled ()) {
+		MONO_STACKDATA (stackdata);
+		mono_threads_enter_gc_safe_region_unbalanced_internal (&stackdata);
+	}
+}
+
+/**
+ * mono_thread_internal_detach:
+ *
+ * COOP: GC thread state is unchanged
+ */
+void
+mono_thread_internal_detach (MonoThread *thread)
+{
+	if (!thread)
+		return;
+	MONO_ENTER_GC_UNSAFE;
+	mono_thread_detach_internal (thread->internal_thread);
+	MONO_EXIT_GC_UNSAFE;
 }
 
 
@@ -3789,6 +3897,8 @@ mono_thread_manage_internal (void)
 	 * Also abort all the background threads
 	 * */
 	do {
+		THREAD_DEBUG (g_message ("%s: abort phase", __func__));
+
 		mono_threads_lock ();
 
 		wait->num = 0;
@@ -6074,15 +6184,15 @@ mono_threads_attach_coop_internal (MonoDomain *domain, gpointer *cookie, MonoSta
 		external = !(info = mono_thread_info_current_unchecked ()) || !mono_thread_info_is_live (info);
 
 	if (!mono_thread_internal_current ()) {
-		mono_thread_attach (domain);
+		mono_thread_internal_attach (domain);
 
 		// #678164
 		mono_thread_set_state (mono_thread_internal_current (), ThreadState_Background);
 	}
 
 	if (mono_threads_is_blocking_transition_enabled ()) {
 		if (external) {
-			/* mono_thread_attach put the thread in RUNNING mode from STARTING, but we need to
+			/* mono_thread_internal_attach put the thread in RUNNING mode from STARTING, but we need to
 			 * return the right cookie. */
 			*cookie = mono_threads_enter_gc_unsafe_region_cookie ();
 		} else {

src/mono/mono/metadata/threads.h

@@ -40,8 +40,10 @@ MONO_API void mono_thread_new_init (intptr_t tid, void* stack_start,
 MONO_API MONO_RT_EXTERNAL_ONLY void
 mono_thread_create (MonoDomain *domain, void* func, void* arg);
 
-MONO_API MonoThread *mono_thread_attach (MonoDomain *domain);
-MONO_API void mono_thread_detach (MonoThread *thread);
+MONO_API MONO_RT_EXTERNAL_ONLY MonoThread *
+mono_thread_attach (MonoDomain *domain);
+MONO_API MONO_RT_EXTERNAL_ONLY void
+mono_thread_detach (MonoThread *thread);
 MONO_API void mono_thread_exit (void);
 
 MONO_API MONO_RT_EXTERNAL_ONLY void

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

@@ -407,6 +407,14 @@ interp_free_context (gpointer ctx)
 {
 	ThreadContext *context = (ThreadContext*)ctx;
 
+	ThreadContext *current_context = (ThreadContext *) mono_native_tls_get_value (thread_context_id);
+	/* at thread exit, we can be called from the JIT TLS key destructor with current_context == NULL */
+	if (current_context != NULL) {
+		/* check that the context we're freeing is the current one before overwriting TLS */
+		g_assert (context == current_context);
+		set_context (NULL);
+	}
+
 	mono_vfree (context->stack_start, INTERP_STACK_SIZE, MONO_MEM_ACCOUNT_INTERP_STACK);
 	/* Prevent interp_mark_stack from trying to scan the data_stack, before freeing it */
 	context->stack_start = NULL;

src/mono/mono/mini/mini-runtime.c

@@ -866,10 +866,9 @@ mono_jit_thread_attach (MonoDomain *domain)
 	attached = mono_tls_get_jit_tls () != NULL;
 
 	if (!attached) {
-		mono_thread_attach (domain);
-
 		// #678164
-		mono_thread_set_state (mono_thread_internal_current (), ThreadState_Background);
+		gboolean background = TRUE;
+		mono_thread_attach_external_native_thread (domain, background);
 
 		/* mono_jit_thread_attach is external-only and not called by
 		 * the runtime on any of our own threads.  So if we get here,
@@ -4649,7 +4648,7 @@ mini_init (const char *filename, const char *runtime_version)
 	mono_install_runtime_cleanup (runtime_cleanup);
 	mono_runtime_init_checked (domain, (MonoThreadStartCB)mono_thread_start_cb, mono_thread_attach_cb, error);
 	mono_error_assert_ok (error);
-	mono_thread_attach (domain);
+	mono_thread_internal_attach (domain);
 	MONO_PROFILER_RAISE (thread_name, (MONO_NATIVE_THREAD_ID_TO_UINT (mono_native_thread_id_get ()), "Main"));
 #endif
 	mono_threads_set_runtime_startup_finished ();

src/mono/mono/profiler/aot.c

@@ -213,7 +213,7 @@ static void prof_save (MonoProfiler *prof, FILE* file);
 static void *
 helper_thread (void *arg)
 {
-	mono_thread_attach (mono_get_root_domain ());
+	mono_thread_internal_attach (mono_get_root_domain ());
 
 	mono_thread_set_name_constant_ignore_error (mono_thread_internal_current (), "AOT Profiler Helper", MonoSetThreadNameFlag_None);
 
@@ -312,7 +312,7 @@ helper_thread (void *arg)
 	prof_shutdown (&aot_profiler);
 
 	mono_thread_info_set_flags (MONO_THREAD_INFO_FLAGS_NONE);
-	mono_thread_detach (mono_thread_current ());
+	mono_thread_internal_detach (mono_thread_current ());
 
 	return NULL;
 }

src/mono/mono/profiler/log.c

@@ -3150,7 +3150,7 @@ profiler_thread_begin_function (const char *name8, const gunichar2* name16, size
 	mono_thread_info_attach ();
 	MonoProfilerThread *thread = init_thread (FALSE);
 
-	mono_thread_attach (mono_get_root_domain ());
+	mono_thread_internal_attach (mono_get_root_domain ());
 
 	MonoInternalThread *internal = mono_thread_internal_current ();
 
@@ -3199,7 +3199,7 @@ profiler_thread_check_detach (MonoProfilerThread *thread)
 		thread->did_detach = TRUE;
 
 		mono_thread_info_set_flags (MONO_THREAD_INFO_FLAGS_NONE);
-		mono_thread_detach (mono_thread_current ());
+		mono_thread_internal_detach (mono_thread_current ());
 
 		mono_os_sem_post (&log_profiler.detach_threads_sem);
 	}

src/mono/mono/tests/.gitignore

@@ -19,7 +19,7 @@
 /*.pdb
 /*.o
 /*.lo
-/*.so
+**.so
 **.dylib
 **.dylib.dSYM
 /*.netmodule

src/mono/mono/tests/Makefile.am

@@ -342,6 +342,7 @@ TESTS_CS_SRC=		\
 	pinvoke11.cs		\
 	pinvoke13.cs		\
 	pinvoke17.cs		\
+	pinvoke-detach-1.cs	\
 	invoke.cs		\
 	invoke2.cs		\
 	runtime-invoke.cs		\