Merge pull request #4274 from kumpera/cctor-abort
authorRodrigo Kumpera <kumpera@users.noreply.github.com>
Sun, 22 Jan 2017 15:20:38 +0000 (10:20 -0500)
committerGitHub <noreply@github.com>
Sun, 22 Jan 2017 15:20:38 +0000 (10:20 -0500)
[runtime] Latest attempt at the cctor abort race. All tests passing locally.

mono/metadata/icall.c
mono/metadata/object-internals.h
mono/metadata/object.c
mono/metadata/threads-types.h
mono/metadata/threads.c
mono/tests/abort-cctor.cs

index 3e87be6daf94477e3de8824cdcf2ca0a7f128956..774bf8bc50fc32363f9d11632acf7b0422bf80cb 100644 (file)
@@ -6646,6 +6646,10 @@ ves_icall_System_Environment_Exit (int result)
        /* Suspend all managed threads since the runtime is going away */
        mono_thread_suspend_all_other_threads ();
 
+       //FIXME shutdown is, weirdly enough, abortible in gc.c so we add this hack for now, see https://bugzilla.xamarin.com/show_bug.cgi?id=51653
+       mono_threads_begin_abort_protected_block ();
+       mono_thread_info_clear_self_interrupt ();
+
        mono_runtime_quit ();
 #endif
 
index 67a185e07fa7565497e266f7cdcac2fe40df8220..2c4780ac2330963984e52efe5e8f26ba8143ecf2 100644 (file)
@@ -375,7 +375,7 @@ struct _MonoInternalThread {
        int _serialized_principal_version;
        gpointer appdomain_refs;
        /* This is modified using atomic ops, so keep it a gint32 */
-       gint32 interruption_requested;
+       gint32 __interruption_requested;
        MonoCoopMutex *synch_cs;
        MonoBoolean threadpool_thread;
        MonoBoolean thread_interrupt_requested;
@@ -388,18 +388,19 @@ struct _MonoInternalThread {
        gpointer interrupt_on_stop;
        gsize    flags;
        gpointer thread_pinning_ref;
-       gsize abort_protected_block_count;
+       gsize __abort_protected_block_count;
        gint32 priority;
        GPtrArray *owned_mutexes;
        MonoOSEvent *suspended;
        gint32 self_suspended; // TRUE | FALSE
+
+       gsize thread_state;
        /* 
         * These fields are used to avoid having to increment corlib versions
         * when a new field is added to this structure.
         * Please synchronize any changes with InternalThread in Thread.cs, i.e. add the
         * same field there.
         */
-       gsize unused1;
        gsize unused2;
 
        /* This is used only to check that we are in sync between the representation
index 699c7d225fb78b8e5899d38738fef4b01619464a..9da8a225049d2dd73cb916a5180ba7a3f6643024 100644 (file)
@@ -441,7 +441,7 @@ mono_runtime_class_init_full (MonoVTable *vtable, MonoError *error)
 
                mono_threads_begin_abort_protected_block ();
                mono_runtime_try_invoke (method, NULL, NULL, (MonoObject**) &exc, error);
-               mono_threads_end_abort_protected_block ();
+               gboolean got_pending_interrupt = mono_threads_end_abort_protected_block ();
 
                //exception extracted, error will be set to the right value later
                if (exc == NULL && !mono_error_ok (error))//invoking failed but exc was not set
@@ -484,10 +484,13 @@ mono_runtime_class_init_full (MonoVTable *vtable, MonoError *error)
                        mono_domain_set (last_domain, TRUE);
                lock->done = TRUE;
                mono_type_init_unlock (lock);
+
+               //This can happen if the cctor self-aborts
                if (exc && mono_object_class (exc) == mono_defaults.threadabortexception_class)
                        pending_tae = exc;
+
                //TAEs are blocked around .cctors, they must escape as soon as no cctor is left to run.
-               if (!pending_tae && mono_get_eh_callbacks ()->mono_above_abort_threshold ())
+               if (!pending_tae && got_pending_interrupt)
                        pending_tae = mono_thread_try_resume_interruption ();
        } else {
                /* this just blocks until the initializing thread is done */
index 626a26bb31b61b214a561a40402e1cb8ffeb7fa4..c473777932a32d97d7ba6899877ebacff9b6fff4 100644 (file)
@@ -248,7 +248,7 @@ MONO_API void
 mono_threads_detach_coop (gpointer cookie, gpointer *dummy);
 
 void mono_threads_begin_abort_protected_block (void);
-void mono_threads_end_abort_protected_block (void);
+gboolean mono_threads_end_abort_protected_block (void);
 MonoException* mono_thread_try_resume_interruption (void);
 
 gboolean
index abc29bf0022c3d70c33af64cac939df00c980834..bbbb9f69b88052bd30133aac0b262a525a511911 100644 (file)
@@ -229,6 +229,167 @@ get_next_managed_thread_id (void)
        return InterlockedIncrement (&managed_thread_id_counter);
 }
 
+enum {
+       INTERRUPT_REQUESTED_BIT = 0x1,
+       INTERRUPT_REQUEST_DEFERRED_BIT = 0x2,
+       ABORT_PROT_BLOCK_SHIFT = 2,
+       ABORT_PROT_BLOCK_BITS = 8,
+       ABORT_PROT_BLOCK_MASK = (((1 << ABORT_PROT_BLOCK_BITS) - 1) << ABORT_PROT_BLOCK_SHIFT)
+};
+
+static int
+mono_thread_get_abort_prot_block_count (MonoInternalThread *thread)
+{
+       gsize state = thread->thread_state;
+       return (state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT;
+}
+
+static void
+verify_thread_state (gsize state)
+{
+       //can't have both INTERRUPT_REQUESTED_BIT and INTERRUPT_REQUEST_DEFERRED_BIT set at the same time
+       g_assert ((state & (INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT)) != (INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT));
+
+       //XXX This would be nice to be true, but can happen due to self-aborts (and possibly set-pending-exception)
+       //if prot_count > 0, INTERRUPT_REQUESTED_BIT must never be set
+       // int prot_count = (state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT;
+       // g_assert (!(prot_count > 0 && (state & INTERRUPT_REQUESTED_BIT)));
+}
+
+void
+mono_threads_begin_abort_protected_block (void)
+{
+       MonoInternalThread *thread = mono_thread_internal_current ();
+       gsize old_state, new_state;
+       do {
+               old_state = thread->thread_state;
+               verify_thread_state (old_state);
+
+               int new_val = ((old_state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT) + 1;
+
+               new_state = 0;
+               if (old_state & (INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT)) {
+                       if (old_state & INTERRUPT_REQUESTED_BIT)
+                               printf ("begin prot happy as it demoted interrupt to deferred interrupt\n");
+                       new_state |= INTERRUPT_REQUEST_DEFERRED_BIT;
+               }
+
+               //bounds check abort_prot_count
+               g_assert (new_val > 0);
+               g_assert (new_val < (1 << ABORT_PROT_BLOCK_BITS));
+               new_state |= new_val << ABORT_PROT_BLOCK_SHIFT;
+
+       } while (InterlockedCompareExchangePointer ((volatile gpointer)&thread->thread_state, (gpointer)new_state, (gpointer)old_state) != (gpointer)old_state);
+}
+
+gboolean
+mono_threads_end_abort_protected_block (void)
+{
+       MonoInternalThread *thread = mono_thread_internal_current ();
+       gsize old_state, new_state;
+       do {
+               old_state = thread->thread_state;
+               verify_thread_state (old_state);
+
+               int new_val = ((old_state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT) - 1;
+               new_state = 0;
+
+               if ((old_state & INTERRUPT_REQUEST_DEFERRED_BIT) && new_val == 0) {
+                       printf ("end abort on alert, promoted deferred to pront interrupt\n");
+                       new_state |= INTERRUPT_REQUESTED_BIT;
+               }
+
+               //bounds check abort_prot_count
+               g_assert (new_val >= 0);
+               g_assert (new_val < (1 << ABORT_PROT_BLOCK_BITS));
+               new_state |= new_val << ABORT_PROT_BLOCK_SHIFT;
+
+       } while (InterlockedCompareExchangePointer ((volatile gpointer)&thread->thread_state, (gpointer)new_state, (gpointer)old_state) != (gpointer)old_state);
+       return (new_state & INTERRUPT_REQUESTED_BIT) == INTERRUPT_REQUESTED_BIT;
+}
+
+
+//Don't use this function, use inc/dec below
+static void
+mono_thread_abort_prot_block_count_add (MonoInternalThread *thread, int val)
+{
+       gsize old_state, new_state;
+       do {
+               old_state = thread->thread_state;
+               verify_thread_state (old_state);
+
+               int new_val = val + ((old_state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT);
+               //bounds check abort_prot_count
+               g_assert (new_val >= 0);
+               g_assert (new_val < (1 << ABORT_PROT_BLOCK_BITS));
+               new_state = (old_state & ~ABORT_PROT_BLOCK_MASK) | (new_val << ABORT_PROT_BLOCK_SHIFT);
+
+       } while (InterlockedCompareExchangePointer ((volatile gpointer)&thread->thread_state, (gpointer)new_state, (gpointer)old_state) != (gpointer)old_state);
+}
+
+static void
+mono_thread_inc_abort_prot_block_count (MonoInternalThread *thread)
+{
+       mono_thread_abort_prot_block_count_add (thread, 1);
+}
+
+static void
+mono_thread_dec_abort_prot_block_count (MonoInternalThread *thread)
+{
+       mono_thread_abort_prot_block_count_add (thread, -1);
+}
+
+static gboolean
+mono_thread_get_interruption_requested (MonoInternalThread *thread)
+{
+       gsize state = thread->thread_state;
+       return (state & INTERRUPT_REQUESTED_BIT) == INTERRUPT_REQUESTED_BIT;
+}
+
+/* Returns TRUE is there was a state change */
+static gboolean
+mono_thread_clear_interruption_requested (MonoInternalThread *thread)
+{
+       gsize old_state, new_state;
+       do {
+               old_state = thread->thread_state;
+               verify_thread_state (old_state);
+
+               //Already cleared
+               if (!(old_state & (INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT)))
+                       return FALSE;
+               new_state = old_state & ~(INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT);
+       } while (InterlockedCompareExchangePointer ((volatile gpointer)&thread->thread_state, (gpointer)new_state, (gpointer)old_state) != (gpointer)old_state);
+       return TRUE;
+}
+
+/* Returns TRUE is there was a state change */
+static gboolean
+mono_thread_set_interruption_requested (MonoInternalThread *thread)
+{
+       //always force when the current thread is doing it to itself.
+       gboolean force_interrupt = thread == mono_thread_internal_current ();
+       gsize old_state, new_state;
+       do {
+               old_state = thread->thread_state;
+               verify_thread_state (old_state);
+
+               int prot_count = ((old_state & ABORT_PROT_BLOCK_MASK) >> ABORT_PROT_BLOCK_SHIFT);
+               //Already set
+               if (old_state & (INTERRUPT_REQUESTED_BIT | INTERRUPT_REQUEST_DEFERRED_BIT))
+                       return FALSE;
+
+               //If there's an outstanding prot block, we queue it
+               if (prot_count && !force_interrupt) {
+                       printf ("set interrupt unhappy, as it's only putting a deferred req %d\n", force_interrupt);
+                       new_state = old_state | INTERRUPT_REQUEST_DEFERRED_BIT;
+               } else
+                       new_state = old_state | INTERRUPT_REQUESTED_BIT;
+       } while (InterlockedCompareExchangePointer ((volatile gpointer)&thread->thread_state, (gpointer)new_state, (gpointer)old_state) != (gpointer)old_state);
+
+       return (new_state & INTERRUPT_REQUESTED_BIT) == INTERRUPT_REQUESTED_BIT;
+}
+
 static inline MonoNativeThreadId
 thread_get_tid (MonoInternalThread *thread)
 {
@@ -1008,7 +1169,7 @@ mono_thread_detach_internal (MonoInternalThread *thread)
        Leaving the counter unbalanced will cause a performance degradation since all threads
        will now keep checking their local flags all the time.
        */
-       if (InterlockedExchange (&thread->interruption_requested, 0) != 0)
+       if (mono_thread_clear_interruption_requested (thread))
                InterlockedDecrement (&thread_interruption_requested);
 
        mono_threads_lock ();
@@ -4307,7 +4468,7 @@ mono_thread_execute_interruption (void)
        LOCK_THREAD (thread);
 
        /* MonoThread::interruption_requested can only be changed with atomics */
-       if (InterlockedCompareExchange (&thread->interruption_requested, FALSE, TRUE)) {
+       if (mono_thread_clear_interruption_requested (thread)) {
                /* this will consume pending APC calls */
 #ifdef HOST_WIN32
                WaitForSingleObjectEx (GetCurrentThread(), 0, TRUE);
@@ -4387,7 +4548,7 @@ mono_thread_request_interruption (gboolean running_managed)
                thread->state & ThreadState_Background)
                ExitThread (1);
 #endif
-       if (InterlockedCompareExchange (&thread->interruption_requested, 1, 0) == 1)
+       if (!mono_thread_set_interruption_requested (thread))
                return NULL;
        InterlockedIncrement (&thread_interruption_requested);
 
@@ -4431,7 +4592,7 @@ mono_thread_resume_interruption (void)
        if (!still_aborting)
                return FALSE;
 
-       if (InterlockedCompareExchange (&thread->interruption_requested, 1, 0) == 1)
+       if (!mono_thread_set_interruption_requested (thread))
                return NULL;
        InterlockedIncrement (&thread_interruption_requested);
 
@@ -4446,7 +4607,7 @@ gboolean mono_thread_interruption_requested ()
                MonoInternalThread *thread = mono_thread_internal_current ();
                /* The thread may already be stopping */
                if (thread != NULL) 
-                       return (thread->interruption_requested);
+                       return mono_thread_get_interruption_requested (thread);
        }
        return FALSE;
 }
@@ -4459,7 +4620,7 @@ mono_thread_interruption_checkpoint_request (gboolean bypass_abort_protection)
        /* The thread may already be stopping */
        if (!thread)
                return NULL;
-       if (!thread->interruption_requested)
+       if (!mono_thread_get_interruption_requested (thread))
                return NULL;
        if (!bypass_abort_protection && is_running_protected_wrapper ())
                return NULL;
@@ -4690,11 +4851,11 @@ async_abort_critical (MonoThreadInfo *info, gpointer ud)
        The target thread is running at least one protected block, which must not be interrupted, so we give up.
        The protected block code will give them a chance when appropriate.
        */
-       if (thread->abort_protected_block_count)
+       if (mono_thread_get_abort_prot_block_count (thread) > 0)
                return MonoResumeThread;
 
        /*someone is already interrupting it*/
-       if (InterlockedCompareExchange (&thread->interruption_requested, 1, 0) == 1)
+       if (!mono_thread_set_interruption_requested (thread))
                return MonoResumeThread;
 
        InterlockedIncrement (&thread_interruption_requested);
@@ -4789,7 +4950,7 @@ async_suspend_critical (MonoThreadInfo *info, gpointer ud)
                        return KeepSuspended;
                }
        } else {
-               if (InterlockedCompareExchange (&thread->interruption_requested, 1, 0) == 0)
+               if (mono_thread_set_interruption_requested (thread))
                        InterlockedIncrement (&thread_interruption_requested);
                if (data->interrupt)
                        data->interrupt_token = mono_thread_info_prepare_interrupt ((MonoThreadInfo *)thread->thread_info);
@@ -5097,34 +5258,15 @@ mono_threads_detach_coop (gpointer cookie, gpointer *dummy)
        }
 }
 
-void
-mono_threads_begin_abort_protected_block (void)
-{
-       MonoInternalThread *thread;
-
-       thread = mono_thread_internal_current ();
-       ++thread->abort_protected_block_count;
-       mono_memory_barrier ();
-}
-
-void
-mono_threads_end_abort_protected_block (void)
-{
-       MonoInternalThread *thread;
-
-       thread = mono_thread_internal_current ();
-
-       mono_memory_barrier ();
-       --thread->abort_protected_block_count;
-}
-
 MonoException*
 mono_thread_try_resume_interruption (void)
 {
        MonoInternalThread *thread;
 
        thread = mono_thread_internal_current ();
-       if (thread->abort_protected_block_count || mono_get_eh_callbacks ()->mono_current_thread_has_handle_block_guard ())
+       if (!mono_get_eh_callbacks ()->mono_above_abort_threshold ())
+               return NULL;
+       if (mono_thread_get_abort_prot_block_count (thread) > 0 || mono_get_eh_callbacks ()->mono_current_thread_has_handle_block_guard ())
                return NULL;
 
        return mono_thread_resume_interruption ();
@@ -5144,7 +5286,7 @@ mono_threads_is_ready_to_be_interrupted (void)
                return FALSE;
        }
 
-       if (thread->abort_protected_block_count || mono_get_eh_callbacks ()->mono_current_thread_has_handle_block_guard ()) {
+       if (mono_thread_get_abort_prot_block_count (thread) || mono_get_eh_callbacks ()->mono_current_thread_has_handle_block_guard ()) {
                UNLOCK_THREAD (thread);
                return FALSE;
        }
index 34f87808da9186330451d8e629ac8ebf2ebce2b9..19c494c9d5c3b571978f93a87466a4b075fe24a7 100644 (file)
@@ -44,7 +44,7 @@ class Driver
                if (!StaticConstructor1.gotToEnd) /* the TAE must not land during a .cctor */
                        Environment.Exit (1);
                if (StaticConstructor1.caughtException)
-                       Environment.Exit (1);
+                       Environment.Exit (2);
                        
        }
 
@@ -83,7 +83,7 @@ class Driver
                        Console.WriteLine ("StaticConstructor1 is viable"); /* a TAE doesn't make a type unusable */
                } catch (TypeInitializationException  e) {
                        Console.WriteLine ("StaticConstructor1 not viable");
-                       Environment.Exit (1);
+                       Environment.Exit (3);
                }
        }
 
@@ -141,7 +141,7 @@ class Driver
 
                if (Driver.mre2.WaitOne (500)) {
                        /* We shouldn't reach Driver.mre.Set () in StaticConstructor2.cctor */
-                       Environment.Exit (1);
+                       Environment.Exit (4);
                }
 
                thread.Join ();
@@ -151,7 +151,7 @@ class Driver
                        IsStaticConstructor2Viable ();
                        Console.WriteLine ("StaticConstructor2 is viable");
                        /* A regular exception escaping the .cctor makes the type not usable */
-                       Environment.Exit (1);
+                       Environment.Exit (5);
                } catch (TypeInitializationException e) {
                        Console.WriteLine ("StaticConstructor2 not viable");
                }
@@ -168,7 +168,7 @@ class Driver
                        /* Unreachable */
                        Driver.mre2.Set ();
                        Console.WriteLine ("StaticConstructor3.StaticConstructor3 (2)");
-                       Environment.Exit (1);
+                       Environment.Exit (6);
                }
 
                public static void Init ()
@@ -194,7 +194,7 @@ class Driver
                                StaticConstructor3.Init ();
                                Console.WriteLine ("cctor3 didn't throw?!?!");
                                /* StaticConstructor3 self aborted */
-                               Environment.Exit (1);
+                               Environment.Exit (7);
                        } catch (ThreadAbortException e) {
                                Console.WriteLine ("TEST 3: aborted {0}", e);
                        }
@@ -215,7 +215,7 @@ class Driver
                        IsStaticConstructor3Viable ();
                        Console.WriteLine ("StaticConstructor3 is viable");
                        /* A regular exception escaping the .cctor makes the type not usable */
-                       Environment.Exit (1);
+                       Environment.Exit (8);
                } catch (TypeInitializationException e) {
                        Console.WriteLine ("StaticConstructor3 not viable");
                }
@@ -262,9 +262,9 @@ class Driver
                new StaticConstructor4 ();
                Console.WriteLine ("IsStaticConstructor4Viable: Did it get to the end? {0} Did it catch an exception {1} and end of the finally block {2}", StaticConstructor4.gotToEnd, StaticConstructor4.caughtException, got_to_the_end_of_the_finally);
                if (!StaticConstructor4.gotToEnd) /* the TAE must not land during a .cctor */
-                       Environment.Exit (1);
+                       Environment.Exit (9);
                if (StaticConstructor4.caughtException)
-                       Environment.Exit (1);
+                       Environment.Exit (10);
        }
 
        static void Test4 ()
@@ -305,7 +305,7 @@ class Driver
 
                if (!got_to_the_end_of_the_finally) { 
                        Console.WriteLine ("Did not get to the end of test 4 cctor");
-                       Environment.Exit (1);
+                       Environment.Exit (11);
                }
 
                //is StaticConstructor4viable?
@@ -314,7 +314,7 @@ class Driver
                        Console.WriteLine ("StaticConstructor4 is viable"); /* a TAE doesn't make a type unusable */
                } catch (TypeInitializationException  e) {
                        Console.WriteLine ("StaticConstructor4 not viable");
-                       Environment.Exit (1);
+                       Environment.Exit (12);
                }
        }