Use a different map to distinguish eval contexts

Use a different map to distinguish eval contexts

eval() may introduce a scope which needs to be represented as a context at
runtime, e.g.,

  eval('var x; let y; ()=>y')

introduces a variable y which needs to have a context allocated for it. However,
when traversing upwards to find the declaration context for a variable which leaks,
as the declaration of x does above, this context has to be understood to not be
a declaration context in sloppy mode.

This patch makes that distinction by introducing a different map for eval-introduced
contexts. A dynamic search for the appropriate context will continue past an eval
context to find the appropriate context. Marking contexts as eval contexts rather
than function contexts required updates in each compiler backend.

BUG=v8:5295, chromium:648719
Review-Url: https://codereview.chromium.org/2435023002
Cr-Commit-Position: refs/heads/master@{#41869}
Committed: https://chromium.googlesource.com/v8/v8/+/53fdf9d1926c6a6f9a2d2acf0c303ae4c71210a1

[Dan Ehrenberg, 2016-10-20 12:54:44]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 12:54:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 13:03:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 13:03:25]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/26955)

[Dan Ehrenberg, 2016-10-20 13:46:52]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 13:47:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-10-20 13:49:20]

Description was changed from

==========
Fix up eval scopes

TODO:
- Restore commented-out DCHECK (by weakining it? the closure may be different)
- Pass in the 'eval' flag from more places, not just fullcodegen on x86

BUG=v8:5295
==========

to

==========
Use a different map to distinguish eval contexts

eval() may introduce a scope which needs to be represented as a context at
runtime, e.g.,

  eval('let y; ()=>y')

introduces a variable y which needs ...

BUG=v8:5295
==========

[commit-bot: I haz the power, 2016-10-20 13:52:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 13:52:52]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11217)

[Dan Ehrenberg, 2016-10-20 14:07:49]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 14:07:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-10-20 14:10:55]

Description was changed from

==========
Use a different map to distinguish eval contexts

eval() may introduce a scope which needs to be represented as a context at
runtime, e.g.,

  eval('let y; ()=>y')

introduces a variable y which needs ...

BUG=v8:5295
==========

to

==========
Use a different map to distinguish eval contexts

eval() may introduce a scope which needs to be represented as a context at
runtime, e.g.,

  eval('var x; let y; ()=>y')

introduces a variable y which needs to have a context allocated for it. However,
when traversing upwards to find the declaration context for a variable which
leaks,
as the declaration of x does above, this context has to be understood to not be
a declaration context in sloppy mode.

This patch makes that distinction by introducing a different map for
eval-introduced
contexts. A dynamic search for the appropriate context will continue past an
eval
context to find the appropriate context. Marking contexts as eval contexts
rather
than function contexts required updates in each compiler backend.

BUG=v8:5295
==========

[commit-bot: I haz the power, 2016-10-20 14:24:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 14:24:48]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/16489)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[adamk, 2016-11-10 19:32:06]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2016-11-10 19:32:07]

https://codereview.chromium.org/2435023002/diff/60001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2435023002/diff/60001/src/contexts.cc#newcode116
src/contexts.cc:116: if (IsFunctionContext() || IsModuleContext()) {
I think you want to add "|| IsEvalContext()" here.

[Dan Ehrenberg, 2016-11-10 19:52:23]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 19:52:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 19:56:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-10 19:56:03]

Dry run: Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)

[Dan Ehrenberg, 2016-11-11 23:51:56]

littledan@chromium.org changed reviewers:
+ mstarzinger@chromium.org, yangguo@chromium.org

[Dan Ehrenberg, 2016-11-11 23:51:57]

Not ported to all backends yet, but I'd like some feedback about this approach,
the factoring, etc. before typing all that out.

https://codereview.chromium.org/2435023002/diff/60001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2435023002/diff/60001/src/contexts.cc#newcode116
src/contexts.cc:116: if (IsFunctionContext() || IsModuleContext()) {
On 2016/11/10 at 19:32:07, adamk ooo until nov 28 wrote:
> I think you want to add "|| IsEvalContext()" here.

Added that here and in a bunch of other places

[Dan Ehrenberg, 2016-11-11 23:52:02]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 23:52:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 23:56:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 23:56:02]

Dry run: Try jobs failed on following builders:
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[Dan Ehrenberg, 2016-11-12 00:01:52]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-12 00:01:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-12 00:04:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-12 00:04:28]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28414)

[adamk, 2016-11-12 00:24:16]

Description was changed from

==========
Use a different map to distinguish eval contexts

eval() may introduce a scope which needs to be represented as a context at
runtime, e.g.,

  eval('var x; let y; ()=>y')

introduces a variable y which needs to have a context allocated for it. However,
when traversing upwards to find the declaration context for a variable which
leaks,
as the declaration of x does above, this context has to be understood to not be
a declaration context in sloppy mode.

This patch makes that distinction by introducing a different map for
eval-introduced
contexts. A dynamic search for the appropriate context will continue past an
eval
context to find the appropriate context. Marking contexts as eval contexts
rather
than function contexts required updates in each compiler backend.

BUG=v8:5295
==========

to

==========
Use a different map to distinguish eval contexts

eval() may introduce a scope which needs to be represented as a context at
runtime, e.g.,

  eval('var x; let y; ()=>y')

introduces a variable y which needs to have a context allocated for it. However,
when traversing upwards to find the declaration context for a variable which
leaks,
as the declaration of x does above, this context has to be understood to not be
a declaration context in sloppy mode.

This patch makes that distinction by introducing a different map for
eval-introduced
contexts. A dynamic search for the appropriate context will continue past an
eval
context to find the appropriate context. Marking contexts as eval contexts
rather
than function contexts required updates in each compiler backend.

BUG=v8:5295, chromium:648719
==========

[adamk, 2016-11-12 00:26:06]

I hope mstarzinger might have some suggestion about how to refactor in a less
verbose way, but I don't see an easy way (but then I'm not very familiar with
the bits of code involving FastNewFunctionContextStub).

https://codereview.chromium.org/2435023002/diff/220001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:366: outer_scope =
Can you add a DCHECK here that scope_info->language_mode() is strict?

https://codereview.chromium.org/2435023002/diff/220001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/contexts.cc#newcode78
src/contexts.cc:78: // Note: the closure may change while finding the
declaration context,
Seems like this comment won't be as useful after this CL lands (since it's not
clear why it's here after the DCHECK dies), I'd just leave it out.

https://codereview.chromium.org/2435023002/diff/220001/src/contexts.cc#newcod...
src/contexts.cc:313: (context->IsFunctionContext() || context->IsEvalContext())
Isn't this logic already present in Context::scope_info()?

https://codereview.chromium.org/2435023002/diff/220001/src/contexts.cc#newcod...
src/contexts.cc:338: (context->IsFunctionContext() || context->IsEvalContext()))
{
I don't think it makes sense to add IsEvalContext here, there's no function name
in eval scopes.

https://codereview.chromium.org/2435023002/diff/220001/src/debug/debug-scopes.cc
File src/debug/debug-scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:380: } else if (context_->IsFunctionContext() ||
context_->IsEvalContext()) {
Again, it seems like this is duplicating logic from Context::scope_info()

https://codereview.chromium.org/2435023002/diff/220001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/factory.cc#newcode895
src/factory.cc:895: DCHECK(function->shared()->scope_info()->scope_type() ==
FUNCTION_SCOPE);
I would have expected this DCHECK to fail...shouldn't this be EVAL_SCOPE?

https://codereview.chromium.org/2435023002/diff/220001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:2938:
builder()->CreateEvalContext(slot_count);
Does this need the same branch as we have everywhere else for kMaximumSlots?

https://codereview.chromium.org/2435023002/diff/220001/src/runtime/runtime-sc...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/runtime/runtime-sc...
src/runtime/runtime-scopes.cc:316: DCHECK(context->IsFunctionContext() ||
context->IsEvalContext());
We should never get here for an eval context, since a sloppy eval context never
gets an extension object.

https://codereview.chromium.org/2435023002/diff/220001/test/mjsunit/regress/r...
File test/mjsunit/regress/regress-5295.js (right):

https://codereview.chromium.org/2435023002/diff/220001/test/mjsunit/regress/r...
test/mjsunit/regress/regress-5295.js:12: assertEquals(1, foo());
Can you also add tests that does another eval inside the eval, and show that it
hoists all the way to the top?

[adamk, 2016-11-12 00:32:05]

https://codereview.chromium.org/2435023002/diff/240001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/240001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:369: outer_scope = new (zone) Scope(zone, EVAL_SCOPE,
handle(scope_info));
Actually looking at this again, it seems odd to create different scope types
here, given that ParserBase::NewEvalScope always creates a DeclarationScope. If
you think we should change this, maybe leave a TODO here to change code over
there too...

[neis, 2016-11-14 16:12:23]

On 2016/11/12 00:26:06, adamk ooo until nov 28 wrote:

>
https://codereview.chromium.org/2435023002/diff/220001/src/factory.cc#newcode895
> src/factory.cc:895: DCHECK(function->shared()->scope_info()->scope_type() ==
> FUNCTION_SCOPE);
> I would have expected this DCHECK to fail...shouldn't this be EVAL_SCOPE?

Agreed. I think none of our tests triggers the runtime function.

[Dan Ehrenberg, 2016-12-07 05:41:26]

Rebased and fixed things where comments were made in the review. Offline, Adam
gave me more feedback about code duplication. I'll follow up with another CL
version which reduces duplication and ports this patch to other platforms; no
need for a full review at the moment.

https://codereview.chromium.org/2435023002/diff/220001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:366: outer_scope =
On 2016/11/12 00:26:05, adamk wrote:
> Can you add a DCHECK here that scope_info->language_mode() is strict?

Actually, deleted this code, as it wasn't doing anything; in the future, we
could look into whether these should be Scopes sometimes, but the current logic
to make them always DeclarationScopes at parse-time works.

https://codereview.chromium.org/2435023002/diff/220001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/contexts.cc#newcode78
src/contexts.cc:78: // Note: the closure may change while finding the
declaration context,
On 2016/11/12 00:26:05, adamk wrote:
> Seems like this comment won't be as useful after this CL lands (since it's not
> clear why it's here after the DCHECK dies), I'd just leave it out.

Done.

https://codereview.chromium.org/2435023002/diff/220001/src/contexts.cc#newcod...
src/contexts.cc:313: (context->IsFunctionContext() || context->IsEvalContext())
On 2016/11/12 00:26:05, adamk wrote:
> Isn't this logic already present in Context::scope_info()?

Done.

https://codereview.chromium.org/2435023002/diff/220001/src/contexts.cc#newcod...
src/contexts.cc:338: (context->IsFunctionContext() || context->IsEvalContext()))
{
On 2016/11/12 00:26:05, adamk wrote:
> I don't think it makes sense to add IsEvalContext here, there's no function
name
> in eval scopes.

Done.

https://codereview.chromium.org/2435023002/diff/220001/src/debug/debug-scopes.cc
File src/debug/debug-scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:380: } else if (context_->IsFunctionContext() ||
context_->IsEvalContext()) {
On 2016/11/12 00:26:05, adamk wrote:
> Again, it seems like this is duplicating logic from Context::scope_info()

Done.

https://codereview.chromium.org/2435023002/diff/220001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/factory.cc#newcode895
src/factory.cc:895: DCHECK(function->shared()->scope_info()->scope_type() ==
FUNCTION_SCOPE);
On 2016/11/12 at 00:26:05, adamk wrote:
> I would have expected this DCHECK to fail...shouldn't this be EVAL_SCOPE?

Fixed the DCHECK. Added a new test which triggers this path in
test/mjsunit/regress/regress-655573.js . Verified that it crashes in a debug
build with the incorrect DCHECK.

https://codereview.chromium.org/2435023002/diff/220001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:2938:
builder()->CreateEvalContext(slot_count);
On 2016/11/12 at 00:26:05, adamk wrote:
> Does this need the same branch as we have everywhere else for kMaximumSlots?

Yes, fixed.

https://codereview.chromium.org/2435023002/diff/220001/src/runtime/runtime-sc...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/220001/src/runtime/runtime-sc...
src/runtime/runtime-scopes.cc:316: DCHECK(context->IsFunctionContext() ||
context->IsEvalContext());
On 2016/11/12 at 00:26:05, adamk wrote:
> We should never get here for an eval context, since a sloppy eval context
never gets an extension object.

Reverted and added a comment explaining the DCHECK.

https://codereview.chromium.org/2435023002/diff/220001/test/mjsunit/regress/r...
File test/mjsunit/regress/regress-5295.js (right):

https://codereview.chromium.org/2435023002/diff/220001/test/mjsunit/regress/r...
test/mjsunit/regress/regress-5295.js:12: assertEquals(1, foo());
On 2016/11/12 at 00:26:05, adamk wrote:
> Can you also add tests that does another eval inside the eval, and show that
it hoists all the way to the top?

Done

https://codereview.chromium.org/2435023002/diff/240001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/240001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:369: outer_scope = new (zone) Scope(zone, EVAL_SCOPE,
handle(scope_info));
On 2016/11/12 00:32:05, adamk wrote:
> Actually looking at this again, it seems odd to create different scope types
> here, given that ParserBase::NewEvalScope always creates a DeclarationScope.
If
> you think we should change this, maybe leave a TODO here to change code over
> there too...

You're right, changed it to a DeclarationScope.

[Dan Ehrenberg, 2016-12-08 02:08:43]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 02:08:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 02:13:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 02:14:00]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/19193)

[Dan Ehrenberg, 2016-12-09 01:18:10]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 01:18:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 01:23:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 01:23:45]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13941)

[Dan Ehrenberg, 2016-12-09 01:54:55]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 01:55:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-12-09 01:55:44]

The CQ bit was unchecked by littledan@chromium.org

[Dan Ehrenberg, 2016-12-09 01:55:49]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 01:55:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-12-09 02:12:05]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 02:12:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-12-09 02:23:09]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 02:23:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 02:47:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 02:47:09]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13944)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Dan Ehrenberg, 2016-12-09 23:55:29]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 23:55:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 00:20:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 00:20:40]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17576)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[Dan Ehrenberg, 2016-12-13 00:31:39]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 00:31:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 00:33:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 00:33:21]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/19399)

[Dan Ehrenberg, 2016-12-13 00:39:35]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 00:39:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 00:43:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 00:43:31]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30469)

[Dan Ehrenberg, 2016-12-13 07:21:31]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 07:21:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-12-13 07:24:20]

The new version has much less duplication, and more tests, per review comments.
PTAL.

[commit-bot: I haz the power, 2016-12-13 07:50:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 07:50:14]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-12-13 17:15:06]

littledan@chromium.org changed reviewers:
+ marja@chromium.org, neis@chromium.org

[Dan Ehrenberg, 2016-12-13 17:15:31]

+marja, neis (for a closely related bug) Could someone take a look please?

[marja, 2016-12-14 10:52:54]

Unfortunately I don't have the expertise to review this. The frontend changes
(scopes.cc) trivially LG.

[adamk, 2016-12-14 12:23:31]

Looking good, but I'm not qualified to sign off on the code stub/TF stuff.
mstarzinger, perhaps you could take a look?

https://codereview.chromium.org/2435023002/diff/540001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2435023002/diff/540001/src/contexts.cc#newcod...
src/contexts.cc:230: context->IsFunctionContext() || context->IsEvalContext() ||
I don't think you should need IsEvalContext() here: if this is a sloppy eval
context, then it never had anything added to its extension object (because
declaration_context() would skip over it). And if it's a strict eval context,
then it couldn't have anything in its extension object because it couldn't
possibly have a sloppy eval inside it.

You could add add a DCHECK above this if statement to that effect:

DCHECK_IMPLIES(context->IsEvalContext(), context->extension()->IsTheHole());

https://codereview.chromium.org/2435023002/diff/540001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/2435023002/diff/540001/src/factory.h#newcode299
src/factory.h:299: // Create a function or eval context.
Can you mention here that scope_type should be one of FUNCTION_SCOPE/EVAL_SCOPE?

https://codereview.chromium.org/2435023002/diff/540001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2435023002/diff/540001/src/flag-definitions.h...
src/flag-definitions.h:1139:
DEFINE_BOOL(test_small_max_function_context_stub_size, false,
Can you elaborate on why a flag was needed here? Did you try creating a test
case that would exceed the real maximum slots and find it was too slow?

[neis, 2016-12-14 13:59:37]

https://codereview.chromium.org/2435023002/diff/540001/src/runtime/runtime-sc...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/540001/src/runtime/runtime-sc...
src/runtime/runtime-scopes.cc:230: // not the declaration context.
Comment needs an update.

[Dan Ehrenberg, 2016-12-14 22:53:17]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 22:53:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-12-14 22:53:31]

https://codereview.chromium.org/2435023002/diff/540001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2435023002/diff/540001/src/contexts.cc#newcod...
src/contexts.cc:230: context->IsFunctionContext() || context->IsEvalContext() ||
On 2016/12/14 12:23:31, adamk wrote:
> I don't think you should need IsEvalContext() here: if this is a sloppy eval
> context, then it never had anything added to its extension object (because
> declaration_context() would skip over it). And if it's a strict eval context,
> then it couldn't have anything in its extension object because it couldn't
> possibly have a sloppy eval inside it.
> 
> You could add add a DCHECK above this if statement to that effect:
> 
> DCHECK_IMPLIES(context->IsEvalContext(), context->extension()->IsTheHole());

Done.

https://codereview.chromium.org/2435023002/diff/540001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/2435023002/diff/540001/src/factory.h#newcode299
src/factory.h:299: // Create a function or eval context.
On 2016/12/14 at 12:23:31, adamk wrote:
> Can you mention here that scope_type should be one of
FUNCTION_SCOPE/EVAL_SCOPE?

There are tons of places where scope_type is an instance variable or argument.
Should I put that everywhere? I thought it might be enough to have the DCHECKs.

https://codereview.chromium.org/2435023002/diff/540001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2435023002/diff/540001/src/flag-definitions.h...
src/flag-definitions.h:1139:
DEFINE_BOOL(test_small_max_function_context_stub_size, false,
On 2016/12/14 at 12:23:31, adamk wrote:
> Can you elaborate on why a flag was needed here? Did you try creating a test
case that would exceed the real maximum slots and find it was too slow?

Yes, I found that it consistently made my Z620 run out of memory and freeze up.
It also failed in automation. I manually verified that if I turn on the flag and
introduce a bug that makes the bytecode generator always use FUNCTION_SCOPE for
the overflow path, then one of the newly added tests fails. I didn't add a
comment in the source because none of the other flags have comments.

https://codereview.chromium.org/2435023002/diff/540001/src/runtime/runtime-sc...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/540001/src/runtime/runtime-sc...
src/runtime/runtime-scopes.cc:230: // not the declaration context.
On 2016/12/14 13:59:37, neis wrote:
> Comment needs an update.

Added eval to the list. Added a test for the function path here (previously
there had only been one for var).

[commit-bot: I haz the power, 2016-12-14 22:57:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 22:57:35]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17812)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/14204)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)

[adamk, 2016-12-15 08:46:53]

https://codereview.chromium.org/2435023002/diff/540001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/2435023002/diff/540001/src/factory.h#newcode299
src/factory.h:299: // Create a function or eval context.
On 2016/12/14 22:53:30, Dan Ehrenberg wrote:
> On 2016/12/14 at 12:23:31, adamk wrote:
> > Can you mention here that scope_type should be one of
> FUNCTION_SCOPE/EVAL_SCOPE?
> 
> There are tons of places where scope_type is an instance variable or argument.
> Should I put that everywhere? I thought it might be enough to have the
DCHECKs.

I commented on this case just because it happened to have a comment already. I
don't think you need to sprinkle this comment everywhere.

Also note that UNREACHABLE() is a CHECK, not a DCHECK (I think that's OK, just
want to make sure you're aware).

https://codereview.chromium.org/2435023002/diff/540001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2435023002/diff/540001/src/flag-definitions.h...
src/flag-definitions.h:1139:
DEFINE_BOOL(test_small_max_function_context_stub_size, false,
On 2016/12/14 22:53:31, Dan Ehrenberg wrote:
> On 2016/12/14 at 12:23:31, adamk wrote:
> > Can you elaborate on why a flag was needed here? Did you try creating a test
> case that would exceed the real maximum slots and find it was too slow?
> 
> Yes, I found that it consistently made my Z620 run out of memory and freeze
up.
> It also failed in automation. I manually verified that if I turn on the flag
and
> introduce a bug that makes the bytecode generator always use FUNCTION_SCOPE
for
> the overflow path, then one of the newly added tests fails. I didn't add a
> comment in the source because none of the other flags have comments.

Don't think you need to add a comment, this response is what I meant by
"elaborate". Thanks :)

[Michael Starzinger, 2016-12-15 09:42:59]

LGTM, cool stuff, just nits.

https://codereview.chromium.org/2435023002/diff/560001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2435023002/diff/560001/src/code-stubs.h#newco...
src/code-stubs.h:844: class ScopeTypeBits : public BitField<bool, 0, 8> {};
nit: Can we move this up three lines, so that the macro-generated members stay
at the bottom?

https://codereview.chromium.org/2435023002/diff/560001/src/debug/debug-scopes.cc
File src/debug/debug-scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/560001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:729: DCHECK(CurrentContext()->IsFunctionContext() ||
context_->IsEvalContext());
nit: s/context_/CurrentContext()/

https://codereview.chromium.org/2435023002/diff/560001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2435023002/diff/560001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3021:
->LoadAccumulatorWithRegister(Register::function_closure())
nit: Would MoveRegister(Register::function_closure(), args[0]) do the trick here
instead?

[neis, 2016-12-15 15:09:39]

Dan, please add the test from
https://bugs.chromium.org/p/v8/issues/detail?id=5736 to your CL, as the other
bugfix has landed now.

Thanks!

[neis, 2016-12-15 15:14:52]

BTW, what do you think of renaming CreateFunctionContext to
CreateFunctionOrEvalContext?  I find it confusing that CreateFunctionContext no
longer implies isFunctionContext.

[Dan Ehrenberg, 2016-12-15 23:08:51]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 23:09:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 23:38:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 23:38:39]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-12-15 23:51:50]

PTAL

https://codereview.chromium.org/2435023002/diff/560001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2435023002/diff/560001/src/code-stubs.h#newco...
src/code-stubs.h:844: class ScopeTypeBits : public BitField<bool, 0, 8> {};
On 2016/12/15 09:42:59, Michael Starzinger wrote:
> nit: Can we move this up three lines, so that the macro-generated members stay
> at the bottom?

Done.

https://codereview.chromium.org/2435023002/diff/560001/src/debug/debug-scopes.cc
File src/debug/debug-scopes.cc (right):

https://codereview.chromium.org/2435023002/diff/560001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:729: DCHECK(CurrentContext()->IsFunctionContext() ||
context_->IsEvalContext());
On 2016/12/15 09:42:59, Michael Starzinger wrote:
> nit: s/context_/CurrentContext()/

Done.

https://codereview.chromium.org/2435023002/diff/560001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2435023002/diff/560001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3021:
->LoadAccumulatorWithRegister(Register::function_closure())
On 2016/12/15 09:42:59, Michael Starzinger wrote:
> nit: Would MoveRegister(Register::function_closure(), args[0]) do the trick
here
> instead?

Done.

[marja, 2016-12-19 11:51:03]

neis@ is ooo so you shouldn't block this on his review. Is there anybody else
who needs to review this or should we consider it sufficiently reviewed? adamk@?

[adamk, 2016-12-19 17:59:26]

lgtm

I think the only thing neis@ was looking for was a rename, but I suspect he'd be
fine with doing that in a later change (especially given the size of this
patch). I'd say this is ready to go for now.

I think you'll need another owner for src/crankshaft/ (and maybe Yang for
src/debug/)?

[Dan Ehrenberg, 2016-12-19 21:16:54]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2016-12-19 21:16:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2435023002/#ps580001
(title: "Changes from review")

[commit-bot: I haz the power, 2016-12-19 21:17:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 21:20:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 21:20:40]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30999)

[Dan Ehrenberg, 2016-12-20 15:44:46]

littledan@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2016-12-20 16:12:00]

LGTM (rubber-stamped)

[Dan Ehrenberg, 2016-12-20 16:20:47]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2016-12-20 16:20:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 16:23:20]

CQ is committing da patch.

Bot data: {"patchset_id": 580001, "attempt_start_ts": 1482250847813100,
"parent_rev": "3d9c77d8126a8f48c3acb347cbf382052a9824ef", "commit_rev":
"53fdf9d1926c6a6f9a2d2acf0c303ae4c71210a1"}

[commit-bot: I haz the power, 2016-12-20 16:23:26]

Description was changed from

==========
Use a different map to distinguish eval contexts

eval() may introduce a scope which needs to be represented as a context at
runtime, e.g.,

  eval('var x; let y; ()=>y')

introduces a variable y which needs to have a context allocated for it. However,
when traversing upwards to find the declaration context for a variable which
leaks,
as the declaration of x does above, this context has to be understood to not be
a declaration context in sloppy mode.

This patch makes that distinction by introducing a different map for
eval-introduced
contexts. A dynamic search for the appropriate context will continue past an
eval
context to find the appropriate context. Marking contexts as eval contexts
rather
than function contexts required updates in each compiler backend.

BUG=v8:5295, chromium:648719
==========

to

==========
Use a different map to distinguish eval contexts

eval() may introduce a scope which needs to be represented as a context at
runtime, e.g.,

  eval('var x; let y; ()=>y')

introduces a variable y which needs to have a context allocated for it. However,
when traversing upwards to find the declaration context for a variable which
leaks,
as the declaration of x does above, this context has to be understood to not be
a declaration context in sloppy mode.

This patch makes that distinction by introducing a different map for
eval-introduced
contexts. A dynamic search for the appropriate context will continue past an
eval
context to find the appropriate context. Marking contexts as eval contexts
rather
than function contexts required updates in each compiler backend.

BUG=v8:5295, chromium:648719

Review-Url: https://codereview.chromium.org/2435023002
Cr-Commit-Position: refs/heads/master@{#41869}
Committed:
https://chromium.googlesource.com/v8/v8/+/53fdf9d1926c6a6f9a2d2acf0c303ae4c71...
==========

[commit-bot: I haz the power, 2016-12-20 16:23:27]

Committed patchset #30 (id:580001) as
https://chromium.googlesource.com/v8/v8/+/53fdf9d1926c6a6f9a2d2acf0c303ae4c71...