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.

1 files changed, 33 insertions, 14 deletions

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)