609617 - delete(eval(...)) calls indirect eval

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jan de Mooij [:jandem]]

Following test case fails in SM:
----
var x = "fail";
function f() {
    var x = "pass"; 
    delete(eval("print(x)"));
}
f();
----
This works for typeof and void, and works in Chrome and Opera 11; evilpies confirmed it also calls direct eval in Besen.

Comment 1

[Tom S [:evilpie]]

Problem seems to be some constant folding done in the interpreter.
|void eval("print(x)")|
>callname "eval"
>string "print(x)"
>eval 1
|delete eval("print(x)")|, gets constant folded to eval("print(x)"), true
>callname "eval"
>string "print(x)"
>call 1

I think |call 1| should be |eval 1| in this case, too.

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

Comment 3

[Jim Blandy :jimb]

Would it be better to fix this in MakeSetCall itself? This should be a direct call, too, I would think:

js> dis(function f() { eval('x')++; }
)
flags: LAMBDA HEAVYWEIGHT
main:
00000:  callname "eval"
00003:  string "x"
00006:  setcall 1
00009:  eleminc
00010:  pop
00011:  stop

Source notes:
  0:     6 [   6] pcbase   offset 6
  2:     9 [   3] pcbase   offset 9
js>

Comment 4

[Brendan Eich [:brendan]]

I tested the other cases, they all throw:

js> x=1
1
js> f() 
typein:1: SyntaxError: invalid assignment left-hand side
js> function f() { eval('x')++; }
js> f()                           
typein:5: SyntaxError: invalid assignment left-hand side

However it does seem better to be safe and use belt in addition to braces, lest our clown pants fall down again. Thanks, new patch in a minute.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comment 6

[Brendan Eich [:brendan]]

As noted on IRC, I don't want to make an early error and risk breaking some IE-only path in a script's control flow graph that does insane stuff like this.

/be

Comment 7

[Jim Blandy :jimb]

Here's another test case:

function g() { print("bleah") }
function f() { function g() { throw('food'); } eval('g()')++; } 
f()

This should throw food, not print "bleah".

Comment 8

[Jim Blandy :jimb]

Is this still r? me, or is there more to fix in the emitter?

Comment 9

[Brendan Eich [:brendan]]

We talked before comment 8:

[12:39pm] brendan: also gdb is stalling at startup for me -- time to reboot mbp
[12:40pm] brendan: jimb: argh, emitter depended on the other MakeSetCall cases doing their thing
[12:40pm] brendan: jimb: new patch when i reconnect
[12:40pm] brendan: will beef up test
[12:41pm] jimb: brendan: I put my example in the bug.

but I didn't withdraw r? -- I'll obsolete that patch soon. This bug is tricky due to emitter assumptions about AST shape, tricky enough to minimize change to minimize regression risk for Firefox 4.

/be

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Here's another test case:
> 
> function g() { print("bleah") }
> function f() { function g() { throw('food'); } eval('g()')++; } 
> f()
> 
> This should throw food, not print "bleah".

js> dis(f)
flags: HEAVYWEIGHT
main:
00000:  deflocalfun 0 function g() {throw "food";}
00005:  nullblockchain
00006:  nop
00007:  callname "eval"
00010:  string "g()"
00013:  setcall 1
00016:  eleminc
00017:  pop
00018:  stop

Source notes:
  0:     6 [   6] funcdef  function 0 (function g() {throw "food";})
  2:    13 [   7] pcbase   offset 6
  4:    16 [   3] pcbase   offset 9

I think this is doing the right thing. We don't want to call eval('g()') and let the eval have whatever effects it wants, and then return a non-reference, and only then throw. We simply must avoid an early error (compile-time all-paths error checking breaks IE-only calls-as-lvalues).

/be

Comment 11

[Jeff Walden [:Waldo]]

(In reply to comment #10)
> I think this is doing the right thing. We don't want to call eval('g()') and
> let the eval have whatever effects it wants, and then return a non-reference,
> and only then throw.

Why don't we?  Per 11.3.1 we 1) call the eval function to produce a result, 2) don't throw because that result's not an attempted modification of an eval or arguments binding, 3) get a value from the result and call ToNumber on it, 4) add 1 to that, then 5) try to assign that value to the result of the eval function but throw because it's not a Reference.  The spec seems reasonably clear about what must be done.

Comment 12

[Brendan Eich [:brendan]]

Attachment: fix, v3

Main thing here is that all VB-ism "setcall" usages are strict mode errors. Take that, ancient IE extension! The test tries to cover all cases.

I fussed over ReportStrictModeError in really fussy ways, fussily, and uncapitalized the one capitalized (not counting acronyms) js.msg message.

/be

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #11)
> (In reply to comment #10)
> > I think this is doing the right thing. We don't want to call eval('g()') and
> > let the eval have whatever effects it wants, and then return a non-reference,
> > and only then throw.
> 
> Why don't we?  Per 11.3.1 we 1) call the eval function to produce a result, 2)
> don't throw because that result's not an attempted modification of an eval or
> arguments binding, 3) get a value from the result and call ToNumber on it, 4)
> add 1 to that, then 5) try to assign that value to the result of the eval
> function but throw because it's not a Reference.  The spec seems reasonably
> clear about what must be done.

You're right, however: this would require redoing our code generation to work differently from how JSOP_SETCALL works. I'm not going do that here.

You could file a bug; I'd argue WONTFIX.

/be

Comment 14

[Jeff Walden [:Waldo]]

Okay, filed as bug 609756, which we can circle back to after the 4.0 release.

Comment 15

[Brendan Eich [:brendan]]

Attachment: fix, v3a (rebased and with better assertEq "not reached" telltales)

Comment 16

[Jim Blandy :jimb]

(In reply to comment #10)
> I think this is doing the right thing. We don't want to call eval('g()') and
> let the eval have whatever effects it wants, and then return a non-reference,
> and only then throw. We simply must avoid an early error (compile-time
> all-paths error checking breaks IE-only calls-as-lvalues).

We already do call the eval. JSOP_SETCALL does the call, and then always throws.

(As an aside, having that eleminc there following code that pushes only one value gives me the creeps...)

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> (In reply to comment #10)
> > I think this is doing the right thing. We don't want to call eval('g()') and
> > let the eval have whatever effects it wants, and then return a non-reference,
> > and only then throw. We simply must avoid an early error (compile-time
> > all-paths error checking breaks IE-only calls-as-lvalues).
> 
> We already do call the eval. JSOP_SETCALL does the call, and then always
> throws.

You're right -- so what is causing the bug you wrote a testcase to demonstrate?

> (As an aside, having that eleminc there following code that pushes only one
> value gives me the creeps...)

JSOP_SETCALL used to push two stack items, but since it always throws, it pushes nothing ;-). It does not bother to adjust the stack pointer, even (hmm, that could be a problem -- cc'ing igor).

/be

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)
> (In reply to comment #16)
> > We already do call the eval. JSOP_SETCALL does the call, and then always
> > throws.
> 
> You're right -- so what is causing the bug you wrote a testcase to demonstrate?

Duh, just indirect eval. Will adjust test and add fix in v4 patch.

/be

Comment 19

[Jim Blandy :jimb]

(In reply to comment #17)
> You're right -- so what is causing the bug you wrote a testcase to demonstrate?

I'm confused. Here's how the current code behaves:

js> function g() { print("bleah") }
js> function f() { function g() { throw('food'); } eval('g()')++; } 
js> f()
bleah
typein:2: SyntaxError: invalid assignment left-hand side
js> 

It does so exactly *because* JSOP_SETCALL performs the call. The eval is incorrectly deemed indirect, and the "bleah" definition is used. It is always JSOP_SETCALL that throws, never the eval'ed code.

(In reply to comment #11)
> Why don't we?  Per 11.3.1 we 1) call the eval function to produce a result, 2)
> don't throw because that result's not an attempted modification of an eval or
> arguments binding, 3) get a value from the result and call ToNumber on it, 4)
> add 1 to that, then 5) try to assign that value to the result of the eval
> function but throw because it's not a Reference.  The spec seems reasonably
> clear about what must be done.

Except that ES5 Chapter 16 actually requires us not to call the function. To comply with ES5, we would need to throw an error, not call the eval code. We should deviate from ES5 by treating the error as non-early, not by doing the eval. So I think the fix for the put-to-non-reference cases is simply to change JSOP_SETCALL to always throw a BAD_LEFTSIDE_OF_DONKEY --- delete the Invoke call.

(I think this contradicts things I've said above; please treat this comment as my current understanding if so.)

Comment 20

[Jim Blandy :jimb]

(In reply to comment #18)
> Duh, just indirect eval. Will adjust test and add fix in v4 patch.

Okay, great.

Comment 21

[Brendan Eich [:brendan]]

My eyes failed to see the "bleah" output in my tiny eye-strain-o-vision terminal font -- so I thought we were throwing before eval'ing. Better get my eyes checked :-/.

> To comply with ES5, we would need to throw an error, not call the eval code. We
> should deviate from ES5 by treating the error as non-early, not by doing the
> eval. So I think the fix for the put-to-non-reference cases is simply to change
> JSOP_SETCALL to always throw a BAD_LEFTSIDE_OF_DONKEY --- delete the Invoke
> call.

So now that you know I missed the "bleah" and thought we did this, your argument is why I tried to say "works already". Waldo argued back and apart from the spec requiring something in Chapter 16 that is not (we think) web-compatible, and which makes for less interoperation (IE has Reference-returning built-ins, other browsers do not), I think he has a point.

Probably the right thing is to fix the direct -> indirect eval change and not try to throw a late-but-earlier-than-the-call error. Given that we can't do the early error, the rest of ES5 is clear: the call happens, and then only if it didn't return a Reference (no call in our system can), we throw.

/be

Comment 22

[Jim Blandy :jimb]

Yeah, I think that's probably right. Those puts are always erroneous; the only effect of Ch. 16's language is to make them early. Since we have decided not to make them early, the fallback position is to make them late, which entails doing the eval as per ES5 --- meaning direct.

Comment 23

[Brendan Eich [:brendan]]

Attachment: fix, v4

JSOP_SETCALL is now a suffix op emitted after JSOP_{CALL,EVAL,FUN{APPLY,CALL}}, which avoids loss of direct eval and simplifies things generally. It now truly is the op that always and only throws.

I did not fix bug 609756, but that is easier to fix now. It requires reordering eleminc to come before setcall, e.g.

/be

Comment 24

[Brendan Eich [:brendan]]

Attachment: fix, v4a

Ok, I would be shocked if some XUL JS that is fastloaded used JSOP_SETCALL, which changed arity, but just in case, bump JSXDR_BYTECODE_VERSION.

/be

Comment 25

[Jim Blandy :jimb]

Comment on attachment 488625 [details] [diff] [review]
fix, v4a

js> function f() { return 42; }  
js> function g() { f() = 3; }    
js> g()
typein:2: SyntaxError: invalid assignment left-hand side
js> 

That should be a ReferenceError. I think that's the case even when it's detected early, so just changing js.msg should be enough.

Doesn't this fix bug 609756, too?

js> function f() { print("hi"); }
js> function g() { f()++; }
js> g()
hi
typein:2: SyntaxError: invalid assignment left-hand side
js>

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #25)
> Comment on attachment 488625 [details] [diff] [review]
> fix, v4a
> 
> js> function f() { return 42; }  
> js> function g() { f() = 3; }    
> js> g()
> typein:2: SyntaxError: invalid assignment left-hand side
> js> 
> 
> That should be a ReferenceError. I think that's the case even when it's
> detected early, so just changing js.msg should be enough.

We've always done SyntaxError -- will fix.

> Doesn't this fix bug 609756, too?
> 
> js> function f() { print("hi"); }
> js> function g() { f()++; }
> js> g()
> hi
> typein:2: SyntaxError: invalid assignment left-hand side
> js>

That bug should be specifically about the case jorendorff came up with at bug 609756 comment 10:

    var x = 'ok';
    function f() {
        return {valueOf: function() {x = 'ok';}};
    }
    try {
        eval("x = 'bad'; f()++;");
        throw "expected an error";
    } catch (exc) {
        assert(exc instanceof ReferenceError);
    }
    assertEq(x, 'ok');  // fails

I'll adjust the summary.

/be

Comment 27

[Brendan Eich [:brendan]]

Attachment: patch to commit

Comment 28

[Brendan Eich [:brendan]]

Attachment: patch to commit, with test adjusted for ReferenceError not SyntaxError

Comment 29

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/0f78c4619855

/be

Comment 30

[Jim Blandy :jimb]

Looks good to me!

Comment 31

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0f78c4619855