Direct call to eval passes strict mode through.

Direct call to eval passes strict mode through.

BUG=
TEST=test/mjsunit/strict-mode-eval.js

[Martin Maly, 2011-02-02 05:07:26]

A draft really, there are some pending questions:

- how to best sniff that caller is strict mode (pass argument to the runtime
functions, walk stack frames, ...)
- what to do about debugger (looking for the right contact with whom to discuss
debugger issues)

Thank you!
Martin

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode7647
src/runtime.cc:7647: Handle<JSFunction>
caller(JSFunction::cast(frame->function()));
Alternative to walking the stack frames is passing additional argument (strict
mode flag). This works well but am not sure whether it is the right way to do
this. Open to suggestions.

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode7671
src/runtime.cc:7671: JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);
Alternative to walking the stack frames is passing additional argument (strict
mode flag). This works well but am not sure whether it is the right way to do
this. Open to suggestions.

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode9817
src/runtime.cc:9817: false);
Need to follow up with debugger folks what the right behavior is here. Who is
the best contact?

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode9900
src/runtime.cc:9900: Compiler::CompileEval(source, context, is_global, false);
ditto, follow up with debugger folks what the right behavior is.

[Mads Ager (chromium), 2011-02-02 10:16:05]

Adding Søren to the reviewer list to get input on the debugger part.

[Mads Ager (chromium), 2011-02-02 10:30:03]

What happens if you have an indirect call to eval from strict mode code. Should
the eval compile in strict mode or not?

function f(this_could_be_eval, s) { this_could_be_eval(s); }
f(eval, "with (String.prototype) { ... }")

Should that throw an exception or not? This hits the builtin eval function with
the global object as the scope currently. It does not go through the paths that
you have modified. Is non-strict the right behavior for these global evals
through aliases?

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode7647
src/runtime.cc:7647: Handle<JSFunction>
caller(JSFunction::cast(frame->function()));
On 2011/02/02 05:07:26, Martin Maly wrote:
> Alternative to walking the stack frames is passing additional argument (strict
> mode flag). This works well but am not sure whether it is the right way to do
> this. Open to suggestions.

When you are compiling the caller you know whether or not it is strict mode so I
would pass that in as an extra argument. Iterating stack frames is less direct
and it is also not cheap.

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode7671
src/runtime.cc:7671: JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);
On 2011/02/02 05:07:26, Martin Maly wrote:
> Alternative to walking the stack frames is passing additional argument (strict
> mode flag). This works well but am not sure whether it is the right way to do
> this. Open to suggestions.

I would go for the additional argument here too.

[Martin Maly, 2011-02-02 23:56:21]

Thanks for looking at the draft, Mads.
I changed the code to:
- pass the strict mode flag as an additional argument (changed all 6 codegens)
- eval then passes the flag back to compiler to compile in strict mode
- eval compiled code cache must take strict mode flag into consideration also.

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode7647
src/runtime.cc:7647: Handle<JSFunction>
caller(JSFunction::cast(frame->function()));
On 2011/02/02 10:30:03, Mads Ager wrote:
> On 2011/02/02 05:07:26, Martin Maly wrote:
> > Alternative to walking the stack frames is passing additional argument
(strict
> > mode flag). This works well but am not sure whether it is the right way to
do
> > this. Open to suggestions.
> 
> When you are compiling the caller you know whether or not it is strict mode so
I
> would pass that in as an extra argument. Iterating stack frames is less direct
> and it is also not cheap.

Done.

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode7671
src/runtime.cc:7671: JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);
On 2011/02/02 10:30:03, Mads Ager wrote:
> On 2011/02/02 05:07:26, Martin Maly wrote:
> > Alternative to walking the stack frames is passing additional argument
(strict
> > mode flag). This works well but am not sure whether it is the right way to
do
> > this. Open to suggestions.
> 
> I would go for the additional argument here too.

Done.

http://codereview.chromium.org/6286043/diff/1009/src/compilation-cache.h
File src/compilation-cache.h (right):

http://codereview.chromium.org/6286043/diff/1009/src/compilation-cache.h#newc...
src/compilation-cache.h:52: static Handle<SharedFunctionInfo>
LookupEval(Handle<String> source,
compiled pieces of eval code are cached so we must consider strict as part of
the equality comparison on the compiled snippets. Consider:
a.js:

eval("function arguments(){};");

b.js:

"use strict";
eval("function arguments(){};");

When executing both files (d8 a.js b.js) the second eval would experience cache
hit and code would never be compiled, error never reported etc.

http://codereview.chromium.org/6286043/diff/1009/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6286043/diff/1009/src/objects.cc#newcode8011
src/objects.cc:8011: hash ^= strict ? 0x8000 : 0x800;
Just a guess, there may be better (or more V8 code compliant) hash tricks. Open
to any suggestions.

http://codereview.chromium.org/6286043/diff/1009/src/objects.cc#newcode9006
src/objects.cc:9006: StringSharedKey key(src, context->closure()->shared(),
is_strict);
It seems weird that this function hashes by:
context->closure()->shared()
shouldn't we want to hash here by executing function? As far as I can see,
context->closure()->shared() is not always the executing JSFunction (if ever).

http://codereview.chromium.org/6286043/diff/1009/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6286043/diff/1009/src/objects.h#newcode4847
src/objects.h:4847: SharedFunctionInfo* value);
All callers passed SharedFunctionInfo so it seems better to use stronger type.
Plus, the PutEval function needs to add the strict mode-ness of the
SharedFunctionInfo into the hash key.

http://codereview.chromium.org/6286043/diff/1009/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6286043/diff/1009/src/parser.cc#newcode667
src/parser.cc:667: if (strict_mode) temp_scope.EnableStrictMode();
Restore strict mode (the eval call path)

http://codereview.chromium.org/6286043/diff/1009/src/parser.cc#newcode755
src/parser.cc:755: }
In lazy compilation, strict flag is stored on SharedFunctionInfo so here we
restore it back for the parser.

[Martin Maly, 2011-02-03 01:04:39]

On 2011/02/02 10:30:03, Mads Ager wrote:
> What happens if you have an indirect call to eval from strict mode code.
Should
> the eval compile in strict mode or not?


If it is direct call to eval, yes.


> function f(this_could_be_eval, s) { this_could_be_eval(s); }
> f(eval, "with (String.prototype) { ... }")
> 
> Should that throw an exception or not? This hits the builtin eval function
with
> the global object as the scope currently. It does not go through the paths
that
> you have modified. Is non-strict the right behavior for these global evals
> through aliases?

This is correct. eval may not be aliased (otherwise it is not a direct call to
eval and strict mode no longer passes through it, nor the current 'this'.
Instead, global object is provided as 'this')

> 
> http://codereview.chromium.org/6286043/diff/1/src/runtime.cc
> File src/runtime.cc (right):
> 
> http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode7647
> src/runtime.cc:7647: Handle<JSFunction>
> caller(JSFunction::cast(frame->function()));
> On 2011/02/02 05:07:26, Martin Maly wrote:
> > Alternative to walking the stack frames is passing additional argument
(strict
> > mode flag). This works well but am not sure whether it is the right way to
do
> > this. Open to suggestions.
> 
> When you are compiling the caller you know whether or not it is strict mode so
I
> would pass that in as an extra argument. Iterating stack frames is less direct
> and it is also not cheap.
> 
> http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode7671
> src/runtime.cc:7671: JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);
> On 2011/02/02 05:07:26, Martin Maly wrote:
> > Alternative to walking the stack frames is passing additional argument
(strict
> > mode flag). This works well but am not sure whether it is the right way to
do
> > this. Open to suggestions.
> 
> I would go for the additional argument here too.

[Lasse Reichstein, 2011-02-03 12:10:41]

LGTM

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode7557
src/runtime.cc:7557: false);
Passing too many boolean literals (some say one is too many) makes the code hard
to read. It's not obvious what "true" and "false" means here.

At least name the argument here, e.g.,
 bool is_strict = false;
 ... ::CompileEval(source, context, true, is_strict);
or better yet, introduce an enum type for code strictness with kStrict and
kNonStrict as members, and use that to pass around strictness parameters.

http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.js
File test/mjsunit/strict-mode-eval.js (right):

http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.j...
test/mjsunit/strict-mode-eval.js:31: var no_exc = true;
is "exc" short for "exception"?
I typically see "exn" for that (not that it's much better, just spell it all
out).

http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.j...
test/mjsunit/strict-mode-eval.js:34: no_exc = false;
Just use
 assertUnreachable("did not throw exception")
here.

http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.j...
test/mjsunit/strict-mode-eval.js:53: eval("var x = '\\020;");
The string looks unterminated. You are probably not getting the octal-escape
SyntaxError.

Perhaps make check for each argument to eval that it is not an error in
non-strict mode.

Why the nested functions?

[Lasse Reichstein, 2011-02-03 12:36:01]

More comments.

http://codereview.chromium.org/6286043/diff/1009/src/compilation-cache.h
File src/compilation-cache.h (right):

http://codereview.chromium.org/6286043/diff/1009/src/compilation-cache.h#newc...
src/compilation-cache.h:52: static Handle<SharedFunctionInfo>
LookupEval(Handle<String> source,
Good catch. We wouldn't want that.

http://codereview.chromium.org/6286043/diff/1009/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/6286043/diff/1009/src/compiler.h#newcode168
src/compiler.h:168: class IsStrict: public BitField<bool, 4, 1> {};
Move it below IsInLoop, to keep bits in order.

http://codereview.chromium.org/6286043/diff/1009/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6286043/diff/1009/src/objects.cc#newcode7993
src/objects.cc:7993: bool strict = Smi::cast(pair->get(2))->value() != 0;
Do you need to store the strictness in a separate field? Could we store it on
the SharedFunctionInfo instead?

http://codereview.chromium.org/6286043/diff/1009/src/objects.cc#newcode8011
src/objects.cc:8011: hash ^= strict ? 0x8000 : 0x800;
I don't think it matters what you do for strict-mode hash, since we are unlikely
to have the same code as both strict and non-strict. I wouldn't mind a hash
collision in that case (but xoring by 0x8000 (or anything non-zero) in one case
is fine, I don't see why you xor by something non-zero in the other case).

http://codereview.chromium.org/6286043/diff/1009/src/objects.cc#newcode9006
src/objects.cc:9006: StringSharedKey key(src, context->closure()->shared(),
is_strict);
I'm not sure I understand the question.
You can have many JSFunctions created using the same SharedFunctionInfo. The
SharedFunctionInfo corresponds to the function literal in the source, so it
identifies the function code as well as the closure that it has access to.
If a two functions have the same SharedFunctionInfo, they will be able to use
the same code, because they run in the same environment.

http://codereview.chromium.org/6286043/diff/1009/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6286043/diff/1009/src/objects.h#newcode4175
src/objects.h:4175: inline void set_strict_mode(bool value);
Is the argument needed? I.e., do we ever set a function back from strict mode to
non-strict mode?

http://codereview.chromium.org/6286043/diff/1009/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6286043/diff/1009/src/parser.cc#newcode755
src/parser.cc:755: }
Is it only for lazily compiled functions that the SharedFunctionInfo holds the
strict-mode, or is it all SharedFunctionInfo?

http://codereview.chromium.org/6286043/diff/1009/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6286043/diff/1009/src/runtime.cc#newcode9816
src/runtime.cc:9816: false);
It deserves a comment that we always run the code as non-strict, even in a
strict code context.

[Lasse Reichstein, 2011-02-03 13:25:33]

And I'll just add a reminder to everyone (self included) that we still need to
handle direct calls to eval in strict code correctly: It should run in a
separate lexical and variable environment and not be able to add variables to
the surrounding variable scope.

[Martin Maly, 2011-02-04 01:02:34]

Thanks for the review, Lasse, I applied all changes. Because passing the enum
instead of bool around touches a lot of places (same places I already modified
but nonetheless) I am not committing just yet and will wait for you to take
another look, if you like.

I plan to commit tomorrow unless I hear otherwise.

Thank you!
Martin

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode7557
src/runtime.cc:7557: false);
On 2011/02/03 12:10:41, Lasse Reichstein wrote:
> Passing too many boolean literals (some say one is too many) makes the code
hard
> to read. It's not obvious what "true" and "false" means here.
> 
> At least name the argument here, e.g.,
>  bool is_strict = false;
>  ... ::CompileEval(source, context, true, is_strict);
> or better yet, introduce an enum type for code strictness with kStrict and
> kNonStrict as members, and use that to pass around strictness parameters.

Done.

http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.js
File test/mjsunit/strict-mode-eval.js (right):

http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.j...
test/mjsunit/strict-mode-eval.js:31: var no_exc = true;
On 2011/02/03 12:10:41, Lasse Reichstein wrote:
> is "exc" short for "exception"?
> I typically see "exn" for that (not that it's much better, just spell it all
> out).

Done.

http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.j...
test/mjsunit/strict-mode-eval.js:34: no_exc = false;
On 2011/02/03 12:10:41, Lasse Reichstein wrote:
> Just use
>  assertUnreachable("did not throw exception")
> here.

Done.

http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.j...
test/mjsunit/strict-mode-eval.js:53: eval("var x = '\\020;");
The nested functions exercise the lazy compilation. Strictly speaking the 4
levels are not necessary but it tests yet another facet of the code.

On 2011/02/03 12:10:41, Lasse Reichstein wrote:
> The string looks unterminated. You are probably not getting the octal-escape
> SyntaxError.
> 
> Perhaps make check for each argument to eval that it is not an error in
> non-strict mode.
> 
> Why the nested functions?

http://codereview.chromium.org/6286043/diff/1009/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/6286043/diff/1009/src/compiler.h#newcode168
src/compiler.h:168: class IsStrict: public BitField<bool, 4, 1> {};
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
> Move it below IsInLoop, to keep bits in order.

Done. Added comment to clarify that IsStrict is used in eager compilation (that
was originally the motivation for the out of order bits).

http://codereview.chromium.org/6286043/diff/1009/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6286043/diff/1009/src/objects.cc#newcode7993
src/objects.cc:7993: bool strict = Smi::cast(pair->get(2))->value() != 0;
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
> Do you need to store the strictness in a separate field? Could we store it on
> the SharedFunctionInfo instead?

I believe I do need to store the strict flag. The strict is stored on the
SharedFunctionInfo but the SharedFunctionInfo is not always the info of the
immediately calling JSFunction. The canonical counter-example is:

"use strict";
eval("x = 1");

When we call into the Runtime_ResolvePossiblyDirectEval I do:

StackFrameLocator locator;
JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);

And observe that:

Top::context()->closure() != frame->function()

and moreover:
Top::context()->closure()->shared()->strict_mode() == false

whereas:
((JSFunction*)(frame->function()))->shared()->strict_mode() == true

So the SharedFunctionInfo we have available here as a key into the eval cache is
not sufficient piece of information to key off of ... so I have to store the
strict mode flag in the key explicitly.

Unless the disparity I describe above is symptom of some deeper problem in the
system.

http://codereview.chromium.org/6286043/diff/1009/src/objects.cc#newcode8011
src/objects.cc:8011: hash ^= strict ? 0x8000 : 0x800;
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
> I don't think it matters what you do for strict-mode hash, since we are
unlikely
> to have the same code as both strict and non-strict. I wouldn't mind a hash
> collision in that case (but xoring by 0x8000 (or anything non-zero) in one
case
> is fine, I don't see why you xor by something non-zero in the other case).

Done.

http://codereview.chromium.org/6286043/diff/1009/src/objects.cc#newcode9006
src/objects.cc:9006: StringSharedKey key(src, context->closure()->shared(),
is_strict);
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
> I'm not sure I understand the question.
> You can have many JSFunctions created using the same SharedFunctionInfo. The
> SharedFunctionInfo corresponds to the function literal in the source, so it
> identifies the function code as well as the closure that it has access to.
> If a two functions have the same SharedFunctionInfo, they will be able to use
> the same code, because they run in the same environment.

Makes sense, thanks for the explanation.

http://codereview.chromium.org/6286043/diff/1009/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6286043/diff/1009/src/objects.h#newcode4175
src/objects.h:4175: inline void set_strict_mode(bool value);
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
> Is the argument needed? I.e., do we ever set a function back from strict mode
to
> non-strict mode?

Strictly speaking, the argument is not needed (assuming that GC heap allocated
objects are zeroed out by the memory allocator).
Function never transitions strict->non-strict. However, where the
SharedFunctionInfo is being set up, all flags on shared info are set in similar
way:
function_info_>set_xxx_xxx(lit->xxx_xxx());
So the shared function info transitions from "newly created" to either strict or
non-strict.
It felt cleaner to keep the same code pattern across the board than have strict
mode behaving differently than all the other flags. I am open to changing this
although this version of code has the high degree of consistency going for it.

http://codereview.chromium.org/6286043/diff/1009/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6286043/diff/1009/src/parser.cc#newcode755
src/parser.cc:755: }
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
> Is it only for lazily compiled functions that the SharedFunctionInfo holds the
> strict-mode, or is it all SharedFunctionInfo?

All SharedFunctionInfo have the correct strict mode set (unless I have bug
somewhere but I tried my best to ensure that all SharedFunctionInfo have the
strict mode flag set). Whenever SharedFunctionInfo is created the strict mode
flag is copied to it from the FunctionLiteral.

http://codereview.chromium.org/6286043/diff/1009/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6286043/diff/1009/src/runtime.cc#newcode9816
src/runtime.cc:9816: false);
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
> It deserves a comment that we always run the code as non-strict, even in a
> strict code context.

Done.