Let g_thread_list_lock only protect g_thread_list.

As glibc/netbsd don't protect access to thread struct members by a global
lock, we don't want to do it either. This change reduces the
responsibility of g_thread_list_lock to only protect g_thread_list.

Bug: 19636317
Change-Id: I897890710653dac165d8fa4452c7ecf74abdbf2b

libc/Android.mk

@@ -521,7 +521,7 @@ libc_pthread_src_files := \
     bionic/pthread_getcpuclockid.cpp \
     bionic/pthread_getschedparam.cpp \
     bionic/pthread_gettid_np.cpp \
-    bionic/pthread_internals.cpp \
+    bionic/pthread_internal.cpp \
     bionic/pthread_join.cpp \
     bionic/pthread_key.cpp \
     bionic/pthread_kill.cpp \

libc/bionic/libc_init_common.cpp

@@ -92,7 +92,7 @@ void __libc_init_tls(KernelArgumentBlock& args) {
   main_thread.attr.stack_size = 0; // User code should never see this; we'll compute it when asked.
   // TODO: the main thread's sched_policy and sched_priority need to be queried.
 
-  __init_thread(&main_thread, false);
+  __init_thread(&main_thread);
   __init_tls(&main_thread);
   __set_tls(main_thread.tls);
   main_thread.tls[TLS_SLOT_BIONIC_PREINIT] = &args;
@@ -113,7 +113,7 @@ void __libc_init_common(KernelArgumentBlock& args) {
 
   // Get the main thread from TLS and add it to the thread list.
   pthread_internal_t* main_thread = __get_thread();
-  _pthread_internal_add(main_thread);
+  __pthread_internal_add(main_thread);
 
   __system_properties_init(); // Requires 'environ'.
 

libc/bionic/pthread_accessor.h

@@ -1,64 +0,0 @@
-/*
- * Copyright (C) 2013 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#ifndef PTHREAD_ACCESSOR_H
-#define PTHREAD_ACCESSOR_H
-
-#include <pthread.h>
-
-#include "private/bionic_macros.h"
-#include "pthread_internal.h"
-
-class pthread_accessor {
- public:
-  explicit pthread_accessor(pthread_t desired_thread) {
-    Lock();
-    for (thread_ = g_thread_list; thread_ != NULL; thread_ = thread_->next) {
-      if (thread_ == reinterpret_cast<pthread_internal_t*>(desired_thread)) {
-        break;
-      }
-    }
-  }
-
-  ~pthread_accessor() {
-    Unlock();
-  }
-
-  void Unlock() {
-    if (is_locked_) {
-      is_locked_ = false;
-      thread_ = NULL;
-      pthread_mutex_unlock(&g_thread_list_lock);
-    }
-  }
-
-  pthread_internal_t& operator*() const { return *thread_; }
-  pthread_internal_t* operator->() const { return thread_; }
-  pthread_internal_t* get() const { return thread_; }
-
- private:
-  pthread_internal_t* thread_;
-  bool is_locked_;
-
-  void Lock() {
-    pthread_mutex_lock(&g_thread_list_lock);
-    is_locked_ = true;
-  }
-
-  DISALLOW_COPY_AND_ASSIGN(pthread_accessor);
-};
-
-#endif // PTHREAD_ACCESSOR_H

libc/bionic/pthread_create.cpp

@@ -83,7 +83,7 @@ void __init_alternate_signal_stack(pthread_internal_t* thread) {
   }
 }
 
-int __init_thread(pthread_internal_t* thread, bool add_to_thread_list) {
+int __init_thread(pthread_internal_t* thread) {
   int error = 0;
 
   if (__predict_true((thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) == 0)) {
@@ -108,10 +108,6 @@ int __init_thread(pthread_internal_t* thread, bool add_to_thread_list) {
 
   thread->cleanup_stack = NULL;
 
-  if (add_to_thread_list) {
-    _pthread_internal_add(thread);
-  }
-
   return error;
 }
 
@@ -265,7 +261,7 @@ int pthread_create(pthread_t* thread_out, pthread_attr_t const* attr,
     return clone_errno;
   }
 
-  int init_errno = __init_thread(thread, true);
+  int init_errno = __init_thread(thread);
   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.
@@ -276,7 +272,7 @@ int pthread_create(pthread_t* thread_out, pthread_attr_t const* attr,
   }
 
   // Publish the pthread_t and unlock the mutex to let the new thread start running.
-  *thread_out = reinterpret_cast<pthread_t>(thread);
+  *thread_out = __pthread_internal_add(thread);
   pthread_mutex_unlock(&thread->startup_handshake_mutex);
 
   return 0;

libc/bionic/pthread_detach.cpp

@@ -29,12 +29,11 @@
 #include <errno.h>
 #include <pthread.h>
 
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
 
 int pthread_detach(pthread_t t) {
-  {
+  pthread_internal_t* thread = __pthread_internal_find(t);
-    pthread_accessor thread(t);
+  if (thread == NULL) {
-    if (thread.get() == NULL) {
     return ESRCH;
   }
 
@@ -42,14 +41,12 @@ int pthread_detach(pthread_t t) {
   while (old_state == THREAD_NOT_JOINED &&
          !atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_DETACHED)) {
   }
-    switch (old_state) {
-      case THREAD_NOT_JOINED: return 0;
-      case THREAD_JOINED:     return EINVAL;
-      case THREAD_DETACHED:   return EINVAL;
-      case THREAD_EXITED_NOT_JOINED: break;  // Call pthread_join out of scope of pthread_accessor.
-    }
-  }
 
-  // The thread is in THREAD_EXITED_NOT_JOINED, use pthread_join to clean it up.
+  if (old_state == THREAD_NOT_JOINED) {
+    return 0;
+  } else if (old_state == THREAD_EXITED_NOT_JOINED) {
+    // Use pthread_join to clean it up.
     return pthread_join(t, NULL);
+  }
+  return EINVAL;
 }

libc/bionic/pthread_exit.cpp

@@ -100,9 +100,7 @@ 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(thread);
-    _pthread_internal_remove_locked(thread, false);
-    pthread_mutex_unlock(&g_thread_list_lock);
 
     if (thread->mmap_size != 0) {
       // We need to free mapped space for detached threads when they exit.

libc/bionic/pthread_getcpuclockid.cpp

@@ -28,11 +28,11 @@
 
 #include <errno.h>
 
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
 
 int pthread_getcpuclockid(pthread_t t, clockid_t* clockid) {
-  pthread_accessor thread(t);
+  pthread_internal_t* thread = __pthread_internal_find(t);
-  if (thread.get() == NULL) {
+  if (thread == NULL) {
     return ESRCH;
   }
 

libc/bionic/pthread_getschedparam.cpp

@@ -29,13 +29,13 @@
 #include <errno.h>
 
 #include "private/ErrnoRestorer.h"
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
 
 int pthread_getschedparam(pthread_t t, int* policy, sched_param* param) {
   ErrnoRestorer errno_restorer;
 
-  pthread_accessor thread(t);
+  pthread_internal_t* thread = __pthread_internal_find(t);
-  if (thread.get() == NULL) {
+  if (thread == NULL) {
     return ESRCH;
   }
 

libc/bionic/pthread_internal.cpp

@@ -38,26 +38,10 @@
 #include "private/libc_logging.h"
 #include "private/ScopedPthreadMutexLocker.h"
 
-pthread_internal_t* g_thread_list = NULL;
+static pthread_internal_t* g_thread_list = NULL;
-pthread_mutex_t g_thread_list_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t g_thread_list_lock = PTHREAD_MUTEX_INITIALIZER;
 
-void _pthread_internal_remove_locked(pthread_internal_t* thread, bool free_thread) {
+pthread_t __pthread_internal_add(pthread_internal_t* thread) {
-  if (thread->next != NULL) {
-    thread->next->prev = thread->prev;
-  }
-  if (thread->prev != NULL) {
-    thread->prev->next = thread->next;
-  } else {
-    g_thread_list = thread->next;
-  }
-
-  if (free_thread && thread->mmap_size != 0) {
-    // Free mapped space, including thread stack and pthread_internal_t.
-    munmap(thread->attr.stack_base, thread->mmap_size);
-  }
-}
-
-void _pthread_internal_add(pthread_internal_t* thread) {
   ScopedPthreadMutexLocker locker(&g_thread_list_lock);
 
   // We insert at the head.
@@ -67,4 +51,42 @@ void _pthread_internal_add(pthread_internal_t* thread) {
     thread->next->prev = thread;
   }
   g_thread_list = thread;
+  return reinterpret_cast<pthread_t>(thread);
+}
+
+void __pthread_internal_remove(pthread_internal_t* thread) {
+  ScopedPthreadMutexLocker locker(&g_thread_list_lock);
+
+  if (thread->next != NULL) {
+    thread->next->prev = thread->prev;
+  }
+  if (thread->prev != NULL) {
+    thread->prev->next = thread->next;
+  } else {
+    g_thread_list = thread->next;
+  }
+}
+
+static void __pthread_internal_free(pthread_internal_t* thread) {
+  if (thread->mmap_size != 0) {
+    // Free mapped space, including thread stack and pthread_internal_t.
+    munmap(thread->attr.stack_base, thread->mmap_size);
+  }
+}
+
+void __pthread_internal_remove_and_free(pthread_internal_t* thread) {
+  __pthread_internal_remove(thread);
+  __pthread_internal_free(thread);
+}
+
+pthread_internal_t* __pthread_internal_find(pthread_t thread_id) {
+  pthread_internal_t* thread = reinterpret_cast<pthread_internal_t*>(thread_id);
+  ScopedPthreadMutexLocker locker(&g_thread_list_lock);
+
+  for (pthread_internal_t* t = g_thread_list; t != NULL; t = t->next) {
+    if (t == thread) {
+      return thread;
+    }
+  }
+  return NULL;
 }

libc/bionic/pthread_internal.h

@@ -105,10 +105,14 @@ struct pthread_internal_t {
   char dlerror_buffer[__BIONIC_DLERROR_BUFFER_SIZE];
 } __attribute__((aligned(16))); // Align it as thread stack top below it should be aligned.
 
-__LIBC_HIDDEN__ int __init_thread(pthread_internal_t* thread, bool add_to_thread_list);
+__LIBC_HIDDEN__ int __init_thread(pthread_internal_t* thread);
 __LIBC_HIDDEN__ void __init_tls(pthread_internal_t* thread);
 __LIBC_HIDDEN__ void __init_alternate_signal_stack(pthread_internal_t*);
-__LIBC_HIDDEN__ void _pthread_internal_add(pthread_internal_t* thread);
+
+__LIBC_HIDDEN__ pthread_t           __pthread_internal_add(pthread_internal_t* thread);
+__LIBC_HIDDEN__ pthread_internal_t* __pthread_internal_find(pthread_t pthread_id);
+__LIBC_HIDDEN__ void                __pthread_internal_remove(pthread_internal_t* thread);
+__LIBC_HIDDEN__ void                __pthread_internal_remove_and_free(pthread_internal_t* thread);
 
 // Make __get_thread() inlined for performance reason. See http://b/19825434.
 static inline __always_inline pthread_internal_t* __get_thread() {
@@ -116,7 +120,6 @@ static inline __always_inline pthread_internal_t* __get_thread() {
 }
 
 __LIBC_HIDDEN__ void pthread_key_clean_all(void);
-__LIBC_HIDDEN__ void _pthread_internal_remove_locked(pthread_internal_t* thread, bool free_thread);
 
 /*
  * Traditionally we gave threads a 1MiB stack. When we started
@@ -127,9 +130,6 @@ __LIBC_HIDDEN__ void _pthread_internal_remove_locked(pthread_internal_t* thread,
  */
 #define PTHREAD_STACK_SIZE_DEFAULT ((1 * 1024 * 1024) - SIGSTKSZ)
 
-__LIBC_HIDDEN__ extern pthread_internal_t* g_thread_list;
-__LIBC_HIDDEN__ extern pthread_mutex_t g_thread_list_lock;
-
 /* Needed by fork. */
 __LIBC_HIDDEN__ extern void __bionic_atfork_run_prepare();
 __LIBC_HIDDEN__ extern void __bionic_atfork_run_child();

libc/bionic/pthread_join.cpp

@@ -29,18 +29,15 @@
 #include <errno.h>
 
 #include "private/bionic_futex.h"
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
 
 int pthread_join(pthread_t t, void** return_value) {
   if (t == pthread_self()) {
     return EDEADLK;
   }
 
-  pid_t tid;
+  pthread_internal_t* thread = __pthread_internal_find(t);
-  volatile int* tid_ptr;
+  if (thread == NULL) {
-  {
-    pthread_accessor thread(t);
-    if (thread.get() == NULL) {
     return ESRCH;
   }
 
@@ -53,11 +50,10 @@ int pthread_join(pthread_t t, void** return_value) {
     return EINVAL;
   }
 
-    tid = thread->tid;
+  pid_t tid = thread->tid;
-    tid_ptr = &thread->tid;
+  volatile int* tid_ptr = &thread->tid;
-  }
 
-  // We set the PTHREAD_ATTR_FLAG_JOINED flag with the lock held,
+  // We set thread->join_state to THREAD_JOINED with atomic operation,
   // so no one is going to remove this thread except us.
 
   // Wait for the thread to actually exit, if it hasn't already.
@@ -65,14 +61,10 @@ int pthread_join(pthread_t t, void** return_value) {
     __futex_wait(tid_ptr, tid, NULL);
   }
 
-  // Take the lock again so we can pull the thread's return value
-  // and remove the thread from the list.
-  pthread_accessor thread(t);
-
   if (return_value) {
     *return_value = thread->return_value;
   }
 
-  _pthread_internal_remove_locked(thread.get(), true);
+  __pthread_internal_remove_and_free(thread);
   return 0;
 }

libc/bionic/pthread_kill.cpp

@@ -30,26 +30,17 @@
 #include <unistd.h>
 
 #include "private/ErrnoRestorer.h"
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
 
 extern "C" int tgkill(int tgid, int tid, int sig);
 
 int pthread_kill(pthread_t t, int sig) {
   ErrnoRestorer errno_restorer;
 
-  pthread_accessor thread(t);
+  pthread_internal_t* thread = __pthread_internal_find(t);
-  if (thread.get() == NULL) {
+  if (thread == NULL) {
     return ESRCH;
   }
 
-  // There's a race here, but it's one we share with all other C libraries.
+  return (tgkill(getpid(), thread->tid, sig) == -1) ? errno : 0;
-  pid_t tid = thread->tid;
-  thread.Unlock();
-
-  int rc = tgkill(getpid(), tid, sig);
-  if (rc == -1) {
-    return errno;
-  }
-
-  return 0;
 }

libc/bionic/pthread_setname_np.cpp

@@ -36,9 +36,8 @@
 #include <sys/types.h>
 #include <unistd.h>
 
-#include "pthread_accessor.h"
-#include "pthread_internal.h"
 #include "private/ErrnoRestorer.h"
+#include "pthread_internal.h"
 
 // This value is not exported by kernel headers.
 #define MAX_TASK_COMM_LEN 16
@@ -58,14 +57,12 @@ int pthread_setname_np(pthread_t t, const char* thread_name) {
   }
 
   // We have to change another thread's name.
-  pid_t tid = 0;
+  pthread_internal_t* thread = __pthread_internal_find(t);
-  {
+  if (thread == NULL) {
-    pthread_accessor thread(t);
-    if (thread.get() == NULL) {
     return ENOENT;
   }
-    tid = thread->tid;
+  pid_t tid = thread->tid;
-  }
+
   char comm_name[sizeof(TASK_COMM_FMT) + 8];
   snprintf(comm_name, sizeof(comm_name), TASK_COMM_FMT, tid);
   int fd = open(comm_name, O_CLOEXEC | O_WRONLY);

libc/bionic/pthread_setschedparam.cpp

@@ -29,13 +29,13 @@
 #include <errno.h>
 
 #include "private/ErrnoRestorer.h"
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
 
 int pthread_setschedparam(pthread_t t, int policy, const sched_param* param) {
   ErrnoRestorer errno_restorer;
 
-  pthread_accessor thread(t);
+  pthread_internal_t* thread = __pthread_internal_find(t);
-  if (thread.get() == NULL) {
+  if (thread == NULL) {
     return ESRCH;
   }