am 770379e1: am 6efd4463: Fix a deadlock in the breakpoint code.
Merge commit '770379e17e694ec6e08f1edb690a5e33ab81c684' into kraken
* commit '770379e17e694ec6e08f1edb690a5e33ab81c684':
Fix a deadlock in the breakpoint code.
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;
