Make non-Module generators only context allocate parameters.

Make non-Module generators only context allocate parameters.

In particular, local variables should be allocated on stack (in bytecode register), and stored/loaded to the generator object on generator suspend/resume.

The CL is based on @adamk's change to scoping/parsers (https://chromium-review.googlesource.com/c/498538/), I only made the debugger cope with this change.

I should note that the CL changes the scope type of suspended generators from ScopeType.Closure to ScopeType.Local. In the future we might want to introduce ScopeType.SuspendedGenerator to make the distinction explicit.

Some of the changes in the tests have been made because the debugger functions do not return scopes of closed generators anymore. Generators should be allowed to throw away their internal state when they finish.

BUG=v8:6368
Review-Url: https://codereview.chromium.org/2898163002
Cr-Commit-Position: refs/heads/master@{#45515}
Committed: https://chromium.googlesource.com/v8/v8/+/a957b0f42468d632b09574e3d6719c704eebbfea

[Jarin, 2017-05-23 13:46:00]

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

[commit-bot: I haz the power, 2017-05-23 13:46:06]

Dry run: CQ is trying da patch.

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

[Jarin, 2017-05-23 13:49:48]

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

[commit-bot: I haz the power, 2017-05-23 13:49:56]

Dry run: CQ is trying da patch.

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

[Jarin, 2017-05-23 14:03:32]

Description was changed from

==========
Make non-Module generators only context allocate parameters.

BUG=v8:6368
==========

to

==========
Make non-Module generators only context allocate parameters.

In particular, local variables should be allocated on stack (in bytecode
register), and stored/loaded to the generator object on generator
suspend/resume.

The CL is based on @adamk's change to scoping/parsers
(https://chromium-review.googlesource.com/c/498538/), I only made the debugger
cope with this change.

I should note that the CL changes the scope type of suspended generators from
ScopeType.Closure to ScopeType.Local. In the future we might want to introduce
ScopeType.SuspendedGenerator to make the distinction explicit.

BUG=v8:6368
==========

[Jarin, 2017-05-23 14:03:32]

jarin@chromium.org changed reviewers:
+ adamk@chromium.org, jgruber@chromium.org

[Jarin, 2017-05-23 14:04:07]

Could you take a look, please?

[commit-bot: I haz the power, 2017-05-23 14:07:11]

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

[commit-bot: I haz the power, 2017-05-23 14:07:12]

Dry run: Try jobs failed on following builders:
  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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Jarin, 2017-05-23 14:50:45]

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

[commit-bot: I haz the power, 2017-05-23 14:50:56]

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, 2017-05-23 15:10:01]

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

[commit-bot: I haz the power, 2017-05-23 15:10:02]

Dry run: Try jobs failed on following builders:
  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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Jarin, 2017-05-23 15:13:39]

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

[commit-bot: I haz the power, 2017-05-23 15:13: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, 2017-05-23 15:30:31]

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

[commit-bot: I haz the power, 2017-05-23 15:30:32]

Dry run: Try jobs failed on following builders:
  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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Jarin, 2017-05-23 17:39:19]

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

[commit-bot: I haz the power, 2017-05-23 17:39:27]

Dry run: CQ is trying da patch.

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

[Jarin, 2017-05-23 17:42:17]

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

[commit-bot: I haz the power, 2017-05-23 17:42:29]

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, 2017-05-23 18:01:37]

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

[commit-bot: I haz the power, 2017-05-23 18:01:38]

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/26429)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[Jarin, 2017-05-23 18:13:02]

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

[commit-bot: I haz the power, 2017-05-23 18:13:07]

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, 2017-05-23 18:28:01]

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

[commit-bot: I haz the power, 2017-05-23 18:28:02]

Dry run: Try jobs failed on following builders:
  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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Jarin, 2017-05-23 18:29:32]

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

[commit-bot: I haz the power, 2017-05-23 18:29:36]

Dry run: CQ is trying da patch.

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

[Jarin, 2017-05-23 18:29:51]

Patchset #9 (id:150001) has been deleted

[Jarin, 2017-05-23 18:30:15]

Patchset #8 (id:130001) has been deleted

[commit-bot: I haz the power, 2017-05-23 18:59:33]

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

[commit-bot: I haz the power, 2017-05-23 18:59:36]

Dry run: This issue passed the CQ dry run.

[adamk, 2017-05-23 20:20:37]

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

[adamk, 2017-05-23 20:20:38]

Thanks for picking this up! Since I wrote the parser/scope bits, I think Georg
should probably review this rather than me. But the code lgtm.

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

https://codereview.chromium.org/2898163002/diff/170001/src/parsing/parser.cc#...
src/parsing/parser.cc:2537: // it minimizes the work needed to suspend and
resume an activation.  The
This first sentence is now obsolete, and should be removed; I think the rest of
the comment stands on its own.

https://codereview.chromium.org/2898163002/diff/170001/test/inspector/runtime...
File test/inspector/runtime/internal-properties-expected.txt (left):

https://codereview.chromium.org/2898163002/diff/170001/test/inspector/runtime...
test/inspector/runtime/internal-properties-expected.txt:343: name : [[Scopes]]
What happened to this?

[Jarin, 2017-05-24 05:03:27]

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

https://codereview.chromium.org/2898163002/diff/170001/src/parsing/parser.cc#...
src/parsing/parser.cc:2537: // it minimizes the work needed to suspend and
resume an activation.  The
On 2017/05/23 20:20:38, adamk wrote:
> This first sentence is now obsolete, and should be removed; I think the rest
of
> the comment stands on its own.

Done.

https://codereview.chromium.org/2898163002/diff/170001/test/inspector/runtime...
File test/inspector/runtime/internal-properties-expected.txt (left):

https://codereview.chromium.org/2898163002/diff/170001/test/inspector/runtime...
test/inspector/runtime/internal-properties-expected.txt:343: name : [[Scopes]]
On 2017/05/23 20:20:38, adamk wrote:
> What happened to this?

We now only report scopes for suspended generators. This one is closed. In
general, closed generator should be allowed to throw away all its internal
state.

[Jarin, 2017-05-24 05:53:52]

Description was changed from

==========
Make non-Module generators only context allocate parameters.

In particular, local variables should be allocated on stack (in bytecode
register), and stored/loaded to the generator object on generator
suspend/resume.

The CL is based on @adamk's change to scoping/parsers
(https://chromium-review.googlesource.com/c/498538/), I only made the debugger
cope with this change.

I should note that the CL changes the scope type of suspended generators from
ScopeType.Closure to ScopeType.Local. In the future we might want to introduce
ScopeType.SuspendedGenerator to make the distinction explicit.

BUG=v8:6368
==========

to

==========
Make non-Module generators only context allocate parameters.

In particular, local variables should be allocated on stack (in bytecode
register), and stored/loaded to the generator object on generator
suspend/resume.

The CL is based on @adamk's change to scoping/parsers
(https://chromium-review.googlesource.com/c/498538/), I only made the debugger
cope with this change.

I should note that the CL changes the scope type of suspended generators from
ScopeType.Closure to ScopeType.Local. In the future we might want to introduce
ScopeType.SuspendedGenerator to make the distinction explicit.

Note that the debugger functions do not return scopes of closed generators
anymore. Generators should be allowed to throw away their internal state when
they finish.

BUG=v8:6368
==========

[Jarin, 2017-05-24 05:55:24]

Description was changed from

==========
Make non-Module generators only context allocate parameters.

In particular, local variables should be allocated on stack (in bytecode
register), and stored/loaded to the generator object on generator
suspend/resume.

The CL is based on @adamk's change to scoping/parsers
(https://chromium-review.googlesource.com/c/498538/), I only made the debugger
cope with this change.

I should note that the CL changes the scope type of suspended generators from
ScopeType.Closure to ScopeType.Local. In the future we might want to introduce
ScopeType.SuspendedGenerator to make the distinction explicit.

Note that the debugger functions do not return scopes of closed generators
anymore. Generators should be allowed to throw away their internal state when
they finish.

BUG=v8:6368
==========

to

==========
Make non-Module generators only context allocate parameters.

In particular, local variables should be allocated on stack (in bytecode
register), and stored/loaded to the generator object on generator
suspend/resume.

The CL is based on @adamk's change to scoping/parsers
(https://chromium-review.googlesource.com/c/498538/), I only made the debugger
cope with this change.

I should note that the CL changes the scope type of suspended generators from
ScopeType.Closure to ScopeType.Local. In the future we might want to introduce
ScopeType.SuspendedGenerator to make the distinction explicit.

Some of the changes in the tests have been made because the debugger functions
do not return scopes of closed generators anymore. Generators should be allowed
to throw away their internal state when they finish.

BUG=v8:6368
==========

[jgruber, 2017-05-24 08:01:49]

lgtm, thanks!

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

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:680: GetFrame()->SetParameterValue(i, *new_value);
Nit: frame->SetParameterValue

[neis, 2017-05-24 10:39:12]

lgtm with minor comments.
Haven't looked very carefully at the debug-scopes tests.

[neis, 2017-05-24 10:39:37]

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

https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:2198: MustAllocateInContext(var)) {
This should be moved into MustAllocateInContext(var). Or does that function not
know whether var is a parameter?

https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.h#newco...
src/ast/scopes.h:333: 
I don't like the fact that has_forced_context_allocation() does not imply
has_forced_context_allocation_for_parameters().

How about having an enum ForcedContextAllocation { None, ParametersOnly,
Everything }?

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

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:471: DCHECK(!generator_.is_null());
Could you move these DCHECKs into the ScopeIterator constructor that takes a
generator?

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:540: if (value->IsTheHole(isolate_) ||
value->IsOptimizedOut(isolate_)) {
Maybe add a TODO that we might need to convert StaleRegisterValue once we
optimize away the storing of unused stack locals at suspend.

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:675: DCHECK_NOT_NULL(frame_inspector_);
Please add a comment here explaining that it cannot be a generator because we
force context allocation for generator parameters.

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:690: // Setting stack locals of optimized frames is
not supported.
Maybe add that it's supported for optimized suspended generators.

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:700: } else {
...SetExpression(stack_local_index, ...)

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes.h
File src/debug/debug-scopes.h (right):

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.h:107: Handle<Context> GetFrameContext();
Why not just GetContext? (like GetFunction etc)

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

https://codereview.chromium.org/2898163002/diff/180001/src/parsing/parser.cc#...
src/parsing/parser.cc:709: scope->ForceContextAllocation();
Do we still need this? If not, do we still want it or should we remove it in a
followup CL? Offhand I don't see a reason for keeping it.

https://codereview.chromium.org/2898163002/diff/180001/test/debugger/debug/de...
File test/debugger/debug/debug-scopes-suspended-generators.js (right):

https://codereview.chromium.org/2898163002/diff/180001/test/debugger/debug/de...
test/debugger/debug/debug-scopes-suspended-generators.js:278:
CheckScopeChain([// debug.ScopeType.Block,
What's going on here?

[Jarin, 2017-05-24 12:32:21]

I fixed all the debugger-related comments, but I have no idea about the
scoping/parser part, so I will leave this to Adam to answer.

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

https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:2198: MustAllocateInContext(var)) {
On 2017/05/24 10:39:36, neis wrote:
> This should be moved into MustAllocateInContext(var). Or does that function
not
> know whether var is a parameter?

I do not know this code well enough to say whether it is good idea to move it
there. Perhaps Adam can help?

https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.h#newco...
src/ast/scopes.h:333: 
On 2017/05/24 10:39:37, neis wrote:
> I don't like the fact that has_forced_context_allocation() does not imply
> has_forced_context_allocation_for_parameters().
> 
> How about having an enum ForcedContextAllocation { None, ParametersOnly,
> Everything }?

Not sure. Adam, do you think it is a good idea?

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

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:471: DCHECK(!generator_.is_null());
On 2017/05/24 10:39:37, neis wrote:
> Could you move these DCHECKs into the ScopeIterator constructor that takes a
> generator?

I can add this to the constructor, but here I want to say that we have either
generator or frame_inspector.

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:675: DCHECK_NOT_NULL(frame_inspector_);
On 2017/05/24 10:39:37, neis wrote:
> Please add a comment here explaining that it cannot be a generator because we
> force context allocation for generator parameters.

Done.

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:680: GetFrame()->SetParameterValue(i, *new_value);
On 2017/05/24 08:01:48, jgruber wrote:
> Nit: frame->SetParameterValue

Done.

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:690: // Setting stack locals of optimized frames is
not supported.
On 2017/05/24 10:39:37, neis wrote:
> Maybe add that it's supported for optimized suspended generators.

Done.

https://codereview.chromium.org/2898163002/diff/180001/src/debug/debug-scopes...
src/debug/debug-scopes.cc:700: } else {
On 2017/05/24 10:39:37, neis wrote:
> ...SetExpression(stack_local_index, ...)

Done.

https://codereview.chromium.org/2898163002/diff/180001/test/debugger/debug/de...
File test/debugger/debug/debug-scopes-suspended-generators.js (right):

https://codereview.chromium.org/2898163002/diff/180001/test/debugger/debug/de...
test/debugger/debug/debug-scopes-suspended-generators.js:278:
CheckScopeChain([// debug.ScopeType.Block,
On 2017/05/24 10:39:37, neis wrote:
> What's going on here?

Forgotten scope. Now removed.

[Jarin, 2017-05-24 12:35:20]

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

[commit-bot: I haz the power, 2017-05-24 12:35:26]

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, 2017-05-24 12:36:56]

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

[commit-bot: I haz the power, 2017-05-24 12:36:57]

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_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_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)

[adamk, 2017-05-24 13:04:00]

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

https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:2198: MustAllocateInContext(var)) {
On 2017/05/24 10:39:36, neis wrote:
> This should be moved into MustAllocateInContext(var). Or does that function
not
> know whether var is a parameter?

Indeed the problem is that MustAllocateInContext doesn't know the variable is a
parameter. So this seemed the simpler option than giving MustAllocateInContext a
second argument.

https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2898163002/diff/180001/src/ast/scopes.h#newco...
src/ast/scopes.h:333: 
On 2017/05/24 12:32:21, Jarin wrote:
> On 2017/05/24 10:39:37, neis wrote:
> > I don't like the fact that has_forced_context_allocation() does not imply
> > has_forced_context_allocation_for_parameters().
> > 
> > How about having an enum ForcedContextAllocation { None, ParametersOnly,
> > Everything }?
> 
> Not sure. Adam, do you think it is a good idea?

I think an enum is fine if Georg finds this confusing. I agree the current
naming is ambiguous.

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

https://codereview.chromium.org/2898163002/diff/180001/src/parsing/parser.cc#...
src/parsing/parser.cc:709: scope->ForceContextAllocation();
On 2017/05/24 10:39:37, neis wrote:
> Do we still need this? If not, do we still want it or should we remove it in a
> followup CL? Offhand I don't see a reason for keeping it.

Do you mean "is this call now redundant?" or "do we want to make module scopes
not all context-allocated?" The former is not the case: this call is necessary
to get module scopes to be context-allocated (a fact which we depend on to allow
preparsing of functions in modules). For the latter, it's not clear to me if the
tradeoffs are different than the ones in generators, so I mostly wanted to avoid
making that decision for modules in this patch.

[Jarin, 2017-05-24 13:07:40]

The CQ bit was checked by jarin@chromium.org

[Jarin, 2017-05-24 13:07:41]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
neis@chromium.org, jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2898163002/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-24 13:07:48]

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, 2017-05-24 13:54:58]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1495631260795600,
"parent_rev": "9d8bd0551657d60c66b2f96544eecedfba1cba8a", "commit_rev":
"a957b0f42468d632b09574e3d6719c704eebbfea"}

[commit-bot: I haz the power, 2017-05-24 13:55:04]

Description was changed from

==========
Make non-Module generators only context allocate parameters.

In particular, local variables should be allocated on stack (in bytecode
register), and stored/loaded to the generator object on generator
suspend/resume.

The CL is based on @adamk's change to scoping/parsers
(https://chromium-review.googlesource.com/c/498538/), I only made the debugger
cope with this change.

I should note that the CL changes the scope type of suspended generators from
ScopeType.Closure to ScopeType.Local. In the future we might want to introduce
ScopeType.SuspendedGenerator to make the distinction explicit.

Some of the changes in the tests have been made because the debugger functions
do not return scopes of closed generators anymore. Generators should be allowed
to throw away their internal state when they finish.

BUG=v8:6368
==========

to

==========
Make non-Module generators only context allocate parameters.

In particular, local variables should be allocated on stack (in bytecode
register), and stored/loaded to the generator object on generator
suspend/resume.

The CL is based on @adamk's change to scoping/parsers
(https://chromium-review.googlesource.com/c/498538/), I only made the debugger
cope with this change.

I should note that the CL changes the scope type of suspended generators from
ScopeType.Closure to ScopeType.Local. In the future we might want to introduce
ScopeType.SuspendedGenerator to make the distinction explicit.

Some of the changes in the tests have been made because the debugger functions
do not return scopes of closed generators anymore. Generators should be allowed
to throw away their internal state when they finish.

BUG=v8:6368

Review-Url: https://codereview.chromium.org/2898163002
Cr-Commit-Position: refs/heads/master@{#45515}
Committed:
https://chromium.googlesource.com/v8/v8/+/a957b0f42468d632b09574e3d6719c704ee...
==========

[commit-bot: I haz the power, 2017-05-24 13:55:06]

Committed patchset #11 (id:220001) as
https://chromium.googlesource.com/v8/v8/+/a957b0f42468d632b09574e3d6719c704ee...

[neis, 2017-05-24 13:59:46]

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

https://codereview.chromium.org/2898163002/diff/180001/src/parsing/parser.cc#...
src/parsing/parser.cc:709: scope->ForceContextAllocation();
On 2017/05/24 13:04:00, adamk wrote:
> On 2017/05/24 10:39:37, neis wrote:
> > Do we still need this? If not, do we still want it or should we remove it in
a
> > followup CL? Offhand I don't see a reason for keeping it.
> 
> Do you mean "is this call now redundant?" or "do we want to make module scopes
> not all context-allocated?" The former is not the case: this call is necessary
> to get module scopes to be context-allocated (a fact which we depend on to
allow
> preparsing of functions in modules). For the latter, it's not clear to me if
the
> tradeoffs are different than the ones in generators, so I mostly wanted to
avoid
> making that decision for modules in this patch.

I meant do we still need/want to force context allocation for modules. I'll
check if there's still a technical reason to do so.

[adamk, 2017-05-24 14:31:23]

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

https://codereview.chromium.org/2898163002/diff/180001/src/parsing/parser.cc#...
src/parsing/parser.cc:709: scope->ForceContextAllocation();
On 2017/05/24 13:59:46, neis wrote:
> On 2017/05/24 13:04:00, adamk wrote:
> > On 2017/05/24 10:39:37, neis wrote:
> > > Do we still need this? If not, do we still want it or should we remove it
in
> a
> > > followup CL? Offhand I don't see a reason for keeping it.
> > 
> > Do you mean "is this call now redundant?" or "do we want to make module
scopes
> > not all context-allocated?" The former is not the case: this call is
necessary
> > to get module scopes to be context-allocated (a fact which we depend on to
> allow
> > preparsing of functions in modules). For the latter, it's not clear to me if
> the
> > tradeoffs are different than the ones in generators, so I mostly wanted to
> avoid
> > making that decision for modules in this patch.
> 
> I meant do we still need/want to force context allocation for modules. I'll
> check if there's still a technical reason to do so.

This would depend on how advanced the preparsing-of-inner-functions work is, so
checking with Marja seems like the right next step.

[Michael Achenbach, 2017-05-25 17:38:40]

A revert of this CL (patchset #11 id:220001) has been created in
https://codereview.chromium.org/2902423002/ by machenbach@chromium.org.

The reason for reverting is: Speculative revert for layout test changes:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui...

Looks like this might have caused the change in:
inspector/sources/debugger-ui/function-generator-details.html

See also:
https://github.com/v8/v8/wiki/Blink-layout-tests.