999858 - [jsdbg2] Debugger.Frame.prototype.eval doesn't handle variable declarations correctly

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

The following test ought to pass, per spec, but doesn't:

    var g = newGlobal();
    var dbg = new Debugger(g);
     
    g.eval('var x = "global";');
    g.eval('function a() { function b() { return x; } c(); return b; }');
    g.c = function () {
      dbg.getNewestFrame().eval('var x = "local";');
    }
     
    assertEq(g.a()(), 'local'); 

The reason is that the front end generates a JSOP_NAME opcode for the reference to 'x' in b, so b never searches its scope chain, and never sees the new declaration of 'x' that the debugger added.

Debugger.Frame.prototype.eval is explicitly specified to add bindings to the frame's environment in this case. From js/src/doc/Debugger/Debugger.Frame.md:

    If <i>code</i> is not strict mode code, then variable declarations in
    <i>code</i> affect the environment of this frame. (In the terms used by
    the ECMAScript specification, the `VariableEnvironment` of the
    execution context for the eval code is the `VariableEnvironment` of the
    execution context that this frame represents.) If implementation
    restrictions prevent SpiderMonkey from extending this frame's
    environment as requested, this call throws an Error exception.

In this sense, Debugger.Frame.prototype.eval resembles a direct, non-strict eval; the following test passes:

    g.eval('var x = "global";');
    g.eval('function a(s) { function b() { return x; } eval(s); return b; }');

    assertEq(g.a('var x = "local";')(), 'local');

Comment 1

[Jim Blandy :jimb]

Edited IRC log:

<jimb> shu: Would this test pass? [text as above]
<shu> jimb: let's see  [17:52]
<shu> jimb: that doesn't pass
<shu> jimb: that doesn't pass in the interpreter, i should say
<jimb> Does the assertion fail, or does it throw, or what?  [17:57]
<shu> % dist/bin/js 4910230.js 
<shu> 4910230.js:12:0 Error: Assertion failed: got "global", expected "local"
<shu> i mean, this fails with an assertion too:  [17:58]
<shu> jimb: [pastebin with the following text:

    var g = newGlobal();
    var dbg = new Debugger(g);
     
    g.eval('var x = "global";');
    g.eval('function a() { function b() { return x; } c(); return b; }');
    g.c = function () {
      // This should either work (as this test assumes), or throw.
      dbg.getNewestFrame().eval('var x = "local";');
    }
     
    assertEq(g.a()(), 'local');

]

<jimb> shu: Okay. That's what I expected.  [17:59]
<jimb> shu: I think that test messes with lazy bytecode generation's
       assumptions, too...
<shu> jimb: what's supposed to happen?  [18:00]
<shu> jimb: i'm not clear on the semantics here
<jimb> shu: Well, ideally, the new var would capture the reference in that b,
       but not other b's.
<jimb> shu: That's what you'd expect to happen given the way the ECMAScript
       standard defines the semantics.
<jimb> since the spec defines variable resolution in terms of a search done at
       every reference.  [18:01]
<jimb> And that's the effect a direct eval in the body of a would have.
<shu> jimb: that's an eval in a function frame  [18:04]
<jimb> right
<shu> jimb: and var would bind locally
<jimb> A direct eval in non-strict code won't.
<shu> jimb: oh
<shu> jimb: excuse me if i don't have the highest respect for those semantics
<jimb> shu: For example, this passes: [the non-strict direct eval example above]
<shu> jimb: right, but
<shu> jimb: and you specced eval in frame to be as-if it were direct eval in
      that frame?
<jimb> shu: Yeah.
<jimb> shu: Because the *intent* is to let people mess with stuff.
<shu> jimb: can we... unspec it to be indirect eval?
<jimb> shu: That would be unhelpful; you couldn't even do what GDB does.
<jimb> I think you don't mean 'indirect' eval.
<jimb> I think you mean 'strict' eval.
<shu> jimb: perhaps  [18:09]
<shu> jimb: so...  [18:10]
<jimb> shu: Thinking about it from the point of view of someone who wants to
       be able to tweak their program, it just seems like direct non-strict
       eval is the more interesting primitive.
<jimb> shu: We're not designing a robust program here; we're trying to provide
       maximal power to observe the execution of an existing proram.
<shu> jimb: okay, i see  [18:11]
<shu> jimb: to support direct eval then
<shu> jimb: we need to do OSR for bytecode too
<shu> jimb: not just jitcode
<shu> jimb: the scope coords/aliasing stuff is baked in there
<jimb> right, exactly
<jimb> shu: If we had function re-lazification working, that'd be most of it.
                                                                        [18:12]
<shu> jimb: i don't know how to architect that off the top of my head
<shu> jimb: are you supposed to be able to relazify on-stack scripts?
<jimb> No... :(  [18:13]
<shu> jimb: seems like a poor choice :)
<shu> jimb: to OSR bytecode wouldn't be too hard
<shu> well, i take that back
<jimb> haha
<shu> i was about to walk into a trap there  [18:14]
<shu> jimb: but my thinking is, with baseline there
<jimb> Why not just stop doing binding analysis in the bytecode compiler?
<shu> jimb: interpreter perf is *really* low priority now
<shu> jimb: that might be even easier, yes
<shu> jimb: delay binding analysis until baseline
<shu> jimb: go back to a naive but flexible interpreter  [18:15]
<jimb> And baseline can be re-jitted.
<shu> jimb: that decision goes beyond you and me though
<jimb> shu: Oh, sure.
<shu> jimb: nobody wants to touch the frontend really
<jimb> shu: I would phrase it "move the interpreter closer to the specified
       language semantics"
<shu> jimb: but it makes sense to move more and more analyses we used to do in
      the frontend to the backend

Comment 2

[Luke Wagner [:luke]]

(In reply to Jim Blandy :jimb from comment #0)
> The reason is that the front end generates a JSOP_NAME opcode for the
> reference to 'x' in b, so b never searches its scope chain, and never sees
> the new declaration of 'x' that the debugger added.

Actually, JSOP_NAME is exactly the op that searches scope chain; the problem is that the scope which receives the local binding 'x' isn't heavyweight, so it's not on the scope chain of b.  There is a DebugScopeProxy scope chain that includes a proxy for b's missing CallObject, but this is only visible from the debugger's pov.

In the abstract, I like the idea of re-compiling everything from source to despecialize all name ops and mark everything heavyweight, but that seems like a pretty mammoth undertaking.

Comment 3

[Shu-yu Guo [:shu]]

Recompiling wouldn't be too hard. Luke, what do you think of moving all the heavyweight and aliasing analysis to baseline compilation?

Comment 4

[Luke Wagner [:luke]]

Recompiling and moving the aliasing analysis to baseline seems straightforward and probably even really nice for reducing overall complexity of the frontend.  The harder part seems to be walking the whole heap and fixing up all the related scripts, functions, scopes, lazyscripts, Debugger objects, etc to be coherent.  Also, we'd want to be careful to implement this interpreter-OSR in a way that didn't involve checking "have I recompiled" all over the place; perhaps by putting all the central variables (like 'script') in the InterpreterActivation where they can be synchronously updated (like we do with enableInterruptsIfRunning()).

It seems like the same mechanism could be used to implement edit-and-continue; that could better amortize the cost.

Comment 5

[Shu-yu Guo [:shu]]

Yeah, the JIT debug mode OSR is local in the sense that you don't need to fix up the callgraph, just the stack. I didn't realize this point.

Comment 6

[Jim Blandy :jimb]

To highlight the bug Luke points out in comment 2, because I missed it:

While the failure of the assertion in the code below is consistent with Debugger.Frame.prototype.eval behaving like strict direct eval (using its own VariableEnvironment), uncommenting the 'eval("")' in the code below makes the test pass. *That* is inconsistent with any reasonable definition of D.F.p.eval, and is clearly a bug.

    var g = newGlobal();
    var dbg = new Debugger(g);

    g.eval('var x = "global";');
    g.eval('function a() { /* eval(""); */ function b() { return x; } c(); return b; }');
    g.c = function () {
      dbg.getNewestFrame().eval('var x = "local";');
    }

    assertEq(g.a()(), 'local');

Comment 7

[Jim Blandy :jimb]

In the end, I'm not actually coming up with any good stories about why it's critically important for Debugger.Frame.prototype.eval to be able to introduce bindings into the frame's environment. That being able to do so is more consistent with direct non-strict eval should count for something --- but then again, ES5 decided to remove this behavior in strict mode code.

So I'd like to change the spec to make Debugger.Frame.prototype.eval and .evalInFrame to behave like strict direct eval, and morph this bug into fixing the inconsistency described in comment 6.

Thanks for folks' interest and comments.

Comment 8

[Jim Blandy :jimb]

Attachment: Change Debugger.Frame.prototype.eval specification to make 'var' declarations in the eval code local to that eval.

Proposed spec change.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8413028 [details] [diff] [review]
Change Debugger.Frame.prototype.eval specification to make 'var' declarations in the eval code local to that eval.

That's great. I hope it's easy to implement that way.