summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorIvan Maidanski <ivmai@mail.ru>2022-04-17 15:18:43 +0300
committerIvan Maidanski <ivmai@mail.ru>2022-04-17 15:19:14 +0300
commit5d2775378c29cb71a1f2f0076bca6ff584babbc6 (patch)
treef1309bc37ce9c9a82becc13b4680c3f724c71361
parent82c8af1a73c54768aba43648849eb8592cbae361 (diff)
downloadbdwgc-5d2775378c29cb71a1f2f0076bca6ff584babbc6.tar.gz
Fix hang on sem_wait in GC_suspend_thread if thread was resumed recently
If GC_resume_thread(t) is followed by GC_suspend_thread(t) immediately, then there is a risk of the loop (which had been entered before GC_resume_thread(t) is called) in GC_suspend_self_inner() continue iterating, thus the suspend signal handler stays pending, and thus the relevant sem_post() will not be called. The solution is to replace a boolean flag (suspended_ext) with a counter (thread_stop_info.ext_suspend_cnt) with the lowest bit indicating whether the thread is suspended manually. * include/private/pthread_stop_world.h [SIGNAL_BASED_STOP_WORLD && GC_ENABLE_SUSPEND_THREAD] (thread_stop_info.ext_suspend_cnt): Add field; add comment. * include/private/pthread_support.h [SIGNAL_BASED_STOP_WORLD && GC_ENABLE_SUSPEND_THREAD] (GC_thread.suspended_ext): Remove field. * include/private/pthread_support.h [SIGNAL_BASED_STOP_WORLD && GC_ENABLE_SUSPEND_THREAD] (GC_suspend_self_inner): Add suspend_cnt argument. * pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && GC_ENABLE_SUSPEND_THREAD] (GC_suspend_self_inner): Likewise. * pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && GC_ENABLE_SUSPEND_THREAD] (GC_suspend_handler_inner): Declare suspend_cnt local variable, set it from me->stop_info.ext_suspend_cnt, pass it GC_suspend_self_inner(). * pthread_support.c [GC_ENABLE_SUSPEND_THREAD && SIGNAL_BASED_STOP_WORLD] (GC_do_blocking_inner, GC_suspend_self_blocked, GC_call_with_gc_active): Likewise. * pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && GC_ENABLE_SUSPEND_THREAD] (GC_suspend_handler_inner): Replace me->suspended_ext with suspend_cnt&1. * pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && GC_ENABLE_SUSPEND_THREAD] (GC_suspend_self_inner): Add assertion that suspend_cnt is odd; replace me->suspended_ext loop condition to me->stop_info.ext_suspend_cnt==suspend_cnt. * pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && GC_ENABLE_SUSPEND_THREAD] (GC_suspend_thread, GC_resume_thread): Declare/use suspend_cnt local variable; increment t->stop_info.ext_suspend_cnt to indicate suspend/resume; add comment. * pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && GC_ENABLE_SUSPEND_THREAD] (GC_suspend_thread, GC_resume_thread, GC_is_thread_suspended, GC_suspend_all, GC_stop_world): Replace t->suspended_ext to t->stop_info.ext_suspend_cnt&1. * pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && E2K && GC_ENABLE_SUSPEND_THREAD] (GC_push_all_stacks): Likewise. * pthread_support.c [GC_ENABLE_SUSPEND_THREAD && SIGNAL_BASED_STOP_WORLD] (GC_do_blocking_inner, GC_suspend_self_blocked, GC_call_with_gc_active, GC_register_my_thread): Likewise.
-rw-r--r--include/private/pthread_stop_world.h8
-rw-r--r--include/private/pthread_support.h6
-rw-r--r--pthread_stop_world.c47
-rw-r--r--pthread_support.c22
4 files changed, 56 insertions, 27 deletions
diff --git a/include/private/pthread_stop_world.h b/include/private/pthread_stop_world.h
index cdaae7c8..828a37dc 100644
--- a/include/private/pthread_stop_world.h
+++ b/include/private/pthread_stop_world.h
@@ -25,6 +25,14 @@ struct thread_stop_info {
volatile AO_t last_stop_count;
/* The value of GC_stop_count when the thread */
/* last successfully handled a suspend signal. */
+# ifdef GC_ENABLE_SUSPEND_THREAD
+ volatile AO_t ext_suspend_cnt;
+ /* An odd value means thread was suspended */
+ /* externally. Incremented on every call of */
+ /* GC_suspend_thread() and GC_resume_thread(). */
+ /* Updated with the GC lock held, but could be */
+ /* read from a signal handler. */
+# endif
# endif
ptr_t stack_ptr; /* Valid only when stopped. */
diff --git a/include/private/pthread_support.h b/include/private/pthread_support.h
index c7053b64..b8dd30aa 100644
--- a/include/private/pthread_support.h
+++ b/include/private/pthread_support.h
@@ -66,10 +66,6 @@ typedef struct GC_Thread_Rep {
/* Extra bookkeeping information the stopping code uses */
struct thread_stop_info stop_info;
-# if defined(GC_ENABLE_SUSPEND_THREAD) && defined(SIGNAL_BASED_STOP_WORLD)
- volatile AO_t suspended_ext; /* Thread was suspended externally. */
-# endif
-
unsigned char flags; /* Protected by GC lock. */
# define FINISHED 1 /* Thread has exited. */
# define DETACHED 2 /* Thread is treated as detached. */
@@ -164,7 +160,7 @@ GC_INNER GC_thread GC_lookup_thread(pthread_t id);
#endif
#if defined(GC_ENABLE_SUSPEND_THREAD) && defined(SIGNAL_BASED_STOP_WORLD)
- GC_INNER void GC_suspend_self_inner(GC_thread me);
+ GC_INNER void GC_suspend_self_inner(GC_thread me, word suspend_cnt);
GC_INNER void GC_suspend_self_blocked(ptr_t thread_me, void *context);
/* Wrapper over GC_suspend_self_inner. */
diff --git a/pthread_stop_world.c b/pthread_stop_world.c
index 2f85457f..9cc4c84f 100644
--- a/pthread_stop_world.c
+++ b/pthread_stop_world.c
@@ -330,6 +330,9 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
size_t stack_size;
# endif
IF_CANCEL(int cancel_state;)
+# ifdef GC_ENABLE_SUSPEND_THREAD
+ word suspend_cnt;
+# endif
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. */
@@ -354,7 +357,8 @@ 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_async(&me->suspended_ext)) {
+ suspend_cnt = (word)ao_load_async(&(me -> stop_info.ext_suspend_cnt));
+ if ((suspend_cnt & 1) != 0) {
GC_store_stack_ptr(me);
# ifdef E2K
GC_ASSERT(NULL == me -> backing_store_end);
@@ -363,7 +367,7 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
me -> backing_store_ptr = bs_lo + stack_size;
# endif
sem_post(&GC_suspend_ack_sem);
- GC_suspend_self_inner(me);
+ GC_suspend_self_inner(me, suspend_cnt);
# ifdef DEBUG_THREADS
GC_log_printf("Continuing %p on GC_resume_thread\n", (void *)self);
# endif
@@ -627,8 +631,10 @@ STATIC void GC_restart_handler(int sig)
(void)select(0, 0, 0, 0, &tv);
}
- GC_INNER void GC_suspend_self_inner(GC_thread me) {
- while (ao_load_acquire_async(&me->suspended_ext)) {
+ GC_INNER void GC_suspend_self_inner(GC_thread me, word suspend_cnt) {
+ GC_ASSERT((suspend_cnt & 1) != 0);
+ while ((word)ao_load_acquire_async(&(me -> stop_info.ext_suspend_cnt))
+ == suspend_cnt) {
/* TODO: Use sigsuspend() instead. */
GC_brief_async_signal_safe_sleep();
}
@@ -637,24 +643,30 @@ STATIC void GC_restart_handler(int sig)
GC_API void GC_CALL GC_suspend_thread(GC_SUSPEND_THREAD_ID thread) {
GC_thread t;
AO_t saved_stop_count;
+ word suspend_cnt;
IF_CANCEL(int cancel_state;)
DCL_LOCK_STATE;
LOCK();
t = GC_lookup_thread((pthread_t)thread);
- if (t == NULL || t -> suspended_ext) {
+ if (NULL == t) {
+ UNLOCK();
+ return;
+ }
+ suspend_cnt = (word)(t -> stop_info.ext_suspend_cnt);
+ if ((suspend_cnt & 1) != 0) /* already suspended? */ {
UNLOCK();
return;
}
if ((t -> flags & (FINISHED | DO_BLOCKING)) != 0) {
- t -> suspended_ext = TRUE;
+ t -> stop_info.ext_suspend_cnt = (AO_t)(suspend_cnt | 1); /* suspend */
/* Terminated but not joined yet, or in do-blocking state. */
UNLOCK();
return;
}
if (THREAD_EQUAL((pthread_t)thread, pthread_self())) {
- t -> suspended_ext = TRUE;
+ t -> stop_info.ext_suspend_cnt = (AO_t)(suspend_cnt | 1);
GC_with_callee_saves_pushed(GC_suspend_self_blocked, (ptr_t)t);
UNLOCK();
return;
@@ -687,7 +699,8 @@ STATIC void GC_restart_handler(int sig)
AO_store(&GC_stop_count, saved_stop_count + THREAD_RESTARTED);
/* Set the flag making the change visible to the signal handler. */
- AO_store_release(&t->suspended_ext, TRUE);
+ AO_store_release(&(t -> stop_info.ext_suspend_cnt),
+ (AO_t)(suspend_cnt | 1));
/* TODO: Support GC_retry_signals (not needed for TSan) */
switch (raise_signal(t, GC_sig_suspend)) {
@@ -719,8 +732,14 @@ STATIC void GC_restart_handler(int sig)
LOCK();
t = GC_lookup_thread((pthread_t)thread);
- if (t != NULL)
- AO_store(&t->suspended_ext, FALSE);
+ if (t != NULL) {
+ word suspend_cnt = (word)(t -> stop_info.ext_suspend_cnt);
+
+ if ((suspend_cnt & 1) != 0) /* is suspended? */ {
+ /* Mark the thread as not suspended - it will be resumed shortly. */
+ AO_store(&(t -> stop_info.ext_suspend_cnt), (AO_t)(suspend_cnt + 1));
+ }
+ }
UNLOCK();
}
@@ -731,7 +750,7 @@ STATIC void GC_restart_handler(int sig)
LOCK();
t = GC_lookup_thread((pthread_t)thread);
- if (t != NULL && t -> suspended_ext)
+ if (t != NULL && (t -> stop_info.ext_suspend_cnt & 1) != 0)
is_suspended = (int)TRUE;
UNLOCK();
return is_suspended;
@@ -845,7 +864,7 @@ GC_INNER void GC_push_all_stacks(void)
if ((GC_stop_count & THREAD_RESTARTED) != 0
&& (p -> flags & DO_BLOCKING) == 0
# ifdef GC_ENABLE_SUSPEND_THREAD
- && !p->suspended_ext
+ && (p -> stop_info.ext_suspend_cnt & 1) == 0
# endif
&& !THREAD_EQUAL(p -> id, self))
continue; /* procedure stack buffer has already been freed */
@@ -903,7 +922,7 @@ STATIC int GC_suspend_all(void)
if ((p -> flags & (FINISHED | DO_BLOCKING)) != 0) continue;
# ifndef GC_OPENBSD_UTHREADS
# ifdef GC_ENABLE_SUSPEND_THREAD
- if (p -> suspended_ext) continue;
+ if ((p -> stop_info.ext_suspend_cnt & 1) != 0) continue;
# endif
if (AO_load(&p->stop_info.last_stop_count) == GC_stop_count)
continue; /* matters only if GC_retry_signals */
@@ -1236,7 +1255,7 @@ GC_INNER void GC_stop_world(void)
if ((p -> flags & (FINISHED | DO_BLOCKING)) != 0) continue;
# ifndef GC_OPENBSD_UTHREADS
# ifdef GC_ENABLE_SUSPEND_THREAD
- if (p -> suspended_ext) continue;
+ if ((p -> stop_info.ext_suspend_cnt & 1) != 0) continue;
# endif
if (GC_retry_signals
&& AO_load(&p->stop_info.last_stop_count) == GC_stop_count)
diff --git a/pthread_support.c b/pthread_support.c
index c1264fff..1f32bc4d 100644
--- a/pthread_support.c
+++ b/pthread_support.c
@@ -1596,9 +1596,12 @@ GC_INNER void GC_do_blocking_inner(ptr_t data, void * context GC_ATTR_UNUSED)
/* Note: this code cannot be moved into do_blocking_leave() */
/* otherwise there could be a static analysis tool warning */
/* (false positive) about unlock without a matching lock. */
- while (EXPECT(me -> suspended_ext, FALSE)) {
+ while (EXPECT((me -> stop_info.ext_suspend_cnt & 1) != 0, FALSE)) {
+ word suspend_cnt = (word)(me -> stop_info.ext_suspend_cnt);
+ /* read suspend counter (number) before unlocking */
+
UNLOCK();
- GC_suspend_self_inner(me);
+ GC_suspend_self_inner(me, suspend_cnt);
LOCK();
}
# endif
@@ -1616,11 +1619,13 @@ GC_INNER void GC_do_blocking_inner(ptr_t data, void * context GC_ATTR_UNUSED)
GC_bool topOfStackUnset = do_blocking_enter(me);
DCL_LOCK_STATE;
- while (me -> suspended_ext) {
+ do {
+ word suspend_cnt = (word)(me -> stop_info.ext_suspend_cnt);
+
UNLOCK();
- GC_suspend_self_inner(me);
+ GC_suspend_self_inner(me, suspend_cnt);
LOCK();
- }
+ } while ((me -> stop_info.ext_suspend_cnt & 1) != 0);
do_blocking_leave(me, topOfStackUnset);
}
#endif
@@ -1725,9 +1730,10 @@ 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 -> suspended_ext, FALSE)) {
+ while (EXPECT((me -> stop_info.ext_suspend_cnt & 1) != 0, FALSE)) {
+ word suspend_cnt = (word)(me -> stop_info.ext_suspend_cnt);
UNLOCK();
- GC_suspend_self_inner(me);
+ GC_suspend_self_inner(me, suspend_cnt);
LOCK();
}
# endif
@@ -2063,7 +2069,7 @@ GC_API int GC_CALL GC_register_my_thread(const struct GC_stack_base *sb)
# endif
# if defined(GC_ENABLE_SUSPEND_THREAD) \
&& defined(SIGNAL_BASED_STOP_WORLD)
- if (me -> suspended_ext) {
+ if ((me -> stop_info.ext_suspend_cnt & 1) != 0) {
GC_with_callee_saves_pushed(GC_suspend_self_blocked, (ptr_t)me);
}
# endif