610793 - JM: Support single-stepping with Firebug in compiled code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

There are a bunch of bugs that are tangential to this, but for some reason we don't have an official bug on it yet. So this bug is to make single-stepping methodjit-compiled code work with Firebug by any means necessary.

Comment 2

[Steve Fink [:sfink] [:s:]]

Someone check me here.

I see three main options:

Option 1. Implement interruptHook for JM (this would be bug 609663)
 - simple to implement the calling half (just prefix the code for every op with a fixed prolog)
 - I don't know how to allow the interruptHook to continue/return/return a value. I didn't think JM liked exiting to the interpreter at arbitrary points, but I could be wrong.
 - the resulting code is kind of silly. It may as well be an interpreter if it has to maintain completely consistent state all the time.

Option 2. Set a trap on every source line and recompile
 - simple implementation, now that we have the recompiler
 - have to catch function calls, returns, and nonlocal exceptions (the local ones should hit a line trap)
 - it seems like the PC for a line is not unique, so you'd really have to set a breakpoint on the first PC for a line and every PC that is a jump target for that line. ('for (x = 0; x < foo(); x++)' should stop every time through the loop, no?)

Option 3. Fall back to the interpreter
 - first variant of this is to disable JM entirely before entering any method that you want to be able to single-step through.
   - which would be unfortunately, because JM can handle a lot of the other debugger APIs, even ones TM can't.
 - second variant is to switch to the interpreter when a single-step is requested
   - is this simple/doable? Can you bail out of JM at all the places where you might want to start a single-step?

So option 2 seems preferable to me right now. There's also dmandelin's minimalist variant, where you have a way of identifying just the PCs of the lines that could possibly follow the current one (plus exception targets etc), so you only have to set breakpoints or traps on those. That feels like a bit more complexity API-wise, so I'm leaning towards the every-line option right now.

When starting in on this, I was attempting to use firebug 1.7 as the test (or at least the example), but I ran into a problem: even after disabling both JITs, it doesn't seem to be getting correct line <-> PC translations. The script pane shows which lines you can set breakpoints on, and they're completely wrong. Firebug 1.5.4 running on Firefox 3.6.12 gets it right. I don't know if this is Firebug 1.7 or FF4 messing things up; I tried dumping src notes for a similar script in the js shell, and they looked ok, but I don't know how to do that for a toplevel script so my results are probably meaningless.

As a side comment, the whole line <-> PC mapping as seen through the jsd API seems very suboptimal for large scripts or functions. If you just want to annotate each line with whether it's breakable or not, it seems like you're quadratic on a linear problem, and not in a good way -- you end up doing both js_PCToLineNumber and js_LineNumberToPC, each of which is a linear scan through the source notes (and line->pc can't even stop early), for every line. I guess that's probably news to nobody, though.

Comment 3

[John J. Barton]

(In reply to comment #2)
> When starting in on this, I was attempting to use firebug 1.7 as the test (or
> at least the example), but I ran into a problem: even after disabling both
> JITs, it doesn't seem to be getting correct line <-> PC translations. The
> script pane shows which lines you can set breakpoints on, and they're
> completely wrong. Firebug 1.5.4 running on Firefox 3.6.12 gets it right. I
> don't know if this is Firebug 1.7 or FF4 messing things up; I tried dumping src
> notes for a similar script in the js shell, and they looked ok, but I don't
> know how to do that for a toplevel script so my results are probably
> meaningless.

Please try Firebug 1.7 on Firefox 3.6.12. I believe you will find the line numbers correct.

If you open Firebug > Firebug Icon Menu > Open Tracing and set Options FBS_CREATION and FBS_SRCUNITS you will see debug tracing with the line ranges from jsd. You will want to turn FBS_CREATION off directly after since otherwise the browser will become non-responsive the next time you start up as all XUL scripts are logged.

Comment 4

[Brendan Eich [:brendan]]

Is this but about step-over but not step-into? It's not clear from the option 2 description.

If only the API were phrased in user-level task terms. Unfortunately it was long ago lowered to interruptHook, which is powerful but too low-level and tied to the interpreter. Some higher-level API work could help separate concerns for all of jsdbgapi, jsd, and jsd's debugger clients.

/be

Comment 5

[John J. Barton]

We use CallHook to trap call/return; we could use throwHook to trap exception (I'm not sure why we don't have to do that in FF3.6).

Comment 6

[Steve Fink [:sfink] [:s:]]

(In reply to comment #4)
> Is this bug about step-over but not step-into? It's not clear from the option 2
> description.

step-over and step-into. Both require extra hooks to complete the user-level functionality (CallHook and ThrowHook, at least).

But not break-on-next or whatever it's called. I haven't looked into that. I guess it uses ExecuteHook or something?

> If only the API were phrased in user-level task terms. Unfortunately it was
> long ago lowered to interruptHook, which is powerful but too low-level and tied
> to the interpreter. Some higher-level API work could help separate concerns for
> all of jsdbgapi, jsd, and jsd's debugger clients.
> 
> /be

I agree, and could suggest several more pieces of the API that would be more usefully done in user-level terms (not just for ease of use but also allow them to be more efficient -- see the line <--> PC stuff above.) But I also assume that this is a matter for jsdv2, and that it's not the right time to be mutating jsdv1.

Comment 7

[John J. Barton]

(In reply to comment #6)
> 
> But not break-on-next or whatever it's called. I haven't looked into that. I
> guess it uses ExecuteHook or something?

See https://bugzilla.mozilla.org/show_bug.cgi?id=609663#c17

Comment 8

[Steve Roussey (:sroussey)]

> But I also assume that this is a matter for jsdv2, and that 
> it's not the right time to be mutating jsdv1.

Would rather have things now then at some obscure point far in the future. But after looking at the hg log for JSD files, I think expectations should be set on just fixing the issue at hand.

If you have suggestions for C code stuff that would make things faster, perhaps Firebug could include its own copy of JSD (or JSD helper routines) in binary form (at least for Windows). That could be iterated over independently.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> > But I also assume that this is a matter for jsdv2, and that 
> > it's not the right time to be mutating jsdv1.
> 
> Would rather have things now then at some obscure point far in the future. But
> after looking at the hg log for JSD files, I think expectations should be set
> on just fixing the issue at hand.

Agreed, and that may want some interpreter-only mode to preserve jsd compat.

> If you have suggestions for C code stuff that would make things faster, perhaps
> Firebug could include its own copy of JSD (or JSD helper routines) in binary
> form (at least for Windows). That could be iterated over independently.

Binary components are always trouble. What's more, we want to revise the JSD/JS-engine internal API freely, while keeping JSD (for now) or JSD2 (later; it may be extensible in JSON protocol terms that transcend these binary API compatibility worries) relatively stable.

So it doesn't make sense to put the moving parts in binary in Firebug. We want those in Firefox (libxul) with less moving or stable APIs for debuggers to use.

We will ship JSD2 as soon as it's ready. Cc'ing sayrer.

/be

Comment 10

[Steve Fink [:sfink] [:s:]]

I've run up against an obstacle.

Just to try it out, I hacked in something like option 1 (implement full-on interruptHook), though I had to force it to be on all the time because of the timing. That more or less works (as in, I can successfully single-step, no firebug code changes needed, but it mangles *all* mjitted scripts.)

My next attempt was to implement option 2 (set a trap on every line and recompile). But I can't figure out how to do it without JSD API changes. I was thinking I'd just make interruptHook work the same as before but only give you a callback for a subset of PCs. But when you set the interruptHook, you don't pass in the script (unlike when setting a breakpoint.) I should be able to do a fairly minimal API change and add a 'singleStep()' right alongside 'setBreakpoint(pc)'. Or call it 'setBreakpointsOnAllLines()'. Are such API changes on the table right now?

With an API change, I could also go back to option 1 and add a setInterruptHookForJaegerMonkeyBecauseItsDifferent(script) API call.

The difference is whether the interrupt hook is called or the breakpoint hook, and how many callbacks are fired (one per PC or one-ish per line).

Or I could go nuts and add a separate single-stepping API with its own callback hooks. Or add an API to return a list of PCs for all the lines in the script and let the caller iterate over them (lots of recompiles! Without even more changes.)

Then there's always the option to throw you back to the interpreter and not do anything in JM. But I don't know how to do that with the current API either.

How much API munging are we ok with?

Comment 11

[John J. Barton]

(In reply to comment #10)
> How much API munging are we ok with?

From the Firebug side, if you munge it, we will use it.

Comment 12

[Brendan Eich [:brendan]]

We should change the API in ways that make sense. Don't keep old entry points, just add script if missing, e.g. And singleStep sounds good to me.

/be

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: Imlement full interruptMode in the most expensive way possible

While wandering through data structures, I discovered that it's *possible* to implement this with no API change at all. At least in my limited testing, the attached patch allows single-stepping with existing versions of Firebug.

This is PoC only (proof of concept, or piece of crap). It handles setting the interrupt hook by digging through all scripts in all contexts within the current runtime. Any script that isn't already compiled for "interrupt mode", and is used by a context that (now) has the interrupt hook set, will be recompiled.

The recompilation makes every op a safe point and invokes the interruptHook before each op.

That wouldn't even be so bad, but it turns out that the interrupt hook is constantly enabled and disabled (by jsd; it's protecting against some unsafe nestings), so lots of scripts will end up getting recompiled during every step.

Btw, singleStep() as an API entry name isn't quite right, because it doesn't actually *do* the single stepping; it only sets things up so that single stepping callbacks will be invoked after you return, allowing execution to continue.

I'll come up with a more realistic patch, with a new API.

Comment 14

[Steve Fink [:sfink] [:s:]]

Attachment: Add JSD script method enableSingleStepInterrupts

Setting the JSD interruptHook does not work for JM code (the hook will not be called.) Add a per-script enableSingleStepInterrupts() that in debug mode will cause JM to call the hook at least once per source line.

This should be called after setting or clearing the interrupt hook.

It reuses the SetTrap infrastructure, but uses its own stubs::SingleStep callback rather than stubs::Trap, just so it can call the interrupt hook instead of the trap hook. Reusing stubs::Trap is probably better (perhaps with a param saying which hook to call?), but I wanted to get the basic mechanism reviewed first.

I apologize for cluttering up generateMethod(). Maybe I should make an iterator.

Comment 15

[Steve Fink [:sfink] [:s:]]

Attachment: Patch to firebug-1.7 to use the new API

For reference, here's the patch to Firebug 1.7 that makes use of the enableSingleStepInterrupts API. At least in my minimal test case, single-stepping works for me.

Comment 16

[John J. Barton]

I guess we need to track the jsdIScript(s) we enableSingleStepInterrupts(), unless we determine for sure that the frame.script value at unhookInterrupts === the one at hookInterrupts (considering function call/return and exceptions).

Comment 17

[David Mandelin [:dmandelin]]

Comment on attachment 490679 [details] [diff] [review]
Add JSD script method enableSingleStepInterrupts

Well, I'm not an expert on source notes, but from what I do understand, it looks pretty good. I do have a few factoring requests, though.

>diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp
>+JS_FRIEND_API(JSBool)
>+js_SetSingleStepMode(JSContext *cx, JSScript *script, JSBool singleStep)
>+{
>+    if (script->singleStepMode == (bool) singleStep)
>+        return JS_TRUE;
>+
>+    JS_ASSERT_IF(singleStep, cx->compartment->debugMode);
>+
>+#ifdef JS_METHODJIT
>+    js::mjit::JITScript *jit = script->jitNormal ? script->jitNormal : script->jitCtor;
>+

I suspect this part reads slightly cleaner if you set script->singleStepMode
only after success, but it's up to you.

>+    script->singleStepMode = (bool) singleStep;
>+    if (jit && (bool) script->singleStepMode != jit->singleStepMode) {
>+        js::mjit::Recompiler recompiler(cx, script);
>+        if (!recompiler.recompile()) {
>+            script->singleStepMode = (bool) !singleStep;
>+            return JS_FALSE;
>+        }
>+    }
>+#endif
>+    return JS_TRUE;
>+}

>diff --git a/js/src/jsdbgapi.h b/js/src/jsdbgapi.h
>--- a/js/src/jsdbgapi.h
>+++ b/js/src/jsdbgapi.h

Copy/paste typo in comment:

>+/* Turn on debugging mode. */
>+extern JS_PUBLIC_API(JSBool)
>+JS_SetSingleStepMode(JSContext *cx, JSScript *script, JSBool singleStep);

>diff --git a/js/src/jsscript.h b/js/src/jsscript.h
>--- a/js/src/jsscript.h
>+++ b/js/src/jsscript.h
>@@ -240,16 +240,17 @@ struct JSScript {
>     bool            compileAndGo:1;   /* script was compiled with TCF_COMPILE_N_GO */
>     bool            usesEval:1;       /* script uses eval() */
>     bool            usesArguments:1;  /* script uses arguments */
>     bool            warnedAboutTwoArgumentEval:1; /* have warned about use of
>                                                      obsolete eval(s, o) in
>                                                      this script */
> #ifdef JS_METHODJIT
>     bool            debugMode:1;      /* script was compiled in debug mode */

I think this should be formatted to be within 79 cols, like warnedAboutTwoArgumentEval. Unless we have changed that rule?

>+    bool            singleStepMode:1; /* script should be compiled in single-step mode */

>diff --git a/js/src/methodjit/Compiler.cpp b/js/src/methodjit/Compiler.cpp
>--- a/js/src/methodjit/Compiler.cpp
>+++ b/js/src/methodjit/Compiler.cpp

> CompileStatus
> mjit::Compiler::generateMethod()
> {
>     mjit::AutoScriptRetrapper trapper(cx, script);
> 
>+    jssrcnote *sn = script->notes();
>+    ptrdiff_t offset = 0;
>+    
>     for (;;) {
>         JSOp op = JSOp(*PC);
>         bool trap = (op == JSOP_TRAP);
> 
>         if (trap) {
>             if (!trapper.untrap(PC))
>                 return Compile_Error;
>             op = JSOp(*PC);
>@@ -820,26 +825,58 @@ mjit::Compiler::generateMethod()
>                 break;
>             if (js_CodeSpec[op].length != -1)
>                 PC += js_CodeSpec[op].length;
>             else
>                 PC += js_GetVariableBytecodeLength(PC);
>             continue;
>         }
> 

This piece looks like it might want to be cut out as a helper function
(possibly with helper class to hold notes/offset state):

>+        /*
>+         * In single step mode, check if op is the first one for a
>+         * source line. This is unnecessary if this op is a jump
>+         * target, since we'll always single step for those, and we
>+         * can skip over the source notes slightly faster when
>+         * processing the next op.
>+         */
>+        bool firstOpInLine = false;
>+        if (script->singleStepMode && !opinfo->jumpTarget) {
>+            while ((offset < PC - script->code) && !SN_IS_TERMINATOR(sn)) {
>+                offset += SN_DELTA(sn);
>+                sn = SN_NEXT(sn);
>+            }
>+            while ((offset == PC - script->code) && !SN_IS_TERMINATOR(sn)) {
>+                JSSrcNoteType type = (JSSrcNoteType) SN_TYPE(sn);
>+                if (type == SRC_SETLINE || type == SRC_NEWLINE)
>+                    firstOpInLine = true;
>+                
>+                offset += SN_DELTA(sn);
>+                sn = SN_NEXT(sn);
>+            }
>+        }
>+

It might be nice to have this, and the trap stub callouts, as helper functions
as well.

> 
>+        if (script->singleStepMode && (firstOpInLine || opinfo->jumpTarget)) {
>+            prepareStubCall(Uses(0));
>+            masm.move(ImmPtr(PC), Registers::ArgReg1);
>+            Call cl = emitStubCall(JS_FUNC_TO_DATA_PTR(void *, stubs::SingleStep));
>+            InternalCallSite site(masm.callReturnOffset(cl), PC,
>+                                  CallSite::MAGIC_TRAP_ID, true, false);
>+            addCallSite(site);
>+        }
>+
>         if (trap) {
>             prepareStubCall(Uses(0));
>             masm.move(ImmPtr(PC), Registers::ArgReg1);
>             Call cl = emitStubCall(JS_FUNC_TO_DATA_PTR(void *, stubs::Trap));
>             InternalCallSite site(masm.callReturnOffset(cl), PC,
>                                   CallSite::MAGIC_TRAP_ID, true, false);
>             addCallSite(site);
>         } else if (savedTraps && savedTraps[PC - script->code]) {
>diff --git a/js/src/methodjit/MethodJIT.h b/js/src/methodjit/MethodJIT.h
>--- a/js/src/methodjit/MethodJIT.h
>+++ b/js/src/methodjit/MethodJIT.h
>@@ -315,16 +315,17 @@ struct JITScript {
>     ic::GetElementIC *getElems;
>     uint32           nGetElems;
>     ic::SetElementIC *setElems;
>     uint32           nSetElems;
> #endif
>     void            *invokeEntry;       /* invoke address */
>     void            *fastEntry;         /* cached entry, fastest */
>     void            *arityCheckEntry;   /* arity check address */
>+    bool            singleStepMode;     /* compiled in "single step mode" */
> 
>     ~JITScript();
> 
>     bool isValidCode(void *ptr) {
>         char *jitcode = (char *)code.m_code.executableAddress();
>         char *jcheck = (char *)ptr;
>         return jcheck >= jitcode && jcheck < jitcode + code.m_size;
>     }
>diff --git a/js/src/methodjit/StubCalls.cpp b/js/src/methodjit/StubCalls.cpp
>--- a/js/src/methodjit/StubCalls.cpp
>+++ b/js/src/methodjit/StubCalls.cpp
>@@ -1346,16 +1346,58 @@ stubs::Trap(VMFrame &f, jsbytecode *pc)
>         THROW();
> 
>       default:
>         break;
>     }
> }
> 
> void JS_FASTCALL
>+stubs::SingleStep(VMFrame &f, jsbytecode *pc)
>+{
>+    Value rval;
>+
>+    /*
>+     * single step mode may be paused without recompiling by setting the
>+     * interruptHook to NULL.
>+     */
>+    JSInterruptHook hook = f.cx->debugHooks->interruptHook;
>+    if (!hook)
>+        return;
>+
>+    switch (hook(f.cx, f.cx->fp()->script(), pc, Jsvalify(&rval),
>+                 f.cx->debugHooks->interruptHookData))
>+    {
>+      case JSTRAP_THROW:
>+        f.cx->throwing = JS_TRUE;
>+        f.cx->exception = rval;
>+        THROW();
>+
>+      case JSTRAP_RETURN:
>+        f.cx->throwing = JS_FALSE;
>+        f.cx->fp()->setReturnValue(rval);
>+#if (defined(JS_NO_FASTCALL) && defined(JS_CPU_X86)) || defined(_WIN64)
>+        *f.returnAddressLocation() = JS_FUNC_TO_DATA_PTR(void *,
>+                                     f.cx->jaegerCompartment()->forceReturnFastTrampoline());
>+#else
>+        *f.returnAddressLocation() = JS_FUNC_TO_DATA_PTR(void *,
>+                                     f.cx->jaegerCompartment()->forceReturnTrampoline());
>+#endif
>+        break;
>+
>+      case JSTRAP_ERROR:
>+        f.cx->throwing = JS_FALSE;
>+        THROW();
>+
>+      default:
>+        break;
>+    }
>+}
>+
>+void JS_FASTCALL
> stubs::This(VMFrame &f)
> {
>     if (!f.fp()->computeThis(f.cx))
>         THROW();
>     f.regs.sp[-1] = f.fp()->thisValue();
> }
> 
> void JS_FASTCALL
>diff --git a/js/src/methodjit/StubCalls.h b/js/src/methodjit/StubCalls.h
>--- a/js/src/methodjit/StubCalls.h
>+++ b/js/src/methodjit/StubCalls.h
>@@ -46,16 +46,17 @@
> namespace js {
> namespace mjit {
> namespace stubs {
> 
> void JS_FASTCALL This(VMFrame &f);
> JSObject * JS_FASTCALL NewInitArray(VMFrame &f, uint32 count);
> JSObject * JS_FASTCALL NewInitObject(VMFrame &f, uint32 count);
> JSObject * JS_FASTCALL NewArray(VMFrame &f, uint32 len);
>+void JS_FASTCALL SingleStep(VMFrame &f, jsbytecode *pc);
> void JS_FASTCALL Trap(VMFrame &f, jsbytecode *pc);
> void JS_FASTCALL Debugger(VMFrame &f, jsbytecode *pc);
> void JS_FASTCALL Interrupt(VMFrame &f, jsbytecode *pc);
> void JS_FASTCALL InitElem(VMFrame &f, uint32 last);
> void JS_FASTCALL InitProp(VMFrame &f, JSAtom *atom);
> void JS_FASTCALL InitMethod(VMFrame &f, JSAtom *atom);
>

Comment 18

[Steve Fink [:sfink] [:s:]]

Attachment: Add JSD script method enableSingleStepInterrupts

I stumbled over bug 612717 when working on this, but it appears to be unrelated to this patch.

(In reply to comment #17)
> Comment on attachment 490679 [details] [diff] [review]
> Add JSD script method enableSingleStepInterrupts
> 
> Well, I'm not an expert on source notes, but from what I do understand, it
> looks pretty good. I do have a few factoring requests, though.
> 
> >diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp
> >+JS_FRIEND_API(JSBool)
> >+js_SetSingleStepMode(JSContext *cx, JSScript *script, JSBool singleStep)
> >+{
> >+    if (script->singleStepMode == (bool) singleStep)
> >+        return JS_TRUE;
> >+
> >+    JS_ASSERT_IF(singleStep, cx->compartment->debugMode);
> >+
> >+#ifdef JS_METHODJIT
> >+    js::mjit::JITScript *jit = script->jitNormal ? script->jitNormal : script->jitCtor;
> >+
> 
> I suspect this part reads slightly cleaner if you set script->singleStepMode
> only after success, but it's up to you.

I can't. script->singleStepMode is what tells it to recompile in single-step mode. I added a comment.

> >diff --git a/js/src/jsdbgapi.h b/js/src/jsdbgapi.h
> >--- a/js/src/jsdbgapi.h
> >+++ b/js/src/jsdbgapi.h
> 
> Copy/paste typo in comment:
> 
> >+/* Turn on debugging mode. */
> >+extern JS_PUBLIC_API(JSBool)
> >+JS_SetSingleStepMode(JSContext *cx, JSScript *script, JSBool singleStep);

Not only that, but the previous comment was flat wrong. Fixed.

> >diff --git a/js/src/jsscript.h b/js/src/jsscript.h
> >--- a/js/src/jsscript.h
> >+++ b/js/src/jsscript.h
> >@@ -240,16 +240,17 @@ struct JSScript {
> >     bool            compileAndGo:1;   /* script was compiled with TCF_COMPILE_N_GO */
> >     bool            usesEval:1;       /* script uses eval() */
> >     bool            usesArguments:1;  /* script uses arguments */
> >     bool            warnedAboutTwoArgumentEval:1; /* have warned about use of
> >                                                      obsolete eval(s, o) in
> >                                                      this script */
> > #ifdef JS_METHODJIT
> >     bool            debugMode:1;      /* script was compiled in debug mode */
> 
> I think this should be formatted to be within 79 cols, like
> warnedAboutTwoArgumentEval. Unless we have changed that rule?
> 
> >+    bool            singleStepMode:1; /* script should be compiled in single-step mode */

Reworded to fit.

> >diff --git a/js/src/methodjit/Compiler.cpp b/js/src/methodjit/Compiler.cpp
> >--- a/js/src/methodjit/Compiler.cpp
> >+++ b/js/src/methodjit/Compiler.cpp
> 
> This piece looks like it might want to be cut out as a helper function
> (possibly with helper class to hold notes/offset state):
> 
> >+        /*
> >+         * In single step mode, check if op is the first one for a
> >+         * source line. This is unnecessary if this op is a jump
> >+         * target, since we'll always single step for those, and we
> >+         * can skip over the source notes slightly faster when
> >+         * processing the next op.
> >+         */
> >+        bool firstOpInLine = false;
> >+        if (script->singleStepMode && !opinfo->jumpTarget) {
> >+            while ((offset < PC - script->code) && !SN_IS_TERMINATOR(sn)) {
> >+                offset += SN_DELTA(sn);
> >+                sn = SN_NEXT(sn);
> >+            }
> >+            while ((offset == PC - script->code) && !SN_IS_TERMINATOR(sn)) {
> >+                JSSrcNoteType type = (JSSrcNoteType) SN_TYPE(sn);
> >+                if (type == SRC_SETLINE || type == SRC_NEWLINE)
> >+                    firstOpInLine = true;
> >+                
> >+                offset += SN_DELTA(sn);
> >+                sn = SN_NEXT(sn);
> >+            }
> >+        }
> >+

Ok, I made a helper class for it.

> It might be nice to have this, and the trap stub callouts, as helper functions
> as well.
> 
> > 
> >+        if (script->singleStepMode && (firstOpInLine || opinfo->jumpTarget)) {
> >+            prepareStubCall(Uses(0));
> >+            masm.move(ImmPtr(PC), Registers::ArgReg1);
> >+            Call cl = emitStubCall(JS_FUNC_TO_DATA_PTR(void *, stubs::SingleStep));
> >+            InternalCallSite site(masm.callReturnOffset(cl), PC,
> >+                                  CallSite::MAGIC_TRAP_ID, true, false);
> >+            addCallSite(site);
> >+        }
> >+
> >         if (trap) {
> >             prepareStubCall(Uses(0));
> >             masm.move(ImmPtr(PC), Registers::ArgReg1);
> >             Call cl = emitStubCall(JS_FUNC_TO_DATA_PTR(void *, stubs::Trap));
> >             InternalCallSite site(masm.callReturnOffset(cl), PC,
> >                                   CallSite::MAGIC_TRAP_ID, true, false);
> >             addCallSite(site);
> >         } else if (savedTraps && savedTraps[PC - script->code]) {

All that duplication was bothering me, and not just because of the code bloat: in one debugger session I stepped through, I stopped on a trap (a firebug breakpoint) and turned on single-stepping. This triggered a recompile, but after the recompile (which added the stubs::SingleStep callout), it continued on and re-executed the trap for the same op. I didn't think that was supposed to happen, but perhaps it's a little dangerous to have multiple disappearing call sites per JSOp? (Note that it didn't actually cause any problems.)

What do you think of the new approach? The updated patch uses stubs::Trap for everything, but overloads it to call either or both of the single-stepping and trap callbacks. It clutters up stubs::Trap, but minimizes gunk in generateMethod() while avoiding the weirdness I talked about above.

Comment 19

[Steve Fink [:sfink] [:s:]]

(In reply to comment #16)
> I guess we need to track the jsdIScript(s) we enableSingleStepInterrupts(),
> unless we determine for sure that the frame.script value at unhookInterrupts
> === the one at hookInterrupts (considering function call/return and
> exceptions).

Good point, I didn't think of that.

I think it's probably better for you to track them externally than to do it within JSD somewhere, though I may be wrong about that. Let me know if you disagree.

In reality, you could just not bother to call enableSingleStepInterrupts(false) at all. Once you clear the interruptHook, you won't be getting callbacks anyway. It would mean that if you set the interruptHook again, you'd start getting interrupts for those scripts, though. And while you're in debug mode, it'd also be slower.

Comment 20

[John J. Barton]

(In reply to comment #19)
> I think it's probably better for you to track them externally...

Sorry I should have said: that was really directed to Honza (Jan Odvarko) in case he tries the patch.  Yes we can easily track the scripts we mark and unmark.

Comment 21

[David Mandelin [:dmandelin]]

Comment on attachment 491008 [details] [diff] [review]
Add JSD script method enableSingleStepInterrupts

This looks pretty good, except for the opaque ints for trap types. That should be a little enum or const int defs. Also, let's put the computation of the trap type all together if possible instead of splitting. Otherwise I think it is ready.

I do have one other concern, which is that the "control system" of having both debug mode and the ability to null the interrupt hook is a bit complex, and could run us into a little bit of trouble with debug mode not getting turned off and perf staying too slow. But I personally don't have the answer today about the "right way" of controlling this--unless you/jjb/Odvarko already know how to do it, we might as well start here and improve/simplify later if needed.

Comment 22

[John J. Barton]

(In reply to comment #21)
...
> 
> I do have one other concern, which is that the "control system" of having both
> debug mode and the ability to null the interrupt hook is a bit complex, ...

Not a problem for us, we already have this (and more). It's not as complex as say DOM event listeners where we don't know how many we have forgotten to remove. Someday it would be nice to have a test like "jitIsABlasting" so we can check our work.

Comment 23

[Steve Fink [:sfink] [:s:]]

Where's a good place to put the trap type enums? jsprvtd.h? (They are shared between Compiler.cpp and StubCalls.cpp.)

Yes, debug mode + interrupt hook + single-step mode is messy. Actually, having all 3 isn't too bad, since they have distinct meanings. The problem is that the meanings are different depending on whether you're running the interpreter or JM.

Interpreter:
  debug mode = no-op (I think?)
  interrupt hook = specify your single step callback, and turn it on
  single step mode = no-op

JM:
  debug mode = enable debugging (aka slow everything down)
  interrupt hook = specify your single step callback
  single step mode = turn it on

Actually, in interpreter mode setting the interrupt hook also turns off TM and therefore could be described as "enable debugging (aka slow everything down)" as well.

I could make setting the interrupt hook not turn interrupts on in interpreter mode either. That would make things more consistent, though slightly less backwards-compatible.

As for accidentally turning off the interrupt hook and slowing things down: should I make that an API usage error? As in, require the interrupt hook to still be set when single step mode is turned off?

Then there's the option of nuking the interrupt hook and enableSingleStepMode, and having a single setSingleStepHook(script, callback). I guess I was just trying to maintain the existing API as much as possible. I'd also have to keep a callback with the script, and so it's more internal churn. But maybe it's the right answer.

Comment 24

[David Mandelin [:dmandelin]]

(In reply to comment #23)
> Where's a good place to put the trap type enums? jsprvtd.h? (They are shared
> between Compiler.cpp and StubCalls.cpp.)

StubCalls.h would be good.

> I could make setting the interrupt hook not turn interrupts on in interpreter
> mode either. That would make things more consistent, though slightly less
> backwards-compatible.

I think it would be good to maintain compatibility on interpreter behavior for now.

> Then there's the option of nuking the interrupt hook and enableSingleStepMode,
> and having a single setSingleStepHook(script, callback). I guess I was just
> trying to maintain the existing API as much as possible. I'd also have to keep
> a callback with the script, and so it's more internal churn. But maybe it's the
> right answer.

The goal is to get it fully working in Fx4 with minimum risk; bigger API changes are planned for later anyway, so doing lots of work to simplify the API seems like the wrong thing right now. So it looks like you are on the right track.

Comment 25

[timeless]

so, casting to bool isn't "the way" to convert things to a bool, please use !foo or !!foo...

also, please bump the iid on jsd .idl files -- including changing interfaces which depend on the interface you're changing:

 [uuid(x1)]
 interface jsdIX {
 void aa();
+void ab();
 }

 [uuid(y1)]
 interface jsdIY {
 void bb(in jsdIX);
 }

bump the uuid for jsdIX because you changed it, and bump jsdIY because you changed something people need to use to pass to it.

Comment 26

[Steve Fink [:sfink] [:s:]]

Ok, thanks, will fix.

There's another problem with the patch. I haven't seen any problems arising from it yet, but in looking into bug 612717 I discovered that stub calls are only allowed one optional argument. So the trapType parameter isn't going to fly. (I'm probably stomping on a register with my current version.)

I don't understand why the preexisting stubs::Trap takes a pc parameter, though. It's invoked with emitStubCall, which makes a fallibleVMCall, which syncs up the PC already. (And it needs to be fallible, so it's not just a missed optimization.) If I could remove that, then I'd be fine. Otherwise, I'd need to make stubs::Trap + stubs::SingleStep + stubs::SingleStepAndTrap, or revert to the previous approach.

Comment 27

[David Mandelin [:dmandelin]]

(In reply to comment #26)
> Ok, thanks, will fix.

I should have caught that, I made that mistake before. But I hack IDL stuff so rarely.

> There's another problem with the patch. I haven't seen any problems arising
> from it yet, but in looking into bug 612717 I discovered that stub calls are
> only allowed one optional argument. So the trapType parameter isn't going to
> fly. (I'm probably stomping on a register with my current version.)
> 
> I don't understand why the preexisting stubs::Trap takes a pc parameter,
> though. It's invoked with emitStubCall, which makes a fallibleVMCall, which
> syncs up the PC already. (And it needs to be fallible, so it's not just a
> missed optimization.) If I could remove that, then I'd be fine. Otherwise, I'd
> need to make stubs::Trap + stubs::SingleStep + stubs::SingleStepAndTrap, or
> revert to the previous approach.

It seems like using the PC from out of the VMFrame should be fine. If for some reason that doesn't work, dvander's been working on generalizing the function call generator. So we can just add that capability if we need it. On the other hand, having 3 separate function entry points doesn't seem that bad.

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I wanted to try out the attached patch with Firebug, but facing some problems.
I am using: http://hg.mozilla.org/tracemonkey + attachment 491008 [details] [diff] [review]

Patch applied successfully, but there are some errors when compiling:

c:/mozilla-src/tracemonkey/js/src/methodjit/Compiler.cpp(791) : warning C4018: '
<' : signed/unsigned mismatch
c:/mozilla-src/tracemonkey/js/src/methodjit/Compiler.cpp(875) : error C2039: 'Ar
gReg2' : is not a member of 'js::mjit::Registers'
        c:\mozilla-src\tracemonkey\js\src\methodjit/MachineRegs.h(50) : see decl
aration of 'js::mjit::Registers'
c:/mozilla-src/tracemonkey/js/src/methodjit/Compiler.cpp(875) : error C2065: 'Ar
gReg2' : undeclared identifier
make[4]: *** [Compiler.obj] Error 2
make[4]: Leaving directory `/c/mozilla-src/tracemonkey/_obj-browser-debug/js/src
'
make[3]: *** [libs_tier_js] Error 2
make[3]: Leaving directory `/c/mozilla-src/tracemonkey/_obj-browser-debug'
make[2]: *** [tier_js] Error 2
make[2]: Leaving directory `/c/mozilla-src/tracemonkey/_obj-browser-debug'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/c/mozilla-src/tracemonkey/_obj-browser-debug'
make: *** [build] Error 2

Should I apply the patch to different branch?
I tried http://hg.mozilla.org/projects/jaegermonkey/
but couldn't apply the patch without errors.

Hunk #1 FAILED at 79.
1 out of 1 hunk FAILED -- saving rejects to file js/src/jsscript.cpp.rej
Hunk #5 FAILED at 909.
1 out of 5 hunks FAILED -- saving rejects to file js/src/methodjit/Compiler.cpp.
rej
Hunk #1 FAILED at 46.
1 out of 1 hunk FAILED -- saving rejects to file js/src/methodjit/StubCalls.h.re
j

Honza

Comment 29

[Steve Fink [:sfink] [:s:]]

Attachment: Add JSD script method enableSingleStepInterrupts

(In reply to comment #21)
> Comment on attachment 491008 [details] [diff] [review]
> Add JSD script method enableSingleStepInterrupts
> 
> This looks pretty good, except for the opaque ints for trap types. That should
> be a little enum or const int defs. Also, let's put the computation of the trap
> type all together if possible instead of splitting. Otherwise I think it is
> ready.

Done. It broke a pointless optimization but fixed a bug (it was testing firstOpInline() on the following PC). Good call.

I've been hitting bug 612717 very frequently, with or without this patch, so I've had this bug on the backburner to fight with the crash.

(In reply to comment #25)
> so, casting to bool isn't "the way" to convert things to a bool, please use
> !foo or !!foo...

Ok, fixed, together with the place where I copied it from.

> also, please bump the iid on jsd .idl files -- including changing interfaces
> which depend on the interface you're changing:
> 
> bump the uuid for jsdIX because you changed it, and bump jsdIY because you
> changed something people need to use to pass to it.

Ugh. Done, which means I changed almost every uuid in the file. I changed jsdIScript, which is used by jsdIValue, which is used by just about everything.

(In reply to comment #28)
> I wanted to try out the attached patch with Firebug, but facing some problems.
> I am using: http://hg.mozilla.org/tracemonkey + attachment 491008 [details] [diff] [review]
> 
> Patch applied successfully, but there are some errors when compiling:
> 
> c:/mozilla-src/tracemonkey/js/src/methodjit/Compiler.cpp(791) : warning C4018:
> '
> <' : signed/unsigned mismatch
> c:/mozilla-src/tracemonkey/js/src/methodjit/Compiler.cpp(875) : error C2039:
> 'Ar
> gReg2' : is not a member of 'js::mjit::Registers'
>         c:\mozilla-src\tracemonkey\js\src\methodjit/MachineRegs.h(50) : see
> decl
> aration of 'js::mjit::Registers'

Oh. That's what I get for compiling on x64. I have more argument registers. But this latest patch will fix that too, since I'm not supposed to be using that many arguments here anyway.

Can you give this patch a shot?

Comment 30

[David Mandelin [:dmandelin]]

Comment on attachment 491916 [details] [diff] [review]
Add JSD script method enableSingleStepInterrupts

Looks great.

Comment 31

[timeless]

> Ugh. Done, which means I changed almost every uuid in the file.

uuid's are cheap. debugging confusing crashes because of vtable corruption is not. -- i'm sorry i didn't invest in a tool to solve this problem 9 years ago, but i didn't.

anyway, looks good

Comment 32

[Damon Sicore (:damons)]

Moving this to blocking last beta.  I think it's really important we have enough time to get feedback.

Comment 33

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I wanted to try the patch with Firebug and so, pulled from tracemonkey branch. When compiling I am seeing the following error:

c:/mozilla-src/tracemonkey/js/src/methodjit/Compiler.cpp(791) : warning C4018: '
<' : signed/unsigned mismatch
c:/mozilla-src/tracemonkey/js/src/methodjit/Compiler.cpp(875) : error C2039: 'Ar
gReg2' : is not a member of 'js::mjit::Registers'
        c:\mozilla-src\tracemonkey\js\src\methodjit/MachineRegs.h(50) : see decl
aration of 'js::mjit::Registers'
c:/mozilla-src/tracemonkey/js/src/methodjit/Compiler.cpp(875) : error C2065: 'Ar
gReg2' : undeclared identifier

Should I use different branch to test the patch?
Honza

Comment 34

[Steve Fink [:sfink] [:s:]]

That looks like you're still using the original patch. Can you make sure you're using the current patch ( https://bugzilla.mozilla.org/attachment.cgi?id=491916 )?

Comment 35

[Steve Fink [:sfink] [:s:]]

Attachment: Add JSD script method enableSingleStepInterrupts, again

Unbitrotted. (I'm a little confused, because it still applied for me, but whatever...)

Comment 36

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Compiling is OK now, thanks!
I am going to test with Firebug...
Honza

Comment 37

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I have tried the patch and I am seeing good progress! I can single step in Firebug even if I have found one problem, described here:
http://getfirebug.com/tests/content/script/singleStepping/single-stepping1.html

The break-on-next doesn't work, but I understand that it's out of scope of this bug. Anyway, for anybody interested a test for it is here:
http://getfirebug.com/tests/content/script/singleStepping/index.html

I have committed a patch for Firebug that uses the new enableSingleStepInterrupts here: http://code.google.com/p/fbug/source/detail?r=8698

If you want Firebug xpi with the fix included let me know. Anyway, the source is here: http://code.google.com/p/fbug/source/browse/branches/firebug1.7

I am also seeing some cases when breakpoints do not hit, but not sure if this is in scope of this report.

Honza

Comment 38

[John J. Barton]

Can we get an update on this bug? Firebug is completely stuck without it and I'm sure we will uncover other bugs if we can get unstuck.

Comment 39

[Steve Fink [:sfink] [:s:]]

Sorry, I should have said this earlier. I've been sitting on this patch until beta8 branches off because JSD triggers a whole bunch of different crashes that are much easier to hit with single-stepping working.

Many of those have been fixed now. beta8 is nearly here, so I plan to commit within the next day or two.

I have not yet looked at the remaining issues Honza pointed out above, but it doesn't sound like they need to block the patch from landing.

Comment 40

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/tracemonkey/rev/9ff7b826eab6

Please open separate bugs for remaining problems.

Comment 41

[John J. Barton]

(In reply to comment #40)
> http://hg.mozilla.org/tracemonkey/rev/9ff7b826eab6
> 
> Please open separate bugs for remaining problems.

We need to get this patch in a Firefox build before we can look for and report the remaining problems.

Comment 42

[Mike Shaver (:shaver emeritus)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-tracemonkey/ has builds that include that patch now. (Except the stalled Win64 ones.)

Comment 43

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to comment #40)
> http://hg.mozilla.org/tracemonkey/rev/9ff7b826eab6
> 
> Please open separate bugs for remaining problems.
Done: bug 619749

Honza