This was midstream when we decided to land in m-c. Here's jimb's initial cut.

Here are some tests. This patch is meant to be qfolded with the onStep patch. A few of these fail. I haven't tried to diagnose the failures, much: * onStep doesn't work when the tracejit kicks in (Frame-onStep-07.js fails with -j or -m -a -j) * The interpreter doesn't check for interrupts properly (Frame-onStep-05.js fails with no jitflags) Others fail too, but 05 is the simplest test for this. * Recursion seems to break the "stepping is per-frame, not per-script" fiction (Frame-onStep-04.js always fails) * Stepping apparently doesn't cooperate with breakpoints in methodjit? (Frame-onStep-08.js fails with -m -a) I renamed the original test to Frame-onStep-lines-01.js.

With the new findReferences shell function, I can write a test that reliably fails because a live onStep handler does not keep the debugger alive. Yay!

Well, not so fast. Of course, JS_TraceRuntime doesn't run the iterative marking loop, so findReferences doesn't work on weak maps or Debugger objects.

The patch series I'm about to attach for this depends somewhat on bug 679136 (already r+) --- "somewhat", because: a) 679136 removes a perf problem (an extra test in every END_CASE in js::Interpret), and b) the patch series here could possibly introduce another one to balance it out (taking the address of jumpTable/switchMask, which would otherwise live in a register). I argue that a) pays for b). :)

The comment atop InterpreterFrames explains why this is needed, although it is only used by later patches in the series.

The rationale is explained in the comments in jsscript.h.

This includes all the tests Jason attached above, and a few new ones.

That's the full series. No regressions in js/src/tests or js/src/jit-test. I've used SunSpider and cachegrind to assess the performance impact of Patch 1, and I've also looked at the machine code generated. If I understand what cachegrind was telling me, random changes in the arrangement of memory (and thus the property cache) have effects that dwarf that of Patch 1. I'm leaving on vacation for a week; Jason Orendorff has kindly offered to shepherd this patch series in my absence.

Comment on attachment 553361 [details] [diff] [review] Patch 1: Maintain a list of active js::Interrupt frames, their FrameRegs, and their interruptors. GenericInterruptEnabler seems like a phony abstraction to me, but it makes the code shorter, so ok.

Comment on attachment 553362 [details] [diff] [review] Patch 2: Have each JSScript maintain both a count and a flag indicating whether it should be in single-step mode. >-/* Turn on single step mode. Requires debug mode. */ >-extern JS_FRIEND_API(JSBool) >-js_SetSingleStepMode(JSContext *cx, JSScript *script, JSBool singleStep); >+/* Increment or decrement |script|'s single step count. Requires debug mode. */ >+extern JSBool >+js_ChangeSingleStepModeCount(JSContext *cx, JSScript *script, int delta); This can just be dropped from jsdbgapi.h/cpp, and then part 4 can call JSScript::changeStepModeCount directly. The assertion in js_ChangeSingleStepModeCount can also be moved to the JSScript method.

I didn't quite finish this review, and more to the point, onStep doesn't seem to work with the methodjit on my machine. I'll look at it over the weekend.

Back to patch 2 for a moment: A lot of the JSScript inlines didn't really need to be inline, so I moved them to JSScript.cpp. recompileForStepMode doesn't need the bool argument; I removed it. I changed JSScript::tryNewStepMode to walk the InterpreterFrames. That simplifies two things in part 4. First, I think CHECK_INTERRUPT_HANDLER doesn't have to check script->stepModeEnabled(). The interpreter only has to check that when the value of |script| changes (inline calls and returns, and unwinding). Second, DebuggerFrame_setOnStep is much shorter. Also, there was a bug: > uint32 prior = stepMode; > /* The compiler checks stepMode, so we need to actually update it now. */ > stepMode = newValue; > if (!stepMode != !newValue && !recompileForStepMode(cx, newValue)) { The condition on the last line can't be true. It should be if (!prior != !newValue && ...

Here's my review of patch 4; however, I obviously missed something because a few tests still crash for me when the methodjit is enabled. :-| In js.msg: > MSG_DEF(JSMSG_DEBUG_BAD_LINE, 283, 0, JSEXN_TYPEERR, "invalid line number") > MSG_DEF(JSMSG_DEBUG_NOT_DEBUGGING, 284, 0, JSEXN_ERR, "can't set breakpoint: script global is not a debuggee") >-MSG_DEF(JSMSG_DEBUG_COMPARTMENT_MISMATCH, 285, 2, JSEXN_TYPEERR, "{0}: descriptor .{1} property is an object in a different compartment than the target object") >-MSG_DEF(JSMSG_NOT_CALLABLE_OR_UNDEFINED, 286, 0, JSEXN_TYPEERR, "value is not a function or undefined") >+MSG_DEF(JSMSG_DEBUG_NOT_SCRIPT_FRAME, 285, 0, JSEXN_ERR, "stack frame is not running JavaScript code") >+MSG_DEF(JSMSG_DEBUG_COMPARTMENT_MISMATCH, 286, 2, JSEXN_TYPEERR, "{0}: descriptor .{1} property is an object in a different compartment than the target object") >+MSG_DEF(JSMSG_NOT_CALLABLE_OR_UNDEFINED, 287, 0, JSEXN_TYPEERR, "value is not a function or undefined") Putting the new entry last reduced the size of the diff a bit. In jsinterp.cpp: > #define CHECK_INTERRUPT_HANDLER() \ > JS_BEGIN_MACRO \ >- if (cx->debugHooks->interruptHook) \ >+ if (cx->debugHooks->interruptHook || script->stepModeEnabled()) \ > ENABLE_INTERRUPTS(); \ > JS_END_MACRO I made changes so that CHECK_INTERRUPT_HANDLER does not have to check for script->stepModeEnabled(), per comment 13 about JSScript::tryNewStepMode. But we do have to check script->stepModeEnabled() each time |script| changes, so I implemented that. Now that we're taking the address of jumpTable/switchMask, maybe we should use the new mechanism everywhere and remove CHECK_INTERRUPT_HANDLER. Filed bug 680562 for that. In methodjit/StubCalls.cpp, stubs::Trap: >+ if (trapTypes & JSTRAP_TRAP) >+ result = Debugger::onTrap(f.cx, &rval); >+ /* If we hit a breakpoint here, don't also report a single-step. */ >+ else if (trapTypes & JSTRAP_SINGLESTEP) >+ result = Debugger::onSingleStep(f.cx, &rval); This differed from the interpreter behavior. The interpreter reported both. For now I changed the methodjit to behave like the interpreter, because that makes Frame-onStep-08.js pass, but we should consider the alternatives. Filed bug 680660 for that. In Debugger.cpp: >+#ifdef DEBUG >+ /* >+ * Validate the single-step count on this frame's script, to ensure that >+ * we're not receiving traps we didn't ask for. Even when frames is >+ * non-empty (and thus we know this trap was requested), do the check >+ * anyway, to make sure the count has the correct non-zero value. >+ * >+ * The converse --- ensuring that we do receive traps when we should --- can >+ * be done with unit tests. >+ */ >+ if (GlobalObject::DebuggerVector *debuggers = global->getDebuggers()) { >+ JSScript *trappingScript = fp->script(); >+ uint32 stepperCount = 0; >+ for (Debugger **p = debuggers->begin(); p != debuggers->end(); p++) { >+ Debugger *dbg = *p; >+ for (FrameMap::Range r = dbg->frames.all(); !r.empty(); r.popFront()) { >+ StackFrame *frame = r.front().key; >+ JSObject *debuggerFrame = r.front().value; >+ if (frame->isScriptFrame() && >+ frame->script() == trappingScript && >+ !debuggerFrame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined()) >+ stepperCount++; >+ } >+ } >+ JS_ASSERT(stepperCount == trappingScript->stepModeCount()); >+ } >+#endif For now, I think we still have the situation where a single (non-compile-n-go) script can be run against multiple different globals. This code only searches for debuggers debugging `global`, which comes from the stack frame's scope chain. I weakened the assertion to account for this. It now only expects an exact match when the script is compileAndGo. >- * Mark Debugger.Frame objects that are reachable from JS if we look them up >- * again (because the corresponding StackFrame is still on the stack). >+ * Mark Debugger.Frame objects. These are all reachable from JS, because the >+ * corresponding StackFrames are still on the stack. >+ * >+ * (Once we support generator frames properly, we will need >+ * weakly-referenced Debugger.Frame objects as well, for suspended generator >+ * frames.) Much better. Thanks. >+static JSBool >+DebuggerFrame_getOnStep(JSContext *cx, uintN argc, Value *vp) >+{ >+ THIS_FRAME(cx, argc, vp, "get onStep", args, thisobj, fp); This throws if the frame is no longer on the stack. Same thing in DebuggerFrame_setOnStep. In combination with slowPathOnLeaveFrame not clearing the onStep handler, this means that once the frame exits, it has a reference to the onStep handler that the program can't get or clear. It's not exactly a leak, since debuggers ordinarily won't keep Frames alive after they leave the stack. Just not ideal. I filed follow-up bug 680549 to fix this. In DebuggerFrame_setOnStep: >+ if (!IsValidHook(vp[2])) { >+ js_ReportIsNotFunction(cx, &vp[2], 0); >+ return false; >+ } This should probably be JSMSG_NOT_CALLABLE_OR_UNDEFINED. I changed it. I also changed code that used vp to use args instead. For example, vp[2] --> args[0]. The rest is tests. >+++ b/js/src/jit-test/lib/findReferences.js I removed this since it's unused so far. In Frame-onStep-04.js: >+dbg.onDebuggerStatement = function (frame) { >+ // This is called with 8 nested frames on the stack. >+ [...] >+g.f(6); "nested" here is kind of embarrassing, so I changed the comment to say // This is called with 8 call frames on the stack, 7 down to 0. and then changed g.f(6) to g.f(7) to make that true. In Frame-onStep-06.js: >+g.log = ''; >+g.eval("'' + x;\n" + >+ "log += 'y';\n"); >+assertEq(g.log, 'dxsy'); >+ >+g.log = ''; >+g.eval("0 + x;\n" + >+ "log += 'y';\n"); >+assertEq(g.log, 'dxsy'); >+ >+g.log = ''; >+g.eval("0 - x;\n" + >+ "log += 'y';\n"); >+assertEq(g.log, 'dxsy'); >+ >+g.log = ''; >+g.eval("0 * x;\n" + >+ "log += 'y';\n"); >+assertEq(g.log, 'dxsy'); >[...] Factoring out some common code made this test a lot clearer. In Frame-onStep-10.js: >+stepped = false; >+g.eval("try { f(); } \n" + >+ "catch (x) { assertEq(x, 'mud'); }; \n"); >+assertEq(stepped, true); This would pass if the catch-block didn't execute. I changed it to check. Of course we pass the tougher test just fine. In Frame-onStep-lines-01.js: >+var d = new Debugger(g); I renamed it to dbg, just for consistency's sake. Also changed indentation to 4 spaces and changed tabs to spaces everywhere. In Frame-onStep-resumption-04.js: >\ No newline at end of file I added one.

Here is a test case that crashes for me with -m -a. I'm not sure how single-step mode is causing this, because the onStep hook is never even called, but we crash in ic::SetGlobalName trying to assign to |caught|. We're looking up the atom in the wrong script, because the StackFrame for f() hasn't been removed from the stack; f.regs.fp() returns that frame. var g = newGlobal('new-compartment'); var dbg = Debugger(g); dbg.onEnterFrame = function (frame) { if (frame.type === 'eval') frame.onStep = function () {}; }; g.eval("function f() { throw 0; }"); g.e = 1; g.eval("try { f(); }\n" + "catch (x) { e = 0; }\n");

Looks like the patch is innocent. I was able to reproduce the assertion in comment 15 without the patch, in fact without jsdbg2, using jsdbgapi.h in a jsapi-test. Filed bug 680684.

http://hg.mozilla.org/integration/mozilla-inbound/rev/d44b0dd28b0d http://hg.mozilla.org/integration/mozilla-inbound/rev/7196b000f9df http://hg.mozilla.org/integration/mozilla-inbound/rev/331146c93ebe http://hg.mozilla.org/integration/mozilla-inbound/rev/4a86c9387193

Backed out. There's a compiler warning and a few test failures; in particular one of the tests had a useless dis() call.

I'm getting a crash in Frame-onStep-06.js that didn't appear on the buildbots. Valgrind shows it reliably (for me on Mac). It looks like js::mjit::Recompiler::recompile() is failing to patch something, because we return to code that uses an IC that we freed. // Reduced from Frame-onStep-06.js. var g = newGlobal('new-compartment'); g.eval("var y, x = {toString: function () { debugger; return '1'; }};"); g.eval("'' + x;"); g.eval("var a = {get f() { debugger; y = 0; return function() { y = 0; }; }}, b = {set x(v) { debugger; return y = 0; }}; "); var dbg = Debugger(g); dbg.onDebuggerStatement = function (frame) { frame.older.onStep = function () { g.y.match(/[sy]$/); }; }; g.eval("b.x = 1;"); g.eval("a.f();"); Invalid read of size 8 at 0x1002F60DE: js::mjit::ic::CallProp(js::VMFram…) (PolyIC.cpp:1902) by 0x68AC500: ??? by 0x100298327: js::mjit::EnterMethodJIT(JSContext*, j…) (MethodJIT.cpp:687) by 0x100298436: CheckStackAndEnterMethodJIT(JSContext*…) (MethodJIT.cpp:717) by 0x100298549: js::mjit::JaegerShot(JSContext*) (MethodJIT.cpp:734) by 0x1000E8BCA: js::RunScript(JSContext*, JSScript*, js…) (jsinterp.cpp:611) by 0x1000E8E2E: js::ExecuteKernel(JSContext*, JSScript*…) (jsinterp.cpp:911) by 0x10010DCC6: EvalKernel(JSContext*, js::CallArgs const…) (jsobj.cpp:1278) by 0x10010DEE3: eval(JSContext*, unsigned int, js::Value*) (jsobj.cpp:1320) by 0x10030C6C5: js::CallJSNative(JSContext*, int (*…) (jscntxtinlines.h:281) by 0x1000E93B2: js::InvokeKernel(JSContext*, js::CallAr…) (jsinterp.cpp:657) by 0x1000662F7: js::Invoke(JSContext*, js::InvokeArgsGuar…) (jsinterp.h:163) Address 0x100ab2c50 is 272 bytes inside a block of size 364 free'd at 0xDCDC: free (vg_replace_malloc.c:320) by 0x1000FCD92: js_free (jsutil.h:262) by 0x100113346: js::Foreground::free_(void*) (jsutil.h:504) by 0x1001A8670: JSRuntime::free_(void*) (jscntxt.h:753) by 0x1001A8694: JSContext::free_(void*) (jscntxt.h:1277) by 0x100298065: js::mjit::ReleaseScriptCode(JSContext*…) (MethodJIT.cpp:934) by 0x10030D984: js::mjit::Recompiler::recompile() (Retcon.cpp:182) by 0x10018DCF2: JSScript::recompileForStepMode(JSConte…) (jsscript.cpp:1841) by 0x10018DD4F: JSScript::tryNewStepMode(JSContext*, u…) (jsscript.cpp:1855) by 0x10018DE69: JSScript::changeStepModeCount(JSContex…) (jsscript.cpp:1886) by 0x1001EFA0B: DebuggerFrame_setOnStep(JSContext*, un…) (Debugger.cpp:2812) by 0x10030C6C5: js::CallJSNative(JSContext*, int (*…) (jscntxtinlines.h:281) I checked that recompile is in fact freeing this memory. Still looking.

This might be related to bug 632343. Cc-ing luke and adrake.

The issue with discarding a script's jitcode (and IC structures) while an ic:: stack frame is manipulating that IC should be fixed after landing TI. With TI the ic:: functions need to monitor for recompilations triggered by VM operations and then back out, finishing the property access / etc. that triggered the ic:: call but not touching the IC itself anymore.

(In reply to Brian Hackett from comment #21) Yep. Filed bug 681672 which might or might not be the bug for that.

Explanation of the bug described in comment 19: On the last line of the script, g.eval("a.f();"); the stack looks like this (most recent call on top): js::mjit::Recompiler::recompile DebuggerFrame_setOnStep <jitcode for 'function (frame) { frame.older.onStep = function () {...}; }'> js::mjit::stubs::DebuggerStatement <jitcode for 'get f() { debugger; y = 0; return function() { y = 0; }; }'> js::InvokeGetterOrSetter js_GetMethod js::mjit::ic::CallProp <jitcode for 'a.f()'> eval The recompiler patches the return to a.f(), but it doesn't patch the PICInfo *pic pointer passed to ic::CallProp(). We eventually return to CallProp, which asks for pic->atom and crashes (or gets NULL or something). I'm going to land onStep anyway, with this particular test commented out. We can't keep delaying onStep until the underlying implementation is perfect--single-step mode is already exposed to chrome; it's just not well-tested. Without onStep, tests for this stuff have to be either jsapi-tests or chrome mochitests. With onStep, they're ordinary jit-tests or js-tests; langfuzz can operate on them. Once TI lands (this weekend), we'll fix this particular issue in bug 681672.

Got a green Try Server run, landing again. http://hg.mozilla.org/integration/mozilla-inbound/rev/2b0b39d1a485 http://hg.mozilla.org/integration/mozilla-inbound/rev/337dc46b17a6 http://hg.mozilla.org/integration/mozilla-inbound/rev/2853df3a5b57 http://hg.mozilla.org/integration/mozilla-inbound/rev/29527b07008d

http://hg.mozilla.org/mozilla-central/rev/2b0b39d1a485 http://hg.mozilla.org/mozilla-central/rev/337dc46b17a6 http://hg.mozilla.org/mozilla-central/rev/2853df3a5b57 http://hg.mozilla.org/mozilla-central/rev/29527b07008d