Prevent a deadlock in suspend_thread and after process forking

(fix of commit 0c0e4cd)

Issue #235 (bdwgc).

* pthread_stop_world.c [GC_ENABLE_SUSPEND_THREAD] (GC_suspend_thread):
Remove unneeded comment for AO_store_release() call; invoke
GC_acquire_dirty_lock() and GC_release_dirty_lock() only if
GC_manual_vdb; add comment for GC_acquire_dirty_lock() call.
* pthread_stop_world.c [NACL] (GC_suspend_all): If GC_manual_vdb then
call GC_acquire_dirty_lock() and GC_release_dirty_lock() around the
code which ensures parking of threads.
* pthread_support.c [CAN_HANDLE_FORK] (fork_prepare_proc): Call
GC_acquire_dirty_lock().
* pthread_support.c [CAN_HANDLE_FORK] (fork_parent_proc,
fork_child_proc): Call GC_release_dirty_lock().

diff --git a/pthread_stop_world.c b/pthread_stop_world.c
index 9260597..a94d466 100644 (file)
--- a/pthread_stop_world.c
+++ b/pthread_stop_world.c
@@ -545,9 +545,6 @@ STATIC void GC_restart_handler(int sig)
       }
 
       /* Set the flag making the change visible to the signal handler.  */
-      /* This also removes the protection for t object, preventing      */
-      /* write faults in GC_store_stack_ptr (thus double-locking should */
-      /* not occur in async_set_pht_entry_from_index).                  */
       AO_store_release(&t->suspended_ext, TRUE);
 
       if (THREAD_EQUAL((pthread_t)thread, pthread_self())) {
@@ -577,8 +574,15 @@ STATIC void GC_restart_handler(int sig)
           GC_wait_for_reclaim();
 #     endif
 
+      if (GC_manual_vdb) {
+        /* See the relevant comment in GC_stop_world.   */
+        GC_acquire_dirty_lock();
+      }
+      /* Else do not acquire the lock as the write fault handler might  */
+      /* be trying to acquire this lock too, and the suspend handler    */
+      /* execution is deferred until the write fault handler completes. */
+
       /* TODO: Support GC_retry_signals (not needed for TSan) */
-      GC_acquire_dirty_lock();
       switch (RAISE_SIGNAL(t, GC_sig_suspend)) {
       /* ESRCH cannot happen as terminated threads are handled above.   */
       case 0:
@@ -594,7 +598,8 @@ STATIC void GC_restart_handler(int sig)
         if (errno != EINTR)
           ABORT("sem_wait for handler failed (suspend_self)");
       }
-      GC_release_dirty_lock();
+      if (GC_manual_vdb)
+        GC_release_dirty_lock();
       RESTORE_CANCEL(cancel_state);
       UNLOCK();
     }
@@ -821,6 +826,8 @@ STATIC int GC_suspend_all(void)
     GC_nacl_thread_parker = pthread_self();
     GC_nacl_park_threads_now = 1;
 
+    if (GC_manual_vdb)
+      GC_acquire_dirty_lock();
     while (1) {
       int num_threads_parked = 0;
       struct timespec ts;
@@ -856,6 +863,8 @@ STATIC int GC_suspend_all(void)
         num_sleeps = 0;
       }
     }
+    if (GC_manual_vdb)
+      GC_release_dirty_lock();
 # endif /* NACL */
   return n_live_threads;
 }

diff --git a/pthread_support.c b/pthread_support.c
index fb8db26..22b369d 100644 (file)
--- a/pthread_support.c
+++ b/pthread_support.c
@@ -1087,6 +1087,7 @@ static void fork_prepare_proc(void)
         if (GC_parallel)
           GC_acquire_mark_lock();
 #     endif
+      GC_acquire_dirty_lock();
 }
 
 /* Called in parent after a fork() (even if the latter failed). */
@@ -1095,6 +1096,7 @@ static void fork_prepare_proc(void)
 #endif
 static void fork_parent_proc(void)
 {
+    GC_release_dirty_lock();
 #   if defined(PARALLEL_MARK)
       if (GC_parallel)
         GC_release_mark_lock();
@@ -1109,11 +1111,12 @@ static void fork_parent_proc(void)
 #endif
 static void fork_child_proc(void)
 {
-    /* Clean up the thread table, so that just our thread is left. */
+    GC_release_dirty_lock();
 #   if defined(PARALLEL_MARK)
       if (GC_parallel)
         GC_release_mark_lock();
 #   endif
+    /* Clean up the thread table, so that just our thread is left.      */
     GC_remove_all_threads_but_me();
 #   ifdef PARALLEL_MARK
       /* Turn off parallel marking in the child, since we are probably  */