For JDWP, suspend thread before configuring it for single stepping.
Change-Id: I05a7c28c9e977885195797a78a492aa0f72801b7
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;
diff --git a/src/jdwp/jdwp_event.cc b/src/jdwp/jdwp_event.cc
index 5b65aa4..56ba131 100644
--- a/src/jdwp/jdwp_event.cc
+++ b/src/jdwp/jdwp_event.cc
@@ -143,8 +143,6 @@
* not be added to the list, and an appropriate error will be returned.
*/
JdwpError JdwpState::RegisterEvent(JdwpEvent* pEvent) {
- MutexLock mu(Thread::Current(), event_list_lock_);
-
CHECK(pEvent != NULL);
CHECK(pEvent->prev == NULL);
CHECK(pEvent->next == NULL);
@@ -175,6 +173,7 @@
/*
* Add to list.
*/
+ MutexLock mu(Thread::Current(), event_list_lock_);
if (event_list_ != NULL) {
pEvent->next = event_list_;
event_list_->prev = pEvent;
diff --git a/src/jdwp/object_registry.cc b/src/jdwp/object_registry.cc
index 1e21ed0..54e7a8e 100644
--- a/src/jdwp/object_registry.cc
+++ b/src/jdwp/object_registry.cc
@@ -117,6 +117,15 @@
return self->DecodeJObject(entry.jni_reference);
}
+jobject ObjectRegistry::GetJObject(JDWP::ObjectId id) {
+ Thread* self = Thread::Current();
+ MutexLock mu(self, lock_);
+ id_iterator it = id_to_entry_.find(id);
+ CHECK(it != id_to_entry_.end()) << id;
+ ObjectRegistryEntry& entry = *(it->second);
+ return entry.jni_reference;
+}
+
void ObjectRegistry::DisableCollection(JDWP::ObjectId id) {
Thread* self = Thread::Current();
MutexLock mu(self, lock_);
diff --git a/src/jdwp/object_registry.h b/src/jdwp/object_registry.h
index e2893ca..734bb86 100644
--- a/src/jdwp/object_registry.h
+++ b/src/jdwp/object_registry.h
@@ -76,6 +76,10 @@
// Returned by Get when passed an invalid object id.
static mirror::Object* const kInvalidObject;
+ // This is needed to get the jobject of a thread instead of the Object*.
+ // Avoid using this and use standard Get when possible.
+ jobject GetJObject(JDWP::ObjectId id) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+
private:
JDWP::ObjectId InternalAdd(mirror::Object* o) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
mirror::Object* InternalGet(JDWP::ObjectId id) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);