Fix concurrent bitmap update in GC_dirty

Issue #235 (bdwgc).

* darwin_stop_world.c [!GC_NO_THREADS_DISCOVERY]
(GC_suspend_thread_list): Call GC_acquire_dirty_lock and
GC_release_dirty_lock around thread_suspend().
* darwin_stop_world.c (GC_stop_world): Likewise.
* include/private/gc_priv.h (set_pht_entry_from_index_safe): Remove
unused macro.
* include/private/gc_priv.h [THREADS] (GC_acquire_dirty_lock,
GC_release_dirty_lock): Define new macro; move comment from
win32_threads.c.
* include/private/gc_priv.h [THREADS && !GC_DISABLE_INCREMENTAL]
(GC_fault_handler_lock): Declare global variable (even if not
MPROTECT_VDB).
* os_dep.c [!GC_DISABLE_INCREMENTAL] (async_set_pht_entry_from_index):
Define even if not MPROTECT_VDB; reformat comments.
* os_dep.c [!GC_DISABLE_INCREMENTAL && AO_HAVE_test_and_set_acquire]
(GC_fault_handler_lock): Likewise.
* os_dep.c [THREADS && AO_HAVE_test_and_set_acquire]
(async_set_pht_entry_from_index): Use GC_acquire_dirty_lock and
GC_release_dirty_lock; remove wrong comment.
* os_dep.c [THREADS && !AO_HAVE_test_and_set_acquire]
(currently_updating, async_set_pht_entry_from_index): Remove incorrect
implementation.
* os_dep.c [!GC_DISABLE_INCREMENTAL] (GC_dirty_inner): Call
async_set_pht_entry_from_index instead of set_pht_entry_from_index;
remove FIXME.
* pthread_stop_world.c [GC_ENABLE_SUSPEND_THREAD] (GC_suspend_thread):
Refine comment for AO_store_release call; call GC_acquire_dirty_lock
and GC_release_dirty_lock around a block of RAISE_SIGNAL() and
sem_wait().
* pthread_stop_world.c [GC_OPENBSD_UTHREADS] (GC_suspend_all): Call
GC_acquire_dirty_lock and GC_release_dirty_lock around
pthread_suspend_np().
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS] (GC_suspend_all): Add
comment.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL] (GC_stop_world):
If GC_manual_vdb then call GC_acquire_dirty_lock and
GC_release_dirty_lock around a block of GC_suspend_all() and
suspend_restart_barrier(); add comment.
* win32_threads.c (GC_suspend): Use GC_acquire_dirty_lock and
GC_release_dirty_lock (regardless of MPROTECT_VDB).

darwin_stop_world.c

@@ -517,9 +517,11 @@ STATIC GC_bool GC_suspend_thread_list(thread_act_array_t act_list, int count,
 #   endif
     /* Unconditionally suspend the thread.  It will do no     */
     /* harm if it is already suspended by the client logic.   */
+    GC_acquire_dirty_lock();
     do {
       kern_result = thread_suspend(thread);
     } while (kern_result == KERN_ABORTED);
+    GC_release_dirty_lock();
     if (kern_result != KERN_SUCCESS) {
       /* The thread may have quit since the thread_threads() call we  */
       /* mark already suspended so it's not dealt with anymore later. */
@@ -618,10 +620,11 @@ GC_INNER void GC_stop_world(void)
       for (p = GC_threads[i]; p != NULL; p = p->next) {
         if ((p->flags & FINISHED) == 0 && !p->thread_blocked &&
              p->stop_info.mach_thread != my_thread) {
-
+          GC_acquire_dirty_lock();
           do {
             kern_result = thread_suspend(p->stop_info.mach_thread);
           } while (kern_result == KERN_ABORTED);
+          GC_release_dirty_lock();
           if (kern_result != KERN_SUCCESS)
             ABORT("thread_suspend failed");
           if (GC_on_thread_event)

include/private/gc_priv.h

@@ -961,10 +961,6 @@ typedef word page_hash_table[PHT_SIZE];
                 (((bl)[divWORDSZ(index)] >> modWORDSZ(index)) & 1)
 # define set_pht_entry_from_index(bl, index) \
                 (bl)[divWORDSZ(index)] |= (word)1 << modWORDSZ(index)
-/* And a dumb but thread-safe version of set_pht_entry_from_index.      */
-/* This sets (many) extra bits.                                         */
-# define set_pht_entry_from_index_safe(bl, index) \
-                (bl)[divWORDSZ(index)] = ONES
 
 /* And, one more version for GC_add_to_black_list_normal/stack.         */
 /* The latter ones are invoked (indirectly) by GC_do_local_mark.        */
@@ -2324,7 +2320,17 @@ GC_EXTERN signed_word GC_bytes_found;
                                 /* protected by GC_write_cs.    */
 
 # endif
-# ifdef MPROTECT_VDB
+# if defined(GC_DISABLE_INCREMENTAL)
+#   define GC_acquire_dirty_lock() (void)0
+#   define GC_release_dirty_lock() (void)0
+# else
+    /* Acquire the spin lock we use to update dirty bits.       */
+    /* Threads should not get stopped holding it.  But we may   */
+    /* acquire and release it during GC_remove_protection call. */
+#   define GC_acquire_dirty_lock() \
+        do { /* empty */ \
+        } while (AO_test_and_set_acquire(&GC_fault_handler_lock) == AO_TS_SET)
+#   define GC_release_dirty_lock() AO_CLEAR(&GC_fault_handler_lock)
     GC_EXTERN volatile AO_TS_t GC_fault_handler_lock;
                                         /* defined in os_dep.c */
 # endif

os_dep.c

@@ -2958,6 +2958,29 @@ GC_API GC_push_other_roots_proc GC_CALL GC_get_push_other_roots(void)
   }
 #endif /* DEFAULT_VDB */
 
+#ifndef GC_DISABLE_INCREMENTAL
+# ifndef THREADS
+#   define async_set_pht_entry_from_index(db, index) \
+                        set_pht_entry_from_index(db, index)
+# elif defined(AO_HAVE_test_and_set_acquire)
+    /* We need to lock around the bitmap update (in the write fault     */
+    /* handler or GC_dirty) in order to avoid the risk of losing a bit. */
+    /* We do this with a test-and-set spin lock if possible.            */
+    GC_INNER volatile AO_TS_t GC_fault_handler_lock = AO_TS_INITIALIZER;
+
+    GC_ATTR_NO_SANITIZE_THREAD
+    static void async_set_pht_entry_from_index(volatile page_hash_table db,
+                                               size_t index)
+    {
+      GC_acquire_dirty_lock();
+      set_pht_entry_from_index(db, index);
+      GC_release_dirty_lock();
+    }
+# else
+#   error No test_and_set operation: Introduces a race.
+# endif /* THREADS && !AO_HAVE_test_and_set_acquire */
+#endif /* !GC_DISABLE_INCREMENTAL */
+
 #ifdef MPROTECT_VDB
   /*
    * This implementation maintains dirty bits itself by catching write
@@ -3066,60 +3089,7 @@ GC_API GC_push_other_roots_proc GC_CALL GC_get_push_other_roots(void)
 # endif /* !MSWIN32 */
 #endif /* !DARWIN */
 
-#if defined(THREADS)
-/* We need to lock around the bitmap update in the write fault handler  */
-/* in order to avoid the risk of losing a bit.  We do this with a       */
-/* test-and-set spin lock if we know how to do that.  Otherwise we      */
-/* check whether we are already in the handler and use the dumb but     */
-/* safe fallback algorithm of setting all bits in the word.             */
-/* Contention should be very rare, so we do the minimum to handle it    */
-/* correctly.                                                           */
-#ifdef AO_HAVE_test_and_set_acquire
-  GC_INNER volatile AO_TS_t GC_fault_handler_lock = AO_TS_INITIALIZER;
-
-  GC_ATTR_NO_SANITIZE_THREAD
-  static void async_set_pht_entry_from_index(volatile page_hash_table db,
-                                             size_t index)
-  {
-    while (AO_test_and_set_acquire(&GC_fault_handler_lock) == AO_TS_SET) {
-      /* empty */
-    }
-    /* Could also revert to set_pht_entry_from_index_safe if initial    */
-    /* GC_test_and_set fails.                                           */
-    set_pht_entry_from_index(db, index);
-    AO_CLEAR(&GC_fault_handler_lock);
-  }
-#else /* !AO_HAVE_test_and_set_acquire */
-# error No test_and_set operation: Introduces a race.
-  /* THIS WOULD BE INCORRECT!                                           */
-  /* The dirty bit vector may be temporarily wrong,                     */
-  /* just before we notice the conflict and correct it. We may end up   */
-  /* looking at it while it's wrong.  But this requires contention      */
-  /* exactly when a GC is triggered, which seems far less likely to     */
-  /* fail than the old code, which had no reported failures.  Thus we   */
-  /* leave it this way while we think of something better, or support   */
-  /* GC_test_and_set on the remaining platforms.                        */
-  static int * volatile currently_updating = 0;
-  static void async_set_pht_entry_from_index(volatile page_hash_table db,
-                                             size_t index)
-  {
-    int update_dummy;
-    currently_updating = &update_dummy;
-    set_pht_entry_from_index(db, index);
-    /* If we get contention in the 10 or so instruction window here,    */
-    /* and we get stopped by a GC between the two updates, we lose!     */
-    if (currently_updating != &update_dummy) {
-        set_pht_entry_from_index_safe(db, index);
-        /* We claim that if two threads concurrently try to update the  */
-        /* dirty bit vector, the first one to execute UPDATE_START      */
-        /* will see it changed when UPDATE_END is executed.  (Note that */
-        /* &update_dummy must differ in two distinct threads.)  It      */
-        /* will then execute set_pht_entry_from_index_safe, thus        */
-        /* returning us to a safe state, though not soon enough.        */
-    }
-  }
-#endif /* !AO_HAVE_test_and_set_acquire */
-
+#ifdef THREADS
   /* This function is used only by the fault handler.  Potential data   */
   /* race between this function and GC_install_header, GC_remove_header */
   /* should not be harmful because the added or removed header should   */
@@ -3135,10 +3105,7 @@ GC_API GC_push_other_roots_proc GC_CALL GC_get_push_other_roots(void)
       return HDR_INNER(addr) != NULL;
 #   endif
   }
-
-#else /* !THREADS */
-# define async_set_pht_entry_from_index(db, index) \
-                        set_pht_entry_from_index(db, index)
+#else
 # define is_header_found_async(addr) (HDR(addr) != NULL)
 #endif /* !THREADS */
 
@@ -3720,7 +3687,7 @@ GC_INNER GC_bool GC_dirty_init(void)
       /* page unprotection.                                             */
       GC_ASSERT(GC_manual_vdb);
 #   endif
-    set_pht_entry_from_index(GC_dirty_pages, index); /* FIXME: concurrent */
+    async_set_pht_entry_from_index(GC_dirty_pages, index);
   }
 
   /* Retrieve system dirty bits for the heap to a local buffer (unless  */

pthread_stop_world.c

@@ -545,6 +545,9 @@ 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())) {
@@ -575,6 +578,7 @@ STATIC void GC_restart_handler(int sig)
 #     endif
 
       /* 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:
@@ -590,6 +594,7 @@ STATIC void GC_restart_handler(int sig)
         if (errno != EINTR)
           ABORT("sem_wait for handler failed (suspend_self)");
       }
+      GC_release_dirty_lock();
       RESTORE_CANCEL(cancel_state);
       UNLOCK();
     }
@@ -763,8 +768,11 @@ STATIC int GC_suspend_all(void)
 #           ifdef GC_OPENBSD_UTHREADS
               {
                 stack_t stack;
+
+                GC_acquire_dirty_lock();
                 if (pthread_suspend_np(p -> id) != 0)
                   ABORT("pthread_suspend_np failed");
+                GC_release_dirty_lock();
                 if (pthread_stackseg_np(p->id, &stack))
                   ABORT("pthread_stackseg_np failed");
                 p -> stop_info.stack_ptr = (ptr_t)stack.ss_sp - stack.ss_size;
@@ -773,6 +781,11 @@ STATIC int GC_suspend_all(void)
                                      (void *)p->id);
               }
 #           else
+              /* The synchronization between GC_dirty (based on         */
+              /* test-and-set) and the signal-based thread suspension   */
+              /* is performed in GC_stop_world because                  */
+              /* GC_release_dirty_lock cannot be called before          */
+              /* acknowledging the thread is really suspended.          */
               result = RAISE_SIGNAL(p, GC_sig_suspend);
               switch(result) {
                 case ESRCH:
@@ -876,11 +889,19 @@ GC_INNER void GC_stop_world(void)
 # else
     AO_store(&GC_stop_count, (AO_t)((word)GC_stop_count + 2));
         /* Only concurrent reads are possible. */
+    if (GC_manual_vdb) {
+      GC_acquire_dirty_lock();
+      /* The write fault handler cannot be called if GC_manual_vdb      */
+      /* (thus double-locking should not occur in                       */
+      /* async_set_pht_entry_from_index based on test-and-set).         */
+    }
     AO_store_release(&GC_world_is_stopped, TRUE);
     n_live_threads = GC_suspend_all();
     if (GC_retry_signals)
       n_live_threads = resend_lost_signals(n_live_threads, GC_suspend_all);
     suspend_restart_barrier(n_live_threads);
+    if (GC_manual_vdb)
+      GC_release_dirty_lock(); /* cannot be done in GC_suspend_all */
 # endif
 
 # ifdef PARALLEL_MARK

win32_threads.c

@@ -1186,15 +1186,7 @@ STATIC void GC_suspend(GC_thread t)
       return;
     }
 # endif
-# if defined(MPROTECT_VDB)
-    /* Acquire the spin lock we use to update dirty bits.       */
-    /* Threads shouldn't get stopped holding it.  But we may    */
-    /* acquire and release it in the UNPROTECT_THREAD call.     */
-    while (AO_test_and_set_acquire(&GC_fault_handler_lock) == AO_TS_SET) {
-      /* empty */
-    }
-# endif
-
+  GC_acquire_dirty_lock();
 # ifdef MSWINCE
     /* SuspendThread() will fail if thread is running kernel code.      */
     while (SuspendThread(THREAD_HANDLE(t)) == (DWORD)-1)
@@ -1204,9 +1196,7 @@ STATIC void GC_suspend(GC_thread t)
       ABORT("SuspendThread failed");
 # endif /* !MSWINCE */
   t -> suspended = (unsigned char)TRUE;
-# if defined(MPROTECT_VDB)
-    AO_CLEAR(&GC_fault_handler_lock);
-# endif
+  GC_release_dirty_lock();
   if (GC_on_thread_event)
     GC_on_thread_event(GC_EVENT_THREAD_SUSPENDED, THREAD_HANDLE(t));
 }