JDWP: allocate DebugInvokeReq only when requested

Only allocates thread-local DebugInvokeReq when the debugger requests
a thread to invoke a method. The JDWP thread allocates that structure
then attaches it to the target thread. When the thread is resumed, it
executes the method. Once the invocation completes, the thread
detaches the DebugInvokeReq, signals the JDWP thread then suspends.
Finally, the JDWP thread wakes up, prepares the reply with the invoke
result (or exception) and deallocates the DebugInvokeReq.

Also ensures GC safety for object returned by the invoke. We add the
object to the JDWP object registry right after the invoke. We now
reference that object with a JDWP ObjectID instead of an Object* in
the DebugInvokeReq struct. This prevent from accessing a stale
reference if the GC runs and moves the Object*.

This CL includes the following changes:
- Move former DebugInvokeReq::ready flag to
  Thread::tls_32bit_sized_values::ready_for_debug_invoke. It's needed
  to know whether a thread has been suspended by an event, thus ready
  to invoke a method from the debugger.
- Remove DebugInvokeReq::invoke_needed: we now test if we attached a
  DebugInvokeReq* to the thread.
- Rename misleading FinishMethod function to RequestMethod.

Bug: 19142632
Bug: 18166750
Change-Id: I351fb4eb94bfe69fcafb544d21d55ff35a033000

2 files changed, 36 insertions, 43 deletions

diff --git a/runtime/jdwp/jdwp_event.cc b/runtime/jdwp/jdwp_event.cc
index fc08d23274..4bf7142692 100644
--- a/runtime/jdwp/jdwp_event.cc
+++ b/runtime/jdwp/jdwp_event.cc
@@ -596,17 +596,15 @@ void JdwpState::SuspendByPolicy(JdwpSuspendPolicy suspend_policy, JDWP::ObjectId
     return;
   }
 
-  DebugInvokeReq* pReq = Dbg::GetInvokeReq();
   while (true) {
-    pReq->ready = true;
     Dbg::SuspendSelf();
-    pReq->ready = false;
 
     /*
      * The JDWP thread has told us (and possibly all other threads) to
      * resume.  See if it has left anything in our DebugInvokeReq mailbox.
      */
-    if (!pReq->invoke_needed) {
+    DebugInvokeReq* const pReq = Dbg::GetInvokeReq();
+    if (pReq == nullptr) {
       /*LOGD("SuspendByPolicy: no invoke needed");*/
       break;
     }
@@ -614,10 +612,7 @@ void JdwpState::SuspendByPolicy(JdwpSuspendPolicy suspend_policy, JDWP::ObjectId
     /* grab this before posting/suspending again */
     AcquireJdwpTokenForEvent(thread_self_id);
 
-    /* leave pReq->invoke_needed_ raised so we can check reentrancy */
     Dbg::ExecuteMethod(pReq);
-
-    pReq->error = ERR_NONE;
   }
 }
 
@@ -650,7 +645,7 @@ void JdwpState::SendRequestAndPossiblySuspend(ExpandBuf* pReq, JdwpSuspendPolicy
  */
 bool JdwpState::InvokeInProgress() {
   DebugInvokeReq* pReq = Dbg::GetInvokeReq();
-  return pReq->invoke_needed;
+  return pReq != nullptr;
 }
 
 void JdwpState::AcquireJdwpTokenForCommand() {
diff --git a/runtime/jdwp/jdwp_handler.cc b/runtime/jdwp/jdwp_handler.cc
index 0ce4de7f61..c7083dcedc 100644
--- a/runtime/jdwp/jdwp_handler.cc
+++ b/runtime/jdwp/jdwp_handler.cc
@@ -91,9 +91,9 @@ static JdwpError WriteTaggedObjectList(ExpandBuf* reply, const std::vector<Objec
  * If "is_constructor" is set, this returns "object_id" rather than the
  * expected-to-be-void return value of the called function.
  */
-static JdwpError FinishInvoke(JdwpState*, Request* request, ExpandBuf* pReply,
-                              ObjectId thread_id, ObjectId object_id,
-                              RefTypeId class_id, MethodId method_id, bool is_constructor)
+static JdwpError RequestInvoke(JdwpState*, Request* request, ExpandBuf* pReply,
+                               ObjectId thread_id, ObjectId object_id,
+                               RefTypeId class_id, MethodId method_id, bool is_constructor)
     SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
   CHECK(!is_constructor || object_id != 0);
 
@@ -131,37 +131,35 @@ static JdwpError FinishInvoke(JdwpState*, Request* request, ExpandBuf* pReply,
     return err;
   }
 
-  if (err == ERR_NONE) {
-    if (is_constructor) {
-      // If we invoked a constructor (which actually returns void), return the receiver,
-      // unless we threw, in which case we return NULL.
-      resultTag = JT_OBJECT;
-      resultValue = (exceptObjId == 0) ? object_id : 0;
-    }
+  if (is_constructor) {
+    // If we invoked a constructor (which actually returns void), return the receiver,
+    // unless we threw, in which case we return NULL.
+    resultTag = JT_OBJECT;
+    resultValue = (exceptObjId == 0) ? object_id : 0;
+  }
 
-    size_t width = Dbg::GetTagWidth(resultTag);
-    expandBufAdd1(pReply, resultTag);
-    if (width != 0) {
-      WriteValue(pReply, width, resultValue);
-    }
-    expandBufAdd1(pReply, JT_OBJECT);
-    expandBufAddObjectId(pReply, exceptObjId);
-
-    VLOG(jdwp) << "  --> returned " << resultTag
-        << StringPrintf(" %#" PRIx64 " (except=%#" PRIx64 ")", resultValue, exceptObjId);
-
-    /* show detailed debug output */
-    if (resultTag == JT_STRING && exceptObjId == 0) {
-      if (resultValue != 0) {
-        if (VLOG_IS_ON(jdwp)) {
-          std::string result_string;
-          JDWP::JdwpError error = Dbg::StringToUtf8(resultValue, &result_string);
-          CHECK_EQ(error, JDWP::ERR_NONE);
-          VLOG(jdwp) << "      string '" << result_string << "'";
-        }
-      } else {
-        VLOG(jdwp) << "      string (null)";
+  size_t width = Dbg::GetTagWidth(resultTag);
+  expandBufAdd1(pReply, resultTag);
+  if (width != 0) {
+    WriteValue(pReply, width, resultValue);
+  }
+  expandBufAdd1(pReply, JT_OBJECT);
+  expandBufAddObjectId(pReply, exceptObjId);
+
+  VLOG(jdwp) << "  --> returned " << resultTag
+      << StringPrintf(" %#" PRIx64 " (except=%#" PRIx64 ")", resultValue, exceptObjId);
+
+  /* show detailed debug output */
+  if (resultTag == JT_STRING && exceptObjId == 0) {
+    if (resultValue != 0) {
+      if (VLOG_IS_ON(jdwp)) {
+        std::string result_string;
+        JDWP::JdwpError error = Dbg::StringToUtf8(resultValue, &result_string);
+        CHECK_EQ(error, JDWP::ERR_NONE);
+        VLOG(jdwp) << "      string '" << result_string << "'";
       }
+    } else {
+      VLOG(jdwp) << "      string (null)";
     }
   }
 
@@ -693,7 +691,7 @@ static JdwpError CT_InvokeMethod(JdwpState* state, Request* request, ExpandBuf*
   ObjectId thread_id = request->ReadThreadId();
   MethodId method_id = request->ReadMethodId();
 
-  return FinishInvoke(state, request, pReply, thread_id, 0, class_id, method_id, false);
+  return RequestInvoke(state, request, pReply, thread_id, 0, class_id, method_id, false);
 }
 
 /*
@@ -717,7 +715,7 @@ static JdwpError CT_NewInstance(JdwpState* state, Request* request, ExpandBuf* p
   if (object_id == 0) {
     return ERR_OUT_OF_MEMORY;
   }
-  return FinishInvoke(state, request, pReply, thread_id, object_id, class_id, method_id, true);
+  return RequestInvoke(state, request, pReply, thread_id, object_id, class_id, method_id, true);
 }
 
 /*
@@ -879,7 +877,7 @@ static JdwpError OR_InvokeMethod(JdwpState* state, Request* request, ExpandBuf*
   RefTypeId class_id = request->ReadRefTypeId();
   MethodId method_id = request->ReadMethodId();
 
-  return FinishInvoke(state, request, pReply, thread_id, object_id, class_id, method_id, false);
+  return RequestInvoke(state, request, pReply, thread_id, object_id, class_id, method_id, false);
 }
 
 static JdwpError OR_DisableCollection(JdwpState*, Request* request, ExpandBuf*)