Various fixes for JDWP.
- Moved lock of thread list lock into DecodeThread from its callers
- Fixed scope of various locks to prevent locking violations
- Added transition for current thread from runnable to suspended before
suspending vm, and then a transition back
- Reworked lock ordering to allow JDWP locks to be held while grabbing
the thread list lock
- Moved debugger PostException until after suspension is re-allowed
Change-Id: Ie53e47ff1538e6cd3125c48ddb4c13758b29be63
diff --git a/src/debugger.cc b/src/debugger.cc
index 672b660..87e9c72 100644
--- a/src/debugger.cc
+++ b/src/debugger.cc
@@ -224,6 +224,7 @@
}
static Thread* DecodeThread(ScopedObjectAccessUnchecked& soa, JDWP::ObjectId threadId)
+ EXCLUSIVE_LOCKS_REQUIRED(Locks::thread_list_lock_)
LOCKS_EXCLUDED(Locks::thread_suspend_count_lock_)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
Object* thread_peer = gRegistry->Get<Object*>(threadId);
@@ -1334,9 +1335,8 @@
}
bool Dbg::GetThreadName(JDWP::ObjectId threadId, std::string& name) {
- Thread* self = Thread::Current();
- MutexLock mu(self, *Locks::thread_list_lock_);
- ScopedObjectAccessUnchecked soa(self);
+ ScopedObjectAccessUnchecked soa(Thread::Current());
+ MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
Thread* thread = DecodeThread(soa, threadId);
if (thread == NULL) {
return false;
@@ -1581,6 +1581,7 @@
int Dbg::GetThreadFrameCount(JDWP::ObjectId threadId) {
ScopedObjectAccess soa(Thread::Current());
+ MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
return GetStackDepth(DecodeThread(soa, threadId));
}
@@ -1624,6 +1625,7 @@
};
ScopedObjectAccessUnchecked soa(Thread::Current());
+ MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
Thread* thread = DecodeThread(soa, thread_id); // Caller already checked thread is suspended.
GetFrameVisitor visitor(thread->GetManagedStack(), thread->GetInstrumentationStack(), start_frame, frame_count, buf);
visitor.WalkStack();
@@ -1669,8 +1671,11 @@
void Dbg::ResumeThread(JDWP::ObjectId threadId) {
ScopedObjectAccessUnchecked soa(Thread::Current());
Object* peer = gRegistry->Get<Object*>(threadId);
- MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
- Thread* thread = Thread::FromManagedThread(soa, peer);
+ Thread* thread;
+ {
+ MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
+ thread = Thread::FromManagedThread(soa, peer);
+ }
if (thread == NULL) {
LOG(WARNING) << "No such thread for resume: " << peer;
return;
@@ -1878,6 +1883,7 @@
};
ScopedObjectAccessUnchecked soa(Thread::Current());
+ MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
Thread* thread = DecodeThread(soa, threadId);
UniquePtr<Context> context(Context::Create());
GetLocalVisitor visitor(thread->GetManagedStack(), thread->GetInstrumentationStack(), context.get(),
@@ -1961,6 +1967,7 @@
};
ScopedObjectAccessUnchecked soa(Thread::Current());
+ MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
Thread* thread = DecodeThread(soa, threadId);
UniquePtr<Context> context(Context::Create());
SetLocalVisitor visitor(thread->GetManagedStack(), thread->GetInstrumentationStack(), context.get(),
@@ -2165,12 +2172,13 @@
JDWP::JdwpError Dbg::ConfigureStep(JDWP::ObjectId threadId, JDWP::JdwpStepSize step_size,
JDWP::JdwpStepDepth step_depth) {
ScopedObjectAccessUnchecked soa(Thread::Current());
+ MutexLock mu(soa.Self(), *Locks::thread_list_lock_);
Thread* thread = DecodeThread(soa, threadId);
if (thread == NULL) {
return JDWP::ERR_INVALID_THREAD;
}
- MutexLock mu(soa.Self(), gBreakpointsLock);
+ MutexLock mu2(soa.Self(), gBreakpointsLock);
// 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) {
diff --git a/src/jdwp/jdwp.h b/src/jdwp/jdwp.h
index 3186006..fbca7d1 100644
--- a/src/jdwp/jdwp.h
+++ b/src/jdwp/jdwp.h
@@ -286,7 +286,7 @@
explicit JdwpState(const JdwpOptions* options);
bool InvokeInProgress();
bool IsConnected();
- void SuspendByPolicy(JdwpSuspendPolicy suspend_policy, JDWP::ObjectId thread_self_id)
+ void SuspendByPolicy(JdwpSuspendPolicy suspend_policy, JDWP::ObjectId thread_self_id)
LOCKS_EXCLUDED(Locks::mutator_lock_);
void SendRequestAndPossiblySuspend(ExpandBuf* pReq, JdwpSuspendPolicy suspend_policy,
ObjectId threadId)
diff --git a/src/jdwp/jdwp_handler.cc b/src/jdwp/jdwp_handler.cc
index 07e47b5..88677d5 100644
--- a/src/jdwp/jdwp_handler.cc
+++ b/src/jdwp/jdwp_handler.cc
@@ -277,7 +277,10 @@
*/
static JdwpError VM_Suspend(JdwpState*, const uint8_t*, int, ExpandBuf*)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+ Thread* self = Thread::Current();
+ self->TransitionFromRunnableToSuspended(kWaitingForDebuggerSuspension);
Dbg::SuspendVM();
+ self->TransitionFromSuspendedToRunnable();
return ERR_NONE;
}
diff --git a/src/jdwp/jdwp_main.cc b/src/jdwp/jdwp_main.cc
index 0691515..33aadee 100644
--- a/src/jdwp/jdwp_main.cc
+++ b/src/jdwp/jdwp_main.cc
@@ -102,7 +102,7 @@
serial_lock_("JDWP serial lock", kJdwpSerialLock),
request_serial_(0x10000000),
event_serial_(0x20000000),
- event_list_lock_("JDWP event list lock"),
+ event_list_lock_("JDWP event list lock", kJdwpEventListLock),
event_list_(NULL),
event_list_size_(0),
event_thread_lock_("JDWP event thread lock"),
diff --git a/src/locks.h b/src/locks.h
index c009f1d..9da7711 100644
--- a/src/locks.h
+++ b/src/locks.h
@@ -37,18 +37,19 @@
kThreadSuspendCountLock = 2,
kAbortLock = 3,
kDefaultMutexLevel = 4,
- kJdwpAttachLock = 5,
- kJdwpStartLock = 6,
- kJdwpSerialLock = 7,
- kAllocSpaceLock = 8,
- kLoadLibraryLock = 9,
- kClassLinkerClassesLock = 10,
- kThreadListLock = 11,
- kRuntimeShutdownLock = 12,
- kHeapBitmapLock = 13,
- kMonitorLock = 14,
- kMutatorLock = 15,
- kZygoteCreationLock = 16,
+ 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,
kMaxMutexLevel = kZygoteCreationLock,
};
std::ostream& operator<<(std::ostream& os, const LockLevel& rhs);
diff --git a/src/thread.cc b/src/thread.cc
index 7490d2a..f6053a9 100644
--- a/src/thread.cc
+++ b/src/thread.cc
@@ -1725,8 +1725,6 @@
void DoLongJump() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
AbstractMethod* catch_method = *handler_quick_frame_;
- Dbg::PostException(self_, throw_frame_id_, throw_method_, throw_dex_pc_,
- catch_method, handler_dex_pc_, exception_);
if (kDebugExceptionDelivery) {
if (catch_method == NULL) {
LOG(INFO) << "Handler is upcall";
@@ -1738,6 +1736,9 @@
}
self_->SetException(exception_); // Exception back in root set.
self_->EndAssertNoThreadSuspension(last_no_assert_suspension_cause_);
+ // Do debugger PostException after allowing thread suspension again.
+ Dbg::PostException(self_, throw_frame_id_, throw_method_, throw_dex_pc_,
+ catch_method, handler_dex_pc_, exception_);
// Place context back on thread so it will be available when we continue.
self_->ReleaseLongJumpContext(context_);
context_->SetSP(reinterpret_cast<uintptr_t>(handler_quick_frame_));
diff --git a/src/thread.h b/src/thread.h
index 7bd64c8..4d97315 100644
--- a/src/thread.h
+++ b/src/thread.h
@@ -128,9 +128,11 @@
}
static Thread* FromManagedThread(const ScopedObjectAccessUnchecked& ts, Object* thread_peer)
+ EXCLUSIVE_LOCKS_REQUIRED(Locks::thread_list_lock_)
LOCKS_EXCLUDED(Locks::thread_suspend_count_lock_)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
static Thread* FromManagedThread(const ScopedObjectAccessUnchecked& ts, jobject thread)
+ EXCLUSIVE_LOCKS_REQUIRED(Locks::thread_list_lock_)
LOCKS_EXCLUDED(Locks::thread_suspend_count_lock_)
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
diff --git a/src/thread_list.cc b/src/thread_list.cc
index d39d424..3834725 100644
--- a/src/thread_list.cc
+++ b/src/thread_list.cc
@@ -384,16 +384,18 @@
Thread* debug_thread = Dbg::GetDebugThread();
CHECK(debug_thread != NULL);
CHECK(self != debug_thread);
+ CHECK_NE(self->GetState(), kRunnable);
+ Locks::mutator_lock_->AssertNotHeld(self);
- // Collisions with other suspends aren't really interesting. We want
- // to ensure that we're the only one fiddling with the suspend count
- // though.
- MutexLock mu(self, *Locks::thread_suspend_count_lock_);
- self->ModifySuspendCount(self, +1, true);
+ {
+ // Collisions with other suspends aren't really interesting. We want
+ // to ensure that we're the only one fiddling with the suspend count
+ // though.
+ MutexLock mu(self, *Locks::thread_suspend_count_lock_);
+ self->ModifySuspendCount(self, +1, true);
+ CHECK_GT(self->suspend_count_, 0);
+ }
- // Suspend ourselves.
- CHECK_GT(self->suspend_count_, 0);
- self->SetState(kSuspended);
VLOG(threads) << *self << " self-suspending (debugger)";
// Tell JDWP that we've completed suspension. The JDWP thread can't
@@ -401,19 +403,22 @@
// suspend count lock.
Dbg::ClearWaitForEventThread();
- while (self->suspend_count_ != 0) {
- Thread::resume_cond_->Wait(self);
- if (self->suspend_count_ != 0) {
- // The condition was signaled but we're still suspended. This
- // can happen if the debugger lets go while a SIGQUIT thread
- // dump event is pending (assuming SignalCatcher was resumed for
- // just long enough to try to grab the thread-suspend lock).
- LOG(DEBUG) << *self << " still suspended after undo "
- << "(suspend count=" << self->suspend_count_ << ")";
+ {
+ MutexLock mu(self, *Locks::thread_suspend_count_lock_);
+ while (self->suspend_count_ != 0) {
+ Thread::resume_cond_->Wait(self);
+ if (self->suspend_count_ != 0) {
+ // The condition was signaled but we're still suspended. This
+ // can happen if the debugger lets go while a SIGQUIT thread
+ // dump event is pending (assuming SignalCatcher was resumed for
+ // just long enough to try to grab the thread-suspend lock).
+ LOG(DEBUG) << *self << " still suspended after undo "
+ << "(suspend count=" << self->suspend_count_ << ")";
+ }
}
+ CHECK_EQ(self->suspend_count_, 0);
}
- CHECK_EQ(self->suspend_count_, 0);
- self->SetState(kRunnable);
+
VLOG(threads) << *self << " self-reviving (debugger)";
}
