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

* [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 https://github.com/mono/mono/issues/20290 and https://github.com/mono/mono/issues/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.

4 files changed, 261 insertions, 13 deletions

diff --git a/mono/tests/.gitignore b/mono/tests/.gitignore
index 835205a71d1..d4b5120d85c 100644
--- a/mono/tests/.gitignore
+++ b/mono/tests/.gitignore
@@ -19,7 +19,7 @@
 /*.pdb
 /*.o
 /*.lo
-/*.so
+**.so
 **.dylib
 **.dylib.dSYM
 /*.netmodule
diff --git a/mono/tests/Makefile.am b/mono/tests/Makefile.am
index 8b689e9eb55..fc33d5c30c0 100755
--- a/mono/tests/Makefile.am
+++ b/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		\
diff --git a/mono/tests/libtest.c b/mono/tests/libtest.c
index 1e624ea6b09..c2e3639f141 100644
--- a/mono/tests/libtest.c
+++ b/mono/tests/libtest.c
@@ -7789,6 +7789,13 @@ mono_test_native_to_managed_exception_rethrow (NativeToManagedExceptionRethrowFu
 typedef void (*VoidVoidCallback) (void);
 typedef void (*MonoFtnPtrEHCallback) (guint32 gchandle);
 
+typedef void *MonoDomain;
+typedef void *MonoAssembly;
+typedef void *MonoImage;
+typedef void *MonoClass;
+typedef void *MonoMethod;
+typedef void *MonoThread;
+
 typedef long long MonoObject;
 typedef MonoObject MonoException;
 typedef int32_t mono_bool;
@@ -7803,25 +7810,60 @@ static void (*sym_mono_domain_unload) (gpointer);
 static void (*sym_mono_threads_exit_gc_safe_region_unbalanced) (gpointer, gpointer *);
 static void (*null_function_ptr) (void);
 
-static void
-mono_test_init_symbols (void)
-{
-	if (sym_inited)
-		return;
+static MonoDomain *(*sym_mono_get_root_domain) (void);
+
+static MonoDomain *(*sym_mono_domain_get)(void);
+
+static mono_bool (*sym_mono_domain_set)(MonoDomain *, mono_bool /*force */);
+
+static MonoAssembly *(*sym_mono_domain_assembly_open) (MonoDomain *, const char*);
+
+static MonoImage *(*sym_mono_assembly_get_image) (MonoAssembly *);
 
-	sym_mono_install_ftnptr_eh_callback = (void (*) (MonoFtnPtrEHCallback)) (lookup_mono_symbol ("mono_install_ftnptr_eh_callback"));
+static MonoClass *(*sym_mono_class_from_name)(MonoImage *, const char *, const char *);
 
-	sym_mono_gchandle_get_target = (MonoObject* (*) (guint32 gchandle)) (lookup_mono_symbol ("mono_gchandle_get_target"));
+static MonoMethod *(*sym_mono_class_get_method_from_name)(MonoClass *, const char *, int /* arg_count */);
 
-	sym_mono_gchandle_new = (guint32 (*) (MonoObject *, mono_bool)) (lookup_mono_symbol ("mono_gchandle_new"));
+static MonoThread *(*sym_mono_thread_attach)(MonoDomain *);
 
-	sym_mono_gchandle_free = (void (*) (guint32 gchandle)) (lookup_mono_symbol ("mono_gchandle_free"));
+static void (*sym_mono_thread_detach)(MonoThread *);
 
-	sym_mono_raise_exception = (void (*) (MonoException *)) (lookup_mono_symbol ("mono_raise_exception"));
+static MonoObject *(*sym_mono_runtime_invoke) (MonoMethod *, void*, void**, MonoObject**);
 
-	sym_mono_domain_unload = (void (*) (gpointer)) (lookup_mono_symbol ("mono_domain_unload"));
 
-	sym_mono_threads_exit_gc_safe_region_unbalanced = (void (*) (gpointer, gpointer *)) (lookup_mono_symbol ("mono_threads_exit_gc_safe_region_unbalanced"));
+// SYM_LOOKUP(mono_runtime_invoke)
+// expands to
+//  sym_mono_runtime_invoke = g_cast (lookup_mono_symbol ("mono_runtime_invoke"));
+//
+// (the g_cast is necessary for C++ builds)
+#define SYM_LOOKUP(name) do {			\
+	sym_##name = g_cast (lookup_mono_symbol (#name));	\
+	} while (0)
+
+static void
+mono_test_init_symbols (void)
+{
+	if (sym_inited)
+		return;
+
+	SYM_LOOKUP (mono_install_ftnptr_eh_callback);
+	SYM_LOOKUP (mono_gchandle_get_target);
+	SYM_LOOKUP (mono_gchandle_new);
+	SYM_LOOKUP (mono_gchandle_free);
+	SYM_LOOKUP (mono_raise_exception);
+	SYM_LOOKUP (mono_domain_unload);
+	SYM_LOOKUP (mono_threads_exit_gc_safe_region_unbalanced);
+
+	SYM_LOOKUP (mono_get_root_domain);
+	SYM_LOOKUP (mono_domain_get);
+	SYM_LOOKUP (mono_domain_set);
+	SYM_LOOKUP (mono_domain_assembly_open);
+	SYM_LOOKUP (mono_assembly_get_image);
+	SYM_LOOKUP (mono_class_from_name);
+	SYM_LOOKUP (mono_class_get_method_from_name);
+	SYM_LOOKUP (mono_thread_attach);
+	SYM_LOOKUP (mono_thread_detach);
+	SYM_LOOKUP (mono_runtime_invoke);
 
 	sym_inited = 1;
 }
@@ -8259,6 +8301,155 @@ mono_test_marshal_return_array (void)
 	return static_arr;
 }
 
+struct invoke_names {
+	char *assm_name;
+	char *name_space;
+	char *name;
+	char *meth_name;
+};
+
+static struct invoke_names *
+make_invoke_names (const char *assm_name, const char *name_space, const char *name, const char *meth_name)
+{
+	struct invoke_names *names = (struct invoke_names*) malloc (sizeof (struct invoke_names));
+	names->assm_name = strdup (assm_name);
+	names->name_space = strdup (name_space);
+	names->name = strdup (name);
+	names->meth_name = strdup (meth_name);
+	return names;
+}
+
+static void
+destroy_invoke_names (struct invoke_names *n)
+{
+	free (n->assm_name);
+	free (n->name_space);
+	free (n->name);
+	free (n->meth_name);
+	free (n);
+}
+
+static void
+test_invoke_by_name (struct invoke_names *names)
+{
+	mono_test_init_symbols ();
+
+	MonoDomain *domain = sym_mono_domain_get ();
+	MonoThread *thread = NULL;
+	if (!domain) {
+		thread = sym_mono_thread_attach (sym_mono_get_root_domain ());
+	}
+	domain = sym_mono_domain_get ();
+	g_assert (domain);
+	MonoAssembly *assm = sym_mono_domain_assembly_open (domain, names->assm_name);
+	g_assert (assm);
+	MonoImage *image = sym_mono_assembly_get_image (assm);
+	MonoClass *klass = sym_mono_class_from_name (image, names->name_space, names->name);
+	g_assert (klass);
+	/* meth_name should be a static method that takes no arguments */
+	MonoMethod *method = sym_mono_class_get_method_from_name (klass, names->meth_name, -1);
+	g_assert (method);
+
+	MonoObject *args[] = {NULL, };
+
+	sym_mono_runtime_invoke (method, NULL, (void**)args, NULL);
+
+	if (thread)
+		sym_mono_thread_detach (thread);
+}
+
+#ifndef HOST_WIN32
+static void*
+invoke_foreign_thread (void* user_data)
+{
+	struct invoke_names *names = (struct invoke_names*)user_data;
+        /*
+         * Run a couple of times to check that attach/detach multiple
+         * times from the same thread leaves it in a reasonable coop
+         * thread state.
+         */
+        for (int i = 0; i < 5; ++i) {
+                test_invoke_by_name (names);
+                sleep (2);
+        }
+	destroy_invoke_names (names);
+	return NULL;
+}
+#endif
+
+
+LIBTEST_API mono_bool STDCALL
+mono_test_attach_invoke_foreign_thread (const char *assm_name, const char *name_space, const char *name, const char *meth_name)
+{
+#ifndef HOST_WIN32
+	struct invoke_names *names = make_invoke_names (assm_name, name_space, name, meth_name);
+	pthread_t t;
+	int res = pthread_create (&t, NULL, invoke_foreign_thread, (void*)names);
+	g_assert (res == 0);
+	pthread_join (t, NULL);
+	return 0;
+#else
+	// TODO: Win32 version of this test
+	return 1;
+#endif
+}
+
+#ifndef HOST_WIN32
+struct names_and_mutex {
+	struct invoke_names *names;
+        /* mutex to coordinate test and foreign thread */
+        pthread_mutex_t coord_mutex;
+        pthread_cond_t coord_cond;
+        /* mutex to block the foreign thread */
+	pthread_mutex_t deadlock_mutex;
+};
+
+static void*
+invoke_block_foreign_thread (void *user_data)
+{
+	// This thread calls into the runtime and then blocks. It should not
+	// prevent the runtime from shutting down.
+	struct names_and_mutex *nm = (struct names_and_mutex *)user_data;
+	test_invoke_by_name (nm->names);
+        pthread_mutex_lock (&nm->coord_mutex);
+        /* signal the test thread that we called the runtime */
+        pthread_cond_signal (&nm->coord_cond);
+        pthread_mutex_unlock (&nm->coord_mutex);
+
+	pthread_mutex_lock (&nm->deadlock_mutex); // blocks forever
+	g_assert_not_reached ();
+}
+#endif
+
+LIBTEST_API mono_bool STDCALL
+mono_test_attach_invoke_block_foreign_thread (const char *assm_name, const char *name_space, const char *name, const char *meth_name)
+{
+#ifndef HOST_WIN32
+	struct invoke_names *names = make_invoke_names (assm_name, name_space, name, meth_name);
+	struct names_and_mutex *nm = malloc (sizeof (struct names_and_mutex));
+	nm->names = names;
+        pthread_mutex_init (&nm->coord_mutex, NULL);
+        pthread_cond_init (&nm->coord_cond, NULL);
+	pthread_mutex_init (&nm->deadlock_mutex, NULL);
+
+	pthread_mutex_lock (&nm->deadlock_mutex); // lock the mutex and never unlock it.
+	pthread_t t;
+	int res = pthread_create (&t, NULL, invoke_block_foreign_thread, (void*)nm);
+	g_assert (res == 0);
+        /* wait for the foreign thread to finish calling the runtime before
+         * detaching it and returning
+         */
+        pthread_mutex_lock (&nm->coord_mutex);
+        pthread_cond_wait (&nm->coord_cond, &nm->coord_mutex);
+        pthread_mutex_unlock (&nm->coord_mutex);
+	pthread_detach (t);
+	return 0;
+#else
+	// TODO: Win32 version of this test
+	return 1;
+#endif
+}
+
 #ifdef __cplusplus
 } // extern C
 #endif
diff --git a/mono/tests/pinvoke-detach-1.cs b/mono/tests/pinvoke-detach-1.cs
new file mode 100644
index 00000000000..5b475fd5360
--- /dev/null
+++ b/mono/tests/pinvoke-detach-1.cs
@@ -0,0 +1,56 @@
+//
+// pinvoke-detach-1.cs:
+//
+//   Test attaching and detaching a new thread from native.
+//  If everything is working, this should not hang on shutdown.
+using System;
+using System.Threading;
+using System.Runtime.InteropServices;
+
+public class MonoPInvokeCallbackAttribute : Attribute {
+	public MonoPInvokeCallbackAttribute (Type delegateType) { }
+}
+
+public class Tests {
+	public static int Main ()
+	{
+		return TestDriver.RunTests (typeof (Tests));
+	}
+
+	public delegate void VoidVoidDelegate ();
+
+	static int was_called;
+
+	[MonoPInvokeCallback (typeof (VoidVoidDelegate))]
+	private static void MethodInvokedFromNative ()
+	{
+		was_called++;
+	}
+
+	[DllImport ("libtest", EntryPoint="mono_test_attach_invoke_foreign_thread")]
+	public static extern bool mono_test_attach_invoke_foreign_thread (string assm_name, string name_space, string class_name, string method_name);
+
+	public static int test_0_attach_invoke_foreign_thread ()
+	{
+		was_called = 0;
+		bool skipped = mono_test_attach_invoke_foreign_thread (typeof (Tests).Assembly.Location, "", "Tests", "MethodInvokedFromNative");
+		return skipped || was_called == 5 ? 0 : 1;
+	}
+
+	[MonoPInvokeCallback (typeof (VoidVoidDelegate))]
+	private static void MethodInvokedFromNative2 ()
+	{
+	}
+
+	[DllImport ("libtest", EntryPoint="mono_test_attach_invoke_block_foreign_thread")]
+	public static extern bool mono_test_attach_invoke_block_foreign_thread (string assm_name, string name_space, string class_name, string method_name);
+
+	public static int test_0_attach_invoke_block_foreign_thread ()
+	{
+		bool skipped = mono_test_attach_invoke_block_foreign_thread (typeof (Tests).Assembly.Location, "", "Tests", "MethodInvokedFromNative2");
+		GC.Collect (); // should not hang waiting for the foreign thread
+		return 0; // really we succeed if the app can shut down without hanging
+	}
+
+
+}