Closed Bug 673125 (onStep) Opened 8 years ago Closed 8 years ago

Implement Debugger.Frame.prototype.onStep

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: jorendorff, Assigned: jimb)

References

(Blocks 1 open bug)

Details

(Whiteboard: [inbound])

Attachments

(4 files, 2 obsolete files)

Attached patch WIP 1 (obsolete) — Splinter Review
This was midstream when we decided to land in m-c.

Here's jimb's initial cut.
Attachment #547403 - Attachment is patch: true
Attached patch tests (obsolete) — Splinter 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.
Alias: onStep
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). :)
Depends on: 679136
The comment atop InterpreterFrames explains why this is needed, although it
is only used by later patches in the series.
Attachment #547403 - Attachment is obsolete: true
Attachment #547927 - Attachment is obsolete: true
Attachment #553361 - Flags: review?(jorendorff)
The rationale is explained in the comments in jsscript.h.
Attachment #553362 - Flags: review?(jorendorff)
This includes all the tests Jason attached above, and a few new ones.
Attachment #553364 - Flags: review?(jorendorff)
Attachment #553363 - Attachment description: Provide stack-allocated vectors of JSObject pointers. → Patch 3: Provide stack-allocated vectors of JSObject pointers.
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.
Status: NEW → ASSIGNED
Attachment #553363 - Flags: review?(luke) → review+
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.
Attachment #553361 - Flags: review?(jorendorff) → review+
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.
Attachment #553362 - Flags: review?(jorendorff) → review+
Blocks: 680562
Blocks: 680549
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.
Depends on: 680684
Attachment #553364 - Flags: review?(jorendorff) → review+
Backed out. There's a compiler warning and a few test failures; in particular one of the tests had a useless dis() call.
Whiteboard: [inbound]
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.
Blocks: 681672
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.
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.