Add the missing link between compiled code and the debugger.

When a debugger connects and disconnects, we now let compiled code know that we
need to be kept informed about what's going on.

Also fix a threading bug when threads exit with a debugger attached.

Also some minor tidying, mostly involving naming.

Change-Id: Iba0e8b9d192ac76ba1cd29a8b1e6d94f6f20dea8
diff --git a/src/dalvik_system_VMDebug.cc b/src/dalvik_system_VMDebug.cc
index 76750d7..f125272 100644
--- a/src/dalvik_system_VMDebug.cc
+++ b/src/dalvik_system_VMDebug.cc
@@ -104,11 +104,11 @@
 }
 
 static jboolean VMDebug_isDebuggerConnected(JNIEnv*, jclass) {
-  return Dbg::IsDebuggerConnected();
+  return Dbg::IsDebuggerActive();
 }
 
 static jboolean VMDebug_isDebuggingEnabled(JNIEnv*, jclass) {
-  return Dbg::IsDebuggingEnabled();
+  return Dbg::IsJdwpConfigured();
 }
 
 static jlong VMDebug_lastDebuggerActivity(JNIEnv*, jclass) {
diff --git a/src/dalvik_system_VMRuntime.cc b/src/dalvik_system_VMRuntime.cc
index ef11f4b..030caed 100644
--- a/src/dalvik_system_VMRuntime.cc
+++ b/src/dalvik_system_VMRuntime.cc
@@ -96,7 +96,7 @@
 }
 
 static jboolean VMRuntime_isDebuggerActive(JNIEnv*, jobject) {
-  return Dbg::IsDebuggerConnected();
+  return Dbg::IsDebuggerActive();
 }
 
 static jobjectArray VMRuntime_properties(JNIEnv* env, jobject) {
@@ -125,7 +125,6 @@
 }
 
 static void DisableCheckJniCallback(Thread* t, void*) {
-  LOG(INFO) << "Disabling CheckJNI for " << *t;
   t->GetJniEnv()->SetCheckJniEnabled(false);
 }
 
diff --git a/src/debugger.cc b/src/debugger.cc
index 71776ae..569ea81 100644
--- a/src/debugger.cc
+++ b/src/debugger.cc
@@ -149,10 +149,10 @@
 // JDWP is allowed unless the Zygote forbids it.
 static bool gJdwpAllowed = true;
 
-// Was there a -Xrunjdwp or -agent argument on the command-line?
+// Was there a -Xrunjdwp or -agentlib:jdwp= argument on the command line?
 static bool gJdwpConfigured = false;
 
-// Broken-down JDWP options. (Only valid if gJdwpConfigured is true.)
+// Broken-down JDWP options. (Only valid if IsJdwpConfigured() is true.)
 static JDWP::JdwpOptions gJdwpOptions;
 
 // Runtime JDWP state.
@@ -396,7 +396,7 @@
 }
 
 void Dbg::StartJdwp() {
-  if (!gJdwpAllowed || !gJdwpConfigured) {
+  if (!gJdwpAllowed || !IsJdwpConfigured()) {
     // No JDWP for you!
     return;
   }
@@ -477,6 +477,16 @@
   return gDisposed;
 }
 
+static void SetDebuggerUpdatesEnabledCallback(Thread* t, void* user_data) {
+  t->SetDebuggerUpdatesEnabled(*reinterpret_cast<bool*>(user_data));
+}
+
+static void SetDebuggerUpdatesEnabled(bool enabled) {
+  Runtime* runtime = Runtime::Current();
+  ScopedThreadListLock thread_list_lock;
+  runtime->GetThreadList()->ForEach(SetDebuggerUpdatesEnabledCallback, &enabled);
+}
+
 void Dbg::GoActive() {
   // Enable all debugging features, including scans for breakpoints.
   // This is a no-op if we're already active.
@@ -487,29 +497,33 @@
 
   LOG(INFO) << "Debugger is active";
 
-  // TODO: CHECK we don't have any outstanding breakpoints.
+  {
+    // TODO: dalvik only warned if there were breakpoints left over. clear in Dbg::Disconnected?
+    MutexLock mu(gBreakpointsLock);
+    CHECK_EQ(gBreakpoints.size(), 0U);
+  }
 
   gDebuggerActive = true;
-
-  //dvmEnableAllSubMode(kSubModeDebuggerActive);
+  SetDebuggerUpdatesEnabled(true);
 }
 
 void Dbg::Disconnected() {
   CHECK(gDebuggerConnected);
 
-  gDebuggerActive = false;
+  LOG(INFO) << "Debugger is no longer active";
 
-  //dvmDisableAllSubMode(kSubModeDebuggerActive);
+  gDebuggerActive = false;
+  SetDebuggerUpdatesEnabled(false);
 
   gRegistry->Clear();
   gDebuggerConnected = false;
 }
 
-bool Dbg::IsDebuggerConnected() {
+bool Dbg::IsDebuggerActive() {
   return gDebuggerActive;
 }
 
-bool Dbg::IsDebuggingEnabled() {
+bool Dbg::IsJdwpConfigured() {
   return gJdwpConfigured;
 }
 
@@ -1379,7 +1393,6 @@
   case Thread::kTimedWaiting: *pThreadStatus = JDWP::TS_WAIT;     break;
   case Thread::kBlocked:      *pThreadStatus = JDWP::TS_MONITOR;  break;
   case Thread::kWaiting:      *pThreadStatus = JDWP::TS_WAIT;     break;
-  case Thread::kInitializing: *pThreadStatus = JDWP::TS_ZOMBIE;   break;
   case Thread::kStarting:     *pThreadStatus = JDWP::TS_ZOMBIE;   break;
   case Thread::kNative:       *pThreadStatus = JDWP::TS_RUNNING;  break;
   case Thread::kVmWait:       *pThreadStatus = JDWP::TS_WAIT;     break;
@@ -1736,7 +1749,7 @@
 }
 
 void Dbg::PostException(Method** sp, Method* throwMethod, uintptr_t throwNativePc, Method* catchMethod, uintptr_t catchNativePc, Object* exception) {
-  if (!gDebuggerActive) {
+  if (!IsDebuggerActive()) {
     return;
   }
 
@@ -1765,7 +1778,7 @@
 }
 
 void Dbg::PostClassPrepare(Class* c) {
-  if (!gDebuggerActive) {
+  if (!IsDebuggerActive()) {
     return;
   }
 
@@ -1778,7 +1791,7 @@
 }
 
 void Dbg::UpdateDebugger(int32_t dex_pc, Thread* self, Method** sp) {
-  if (!gDebuggerActive || dex_pc == -2 /* fake method exit */) {
+  if (!IsDebuggerActive() || dex_pc == -2 /* fake method exit */) {
     return;
   }
 
@@ -2461,9 +2474,12 @@
 }
 
 void Dbg::PostThreadStartOrStop(Thread* t, uint32_t type) {
-  if (gDebuggerActive) {
+  if (IsDebuggerActive()) {
     JDWP::ObjectId id = gRegistry->Add(t->GetPeer());
     gJdwpState->PostThreadChange(id, type == CHUNK_TYPE("THCR"));
+    // If this thread's just joined the party while we're already debugging, make sure it knows
+    // to give us updates when it's running.
+    t->SetDebuggerUpdatesEnabled(true);
   }
   Dbg::DdmSendThreadNotification(t, type);
 }
diff --git a/src/debugger.h b/src/debugger.h
index 77c307c..6c50e9e 100644
--- a/src/debugger.h
+++ b/src/debugger.h
@@ -95,13 +95,12 @@
   static void Disconnected();
   static void Disposed();
 
-  /*
-   * Returns "true" if a debugger is connected.  Returns "false" if it's
-   * just DDM.
-   */
-  static bool IsDebuggerConnected();
+  // Returns true if we're actually debugging with a real debugger, false if it's
+  // just DDMS (or nothing at all).
+  static bool IsDebuggerActive();
 
-  static bool IsDebuggingEnabled();
+  // Returns true if we had -Xrunjdwp or -agentlib:jdwp= on the command line.
+  static bool IsJdwpConfigured();
 
   static bool IsDisposed();
 
diff --git a/src/jdwp/jdwp_main.cc b/src/jdwp/jdwp_main.cc
index 8fa650a..ad2281f 100644
--- a/src/jdwp/jdwp_main.cc
+++ b/src/jdwp/jdwp_main.cc
@@ -414,7 +414,7 @@
  * processing a debugger request.
  */
 int64_t JdwpState::LastDebuggerActivity() {
-  if (!Dbg::IsDebuggerConnected()) {
+  if (!Dbg::IsDebuggerActive()) {
     LOG(DEBUG) << "no active debugger";
     return -1;
   }
diff --git a/src/runtime_support.h b/src/runtime_support.h
index 00ca5bd..0b9e0c4 100644
--- a/src/runtime_support.h
+++ b/src/runtime_support.h
@@ -51,10 +51,11 @@
 extern int64_t F2L(float f);
 
 }  // namespace art
-/* Helper for both JNI and regular compiled code */
-extern "C" void art_deliver_exception_from_code(void*);
 
+// Helpers for both compiled code and libart.
+extern "C" void art_deliver_exception_from_code(void*);
 extern "C" void art_proxy_invoke_handler();
+extern "C" void art_update_debugger(void*, void*, int32_t, void*);
 
 #if defined(__arm__)
   /* Compiler helpers */
@@ -102,7 +103,6 @@
   extern "C" void art_trace_entry_from_code(void*);
   extern "C" void art_trace_exit_from_code();
   extern "C" void* art_resolve_string_from_code(void*, uint32_t);
-  extern "C" void art_update_debugger(void*, void*, int32_t, void*);
   extern "C" void art_work_around_app_jni_bugs();
 
   /* Conversions */
diff --git a/src/runtime_support_x86.S b/src/runtime_support_x86.S
index 3d57d5d..7fd7af1 100644
--- a/src/runtime_support_x86.S
+++ b/src/runtime_support_x86.S
@@ -4,6 +4,7 @@
     // Mac OS X mangles the functions with an underscore prefix
     #define art_deliver_exception_from_code _art_deliver_exception_from_code
     #define art_proxy_invoke_handler _art_proxy_invoke_handler
+    #define art_update_debugger _art_update_debugger
     #define artDeliverExceptionFromCode _artDeliverExceptionFromCode
 #endif
 
@@ -35,3 +36,8 @@
     .globl art_proxy_invoke_handler
 art_proxy_invoke_handler:
     int3
+
+    // TODO
+    .globl art_update_debugger
+art_update_debugger:
+    int3
diff --git a/src/thread.cc b/src/thread.cc
index 5499091..522024c 100644
--- a/src/thread.cc
+++ b/src/thread.cc
@@ -100,8 +100,7 @@
   pCmplFloat = CmplFloat;
   pCmpgDouble = CmpgDouble;
   pCmplDouble = CmplDouble;
-#endif
-#if defined(__arm__)
+#elif defined(__arm__)
   pShlLong = art_shl_long;
   pShrLong = art_shr_long;
   pUshrLong = art_ushr_long;
@@ -172,9 +171,7 @@
   pThrowStackOverflowFromCode = art_throw_stack_overflow_from_code;
   pThrowVerificationErrorFromCode = art_throw_verification_error_from_code;
   pUnlockObjectFromCode = art_unlock_object_from_code;
-  pUpdateDebuggerFromCode = NULL;  // To enable, set to art_update_debugger
-#endif
-#if defined(__i386__)
+#elif defined(__i386__)
   pShlLong = NULL;
   pShrLong = NULL;
   pUshrLong = NULL;
@@ -245,7 +242,6 @@
   pThrowStackOverflowFromCode = NULL;
   pThrowVerificationErrorFromCode = NULL;
   pUnlockObjectFromCode = NULL;
-  pUpdateDebuggerFromCode = NULL;  // To enable, set to art_update_debugger
 #endif
   pF2l = F2L;
   pD2l = D2L;
@@ -258,6 +254,12 @@
   pInstanceofNonTrivialFromCode = IsAssignableFromCode;
   pThrowAbstractMethodErrorFromCode = ThrowAbstractMethodErrorFromCode;
   pUnresolvedDirectMethodTrampolineFromCode = UnresolvedDirectMethodTrampolineFromCode;
+  pUpdateDebuggerFromCode = NULL; // Controlled by SetDebuggerUpdatesEnabled.
+}
+
+void Thread::SetDebuggerUpdatesEnabled(bool enabled) {
+  LOG(INFO) << "Turning debugger updates " << (enabled ? "on" : "off") << " for " << *this;
+  pUpdateDebuggerFromCode = (enabled ? art_update_debugger : NULL);
 }
 
 void Thread::InitTid() {
@@ -972,22 +974,22 @@
   entered_monitor->MonitorExit(Thread::Current());
 }
 
-Thread::~Thread() {
+void Thread::Destroy() {
   // On thread detach, all monitors entered with JNI MonitorEnter are automatically exited.
   if (jni_env_ != NULL) {
     jni_env_->monitors.VisitRoots(MonitorExitVisitor, NULL);
   }
 
   if (peer_ != NULL) {
+    Thread* self = this;
 
     // this.vmData = 0;
     SetVmData(peer_, NULL);
 
-    Dbg::PostThreadDeath(this);
+    Dbg::PostThreadDeath(self);
 
     // Thread.join() is implemented as an Object.wait() on the Thread.lock
     // object. Signal anyone who is waiting.
-    Thread* self = Thread::Current();
     Object* lock = gThread_lock->GetObject(peer_);
     // (This conditional is only needed for tests, where Thread.lock won't have been set.)
     if (lock != NULL) {
@@ -996,7 +998,9 @@
       lock->MonitorExit(self);
     }
   }
+}
 
+Thread::~Thread() {
   delete jni_env_;
   jni_env_ = NULL;
 
diff --git a/src/thread.h b/src/thread.h
index ece54a2..a8d0812 100644
--- a/src/thread.h
+++ b/src/thread.h
@@ -77,11 +77,10 @@
     kBlocked      = 3,        // TS_MONITOR on a monitor
     kWaiting      = 4,        // TS_WAIT in Object.wait()
     // Non-JDWP states.
-    kInitializing = 5,        // allocated, not yet running --- TODO: unnecessary?
-    kStarting     = 6,        // native thread started, not yet ready to run managed code
-    kNative       = 7,        // off in a JNI native method
-    kVmWait       = 8,        // waiting on an internal runtime resource
-    kSuspended    = 9,        // suspended, usually by GC or debugger
+    kStarting     = 5,        // native thread started, not yet ready to run managed code
+    kNative       = 6,        // off in a JNI native method
+    kVmWait       = 7,        // waiting on an internal runtime resource
+    kSuspended    = 8,        // suspended, usually by GC or debugger
   };
 
   // Space to throw a StackOverflowError in.
@@ -419,6 +418,8 @@
     return debug_invoke_req_;
   }
 
+  void SetDebuggerUpdatesEnabled(bool enabled);
+
   bool IsTraceStackEmpty() const {
     return trace_stack_->empty();
   }
@@ -443,7 +444,8 @@
  private:
   Thread();
   ~Thread();
-  friend class ThreadList;  // For ~Thread.
+  void Destroy();
+  friend class ThreadList;  // For ~Thread and Destroy.
 
   void CreatePeer(const char* name, bool as_daemon, Object* thread_group);
   friend class Runtime; // For CreatePeer.
diff --git a/src/thread_list.cc b/src/thread_list.cc
index 9cefc82..8927820 100644
--- a/src/thread_list.cc
+++ b/src/thread_list.cc
@@ -326,31 +326,23 @@
 
   VLOG(threads) << "ThreadList::Unregister() " << *self;
 
-  if (self->GetPeer() != NULL) {
-      self->SetState(Thread::kRunnable);
+  // Any time-consuming destruction, plus anything that can call back into managed code or
+  // suspend and so on, must happen at this point, and not in ~Thread.
+  self->Destroy();
 
-      // This may need to call user-supplied managed code. Make sure we do this before we start tearing
-      // down the Thread* and removing it from the thread list (or start taking any locks).
-      self->HandleUncaughtExceptions();
+  {
+    // Remove this thread from the list.
+    ScopedThreadListLock thread_list_lock;
+    CHECK(Contains(self));
+    list_.remove(self);
 
-      // Make sure we remove from ThreadGroup before taking the
-      // thread_list_lock_ since it allocates an Iterator which can cause
-      // a GC which will want to suspend.
-      self->RemoveFromThreadGroup();
+    // Delete the Thread* and release the thin lock id.
+    uint32_t thin_lock_id = self->thin_lock_id_;
+    delete self;
+    ReleaseThreadId(thin_lock_id);
   }
 
-  ScopedThreadListLock thread_list_lock;
-
-  // Remove this thread from the list.
-  CHECK(Contains(self));
-  list_.remove(self);
-
-  // Delete the Thread* and release the thin lock id.
-  uint32_t thin_lock_id = self->thin_lock_id_;
-  delete self;
-  ReleaseThreadId(thin_lock_id);
-
-  // Clear the TLS data, so that thread is recognizably detached.
+  // Clear the TLS data, so that the underlying native thread is recognizably detached.
   // (It may wish to reattach later.)
   CHECK_PTHREAD_CALL(pthread_setspecific, (Thread::pthread_key_self_, NULL), "detach self");