Fix a deadlock in the breakpoint code.

In froyo we started using "hard" breakpoints, where we replace the
existing opcodes with breakpoint instructions.  This requires some
coordination to avoid confusing the verifier.  The previous approach
allowed the breakpoints to be inserted, and "undid" them while the
verifier ran; this worked, but caused us to be holding a lock for
an extended period.

The new approach just avoids altering the bytecode of unverified
classes, and then "flushes" the breakpoint set out between the time
when verification completes and class initialization starts.  This
removes the possibility of blocking with the lock held, and makes
everything much simpler.

For bug 2615063.

(cherry-pick from dalvik-dev)

Change-Id: I75f19b0cc71fc0babb50ab299c6c5a865e06c919
diff --git a/vm/analysis/DexVerify.c b/vm/analysis/DexVerify.c
index c490691..088309c 100644
--- a/vm/analysis/DexVerify.c
+++ b/vm/analysis/DexVerify.c
@@ -150,66 +150,6 @@
     return true;
 }
 
-/*
- * Temporarily "undo" any breakpoints found in this method.  There is no risk
- * of confusing the interpreter, because unverified code cannot be executed.
- *
- * Breakpoints can be set after a class is loaded but before it has been
- * verified.
- *
- * The "breakpoint" opcode can replace any other opcode, leaving no
- * indication of the original instruction's width or purpose in the
- * instruction stream.  We either have to quietly undo the breakpoints
- * before verification, or look up the original opcode whenever we need it.
- * The latter is more efficient since we only slow down on code that
- * actually has breakpoints, but it requires explicit handling in every
- * function that examines the instruction stream.
- *
- * We need to ensure that the debugger doesn't insert any additional
- * breakpoints while we work.  This either requires holding a lock on the
- * breakpoint set throughout the verification of this method, or adding a
- * "do not touch anything on these pages" list to the set.  Either way,
- * the caller of this method must ensure that it calls "redo" to release
- * state.
- *
- * A debugger could connect while we work, so we return without doing
- * anything if a debugger doesn't happen to be connected now.  We can only
- * avoid doing work if the debugger thread isn't running (dexopt, zygote,
- * or debugging not configured).
- *
- * Returns "false" if we did nothing, "true" if we did stuff (and, hence,
- * need to call "redo" at some point).
- */
-static bool undoBreakpoints(Method* meth)
-{
-#ifdef WITH_DEBUGGER
-    if (gDvm.optimizing || gDvm.zygote || !gDvm.jdwpConfigured)
-        return false;
-    dvmUndoBreakpoints(meth);
-    return true;
-#else
-    return false;
-#endif
-}
-
-/*
- * Restore any breakpoints we undid previously.  Also has to update the
- * stored "original opcode" value for any instruction that we replaced
- * with a throw-verification-error op.
- */
-static void redoBreakpoints(Method* meth)
-{
-#ifdef WITH_DEBUGGER
-    if (gDvm.optimizing || gDvm.zygote || !gDvm.jdwpConfigured) {
-        /* should not be here */
-        assert(false);
-        return;
-    }
-    dvmRedoBreakpoints(meth);
-#else
-    assert(false);
-#endif
-}
 
 /*
  * Perform verification on a single method.
@@ -238,9 +178,6 @@
     UninitInstanceMap* uninitMap = NULL;
     InsnFlags* insnFlags = NULL;
     int i, newInstanceCount;
-    bool undidBreakpoints;
-
-    undidBreakpoints = undoBreakpoints(meth);
 
     /*
      * If there aren't any instructions, make sure that's expected, then
@@ -322,8 +259,6 @@
     result = true;
 
 bail:
-    if (undidBreakpoints)
-        redoBreakpoints(meth);
     dvmFreeUninitInstanceMap(uninitMap);
     free(insnFlags);
     return result;
diff --git a/vm/interp/Interp.c b/vm/interp/Interp.c
index 4401520..002dfb1 100644
--- a/vm/interp/Interp.c
+++ b/vm/interp/Interp.c
@@ -71,6 +71,7 @@
  * We only remove the breakpoint when the last instance is cleared.
  */
 typedef struct {
+    Method*     method;                 /* method we're associated with */
     u2*         addr;                   /* absolute memory address */
     u1          originalOpCode;         /* original 8-bit opcode value */
     int         setCount;               /* #of times this breakpoint was set */
@@ -116,10 +117,20 @@
 
 /*
  * Lock the breakpoint set.
+ *
+ * It's not currently necessary to switch to VMWAIT in the event of
+ * contention, because nothing in here can block.  However, it's possible
+ * that the bytecode-updater code could become fancier in the future, so
+ * we do the trylock dance as a bit of future-proofing.
  */
 static void dvmBreakpointSetLock(BreakpointSet* pSet)
 {
-    dvmLockMutex(&pSet->lock);
+    if (dvmTryLockMutex(&pSet->lock) != 0) {
+        Thread* self = dvmThreadSelf();
+        int oldStatus = dvmChangeStatus(self, THREAD_VMWAIT);
+        dvmLockMutex(&pSet->lock);
+        dvmChangeStatus(self, oldStatus);
+    }
 }
 
 /*
@@ -212,6 +223,7 @@
         }
 
         pBreak = &pSet->breakpoints[pSet->count++];
+        pBreak->method = method;
         pBreak->addr = (u2*)addr;
         pBreak->originalOpCode = *(u1*)addr;
         pBreak->setCount = 1;
@@ -219,16 +231,35 @@
         /*
          * Change the opcode.  We must ensure that the BreakpointSet
          * updates happen before we change the opcode.
+         *
+         * If the method has not been verified, we do NOT insert the
+         * breakpoint yet, since that will screw up the verifier.  The
+         * debugger is allowed to insert breakpoints in unverified code,
+         * but since we don't execute unverified code we don't need to
+         * alter the bytecode yet.
+         *
+         * The class init code will "flush" all relevant breakpoints when
+         * verification completes.
          */
         MEM_BARRIER();
         assert(*(u1*)addr != OP_BREAKPOINT);
-        dvmDexChangeDex1(method->clazz->pDvmDex, (u1*)addr, OP_BREAKPOINT);
+        if (dvmIsClassVerified(method->clazz)) {
+            LOGV("Class %s verified, adding breakpoint at %p\n",
+                method->clazz->descriptor, addr);
+            dvmDexChangeDex1(method->clazz->pDvmDex, (u1*)addr, OP_BREAKPOINT);
+        } else {
+            LOGV("Class %s NOT verified, deferring breakpoint at %p\n",
+                method->clazz->descriptor, addr);
+        }
     } else {
         pBreak = &pSet->breakpoints[idx];
         pBreak->setCount++;
 
-        /* verify instruction stream has break op */
-        assert(*(u1*)addr == OP_BREAKPOINT);
+        /*
+         * Instruction stream may not have breakpoint opcode yet -- flush
+         * may be pending during verification of class.
+         */
+        //assert(*(u1*)addr == OP_BREAKPOINT);
     }
 
     return true;
@@ -262,6 +293,11 @@
         if (pBreak->setCount == 1) {
             /*
              * Must restore opcode before removing set entry.
+             *
+             * If the breakpoint was never flushed, we could be ovewriting
+             * a value with the same value.  Not a problem, though we
+             * could end up causing a copy-on-write here when we didn't
+             * need to.  (Not worth worrying about.)
              */
             dvmDexChangeDex1(method->clazz->pDvmDex, (u1*)addr,
                 pBreak->originalOpCode);
@@ -282,58 +318,30 @@
 }
 
 /*
- * Restore the original opcode on any breakpoints that are in the specified
- * method.  The breakpoints are NOT removed from the set.
+ * Flush any breakpoints associated with methods in "clazz".  We want to
+ * change the opcode, which might not have happened when the breakpoint
+ * was initially set because the class was in the process of being
+ * verified.
  *
  * The BreakpointSet's lock must be acquired before calling here.
  */
-static void dvmBreakpointSetUndo(BreakpointSet* pSet, Method* method)
+static void dvmBreakpointSetFlush(BreakpointSet* pSet, ClassObject* clazz)
 {
-    const u2* start = method->insns;
-    const u2* end = method->insns + dvmGetMethodInsnsSize(method);
-
     int i;
     for (i = 0; i < pSet->count; i++) {
         Breakpoint* pBreak = &pSet->breakpoints[i];
-        if (pBreak->addr >= start && pBreak->addr < end) {
-            LOGV("UNDO %s.%s [%d]\n",
-                method->clazz->descriptor, method->name, i);
-            dvmDexChangeDex1(method->clazz->pDvmDex, (u1*)pBreak->addr,
-                pBreak->originalOpCode);
+        if (pBreak->method->clazz == clazz) {
+            /*
+             * The breakpoint is associated with a method in this class.
+             * It might already be there or it might not; either way,
+             * flush it out.
+             */
+            LOGV("Flushing breakpoint at %p for %s\n",
+                pBreak->addr, clazz->descriptor);
+            dvmDexChangeDex1(clazz->pDvmDex, (u1*)pBreak->addr, OP_BREAKPOINT);
         }
     }
 }
-
-/*
- * Put the breakpoint opcode back into the instruction stream, and check
- * to see if the original opcode has changed.
- *
- * The BreakpointSet's lock must be acquired before calling here.
- */
-static void dvmBreakpointSetRedo(BreakpointSet* pSet, Method* method)
-{
-    const u2* start = method->insns;
-    const u2* end = method->insns + dvmGetMethodInsnsSize(method);
-
-    int i;
-    for (i = 0; i < pSet->count; i++) {
-        Breakpoint* pBreak = &pSet->breakpoints[i];
-        if (pBreak->addr >= start && pBreak->addr < end) {
-            LOGV("REDO %s.%s [%d]\n",
-                method->clazz->descriptor, method->name, i);
-            u1 currentOpCode = *(u1*)pBreak->addr;
-            if (pBreak->originalOpCode != currentOpCode) {
-                /* verifier can drop in a throw-verification-error */
-                LOGD("NOTE: updating originalOpCode from 0x%02x to 0x%02x\n",
-                    pBreak->originalOpCode, currentOpCode);
-                pBreak->originalOpCode = currentOpCode;
-            }
-            dvmDexChangeDex1(method->clazz->pDvmDex, (u1*)pBreak->addr,
-                OP_BREAKPOINT);
-        }
-    }
-}
-
 #endif /*WITH_DEBUGGER*/
 
 
@@ -428,32 +436,22 @@
 }
 
 /*
- * Temporarily "undo" any breakpoints set in a specific method.  Used
- * during verification.
+ * Flush any breakpoints associated with methods in "clazz".
  *
- * Locks the breakpoint set, and leaves it locked.
+ * We don't want to modify the bytecode of a method before the verifier
+ * gets a chance to look at it, so we postpone opcode replacement until
+ * after verification completes.
  */
-void dvmUndoBreakpoints(Method* method)
+void dvmFlushBreakpoints(ClassObject* clazz)
 {
     BreakpointSet* pSet = gDvm.breakpointSet;
 
+    if (pSet == NULL)
+        return;
+
+    assert(dvmIsClassVerified(clazz));
     dvmBreakpointSetLock(pSet);
-    dvmBreakpointSetUndo(pSet, method);
-    /* lock remains held */
-}
-
-/*
- * "Redo" the breakpoints cleared by a previous "undo", re-inserting the
- * breakpoint opcodes and updating the "original opcode" values.
- *
- * Unlocks the breakpoint set, which must be held by a previous "undo".
- */
-void dvmRedoBreakpoints(Method* method)
-{
-    BreakpointSet* pSet = gDvm.breakpointSet;
-
-    /* lock already held */
-    dvmBreakpointSetRedo(pSet, method);
+    dvmBreakpointSetFlush(pSet, clazz);
     dvmBreakpointSetUnlock(pSet);
 }
 #endif
diff --git a/vm/interp/Interp.h b/vm/interp/Interp.h
index 015c3db..1964b57 100644
--- a/vm/interp/Interp.h
+++ b/vm/interp/Interp.h
@@ -56,20 +56,9 @@
 u1 dvmGetOriginalOpCode(const u2* addr);
 
 /*
- * Temporarily "undo" any breakpoints set in a specific method.  Used
- * during verification.
- *
- * Locks the breakpoint set, and leaves it locked.
+ * Flush any breakpoints associated with methods in "clazz".
  */
-void dvmUndoBreakpoints(Method* method);
-
-/*
- * "Redo" the breakpoints cleared by a previous "undo", re-inserting the
- * breakpoint opcodes and updating the "original opcode" values.
- *
- * Unlocks the breakpoint set, which must be held by a previous "undo".
- */
-void dvmRedoBreakpoints(Method* method);
+void dvmFlushBreakpoints(ClassObject* clazz);
 #endif
 
 #endif /*_DALVIK_INTERP_INTERP*/
diff --git a/vm/oo/Class.c b/vm/oo/Class.c
index 0b03d94..625a572 100644
--- a/vm/oo/Class.c
+++ b/vm/oo/Class.c
@@ -4286,8 +4286,10 @@
             (gDvm.classVerifyMode == VERIFY_MODE_REMOTE &&
              clazz->classLoader == NULL))
         {
+            /* advance to "verified" state */
             LOGV("+++ not verifying class %s (cl=%p)\n",
                 clazz->descriptor, clazz->classLoader);
+            clazz->status = CLASS_VERIFIED;
             goto noverify;
         }
 
@@ -4321,6 +4323,11 @@
     }
 noverify:
 
+#ifdef WITH_DEBUGGER
+    /* update instruction stream now that the verifier is done */
+    dvmFlushBreakpoints(clazz);
+#endif
+
     if (clazz->status == CLASS_INITIALIZED)
         goto bail_unlock;