Remove CONST_LEGACY VariableMode

Remove CONST_LEGACY VariableMode

The only remaining use of this VariableMode is for the names of sloppy
named function expressions. This patch instead uses CONST for such
bindings (just as we do in strict mode) and instead marks those
Variables specially. During code generation a new helper method,
Variable::throw_on_const_assignment(), is called to decide whether
to throw or silently ignore the assignment.

Committed: https://crrev.com/7516fe1eaa7910015d7732a1aa34e45e3e37586c
Cr-Commit-Position: refs/heads/master@{#39052}

[adamk, 2016-08-11 00:56:42]

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

[adamk, 2016-08-11 00:56:42]

I'm not entirely sure this is worthwhile (it removes a bit of cognitive overhead
but adds another). I'd be interested what you think.

[adamk, 2016-08-11 00:57:44]

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

[commit-bot: I haz the power, 2016-08-11 00:57:52]

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

[adamk, 2016-08-11 01:02:39]

https://codereview.chromium.org/2233673003/diff/1/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2233673003/diff/1/src/contexts.cc#newcode312
src/contexts.cc:312: if (follow_context_chain && (flags &
STOP_AT_DECLARATION_SCOPE) == 0 &&
This was wrong before (the function name is conceptually in an outer scope), but
didn't cause any problems due to the way this function is called.

Without this fix, the following becomes an error when it should not be:

(function foo() { eval("var foo") })()

https://codereview.chromium.org/2233673003/diff/1/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):

https://codereview.chromium.org/2233673003/diff/1/src/crankshaft/hydrogen.cc#...
src/crankshaft/hydrogen.cc:6988: if
(var->throw_on_const_assignment(function_language_mode())) {
For Crankshaft, I suspect we could safely drop this branch and always bail out
if const assignment ever happens. But I'm not sure what caused the handling to
get added in the first place...

[commit-bot: I haz the power, 2016-08-11 01:03:22]

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

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

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...)
  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_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Michael Starzinger, 2016-08-11 14:54:08]

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

[Michael Starzinger, 2016-08-11 14:54:09]

+Benedikt, as he also reviewed earlier changes related to variable loading and
assignment in the backends.

[Benedikt Meurer, 2016-08-12 03:35:20]

bmeurer@chromium.org changed reviewers:
+ neis@chromium.org, verwaest@chromium.org

[Benedikt Meurer, 2016-08-12 03:35:21]

bmeurer@chromium.org changed required reviewers:
+ verwaest@chromium.org

[Benedikt Meurer, 2016-08-12 03:35:21]

+Toon, +Georg

[adamk, 2016-08-15 17:50:31]

The only feedback I've heard on this so far has been mildly negative, so unless
I hear some positivity I'm likely to drop this and simply make it clearer in
globals.h what exactly CONST_LEGACY is used for.

[neis, 2016-08-17 10:50:59]

lgtm

On 2016/08/15 17:50:31, adamk wrote:
> The only feedback I've heard on this so far has been mildly negative, so
unless
> I hear some positivity I'm likely to drop this and simply make it clearer in
> globals.h what exactly CONST_LEGACY is used for.

What are the concrete concerns?  I actually like the CL, it makes several things
simpler.

[Toon Verwaest, 2016-08-18 07:05:27]

I like this change as well.

https://codereview.chromium.org/2233673003/diff/1/src/compiler/ast-graph-buil...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/2233673003/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3456: } else if (IsLexicalVariableMode(mode)
&& op != Token::INIT) {
If I understood correctly, there was some unhappiness about the change in
structure here. This used to explicitly handle all 4 combinations separately:
LET && INIT
CONST && INIT
LET && !INIT
CONST && !INIT

now the !INIT cases are partially merged and it's not as clear anymore. To
enable sharing + keeping the structure clear I'd suggest a helper function for
the shareable part.

https://codereview.chromium.org/2233673003/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3467: if
(variable->throw_on_const_assignment(language_mode())) {
What about returning a ShouldThrow flag from throw_on_const_assignment, passing
it into BuildThrowConstAssignError, which should probably be renamed to
BuildAssignToConst() in that case? That introduces a guarantee that we won't
forget to check this case if we need to introduce a new
BuildThrowConstAssignError conditionally on throw_on_const_assignment. That's
the pattern we use in the runtime now (see e.g., WriteToReadOnlyProperty).

https://codereview.chromium.org/2233673003/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3488: } else if (IsLexicalVariableMode(mode)
&& op != Token::INIT) {
the same here

https://codereview.chromium.org/2233673003/diff/1/src/full-codegen/x64/full-c...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/2233673003/diff/1/src/full-codegen/x64/full-c...
src/full-codegen/x64/full-codegen-x64.cc:2079: } else if
(IsLexicalVariableMode(var->mode()) && op != Token::INIT) {
probably the same explicit control flow was used here?

[adamk, 2016-08-18 18:40:19]

I haven't made Toon's suggestions, as I'd like to get feedback from compiler/
OWNERS. The new patch keeps a separate clause for this case, but moves it down
in the if/else chain (to show that it's something of a special case).

https://codereview.chromium.org/2233673003/diff/1/src/compiler/ast-graph-buil...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/2233673003/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3456: } else if (IsLexicalVariableMode(mode)
&& op != Token::INIT) {
On 2016/08/18 07:05:26, Toon Verwaest wrote:
> If I understood correctly, there was some unhappiness about the change in
> structure here. This used to explicitly handle all 4 combinations separately:
> LET && INIT
> CONST && INIT
> LET && !INIT
> CONST && !INIT
> 
> now the !INIT cases are partially merged and it's not as clear anymore. To
> enable sharing + keeping the structure clear I'd suggest a helper function for
> the shareable part.

Yeah, I've reverted that change on master, and updated this change to be based
on current master.

https://codereview.chromium.org/2233673003/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3467: if
(variable->throw_on_const_assignment(language_mode())) {
On 2016/08/18 07:05:26, Toon Verwaest wrote:
> What about returning a ShouldThrow flag from throw_on_const_assignment,
passing
> it into BuildThrowConstAssignError, which should probably be renamed to
> BuildAssignToConst() in that case? That introduces a guarantee that we won't
> forget to check this case if we need to introduce a new
> BuildThrowConstAssignError conditionally on throw_on_const_assignment. That's
> the pattern we use in the runtime now (see e.g., WriteToReadOnlyProperty).

I'm worried that still runs afoul of making each case more explicit. I've
instead left this here, with a check for the variable being a
SLOPPY_FUNCTION_NAME. That makes it clearer that this is a special case, but one
that has its own logic separate from "normal" CONST handling.

https://codereview.chromium.org/2233673003/diff/1/src/full-codegen/x64/full-c...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/2233673003/diff/1/src/full-codegen/x64/full-c...
src/full-codegen/x64/full-codegen-x64.cc:2079: } else if
(IsLexicalVariableMode(var->mode()) && op != Token::INIT) {
On 2016/08/18 07:05:26, Toon Verwaest wrote:
> probably the same explicit control flow was used here?

Sort of. It wasn't as explicit (due to the fact that we share the code for
initializing assignments to LET/CONST and all assignments to VAR), so I didn't
feel like it was worth reverting.

[adamk, 2016-08-18 18:40:28]

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

[commit-bot: I haz the power, 2016-08-18 18:40:39]

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-08-18 18:44:29]

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

[commit-bot: I haz the power, 2016-08-18 18:44:30]

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_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)

[rmcilroy, 2016-08-19 08:56:00]

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

[rmcilroy, 2016-08-19 08:56:00]

I'm not a compiler/ owner, but I like the changes in interpreter/

[adamk, 2016-08-19 21:39:13]

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

[commit-bot: I haz the power, 2016-08-19 21:39:20]

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-08-19 21:48:30]

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

[commit-bot: I haz the power, 2016-08-19 21:48:31]

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...)

[adamk, 2016-08-19 21:49:27]

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

[commit-bot: I haz the power, 2016-08-19 21:49:30]

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

[adamk, 2016-08-19 21:58:19]

Ported to various architectures for completeness. bmeurer & mstarzinger, any
thoughts? What do you think of Toon's idea?

[commit-bot: I haz the power, 2016-08-19 22:39:29]

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

[commit-bot: I haz the power, 2016-08-19 22:39:29]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-08-26 18:27:48]

Ping bmeurer (now that you're back from vacation :)

[Benedikt Meurer, 2016-08-29 05:02:21]

LGTM.

[adamk, 2016-08-31 00:23:52]

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

[adamk, 2016-08-31 00:23:54]

+bradnelson for asmjs/ (which just picked up a dependency on CONST_LEGACY)

Toon, let me know if you'd like me to do something like ShouldThrow before I
land, otherwise I intend to land this tomorrow.

[bradnelson, 2016-08-31 00:39:45]

lgtm for asmjs

[adamk, 2016-08-31 17:38:43]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-08-31 17:38:44]

The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org,
bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2233673003/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2016-08-31 17:38:55]

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-08-31 17:38:57]

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

[commit-bot: I haz the power, 2016-08-31 17:38:58]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[verwaest1, 2016-08-31 18:20:56]

verwaest@google.com changed reviewers:
+ verwaest@google.com

[verwaest1, 2016-08-31 18:20:57]

lgtm

[adamk, 2016-08-31 18:21:52]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2016-08-31 18:22:05]

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-08-31 18:22:07]

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

[commit-bot: I haz the power, 2016-08-31 18:22:08]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[adamk, 2016-08-31 18:22:31]

Description was changed from

==========
Remove CONST_LEGACY VariableMode

The only remaining use of this VariableMode is for the names of sloppy
named function expressions. This patch instead uses CONST for such
bindings (just as we do in strict mode) and instead marks those
Variables specially. During code generation a new helper method,
Variable::throw_on_const_assignment(), is called to decide whether
to throw or silently ignore the assignment.
==========

to

==========
Remove CONST_LEGACY VariableMode

The only remaining use of this VariableMode is for the names of sloppy
named function expressions. This patch instead uses CONST for such
bindings (just as we do in strict mode) and instead marks those
Variables specially. During code generation a new helper method,
Variable::throw_on_const_assignment(), is called to decide whether
to throw or silently ignore the assignment.
==========

[adamk, 2016-08-31 18:22:31]

adamk@chromium.org changed reviewers:
- verwaest@chromium.org

[adamk, 2016-08-31 18:22:31]

adamk@chromium.org changed required reviewers:
+ verwaest@google.com
- verwaest@chromium.org

[adamk, 2016-08-31 18:22:49]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2016-08-31 18:22:53]

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

[Toon Verwaest, 2016-08-31 18:23:55]

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

[Toon Verwaest, 2016-08-31 18:23:56]

lgtm

[commit-bot: I haz the power, 2016-08-31 18:49:57]

Description was changed from

==========
Remove CONST_LEGACY VariableMode

The only remaining use of this VariableMode is for the names of sloppy
named function expressions. This patch instead uses CONST for such
bindings (just as we do in strict mode) and instead marks those
Variables specially. During code generation a new helper method,
Variable::throw_on_const_assignment(), is called to decide whether
to throw or silently ignore the assignment.
==========

to

==========
Remove CONST_LEGACY VariableMode

The only remaining use of this VariableMode is for the names of sloppy
named function expressions. This patch instead uses CONST for such
bindings (just as we do in strict mode) and instead marks those
Variables specially. During code generation a new helper method,
Variable::throw_on_const_assignment(), is called to decide whether
to throw or silently ignore the assignment.
==========

[commit-bot: I haz the power, 2016-08-31 18:49:59]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-31 18:50:36]

Description was changed from

==========
Remove CONST_LEGACY VariableMode

The only remaining use of this VariableMode is for the names of sloppy
named function expressions. This patch instead uses CONST for such
bindings (just as we do in strict mode) and instead marks those
Variables specially. During code generation a new helper method,
Variable::throw_on_const_assignment(), is called to decide whether
to throw or silently ignore the assignment.
==========

to

==========
Remove CONST_LEGACY VariableMode

The only remaining use of this VariableMode is for the names of sloppy
named function expressions. This patch instead uses CONST for such
bindings (just as we do in strict mode) and instead marks those
Variables specially. During code generation a new helper method,
Variable::throw_on_const_assignment(), is called to decide whether
to throw or silently ignore the assignment.

Committed: https://crrev.com/7516fe1eaa7910015d7732a1aa34e45e3e37586c
Cr-Commit-Position: refs/heads/master@{#39052}
==========

[commit-bot: I haz the power, 2016-08-31 18:50:37]

Patchset 6 (id:??) landed as
https://crrev.com/7516fe1eaa7910015d7732a1aa34e45e3e37586c
Cr-Commit-Position: refs/heads/master@{#39052}