Use reader mode of lock in API functions with thread-local stores only

Issue #473 (bdwgc).

These are GC_register_altstack, GC_do_blocking, GC_call_with_gc_active,
GC_set_stackbottom, GC_calloc_do_explicitly_typed functions.

* include/gc/gc.h (GC_call_with_reader_lock, GC_set_stackbottom):
Update comment.
* include/gc/gc.h (GC_register_altstack, GC_do_blocking,
GC_call_with_gc_active): Refine comment about the allocator lock.
* include/private/gc_locks.h (READER_UNLOCK_RELEASE): Define macro (to
READER_UNLOCK).
* misc.c [HAS_REAL_READER_LOCK] (GC_call_with_reader_lock): Call
READER_UNLOCK_RELEASE() if release argument is non-zero.
* pthread_support.c (GC_register_altstack, GC_do_blocking_inner,
GC_call_with_gc_active): Replace LOCK/UNLOCK() with
READER_LOCK/READER_UNLOCK_RELEASE().
* typd_mlc.c (GC_calloc_do_explicitly_typed): Likewise.
* pthread_support.c [!E2K] (do_blocking_enter): Replace I_HOLD_LOCK()
to I_HOLD_READER_LOCK().
* pthread_support.c (do_blocking_leave, GC_set_stackbottom): Likewise.
* pthread_support.c [GC_ENABLE_SUSPEND_THREAD
&& SIGNAL_BASED_STOP_WORLD] (GC_suspend_self_blocked): Add comment.
* tests/gctest.c (run_one_test): Call GC_call_with_reader_lock (with
release argument set to 1) for set_stackbottom instead of
GC_call_with_alloc_lock.

include/gc/gc.h

@@ -1564,10 +1564,12 @@ GC_API void * GC_CALL GC_call_with_alloc_lock(GC_fn_type /* fn */,
                                 void * /* client_data */) GC_ATTR_NONNULL(1);
 
 /* Execute given function with the allocator lock held in the reader    */
-/* (shared) mode.  The 3rd argument should be zero.                     */
+/* (shared) mode.  The 3rd argument (release), if non-zero, indicates   */
+/* that fn wrote some data that should be made visible to the thread    */
+/* which acquires the allocator lock in the exclusive mode later.       */
 GC_API void * GC_CALL GC_call_with_reader_lock(GC_fn_type /* fn */,
                                 void * /* client_data */,
-                                int /* unused */) GC_ATTR_NONNULL(1);
+                                int /* release */) GC_ATTR_NONNULL(1);
 
 /* These routines are intended to explicitly notify the collector       */
 /* of new threads.  Often this is unnecessary because thread creation   */
@@ -1687,7 +1689,8 @@ GC_API void GC_CALL GC_start_mark_threads(void);
   /* Notify the collector about the stack and the alt-stack of the      */
   /* current thread.  normstack and normstack_size are used to          */
   /* determine the "normal" stack boundaries when a thread is suspended */
-  /* while it is on an alt-stack.                                       */
+  /* while it is on an alt-stack.  Acquires the allocator lock in the   */
+  /* reader mode.                                                       */
   GC_API void GC_CALL GC_register_altstack(void * /* normstack */,
                                            GC_word /* normstack_size */,
                                            void * /* altstack */,
@@ -1741,6 +1744,8 @@ GC_API void GC_CALL GC_start_mark_threads(void);
 /* technique might be used to make stack scanning more precise (i.e.    */
 /* scan only stack frames of functions that allocate garbage collected  */
 /* memory and/or manipulate pointers to the garbage collected heap).    */
+/* Acquires the allocator lock in the reader mode (but fn is called     */
+/* not holding it).                                                     */
 GC_API void * GC_CALL GC_do_blocking(GC_fn_type /* fn */,
                                 void * /* client_data */) GC_ATTR_NONNULL(1);
 
@@ -1752,7 +1757,8 @@ GC_API void * GC_CALL GC_do_blocking(GC_fn_type /* fn */,
 /* initialized and the current thread is registered.  fn may toggle     */
 /* the collector thread's state temporarily to "inactive" one by using  */
 /* GC_do_blocking.  GC_call_with_gc_active() often can be used to       */
-/* provide a sufficiently accurate stack bottom.                        */
+/* provide a sufficiently accurate stack bottom.  Acquires the          */
+/* allocator lock in the reader mode (but fn is called not holding it). */
 GC_API void * GC_CALL GC_call_with_gc_active(GC_fn_type /* fn */,
                                 void * /* client_data */) GC_ATTR_NONNULL(1);
 
@@ -1783,10 +1789,14 @@ GC_API void * GC_CALL GC_get_my_stackbottom(struct GC_stack_base *)
 /* thread.  The GC thread handle is either the one returned by          */
 /* GC_get_my_stackbottom or NULL (the latter designates the current     */
 /* thread).  The caller should hold the allocator lock (e.g. using      */
-/* GC_call_with_alloc_lock).  Also, the function could be used for      */
+/* GC_call_with_reader_lock with release argument set to 1), the reader */
+/* mode should be enough typically, at least for the collector itself   */
+/* (the client is responsible to avoid data race between this and       */
+/* GC_get_my_stackbottom functions if the client acquires the allocator */
+/* lock in the reader mode).  Also, the function could be used for      */
 /* setting GC_stackbottom value (the bottom of the primordial thread)   */
 /* before the collector is initialized (the allocator lock is not       */
-/* needed to be acquired in this case).                                 */
+/* needed to be acquired at all in this case).                          */
 GC_API void GC_CALL GC_set_stackbottom(void * /* gc_thread_handle */,
                                        const struct GC_stack_base *)
                                                         GC_ATTR_NONNULL(2);

include/private/gc_locks.h

@@ -351,6 +351,12 @@
 # endif
 #endif /* !READER_LOCK */
 
+/* A variant of READER_UNLOCK() which ensures that data written before  */
+/* the unlock will be visible to the thread which acquires the          */
+/* allocator lock in the exclusive mode.  But according to some rwlock  */
+/* documentation: writers synchronize with prior writers and readers.   */
+#define READER_UNLOCK_RELEASE() READER_UNLOCK()
+
 # ifndef ENTER_GC
 #   define ENTER_GC()
 #   define EXIT_GC()

misc.c

@@ -2177,9 +2177,19 @@ GC_API void *GC_CALL GC_call_with_reader_lock(GC_fn_type fn,
 {
     void *result;
 
-    UNUSED_ARG(release);
     READER_LOCK();
     result = fn(client_data);
+#   ifdef HAS_REAL_READER_LOCK
+      if (release) {
+        READER_UNLOCK_RELEASE();
+#       ifdef LINT2
+          GC_noop1((unsigned)release);
+#       endif
+        return result;
+      }
+#   else
+      UNUSED_ARG(release);
+#   endif
     READER_UNLOCK();
     return result;
 }

pthread_support.c

@@ -920,7 +920,7 @@ GC_API void GC_CALL GC_register_altstack(void *normstack,
   GC_thread me;
   GC_stack_context_t crtn;
 
-  LOCK();
+  READER_LOCK();
   me = GC_self_thread_inner();
   if (EXPECT(NULL == me, FALSE)) {
     /* We are called before GC_thr_init. */
@@ -931,7 +931,7 @@ GC_API void GC_CALL GC_register_altstack(void *normstack,
   crtn -> normstack_size = normstack_size;
   crtn -> altstack = (ptr_t)altstack;
   crtn -> altstack_size = altstack_size;
-  UNLOCK();
+  READER_UNLOCK_RELEASE();
 #endif
 }
 
@@ -1824,7 +1824,7 @@ GC_INNER void GC_init_parallel(void)
 #   endif
     GC_stack_context_t crtn = me -> crtn;
 
-    GC_ASSERT(I_HOLD_LOCK());
+    GC_ASSERT(I_HOLD_READER_LOCK());
     GC_ASSERT((me -> flags & DO_BLOCKING) == 0);
     *pTopOfStackUnset = FALSE;
 #   ifdef SPARC
@@ -1850,7 +1850,7 @@ GC_INNER void GC_init_parallel(void)
 
 static void do_blocking_leave(GC_thread me, GC_bool topOfStackUnset)
 {
-    GC_ASSERT(I_HOLD_LOCK());
+    GC_ASSERT(I_HOLD_READER_LOCK());
     me -> flags &= (unsigned char)~DO_BLOCKING;
 #   ifdef E2K
       {
@@ -1876,14 +1876,14 @@ GC_INNER void GC_do_blocking_inner(ptr_t data, void *context)
     GC_bool topOfStackUnset;
 
     UNUSED_ARG(context);
-    LOCK();
+    READER_LOCK();
     me = GC_self_thread_inner();
     do_blocking_enter(&topOfStackUnset, me);
-    UNLOCK();
+    READER_UNLOCK_RELEASE();
 
     d -> client_data = (d -> fn)(d -> client_data);
 
-    LOCK();   /* This will block if the world is stopped.       */
+    READER_LOCK();  /* This will block if the world is stopped. */
 #   ifdef LINT2
       {
 #        ifdef GC_ASSERTIONS
@@ -1907,13 +1907,13 @@ GC_INNER void GC_do_blocking_inner(ptr_t data, void *context)
         word suspend_cnt = (word)(me -> ext_suspend_cnt);
                         /* read suspend counter (number) before unlocking */
 
-        UNLOCK();
+        READER_UNLOCK_RELEASE();
         GC_suspend_self_inner(me, suspend_cnt);
-        LOCK();
+        READER_LOCK();
       }
 #   endif
     do_blocking_leave(me, topOfStackUnset);
-    UNLOCK();
+    READER_UNLOCK_RELEASE();
 }
 
 #if defined(GC_ENABLE_SUSPEND_THREAD) && defined(SIGNAL_BASED_STOP_WORLD)
@@ -1926,6 +1926,10 @@ GC_INNER void GC_do_blocking_inner(ptr_t data, void *context)
 
     UNUSED_ARG(context);
     GC_ASSERT(I_HOLD_LOCK());
+                        /* The caller holds the allocator lock in the   */
+                        /* exclusive mode, thus we require and restore  */
+                        /* it to the same mode upon return from the     */
+                        /* function.                                    */
     do_blocking_enter(&topOfStackUnset, me);
     while ((me -> ext_suspend_cnt & 1) != 0) {
       word suspend_cnt = (word)(me -> ext_suspend_cnt);
@@ -1955,7 +1959,7 @@ GC_API void GC_CALL GC_set_stackbottom(void *gc_thread_handle,
       return;
     }
 
-    GC_ASSERT(I_HOLD_LOCK());
+    GC_ASSERT(I_HOLD_READER_LOCK());
     if (NULL == t) /* current thread? */
       t = GC_self_thread_inner();
     GC_ASSERT(!KNOWN_FINISHED(t));
@@ -2007,7 +2011,7 @@ GC_API void * GC_CALL GC_call_with_gc_active(GC_fn_type fn,
       ptr_t saved_bs_ptr, saved_bs_end;
 #   endif
 
-    LOCK();   /* This will block if the world is stopped.       */
+    READER_LOCK();  /* This will block if the world is stopped. */
     me = GC_self_thread_inner();
     crtn = me -> crtn;
 
@@ -2024,7 +2028,7 @@ GC_API void * GC_CALL GC_call_with_gc_active(GC_fn_type fn,
 
     if ((me -> flags & DO_BLOCKING) == 0) {
       /* We are not inside GC_do_blocking() - do nothing more.  */
-      UNLOCK();
+      READER_UNLOCK_RELEASE();
       client_data = fn(client_data);
       /* Prevent treating the above as a tail call.     */
       GC_noop1(COVERT_DATAFLOW(&stacksect));
@@ -2034,9 +2038,9 @@ GC_API void * GC_CALL GC_call_with_gc_active(GC_fn_type fn,
 #   if defined(GC_ENABLE_SUSPEND_THREAD) && defined(SIGNAL_BASED_STOP_WORLD)
       while (EXPECT((me -> ext_suspend_cnt & 1) != 0, FALSE)) {
         word suspend_cnt = (word)(me -> ext_suspend_cnt);
-        UNLOCK();
+        READER_UNLOCK_RELEASE();
         GC_suspend_self_inner(me, suspend_cnt);
-        LOCK();
+        READER_LOCK();
         GC_ASSERT(me -> crtn == crtn);
       }
 #   endif
@@ -2060,15 +2064,15 @@ GC_API void * GC_CALL GC_call_with_gc_active(GC_fn_type fn,
     me -> flags &= (unsigned char)~DO_BLOCKING;
     crtn -> traced_stack_sect = &stacksect;
 
-    UNLOCK();
+    READER_UNLOCK_RELEASE();
     client_data = fn(client_data);
     GC_ASSERT((me -> flags & DO_BLOCKING) == 0);
 
     /* Restore original "stack section".        */
 #   ifdef E2K
       (void)GC_save_regs_in_stack();
 #   endif
-    LOCK();
+    READER_LOCK();
     GC_ASSERT(me -> crtn == crtn);
     GC_ASSERT(crtn -> traced_stack_sect == &stacksect);
 #   ifdef CPPCHECK
@@ -2084,7 +2088,7 @@ GC_API void * GC_CALL GC_call_with_gc_active(GC_fn_type fn,
 #   endif
     me -> flags |= DO_BLOCKING;
     crtn -> stack_ptr = stacksect.saved_stack_ptr;
-    UNLOCK();
+    READER_UNLOCK_RELEASE();
     return client_data; /* result */
 }
 

tests/gctest.c

@@ -1854,7 +1854,8 @@ static void run_one_test(void)
           exit(0);
         }
 #   endif
-    (void)GC_call_with_alloc_lock(set_stackbottom, &thr_hndl_sb);
+    (void)GC_call_with_reader_lock(set_stackbottom, &thr_hndl_sb,
+                                   1 /* release */);
 
     /* Repeated list reversal test. */
 #   ifndef NO_CLOCK

typd_mlc.c

@@ -554,22 +554,23 @@ GC_API GC_ATTR_MALLOC void * GC_CALL GC_calloc_do_explicitly_typed(
       lp -> ld_size = pctd -> leaf.ld_size;
       lp -> ld_nelements = pctd -> leaf.ld_nelements;
       lp -> ld_descriptor = pctd -> leaf.ld_descriptor;
-      /* Hold the allocator lock while writing the descriptor word      */
-      /* to the object to ensure that the descriptor contents are seen  */
-      /* by GC_array_mark_proc as expected.                             */
+      /* Hold the allocator lock (in the reader mode which should be    */
+      /* enough) while writing the descriptor word to the object to     */
+      /* ensure that the descriptor contents are seen by                */
+      /* GC_array_mark_proc as expected.                                */
       /* TODO: It should be possible to replace locking with the atomic */
       /* operations (with the release barrier here) but, in this case,  */
       /* avoiding the acquire barrier in GC_array_mark_proc seems to    */
       /* be tricky as GC_mark_some might be invoked with the world      */
       /* running.                                                       */
-      LOCK();
+      READER_LOCK();
       ((word *)op)[nwords - 1] = (word)lp;
-      UNLOCK();
+      READER_UNLOCK_RELEASE();
     } else {
 #     ifndef GC_NO_FINALIZATION
-        LOCK();
+        READER_LOCK();
         ((word *)op)[nwords - 1] = (word)(pctd -> complex_d);
-        UNLOCK();
+        READER_UNLOCK_RELEASE();
 
         GC_dirty((word *)op + nwords - 1);
         REACHABLE_AFTER_DIRTY(pctd -> complex_d);