Fix deadlocks in write and suspend handlers if AO test-and-set is emulated

This could be tested with -D AO_USE_PTHREAD_DEFS passed to CFLAGS.

* configure.ac (AO_TRYLINK_CFLAGS): New variable.
* configure.ac [$with_libatomic_ops!=none && $need_atomic_ops_asm!=true]
(BASE_ATOMIC_OPS_EMULATED): New AC_DEFINE (defined in case of failure
of AC_TRY_LINK of a code snippet calling AO_test_and_set_acquire,
AO_CLEAR, AO_compiler_barrier, AO_store, AO_load, AO_char_store,
AO_char_load, AO_store_release, AO_load_acquire); use AO_TRYLINK_CFLAGS;
add comment.
* include/private/gcconfig.h [BASE_ATOMIC_OPS_EMULATED] (MPROTECT_VDB):
Undefine.
* mark.c [AO_CLEAR] (GC_noop6): Do not call AO_compiler_barrier() if
BASE_ATOMIC_OPS_EMULATED.
* misc.c [!GC_DISABLE_INCREMENTAL] (GC_init, GC_enable_incremental):
Do not set GC_manual_vdb if BASE_ATOMIC_OPS_EMULATED.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL]
(ao_load_acquire_async, ao_load_async, ao_store_release_async,
ao_store_async): New macro; undefine it after the usage.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL]
(GC_store_stack_ptr): Use ao_store_async() instead of AO_store().
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL]
(GC_suspend_handler_inner): Use ao_load[_acquire]_async() and
ao_store_release_async() instead of AO_load[_acquire]() and
AO_store_release(), respectively.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL
&& GC_ENABLE_SUSPEND_THREAD] (suspend_self_inner): Use
ao_load_acquire_async() instead of AO_load_acquire().

diff --git a/configure.ac b/configure.ac
index 073d007794dc9f605489eb672d53d12f48580dae..af63416b132cef4bbaca34d665b73db317a23d51 100644 (file)
--- a/configure.ac
+++ b/configure.ac
@@ -1087,8 +1087,10 @@ AS_IF([test x"$with_libatomic_ops" != xno],
             AS_IF([test x"$THREADS" != xnone],
                   [ AC_DEFINE([GC_BUILTIN_ATOMIC], [1],
                               [Use C11 (GCC) atomic intrinsics instead of
-                               libatomic_ops primitives.]) ]) ]) ],
+                               libatomic_ops primitives.]) ]) ])
+    AO_TRYLINK_CFLAGS="" ],
   [ AC_MSG_RESULT([internal])
+    AO_TRYLINK_CFLAGS="-I${srcdir}/libatomic_ops/src"
     ATOMIC_OPS_CFLAGS='-I$(top_builddir)/libatomic_ops/src -I$(top_srcdir)/libatomic_ops/src'
     ATOMIC_OPS_LIBS=""
     AC_SUBST([ATOMIC_OPS_CFLAGS])
@@ -1099,6 +1101,26 @@ AM_CONDITIONAL([USE_INTERNAL_LIBATOMIC_OPS],
 AM_CONDITIONAL([NEED_ATOMIC_OPS_ASM],
     [test x$with_libatomic_ops = xno -a x$need_atomic_ops_asm = xtrue])
 
+# Check whether particular AO primitives are emulated with locks.
+# The check below is based on the fact that linking with the libatomic_ops
+# binary file is not needed in case of absence of the emulation (except for
+# Solaris SPARC).
+AS_IF([test x$with_libatomic_ops != xnone -a x$need_atomic_ops_asm != xtrue],
+    [ old_CFLAGS="$CFLAGS"
+      CFLAGS="$CFLAGS $AO_TRYLINK_CFLAGS $CFLAGS_EXTRA"
+      AC_MSG_CHECKING([for lock-free AO load/store, test-and-set primitives])
+      AC_TRY_LINK([#include "atomic_ops.h"],
+ [AO_t x=0;unsigned char c=0;AO_TS_t z=AO_TS_INITIALIZER;
+  (void)AO_test_and_set_acquire(&z);AO_CLEAR(&z);AO_compiler_barrier();
+  AO_store(&x,AO_load(&x)+1);AO_char_store(&c,AO_char_load(&c)+1);
+  AO_store_release(&x,AO_load_acquire(&x)+1)],
+        [ AC_MSG_RESULT(yes) ],
+        [ AC_MSG_RESULT(no)
+          AC_DEFINE([BASE_ATOMIC_OPS_EMULATED], [1],
+                    [AO load, store and/or test-and-set primitives are
+                     implemented in libatomic_ops using locks.]) ])
+      CFLAGS="$old_CFLAGS" ])
+
 dnl Produce the Files
 dnl -----------------
 

diff --git a/include/private/gcconfig.h b/include/private/gcconfig.h
index 9314aec47a6ef7b26f31cc890db7f9fca3bab684..6adbf9c7c83e7ec796411a7322bf68e26ca3e3ee 100644 (file)
--- a/include/private/gcconfig.h
+++ b/include/private/gcconfig.h
@@ -3108,6 +3108,12 @@ EXTERN_C_BEGIN
 # undef GWW_VDB
 #endif
 
+#if defined(BASE_ATOMIC_OPS_EMULATED)
+  /* GC_write_fault_handler() cannot use lock-based atomic primitives   */
+  /* as this could lead to a deadlock.                                  */
+# undef MPROTECT_VDB
+#endif
+
 #if defined(USE_MUNMAP) && defined(GWW_VDB)
 # undef MPROTECT_VDB  /* TODO: Cannot deal with address space holes. */
   /* Else if MPROTECT_VDB is available but not GWW_VDB then decide      */

diff --git a/mark.c b/mark.c
index b1d59026c203cd9654fd8de8c9f32865d6ac3a50..626052a5d4de6bfe37cdc6766ea29175fe2d8e78 100644 (file)
--- a/mark.c
+++ b/mark.c
@@ -40,7 +40,7 @@ void GC_noop6(word arg1 GC_ATTR_UNUSED, word arg2 GC_ATTR_UNUSED,
               word arg5 GC_ATTR_UNUSED, word arg6 GC_ATTR_UNUSED)
 {
   /* Avoid GC_noop6 calls to be optimized away. */
-# ifdef AO_CLEAR
+# if defined(AO_CLEAR) && !defined(BASE_ATOMIC_OPS_EMULATED)
     AO_compiler_barrier(); /* to serve as a special side-effect */
 # else
     GC_noop1(0);

diff --git a/misc.c b/misc.c
index a2574e7356b7062024d828b235769e5d3a9657aa..089da7f8bef58639aff2c87a4545fb02d66efdca 100644 (file)
--- a/misc.c
+++ b/misc.c
@@ -1258,7 +1258,8 @@ GC_API void GC_CALL GC_init(void)
 #   endif
 #   ifndef GC_DISABLE_INCREMENTAL
       if (GC_incremental || 0 != GETENV("GC_ENABLE_INCREMENTAL")) {
-#       if defined(CHECKSUMS) || defined(SMALL_CONFIG)
+#       if defined(BASE_ATOMIC_OPS_EMULATED) || defined(CHECKSUMS) \
+           || defined(SMALL_CONFIG)
           /* TODO: Implement CHECKSUMS for manual VDB. */
 #       else
           if (manual_vdb_allowed) {
@@ -1403,7 +1404,8 @@ GC_API void GC_CALL GC_enable_incremental(void)
           GC_init();
           LOCK();
         } else {
-#         if !defined(CHECKSUMS) && !defined(SMALL_CONFIG)
+#         if !defined(BASE_ATOMIC_OPS_EMULATED) && !defined(CHECKSUMS) \
+             && !defined(SMALL_CONFIG)
             if (manual_vdb_allowed) {
               GC_manual_vdb = TRUE;
               GC_incremental = TRUE;

diff --git a/pthread_stop_world.c b/pthread_stop_world.c
index ad38885a955bc38fccc899fe606558e724515e52..4b2c429751eb6609642ce4ff6e0f97c1d5cf00ec 100644 (file)
--- a/pthread_stop_world.c
+++ b/pthread_stop_world.c
@@ -244,6 +244,22 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy, void *context);
   errno = old_errno;
 }
 
+#ifdef BASE_ATOMIC_OPS_EMULATED
+ /* The AO primitives emulated with locks cannot be used inside signal  */
+ /* handlers as this could cause a deadlock or a double lock.           */
+ /* The following "async" macro definitions are correct only for        */
+ /* an uniprocessor case and are provided for a test purpose.           */
+# define ao_load_acquire_async(p) (*(p))
+# define ao_load_async(p) ao_load_acquire_async(p)
+# define ao_store_release_async(p, v) (void)(*(p) = (v))
+# define ao_store_async(p, v) ao_store_release_async(p, v)
+#else
+# define ao_load_acquire_async(p) AO_load_acquire(p)
+# define ao_load_async(p) AO_load(p)
+# define ao_store_release_async(p, v) AO_store_release(p, v)
+# define ao_store_async(p, v) AO_store(p, v)
+#endif /* !BASE_ATOMIC_OPS_EMULATED */
+
 /* The lookup here is safe, since this is done on behalf        */
 /* of a thread which holds the allocation lock in order         */
 /* to stop the world.  Thus concurrent modification of the      */
@@ -275,13 +291,14 @@ GC_INLINE void GC_store_stack_ptr(GC_thread me)
   /* and fetched (by GC_push_all_stacks) using the atomic primitives to */
   /* avoid the related TSan warning.                                    */
 # ifdef SPARC
-    AO_store((volatile AO_t *)&me->stop_info.stack_ptr,
+    ao_store_async((volatile AO_t *)&me->stop_info.stack_ptr,
              (AO_t)GC_save_regs_in_stack());
 # else
 #   ifdef IA64
       me -> backing_store_ptr = GC_save_regs_in_stack();
 #   endif
-    AO_store((volatile AO_t *)&me->stop_info.stack_ptr, (AO_t)GC_approx_sp());
+    ao_store_async((volatile AO_t *)&me->stop_info.stack_ptr,
+                   (AO_t)GC_approx_sp());
 # endif
 }
 
@@ -291,7 +308,7 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
   pthread_t self = pthread_self();
   GC_thread me;
   IF_CANCEL(int cancel_state;)
-  AO_t my_stop_count = AO_load_acquire(&GC_stop_count);
+  AO_t my_stop_count = ao_load_acquire_async(&GC_stop_count);
                         /* After the barrier, this thread should see    */
                         /* the actual content of GC_threads.            */
 
@@ -312,7 +329,7 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
   me = GC_lookup_thread_async(self);
 
 # ifdef GC_ENABLE_SUSPEND_THREAD
-    if (AO_load(&me->suspended_ext)) {
+    if (ao_load_async(&me->suspended_ext)) {
       GC_store_stack_ptr(me);
       sem_post(&GC_suspend_ack_sem);
       suspend_self_inner(me);
@@ -353,7 +370,7 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
   /* thread has been stopped.  Note that sem_post() is          */
   /* the only async-signal-safe primitive in LinuxThreads.      */
   sem_post(&GC_suspend_ack_sem);
-  AO_store_release(&me->stop_info.last_stop_count, my_stop_count);
+  ao_store_release_async(&me->stop_info.last_stop_count, my_stop_count);
 
   /* Wait until that thread tells us to restart by sending      */
   /* this thread a GC_sig_thr_restart signal (should be masked  */
@@ -367,8 +384,8 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
   /* this code should not be executed.                          */
   do {
       sigsuspend (&suspend_handler_mask);
-  } while (AO_load_acquire(&GC_world_is_stopped)
-           && AO_load(&GC_stop_count) == my_stop_count);
+  } while (ao_load_acquire_async(&GC_world_is_stopped)
+           && ao_load_async(&GC_stop_count) == my_stop_count);
 
 # ifdef DEBUG_THREADS
     GC_log_printf("Continuing %p\n", (void *)self);
@@ -387,8 +404,8 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
 #   endif
     {
       /* Set the flag that the thread has been restarted.       */
-      AO_store_release(&me->stop_info.last_stop_count,
-                       (AO_t)((word)my_stop_count | THREAD_RESTARTED));
+      ao_store_release_async(&me->stop_info.last_stop_count,
+                             (AO_t)((word)my_stop_count | THREAD_RESTARTED));
     }
   }
   RESTORE_CANCEL(cancel_state);
@@ -527,7 +544,7 @@ STATIC void GC_restart_handler(int sig)
     static void *GC_CALLBACK suspend_self_inner(void *client_data) {
       GC_thread me = (GC_thread)client_data;
 
-      while (AO_load_acquire(&me->suspended_ext)) {
+      while (ao_load_acquire_async(&me->suspended_ext)) {
         /* TODO: Use sigsuspend() instead. */
         GC_brief_async_signal_safe_sleep();
       }
@@ -631,6 +648,10 @@ STATIC void GC_restart_handler(int sig)
     }
 # endif /* GC_ENABLE_SUSPEND_THREAD */
 
+# undef ao_load_acquire_async
+# undef ao_load_async
+# undef ao_store_async
+# undef ao_store_release_async
 #endif /* !GC_OPENBSD_UTHREADS && !NACL */
 
 #ifdef IA64