Merge "Some fixes for JDWP with the interpreter." into dalvik-dev

4 files changed, 97 insertions, 83 deletions

diff --git a/src/debugger.cc b/src/debugger.cc
index 87e9c72a3b..5a9faa6b90 100644
--- a/src/debugger.cc
+++ b/src/debugger.cc
@@ -177,13 +177,13 @@ static size_t gAllocRecordHead GUARDED_BY(gAllocTrackerLock) = 0;
 static size_t gAllocRecordCount GUARDED_BY(gAllocTrackerLock) = 0;
 
 // Breakpoints and single-stepping.
-static Mutex gBreakpointsLock DEFAULT_MUTEX_ACQUIRED_AFTER ("breakpoints lock");
-static std::vector<Breakpoint> gBreakpoints GUARDED_BY(gBreakpointsLock);
-static SingleStepControl gSingleStepControl GUARDED_BY(gBreakpointsLock);
+static std::vector<Breakpoint> gBreakpoints GUARDED_BY(Locks::breakpoint_lock_);
+static SingleStepControl gSingleStepControl GUARDED_BY(Locks::breakpoint_lock_);
 
 static bool IsBreakpoint(AbstractMethod* m, uint32_t dex_pc)
+    LOCKS_EXCLUDED(Locks::breakpoint_lock_)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
-  MutexLock mu(Thread::Current(), gBreakpointsLock);
+  MutexLock mu(Thread::Current(), *Locks::breakpoint_lock_);
   for (size_t i = 0; i < gBreakpoints.size(); ++i) {
     if (gBreakpoints[i].method == m && gBreakpoints[i].dex_pc == dex_pc) {
       VLOG(jdwp) << "Hit breakpoint #" << i << ": " << gBreakpoints[i];
@@ -508,7 +508,7 @@ void Dbg::GoActive() {
 
   {
     // TODO: dalvik only warned if there were breakpoints left over. clear in Dbg::Disconnected?
-    MutexLock mu(Thread::Current(), gBreakpointsLock);
+    MutexLock mu(Thread::Current(), *Locks::breakpoint_lock_);
     CHECK_EQ(gBreakpoints.size(), 0U);
   }
 
@@ -1572,7 +1572,7 @@ static int GetStackDepth(Thread* thread)
 
   if (kIsDebugBuild) {
     MutexLock mu(Thread::Current(), *Locks::thread_suspend_count_lock_);
-    CHECK(thread->IsSuspended());
+    CHECK(thread == Thread::Current() || thread->IsSuspended());
   }
   CountStackDepthVisitor visitor(thread->GetManagedStack(), thread->GetInstrumentationStack());
   visitor.WalkStack();
@@ -2065,63 +2065,63 @@ void Dbg::UpdateDebugger(int32_t dex_pc, Thread* self) {
     event_flags |= kBreakpoint;
   }
 
-  // If the debugger is single-stepping one of our threads, check to
-  // see if we're that thread and we've reached a step point.
-  MutexLock mu(Thread::Current(), gBreakpointsLock);
-  if (gSingleStepControl.is_active && gSingleStepControl.thread == self) {
-    CHECK(!m->IsNative());
-    if (gSingleStepControl.step_depth == JDWP::SD_INTO) {
-      // Step into method calls.  We break when the line number
-      // or method pointer changes.  If we're in SS_MIN mode, we
-      // always stop.
-      if (gSingleStepControl.method != m) {
-        event_flags |= kSingleStep;
-        VLOG(jdwp) << "SS new method";
-      } else if (gSingleStepControl.step_size == JDWP::SS_MIN) {
-        event_flags |= kSingleStep;
-        VLOG(jdwp) << "SS new instruction";
-      } else if (gSingleStepControl.dex_pcs.find(dex_pc) == gSingleStepControl.dex_pcs.end()) {
-        event_flags |= kSingleStep;
-        VLOG(jdwp) << "SS new line";
-      }
-    } else if (gSingleStepControl.step_depth == JDWP::SD_OVER) {
-      // Step over method calls.  We break when the line number is
-      // different and the frame depth is <= the original frame
-      // depth.  (We can't just compare on the method, because we
-      // might get unrolled past it by an exception, and it's tricky
-      // to identify recursion.)
-
-      // TODO: can we just use the value of 'sp'?
-      int stack_depth = GetStackDepth(self);
-
-      if (stack_depth < gSingleStepControl.stack_depth) {
-        // popped up one or more frames, always trigger
-        event_flags |= kSingleStep;
-        VLOG(jdwp) << "SS method pop";
-      } else if (stack_depth == gSingleStepControl.stack_depth) {
-        // same depth, see if we moved
-        if (gSingleStepControl.step_size == JDWP::SS_MIN) {
+  {
+    // If the debugger is single-stepping one of our threads, check to
+    // see if we're that thread and we've reached a step point.
+    MutexLock mu(Thread::Current(), *Locks::breakpoint_lock_);
+    if (gSingleStepControl.is_active && gSingleStepControl.thread == self) {
+      CHECK(!m->IsNative());
+      if (gSingleStepControl.step_depth == JDWP::SD_INTO) {
+        // Step into method calls.  We break when the line number
+        // or method pointer changes.  If we're in SS_MIN mode, we
+        // always stop.
+        if (gSingleStepControl.method != m) {
+          event_flags |= kSingleStep;
+          VLOG(jdwp) << "SS new method";
+        } else if (gSingleStepControl.step_size == JDWP::SS_MIN) {
           event_flags |= kSingleStep;
           VLOG(jdwp) << "SS new instruction";
         } else if (gSingleStepControl.dex_pcs.find(dex_pc) == gSingleStepControl.dex_pcs.end()) {
           event_flags |= kSingleStep;
           VLOG(jdwp) << "SS new line";
         }
-      }
-    } else {
-      CHECK_EQ(gSingleStepControl.step_depth, JDWP::SD_OUT);
-      // Return from the current method.  We break when the frame
-      // depth pops up.
-
-      // This differs from the "method exit" break in that it stops
-      // with the PC at the next instruction in the returned-to
-      // function, rather than the end of the returning function.
-
-      // TODO: can we just use the value of 'sp'?
-      int stack_depth = GetStackDepth(self);
-      if (stack_depth < gSingleStepControl.stack_depth) {
-        event_flags |= kSingleStep;
-        VLOG(jdwp) << "SS method pop";
+      } else if (gSingleStepControl.step_depth == JDWP::SD_OVER) {
+        // Step over method calls.  We break when the line number is
+        // different and the frame depth is <= the original frame
+        // depth.  (We can't just compare on the method, because we
+        // might get unrolled past it by an exception, and it's tricky
+        // to identify recursion.)
+
+        int stack_depth = GetStackDepth(self);
+
+        if (stack_depth < gSingleStepControl.stack_depth) {
+          // popped up one or more frames, always trigger
+          event_flags |= kSingleStep;
+          VLOG(jdwp) << "SS method pop";
+        } else if (stack_depth == gSingleStepControl.stack_depth) {
+          // same depth, see if we moved
+          if (gSingleStepControl.step_size == JDWP::SS_MIN) {
+            event_flags |= kSingleStep;
+            VLOG(jdwp) << "SS new instruction";
+          } else if (gSingleStepControl.dex_pcs.find(dex_pc) == gSingleStepControl.dex_pcs.end()) {
+            event_flags |= kSingleStep;
+            VLOG(jdwp) << "SS new line";
+          }
+        }
+      } else {
+        CHECK_EQ(gSingleStepControl.step_depth, JDWP::SD_OUT);
+        // Return from the current method.  We break when the frame
+        // depth pops up.
+
+        // This differs from the "method exit" break in that it stops
+        // with the PC at the next instruction in the returned-to
+        // function, rather than the end of the returning function.
+
+        int stack_depth = GetStackDepth(self);
+        if (stack_depth < gSingleStepControl.stack_depth) {
+          event_flags |= kSingleStep;
+          VLOG(jdwp) << "SS method pop";
+        }
       }
     }
   }
@@ -2151,14 +2151,14 @@ void Dbg::UpdateDebugger(int32_t dex_pc, Thread* self) {
 }
 
 void Dbg::WatchLocation(const JDWP::JdwpLocation* location) {
-  MutexLock mu(Thread::Current(), gBreakpointsLock);
+  MutexLock mu(Thread::Current(), *Locks::breakpoint_lock_);
   AbstractMethod* m = FromMethodId(location->method_id);
   gBreakpoints.push_back(Breakpoint(m, location->dex_pc));
   VLOG(jdwp) << "Set breakpoint #" << (gBreakpoints.size() - 1) << ": " << gBreakpoints[gBreakpoints.size() - 1];
 }
 
 void Dbg::UnwatchLocation(const JDWP::JdwpLocation* location) {
-  MutexLock mu(Thread::Current(), gBreakpointsLock);
+  MutexLock mu(Thread::Current(), *Locks::breakpoint_lock_);
   AbstractMethod* m = FromMethodId(location->method_id);
   for (size_t i = 0; i < gBreakpoints.size(); ++i) {
     if (gBreakpoints[i].method == m && gBreakpoints[i].dex_pc == location->dex_pc) {
@@ -2178,7 +2178,7 @@ JDWP::JdwpError Dbg::ConfigureStep(JDWP::ObjectId threadId, JDWP::JdwpStepSize s
     return JDWP::ERR_INVALID_THREAD;
   }
 
-  MutexLock mu2(soa.Self(), gBreakpointsLock);
+  MutexLock mu2(soa.Self(), *Locks::breakpoint_lock_);
   // TODO: there's no theoretical reason why we couldn't support single-stepping
   // of multiple threads at once, but we never did so historically.
   if (gSingleStepControl.thread != NULL && thread != gSingleStepControl.thread) {
@@ -2194,10 +2194,9 @@ JDWP::JdwpError Dbg::ConfigureStep(JDWP::ObjectId threadId, JDWP::JdwpStepSize s
   struct SingleStepStackVisitor : public StackVisitor {
     SingleStepStackVisitor(const ManagedStack* stack,
                            const std::vector<InstrumentationStackFrame>* instrumentation_stack)
-        EXCLUSIVE_LOCKS_REQUIRED(gBreakpointsLock)
+        EXCLUSIVE_LOCKS_REQUIRED(Locks::breakpoint_lock_)
         SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
         : StackVisitor(stack, instrumentation_stack, NULL) {
-      gBreakpointsLock.AssertHeld(Thread::Current());
       gSingleStepControl.method = NULL;
       gSingleStepControl.stack_depth = 0;
     }
@@ -2205,7 +2204,7 @@ JDWP::JdwpError Dbg::ConfigureStep(JDWP::ObjectId threadId, JDWP::JdwpStepSize s
     // TODO: Enable annotalysis. We know lock is held in constructor, but abstraction confuses
     // annotalysis.
     bool VisitFrame() NO_THREAD_SAFETY_ANALYSIS {
-      gBreakpointsLock.AssertHeld(Thread::Current());
+      Locks::breakpoint_lock_->AssertHeld(Thread::Current());
       const AbstractMethod* m = GetMethod();
       if (!m->IsRuntimeMethod()) {
         ++gSingleStepControl.stack_depth;
@@ -2230,13 +2229,15 @@ JDWP::JdwpError Dbg::ConfigureStep(JDWP::ObjectId threadId, JDWP::JdwpStepSize s
   //
 
   struct DebugCallbackContext {
-    DebugCallbackContext() {
+    DebugCallbackContext() EXCLUSIVE_LOCKS_REQUIRED(Locks::breakpoint_lock_) {
       last_pc_valid = false;
       last_pc = 0;
     }
 
-    static bool Callback(void* raw_context, uint32_t address, uint32_t line_number) {
-      MutexLock mu(Thread::Current(), gBreakpointsLock); // Keep GCC happy.
+    // TODO: Enable annotalysis. We know lock is held in constructor, but abstraction confuses
+    // annotalysis.
+    static bool Callback(void* raw_context, uint32_t address, uint32_t line_number) NO_THREAD_SAFETY_ANALYSIS {
+      Locks::breakpoint_lock_->AssertHeld(Thread::Current());
       DebugCallbackContext* context = reinterpret_cast<DebugCallbackContext*>(raw_context);
       if (static_cast<int32_t>(line_number) == gSingleStepControl.line_number) {
         if (!context->last_pc_valid) {
@@ -2256,8 +2257,10 @@ JDWP::JdwpError Dbg::ConfigureStep(JDWP::ObjectId threadId, JDWP::JdwpStepSize s
       return false; // There may be multiple entries for any given line.
     }
 
-    ~DebugCallbackContext() {
-      MutexLock mu(Thread::Current(), gBreakpointsLock); // Keep GCC happy.
+    // TODO: Enable annotalysis. We know lock is held in constructor, but abstraction confuses
+    // annotalysis.
+    ~DebugCallbackContext() NO_THREAD_SAFETY_ANALYSIS {
+      Locks::breakpoint_lock_->AssertHeld(Thread::Current());
       // If the line number was the last in the position table...
       if (last_pc_valid) {
         size_t end = MethodHelper(gSingleStepControl.method).GetCodeItem()->insns_size_in_code_units_;
@@ -2307,7 +2310,7 @@ JDWP::JdwpError Dbg::ConfigureStep(JDWP::ObjectId threadId, JDWP::JdwpStepSize s
 }
 
 void Dbg::UnconfigureStep(JDWP::ObjectId /*threadId*/) {
-  MutexLock mu(Thread::Current(), gBreakpointsLock);
+  MutexLock mu(Thread::Current(), *Locks::breakpoint_lock_);
 
   gSingleStepControl.is_active = false;
   gSingleStepControl.thread = NULL;
diff --git a/src/debugger.h b/src/debugger.h
index f9d00d1bbb..d7d280068e 100644
--- a/src/debugger.h
+++ b/src/debugger.h
@@ -94,7 +94,7 @@ class Dbg {
    * when the debugger attaches.
    */
   static void Connected();
-  static void GoActive();
+  static void GoActive() LOCKS_EXCLUDED(Locks::breakpoint_lock_);
   static void Disconnected();
   static void Disposed();
 
@@ -295,17 +295,20 @@ class Dbg {
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
   static void UpdateDebugger(int32_t dex_pc, Thread* self)
+      LOCKS_EXCLUDED(Locks::breakpoint_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
   static void WatchLocation(const JDWP::JdwpLocation* pLoc)
+      LOCKS_EXCLUDED(Locks::breakpoint_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
   static void UnwatchLocation(const JDWP::JdwpLocation* pLoc)
+      LOCKS_EXCLUDED(Locks::breakpoint_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
   static JDWP::JdwpError ConfigureStep(JDWP::ObjectId threadId, JDWP::JdwpStepSize size,
                                        JDWP::JdwpStepDepth depth)
-      LOCKS_EXCLUDED(gBreakpointsLock)
+      LOCKS_EXCLUDED(Locks::breakpoint_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
-  static void UnconfigureStep(JDWP::ObjectId threadId);
+  static void UnconfigureStep(JDWP::ObjectId threadId) LOCKS_EXCLUDED(Locks::breakpoint_lock_);
 
   static JDWP::JdwpError InvokeMethod(JDWP::ObjectId threadId, JDWP::ObjectId objectId,
                                       JDWP::RefTypeId classId, JDWP::MethodId methodId,
diff --git a/src/locks.cc b/src/locks.cc
index 312b021d89..0422affddc 100644
--- a/src/locks.cc
+++ b/src/locks.cc
@@ -21,6 +21,7 @@
 namespace art {
 
 Mutex* Locks::abort_lock_ = NULL;
+Mutex* Locks::breakpoint_lock_ = NULL;
 Mutex* Locks::classlinker_classes_lock_ = NULL;
 ReaderWriterMutex* Locks::heap_bitmap_lock_ = NULL;
 Mutex* Locks::logging_lock_ = NULL;
@@ -34,6 +35,7 @@ void Locks::Init() {
   if (logging_lock_ != NULL) {
     // Already initialized.
     DCHECK(abort_lock_ != NULL);
+    DCHECK(breakpoint_lock_ != NULL);
     DCHECK(classlinker_classes_lock_ != NULL);
     DCHECK(heap_bitmap_lock_ != NULL);
     DCHECK(logging_lock_ != NULL);
@@ -45,6 +47,8 @@ void Locks::Init() {
     logging_lock_ = new Mutex("logging lock", kLoggingLock, true);
     abort_lock_ = new Mutex("abort lock", kAbortLock, true);
 
+    DCHECK(breakpoint_lock_ == NULL);
+    breakpoint_lock_ = new Mutex("breakpoint lock", kBreakpointLock);
     DCHECK(classlinker_classes_lock_ == NULL);
     classlinker_classes_lock_ = new Mutex("ClassLinker classes lock", kClassLinkerClassesLock);
     DCHECK(heap_bitmap_lock_ == NULL);
diff --git a/src/locks.h b/src/locks.h
index 9da77110a2..9a59dca6a2 100644
--- a/src/locks.h
+++ b/src/locks.h
@@ -40,16 +40,17 @@ enum LockLevel {
   kAllocSpaceLock = 5,
   kLoadLibraryLock = 6,
   kClassLinkerClassesLock = 7,
-  kThreadListLock = 8,
-  kJdwpEventListLock = 9,
-  kJdwpAttachLock = 10,
-  kJdwpStartLock = 11,
-  kJdwpSerialLock = 12,
-  kRuntimeShutdownLock = 13,
-  kHeapBitmapLock = 14,
-  kMonitorLock = 15,
-  kMutatorLock = 16,
-  kZygoteCreationLock = 17,
+  kBreakpointLock = 8,
+  kThreadListLock = 9,
+  kJdwpEventListLock = 10,
+  kJdwpAttachLock = 11,
+  kJdwpStartLock = 12,
+  kJdwpSerialLock = 13,
+  kRuntimeShutdownLock = 14,
+  kHeapBitmapLock = 15,
+  kMonitorLock = 16,
+  kMutatorLock = 17,
+  kZygoteCreationLock = 18,
   kMaxMutexLevel = kZygoteCreationLock,
 };
 std::ostream& operator<<(std::ostream& os, const LockLevel& rhs);
@@ -129,8 +130,11 @@ class Locks {
   // attaching and detaching.
   static Mutex* thread_list_lock_ ACQUIRED_AFTER(runtime_shutdown_lock_);
 
+  // Guards breakpoints and single-stepping.
+  static Mutex* breakpoint_lock_ ACQUIRED_AFTER(thread_list_lock_);
+
   // Guards lists of classes within the class linker.
-  static Mutex* classlinker_classes_lock_ ACQUIRED_AFTER(thread_list_lock_);
+  static Mutex* classlinker_classes_lock_ ACQUIRED_AFTER(breakpoint_lock_);
 
   // When declaring any Mutex add DEFAULT_MUTEX_ACQUIRED_AFTER to use annotalysis to check the code
   // doesn't try to hold a higher level Mutex.