Make pthread join_state not protected by g_thread_list_lock.

1. Move the representation of thread join_state from pthread.attr.flag
   to pthread.join_state. This clarifies thread state change.
2. Use atomic operations for pthread.join_state. So we don't need to
   protect it by g_thread_list_lock. g_thread_list_lock will be reduced
   to only protect g_thread_list or even removed in further changes.

Bug: 19636317
Change-Id: I31fb143a7c69508c7287307dd3b0776993ec0f43

6 files changed, 60 insertions, 43 deletions

diff --git a/libc/bionic/pthread_attr.cpp b/libc/bionic/pthread_attr.cpp
index be1c252..7ad3431 100644
--- a/libc/bionic/pthread_attr.cpp
+++ b/libc/bionic/pthread_attr.cpp
@@ -170,6 +170,11 @@ int pthread_attr_getguardsize(const pthread_attr_t* attr, size_t* guard_size) {
 int pthread_getattr_np(pthread_t t, pthread_attr_t* attr) {
   pthread_internal_t* thread = reinterpret_cast<pthread_internal_t*>(t);
   *attr = thread->attr;
+  // We prefer reading join_state here to setting thread->attr.flags in pthread_detach.
+  // Because data race exists in the latter case.
+  if (atomic_load(&thread->join_state) == THREAD_DETACHED) {
+    attr->flags |= PTHREAD_ATTR_FLAG_DETACHED;
+  }
   // The main thread's stack information is not stored in thread->attr, and we need to
   // collect that at runtime.
   if (thread->tid == getpid()) {
diff --git a/libc/bionic/pthread_create.cpp b/libc/bionic/pthread_create.cpp
index 2bca43f..a4bd054 100644
--- a/libc/bionic/pthread_create.cpp
+++ b/libc/bionic/pthread_create.cpp
@@ -86,6 +86,12 @@ void __init_alternate_signal_stack(pthread_internal_t* thread) {
 int __init_thread(pthread_internal_t* thread, bool add_to_thread_list) {
   int error = 0;
 
+  if (__predict_true((thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) == 0)) {
+    atomic_init(&thread->join_state, THREAD_NOT_JOINED);
+  } else {
+    atomic_init(&thread->join_state, THREAD_DETACHED);
+  }
+
   // Set the scheduling policy/priority of the thread.
   if (thread->attr.sched_policy != SCHED_NORMAL) {
     sched_param param;
@@ -263,7 +269,7 @@ int pthread_create(pthread_t* thread_out, pthread_attr_t const* attr,
   if (init_errno != 0) {
     // Mark the thread detached and replace its start_routine with a no-op.
     // Letting the thread run is the easiest way to clean up its resources.
-    thread->attr.flags |= PTHREAD_ATTR_FLAG_DETACHED;
+    atomic_store(&thread->join_state, THREAD_DETACHED);
     thread->start_routine = __do_nothing;
     pthread_mutex_unlock(&thread->startup_handshake_mutex);
     return init_errno;
diff --git a/libc/bionic/pthread_detach.cpp b/libc/bionic/pthread_detach.cpp
index c800660..0712d0d 100644
--- a/libc/bionic/pthread_detach.cpp
+++ b/libc/bionic/pthread_detach.cpp
@@ -38,21 +38,18 @@ int pthread_detach(pthread_t t) {
       return ESRCH;
     }
 
-    if (thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) {
-      return EINVAL; // Already detached.
+    ThreadJoinState old_state = THREAD_NOT_JOINED;
+    while (old_state == THREAD_NOT_JOINED &&
+           !atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_DETACHED)) {
     }
-
-    if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
-      return 0; // Already being joined; silently do nothing, like glibc.
-    }
-
-    // If the thread has not exited, we can detach it safely.
-    if ((thread->attr.flags & PTHREAD_ATTR_FLAG_ZOMBIE) == 0) {
-      thread->attr.flags |= PTHREAD_ATTR_FLAG_DETACHED;
-      return 0;
+    switch (old_state) {
+      case THREAD_NOT_JOINED: return 0;
+      case THREAD_JOINED:     return 0; // Already being joined; silently do nothing, like glibc.
+      case THREAD_DETACHED:   return THREAD_DETACHED;
+      case THREAD_EXITED_NOT_JOINED:  // Call pthread_join out of scope of pthread_accessor.
     }
   }
 
-  // The thread is in zombie state, use pthread_join to clean it up.
+  // The thread is in THREAD_EXITED_NOT_JOINED, use pthread_join to clean it up.
   return pthread_join(t, NULL);
 }
diff --git a/libc/bionic/pthread_exit.cpp b/libc/bionic/pthread_exit.cpp
index d0d64b0..81cc67b 100644
--- a/libc/bionic/pthread_exit.cpp
+++ b/libc/bionic/pthread_exit.cpp
@@ -87,9 +87,12 @@ void pthread_exit(void* return_value) {
     thread->alternate_signal_stack = NULL;
   }
 
-  bool free_mapped_space = false;
-  pthread_mutex_lock(&g_thread_list_lock);
-  if ((thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) != 0) {
+  ThreadJoinState old_state = THREAD_NOT_JOINED;
+  while (old_state == THREAD_NOT_JOINED &&
+         !atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_EXITED_NOT_JOINED)) {
+  }
+
+  if (old_state == THREAD_DETACHED) {
     // The thread is detached, no one will use pthread_internal_t after pthread_exit.
     // So we can free mapped space, which includes pthread_internal_t and thread stack.
     // First make sure that the kernel does not try to clear the tid field
@@ -97,28 +100,25 @@ void pthread_exit(void* return_value) {
     __set_tid_address(NULL);
 
     // pthread_internal_t is freed below with stack, not here.
+    pthread_mutex_lock(&g_thread_list_lock);
     _pthread_internal_remove_locked(thread, false);
-    free_mapped_space = true;
-  } else {
-    // Mark the thread as exiting without freeing pthread_internal_t.
-    thread->attr.flags |= PTHREAD_ATTR_FLAG_ZOMBIE;
-  }
-  pthread_mutex_unlock(&g_thread_list_lock);
-
-  if (free_mapped_space && thread->mmap_size != 0) {
-    // We need to free mapped space for detached threads when they exit.
-    // That's not something we can do in C.
-
-    // We don't want to take a signal after we've unmapped the stack.
-    // That's one last thing we can handle in C.
-    sigset_t mask;
-    sigfillset(&mask);
-    sigprocmask(SIG_SETMASK, &mask, NULL);
-
-    _exit_with_stack_teardown(thread->attr.stack_base, thread->mmap_size);
-  } else {
-    // No need to free mapped space. Either there was no space mapped, or it is left for
-    // the pthread_join caller to clean up.
-    __exit(0);
+    pthread_mutex_unlock(&g_thread_list_lock);
+
+    if (thread->mmap_size != 0) {
+      // We need to free mapped space for detached threads when they exit.
+      // That's not something we can do in C.
+
+      // We don't want to take a signal after we've unmapped the stack.
+      // That's one last thing we can handle in C.
+      sigset_t mask;
+      sigfillset(&mask);
+      sigprocmask(SIG_SETMASK, &mask, NULL);
+
+      _exit_with_stack_teardown(thread->attr.stack_base, thread->mmap_size);
+    }
   }
+
+  // No need to free mapped space. Either there was no space mapped, or it is left for
+  // the pthread_join caller to clean up.
+  __exit(0);
 }
diff --git a/libc/bionic/pthread_internal.h b/libc/bionic/pthread_internal.h
index 6ace301..8da99dd 100644
--- a/libc/bionic/pthread_internal.h
+++ b/libc/bionic/pthread_internal.h
@@ -29,6 +29,7 @@
 #define _PTHREAD_INTERNAL_H_
 
 #include <pthread.h>
+#include <stdatomic.h>
 
 #include "private/bionic_tls.h"
 
@@ -46,6 +47,13 @@ struct pthread_key_data_t {
   void* data;
 };
 
+enum ThreadJoinState {
+  THREAD_NOT_JOINED,
+  THREAD_EXITED_NOT_JOINED,
+  THREAD_JOINED,
+  THREAD_DETACHED
+};
+
 struct pthread_internal_t {
   struct pthread_internal_t* next;
   struct pthread_internal_t* prev;
@@ -74,6 +82,8 @@ struct pthread_internal_t {
 
   pthread_attr_t attr;
 
+  _Atomic(ThreadJoinState) join_state;
+
   __pthread_cleanup_t* cleanup_stack;
 
   void* (*start_routine)(void*);
diff --git a/libc/bionic/pthread_join.cpp b/libc/bionic/pthread_join.cpp
index e3350ef..15543b4 100644
--- a/libc/bionic/pthread_join.cpp
+++ b/libc/bionic/pthread_join.cpp
@@ -44,16 +44,15 @@ int pthread_join(pthread_t t, void** return_value) {
       return ESRCH;
     }
 
-    if ((thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) != 0) {
-      return EINVAL;
+    ThreadJoinState old_state = THREAD_NOT_JOINED;
+    while ((old_state == THREAD_NOT_JOINED || old_state == THREAD_EXITED_NOT_JOINED) &&
+           !atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_JOINED)) {
     }
 
-    if ((thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) != 0) {
+    if (old_state == THREAD_DETACHED || old_state == THREAD_JOINED) {
       return EINVAL;
     }
 
-    // Okay, looks like we can signal our intention to join.
-    thread->attr.flags |= PTHREAD_ATTR_FLAG_JOINED;
     tid = thread->tid;
     tid_ptr = &thread->tid;
   }