Merge "JDWP: update thread synchronization"
diff --git a/runtime/debugger.cc b/runtime/debugger.cc
index a3d3b47..13bbdeb 100644
--- a/runtime/debugger.cc
+++ b/runtime/debugger.cc
@@ -605,7 +605,7 @@
}
void Dbg::ClearWaitForEventThread() {
- gJdwpState->ClearWaitForEventThread();
+ gJdwpState->ReleaseJdwpTokenForEvent();
}
void Dbg::Connected() {
@@ -3644,6 +3644,11 @@
thread_list->Resume(targetThread, true);
}
+ // The target thread is resumed but needs the JDWP token we're holding.
+ // We release it now and will acquire it again when the invocation is
+ // complete and the target thread suspends itself.
+ gJdwpState->ReleaseJdwpTokenForCommand();
+
// Wait for the request to finish executing.
while (req->invoke_needed) {
req->cond.Wait(self);
@@ -3653,6 +3658,10 @@
/* wait for thread to re-suspend itself */
SuspendThread(thread_id, false /* request_suspension */);
+
+ // Now the thread is suspended again, we can re-acquire the JDWP token.
+ gJdwpState->AcquireJdwpTokenForCommand();
+
self->TransitionFromSuspendedToRunnable();
}
@@ -3660,7 +3669,7 @@
* Suspend the threads. We waited for the target thread to suspend
* itself, so all we need to do is suspend the others.
*
- * The suspendAllThreads() call will double-suspend the event thread,
+ * The SuspendAllForDebugger() call will double-suspend the event thread,
* so we want to resume the target thread once to keep the books straight.
*/
if ((options & JDWP::INVOKE_SINGLE_THREADED) == 0) {
diff --git a/runtime/jdwp/jdwp.h b/runtime/jdwp/jdwp.h
index 9f37998..e16221c 100644
--- a/runtime/jdwp/jdwp.h
+++ b/runtime/jdwp/jdwp.h
@@ -149,29 +149,19 @@
void ExitAfterReplying(int exit_status);
- /*
- * When we hit a debugger event that requires suspension, it's important
- * that we wait for the thread to suspend itself before processing any
- * additional requests. (Otherwise, if the debugger immediately sends a
- * "resume thread" command, the resume might arrive before the thread has
- * suspended itself.)
- *
- * The thread should call the "set" function before sending the event to
- * the debugger. The main JDWP handler loop calls "get" before processing
- * an event, and will wait for thread suspension if it's set. Once the
- * thread has suspended itself, the JDWP handler calls "clear" and
- * continues processing the current event. This works in the suspend-all
- * case because the event thread doesn't suspend itself until everything
- * else has suspended.
- *
- * It's possible that multiple threads could encounter thread-suspending
- * events at the same time, so we grab a mutex in the "set" call, and
- * release it in the "clear" call.
- */
- // ObjectId GetWaitForEventThread();
- void SetWaitForEventThread(ObjectId threadId)
- LOCKS_EXCLUDED(event_thread_lock_, process_request_lock_);
- void ClearWaitForEventThread() LOCKS_EXCLUDED(event_thread_lock_);
+ // Acquires/releases the JDWP synchronization token for the debugger
+ // thread (command handler) so no event thread posts an event while
+ // it processes a command. This must be called only from the debugger
+ // thread.
+ void AcquireJdwpTokenForCommand() LOCKS_EXCLUDED(jdwp_token_lock_);
+ void ReleaseJdwpTokenForCommand() LOCKS_EXCLUDED(jdwp_token_lock_);
+
+ // Acquires/releases the JDWP synchronization token for the event thread
+ // so no other thread (debugger thread or event thread) interleaves with
+ // it when posting an event. This must NOT be called from the debugger
+ // thread, only event thread.
+ void AcquireJdwpTokenForEvent(ObjectId threadId) LOCKS_EXCLUDED(jdwp_token_lock_);
+ void ReleaseJdwpTokenForEvent() LOCKS_EXCLUDED(jdwp_token_lock_);
/*
* These notify the debug code that something interesting has happened. This
@@ -330,9 +320,37 @@
SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
void SendBufferedRequest(uint32_t type, const std::vector<iovec>& iov);
- void StartProcessingRequest() LOCKS_EXCLUDED(process_request_lock_);
- void EndProcessingRequest() LOCKS_EXCLUDED(process_request_lock_);
- void WaitForProcessingRequest() LOCKS_EXCLUDED(process_request_lock_);
+ /*
+ * When we hit a debugger event that requires suspension, it's important
+ * that we wait for the thread to suspend itself before processing any
+ * additional requests. Otherwise, if the debugger immediately sends a
+ * "resume thread" command, the resume might arrive before the thread has
+ * suspended itself.
+ *
+ * It's also important no event thread suspends while we process a command
+ * from the debugger. Otherwise we could post an event ("thread death")
+ * before sending the reply of the command being processed ("resume") and
+ * cause bad synchronization with the debugger.
+ *
+ * The thread wanting "exclusive" access to the JDWP world must call the
+ * SetWaitForJdwpToken method before processing a command from the
+ * debugger or sending an event to the debugger.
+ * Once the command is processed or the event thread has posted its event,
+ * it must call the ClearWaitForJdwpToken method to allow another thread
+ * to do JDWP stuff.
+ *
+ * Therefore the main JDWP handler loop will wait for the event thread
+ * suspension before processing the next command. Once the event thread
+ * has suspended itself and cleared the token, the JDWP handler continues
+ * processing commands. This works in the suspend-all case because the
+ * event thread doesn't suspend itself until everything else has suspended.
+ *
+ * It's possible that multiple threads could encounter thread-suspending
+ * events at the same time, so we grab a mutex in the SetWaitForJdwpToken
+ * call, and release it in the ClearWaitForJdwpToken call.
+ */
+ void SetWaitForJdwpToken(ObjectId threadId) LOCKS_EXCLUDED(jdwp_token_lock_);
+ void ClearWaitForJdwpToken() LOCKS_EXCLUDED(jdwp_token_lock_);
public: // TODO: fix privacy
const JdwpOptions* options_;
@@ -368,24 +386,21 @@
// Linked list of events requested by the debugger (breakpoints, class prep, etc).
Mutex event_list_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER ACQUIRED_BEFORE(Locks::breakpoint_lock_);
-
JdwpEvent* event_list_ GUARDED_BY(event_list_lock_);
size_t event_list_size_ GUARDED_BY(event_list_lock_); // Number of elements in event_list_.
- // Used to synchronize suspension of the event thread (to avoid receiving "resume"
- // events before the thread has finished suspending itself).
- Mutex event_thread_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
- ConditionVariable event_thread_cond_ GUARDED_BY(event_thread_lock_);
- ObjectId event_thread_id_;
-
- // Used to synchronize request processing and event sending (to avoid sending an event before
- // sending the reply of a command being processed).
- Mutex process_request_lock_ ACQUIRED_AFTER(event_thread_lock_);
- ConditionVariable process_request_cond_ GUARDED_BY(process_request_lock_);
- bool processing_request_ GUARDED_BY(process_request_lock_);
+ // Used to synchronize JDWP command handler thread and event threads so only one
+ // thread does JDWP stuff at a time. This prevent from interleaving command handling
+ // and event notification. Otherwise we could receive a "resume" command for an
+ // event thread that is not suspended yet, or post a "thread death" or event "VM death"
+ // event before sending the reply of the "resume" command that caused it.
+ Mutex jdwp_token_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
+ ConditionVariable jdwp_token_cond_ GUARDED_BY(jdwp_token_lock_);
+ ObjectId jdwp_token_owner_thread_id_;
bool ddm_is_active_;
+ // Used for VirtualMachine.Exit command handling.
bool should_exit_;
int exit_status_;
};
diff --git a/runtime/jdwp/jdwp_event.cc b/runtime/jdwp/jdwp_event.cc
index a8eaa26..b71f6cd 100644
--- a/runtime/jdwp/jdwp_event.cc
+++ b/runtime/jdwp/jdwp_event.cc
@@ -612,7 +612,7 @@
}
/* grab this before posting/suspending again */
- SetWaitForEventThread(thread_self_id);
+ AcquireJdwpTokenForEvent(thread_self_id);
/* leave pReq->invoke_needed_ raised so we can check reentrancy */
Dbg::ExecuteMethod(pReq);
@@ -630,7 +630,7 @@
JDWP::ObjectId thread_self_id = Dbg::GetThreadSelfId();
self->TransitionFromRunnableToSuspended(kWaitingForDebuggerSend);
if (suspend_policy != SP_NONE) {
- SetWaitForEventThread(threadId);
+ AcquireJdwpTokenForEvent(threadId);
}
EventFinish(pReq);
SuspendByPolicy(suspend_policy, thread_self_id);
@@ -649,63 +649,82 @@
return pReq->invoke_needed;
}
+void JdwpState::AcquireJdwpTokenForCommand() {
+ CHECK_EQ(Thread::Current(), GetDebugThread()) << "Expected debugger thread";
+ SetWaitForJdwpToken(debug_thread_id_);
+}
+
+void JdwpState::ReleaseJdwpTokenForCommand() {
+ CHECK_EQ(Thread::Current(), GetDebugThread()) << "Expected debugger thread";
+ ClearWaitForJdwpToken();
+}
+
+void JdwpState::AcquireJdwpTokenForEvent(ObjectId threadId) {
+ CHECK_NE(Thread::Current(), GetDebugThread()) << "Expected event thread";
+ CHECK_NE(debug_thread_id_, threadId) << "Not expected debug thread";
+ SetWaitForJdwpToken(threadId);
+}
+
+void JdwpState::ReleaseJdwpTokenForEvent() {
+ CHECK_NE(Thread::Current(), GetDebugThread()) << "Expected event thread";
+ ClearWaitForJdwpToken();
+}
+
/*
* We need the JDWP thread to hold off on doing stuff while we post an
* event and then suspend ourselves.
*
- * Call this with a threadId of zero if you just want to wait for the
- * current thread operation to complete.
- *
* This could go to sleep waiting for another thread, so it's important
* that the thread be marked as VMWAIT before calling here.
*/
-void JdwpState::SetWaitForEventThread(ObjectId threadId) {
+void JdwpState::SetWaitForJdwpToken(ObjectId threadId) {
bool waited = false;
+ Thread* const self = Thread::Current();
+ CHECK_NE(threadId, 0u);
+ CHECK_NE(self->GetState(), kRunnable);
+ Locks::mutator_lock_->AssertNotHeld(self);
/* this is held for very brief periods; contention is unlikely */
- Thread* self = Thread::Current();
- MutexLock mu(self, event_thread_lock_);
+ MutexLock mu(self, jdwp_token_lock_);
+
+ CHECK_NE(jdwp_token_owner_thread_id_, threadId) << "Thread is already holding event thread lock";
/*
* If another thread is already doing stuff, wait for it. This can
* go to sleep indefinitely.
*/
- while (event_thread_id_ != 0) {
+ while (jdwp_token_owner_thread_id_ != 0) {
VLOG(jdwp) << StringPrintf("event in progress (%#" PRIx64 "), %#" PRIx64 " sleeping",
- event_thread_id_, threadId);
+ jdwp_token_owner_thread_id_, threadId);
waited = true;
- event_thread_cond_.Wait(self);
+ jdwp_token_cond_.Wait(self);
}
- if (waited || threadId != 0) {
+ if (waited || threadId != debug_thread_id_) {
VLOG(jdwp) << StringPrintf("event token grabbed (%#" PRIx64 ")", threadId);
}
- if (threadId != 0) {
- event_thread_id_ = threadId;
- }
+ jdwp_token_owner_thread_id_ = threadId;
}
/*
* Clear the threadId and signal anybody waiting.
*/
-void JdwpState::ClearWaitForEventThread() {
+void JdwpState::ClearWaitForJdwpToken() {
/*
* Grab the mutex. Don't try to go in/out of VMWAIT mode, as this
- * function is called by dvmSuspendSelf(), and the transition back
+ * function is called by Dbg::SuspendSelf(), and the transition back
* to RUNNING would confuse it.
*/
- Thread* self = Thread::Current();
- MutexLock mu(self, event_thread_lock_);
+ Thread* const self = Thread::Current();
+ MutexLock mu(self, jdwp_token_lock_);
- CHECK_NE(event_thread_id_, 0U);
- VLOG(jdwp) << StringPrintf("cleared event token (%#" PRIx64 ")", event_thread_id_);
+ CHECK_NE(jdwp_token_owner_thread_id_, 0U);
+ VLOG(jdwp) << StringPrintf("cleared event token (%#" PRIx64 ")", jdwp_token_owner_thread_id_);
- event_thread_id_ = 0;
-
- event_thread_cond_.Signal(self);
+ jdwp_token_owner_thread_id_ = 0;
+ jdwp_token_cond_.Signal(self);
}
-
/*
* Prep an event. Allocates storage for the message and leaves space for
* the header.
@@ -730,11 +749,6 @@
Set1(buf + 9, kJdwpEventCommandSet);
Set1(buf + 10, kJdwpCompositeCommand);
- // Prevents from interleaving commands and events. Otherwise we could end up in sending an event
- // before sending the reply of the command being processed and would lead to bad synchronization
- // between the debugger and the debuggee.
- WaitForProcessingRequest();
-
SendRequest(pReq);
expandBufFree(pReq);
diff --git a/runtime/jdwp/jdwp_handler.cc b/runtime/jdwp/jdwp_handler.cc
index a1d2a6c..0ce4de7 100644
--- a/runtime/jdwp/jdwp_handler.cc
+++ b/runtime/jdwp/jdwp_handler.cc
@@ -1633,27 +1633,15 @@
/*
* If a debugger event has fired in another thread, wait until the
- * initiating thread has suspended itself before processing messages
+ * initiating thread has suspended itself before processing commands
* from the debugger. Otherwise we (the JDWP thread) could be told to
* resume the thread before it has suspended.
*
- * We call with an argument of zero to wait for the current event
- * thread to finish, and then clear the block. Depending on the thread
- * suspend policy, this may allow events in other threads to fire,
- * but those events have no bearing on what the debugger has sent us
- * in the current request->
- *
* Note that we MUST clear the event token before waking the event
* thread up, or risk waiting for the thread to suspend after we've
* told it to resume.
*/
- SetWaitForEventThread(0);
-
- /*
- * We do not want events to be sent while we process a request-> Indicate the JDWP thread starts
- * to process a request so other threads wait for it to finish before sending an event.
- */
- StartProcessingRequest();
+ AcquireJdwpTokenForCommand();
/*
* Tell the VM that we're running and shouldn't be interrupted by GC.
@@ -1719,50 +1707,6 @@
return replyLength;
}
-/*
- * Indicates a request is about to be processed. If a thread wants to send an event in the meantime,
- * it will need to wait until we processed this request (see EndProcessingRequest).
- */
-void JdwpState::StartProcessingRequest() {
- Thread* self = Thread::Current();
- CHECK_EQ(self, GetDebugThread()) << "Requests are only processed by debug thread";
- MutexLock mu(self, process_request_lock_);
- CHECK_EQ(processing_request_, false);
- processing_request_ = true;
-}
-
-/*
- * Indicates a request has been processed (and we sent its reply). All threads waiting for us (see
- * WaitForProcessingRequest) are waken up so they can send events again.
- */
-void JdwpState::EndProcessingRequest() {
- Thread* self = Thread::Current();
- CHECK_EQ(self, GetDebugThread()) << "Requests are only processed by debug thread";
- MutexLock mu(self, process_request_lock_);
- CHECK_EQ(processing_request_, true);
- processing_request_ = false;
- process_request_cond_.Broadcast(self);
-}
-
-/*
- * Waits for any request being processed so we do not send an event in the meantime.
- */
-void JdwpState::WaitForProcessingRequest() {
- Thread* self = Thread::Current();
- CHECK_NE(self, GetDebugThread()) << "Events should not be posted by debug thread";
- MutexLock mu(self, process_request_lock_);
- bool waited = false;
- while (processing_request_) {
- VLOG(jdwp) << StringPrintf("wait for processing request");
- waited = true;
- process_request_cond_.Wait(self);
- }
- if (waited) {
- VLOG(jdwp) << StringPrintf("finished waiting for processing request");
- }
- CHECK_EQ(processing_request_, false);
-}
-
} // namespace JDWP
} // namespace art
diff --git a/runtime/jdwp/jdwp_main.cc b/runtime/jdwp/jdwp_main.cc
index b04aa6e..b6fedd9 100644
--- a/runtime/jdwp/jdwp_main.cc
+++ b/runtime/jdwp/jdwp_main.cc
@@ -220,12 +220,9 @@
event_list_lock_("JDWP event list lock", kJdwpEventListLock),
event_list_(nullptr),
event_list_size_(0),
- event_thread_lock_("JDWP event thread lock"),
- event_thread_cond_("JDWP event thread condition variable", event_thread_lock_),
- event_thread_id_(0),
- process_request_lock_("JDWP process request lock"),
- process_request_cond_("JDWP process request condition variable", process_request_lock_),
- processing_request_(false),
+ jdwp_token_lock_("JDWP token lock"),
+ jdwp_token_cond_("JDWP token condition variable", jdwp_token_lock_),
+ jdwp_token_owner_thread_id_(0),
ddm_is_active_(false),
should_exit_(false),
exit_status_(0) {
@@ -331,7 +328,7 @@
* Should not have one of these in progress. If the debugger went away
* mid-request, though, we could see this.
*/
- if (event_thread_id_ != 0) {
+ if (jdwp_token_owner_thread_id_ != 0) {
LOG(WARNING) << "Resetting state while event in progress";
DCHECK(false);
}
@@ -382,10 +379,9 @@
ssize_t cc = netStateBase->WritePacket(pReply, replyLength);
/*
- * We processed this request and sent its reply. Notify other threads waiting for us they can now
- * send events.
+ * We processed this request and sent its reply so we can release the JDWP token.
*/
- EndProcessingRequest();
+ ReleaseJdwpTokenForCommand();
if (cc != static_cast<ssize_t>(replyLength)) {
PLOG(ERROR) << "Failed sending reply to debugger";