Allocate block scopes in block states when possible

Allocate block scopes in block states when possible

This is another step towards lazily allocating them in the block state.
ClassLiteral should also have a lazy block-scope for the outermost scope,
but currently that doesn't work due to the parameter initializer rewriter
and minor implementation details in ignition and turbofan.

BUG=v8:5209
Committed: https://crrev.com/e9887b6aad65f76d78d55a3e6b29b37c736b4e6a
Cr-Commit-Position: refs/heads/master@{#38196}

[Toon Verwaest, 2016-07-21 14:50:36]

verwaest@chromium.org changed reviewers:
+ adamk@chromium.org, marja@chromium.org

[Toon Verwaest, 2016-07-21 14:50:36]

ptal

[adamk, 2016-07-21 17:16:32]

Description was changed from

==========
Allocate block scopes in block states when possible

This is another step towards lazily allocating them in the block state.
ClassLiteral should also have a lazy block-scope for the outermost scope, but
currently that doesn't work due to the parameter initialer rewriter and minor
implementation details in ignition and turbofan.

BUG=v8:5209
==========

to

==========
Allocate block scopes in block states when possible

This is another step towards lazily allocating them in the block state.
ClassLiteral should also have a lazy block-scope for the outermost scope,
but currently that doesn't work due to the parameter initializer rewriter
and minor implementation details in ignition and turbofan.

BUG=v8:5209
==========

[adamk, 2016-07-21 17:48:05]

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:299: class BlockState final : public ScopeState {
Can you add a comment here talking about the two constructors and the new
behavior? Maybe with a TODO to remove the original constructor?

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:307: void SetNonlinear() {
this->scope()->SetNonlinear(); }
These "this->" prefixes shouldn't needed here and in all the new methods below.

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:319: Scope* NewScope(ScopeState* state) {
outer_state or parent_state would be clearer here

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2310: body->set_scope(block_state.FinalizedBlockScope());
Calling FinalizeBlockScope while still inside the block_state is a little scary,
since scope() is now this weird detached thing. What are the chances we could
pass the Block to the BlockState constructor? I realize that wouldn't work
everywhere though.

[marja, 2016-07-22 07:17:00]

lgtm once you've sorted out adamk@'s comments.

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2310: body->set_scope(block_state.FinalizedBlockScope());
On 2016/07/21 17:48:05, adamk wrote:
> Calling FinalizeBlockScope while still inside the block_state is a little
scary,
> since scope() is now this weird detached thing. What are the chances we could
> pass the Block to the BlockState constructor? I realize that wouldn't work
> everywhere though.

(I like this version because it has the set_scope and FinalizeBlockScope calls
here, instead of hiding them inside the *State helpers. If we push too much
logic there, the AST - Scope connections will become more difficult to follow.
But that's a matter of taste, I don't have a strong opinion about this.)

[Toon Verwaest, 2016-07-22 11:02:53]

Addressed comments. I held off on changing set_block, even though I agree that
it's a little weird and possibly scary. I hope a later stage of refactoring will
make it more obvious how to better structure it :)

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:299: class BlockState final : public ScopeState {
On 2016/07/21 17:48:05, adamk wrote:
> Can you add a comment here talking about the two constructors and the new
> behavior? Maybe with a TODO to remove the original constructor?

Done.

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:307: void SetNonlinear() {
this->scope()->SetNonlinear(); }
On 2016/07/21 17:48:05, adamk wrote:
> These "this->" prefixes shouldn't needed here and in all the new methods
below.

They are because the outer class is a templatized class (otherwise it would just
work), and it also has a scope() method. Scoping in C++ is weird...

https://codereview.chromium.org/2166843003/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:319: Scope* NewScope(ScopeState* state) {
On 2016/07/21 17:48:05, adamk wrote:
> outer_state or parent_state would be clearer here

Done.

[adamk, 2016-07-22 22:44:09]

lgtm

As for the scariness, we could add a debug-only field to Scope that detects
whether it's being mutated after FinalizeBlockScope has been called...but that
may be overkill.

[marja, 2016-07-25 07:53:29]

I generally like that idea :) We should tie our hands as much as possible and
also make it clear for future coders that that invariant needs to hold. The
whole Scope / Ast area is a bit... fragile, as in, it's easy to break some
unwritten assumptions here and there.

[Toon Verwaest, 2016-08-01 08:28:07]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-08-01 08:28:08]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org,
adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2166843003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-01 08:28:18]

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-01 08:29:27]

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

[commit-bot: I haz the power, 2016-08-01 08:29:28]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10015)
  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_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10000)
  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_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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/9958)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/20612)

[Toon Verwaest, 2016-08-01 08:31:35]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-08-01 08:31:35]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2166843003/#ps120001
(title: "rebase")

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

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-01 09:04:18]

Description was changed from

==========
Allocate block scopes in block states when possible

This is another step towards lazily allocating them in the block state.
ClassLiteral should also have a lazy block-scope for the outermost scope,
but currently that doesn't work due to the parameter initializer rewriter
and minor implementation details in ignition and turbofan.

BUG=v8:5209
==========

to

==========
Allocate block scopes in block states when possible

This is another step towards lazily allocating them in the block state.
ClassLiteral should also have a lazy block-scope for the outermost scope,
but currently that doesn't work due to the parameter initializer rewriter
and minor implementation details in ignition and turbofan.

BUG=v8:5209
==========

[commit-bot: I haz the power, 2016-08-01 09:04:19]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-01 09:05:18]

Description was changed from

==========
Allocate block scopes in block states when possible

This is another step towards lazily allocating them in the block state.
ClassLiteral should also have a lazy block-scope for the outermost scope,
but currently that doesn't work due to the parameter initializer rewriter
and minor implementation details in ignition and turbofan.

BUG=v8:5209
==========

to

==========
Allocate block scopes in block states when possible

This is another step towards lazily allocating them in the block state.
ClassLiteral should also have a lazy block-scope for the outermost scope,
but currently that doesn't work due to the parameter initializer rewriter
and minor implementation details in ignition and turbofan.

BUG=v8:5209

Committed: https://crrev.com/e9887b6aad65f76d78d55a3e6b29b37c736b4e6a
Cr-Commit-Position: refs/heads/master@{#38196}
==========

[commit-bot: I haz the power, 2016-08-01 09:05:19]

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