1092910 - [jsdbg2] implement generator support for Debugger

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[James Long (:jlongster)]

If I have a generator instance, and I call `next()` on it, I can't step into it. STR:

1. http://jlongster.com/s/step-into-gen/
2. Set a breakpoint on line 10
3. Call `bar` from the console
4. Click "step into" when paused

It just skips over the line, as if we did a normal step. But I most certainly told it to step *into*. Is there something difficult about this? Playing around with the debugger backed, this doesn't seem to have an easy fix. It appears we may not even get an `onEnterFrame` notification from `Debugger` for generator frames. I suspect there will be platform improvements needed for it.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Did you verify that we handle completion and resumption values properly when they contain 'yield'?

https://developer.mozilla.org/docs/Tools/Debugger-API/Conventions#Completion_Values

I don't think anyone has ever verified that the debugger server does the right thing in those cases and I'm pretty sure back when I originally wrote that code I didn't test with generators at all. I've been meaning to come back to this now that their use is more prevalent and I've been wondering whether this could be teh reason behind some of the bugs related to chrome debugging that we have.

Comment 2

[James Long (:jlongster)]

Where are those resumption values handled? For stepping I see in `_handleResumeLimit` that it adds an `onEnterFrame` hook and will pause when that is hit (if it's not blackboxed). The only thing I can think of is that we don't get an enter frame notification, but surely we do?

I haven't looked much into it though; if you think it might not be hard to fix, great!

(side note: this doesn't work in Chrome either, hah, would be great to be the first to fix!)

Comment 3

[Tom Tromey :tromey]

I looked into this.  I was under the impression that generator debugging had landed,
but I can't find the bug (just bug 674167 referring to writing tests).

The Debugger.Frame docs refer to an "onResume" property; but this isn't implemented
(the only grep hit is the docs).  I wasn't sure if this is because the work hasn't been
done, or because the work was done some other way and this is a dead bit of documentation.

Also, onEnterFrame is not called when re-entering generator frames.  Here's a simple test:

g = newGlobal();
g.eval("function* gen() {\n" +
       "  yield 1;\n" +
       "  yield 2;\n" +
       "  yield 3;\n" +
       "}\n");

all = [];

d = new Debugger(g);

function handleFrame(frame) {
    print(frame.script.displayName + " => " + frame.generator);
    if (frame.generator) {
	frame.onResume = handleFrame;
    }
}

d.onEnterFrame = handleFrame;
g.eval("x = gen(); print(x.next().value); print(x.next().value); print(x.next().value);");


Running it, I get:

pokyo. ./obj-x86_64-unknown-linux-gnu/dist/bin/js /tmp/s.js
undefined => false
gen => true
1
2
3


Here you can see that the calls to next() are not reported via onEnterFrame or
onResume.


One final note -- the various "yield" bits in completion and resumption values are all
marked as unimplemented in the docs.  So I think there's still some work to be done
in the js core before this bug can be fixed.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Oh, that's interesting. However, I don't think we ever use frame.onResume in the debugger server. We rely only on frame.{onEnterFrame,onPop,onStep}. I've seen tests like Frame-onPop-15.js that indicate onPop should fire correctly for generator frames, so it seems to me that things should just work.

Jim should know what's missing (if anything) from SpiderMonkey for generator debugging.

Comment 5

[Jim Blandy :jimb]

onResume was never implemented.

Debugger.Frame.prototype.onPop fires for resumption and yielding. It seems like js/src/jit-test/tests/debug/Frame-onPop-15.js should cover the onPop behavior, treating yields as if they were returns.

But without onResume, or without onEnterFrame firing on resumption, we can't implement stepping into resuming generators.

Comment 6

[Jan de Mooij [:jandem]]

This should be pretty easy to implement, we can fire the onResume hook as part of JSOP_DEBUGAFTERYIELD.

Comment 7

[Jim Blandy :jimb]

(In reply to Jan de Mooij [:jandem] from comment #6)
> This should be pretty easy to implement, we can fire the onResume hook as
> part of JSOP_DEBUGAFTERYIELD.

Does that mean that it'd be easy to support in the JITs as well?

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Jim Blandy :jimb from comment #7)
> Does that mean that it'd be easy to support in the JITs as well?

Yes. Baseline already does a VM call for JSOP_DEBUGAFTERYIELD if the script is in debug mode (Ion does not compile generator scripts). It'd be very similar to debugger statements (JSOP_DEBUGGER).

Making onResume a property of Debugger.Frame will be annoying though, because generators push a brand new frame whenever they resume. We no longer have "floating stack frames" since the big generator rewrite. GeneratorObject holds all state (scope chain, arguments object, etc) and we push a new frame based on that whenever we resume.

So it might be simpler to add Debugger.prototype.onResumeFrame (like onEnterFrame).

Comment 9

[Jim Blandy :jimb]

Okay: jorendorff, Shu, and I just met and talked about this. Thanks, guys!!!

I.

Implementing Debugger.Frame.prototype.onResume as specified probably isn't helpful. Our devtools' 'step into' functionality is implemented by setting onStep, onPop, and onEnterFrame handlers, and then resuming the debuggee. If generator resumption fired only an onResume hook, then we'd have to have onResume hooks set on every generator that could possibly be resumed, which is dumb. We need some hook that will fire on *any* generator's resumption.

The simplest change is probably to make onEnterFrame fire for generator resumption. Then our existing 'step into' code will just work.

We'd need to specify what passing a value into a generator via its 'next' function looks like in a call, and provide some way to distinguish resumptions from the initial onEnterFrame that signals the generator's creation.

As a footnote: onEnterFrame turns out to be an expensive hook to implement, as it requires debug code in essentially every debuggee script's prologue. It would be much better to move onEnterFrame to Debugger.Frame, and then provide a separate onExNihiloFrame hook for the oldest frames of event handlers and so on.

II.

Completion values should be able to distinguish yields from returns. I think the simplest way to do this is to have 'yield V' produce a completion value of the form { return: V, done: false }. From the caller's point of view, this *is* the return value from the 'next' call. The addition of the 'done' property allows interested tooling to distinguish yields from ordinary returns, if they care.

Resumption values of the form { return: V' } should be able to change the yielded value to V', just as they can change returned values; and change yields to throws or termination; and so on. However, we need not provide any way for a resumption value from an onStep or onDebuggerStatement handler or such like to force an 'early yield', the way we can force an early return: an early yield isn't meaningful, because the generator isn't prepared to be resumed at arbitrary points in its code.

III.

The specification says that all resumptions of a given generator are represented by a single Debugger.Frame, which disappears from the stack on yield, and reappears on the stack (possibly with a different parent!) on resume. This does not correspond well to the implementation, in which the stack frames pushed for different resumptions are essentially unrelated to each other (beyond having a reference to the same generator state). However, at the Debugger API level, it is very valuable to have a persistent Debugger.Frame: it's perfectly reasonable to ask about the script, bytecode offset, environment, 'this', 'arguments', and so on of a suspended generator; these are all part of the Debugger.Frame API, so it only makes sense that a suspended generator should have a D.F available. In that light, it seems difficult to justify having other fresh D.Fs for each resumption.

As a footnote: persistent Debugger.Frames for generators add more complexity to the mapping between Debugger.Frame instances and the actual frames they represent. We already spend a great deal of complexity ensuring that Debugger.Frames get invalidated when frames are popped; or fixed up when frames change representation. (Shu said that the code for this is "everywhere"; it seems comparable in complexity to the SPS synchronization problem which djvj recently put a lot of effort into removing.) I think I might see how we could *lazily* invalidate Debugger.Frame instances, such that the engine wouldn't need to do anything when a frame was popped or replaced or bailed out. I'm on the hook to write this up (or realize that the technique won't work).

Comment 10

[Jim Blandy :jimb]

Attachment: Draft spec for new Debugger generator support

Comment 11

[Shu-yu Guo [:shu]]

Comment on attachment 8548451 [details] [diff] [review]
Draft spec for new Debugger generator support

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

::: js/src/doc/Debugger/Debugger.Frame.md
@@ +135,5 @@
>      [Generator Frames][generator].)
>  
>  `depth`
>  :   The depth of this frame, counting from oldest to youngest; the oldest
>      frame has a depth of zero.

Do generator frames always have a .depth of 0? If we have a mercurial .older, this should change as well.

@@ +345,5 @@
> +A generator object refers to a stack frame with no fixed continuation frame.
> +While the generator's code is running, its continuation is whatever frame called
> +its `next` method; while the generator is suspended, it has no particular
> +continuation frame; and when it resumes again, the continuation frame for that
> +resumption could be different from that of the previous resumption.

Do generator D.Fs always have a null .older property, or is it non-null when the D.F is executing (not suspended)? It seems useful to have a mercurial .older so that when we are paused within a generator frame, the stack trace may be displayed. In any case a single binary .live isn't enough to capture the tristate of generator frames (live-executing, live-suspended, and dead).

::: js/src/doc/Debugger/Debugger.md
@@ +106,5 @@
> +    [visible frame][vf].)
> +
> +    If <i>resuming</i> is a true value, then we are resuming a generator frame,
> +    and <i>sentValue</i> is a debuggee value representing the value we are
> +    sending it. (If no value is sent, then <i>sentValue</i> is `undefined`.)

Is 'resuming' needed? Could we check for frame.generator? Generator frames can't be 'entered' normally anyways.

Comment 12

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #11)
> Do generator frames always have a .depth of 0? If we have a mercurial
> .older, this should change as well.

I agree. I'll mention generator frames in the 'depth' text.

> Do generator D.Fs always have a null .older property, or is it non-null when
> the D.F is executing (not suspended)? It seems useful to have a mercurial
> .older so that when we are paused within a generator frame, the stack trace
> may be displayed. In any case a single binary .live isn't enough to capture
> the tristate of generator frames (live-executing, live-suspended, and dead).

No, 'older' is supposed to be "mercurial", as you say. I'll mention generator frames there, too.

> Is 'resuming' needed? Could we check for frame.generator? Generator frames
> can't be 'entered' normally anyways.

Oh, that's a good point. If frame.generator, then <i>sentValue</i> is always present (although perhaps 'undefined'). That's great!

Thanks for slogging through the horrible diff!