commit 0878d654e7be8c9666579e22522704d8887415cc
author Ian Rogers <irogers@google.com> Thu Apr 18 17:38:35 2013 -0700
committer Ian Rogers <irogers@google.com> Thu Apr 18 17:42:02 2013 -0700
tree 0e57c50e7f47a01900e540c0b5e509df4297afed
parent f8adc60455b90c95a6a3c8f87830e4e88b2644e1

Thread tidying.

Add is_started_ boolean to Thread so that we don't read an uncreated
pthread_key_self_, don't start twice or call shutdown when not started.
Don't use a MutexLock in ThreadList::Unregister, as the MutexLock will
hold a copy of self for the thread that's deleted.
Don't memory leak the resume condition variable.

Change-Id: I767968a9f785e560fc9e356a339e684de5ce2ffc

src/thread.cc

diff --git a/src/thread.cc b/src/thread.cc
index e6c32ca..07a3a90 100644
--- a/src/thread.cc
+++ b/src/thread.cc
@@ -66,8 +66,9 @@
 
 namespace art {
 
+bool Thread::is_started_ = false;
 pthread_key_t Thread::pthread_key_self_;
-ConditionVariable* Thread::resume_cond_;
+ConditionVariable* Thread::resume_cond_ = NULL;
 
 static const char* kThreadNameDuringStartup = "<native thread without managed peer>";
 
@@ -323,6 +324,7 @@
   // Set pthread_self_ ahead of pthread_setspecific, that makes Thread::Current function, this
   // avoids pthread_self_ ever being invalid when discovered from Thread::Current().
   pthread_self_ = pthread_self();
+  CHECK(is_started_);
   CHECK_PTHREAD_CALL(pthread_setspecific, (Thread::pthread_key_self_, this), "attach self");
   DCHECK_EQ(Thread::Current(), this);
 
@@ -829,7 +831,7 @@
 
   std::ostream& os;
   const Thread* thread;
-  bool can_allocate;
+  const bool can_allocate;
   MethodHelper mh;
   mirror::AbstractMethod* last_method;
   int last_line_number;
@@ -880,6 +882,7 @@
   Thread* self = reinterpret_cast<Thread*>(arg);
   if (self->thread_exit_check_count_ == 0) {
     LOG(WARNING) << "Native thread exiting without having called DetachCurrentThread (maybe it's going to use a pthread_key_create destructor?): " << *self;
+    CHECK(is_started_);
     CHECK_PTHREAD_CALL(pthread_setspecific, (Thread::pthread_key_self_, self), "reattach self");
     self->thread_exit_check_count_ = 1;
   } else {
@@ -888,6 +891,8 @@
 }
 
 void Thread::Startup() {
+  CHECK(!is_started_);
+  is_started_ = true;
   {
     MutexLock mu(Thread::Current(), *Locks::thread_suspend_count_lock_);  // Keep GCC happy.
     resume_cond_ = new ConditionVariable("Thread resumption condition variable",
@@ -915,7 +920,13 @@
 }
 
 void Thread::Shutdown() {
+  CHECK(is_started_);
+  is_started_ = false;
   CHECK_PTHREAD_CALL(pthread_key_delete, (Thread::pthread_key_self_), "self key");
+  if (resume_cond_ != NULL) {
+    delete resume_cond_;
+    resume_cond_ = NULL;
+  }
 }
 
 Thread::Thread(bool daemon)

src/thread.h

diff --git a/src/thread.h b/src/thread.h
index 37f2721..f642d56 100644
--- a/src/thread.h
+++ b/src/thread.h
@@ -101,11 +101,15 @@
   // Reset internal state of child thread after fork.
   void InitAfterFork();
 
-  static Thread* Current() __attribute__ ((pure)) {
+  static Thread* Current() {
     // We rely on Thread::Current returning NULL for a detached thread, so it's not obvious
     // that we can replace this with a direct %fs access on x86.
-    void* thread = pthread_getspecific(Thread::pthread_key_self_);
-    return reinterpret_cast<Thread*>(thread);
+    if(!is_started_) {
+      return NULL;
+    } else {
+      void* thread = pthread_getspecific(Thread::pthread_key_self_);
+      return reinterpret_cast<Thread*>(thread);
+    }
   }
 
   static Thread* FromManagedThread(const ScopedObjectAccessUnchecked& ts,
@@ -617,6 +621,9 @@
 
   static void ThreadExitCallback(void* arg);
 
+  // Has Thread::Startup been called?
+  static bool is_started_;
+
   // TLS key used to retrieve the Thread*.
   static pthread_key_t pthread_key_self_;
 

src/thread_list.cc

diff --git a/src/thread_list.cc b/src/thread_list.cc
index 45ddd23..ebb63dd 100644
--- a/src/thread_list.cc
+++ b/src/thread_list.cc
@@ -554,7 +554,8 @@
   while (self != NULL) {
     // Remove and delete the Thread* while holding the thread_list_lock_ and
     // thread_suspend_count_lock_ so that the unregistering thread cannot be suspended.
-    MutexLock mu(self, *Locks::thread_list_lock_);
+    // Note: deliberately not using MutexLock that could hold a stale self pointer.
+    Locks::thread_list_lock_->ExclusiveLock(self);
     CHECK(Contains(self));
     // Note: we don't take the thread_suspend_count_lock_ here as to be suspending a thread other
     // than yourself you need to hold the thread_list_lock_ (see Thread::ModifySuspendCount).
@@ -563,6 +564,7 @@
       delete self;
       self = NULL;
     }
+    Locks::thread_list_lock_->ExclusiveUnlock(self);
   }
 
   // Clear the TLS data, so that the underlying native thread is recognizably detached.