Test for var declarations in eval which conflict with let

Test for var declarations in eval which conflict with let

Previously, name conflicts between var and let declarations were only
made into exceptions if they were visible at parse-time. This patch adds
runtime checks so that sloppy-mode direct eval can't introduce conflicting
var declarations. The change is implemented by traversing the scope chain
when a direct eval introduces a var declaration to look for conflicting
let declarations, up to the function boundary.

BUG=v8:4454
R=adamk
LOG=Y

Committed: https://crrev.com/d515e5138d30ab5b2a79e4cac99fcc5ca8d8991b
Cr-Commit-Position: refs/heads/master@{#31211}

[Dan Ehrenberg, 2015-10-02 23:42:07]

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

[commit-bot: I haz the power, 2015-10-02 23:42:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1382513003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1382513003/1

[commit-bot: I haz the power, 2015-10-03 00:06:20]

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

[commit-bot: I haz the power, 2015-10-03 00:06:21]

Dry run: This issue passed the CQ dry run.

[adamk, 2015-10-05 17:20:35]

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

[adamk, 2015-10-05 17:20:36]

+mstarzinger for more eyes on the Context code (picked via git blame, feel free
to find someone else who should look at this instead)

The double lookup makes me sad, trying to think of a way to avoid that, either
by only doing the extra check for sloppy evals (maybe by putting a bit on the
Variable we allocate in that case?) or by somehow combining the two context
lookups (but I realize they have rather different rules).

https://codereview.chromium.org/1382513003/diff/1/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1382513003/diff/1/src/contexts.h#newcode19
src/contexts.h:19: SKIP_WITH_CONTEXT = 8,
Can you change these to "1 << N" form now that we're growing more of them?

https://codereview.chromium.org/1382513003/diff/1/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/1382513003/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:230: flags = LEXICAL_TEST;
Rather than re-assignining this variable it seems like you could just pass the
appropriate value to Lookup in both places.

https://codereview.chromium.org/1382513003/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:231: context_arg->Lookup(name, flags, &index,
&attributes, &binding_flags);
It seems a shame to do this extra context chain walk for cases that aren't
sloppy eval. Is there any way to distinguish that case from other cases (such as
assignments to globals)?

https://codereview.chromium.org/1382513003/diff/1/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-4454.js (right):

https://codereview.chromium.org/1382513003/diff/1/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-4454.js:5: // Flags: --harmony-sloppy
--harmony-sloppy-let --harmony-sloppy-function --no-legacy-const
Nit: split this line into two "Flags: " lines to fit under 80 chars.

https://codereview.chromium.org/1382513003/diff/1/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-4454.js:15: eval('var x = 2');
Can you add tests for function declarations (I'm not actually sure what ES6 says
eval of a sloppy function inside a block should do...)?

Also I'd ideally like to see tests for legacy const, though that'll have to be
in a separate file.

[Dan Ehrenberg, 2015-10-05 18:27:14]

https://codereview.chromium.org/1382513003/diff/1/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-4454.js (right):

https://codereview.chromium.org/1382513003/diff/1/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-4454.js:15: eval('var x = 2');
On 2015/10/05 at 17:20:36, adamk wrote:
> Can you add tests for function declarations (I'm not actually sure what ES6
says eval of a sloppy function inside a block should do...)?
> 
> Also I'd ideally like to see tests for legacy const, though that'll have to be
in a separate file.

The spec says that no hoisting out of lexical blocks happens in the top level of
eval, but I bet this is web-incompatible. I think the behavior needs to be that
it hoists out just like a var declaration and turns into assignments.

[Dan Ehrenberg, 2015-10-08 23:01:26]

Added more tests and skipped the traversal if it's not from an eval.

https://codereview.chromium.org/1382513003/diff/1/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1382513003/diff/1/src/contexts.h#newcode19
src/contexts.h:19: SKIP_WITH_CONTEXT = 8,
On 2015/10/05 at 17:20:36, adamk wrote:
> Can you change these to "1 << N" form now that we're growing more of them?

Done

https://codereview.chromium.org/1382513003/diff/1/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/1382513003/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:230: flags = LEXICAL_TEST;
On 2015/10/05 at 17:20:36, adamk wrote:
> Rather than re-assignining this variable it seems like you could just pass the
appropriate value to Lookup in both places.

Done

https://codereview.chromium.org/1382513003/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:231: context_arg->Lookup(name, flags, &index,
&attributes, &binding_flags);
On 2015/10/05 at 17:20:36, adamk wrote:
> It seems a shame to do this extra context chain walk for cases that aren't
sloppy eval. Is there any way to distinguish that case from other cases (such as
assignments to globals)?

Done

[Dan Ehrenberg, 2015-10-08 23:01:33]

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

[commit-bot: I haz the power, 2015-10-08 23:01:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1382513003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1382513003/20001

[commit-bot: I haz the power, 2015-10-08 23:03:54]

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

[commit-bot: I haz the power, 2015-10-08 23:03:55]

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

[Dan Ehrenberg, 2015-10-08 23:28:48]

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

[commit-bot: I haz the power, 2015-10-08 23:29:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1382513003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1382513003/40001

[commit-bot: I haz the power, 2015-10-08 23:33:02]

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

[commit-bot: I haz the power, 2015-10-08 23:33:03]

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

[Dan Ehrenberg, 2015-10-09 00:07:24]

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

[commit-bot: I haz the power, 2015-10-09 00:07:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1382513003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1382513003/60001

[commit-bot: I haz the power, 2015-10-09 00:19:08]

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

[commit-bot: I haz the power, 2015-10-09 00:19:09]

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

[Dan Ehrenberg, 2015-10-09 00:31:45]

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

[commit-bot: I haz the power, 2015-10-09 00:32:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1382513003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1382513003/80001

[commit-bot: I haz the power, 2015-10-09 00:52:29]

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

[commit-bot: I haz the power, 2015-10-09 00:52:30]

Dry run: This issue passed the CQ dry run.

[adamk, 2015-10-09 15:53:45]

This change lgtm.

mstarzinger, please review (you're needed for full-codegen/OWNERS anyway, but
I'm also interested in your take on this approach).

[Michael Starzinger, 2015-10-12 09:17:34]

LGTM for full-codegen. I don't have an opinion about the changes to the context
lookup code, I think you should have either rossberg@ or verwaest@ take a look
at that.

[Dan Ehrenberg, 2015-10-12 09:21:48]

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

[Toon Verwaest, 2015-10-12 13:50:35]

LGTM
I'm not particularly a fan of abusing PropertyAttributes for all kinds of flags,
but as this CL doesn't introduce the abuse, I guess it's fine for now.

[Dan Ehrenberg, 2015-10-12 14:07:19]

On 2015/10/12 at 13:50:35, verwaest wrote:
> LGTM
> I'm not particularly a fan of abusing PropertyAttributes for all kinds of
flags, but as this CL doesn't introduce the abuse, I guess it's fine for now.

Yeah I wasn't so happy about doing this either, but it seemed like the most
straightforward way to pass the information.

[Dan Ehrenberg, 2015-10-12 14:07:22]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2015-10-12 14:07:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1382513003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1382513003/80001

[commit-bot: I haz the power, 2015-10-12 14:30:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-12 14:31:12]

Patchset 5 (id:??) landed as
https://crrev.com/d515e5138d30ab5b2a79e4cac99fcc5ca8d8991b
Cr-Commit-Position: refs/heads/master@{#31211}