commit 449db33fafa29578df60e8a323f78d5eb6247e76
author Jeff Hao <jeffhao@google.com> Fri Apr 12 18:30:52 2013 -0700
committer Jeff Hao <jeffhao@google.com> Fri Apr 12 18:35:01 2013 -0700
tree a868b59dc78593b140d65239b087b77f59bdebed
parent ed1790e83352e54420018d1495eb010f7cd48a64

For JDWP, suspend thread before configuring it for single stepping.

Change-Id: I05a7c28c9e977885195797a78a492aa0f72801b7

src/debugger.cc

diff --git a/src/debugger.cc b/src/debugger.cc
index 0712c93..64be25c 100644
--- a/src/debugger.cc
+++ b/src/debugger.cc
@@ -2374,22 +2374,75 @@
   }
 }
 
-JDWP::JdwpError Dbg::ConfigureStep(JDWP::ObjectId thread_id, JDWP::JdwpStepSize step_size,
-                                   JDWP::JdwpStepDepth step_depth) {
-  ScopedObjectAccessUnchecked soa(Thread::Current());
-  MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
-  Thread* thread;
-  JDWP::JdwpError error = DecodeThread(soa, thread_id, thread);
-  if (error != JDWP::ERR_NONE) {
-    return error;
+// Scoped utility class to suspend a thread so that we may do tasks such as walk its stack. Doesn't
+// cause suspension if the thread is the current thread.
+class ScopedThreadSuspension {
+ public:
+  ScopedThreadSuspension(Thread* self, JDWP::ObjectId thread_id) :
+      thread_(NULL),
+      error_(JDWP::ERR_NONE),
+      self_suspend_(false),
+      other_suspend_(false) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+    ScopedObjectAccessUnchecked soa(self);
+    {
+      MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
+      error_ = DecodeThread(soa, thread_id, thread_);
+    }
+    if (error_ == JDWP::ERR_NONE) {
+      if (thread_ == soa.Self()) {
+        self_suspend_ = true;
+      } else {
+        soa.Self()->TransitionFromRunnableToSuspended(kWaitingForDebuggerSuspension);
+        jobject thread_peer = gRegistry->GetJObject(thread_id);
+        bool timed_out;
+        Thread* suspended_thread = Thread::SuspendForDebugger(thread_peer, true, &timed_out);
+        CHECK_EQ(soa.Self()->TransitionFromSuspendedToRunnable(), kWaitingForDebuggerSuspension);
+        if (suspended_thread == NULL) {
+          // Thread terminated from under us while suspending.
+          error_ = JDWP::ERR_INVALID_THREAD;
+        } else {
+          CHECK_EQ(suspended_thread, thread_);
+          other_suspend_ = true;
+        }
+      }
+    }
   }
 
-  MutexLock mu2(soa.Self(), *Locks::breakpoint_lock_);
+  Thread* GetThread() const {
+    return thread_;
+  }
+
+  JDWP::JdwpError GetError() const {
+    return error_;
+  }
+
+  ~ScopedThreadSuspension() {
+    if (other_suspend_) {
+      Runtime::Current()->GetThreadList()->Resume(thread_, true);
+    }
+  }
+
+ private:
+  Thread* thread_;
+  JDWP::JdwpError error_;
+  bool self_suspend_;
+  bool other_suspend_;
+};
+
+JDWP::JdwpError Dbg::ConfigureStep(JDWP::ObjectId thread_id, JDWP::JdwpStepSize step_size,
+                                   JDWP::JdwpStepDepth step_depth) {
+  Thread* self = Thread::Current();
+  ScopedThreadSuspension sts(self, thread_id);
+  if (sts.GetError() != JDWP::ERR_NONE) {
+    return sts.GetError();
+  }
+
+  MutexLock mu2(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) {
+  if (gSingleStepControl.thread != NULL && sts.GetThread() != gSingleStepControl.thread) {
     LOG(WARNING) << "single-step already active for " << *gSingleStepControl.thread
-                 << "; switching to " << *thread;
+                 << "; switching to " << *sts.GetThread();
   }
 
   //
@@ -2426,7 +2479,8 @@
       return true;
     }
   };
-  SingleStepStackVisitor visitor(thread);
+
+  SingleStepStackVisitor visitor(sts.GetThread());
   visitor.WalkStack();
 
   //
@@ -2493,7 +2547,7 @@
   // Everything else...
   //
 
-  gSingleStepControl.thread = thread;
+  gSingleStepControl.thread = sts.GetThread();
   gSingleStepControl.step_size = step_size;
   gSingleStepControl.step_depth = step_depth;
   gSingleStepControl.is_active = true;