Stack allocate lexical locals + hoist stack slots

Stack allocate lexical locals + hoist stack slots

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

[Dmitry Lomov (no reviews), 2015-04-16 12:25:43]

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

[commit-bot: I haz the power, 2015-04-16 12:25:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/981203003/140001

[commit-bot: I haz the power, 2015-04-16 12:46:50]

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

[commit-bot: I haz the power, 2015-04-16 12:46:51]

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

[Dmitry Lomov (no reviews), 2015-04-16 16:44:44]

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

[commit-bot: I haz the power, 2015-04-16 16:44:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/981203003/160001

[Dmitry Lomov (no reviews), 2015-04-17 10:41:26]

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

[commit-bot: I haz the power, 2015-04-17 10:41:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/981203003/200001

[commit-bot: I haz the power, 2015-04-17 11:02:25]

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

[commit-bot: I haz the power, 2015-04-17 11:02:26]

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

[Dmitry Lomov (no reviews), 2015-04-17 14:14:35]

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

[commit-bot: I haz the power, 2015-04-17 14:14:43]

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

[commit-bot: I haz the power, 2015-04-17 14:14:44]

Dry run: COMMIT=false is deprecated please use the dry run button instead.

[Dmitry Lomov (no reviews), 2015-04-17 14:21:44]

dslomov@chromium.org changed reviewers:
+ adamk@chromium.org, mstarzinger@chromium.org, rossberg@chromium.org

[Dmitry Lomov (no reviews), 2015-04-17 14:22:17]

This is ready for a first round of comments - some cleaunup still needs to be
done.

https://codereview.chromium.org/981203003/diff/240001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/981203003/diff/240001/src/parser.cc#newcode4061
src/parser.cc:4061: s->ForceContextAllocation();
This does not seem correct; will fix (catch/with scopes etc.)

[rossberg, 2015-04-17 15:16:27]

Dude, heroic debugger hacking, thanks

https://codereview.chromium.org/981203003/diff/260001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/parser.cc#newcode522
src/parser.cc:522: scope->RecordEvalCall();
Hm, why is this needed?

https://codereview.chromium.org/981203003/diff/260001/src/parser.cc#newcode4060
src/parser.cc:4060: s != nullptr && s->is_block_scope(); s = s->outer_scope()) {
I think you have to walk up all the way to the next surrounding
function/global/script/eval context. Otherwise I assume you'll crash on cases
like

  function() {
    for (;;) {
      let x = 9
      try {} catch (y) {
        return function() { return x + y }
      }
    }
  }

https://codereview.chromium.org/981203003/diff/260001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:720: value = isolate->factory()->undefined_value();
Is this safe? Isn't the implication that you can now change an uninitialised
binding to initialised by assigning to it in the debugger? If so, I'm not sure
it's good to allow that, and it may potentially break some assumptions
elsewhere.

I'm not sure, I'd be careful with this change. If at all, can we do it in a
separate CL?

https://codereview.chromium.org/981203003/diff/260001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:1398: // this is a block scope on the stack.
Nit: upper case

https://codereview.chromium.org/981203003/diff/260001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:2261: //       - if a block has context-allocated
varaibles, its context is cloned
typo: variables

https://codereview.chromium.org/981203003/diff/260001/src/scopeinfo.cc
File src/scopeinfo.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/scopeinfo.cc#newcode30
src/scopeinfo.cc:30: // DCHECK(scope->StackLocalCount() >= stack_local_count);
What about this?

https://codereview.chromium.org/981203003/diff/260001/src/scopeinfo.cc#newcod...
src/scopeinfo.cc:156: /*
TODO?

https://codereview.chromium.org/981203003/diff/260001/src/scopeinfo.cc#newcod...
src/scopeinfo.cc:167: // DCHECK(scope->is_block_scope() ||
scope->num_stack_slots() ==
TODO?

https://codereview.chromium.org/981203003/diff/260001/src/scopeinfo.cc#newcod...
src/scopeinfo.cc:477: value = isolate->factory()->undefined_value();
Same as earlier (also, the comment now is out of sync).

https://codereview.chromium.org/981203003/diff/260001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/scopes.cc#newcode1394
src/scopes.cc:1394: // printf("Allocating:"); var->name()->Print();
Clean up

https://codereview.chromium.org/981203003/diff/260001/test/mjsunit/debug-scop...
File test/mjsunit/debug-scopes.js (left):

https://codereview.chromium.org/981203003/diff/260001/test/mjsunit/debug-scop...
test/mjsunit/debug-scopes.js:1060: CheckScopeContent({C1: class { m() {
debugger; }} }, 1, exec_state);
Why eliminate this test?

[Dmitry Lomov (no reviews), 2015-04-17 15:21:55]

Some replies

https://codereview.chromium.org/981203003/diff/260001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/parser.cc#newcode522
src/parser.cc:522: scope->RecordEvalCall();
On 2015/04/17 15:16:27, rossberg wrote:
> Hm, why is this needed?

I need to record eval inside block scopes.

https://codereview.chromium.org/981203003/diff/260001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:720: value = isolate->factory()->undefined_value();
On 2015/04/17 15:16:27, rossberg wrote:
> Is this safe? Isn't the implication that you can now change an uninitialised
> binding to initialised by assigning to it in the debugger? If so, I'm not sure
> it's good to allow that, and it may potentially break some assumptions
> elsewhere.

If we do not do that, shadowing will not work correctly. 
One way to prevent 'uninitialized->initialized' is to disallow assignments to
holes later when I copy the the values back.

> 
> I'm not sure, I'd be careful with this change. If at all, can we do it in a
> separate CL?

https://codereview.chromium.org/981203003/diff/260001/test/mjsunit/debug-scop...
File test/mjsunit/debug-scopes.js (left):

https://codereview.chromium.org/981203003/diff/260001/test/mjsunit/debug-scop...
test/mjsunit/debug-scopes.js:1060: CheckScopeContent({C1: class { m() {
debugger; }} }, 1, exec_state);
On 2015/04/17 15:16:27, rossberg wrote:
> Why eliminate this test?

The test is not eliminated, C1 is just not available here.

[rossberg, 2015-04-17 15:26:07]

https://codereview.chromium.org/981203003/diff/260001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/parser.cc#newcode522
src/parser.cc:522: scope->RecordEvalCall();
On 2015/04/17 15:21:54, Dmitry Lomov (chromium) wrote:
> On 2015/04/17 15:16:27, rossberg wrote:
> > Hm, why is this needed?
> 
> I need to record eval inside block scopes.

Right, I keep forgetting that "declaration scope" = "_var_ declaration scope" :)

https://codereview.chromium.org/981203003/diff/260001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:720: value = isolate->factory()->undefined_value();
On 2015/04/17 15:21:54, Dmitry Lomov (chromium) wrote:
> On 2015/04/17 15:16:27, rossberg wrote:
> > Is this safe? Isn't the implication that you can now change an uninitialised
> > binding to initialised by assigning to it in the debugger? If so, I'm not
sure
> > it's good to allow that, and it may potentially break some assumptions
> > elsewhere.
> 
> If we do not do that, shadowing will not work correctly. 
> One way to prevent 'uninitialized->initialized' is to disallow assignments to
> holes later when I copy the the values back.

I see. Yes, preventing the copying back sounds good to me.

[Dmitry Lomov (no reviews), 2015-04-20 11:53:21]

Ready for next round.

https://codereview.chromium.org/981203003/diff/260001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/parser.cc#newcode4060
src/parser.cc:4060: s != nullptr && s->is_block_scope(); s = s->outer_scope()) {
On 2015/04/17 15:16:27, rossberg wrote:
> I think you have to walk up all the way to the next surrounding
> function/global/script/eval context. Otherwise I assume you'll crash on cases
> like
> 
>   function() {
>     for (;;) {
>       let x = 9
>       try {} catch (y) {
>         return function() { return x + y }
>       }
>     }
>   }

Done.

https://codereview.chromium.org/981203003/diff/260001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:720: value = isolate->factory()->undefined_value();
On 2015/04/17 15:26:07, rossberg wrote:
> On 2015/04/17 15:21:54, Dmitry Lomov (chromium) wrote:
> > On 2015/04/17 15:16:27, rossberg wrote:
> > > Is this safe? Isn't the implication that you can now change an
uninitialised
> > > binding to initialised by assigning to it in the debugger? If so, I'm not
> sure
> > > it's good to allow that, and it may potentially break some assumptions
> > > elsewhere.
> > 
> > If we do not do that, shadowing will not work correctly. 
> > One way to prevent 'uninitialized->initialized' is to disallow assignments
to
> > holes later when I copy the the values back.
> 
> I see. Yes, preventing the copying back sounds good to me.

We actually already have the check (line 770)

https://codereview.chromium.org/981203003/diff/260001/src/scopeinfo.cc
File src/scopeinfo.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/scopeinfo.cc#newcode30
src/scopeinfo.cc:30: // DCHECK(scope->StackLocalCount() >= stack_local_count);
On 2015/04/17 15:16:27, rossberg wrote:
> What about this?

These are incorrect

https://codereview.chromium.org/981203003/diff/260001/src/scopeinfo.cc#newcod...
src/scopeinfo.cc:156: /*
On 2015/04/17 15:16:27, rossberg wrote:
> TODO?

Done.

https://codereview.chromium.org/981203003/diff/260001/src/scopeinfo.cc#newcod...
src/scopeinfo.cc:167: // DCHECK(scope->is_block_scope() ||
scope->num_stack_slots() ==
On 2015/04/17 15:16:27, rossberg wrote:
> TODO?

Done.

https://codereview.chromium.org/981203003/diff/260001/src/scopeinfo.cc#newcod...
src/scopeinfo.cc:477: value = isolate->factory()->undefined_value();
On 2015/04/17 15:16:27, rossberg wrote:
> Same as earlier (also, the comment now is out of sync).

Removed. This is not needed.

https://codereview.chromium.org/981203003/diff/260001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/981203003/diff/260001/src/scopes.cc#newcode1394
src/scopes.cc:1394: // printf("Allocating:"); var->name()->Print();
On 2015/04/17 15:16:27, rossberg wrote:
> Clean up

Done.

[Dmitry Lomov (no reviews), 2015-04-20 11:53:29]

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

[commit-bot: I haz the power, 2015-04-20 11:53:46]

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

[commit-bot: I haz the power, 2015-04-20 11:53:47]

Dry run: COMMIT=false is deprecated please use the dry run button instead.

[Dmitry Lomov (no reviews), 2015-04-20 12:06:59]

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

[commit-bot: I haz the power, 2015-04-20 12:07:09]

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

[commit-bot: I haz the power, 2015-04-20 12:07:11]

Dry run: COMMIT=false is deprecated please use the dry run button instead.

[Dmitry Lomov (no reviews), 2015-04-20 12:08:10]

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

[commit-bot: I haz the power, 2015-04-20 12:08:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/981203003/290001

[commit-bot: I haz the power, 2015-04-20 13:32:54]

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

[commit-bot: I haz the power, 2015-04-20 13:32:55]

Dry run: This issue passed the CQ dry run.

[rossberg, 2015-04-21 14:37:59]

rossberg@chromium.org changed reviewers:
+ yangguo@chromium.org

[rossberg, 2015-04-21 14:41:02]

LGTM module comments, but I can't claim that I fully understand the debugger
changes. Adding Yang as a second reviewer for that.

https://codereview.chromium.org/981203003/diff/290001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/981203003/diff/290001/src/objects.h#newcode4380
src/objects.h:4380: // 2a.StackLocalFirstSlot:
Nit: care to renumber?

https://codereview.chromium.org/981203003/diff/290001/src/objects.h#newcode4381
src/objects.h:4381: //    Index of a first stack slot for stack local.
Can you please expand on this comment? It is rather central for understanding
the stack/scope layout, and this doesn't do it justice.

https://codereview.chromium.org/981203003/diff/290001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/981203003/diff/290001/src/parser.cc#newcode4043
src/parser.cc:4043: s != nullptr && (s->is_block_scope() || s->is_catch_scope()
||
Would it work to make the condition

  s && s != s->DeclarationScope()

to be a bit more generic?

https://codereview.chromium.org/981203003/diff/290001/src/scopeinfo.cc
File src/scopeinfo.cc (right):

https://codereview.chromium.org/981203003/diff/290001/src/scopeinfo.cc#newcod...
src/scopeinfo.cc:492: return StackLocalFirstSlotIndex() + 1;
Why +1?

https://codereview.chromium.org/981203003/diff/290001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/981203003/diff/290001/src/scopes.cc#newcode1313
src/scopes.cc:1313: auto decl_scope = DeclarationScope();
Nit: inline into next line

[Dmitry Lomov (no reviews), 2015-04-21 16:59:24]

Comments addressed.
Yang, could you take a look at changes in runtime-debug.cc?

https://codereview.chromium.org/981203003/diff/290001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/981203003/diff/290001/src/objects.h#newcode4380
src/objects.h:4380: // 2a.StackLocalFirstSlot:
On 2015/04/21 14:41:02, rossberg wrote:
> Nit: care to renumber?

Done.

https://codereview.chromium.org/981203003/diff/290001/src/objects.h#newcode4381
src/objects.h:4381: //    Index of a first stack slot for stack local.
On 2015/04/21 14:41:01, rossberg wrote:
> Can you please expand on this comment? It is rather central for understanding
> the stack/scope layout, and this doesn't do it justice.

Done.

https://codereview.chromium.org/981203003/diff/290001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/981203003/diff/290001/src/parser.cc#newcode4043
src/parser.cc:4043: s != nullptr && (s->is_block_scope() || s->is_catch_scope()
||
On 2015/04/21 14:41:02, rossberg wrote:
> Would it work to make the condition
> 
>   s && s != s->DeclarationScope()
> 
> to be a bit more generic?

Great suggestion, done.

https://codereview.chromium.org/981203003/diff/290001/src/scopeinfo.cc
File src/scopeinfo.cc (right):

https://codereview.chromium.org/981203003/diff/290001/src/scopeinfo.cc#newcod...
src/scopeinfo.cc:492: return StackLocalFirstSlotIndex() + 1;
On 2015/04/21 14:41:02, rossberg wrote:
> Why +1?

The index of the first slot stack index is StackLocalFirstSlotIndex.
The first stack local is directly after that.

[Yang, 2015-04-22 11:32:23]

The debugger part LGTM. Nice clean up!

https://codereview.chromium.org/981203003/diff/310001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/981203003/diff/310001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:698: DCHECK(frame_inspector != nullptr);
DCHECK_NOT_NULL

[Dmitry Lomov (no reviews), 2015-04-22 12:24:31]

Comments addressed, landing

https://codereview.chromium.org/981203003/diff/290001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/981203003/diff/290001/src/scopes.cc#newcode1313
src/scopes.cc:1313: auto decl_scope = DeclarationScope();
On 2015/04/21 14:41:02, rossberg wrote:
> Nit: inline into next line

Done.

https://codereview.chromium.org/981203003/diff/310001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/981203003/diff/310001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:698: DCHECK(frame_inspector != nullptr);
On 2015/04/22 11:32:23, Yang wrote:
> DCHECK_NOT_NULL

Done.

[Dmitry Lomov (no reviews), 2015-04-22 12:26:16]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-04-22 12:26:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/981203003/#ps330001
(title: "Feedback + rebased")

[commit-bot: I haz the power, 2015-04-22 12:26:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/981203003/330001

[commit-bot: I haz the power, 2015-04-22 13:22:42]

Committed patchset #18 (id:330001)

[commit-bot: I haz the power, 2015-04-22 13:22:54]

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