[coop] Fix state transition when unregistering the thread

unregister_thread is the destructor for the current MonoThreadInfo* TLS key, meaning that when in this function, calling mono_thread_info_current_unchecked will always return NULL.

diff --git a/mono/utils/mono-threads-coop.c b/mono/utils/mono-threads-coop.c
index 36f8b441d1e03e7641a988a2161e3e7d44bd3c77..ec6f6ab4658b646e4132722456432f073f3a8c00 100644 (file)
--- a/mono/utils/mono-threads-coop.c
+++ b/mono/utils/mono-threads-coop.c
@@ -173,15 +173,24 @@ copy_stack_data (MonoThreadInfo *info, gpointer *stackdata_begin)
        state->gc_stackdata_size = stackdata_size;
 }
 
+static gpointer
+mono_threads_enter_gc_safe_region_unbalanced_with_info (MonoThreadInfo *info, gpointer *stackdata);
+
 gpointer
 mono_threads_enter_gc_safe_region (gpointer *stackdata)
+{
+       return mono_threads_enter_gc_safe_region_with_info (mono_thread_info_current_unchecked (), stackdata);
+}
+
+gpointer
+mono_threads_enter_gc_safe_region_with_info (MonoThreadInfo *info, gpointer *stackdata)
 {
        gpointer cookie;
 
        if (!mono_threads_is_coop_enabled ())
                return NULL;
 
-       cookie = mono_threads_enter_gc_safe_region_unbalanced (stackdata);
+       cookie = mono_threads_enter_gc_safe_region_unbalanced_with_info (info, stackdata);
 
 #ifdef ENABLE_CHECKED_BUILD_GC
        if (mono_check_mode_enabled (MONO_CHECK_MODE_GC))
@@ -194,8 +203,12 @@ mono_threads_enter_gc_safe_region (gpointer *stackdata)
 gpointer
 mono_threads_enter_gc_safe_region_unbalanced (gpointer *stackdata)
 {
-       MonoThreadInfo *info;
+       return mono_threads_enter_gc_safe_region_unbalanced_with_info (mono_thread_info_current_unchecked (), stackdata);
+}
 
+static gpointer
+mono_threads_enter_gc_safe_region_unbalanced_with_info (MonoThreadInfo *info, gpointer *stackdata)
+{
        if (!mono_threads_is_coop_enabled ())
                return NULL;
 
@@ -267,13 +280,19 @@ mono_threads_assert_gc_safe_region (void)
 
 gpointer
 mono_threads_enter_gc_unsafe_region (gpointer *stackdata)
+{
+       return mono_threads_enter_gc_unsafe_region_with_info (mono_thread_info_current_unchecked (), stackdata);
+}
+
+gpointer
+mono_threads_enter_gc_unsafe_region_with_info (THREAD_INFO_TYPE *info, gpointer *stackdata)
 {
        gpointer cookie;
 
        if (!mono_threads_is_coop_enabled ())
                return NULL;
 
-       cookie = mono_threads_enter_gc_unsafe_region_unbalanced (stackdata);
+       cookie = mono_threads_enter_gc_unsafe_region_unbalanced_with_info (info, stackdata);
 
 #ifdef ENABLE_CHECKED_BUILD_GC
        if (mono_check_mode_enabled (MONO_CHECK_MODE_GC))
@@ -286,8 +305,12 @@ mono_threads_enter_gc_unsafe_region (gpointer *stackdata)
 gpointer
 mono_threads_enter_gc_unsafe_region_unbalanced (gpointer *stackdata)
 {
-       MonoThreadInfo *info;
+       return mono_threads_enter_gc_unsafe_region_unbalanced_with_info (mono_thread_info_current_unchecked (), stackdata);
+}
 
+gpointer
+mono_threads_enter_gc_unsafe_region_unbalanced_with_info (MonoThreadInfo *info, gpointer *stackdata)
+{
        if (!mono_threads_is_coop_enabled ())
                return NULL;
 

diff --git a/mono/utils/mono-threads-coop.h b/mono/utils/mono-threads-coop.h
index d9d64568e7d384d9b7ff70d50149d26191320e16..f6afc34dafc5a58666ad8f444326fb9b6abd0e5c 100644 (file)
--- a/mono/utils/mono-threads-coop.h
+++ b/mono/utils/mono-threads-coop.h
@@ -39,6 +39,36 @@ mono_threads_safepoint (void)
                mono_threads_state_poll ();
 }
 
+/*
+ * The following are used when detaching a thread. We need to pass the MonoThreadInfo*
+ * as a paramater as the thread info TLS key is being destructed, meaning that
+ * mono_thread_info_current_unchecked will return NULL, which would lead to a
+ * runtime assertion error when trying to switch the state of the current thread.
+ */
+
+gpointer
+mono_threads_enter_gc_safe_region_with_info (THREAD_INFO_TYPE *info, gpointer *stackdata);
+
+#define MONO_ENTER_GC_SAFE_WITH_INFO(info)     \
+       do {    \
+               gpointer __gc_safe_dummy;       \
+               gpointer __gc_safe_cookie = mono_threads_enter_gc_safe_region_with_info ((info), &__gc_safe_dummy)
+
+#define MONO_EXIT_GC_SAFE_WITH_INFO    MONO_EXIT_GC_SAFE
+
+gpointer
+mono_threads_enter_gc_unsafe_region_with_info (THREAD_INFO_TYPE *info, gpointer *stackdata);
+
+#define MONO_ENTER_GC_UNSAFE_WITH_INFO(info)   \
+       do {    \
+               gpointer __gc_unsafe_dummy;     \
+               gpointer __gc_unsafe_cookie = mono_threads_enter_gc_unsafe_region_with_info ((info), &__gc_unsafe_dummy)
+
+#define MONO_EXIT_GC_UNSAFE_WITH_INFO  MONO_EXIT_GC_UNSAFE
+
+gpointer
+mono_threads_enter_gc_unsafe_region_unbalanced_with_info (THREAD_INFO_TYPE *info, gpointer *stackdata);
+
 G_END_DECLS
 
 #endif

diff --git a/mono/utils/mono-threads.c b/mono/utils/mono-threads.c
index ecb56cd81c9d63e3bc78c281adadebcd1c67f0a7..a92d250b290448698ece831e6809b4c99a8f3cdd 100644 (file)
--- a/mono/utils/mono-threads.c
+++ b/mono/utils/mono-threads.c
@@ -47,8 +47,15 @@ harder for an operation that is hardly performance critical.
 The GC has to acquire this lock before starting a STW to make sure
 a runtime suspend won't make it wronly see a thread in a safepoint
 when it is in fact not.
+
+This has to be a naked locking primitive, and not a coop aware one, as
+it needs to be usable when destroying thread_info_key, the TLS key for
+the current MonoThreadInfo. In this case, mono_thread_info_current_unchecked,
+(which is used inside MONO_ENTER_GC_SAFE), would return NULL, leading
+to an assertion error. We then simply switch state manually in
+mono_thread_info_suspend_lock_with_info.
 */
-static MonoCoopSem global_suspend_semaphore;
+static MonoSemType global_suspend_semaphore;
 
 static size_t thread_info_size;
 static MonoThreadInfoCallbacks threads_callbacks;
@@ -379,13 +386,25 @@ register_thread (MonoThreadInfo *info, gpointer baseptr)
        return info;
 }
 
+static void
+mono_thread_info_suspend_lock_with_info (MonoThreadInfo *info);
+
 static void
 unregister_thread (void *arg)
 {
-       MonoThreadInfo *info = (MonoThreadInfo *) arg;
-       int small_id = info->small_id;
+       gpointer gc_unsafe_stackdata;
+       MonoThreadInfo *info;
+       int small_id;
+
+       info = (MonoThreadInfo *) arg;
        g_assert (info);
 
+       small_id = info->small_id;
+
+       /* We only enter the GC unsafe region, as when exiting this function, the thread
+        * will be detached, and the current MonoThreadInfo* will be destroyed. */
+       mono_threads_enter_gc_unsafe_region_unbalanced_with_info (info, &gc_unsafe_stackdata);
+
        THREADS_DEBUG ("unregistering info %p\n", info);
 
        mono_native_tls_set_value (thread_exited_key, GUINT_TO_POINTER (1));
@@ -408,7 +427,7 @@ unregister_thread (void *arg)
        if (threads_callbacks.thread_detach)
                threads_callbacks.thread_detach (info);
 
-       mono_thread_info_suspend_lock ();
+       mono_thread_info_suspend_lock_with_info (info);
 
        /*
        Now perform the callback that must be done under locks.
@@ -637,7 +656,7 @@ mono_threads_init (MonoThreadInfoCallbacks *callbacks, size_t info_size)
 
        unified_suspend_enabled = g_getenv ("MONO_ENABLE_UNIFIED_SUSPEND") != NULL || mono_threads_is_coop_enabled ();
 
-       mono_coop_sem_init (&global_suspend_semaphore, 1);
+       mono_os_sem_init (&global_suspend_semaphore, 1);
        mono_os_sem_init (&suspend_semaphore, 0);
 
        mono_lls_init (&thread_list, NULL, HAZARD_FREE_NO_LOCK);
@@ -986,17 +1005,30 @@ The suspend lock is held during any suspend in progress.
 A GC that has safepoints must take this lock as part of its
 STW to make sure no unsafe pending suspend is in progress.   
 */
+
+static void
+mono_thread_info_suspend_lock_with_info (MonoThreadInfo *info)
+{
+       g_assert (info);
+
+       MONO_ENTER_GC_SAFE_WITH_INFO(info);
+
+       int res = mono_os_sem_wait (&global_suspend_semaphore, MONO_SEM_FLAGS_NONE);
+       g_assert (res != -1);
+
+       MONO_EXIT_GC_SAFE_WITH_INFO;
+}
+
 void
 mono_thread_info_suspend_lock (void)
 {
-       int res = mono_coop_sem_wait (&global_suspend_semaphore, MONO_SEM_FLAGS_NONE);
-       g_assert (res != -1);
+       mono_thread_info_suspend_lock_with_info (mono_thread_info_current_unchecked ());
 }
 
 void
 mono_thread_info_suspend_unlock (void)
 {
-       mono_coop_sem_post (&global_suspend_semaphore);
+       mono_os_sem_post (&global_suspend_semaphore);
 }
 
 /*