Jason Orendorff [:jorendorff] Reporter
	Description • 13 years ago

https://wiki.mozilla.org/Debugger

	Jim Blandy :jimb Assignee
	Comment 1 • 13 years ago

I'm implementing Debugger.Frame.prototype.onPop now.

	Jim Blandy :jimb Assignee
	Comment 2 • 13 years ago

I have a Try server run out for this; will post patches now:
https://tbpl.mozilla.org/?tree=Try&rev=058226bffb2a

	Jim Blandy :jimb Assignee
	Comment 3 • 13 years ago

I had a hard time writing a test case for this, so I gave up.

	Jim Blandy :jimb Assignee
	Comment 5 • 13 years ago

This simplifies writing tests, since one can use common structure around
uses of both 'eval' (which produces "eval" frames) and 'evaluate' (which
produces "global" frames).

	Jim Blandy :jimb Assignee
	Comment 6 • 13 years ago

The Debugger implementation has a function (newCompletionValue) that runs
selected code on each side of a compartment transition. At the moment, it
is written to work while leaving a compartment, but the
Debugger.Frame.prototype.onPop implementation would like to be able to use
it when entering a compartment as well.

Giving the 'enter' and 'leave' member functions the same type allows us to
simply pass newCompletionValue a pointer to the appropriate member function.

	Jason Orendorff [:jorendorff] Reporter
	Comment 8 • 13 years ago

Comment on attachment 579776 [details] [diff] [review]
Define Debugger::FrameRange, for iterating over the Debugger.Frame instances referring to a given StackFrame.

Review of attachment 579776 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a few nits

::: js/src/vm/Debugger.cpp
@@ +183,5 @@
> +
> +    const FrameMap::Ptr &frontFrameMapPtr() const {
> +        JS_ASSERT(!empty());
> +        return entry;
> +    }

It seems nicer to have a removeFront() method here. That's the only reason anyone would want this method, right?

@@ +549,4 @@
>      StackFrame *fp = cx->fp();
>      GlobalObject *global = fp->scopeChain().getGlobal();
>  
> +    for (FrameRange r(cx, fp, global); !r.empty(); r.popFront()) {

IIUC you can remove the "global" calculation here, since FrameRange will compute it for you.

@@ -454,5 @@
> -    /*
> -     * FIXME This notifies only current debuggers, so it relies on a hack in
> -     * Debugger::removeDebuggeeGlobal to make sure only current debuggers have
> -     * Frame objects with .live === true.
> -     */

Did you mean to remove this comment? I don't think switching to FrameRange changes this behavior. Maybe you mean that this behavior isn't considered a hack anymore with or without FrameRange? (To me, it still seems like a hack and contrary to the spec.)

@@ +1026,4 @@
>       */
>      AutoObjectVector frames(cx);
>      GlobalObject *global = fp->scopeChain().getGlobal();
> +    for (FrameRange r(cx, fp, global); !r.empty(); r.popFront()) {

Same comment about computing "global" here.

@@ +1029,5 @@
> +    for (FrameRange r(cx, fp, global); !r.empty(); r.popFront()) {
> +        JSObject *frame = r.frontFrame();
> +        if (!frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined() &&
> +            !frames.append(frame))
> +            return JSTRAP_ERROR;

Pre-existing style nit: This should have had curly braces before. Add them?

	Jason Orendorff [:jorendorff] Reporter
	Comment 9 • 13 years ago

Comment on attachment 579777 [details] [diff] [review]
Make the JavaScript shell's 'evaluate' function to return a value.

Frabjous day!

I'm sure some existing tests (in debugger and elsewhere) can be simplified with this change, but don't bother.

I don't think the JS_SET_RVAL line is needed. Remove it?

	Jason Orendorff [:jorendorff] Reporter
	Comment 10 • 13 years ago

Comment on attachment 579780 [details] [diff] [review]
Implement Debugger.Frame.prototype.onPop.

Review of attachment 579780 [details] [diff] [review]:
-----------------------------------------------------------------

These review comments address the tests only. I'll review the C++ tomorrow.

Thinking white-box here, I can think of more things to test. It's up to you how far to go with this, but as it stands the tests look too one-dimensional for my tastes.

- Multiple active frames may have onPop handlers with various values, and they'll be called in the right order.

- Setting .onPop to undefined after it was set to a function value causes the function not to be called when the frame is popped.

- When an exception is thrown across multiple frames, onPop is called on each frame in the order they are popped.

- When an exception is thrown across multiple frames, onPop calls interleave with onExceptionUnwind calls.

- onPop is called correctly when an exception is implicitly rethrown at the end of a finally block.

- dbg.getNewestFrame from a frame's onPop hook returns that frame.

- Setting .onPop on a dead frame throws (I think this is what the spec says; at least it must not crash or assert)

- Setting .onPop from a hook other than onEnterFrame works (particularly from a breakpoint handler, since "step out" will likely make use of that; and from the onExceptionUnwind handler)

- Setting .onPop on a frame other than the newest frame works.

- Setting .onPop on one frame should not affect other frames.

- A frame's .onPop handler is called only once, even if the same function is called several times in a loop.

- Even if the correct .onPop behavior for generator frames is not implemented yet, at least it doesn't crash or do something really astonishing like kill the debuggee.

Also, it would be great to figure out how onPop should interact with proper tail calls and get some tests in place that pass now and should continue to pass when proper tail calls are implemented.

::: js/src/jit-test/tests/debug/Frame-onPop-error-error.js
@@ +57,5 @@
> +test('eval', 'eval(\'debugger; \\\'termination fail\\\';\');');
> +test('global', 'evaluate(\'debugger; \\\'termination fail\\\';\');');
> +
> +// When doing termination tests, it's wise to check that the whole file ran.
> +print('Done!');

Huh. The test runner does flunk us if this test terminates erroneously, with or without the print() call, right? If not, fix it! I think it does, though, looking at check_output() in jt_test.py.

I dunno, what is this print() call for, exactly? Some of the other tests involving termination don't have it. We should have a rule of thumb as to when it's appropriate, if ever. I claim almost all tests (excepting a few that use "|jit-test| error:") are supposed to execute to the end and then pass; and the test runner should (rather strong practical and moral sense of "should" here) catch all forms of failure; and therefore it should take 0 lines of code to state that that is what we expect to have happen.

These "error" tests have the weird quality that if the first onEnterFrame hook fails to trip, they pass. I can live with that.

::: js/src/jit-test/tests/debug/Frame-onPop-multiple-01.js
@@ +2,5 @@
> +
> +function completionsEqual(c1, c2) {
> +    return (c1 === c2 ||
> +            (c1.throw && c1.throw == c2.throw) ||
> +            (c2.return && c1.return == c2.return));

Use the === operator rather than == here?

@@ +23,5 @@
> +// for expectations and responses, so the order in which events get
> +// reported to the debuggers doesn't matter.
> +// 
> +// This list includes every pair of transitions, and is of minimal length.
> +// As if opportunity cost were just some theoretical concern.

You might be pleased to know I did not verify these properties of the list (but if it weren't for the last sentence I probably would have).

@@ +46,5 @@
> +    var dbgs = [];
> +    var log;
> +
> +    // Create a separate debugger to carry out each item in sequence.
> +    for (s in sequence) {

I dunno, it seems plainer to me to write:

>  for (var i = 0; i < sequence.length; i++) {

::: js/src/jit-test/tests/debug/Frame-onPop-multiple-02.js
@@ +1,3 @@
> +// When multiple debuggers have onPop handlers, an uncaught error in one
> +// doesn't prevent the others' onPop handlers from being thrown, but the
> +// debuggee is terminated.

"from being called", not "from being thrown".

I can't find where the spec requires this behavior. Aren't we lying to dbg2 if we do it this way? How does this fit with what the spec requires when a handler function fails (throwing an exception to the debuggee, something we have not implemented yet)?

@@ +38,5 @@
> +// When an error is reported, the shell usually exits with a nonzero exit
> +// code. If we get here, the test passed, so override that behavior. (If we
> +// try to use the "error" jit-test metaline to permit the ReferenceError,
> +// then the test passes whenever a ReferenceError is seen, even if
> +// assertions fail.)

I've written stuff like this too. Is it time for this shell behavior to change?

::: js/src/jit-test/tests/debug/Frame-onPop-return-return.js
@@ +20,5 @@
> +        assertEq(f.type, type);
> +        wasConstructing = f.constructing;
> +        f.onPop = function handlePop(c) {
> +            log += ')';
> +            assertEq(c.return, 'book');

You can also return a compliment (which seems a better parallel to 'favor') or a form, survey, or questionnaire.

	Jason Orendorff [:jorendorff] Reporter
	Comment 11 • 13 years ago

Regarding the print("done!") thing, if we are worried about the tests possibly failing to detect bugs where the engine simply stops executing code and feigns success, then certainly 'print("done!")' alone doesn't address that fear! Someone (meaning, jit_test.py) would have to be expecting that output.

And jit_test.py can certainly do that, with individual tests using a magic
'|jit-test|' comment to opt in (or out). I'll be happy to review such a patch.

	Jason Orendorff [:jorendorff] Reporter
	Comment 12 • 13 years ago

And surfaces:

- Assigning an invalid value to .onPop is a TypeError.

- Debugger.Frame.prototype.onPop === undefined.

- Assigning to Debugger.Frame.prototype.onPop is a TypeError.

- Fishing the getter or setter out with Object.getOwnPropertyDescriptor and
  calling it on a non-Frame object is a TypeError.

- Debugger.Frame.prototype.onPop is configurable and non-enumerable.

And GC:

- A per-compartment GC in the debugger or debuggee doesn't affect .onPop.

- A Frame still holds a strong reference to its .onPop handler even after the frame becomes dead.

	Jason Orendorff [:jorendorff] Reporter
	Comment 13 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #12)
> - Debugger.Frame.prototype.onPop === undefined.

Er, actually maybe that should throw a TypeError.

Three more possible GC tests, covering details of Debugger::slowPathOnLeaveFrame:

- If there are several onPop handlers, and one causes the others to be removed (set to undefined) and then forces GC, the other onPop handlers are not called.

- If there are several Debuggers with onPop handlers, and one causes the others to be disabled, the other onPop handlers are not called.

- If one onPop handler causes a different Debugger to reify a Debugger.Frame object for the frame that is about to be popped, that created-at-the-last-minute Frame still becomes !.live after all the onPop handlers are called. (In other words, slowPathOnLeaveFrame cleans it up properly even though it wasn't in the 'frames' vector.)

Two more possible tests:

- The this-value passed to an onPop handler is the Debugger.Frame object.

- When an onPop hook is called, the Frame is still live. (Even if previous onPop hooks for the same frame were called and changed the completion status.) (The spec doesn't explicitly say that this is the case, but it seems the most likely interpretation.)

	Jason Orendorff [:jorendorff] Reporter
	Comment 14 • 13 years ago

In jsinterp.cpp, js::Interpret:
>     /* State communicated between non-local jumps: */
>-    JSBool interpReturnOK;
>+    bool interpReturnOK;

Great, but also change all the places where we assign JS_TRUE or
JS_FALSE to this variable to use plain old true and false.

In jsinterp.h, comment on ScriptDebugEpilogue:
>+ * This function may be called twice for the same outgoing frame; only the
>+ * first call has any effect. (Permitting double calls simplifies some
>+ * cases where an onPop handler's resumption value changes a return to a
>+ * throw, or vice versa: we can redirect to a complete copy of the
>+ * alternative path, containing its own call to ScriptDebugEpilogue.)

Chatting on IRC I think you came up with a way to eliminate this aspect
of the thing. I think it involved having forceReturn callers explicitly
call ScriptDebugEpilogue rather than having the trampoline code do it. I
think that would be great, for what it's worth.

In methodjit/InvokeHelpers.cpp, js_InternalThrow:
>+        // prologues and epilogues. RunTracer(), Interpret(), and Invoke() all
>         // rely on this property.

While you're in here, remove "RunTracer()" from the comment. That
function is gone.

In Debugger.cpp, Debugger::slowPathOnLeaveFrame:
>+    for (FrameRange r(cx, fp, global); !r.empty(); r.popFront())
>+        if (!frames.append(r.frontFrame())) {
>+            cx->clearPendingException();
>+            return false;
>+        }

Style nit: braces around the if-statement.

>+    /* 
>+     * If any onPop handler produces an uncaught exception, that's something we
>+     * need to relay back no matter what the other handlers do.
>+     */
>+    bool anyUncaught = false;

This seems like a bad hack to me. It complicates both the spec and the
implementation, and it breaks two invariants I can imagine users caring
about: (1) that an onPop handler should never be skipped, except in the
extraordinary case where we OOM trying to call it; and (2) that if the
uncaughtExceptionHook returns continue, the debuggee should not be
disrupted.

Note that in the absence of this feature, an uncaught Debugger exception
can only be bogusly squelched if
  - there are multiple Debuggers with onPop hooks
  - one of the onPop hooks throws, and that Debugger's
    uncaughtExceptionHandler hook does not cope with the exception
  - a later onPop hook for the same frame squelches the exception, which
    by design is hard to do accidentally.

>+            Value completion;
>+            if (!dbg->newCompletionValue(ac, &AutoCompartment::enter, frameOk,
>+                                         fp->returnValue(), &completion)) {
>+                frameStatus = dbg->handleUncaughtException(ac, NULL, false);
>+                break;
>+            }

Hmm. It is unclear at the point where handleUncaughtException is called
whether or not ac was entered.

Prior to the change, newCompletionValue *always* called ac.leave(),
which is infallible, even on error, so the caller knew where matters
stood. With the change, we might be calling ac.enter(), and it might
fail. If that happens we must not call handleUncaughtException; we just
want to set frameStatus appropriately and let the error propagate.

>+            bool hookOk = Invoke(cx, ObjectValue(*frameobj), handler, 1, &completion, &rval);
>+            Value val;
>+            if (hookOk) {
>+                nextStatus = dbg->parseResumptionValue(ac, hookOk, rval, &val);
>+            } else {
>+                nextStatus = dbg->handleUncaughtException(ac, &val, true);
>+                if (nextStatus == JSTRAP_ERROR)
>+                    anyUncaught = true;
>+            }
>+
>+            /* At this point, we are back in the debuggee compartment. */
>+            JS_ASSERT(cx->compartment == global->compartment());

You can also assert that no exception is pending on cx.

>+            /* Prep the context and frame according to the resumption value. */
>+            switch (nextStatus) {
>+              case JSTRAP_CONTINUE:
>+                /* This means 'keep doing whatever we were doing before'. */
>+                switch (frameStatus) {
>+                  case JSTRAP_RETURN:
>+                    break;
>+                  case JSTRAP_THROW:
>+                    /* Put back the exception that newCompletionValue cleared. */
>+                    cx->setPendingException(savedException);
>+                    break;
>+                  case JSTRAP_ERROR:
>+                    cx->clearPendingException();

Since no exception is pending here (see preceding comment) you can delete that last line.

>+              case JSTRAP_ERROR:
>+                frameStatus = JSTRAP_ERROR;
>+                cx->clearPendingException();

Same here.

>+    /* Clean up all Debugger.Frame instances. */
>+    for (JSObject **p = frames.begin(); p != frames.end(); p++) {
>+        JSObject *frameobj = *p;
>+        Debugger *dbg = Debugger::fromChildJSObject(frameobj);
> 
>         frameobj->setPrivate(NULL);
> 
>         /* If this frame had an onStep handler, adjust the script's count. */
>         if (!frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined() &&
>-            fp->isScriptFrame())
>+            fp->isScriptFrame() &&
>+            !fp->script()->changeStepModeCount(cx, -1))
>         {
>-            fp->script()->changeStepModeCount(cx, -1);
>+            frameStatus = JSTRAP_ERROR;
>+            /* Don't exit the loop; we must mark all frames as dead. */
>         }
> 
>-        r.frontDebugger()->frames.remove(r.frontFrameMapPtr());
>+        dbg->frames.remove(fp);

Ooh, the test I described in comment 13, bullet 3 might fail. I think
there could be additional new entries in various Debugger::frames tables
for fp, since JS code has run.

Separately, I think either frameStatus or frameOk should be the single
source of truth throughout this function. Since we let them diverge in
the error case here, and then use frameStatus on the last line, it would
be consistent to use frameOK only to calculate the initial frameStatus,
and never assign to it.

>-Debugger::newCompletionValue(AutoCompartment &ac, bool ok, Value val, Value *vp)
>+Debugger::newCompletionValue(AutoCompartment &ac, bool (AutoCompartment::*transition)(),
>+                             bool ok, Value val, Value *vp)

Changing the return type of AutoCompartment::leave seems wrong. That
would be leaking a yucky implementation detail of the debugger to all
code that uses compartments. It is a potentially misleading thing to
change, too. We definitely don't want people thinking that leave() is
fallible--or, much worse, that it's OK to make it fallible!!

How about this instead. It's maybe 9 more lines of code, but it contains
the grossness better.

>+struct Leaving {
>+    static bool transition(AutoCompartment &ac) { ac.leave(); return true; }
>+};
>+
>+struct Entering {
>+    static bool transition(AutoCompartment &ac) { return ac.enter(); }
>+};
>+
>+template <class Transition>
> bool
> Debugger::newCompletionValue(AutoCompartment &ac, bool ok, Value val, Value *vp)
> {
>...
>-        ac.leave();
>+        if (!Transition::transition(ac))
>+            return false;
>...
> }
> 
...
>-    return dbg->newCompletionValue(ac, ok, rval, vp);
>+    return dbg->newCompletionValue<Leaving>(ac, ok, rval, vp);

Or, perhaps your Curry-Howard Theory of Self-Evidently Correct Code
implies we should common up the three places where ac.leave() is being
called. And then you could just use a bool (or enum) parameter and the
mighty if statement.

    // Query the debuggee's completion status.
    jsid key;
    if (ok) {
        key = ATOM_TO_JSID(cx->runtime->atomState.returnAtom);
    } else if (cx->isExceptionPending()) {
        key = ATOM_TO_JSID(cx->runtime->atomState.throwAtom);
        val = cx->getPendingException();
        cx->clearPendingException();
    } else {
        key = JSID_EMPTY;
    }

    // Cross the compartment boundary from the debuggee to the debugger.
    if (entering) {
        if (!ac.enter())
            return false;
    } else {
        ac.leave();
    }

    // Build the completion value.
    if (key == JSID_EMPTY) {
        vp->setNull();
    } else {
        ...
    }
    return true;

In any case, the comment on Debugger::newCompletionValue in
vm/Debugger.h must be updated to reflect the new contract.

In vm/Debugger.h, Debugger::onEnterFrame:
> {
>     if (cx->compartment->getDebuggees().empty())
>         return JSTRAP_CONTINUE;
>-    return slowPathOnEnterFrame(cx, vp);
>+        return slowPathOnEnterFrame(cx, vp);
> }

Looks like an indentation mistake here.

	Jason Orendorff [:jorendorff] Reporter
	Comment 15 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #14)
> Chatting on IRC I think you came up with a way to eliminate this aspect
> of the thing. I think it involved having forceReturn callers explicitly
> call ScriptDebugEpilogue rather than having the trampoline code do it. I
> think that would be great, for what it's worth.

Er, update: now it's unclear how hard this would be. We should go with whatever makes the best sense from a software engineering standpoint. I don't have a strong opinion about it.

	Jason Orendorff [:jorendorff] Reporter
	Comment 16 • 13 years ago

Comment on attachment 579780 [details] [diff] [review]
Implement Debugger.Frame.prototype.onPop.

r=me with the comments addressed and at least some additional test coverage.

	Jim Blandy :jimb Assignee
	Comment 17 • 13 years ago

Filed bug 716761 to follow up on the extra calls to ScriptDebugEpilogue possible when onPop handlers convert returns to throws or vice versa.

	Jim Blandy :jimb Assignee
	Comment 18 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> It seems nicer to have a removeFront() method here. That's the only reason
> anyone would want this method, right?

Pretty much. Fixed.

> IIUC you can remove the "global" calculation here, since FrameRange will
> compute it for you.

Done.

> Did you mean to remove this comment? I don't think switching to FrameRange
> changes this behavior. Maybe you mean that this behavior isn't considered a
> hack anymore with or without FrameRange? (To me, it still seems like a hack
> and contrary to the spec.)

I'd moved it to Debugger::FrameRange::findNext, but that's crazy. I've put it at the top of Debugger::FrameRange.

> @@ +1026,4 @@
> >       */
> >      AutoObjectVector frames(cx);
> >      GlobalObject *global = fp->scopeChain().getGlobal();
> > +    for (FrameRange r(cx, fp, global); !r.empty(); r.popFront()) {
> 
> Same comment about computing "global" here.

I've moved this computation of global to inside the '#if DEBUG' section.

> @@ +1029,5 @@
> > +    for (FrameRange r(cx, fp, global); !r.empty(); r.popFront()) {
> > +        JSObject *frame = r.frontFrame();
> > +        if (!frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined() &&
> > +            !frames.append(frame))
> > +            return JSTRAP_ERROR;
> 
> Pre-existing style nit: This should have had curly braces before. Add them?

Done.

	Jim Blandy :jimb Assignee
	Comment 19 • 13 years ago

(In reply to Jim Blandy :jimb from comment #18)
> > IIUC you can remove the "global" calculation here, since FrameRange will
> > compute it for you.
> 
> Done.

Hah, no good deed unpunished, later patches in the series add references to it.

	Jim Blandy :jimb Assignee
	Comment 20 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #9)
> Comment on attachment 579777 [details] [diff] [review]
> Make the JavaScript shell's 'evaluate' function to return a value.
> 
> Frabjous day!
> 
> I'm sure some existing tests (in debugger and elsewhere) can be simplified
> with this change, but don't bother.
> 
> I don't think the JS_SET_RVAL line is needed. Remove it?

Done. I was somehow thinking that exceptions were returned there, but of course that's not the case.

	Jim Blandy :jimb Assignee
	Comment 21 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> Thinking white-box here, I can think of more things to test. It's up to you
> how far to go with this, but as it stands the tests look too one-dimensional
> for my tastes.
> 
> - Multiple active frames may have onPop handlers with various values, and
> they'll be called in the right order.
> 
> - Setting .onPop to undefined after it was set to a function value causes
> the function not to be called when the frame is popped.
> 
> - When an exception is thrown across multiple frames, onPop is called on
> each frame in the order they are popped.
> 
> - When an exception is thrown across multiple frames, onPop calls interleave
> with onExceptionUnwind calls.
> 
> - onPop is called correctly when an exception is implicitly rethrown at the
> end of a finally block.
> 
> - dbg.getNewestFrame from a frame's onPop hook returns that frame.
> 
> - Setting .onPop on a dead frame throws (I think this is what the spec says;
> at least it must not crash or assert)

Okay, these are done; I'll do some more later.

	Jim Blandy :jimb Assignee
	Comment 22 • 13 years ago

Added some tests. Attaching for fun.

	Jim Blandy :jimb Assignee
	Comment 23 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> Thinking white-box here, I can think of more things to test. It's up to you
> how far to go with this, but as it stands the tests look too one-dimensional
> for my tastes.

I've added all these tests.

> Also, it would be great to figure out how onPop should interact with proper
> tail calls and get some tests in place that pass now and should continue to
> pass when proper tail calls are implemented.

Yeah, that's an interesting question. I'll reply to this in detail in a separate comment.

> ::: js/src/jit-test/tests/debug/Frame-onPop-error-error.js
> @@ +57,5 @@
> > +test('eval', 'eval(\'debugger; \\\'termination fail\\\';\');');
> > +test('global', 'evaluate(\'debugger; \\\'termination fail\\\';\');');
> > +
> > +// When doing termination tests, it's wise to check that the whole file ran.
> > +print('Done!');
> 
> Huh. The test runner does flunk us if this test terminates erroneously, with
> or without the print() call, right? If not, fix it! I think it does, though,
> looking at check_output() in jt_test.py.
> 
> I dunno, what is this print() call for, exactly? Some of the other tests
> involving termination don't have it. We should have a rule of thumb as to
> when it's appropriate, if ever. I claim almost all tests (excepting a few
> that use "|jit-test| error:") are supposed to execute to the end and then
> pass; and the test runner should (rather strong practical and moral sense of
> "should" here) catch all forms of failure; and therefore it should take 0
> lines of code to state that that is what we expect to have happen.

I think I've gotten this sorted out. First, the patch in bug 716786 makes termination show up as an error status, so if it happens really unexpectedly, we'll fail. Second, we can just use |jit-test| error: to check for a distinctive error thrown at the end of the script. (It seemed wanky at the time, but now it's turning out that auto-generating all those tests in Emacs Lisp was a good idea!)

> These "error" tests have the weird quality that if the first onEnterFrame
> hook fails to trip, they pass. I can live with that.

All fixed. Auto-generation to the rescue again.

> ::: js/src/jit-test/tests/debug/Frame-onPop-multiple-01.js
> @@ +2,5 @@
> > +
> > +function completionsEqual(c1, c2) {
> > +    return (c1 === c2 ||
> > +            (c1.throw && c1.throw == c2.throw) ||
> > +            (c2.return && c1.return == c2.return));
> 
> Use the === operator rather than == here?

Done.

> @@ +46,5 @@
> > +    var dbgs = [];
> > +    var log;
> > +
> > +    // Create a separate debugger to carry out each item in sequence.
> > +    for (s in sequence) {
> 
> I dunno, it seems plainer to me to write:
> 
> >  for (var i = 0; i < sequence.length; i++) {

How is it plainer? I think I like mine better.

> ::: js/src/jit-test/tests/debug/Frame-onPop-multiple-02.js
> @@ +1,3 @@
> > +// When multiple debuggers have onPop handlers, an uncaught error in one
> > +// doesn't prevent the others' onPop handlers from being thrown, but the
> > +// debuggee is terminated.
> 
> "from being called", not "from being thrown".

Fixed --- thanks for reading closely.

> I can't find where the spec requires this behavior. Aren't we lying to dbg2
> if we do it this way? How does this fit with what the spec requires when a
> handler function fails (throwing an exception to the debuggee, something we
> have not implemented yet)?

Well, if an earlier onPop handler returns a non-undefined resumption value, it seems clear that that's what we should pass to later onPop handlers. But the case here is that an earlier handler has simply failed. There, I can see an argument that we should not propagate that to later handlers --- the debuggee's progress *will* be affected by the failure, but we shouldn't represent that failure as the debuggee's own state to other debuggers.

I'll revise this, and make sure the docs are clear.

> @@ +38,5 @@
> > +// When an error is reported, the shell usually exits with a nonzero exit
> > +// code. If we get here, the test passed, so override that behavior. (If we
> > +// try to use the "error" jit-test metaline to permit the ReferenceError,
> > +// then the test passes whenever a ReferenceError is seen, even if
> > +// assertions fail.)
> 
> I've written stuff like this too. Is it time for this shell behavior to
> change?

This can be fixed with the 'expect a distinctive error thrown at the end' hack, too. Done.

> ::: js/src/jit-test/tests/debug/Frame-onPop-return-return.js
> @@ +20,5 @@
> > +        assertEq(f.type, type);
> > +        wasConstructing = f.constructing;
> > +        f.onPop = function handlePop(c) {
> > +            log += ')';
> > +            assertEq(c.return, 'book');
> 
> You can also return a compliment (which seems a better parallel to 'favor')
> or a form, survey, or questionnaire.

I like 'compliment'. Done. Third point for elisp.

	Jim Blandy :jimb Assignee
	Comment 24 • 13 years ago

So, regarding tail recursion and onPop:

A tail call guarantee is a promise to the programmer that certain kinds of calls will not hold stack memory allocated until the call returns, as an ordinary non-tail call must. Such guarantees are quite strange, because although the tail call optimization isn't hard to understand, and it's possible to clearly specify when the optimization is applicable, language specifications usually don't promise anything about memory consumption: you're supposed to just assume the implementors did a good job. But in practice, programmers design against an informal, inferred memory consumption model for the language. Although things like garbage collection and fragmentation complicate the picture, these informal models are close enough to the truth to be generally useful.

In a language that makes tail-call guarantees, a user should be able to assume that a program that can run for arbitrarily long periods of time without exhausting memory can do the same under the debugger. This means that a debugger may not retain records of the caller frames that tail calls replaced with callee frames. Thus, the tail call optimization must be visible to Debugger's client, at the very least because the 'older' chain of frames must omit those frames that make tail calls.

However, if the debugger sets an onPop handler on a frame, I think we should honor that: any reasonable cost model for the debugger itself permits a small constant amount of storage for each onPop handler set. So if an onPop handler is set on a frame, we might
as well recompile that frame not to make tail calls.

	Jim Blandy :jimb Assignee
	Comment 25 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #11)

It turns out jit_test.py has everything we need to do this properly, with a bit of a hack. The tests don't print "Done!" any more, nor do they call quit(0).

	Jim Blandy :jimb Assignee
	Comment 26 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #13)
> (In reply to Jason Orendorff [:jorendorff] from comment #12)
> > - Debugger.Frame.prototype.onPop === undefined.
> 
> Er, actually maybe that should throw a TypeError.

Frame-onPop-17.js checks this.

> Three more possible GC tests, covering details of
> Debugger::slowPathOnLeaveFrame:
> 
> - If there are several onPop handlers, and one causes the others to be
> removed (set to undefined) and then forces GC, the other onPop handlers are
> not called.

Added as Frame-onPop-multiple-03.js.

> - If there are several Debuggers with onPop handlers, and one causes the
> others to be disabled, the other onPop handlers are not called.

Added as Frame-onPop-multiple-04.js.

> - If one onPop handler causes a different Debugger to reify a Debugger.Frame
> object for the frame that is about to be popped, that
> created-at-the-last-minute Frame still becomes !.live after all the onPop
> handlers are called. (In other words, slowPathOnLeaveFrame cleans it up
> properly even though it wasn't in the 'frames' vector.)

This was broken --- good catch! Added as Frame-onPop-multiple-05.js.

> - The this-value passed to an onPop handler is the Debugger.Frame object.

Incorporated into Frame-onPop-01.js.

> - When an onPop hook is called, the Frame is still live. (Even if previous
> onPop hooks for the same frame were called and changed the completion
> status.) (The spec doesn't explicitly say that this is the case, but it
> seems the most likely interpretation.)

Incorporated into Frame-onPop-07.js and Frame-onPop-multiple-01.js.

	Jim Blandy :jimb Assignee
	Comment 27 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #14)
> In jsinterp.cpp, js::Interpret:
> >     /* State communicated between non-local jumps: */
> >-    JSBool interpReturnOK;
> >+    bool interpReturnOK;
> 
> Great, but also change all the places where we assign JS_TRUE or
> JS_FALSE to this variable to use plain old true and false.

Done.

> In jsinterp.h, comment on ScriptDebugEpilogue:
> >+ * This function may be called twice for the same outgoing frame; only the
> >+ * first call has any effect. (Permitting double calls simplifies some
> >+ * cases where an onPop handler's resumption value changes a return to a
> >+ * throw, or vice versa: we can redirect to a complete copy of the
> >+ * alternative path, containing its own call to ScriptDebugEpilogue.)
> 
> Chatting on IRC I think you came up with a way to eliminate this aspect
> of the thing. I think it involved having forceReturn callers explicitly
> call ScriptDebugEpilogue rather than having the trampoline code do it. I
> think that would be great, for what it's worth.

It turns out to be harder than it looks. Filed as a follow-up bug, https://bugzilla.mozilla.org/show_bug.cgi?id=716761

> In methodjit/InvokeHelpers.cpp, js_InternalThrow:
> >+        // prologues and epilogues. RunTracer(), Interpret(), and Invoke() all
> >         // rely on this property.
> 
> While you're in here, remove "RunTracer()" from the comment. That
> function is gone.

Done.

> In Debugger.cpp, Debugger::slowPathOnLeaveFrame:
> >+    for (FrameRange r(cx, fp, global); !r.empty(); r.popFront())
> >+        if (!frames.append(r.frontFrame())) {
> >+            cx->clearPendingException();
> >+            return false;
> >+        }
> 
> Style nit: braces around the if-statement.

Done.

> >+    /* 
> >+     * If any onPop handler produces an uncaught exception, that's something we
> >+     * need to relay back no matter what the other handlers do.
> >+     */
> >+    bool anyUncaught = false;
> 
> This seems like a bad hack to me. It complicates both the spec and the
> implementation, and it breaks two invariants I can imagine users caring
> about: (1) that an onPop handler should never be skipped, except in the
> extraordinary case where we OOM trying to call it; and (2) that if the
> uncaughtExceptionHook returns continue, the debuggee should not be
> disrupted.

Yeah; until bug 729592 is fixed, we can't really distinguish the failures that need to be propagated out to the debuggee from those that can be reported to the next onPop handler anyway. I've taken this out completely.

> >+            Value completion;
> >+            if (!dbg->newCompletionValue(ac, &AutoCompartment::enter, frameOk,
> >+                                         fp->returnValue(), &completion)) {
> >+                frameStatus = dbg->handleUncaughtException(ac, NULL, false);
> >+                break;
> >+            }
> 
> Hmm. It is unclear at the point where handleUncaughtException is called
> whether or not ac was entered.
> 
> Prior to the change, newCompletionValue *always* called ac.leave(),
> which is infallible, even on error, so the caller knew where matters
> stood. With the change, we might be calling ac.enter(), and it might
> fail. If that happens we must not call handleUncaughtException; we just
> want to set frameStatus appropriately and let the error propagate.

Okay, I see. I've introduced a new patch to the series that splits newCompletionValue up differently, so that callers can do their own compartment transition if they like. This removes the whole enter/leave argument, and allows this failure mode to be addressed properly.

> 
> >+            bool hookOk = Invoke(cx, ObjectValue(*frameobj), handler, 1, &completion, &rval);
> >+            Value val;
> >+            if (hookOk) {
> >+                nextStatus = dbg->parseResumptionValue(ac, hookOk, rval, &val);
> >+            } else {
> >+                nextStatus = dbg->handleUncaughtException(ac, &val, true);
> >+                if (nextStatus == JSTRAP_ERROR)
> >+                    anyUncaught = true;
> >+            }
> >+
> >+            /* At this point, we are back in the debuggee compartment. */
> >+            JS_ASSERT(cx->compartment == global->compartment());
> 
> You can also assert that no exception is pending on cx.

Added.

> >+            /* Prep the context and frame according to the resumption value. */
> >+            switch (nextStatus) {
> >+              case JSTRAP_CONTINUE:
> >+                /* This means 'keep doing whatever we were doing before'. */
> >+                switch (frameStatus) {
> >+                  case JSTRAP_RETURN:
> >+                    break;
> >+                  case JSTRAP_THROW:
> >+                    /* Put back the exception that newCompletionValue cleared. */
> >+                    cx->setPendingException(savedException);
> >+                    break;
> >+                  case JSTRAP_ERROR:
> >+                    cx->clearPendingException();
> 
> Since no exception is pending here (see preceding comment) you can delete
> that last line.

This has all been changed in the new patch.

> >+    /* Clean up all Debugger.Frame instances. */
> >+    for (JSObject **p = frames.begin(); p != frames.end(); p++) {
> >+        JSObject *frameobj = *p;
> >+        Debugger *dbg = Debugger::fromChildJSObject(frameobj);
> > 
> >         frameobj->setPrivate(NULL);
> > 
> >         /* If this frame had an onStep handler, adjust the script's count. */
> >         if (!frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined() &&
> >-            fp->isScriptFrame())
> >+            fp->isScriptFrame() &&
> >+            !fp->script()->changeStepModeCount(cx, -1))
> >         {
> >-            fp->script()->changeStepModeCount(cx, -1);
> >+            frameStatus = JSTRAP_ERROR;
> >+            /* Don't exit the loop; we must mark all frames as dead. */
> >         }
> > 
> >-        r.frontDebugger()->frames.remove(r.frontFrameMapPtr());
> >+        dbg->frames.remove(fp);
> 
> Ooh, the test I described in comment 13, bullet 3 might fail. I think
> there could be additional new entries in various Debugger::frames tables
> for fp, since JS code has run.

As mentioned, this was broken, and is now tested and fixed.

> Separately, I think either frameStatus or frameOk should be the single
> source of truth throughout this function. Since we let them diverge in
> the error case here, and then use frameStatus on the last line, it would
> be consistent to use frameOK only to calculate the initial frameStatus,
> and never assign to it.

Until we can properly distinguish synthetic and real errors, yeah, there's no point. This is gone now, too.

> >-Debugger::newCompletionValue(AutoCompartment &ac, bool ok, Value val, Value *vp)
> >+Debugger::newCompletionValue(AutoCompartment &ac, bool (AutoCompartment::*transition)(),
> >+                             bool ok, Value val, Value *vp)
> 
> Changing the return type of AutoCompartment::leave seems wrong. That
> would be leaking a yucky implementation detail of the debugger to all
> code that uses compartments. It is a potentially misleading thing to
> change, too. We definitely don't want people thinking that leave() is
> fallible--or, much worse, that it's OK to make it fallible!!

As mentioned above, taking apart newCompletionValue made it possible for slowPathOnLeaveFrame to become much neater, so these changes are now gone entirely.

> Or, perhaps your Curry-Howard Theory of Self-Evidently Correct Code
> implies we should common up the three places where ac.leave() is being
> called. And then you could just use a bool (or enum) parameter and the
> mighty if statement.

There is indeed only one ac.leave() in newCompletionValue now, which is kind of nice.

> In vm/Debugger.h, Debugger::onEnterFrame:
> > {
> >     if (cx->compartment->getDebuggees().empty())
> >         return JSTRAP_CONTINUE;
> >-    return slowPathOnEnterFrame(cx, vp);
> >+        return slowPathOnEnterFrame(cx, vp);
> > }
> 
> Looks like an indentation mistake here.

This disappeared at some point; fixed, anyway.

	Jim Blandy :jimb Assignee
	Comment 28 • 13 years ago

In class Debugger, split newCompletionValue into:
- resultToCompletion, which takes a standard SpiderMonkey (success, value,
  context's exception) triple and produces the corresponding
  (JSTrapStatus, value) pair; and
- newCompletionValue, which takes a (JSTrapStatus, value) pair and produces
  a JavaScript completion value.

Define receiveCompletionValue to do exactly what newCompletionValue used to
do: the above two operations, with a compartment 'leave' in the middle.
Substitute receiveCompletionValue where newCompletionValue is used now.

	Jim Blandy :jimb Assignee
	Comment 29 • 13 years ago

Many more tests; almost all review comments addressed, as described above.

	Jim Blandy :jimb Assignee
	Comment 30 • 13 years ago

The patch sequence as a whole is still as presented by bugzilla, above.

	Jim Blandy :jimb Assignee
	Comment 31 • 13 years ago

Try push here: https://tbpl.mozilla.org/?tree=Try&rev=2ddbc4d6d561

	Jason Orendorff [:jorendorff] Reporter
	Comment 32 • 13 years ago

(In reply to Jim Blandy :jimb from comment #24)
> In a language that makes tail-call guarantees, a user should be able to
> assume that a program that can run for arbitrarily long periods of time
> without exhausting memory can do the same under the debugger.

Strongly agree.

> This means
> that a debugger may not retain records of the caller frames that tail calls
> replaced with callee frames. Thus, the tail call optimization must be
> visible to Debugger's client, at the very least because the 'older' chain of
> frames must omit those frames that make tail calls.

Strongly agree.

> However, if the debugger sets an onPop handler on a frame, I think we should
> honor that: any reasonable cost model for the debugger itself permits a
> small constant amount of storage for each onPop handler set. So if an onPop
> handler is set on a frame, we might
> as well recompile that frame not to make tail calls.

Hmm. Maybe. Won't it be usual for debugger clients to set .onPop hooks on the current frame, whenever stepping, just as a matter of course? It is a little strange for the stack to be so different, if you step in line by line, vs. if you just set a breakpoint and run to it. That kind of unexplained difference in behavior could lead debugger users to think they might be going insane. An alternative design would be, call frame.onPop('tail call') at the time of the tail call.

The common behavior between the two ideas is that in both cases, each frame's onPop hook is called before control returns to its caller. And of course tail calls do not cause things to randomly crash. :) These properties can be tested, without testing for specific behavior.

	Jason Orendorff [:jorendorff] Reporter
	Comment 33 • 13 years ago

Comment on attachment 601473 [details] [diff] [review]
Separate the construction of a completion value from the debuggee->debugger compartment transition.

Nice patch.

	Jason Orendorff [:jorendorff] Reporter
	Comment 34 • 13 years ago

Let me restate that. That patch is *much* nicer than either of the silly ideas I suggested, and *so* much nicer than the original!

I have to finish this tomorrow. First thing, I promise.

	Jason Orendorff [:jorendorff] Reporter
	Comment 35 • 13 years ago

Comment on attachment 601475 [details] [diff] [review]
Implement Debugger.Frame.prototype.onPop.

"return 'snifter';"?

Looks great. If you can stand it, I'd like one more test. frame.eval() should work when called from an onPop handler. The eval code should be able to poke at the locals and arguments. I think it probably works fine.

	Jim Blandy :jimb Assignee
	Comment 36 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #32)
> > However, if the debugger sets an onPop handler on a frame, I think we should
> > honor that: any reasonable cost model for the debugger itself permits a
> > small constant amount of storage for each onPop handler set. So if an onPop
> > handler is set on a frame, we might
> > as well recompile that frame not to make tail calls.
> 
> Hmm. Maybe. Won't it be usual for debugger clients to set .onPop hooks on
> the current frame, whenever stepping, just as a matter of course? It is a
> little strange for the stack to be so different, if you step in line by
> line, vs. if you just set a breakpoint and run to it. That kind of
> unexplained difference in behavior could lead debugger users to think they
> might be going insane.

Hmm, that's true.

> An alternative design would be, call
> frame.onPop('tail call') at the time of the tail call.

Yeah, or frame.onPop({ tailCall:true }), or frame.onTailCall()...

	Jim Blandy :jimb Assignee
	Comment 37 • 13 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #35)
> Comment on attachment 601475 [details] [diff] [review]
> Implement Debugger.Frame.prototype.onPop.
> 
> "return 'snifter';"?
> 
> Looks great. If you can stand it, I'd like one more test. frame.eval()
> should work when called from an onPop handler. The eval code should be able
> to poke at the locals and arguments. I think it probably works fine.

Added, Frame-onPop-21.js.

	Jim Blandy :jimb Assignee
	Comment 38 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8809550cb08

OMFSM

	Ed Morley [:emorley]
	Comment 39 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/b46cc7472de6
https://hg.mozilla.org/mozilla-central/rev/73f1b9d271a5
https://hg.mozilla.org/mozilla-central/rev/b04b3933ebf2
https://hg.mozilla.org/mozilla-central/rev/e6ca61e5fc81
https://hg.mozilla.org/mozilla-central/rev/c8809550cb08