Fix thread suspension assertion in debugger.

Updates Dbg::GetThreadGroup to ensure we do call EndAssertNoThreadSuspension
after StartAssertNoThreadSuspension and not returning in the middle. Note this
only happens in debug mode where this assertion is enabled.

Also add missing thread safety annotations.

Bug: 13425576
Change-Id: Idb9f32d289038b77771369c1283774676ff433c7

diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index 7e2dfd2..ce3efa3 100644 (file)
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -222,7 +222,8 @@ static bool IsBreakpoint(const mirror::ArtMethod* m, uint32_t dex_pc)
   return false;
 }
 
-static bool IsSuspendedForDebugger(ScopedObjectAccessUnchecked& soa, Thread* thread) {
+static bool IsSuspendedForDebugger(ScopedObjectAccessUnchecked& soa, Thread* thread)
+    LOCKS_EXCLUDED(Locks::thread_suspend_count_lock_) {
   MutexLock mu(soa.Self(), *Locks::thread_suspend_count_lock_);
   // A thread may be suspended for GC; in this code, we really want to know whether
   // there's a debugger suspension active.
@@ -743,8 +744,7 @@ JDWP::JdwpError Dbg::GetMonitorInfo(JDWP::ObjectId object_id, JDWP::ExpandBuf* r
 
 JDWP::JdwpError Dbg::GetOwnedMonitors(JDWP::ObjectId thread_id,
                                       std::vector<JDWP::ObjectId>& monitors,
-                                      std::vector<uint32_t>& stack_depths)
-    SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+                                      std::vector<uint32_t>& stack_depths) {
   ScopedObjectAccessUnchecked soa(Thread::Current());
   MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
   Thread* thread;
@@ -793,8 +793,8 @@ JDWP::JdwpError Dbg::GetOwnedMonitors(JDWP::ObjectId thread_id,
   return JDWP::ERR_NONE;
 }
 
-JDWP::JdwpError Dbg::GetContendedMonitor(JDWP::ObjectId thread_id, JDWP::ObjectId& contended_monitor)
-    SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+JDWP::JdwpError Dbg::GetContendedMonitor(JDWP::ObjectId thread_id,
+                                         JDWP::ObjectId& contended_monitor) {
   ScopedObjectAccessUnchecked soa(Thread::Current());
   MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
   Thread* thread;
@@ -1705,22 +1705,19 @@ JDWP::JdwpError Dbg::GetThreadGroup(JDWP::ObjectId thread_id, JDWP::ExpandBuf* p
   if (error == JDWP::ERR_THREAD_NOT_ALIVE) {
     // Zombie threads are in the null group.
     expandBufAddObjectId(pReply, JDWP::ObjectId(0));
-    return JDWP::ERR_NONE;
-  }
-  if (error != JDWP::ERR_NONE) {
-    return error;
+    error = JDWP::ERR_NONE;
+  } else if (error == JDWP::ERR_NONE) {
+    mirror::Class* c = soa.Decode<mirror::Class*>(WellKnownClasses::java_lang_Thread);
+    CHECK(c != nullptr);
+    mirror::ArtField* f = c->FindInstanceField("group", "Ljava/lang/ThreadGroup;");
+    CHECK(f != NULL);
+    mirror::Object* group = f->GetObject(thread_object);
+    CHECK(group != NULL);
+    JDWP::ObjectId thread_group_id = gRegistry->Add(group);
+    expandBufAddObjectId(pReply, thread_group_id);
   }
-  mirror::Class* c = soa.Decode<mirror::Class*>(WellKnownClasses::java_lang_Thread);
-  CHECK(c != nullptr);
-  mirror::ArtField* f = c->FindInstanceField("group", "Ljava/lang/ThreadGroup;");
-  CHECK(f != NULL);
-  mirror::Object* group = f->GetObject(thread_object);
-  CHECK(group != NULL);
-  JDWP::ObjectId thread_group_id = gRegistry->Add(group);
   soa.Self()->EndAssertNoThreadSuspension(old_cause);
-
-  expandBufAddObjectId(pReply, thread_group_id);
-  return JDWP::ERR_NONE;
+  return error;
 }
 
 std::string Dbg::GetThreadGroupName(JDWP::ObjectId thread_group_id) {
@@ -1798,7 +1795,8 @@ JDWP::JdwpThreadStatus Dbg::ToJdwpThreadStatus(ThreadState state) {
   return JDWP::TS_ZOMBIE;
 }
 
-JDWP::JdwpError Dbg::GetThreadStatus(JDWP::ObjectId thread_id, JDWP::JdwpThreadStatus* pThreadStatus, JDWP::JdwpSuspendStatus* pSuspendStatus) {
+JDWP::JdwpError Dbg::GetThreadStatus(JDWP::ObjectId thread_id, JDWP::JdwpThreadStatus* pThreadStatus,
+                                     JDWP::JdwpSuspendStatus* pSuspendStatus) {
   ScopedObjectAccess soa(Thread::Current());
 
   *pSuspendStatus = JDWP::SUSPEND_STATUS_NOT_SUSPENDED;
@@ -2607,6 +2605,7 @@ void Dbg::UnwatchLocation(const JDWP::JdwpLocation* location) {
 class ScopedThreadSuspension {
  public:
   ScopedThreadSuspension(Thread* self, JDWP::ObjectId thread_id)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) :
       thread_(NULL),
       error_(JDWP::ERR_NONE),

diff --git a/runtime/debugger.h b/runtime/debugger.h
index 6c44bde..6610347 100644 (file)
--- a/runtime/debugger.h
+++ b/runtime/debugger.h
@@ -220,8 +220,11 @@ class Dbg {
   static JDWP::JdwpError GetOwnedMonitors(JDWP::ObjectId thread_id,
                                           std::vector<JDWP::ObjectId>& monitors,
                                           std::vector<uint32_t>& stack_depths)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
-  static JDWP::JdwpError GetContendedMonitor(JDWP::ObjectId thread_id, JDWP::ObjectId& contended_monitor)
+  static JDWP::JdwpError GetContendedMonitor(JDWP::ObjectId thread_id,
+                                             JDWP::ObjectId& contended_monitor)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
   //
@@ -301,7 +304,8 @@ class Dbg {
   static JDWP::JdwpError GetThreadName(JDWP::ObjectId thread_id, std::string& name)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
       LOCKS_EXCLUDED(Locks::thread_list_lock_);
-  static JDWP::JdwpError GetThreadGroup(JDWP::ObjectId thread_id, JDWP::ExpandBuf* pReply);
+  static JDWP::JdwpError GetThreadGroup(JDWP::ObjectId thread_id, JDWP::ExpandBuf* pReply)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_);
   static std::string GetThreadGroupName(JDWP::ObjectId thread_group_id);
   static JDWP::ObjectId GetThreadGroupParent(JDWP::ObjectId thread_group_id)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
@@ -310,8 +314,14 @@ class Dbg {
   static JDWP::ObjectId GetMainThreadGroupId();
 
   static JDWP::JdwpThreadStatus ToJdwpThreadStatus(ThreadState state);
-  static JDWP::JdwpError GetThreadStatus(JDWP::ObjectId thread_id, JDWP::JdwpThreadStatus* pThreadStatus, JDWP::JdwpSuspendStatus* pSuspendStatus);
-  static JDWP::JdwpError GetThreadDebugSuspendCount(JDWP::ObjectId thread_id, JDWP::ExpandBuf* pReply);
+  static JDWP::JdwpError GetThreadStatus(JDWP::ObjectId thread_id,
+                                         JDWP::JdwpThreadStatus* pThreadStatus,
+                                         JDWP::JdwpSuspendStatus* pSuspendStatus)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_);
+  static JDWP::JdwpError GetThreadDebugSuspendCount(JDWP::ObjectId thread_id,
+                                                    JDWP::ExpandBuf* pReply)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_,
+                     Locks::thread_suspend_count_lock_);
   // static void WaitForSuspend(JDWP::ObjectId thread_id);
 
   // Fills 'thread_ids' with the threads in the given thread group. If thread_group_id == 0,
@@ -321,9 +331,11 @@ class Dbg {
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
   static void GetChildThreadGroups(JDWP::ObjectId thread_group_id, std::vector<JDWP::ObjectId>& child_thread_group_ids);
 
-  static JDWP::JdwpError GetThreadFrameCount(JDWP::ObjectId thread_id, size_t& result);
+  static JDWP::JdwpError GetThreadFrameCount(JDWP::ObjectId thread_id, size_t& result)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_);
   static JDWP::JdwpError GetThreadFrames(JDWP::ObjectId thread_id, size_t start_frame,
                                          size_t frame_count, JDWP::ExpandBuf* buf)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
   static JDWP::ObjectId GetThreadSelfId()
@@ -350,12 +362,15 @@ class Dbg {
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
   static void GetLocalValue(JDWP::ObjectId thread_id, JDWP::FrameId frame_id, int slot,
                             JDWP::JdwpTag tag, uint8_t* buf, size_t expectedLen)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
   static void SetLocalValue(JDWP::ObjectId thread_id, JDWP::FrameId frame_id, int slot,
                             JDWP::JdwpTag tag, uint64_t value, size_t width)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
-  static JDWP::JdwpError Interrupt(JDWP::ObjectId thread_id);
+  static JDWP::JdwpError Interrupt(JDWP::ObjectId thread_id)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_);
 
   /*
    * Debugger notification
@@ -413,6 +428,7 @@ class Dbg {
                                        JDWP::JdwpStepDepth depth)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
   static void UnconfigureStep(JDWP::ObjectId thread_id)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
   static JDWP::JdwpError InvokeMethod(JDWP::ObjectId thread_id, JDWP::ObjectId object_id,
@@ -431,7 +447,8 @@ class Dbg {
    */
   static void DdmSendThreadNotification(Thread* t, uint32_t type)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
-  static void DdmSetThreadNotification(bool enable);
+  static void DdmSetThreadNotification(bool enable)
+      LOCKS_EXCLUDED(Locks::thread_list_lock_);
   static bool DdmHandlePacket(JDWP::Request& request, uint8_t** pReplyBuf, int* pReplyLen);
   static void DdmConnected() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
   static void DdmDisconnected() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
@@ -446,17 +463,21 @@ class Dbg {
    * Recent allocation tracking support.
    */
   static void RecordAllocation(mirror::Class* type, size_t byte_count)
+      LOCKS_EXCLUDED(alloc_tracker_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
-  static void SetAllocTrackingEnabled(bool enabled);
+  static void SetAllocTrackingEnabled(bool enabled) LOCKS_EXCLUDED(alloc_tracker_lock_);
   static bool IsAllocTrackingEnabled() {
     return recent_allocation_records_ != nullptr;
   }
-  static jbyteArray GetRecentAllocations() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+  static jbyteArray GetRecentAllocations()
+      LOCKS_EXCLUDED(alloc_tracker_lock_)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
   static size_t HeadIndex() EXCLUSIVE_LOCKS_REQUIRED(alloc_tracker_lock_);
-  static void DumpRecentAllocations();
+  static void DumpRecentAllocations() LOCKS_EXCLUDED(alloc_tracker_lock_);
 
   // Updates the stored direct object pointers (called from SweepSystemWeaks).
   static void UpdateObjectPointers(IsMarkedCallback* callback, void* arg)
+      LOCKS_EXCLUDED(alloc_tracker_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
   enum HpifWhen {