From 8d4ecdfc81edab312a9feef9b5985f327ec50bad Mon Sep 17 00:00:00 2001 From: Ivan Maidanski Date: Tue, 20 Nov 2018 11:42:52 +0300 Subject: [PATCH] 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(). --- configure.ac | 24 +++++++++++++++++++++++- include/private/gcconfig.h | 6 ++++++ mark.c | 2 +- misc.c | 6 ++++-- pthread_stop_world.c | 41 +++++++++++++++++++++++++++++++---------- 5 files changed, 65 insertions(+), 14 deletions(-) diff --git a/configure.ac b/configure.ac index 9ffe81d..7db0645 100644 --- a/configure.ac +++ b/configure.ac @@ -1088,8 +1088,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]) @@ -1100,6 +1102,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 9ed9da8..0838140 100644 --- a/include/private/gcconfig.h +++ b/include/private/gcconfig.h @@ -3125,6 +3125,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 12d927f..283aa2f 100644 --- 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 bcf4c72..7e0bef7 100644 --- 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 ad38885..4b2c429 100644 --- 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 -- 2.7.4