Last Comment Bug 673125 - (onStep) Implement Debugger.Frame.prototype.onStep
: Implement Debugger.Frame.prototype.onStep
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla9
Assigned To: Jim Blandy :jimb
: Jason Orendorff [:jorendorff]
Depends on: 679136 680684
Blocks: 680549 680562 681672
  Show dependency treegraph
Reported: 2011-07-21 08:23 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-08-25 04:36 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

WIP 1 (15.08 KB, patch)
2011-07-21 08:23 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
tests (13.45 KB, patch)
2011-07-23 06:45 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Patch 1: Maintain a list of active js::Interrupt frames, their FrameRegs, and their interruptors. (5.77 KB, patch)
2011-08-15 21:04 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Patch 2: Have each JSScript maintain both a count and a flag indicating whether it should be in single-step mode. (7.40 KB, patch)
2011-08-15 21:04 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Patch 3: Provide stack-allocated vectors of JSObject pointers. (1.55 KB, patch)
2011-08-15 21:05 PDT, Jim Blandy :jimb
luke: review+
Details | Diff | Splinter Review
Patch 4: Implement the Debugger.Frame.prototype.onStep accessor. (33.75 KB, patch)
2011-08-15 21:07 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review

Description User image Jason Orendorff [:jorendorff] 2011-07-21 08:23:02 PDT
Created attachment 547403 [details] [diff] [review]

This was midstream when we decided to land in m-c.

Here's jimb's initial cut.
Comment 1 User image Jason Orendorff [:jorendorff] 2011-07-23 06:45:12 PDT
Created attachment 547927 [details] [diff] [review]

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.
Comment 2 User image Jim Blandy :jimb 2011-07-28 22:42:42 PDT
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!
Comment 3 User image Jim Blandy :jimb 2011-07-29 09:44:37 PDT
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.
Comment 4 User image Jim Blandy :jimb 2011-08-15 20:17:23 PDT
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). :)
Comment 5 User image Jim Blandy :jimb 2011-08-15 21:04:05 PDT
Created attachment 553361 [details] [diff] [review]
Patch 1: Maintain a list of active js::Interrupt frames, their FrameRegs, and their interruptors.

The comment atop InterpreterFrames explains why this is needed, although it
is only used by later patches in the series.
Comment 6 User image Jim Blandy :jimb 2011-08-15 21:04:57 PDT
Created 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.

The rationale is explained in the comments in jsscript.h.
Comment 7 User image Jim Blandy :jimb 2011-08-15 21:05:45 PDT
Created attachment 553363 [details] [diff] [review]
Patch 3: Provide stack-allocated vectors of JSObject pointers.
Comment 8 User image Jim Blandy :jimb 2011-08-15 21:07:02 PDT
Created attachment 553364 [details] [diff] [review]
Patch 4: Implement the Debugger.Frame.prototype.onStep accessor.

This includes all the tests Jason attached above, and a few new ones.
Comment 9 User image Jim Blandy :jimb 2011-08-15 21:11:38 PDT
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 10 User image Jason Orendorff [:jorendorff] 2011-08-19 05:11:12 PDT
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 11 User image Jason Orendorff [:jorendorff] 2011-08-19 05:44:19 PDT
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.
Comment 12 User image Jason Orendorff [:jorendorff] 2011-08-19 15:15:47 PDT
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.
Comment 13 User image Jason Orendorff [:jorendorff] 2011-08-20 04:48:07 PDT
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 && ...
Comment 14 User image Jason Orendorff [:jorendorff] 2011-08-20 05:21:33 PDT
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();                                            \

I made changes so that CHECK_INTERRUPT_HANDLER does not have to check
for script->stepModeEnabled(), per comment 13 about

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(, &rval);
>+        /* If we hit a breakpoint here, don't also report a single-step. */
>+        else if (trapTypes & JSTRAP_SINGLESTEP)
>+            result = Debugger::onSingleStep(, &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());
>+    }

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.
>+ [...]

"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.
Comment 15 User image Jason Orendorff [:jorendorff] 2011-08-20 05:51:26 PDT
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");
Comment 16 User image Jason Orendorff [:jorendorff] 2011-08-20 10:35:10 PDT
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.
Comment 18 User image Jason Orendorff [:jorendorff] 2011-08-23 14:29:03 PDT
Backed out. There's a compiler warning and a few test failures; in particular one of the tests had a useless dis() call.
Comment 19 User image Jason Orendorff [:jorendorff] 2011-08-24 03:31:23 PDT
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.eval("b.x = 1;");

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.
Comment 20 User image Jason Orendorff [:jorendorff] 2011-08-24 04:11:03 PDT
This might be related to bug 632343. Cc-ing luke and adrake.
Comment 21 User image Brian Hackett (:bhackett) 2011-08-24 09:45:00 PDT
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.
Comment 22 User image Jason Orendorff [:jorendorff] 2011-08-24 11:56:50 PDT
(In reply to Brian Hackett from comment #21)

Yep. Filed bug 681672 which might or might not be the bug for that.
Comment 23 User image Jason Orendorff [:jorendorff] 2011-08-24 13:28:38 PDT
Explanation of the bug described in comment 19:

On the last line of the script,
the stack looks like this (most recent call on top):

   <jitcode for 'function (frame) { frame.older.onStep = function () {...}; }'>
   <jitcode for 'get f() { debugger; y = 0; return function() { y = 0; }; }'>
   <jitcode for 'a.f()'>

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.

Note You need to log in before you can comment on or make changes to this bug.