Change the way breakpoints work.
This replaces the breakpoint mechanism with a more efficient approach.
We now insert breakpoint instructions into the bytecode stream instead of
maintaining a table. This requires mapping DEX files as private instead
of shared, which allows copy-on-write to work. mprotect() is used to
guard the pages against inadvertent writes.
Unused opcode EC is now OP_BREAKPOINT. It's not recognized by dexdump or
any interpreter except portdbg, but it can be encountered by the bytecode
verifier (the debugger can request breakpoints in unverified code).
Breakpoint changes are blocked while the verifier runs to avoid races.
This eliminates method->debugBreakpointCount, which is no longer needed.
(Also, it clashed with LinearAlloc's read-only mode.)
The deferred verification error mechanism was using a code-copying
approach to modify the bytecode stream. That has been changed to use
the same copy-on-write modification mechanism.
Also, normalized all PAGE_SIZE/PAGESIZE references to a single
SYSTEM_PAGE_SIZE define.
Simple Fibonacci computation test times (opal-eng):
JIT, no debugger: 10.6ms
Fast interp, no debugger: 36ms
Portable interp, no debugger: 43.8ms
ORIG debug interp, no breakpoints set: 458ms
ORIG debug interp, breakpoint set nearby: 697ms
NEW debug interp, no breakpoints set: 341ms
NEW debug interp, breakpoints set nearby: 341ms
Where "nearby" means there's a breakpoint in the method doing the
computation that isn't actually hit -- the VM had an optimization where
it flagged methods with breakpoints and skipped some of the processing
when possible.
The bottom line is that code should run noticeably faster while a
debugger is attached.
diff --git a/vm/mterp/out/InterpAsm-armv4t.S b/vm/mterp/out/InterpAsm-armv4t.S
index 56d0a26..a4e5c43 100644
--- a/vm/mterp/out/InterpAsm-armv4t.S
+++ b/vm/mterp/out/InterpAsm-armv4t.S
@@ -7618,8 +7618,8 @@
/* ------------------------------ */
.balign 64
-.L_OP_UNUSED_EC: /* 0xec */
-/* File: armv5te/OP_UNUSED_EC.S */
+.L_OP_BREAKPOINT: /* 0xec */
+/* File: armv5te/OP_BREAKPOINT.S */
/* File: armv5te/unused.S */
bl common_abort
diff --git a/vm/mterp/out/InterpAsm-armv5te-vfp.S b/vm/mterp/out/InterpAsm-armv5te-vfp.S
index c7926f6..3bb5409 100644
--- a/vm/mterp/out/InterpAsm-armv5te-vfp.S
+++ b/vm/mterp/out/InterpAsm-armv5te-vfp.S
@@ -7278,8 +7278,8 @@
/* ------------------------------ */
.balign 64
-.L_OP_UNUSED_EC: /* 0xec */
-/* File: armv5te/OP_UNUSED_EC.S */
+.L_OP_BREAKPOINT: /* 0xec */
+/* File: armv5te/OP_BREAKPOINT.S */
/* File: armv5te/unused.S */
bl common_abort
diff --git a/vm/mterp/out/InterpAsm-armv5te.S b/vm/mterp/out/InterpAsm-armv5te.S
index 3c5c496..52e536b 100644
--- a/vm/mterp/out/InterpAsm-armv5te.S
+++ b/vm/mterp/out/InterpAsm-armv5te.S
@@ -7618,8 +7618,8 @@
/* ------------------------------ */
.balign 64
-.L_OP_UNUSED_EC: /* 0xec */
-/* File: armv5te/OP_UNUSED_EC.S */
+.L_OP_BREAKPOINT: /* 0xec */
+/* File: armv5te/OP_BREAKPOINT.S */
/* File: armv5te/unused.S */
bl common_abort
diff --git a/vm/mterp/out/InterpAsm-armv7-a.S b/vm/mterp/out/InterpAsm-armv7-a.S
index aaf85d9..401bb96 100644
--- a/vm/mterp/out/InterpAsm-armv7-a.S
+++ b/vm/mterp/out/InterpAsm-armv7-a.S
@@ -7222,8 +7222,8 @@
/* ------------------------------ */
.balign 64
-.L_OP_UNUSED_EC: /* 0xec */
-/* File: armv5te/OP_UNUSED_EC.S */
+.L_OP_BREAKPOINT: /* 0xec */
+/* File: armv5te/OP_BREAKPOINT.S */
/* File: armv5te/unused.S */
bl common_abort
diff --git a/vm/mterp/out/InterpAsm-x86.S b/vm/mterp/out/InterpAsm-x86.S
index c25071e..4e6623c 100644
--- a/vm/mterp/out/InterpAsm-x86.S
+++ b/vm/mterp/out/InterpAsm-x86.S
@@ -5797,8 +5797,8 @@
/* ------------------------------ */
.balign 64
-.L_OP_UNUSED_EC: /* 0xec */
-/* File: x86/OP_UNUSED_EC.S */
+.L_OP_BREAKPOINT: /* 0xec */
+/* File: x86/OP_BREAKPOINT.S */
/* File: x86/unused.S */
jmp common_abort
diff --git a/vm/mterp/out/InterpC-allstubs.c b/vm/mterp/out/InterpC-allstubs.c
index 04d7c32..0ca466b 100644
--- a/vm/mterp/out/InterpC-allstubs.c
+++ b/vm/mterp/out/InterpC-allstubs.c
@@ -302,6 +302,11 @@
#define INST_INST(_inst) ((_inst) & 0xff)
/*
+ * Replace the opcode (used when handling breakpoints). _opcode is a u1.
+ */
+#define INST_REPLACE_OP(_inst, _opcode) (((_inst) & 0xff00) | _opcode)
+
+/*
* Extract the "vA, vB" 4-bit registers from the instruction word (_inst is u2).
*/
#define INST_A(_inst) (((_inst) >> 8) & 0x0f)
@@ -2783,8 +2788,35 @@
HANDLE_OPCODE(OP_UNUSED_EB)
OP_END
-/* File: c/OP_UNUSED_EC.c */
-HANDLE_OPCODE(OP_UNUSED_EC)
+/* File: c/OP_BREAKPOINT.c */
+HANDLE_OPCODE(OP_BREAKPOINT)
+#if (INTERP_TYPE == INTERP_DBG) && defined(WITH_DEBUGGER)
+ {
+ /*
+ * Restart this instruction with the original opcode. We do
+ * this by simply jumping to the handler.
+ *
+ * It's probably not necessary to update "inst", but we do it
+ * for the sake of anything that needs to do disambiguation in a
+ * common handler with INST_INST.
+ *
+ * The breakpoint itself is handled over in updateDebugger(),
+ * because we need to detect other events (method entry, single
+ * step) and report them in the same event packet, and we're not
+ * yet handling those through breakpoint instructions. By the
+ * time we get here, the breakpoint has already been handled and
+ * the thread resumed.
+ */
+ u1 originalOpCode = dvmGetOriginalOpCode(pc);
+ LOGV("+++ break 0x%02x (0x%04x -> 0x%04x)\n", originalOpCode, inst,
+ INST_REPLACE_OP(inst, originalOpCode));
+ inst = INST_REPLACE_OP(inst, originalOpCode);
+ FINISH_BKPT(originalOpCode);
+ }
+#else
+ LOGE("Breakpoint hit in non-debug interpreter\n");
+ dvmAbort();
+#endif
OP_END
/* File: c/OP_THROW_VERIFICATION_ERROR.c */
diff --git a/vm/mterp/out/InterpC-armv4t.c b/vm/mterp/out/InterpC-armv4t.c
index 6b82bc8..6541684 100644
--- a/vm/mterp/out/InterpC-armv4t.c
+++ b/vm/mterp/out/InterpC-armv4t.c
@@ -302,6 +302,11 @@
#define INST_INST(_inst) ((_inst) & 0xff)
/*
+ * Replace the opcode (used when handling breakpoints). _opcode is a u1.
+ */
+#define INST_REPLACE_OP(_inst, _opcode) (((_inst) & 0xff00) | _opcode)
+
+/*
* Extract the "vA, vB" 4-bit registers from the instruction word (_inst is u2).
*/
#define INST_A(_inst) (((_inst) >> 8) & 0x0f)
diff --git a/vm/mterp/out/InterpC-armv5te-vfp.c b/vm/mterp/out/InterpC-armv5te-vfp.c
index 7312700..0db6ce7 100644
--- a/vm/mterp/out/InterpC-armv5te-vfp.c
+++ b/vm/mterp/out/InterpC-armv5te-vfp.c
@@ -302,6 +302,11 @@
#define INST_INST(_inst) ((_inst) & 0xff)
/*
+ * Replace the opcode (used when handling breakpoints). _opcode is a u1.
+ */
+#define INST_REPLACE_OP(_inst, _opcode) (((_inst) & 0xff00) | _opcode)
+
+/*
* Extract the "vA, vB" 4-bit registers from the instruction word (_inst is u2).
*/
#define INST_A(_inst) (((_inst) >> 8) & 0x0f)
diff --git a/vm/mterp/out/InterpC-armv5te.c b/vm/mterp/out/InterpC-armv5te.c
index ea11551..b2038f6 100644
--- a/vm/mterp/out/InterpC-armv5te.c
+++ b/vm/mterp/out/InterpC-armv5te.c
@@ -302,6 +302,11 @@
#define INST_INST(_inst) ((_inst) & 0xff)
/*
+ * Replace the opcode (used when handling breakpoints). _opcode is a u1.
+ */
+#define INST_REPLACE_OP(_inst, _opcode) (((_inst) & 0xff00) | _opcode)
+
+/*
* Extract the "vA, vB" 4-bit registers from the instruction word (_inst is u2).
*/
#define INST_A(_inst) (((_inst) >> 8) & 0x0f)
diff --git a/vm/mterp/out/InterpC-armv7-a.c b/vm/mterp/out/InterpC-armv7-a.c
index 97799ec..e1872e1 100644
--- a/vm/mterp/out/InterpC-armv7-a.c
+++ b/vm/mterp/out/InterpC-armv7-a.c
@@ -302,6 +302,11 @@
#define INST_INST(_inst) ((_inst) & 0xff)
/*
+ * Replace the opcode (used when handling breakpoints). _opcode is a u1.
+ */
+#define INST_REPLACE_OP(_inst, _opcode) (((_inst) & 0xff00) | _opcode)
+
+/*
* Extract the "vA, vB" 4-bit registers from the instruction word (_inst is u2).
*/
#define INST_A(_inst) (((_inst) >> 8) & 0x0f)
diff --git a/vm/mterp/out/InterpC-portdbg.c b/vm/mterp/out/InterpC-portdbg.c
index 4d15b45..062eadc 100644
--- a/vm/mterp/out/InterpC-portdbg.c
+++ b/vm/mterp/out/InterpC-portdbg.c
@@ -302,6 +302,11 @@
#define INST_INST(_inst) ((_inst) & 0xff)
/*
+ * Replace the opcode (used when handling breakpoints). _opcode is a u1.
+ */
+#define INST_REPLACE_OP(_inst, _opcode) (((_inst) & 0xff00) | _opcode)
+
+/*
* Extract the "vA, vB" 4-bit registers from the instruction word (_inst is u2).
*/
#define INST_A(_inst) (((_inst) >> 8) & 0x0f)
@@ -448,6 +453,8 @@
*
* Assumes the existence of "const u2* pc" and (for threaded operation)
* "u2 inst".
+ *
+ * TODO: remove "switch" version.
*/
#ifdef THREADED_INTERP
# define H(_op) &&op_##_op
@@ -460,9 +467,13 @@
if (CHECK_JIT()) GOTO_bail_switch(); \
goto *handlerTable[INST_INST(inst)]; \
}
+# define FINISH_BKPT(_opcode) { \
+ goto *handlerTable[_opcode]; \
+ }
#else
# define HANDLE_OPCODE(_op) case _op:
# define FINISH(_offset) { ADJUST_PC(_offset); break; }
+# define FINISH_BKPT(opcode) { > not implemented < }
#endif
#define OP_END
@@ -1170,26 +1181,6 @@
/* code in here is only included in portable-debug interpreter */
/*
- * Determine if an address is "interesting" to the debugger. This allows
- * us to avoid scanning the entire event list before every instruction.
- *
- * The "debugBreakAddr" table is global and not synchronized.
- */
-static bool isInterestingAddr(const u2* pc)
-{
- const u2** ptr = gDvm.debugBreakAddr;
- int i;
-
- for (i = 0; i < MAX_BREAKPOINTS; i++, ptr++) {
- if (*ptr == pc) {
- LOGV("BKP: hit on %p\n", pc);
- return true;
- }
- }
- return false;
-}
-
-/*
* Update the debugger on interesting events, such as hitting a breakpoint
* or a single-step point. This is called from the top of the interpreter
* loop, before the current instruction is processed.
@@ -1235,14 +1226,10 @@
*
* Depending on the "mods" associated with event(s) on this address,
* we may or may not actually send a message to the debugger.
- *
- * Checking method->debugBreakpointCount is slower on the device than
- * just scanning the table (!). We could probably work something out
- * where we just check it on method entry/exit and remember the result,
- * but that's more fragile and requires passing more stuff around.
*/
#ifdef WITH_DEBUGGER
- if (method->debugBreakpointCount > 0 && isInterestingAddr(pc)) {
+ if (INST_INST(*pc) == OP_BREAKPOINT) {
+ LOGV("+++ breakpoint hit at %p\n", pc);
eventFlags |= DBG_BREAKPOINT;
}
#endif
@@ -3164,8 +3151,35 @@
HANDLE_OPCODE(OP_UNUSED_EB)
OP_END
-/* File: c/OP_UNUSED_EC.c */
-HANDLE_OPCODE(OP_UNUSED_EC)
+/* File: c/OP_BREAKPOINT.c */
+HANDLE_OPCODE(OP_BREAKPOINT)
+#if (INTERP_TYPE == INTERP_DBG) && defined(WITH_DEBUGGER)
+ {
+ /*
+ * Restart this instruction with the original opcode. We do
+ * this by simply jumping to the handler.
+ *
+ * It's probably not necessary to update "inst", but we do it
+ * for the sake of anything that needs to do disambiguation in a
+ * common handler with INST_INST.
+ *
+ * The breakpoint itself is handled over in updateDebugger(),
+ * because we need to detect other events (method entry, single
+ * step) and report them in the same event packet, and we're not
+ * yet handling those through breakpoint instructions. By the
+ * time we get here, the breakpoint has already been handled and
+ * the thread resumed.
+ */
+ u1 originalOpCode = dvmGetOriginalOpCode(pc);
+ LOGV("+++ break 0x%02x (0x%04x -> 0x%04x)\n", originalOpCode, inst,
+ INST_REPLACE_OP(inst, originalOpCode));
+ inst = INST_REPLACE_OP(inst, originalOpCode);
+ FINISH_BKPT(originalOpCode);
+ }
+#else
+ LOGE("Breakpoint hit in non-debug interpreter\n");
+ dvmAbort();
+#endif
OP_END
/* File: c/OP_THROW_VERIFICATION_ERROR.c */
diff --git a/vm/mterp/out/InterpC-portstd.c b/vm/mterp/out/InterpC-portstd.c
index 6ea6a56..52014f4 100644
--- a/vm/mterp/out/InterpC-portstd.c
+++ b/vm/mterp/out/InterpC-portstd.c
@@ -302,6 +302,11 @@
#define INST_INST(_inst) ((_inst) & 0xff)
/*
+ * Replace the opcode (used when handling breakpoints). _opcode is a u1.
+ */
+#define INST_REPLACE_OP(_inst, _opcode) (((_inst) & 0xff00) | _opcode)
+
+/*
* Extract the "vA, vB" 4-bit registers from the instruction word (_inst is u2).
*/
#define INST_A(_inst) (((_inst) >> 8) & 0x0f)
@@ -443,6 +448,8 @@
*
* Assumes the existence of "const u2* pc" and (for threaded operation)
* "u2 inst".
+ *
+ * TODO: remove "switch" version.
*/
#ifdef THREADED_INTERP
# define H(_op) &&op_##_op
@@ -455,9 +462,13 @@
if (CHECK_JIT()) GOTO_bail_switch(); \
goto *handlerTable[INST_INST(inst)]; \
}
+# define FINISH_BKPT(_opcode) { \
+ goto *handlerTable[_opcode]; \
+ }
#else
# define HANDLE_OPCODE(_op) case _op:
# define FINISH(_offset) { ADJUST_PC(_offset); break; }
+# define FINISH_BKPT(opcode) { > not implemented < }
#endif
#define OP_END
@@ -2880,8 +2891,35 @@
HANDLE_OPCODE(OP_UNUSED_EB)
OP_END
-/* File: c/OP_UNUSED_EC.c */
-HANDLE_OPCODE(OP_UNUSED_EC)
+/* File: c/OP_BREAKPOINT.c */
+HANDLE_OPCODE(OP_BREAKPOINT)
+#if (INTERP_TYPE == INTERP_DBG) && defined(WITH_DEBUGGER)
+ {
+ /*
+ * Restart this instruction with the original opcode. We do
+ * this by simply jumping to the handler.
+ *
+ * It's probably not necessary to update "inst", but we do it
+ * for the sake of anything that needs to do disambiguation in a
+ * common handler with INST_INST.
+ *
+ * The breakpoint itself is handled over in updateDebugger(),
+ * because we need to detect other events (method entry, single
+ * step) and report them in the same event packet, and we're not
+ * yet handling those through breakpoint instructions. By the
+ * time we get here, the breakpoint has already been handled and
+ * the thread resumed.
+ */
+ u1 originalOpCode = dvmGetOriginalOpCode(pc);
+ LOGV("+++ break 0x%02x (0x%04x -> 0x%04x)\n", originalOpCode, inst,
+ INST_REPLACE_OP(inst, originalOpCode));
+ inst = INST_REPLACE_OP(inst, originalOpCode);
+ FINISH_BKPT(originalOpCode);
+ }
+#else
+ LOGE("Breakpoint hit in non-debug interpreter\n");
+ dvmAbort();
+#endif
OP_END
/* File: c/OP_THROW_VERIFICATION_ERROR.c */
diff --git a/vm/mterp/out/InterpC-x86.c b/vm/mterp/out/InterpC-x86.c
index ed42355..30a4e1c 100644
--- a/vm/mterp/out/InterpC-x86.c
+++ b/vm/mterp/out/InterpC-x86.c
@@ -302,6 +302,11 @@
#define INST_INST(_inst) ((_inst) & 0xff)
/*
+ * Replace the opcode (used when handling breakpoints). _opcode is a u1.
+ */
+#define INST_REPLACE_OP(_inst, _opcode) (((_inst) & 0xff00) | _opcode)
+
+/*
* Extract the "vA, vB" 4-bit registers from the instruction word (_inst is u2).
*/
#define INST_A(_inst) (((_inst) >> 8) & 0x0f)