Merge "Handles spurious wake-ups in pthread_join()"

libc/bionic/pthread.c

@@ -130,23 +130,13 @@ void pthread_exit(void * retval)
             thread->tls = NULL;
         }
 
-       /* the join_count field is used to store the number of threads waiting for
-        * the termination of this thread with pthread_join(),
-        *
-        * if it is positive we need to signal the waiters, and we do not touch
-        * the count (it will be decremented by the waiters, the last one will
-        * also remove/free the thread structure
-        *
-        * if it is zero, we set the count value to -1 to indicate that the
-        * thread is in 'zombie' state: it has stopped executing, and its stack
-        * is gone (as well as its TLS area). when another thread calls pthread_join()
-        * on it, it will immediately free the thread and return.
-        */
+       /* Indicate that the thread has exited for joining threads. */
+        thread->attr.flags |= PTHREAD_ATTR_FLAG_ZOMBIE;
         thread->return_value = retval;
-        if (thread->join_count > 0) {
-            pthread_cond_broadcast(&thread->join_cond);
-        } else {
-            thread->join_count = -1;  /* zombie thread */
+
+       /* Signal the joining thread if present. */
+        if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
+            pthread_cond_signal(&thread->join_cond);
         }
     }
     pthread_mutex_unlock(&gThreadListLock);

libc/bionic/pthread_create.cpp

@@ -110,7 +110,6 @@ int _init_thread(pthread_internal_t* thread, bool add_to_thread_list) {
   }
 
   pthread_cond_init(&thread->join_cond, NULL);
-  thread->join_count = 0;
   thread->cleanup_stack = NULL;
 
   if (add_to_thread_list) {

libc/bionic/pthread_detach.cpp

@@ -40,7 +40,7 @@ int pthread_detach(pthread_t t) {
     return EINVAL; // Already detached.
   }
 
-  if (thread->join_count > 0) {
+  if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
     return 0; // Already being joined; silently do nothing, like glibc.
   }
 

libc/bionic/pthread_internal.h

@@ -42,7 +42,6 @@ typedef struct pthread_internal_t
     pid_t                       tid;
     bool                        allocated_on_heap;
     pthread_cond_t              join_cond;
-    int                         join_count;
     void*                       return_value;
     int                         internal_flags;
     __pthread_cleanup_t*        cleanup_stack;
@@ -64,9 +63,18 @@ pthread_internal_t* __get_thread(void);
 __LIBC_HIDDEN__ void pthread_key_clean_all(void);
 __LIBC_HIDDEN__ void _pthread_internal_remove_locked(pthread_internal_t* thread);
 
+/* Has the thread been detached by a pthread_join or pthread_detach call? */
 #define PTHREAD_ATTR_FLAG_DETACHED      0x00000001
+
+/* Was the thread's stack allocated by the user rather than by us? */
 #define PTHREAD_ATTR_FLAG_USER_STACK    0x00000002
 
+/* Has the thread been joined by another thread? */
+#define PTHREAD_ATTR_FLAG_JOINED        0x00000004
+
+/* Has the thread already exited but not been joined? */
+#define PTHREAD_ATTR_FLAG_ZOMBIE        0x00000008
+
 __LIBC_HIDDEN__ extern pthread_internal_t* gThreadList;
 __LIBC_HIDDEN__ extern pthread_mutex_t gThreadListLock;
 

libc/bionic/pthread_join.cpp

@@ -30,7 +30,7 @@
 
 #include "pthread_accessor.h"
 
-int pthread_join(pthread_t t, void ** ret_val) {
+int pthread_join(pthread_t t, void** ret_val) {
   if (t == pthread_self()) {
     return EDEADLK;
   }
@@ -44,25 +44,19 @@ int pthread_join(pthread_t t, void ** ret_val) {
     return EINVAL;
   }
 
-  // Wait for thread death when needed.
+  if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
+    return EINVAL;
+  }
 
-  // If the 'join_count' is negative, this is a 'zombie' thread that
-  // is already dead and without stack/TLS. Otherwise, we need to increment 'join-count'
-  // and wait to be signaled
-  int count = thread->join_count;
-  if (count >= 0) {
-    thread->join_count += 1;
+  // Signal our intention to join, and wait for the thread to exit.
+  thread->attr.flags |= PTHREAD_ATTR_FLAG_JOINED;
+  while ((thread->attr.flags & PTHREAD_ATTR_FLAG_ZOMBIE) == 0) {
     pthread_cond_wait(&thread->join_cond, &gThreadListLock);
-    count = --thread->join_count;
   }
   if (ret_val) {
     *ret_val = thread->return_value;
   }
 
-  // Remove thread from thread list when we're the last joiner or when the
-  // thread was already a zombie.
-  if (count <= 0) {
-    _pthread_internal_remove_locked(thread.get());
-  }
+  _pthread_internal_remove_locked(thread.get());
   return 0;
 }

libc/bionic/pthread_key.cpp

@@ -212,16 +212,13 @@ int pthread_key_delete(pthread_key_t key) {
   // Clear value in all threads.
   pthread_mutex_lock(&gThreadListLock);
   for (pthread_internal_t*  t = gThreadList; t != NULL; t = t->next) {
-    // Avoid zombie threads with a negative 'join_count'. These are really
-    // already dead and don't have a TLS area anymore.
-
+    // Skip zombie threads. They don't have a valid TLS area any more.
     // Similarly, it is possible to have t->tls == NULL for threads that
     // were just recently created through pthread_create() but whose
     // startup trampoline (__thread_entry) hasn't been run yet by the
-    // scheduler. t->tls will also be NULL after it's stack has been
+    // scheduler. t->tls will also be NULL after a thread's stack has been
     // unmapped but before the ongoing pthread_join() is finished.
-    // so check for this too.
-    if (t->join_count < 0 || !t->tls) {
+    if ((t->attr.flags & PTHREAD_ATTR_FLAG_ZOMBIE) || t->tls == NULL) {
       continue;
     }
 

tests/pthread_test.cpp

@@ -317,3 +317,25 @@ TEST(pthread, pthread_kill__no_such_thread) {
 
   ASSERT_EQ(ESRCH, pthread_kill(dead_thread, 0));
 }
+
+TEST(pthread, pthread_join__multijoin) {
+  bool done = false;
+
+  pthread_t t1;
+  ASSERT_EQ(0, pthread_create(&t1, NULL, SpinFn, &done));
+
+  pthread_t t2;
+  ASSERT_EQ(0, pthread_create(&t2, NULL, JoinFn, reinterpret_cast<void*>(t1)));
+
+  sleep(1); // (Give t2 a chance to call pthread_join.)
+
+  // Multiple joins to the same thread should fail.
+  ASSERT_EQ(EINVAL, pthread_join(t1, NULL));
+
+  done = true;
+
+  // ...but t2's join on t1 still goes ahead (which we can tell because our join on t2 finishes).
+  void* join_result;
+  ASSERT_EQ(0, pthread_join(t2, &join_result));
+  ASSERT_EQ(0, reinterpret_cast<int>(join_result));
+}