Modify the behavior of thread suspend shootouts.

The thread doing the suspension doesn't attempt to suspend the other thread
unless it knows another thread isn't trying to suspend it. Use the suspend
count, and its lock, for this purpose.
Re-enable ThreadStress test.
Bug: 15446488

Change-Id: Idd34410c7b89d8abd6973e5699a15ca699472c78

12 files changed, 28 insertions, 64 deletions

diff --git a/build/Android.common_build.mk b/build/Android.common_build.mk
index 7e58f5c..a221cfc 100644
--- a/build/Android.common_build.mk
+++ b/build/Android.common_build.mk
@@ -173,7 +173,9 @@ art_gcc_cflags := -Wunused-but-set-parameter
 
 
 ifeq ($(ART_HOST_CLANG),true)
-  ART_HOST_CFLAGS += $(art_clang_cflags)
+  # Bug: 15446488. We don't omit the frame pointer to work around
+  # clang/libunwind bugs that cause SEGVs in run-test-004-ThreadStress.
+  ART_HOST_CFLAGS += $(art_clang_cflags) -fno-omit-frame-pointer
 else
   ART_HOST_CFLAGS += $(art_gcc_cflags)
 endif
diff --git a/runtime/base/mutex.cc b/runtime/base/mutex.cc
index 423ea77..4957988 100644
--- a/runtime/base/mutex.cc
+++ b/runtime/base/mutex.cc
@@ -57,7 +57,6 @@ Mutex* Locks::reference_queue_soft_references_lock_ = nullptr;
 Mutex* Locks::reference_queue_weak_references_lock_ = nullptr;
 Mutex* Locks::runtime_shutdown_lock_ = nullptr;
 Mutex* Locks::thread_list_lock_ = nullptr;
-Mutex* Locks::thread_list_suspend_thread_lock_ = nullptr;
 Mutex* Locks::thread_suspend_count_lock_ = nullptr;
 Mutex* Locks::trace_lock_ = nullptr;
 Mutex* Locks::unexpected_signal_lock_ = nullptr;
@@ -202,7 +201,7 @@ void BaseMutex::CheckSafeToWait(Thread* self) {
       if (i != level_) {
         BaseMutex* held_mutex = self->GetHeldMutex(static_cast<LockLevel>(i));
         // We expect waits to happen while holding the thread list suspend thread lock.
-        if (held_mutex != NULL && i != kThreadListSuspendThreadLock) {
+        if (held_mutex != NULL) {
           LOG(ERROR) << "Holding \"" << held_mutex->name_ << "\" "
                      << "(level " << LockLevel(i) << ") while performing wait on "
                      << "\"" << name_ << "\" (level " << level_ << ")";
@@ -918,16 +917,14 @@ void Locks::Init() {
     DCHECK(mutator_lock_ != nullptr);
     DCHECK(profiler_lock_ != nullptr);
     DCHECK(thread_list_lock_ != nullptr);
-    DCHECK(thread_list_suspend_thread_lock_ != nullptr);
     DCHECK(thread_suspend_count_lock_ != nullptr);
     DCHECK(trace_lock_ != nullptr);
     DCHECK(unexpected_signal_lock_ != nullptr);
   } else {
     // Create global locks in level order from highest lock level to lowest.
-    LockLevel current_lock_level = kThreadListSuspendThreadLock;
-    DCHECK(thread_list_suspend_thread_lock_ == nullptr);
-    thread_list_suspend_thread_lock_ =
-        new Mutex("thread list suspend thread by .. lock", current_lock_level);
+    LockLevel current_lock_level = kInstrumentEntrypointsLock;
+    DCHECK(instrument_entrypoints_lock_ == nullptr);
+    instrument_entrypoints_lock_ = new Mutex("instrument entrypoint lock", current_lock_level);
 
     #define UPDATE_CURRENT_LOCK_LEVEL(new_level) \
       if (new_level >= current_lock_level) { \
@@ -938,10 +935,6 @@ void Locks::Init() {
       } \
       current_lock_level = new_level;
 
-    UPDATE_CURRENT_LOCK_LEVEL(kInstrumentEntrypointsLock);
-    DCHECK(instrument_entrypoints_lock_ == nullptr);
-    instrument_entrypoints_lock_ = new Mutex("instrument entrypoint lock", current_lock_level);
-
     UPDATE_CURRENT_LOCK_LEVEL(kMutatorLock);
     DCHECK(mutator_lock_ == nullptr);
     mutator_lock_ = new ReaderWriterMutex("mutator lock", current_lock_level);
diff --git a/runtime/base/mutex.h b/runtime/base/mutex.h
index d589eb6..9c93cc6 100644
--- a/runtime/base/mutex.h
+++ b/runtime/base/mutex.h
@@ -101,7 +101,6 @@ enum LockLevel {
   kHeapBitmapLock,
   kMutatorLock,
   kInstrumentEntrypointsLock,
-  kThreadListSuspendThreadLock,
   kZygoteCreationLock,
 
   kLockLevelCount  // Must come last.
@@ -486,17 +485,8 @@ class Locks {
  public:
   static void Init();
 
-  // There's a potential race for two threads to try to suspend each other and for both of them
-  // to succeed and get blocked becoming runnable. This lock ensures that only one thread is
-  // requesting suspension of another at any time. As the the thread list suspend thread logic
-  // transitions to runnable, if the current thread were tried to be suspended then this thread
-  // would block holding this lock until it could safely request thread suspension of the other
-  // thread without that thread having a suspension request against this thread. This avoids a
-  // potential deadlock cycle.
-  static Mutex* thread_list_suspend_thread_lock_;
-
   // Guards allocation entrypoint instrumenting.
-  static Mutex* instrument_entrypoints_lock_ ACQUIRED_AFTER(thread_list_suspend_thread_lock_);
+  static Mutex* instrument_entrypoints_lock_;
 
   // The mutator_lock_ is used to allow mutators to execute in a shared (reader) mode or to block
   // mutators by having an exclusive (writer) owner. In normal execution each mutator thread holds
diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 584743b..e2f6085 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -2408,9 +2408,7 @@ JDWP::JdwpError Dbg::SuspendThread(JDWP::ObjectId thread_id, bool request_suspen
   if (peer.get() == nullptr) {
     return JDWP::ERR_THREAD_NOT_ALIVE;
   }
-  // Suspend thread to build stack trace. Take suspend thread lock to avoid races with threads
-  // trying to suspend this one.
-  MutexLock mu(self, *Locks::thread_list_suspend_thread_lock_);
+  // Suspend thread to build stack trace.
   bool timed_out;
   ThreadList* thread_list = Runtime::Current()->GetThreadList();
   Thread* thread = thread_list->SuspendThreadByPeer(peer.get(), request_suspension, true,
@@ -3322,13 +3320,9 @@ class ScopedThreadSuspension {
         soa.Self()->TransitionFromRunnableToSuspended(kWaitingForDebuggerSuspension);
         jobject thread_peer = Dbg::GetObjectRegistry()->GetJObject(thread_id);
         bool timed_out;
-        Thread* suspended_thread;
-        {
-          // Take suspend thread lock to avoid races with threads trying to suspend this one.
-          MutexLock mu(soa.Self(), *Locks::thread_list_suspend_thread_lock_);
-          ThreadList* thread_list = Runtime::Current()->GetThreadList();
-          suspended_thread = thread_list->SuspendThreadByPeer(thread_peer, true, true, &timed_out);
-        }
+        ThreadList* thread_list = Runtime::Current()->GetThreadList();
+        Thread* suspended_thread = thread_list->SuspendThreadByPeer(thread_peer, true, true,
+                                                                    &timed_out);
         CHECK_EQ(soa.Self()->TransitionFromSuspendedToRunnable(), kWaitingForDebuggerSuspension);
         if (suspended_thread == nullptr) {
           // Thread terminated from under us while suspending.
diff --git a/runtime/monitor.cc b/runtime/monitor.cc
index 6445b88..233267b 100644
--- a/runtime/monitor.cc
+++ b/runtime/monitor.cc
@@ -655,8 +655,6 @@ void Monitor::InflateThinLocked(Thread* self, Handle<mirror::Object> obj, LockWo
     Thread* owner;
     {
       ScopedThreadStateChange tsc(self, kBlocked);
-      // Take suspend thread lock to avoid races with threads trying to suspend this one.
-      MutexLock mu(self, *Locks::thread_list_suspend_thread_lock_);
       owner = thread_list->SuspendThreadByThreadId(owner_thread_id, false, &timed_out);
     }
     if (owner != nullptr) {
diff --git a/runtime/native/dalvik_system_VMStack.cc b/runtime/native/dalvik_system_VMStack.cc
index e396dad..2cdc68f 100644
--- a/runtime/native/dalvik_system_VMStack.cc
+++ b/runtime/native/dalvik_system_VMStack.cc
@@ -38,12 +38,7 @@ static jobject GetThreadStack(const ScopedFastNativeObjectAccess& soa, jobject p
     soa.Self()->TransitionFromRunnableToSuspended(kNative);
     ThreadList* thread_list = Runtime::Current()->GetThreadList();
     bool timed_out;
-    Thread* thread;
-    {
-      // Take suspend thread lock to avoid races with threads trying to suspend this one.
-      MutexLock mu(soa.Self(), *Locks::thread_list_suspend_thread_lock_);
-      thread = thread_list->SuspendThreadByPeer(peer, true, false, &timed_out);
-    }
+    Thread* thread = thread_list->SuspendThreadByPeer(peer, true, false, &timed_out);
     if (thread != nullptr) {
       // Must be runnable to create returned array.
       CHECK_EQ(soa.Self()->TransitionFromSuspendedToRunnable(), kNative);
diff --git a/runtime/native/java_lang_Thread.cc b/runtime/native/java_lang_Thread.cc
index 0722a24..420e9df 100644
--- a/runtime/native/java_lang_Thread.cc
+++ b/runtime/native/java_lang_Thread.cc
@@ -133,11 +133,7 @@ static void Thread_nativeSetName(JNIEnv* env, jobject peer, jstring java_name) {
   ThreadList* thread_list = Runtime::Current()->GetThreadList();
   bool timed_out;
   // Take suspend thread lock to avoid races with threads trying to suspend this one.
-  Thread* thread;
-  {
-    MutexLock mu(self, *Locks::thread_list_suspend_thread_lock_);
-    thread = thread_list->SuspendThreadByPeer(peer, true, false, &timed_out);
-  }
+  Thread* thread = thread_list->SuspendThreadByPeer(peer, true, false, &timed_out);
   if (thread != NULL) {
     {
       ScopedObjectAccess soa(env);
diff --git a/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc b/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
index b74430f..987427e 100644
--- a/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
+++ b/runtime/native/org_apache_harmony_dalvik_ddmc_DdmVmInternal.cc
@@ -63,12 +63,7 @@ static jobjectArray DdmVmInternal_getStackTraceById(JNIEnv* env, jclass, jint th
     }
 
     // Suspend thread to build stack trace.
-    Thread* thread;
-    {
-      // Take suspend thread lock to avoid races with threads trying to suspend this one.
-      MutexLock mu(self, *Locks::thread_list_suspend_thread_lock_);
-      thread = thread_list->SuspendThreadByThreadId(thin_lock_id, false, &timed_out);
-    }
+    Thread* thread = thread_list->SuspendThreadByThreadId(thin_lock_id, false, &timed_out);
     if (thread != nullptr) {
       {
         ScopedObjectAccess soa(env);
diff --git a/runtime/thread-inl.h b/runtime/thread-inl.h
index 94f7585..e30e745 100644
--- a/runtime/thread-inl.h
+++ b/runtime/thread-inl.h
@@ -85,7 +85,7 @@ inline void Thread::AssertThreadSuspensionIsAllowable(bool check_locks) const {
       bool bad_mutexes_held = false;
       for (int i = kLockLevelCount - 1; i >= 0; --i) {
         // We expect no locks except the mutator_lock_ or thread list suspend thread lock.
-        if (i != kMutatorLock && i != kThreadListSuspendThreadLock) {
+        if (i != kMutatorLock) {
           BaseMutex* held_mutex = GetHeldMutex(static_cast<LockLevel>(i));
           if (held_mutex != NULL) {
             LOG(ERROR) << "holding \"" << held_mutex->GetName()
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index 675ce9a..5ff90d6 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -530,6 +530,12 @@ Thread* ThreadList::SuspendThreadByPeer(jobject peer, bool request_suspension,
       {
         MutexLock suspend_count_mu(self, *Locks::thread_suspend_count_lock_);
         if (request_suspension) {
+          if (self->GetSuspendCount() > 0) {
+            // We hold the suspend count lock but another thread is trying to suspend us. Its not
+            // safe to try to suspend another thread in case we get a cycle. Start the loop again
+            // which will allow this thread to be suspended.
+            continue;
+          }
           thread->ModifySuspendCount(self, +1, debug_suspension);
           request_suspension = false;
           did_suspend_request = true;
@@ -608,6 +614,12 @@ Thread* ThreadList::SuspendThreadByThreadId(uint32_t thread_id, bool debug_suspe
       {
         MutexLock suspend_count_mu(self, *Locks::thread_suspend_count_lock_);
         if (suspended_thread == nullptr) {
+          if (self->GetSuspendCount() > 0) {
+            // We hold the suspend count lock but another thread is trying to suspend us. Its not
+            // safe to try to suspend another thread in case we get a cycle. Start the loop again
+            // which will allow this thread to be suspended.
+            continue;
+          }
           thread->ModifySuspendCount(self, +1, debug_suspension);
           suspended_thread = thread;
         } else {
diff --git a/runtime/thread_list.h b/runtime/thread_list.h
index a7f2c53..13684c7 100644
--- a/runtime/thread_list.h
+++ b/runtime/thread_list.h
@@ -68,7 +68,6 @@ class ThreadList {
   // is set to true.
   Thread* SuspendThreadByPeer(jobject peer, bool request_suspension, bool debug_suspension,
                               bool* timed_out)
-      EXCLUSIVE_LOCKS_REQUIRED(Locks::thread_list_suspend_thread_lock_)
       LOCKS_EXCLUDED(Locks::mutator_lock_,
                      Locks::thread_list_lock_,
                      Locks::thread_suspend_count_lock_);
@@ -78,7 +77,6 @@ class ThreadList {
   // the thread terminating. Note that as thread ids are recycled this may not suspend the expected
   // thread, that may be terminating. If the suspension times out then *timeout is set to true.
   Thread* SuspendThreadByThreadId(uint32_t thread_id, bool debug_suspension, bool* timed_out)
-      EXCLUSIVE_LOCKS_REQUIRED(Locks::thread_list_suspend_thread_lock_)
       LOCKS_EXCLUDED(Locks::mutator_lock_,
                      Locks::thread_list_lock_,
                      Locks::thread_suspend_count_lock_);
diff --git a/test/Android.run-test.mk b/test/Android.run-test.mk
index 3c6c058..a6f31b4 100644
--- a/test/Android.run-test.mk
+++ b/test/Android.run-test.mk
@@ -177,15 +177,6 @@ endif
 
 TEST_ART_TIMING_SENSITIVE_RUN_TESTS :=
 
-TEST_ART_BROKEN_RUN_TESTS := \
-  004-ThreadStress
-
-ART_TEST_KNOWN_BROKEN += $(call all-run-test-names,$(TARGET_TYPES),$(RUN_TYPES),$(PREBUILD_TYPES), \
-      $(COMPILER_TYPES),$(RELOCATE_TYPES),$(TRACE_TYPES),$(GC_TYPES),$(JNI_TYPES), \
-      $(IMAGE_TYPES), $(PICTEST_TYPES), $(TEST_ART_BROKEN_RUN_TESTS), $(ALL_ADDRESS_SIZES))
-
-TEST_ART_BROKEN_RUN_TESTS :=
-
 # Note 116-nodex2oat is not broken per-se it just doesn't (and isn't meant to) work with --prebuild.
 TEST_ART_BROKEN_PREBUILD_RUN_TESTS := \
   116-nodex2oat