[parsing] Fix context allocation for async functions.

[parsing] Fix context allocation for async functions.

For generator-based functions (e.g. async functions) we force variables to be
context-allocated.  Due to a bug in the parser, this didn't always work
correctly.  For instance, in "async function foo([a]) { ... }" the variable "a"
could become stack-allocated due to context allocation being forced on the wrong
scope.

Besides fixing this, I'm also cleaning up some related code in the async parsing
setup and adding some guards.

R=adamk@chromium.org, littledan@chromium.org
BUG=

Committed: https://crrev.com/80567914c70ab6538bdfa452913a84e929f3cc25
Cr-Commit-Position: refs/heads/master@{#41635}

[neis, 2016-12-08 12:47:34]

BTW, I can't add any mjsunit tests because nothing goes wrong (at least with my
examples). I believe this is because for Ignition generators the
context-allocation is only needed for actual parameters, not locals.

[adamk, 2016-12-09 03:12:32]

I suspect my questions suggest that Dan is a better reviewer here, but figured
I'd ask anyway.

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

https://codereview.chromium.org/2561093002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3155: if (IsResumableFunction(kind))
PrepareGeneratorVariables();
Why wasn't this needed before?

https://codereview.chromium.org/2561093002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4170: if (function_state_->generator_object_variable() ==
nullptr) {
How would this be already-initialized at this point?

[Dan Ehrenberg, 2016-12-09 06:02:21]

Really nice cleanup and bug fix. I remember trying to merge the two
ForceContextAllocation calls earlier but ran into some issues; great to see you
following up with this.

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

https://codereview.chromium.org/2561093002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:2511: function_state_->scope()->ForceContextAllocation();
I take it this is the key fix and the rest is cleanup, right?

https://codereview.chromium.org/2561093002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3155: if (IsResumableFunction(kind))
PrepareGeneratorVariables();
On 2016/12/09 at 03:12:31, adamk wrote:
> Why wasn't this needed before?

I believe because the logic was (duplicated) in PrepareAsyncFunctionBody.

https://codereview.chromium.org/2561093002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4170: if (function_state_->generator_object_variable() ==
nullptr) {
On 2016/12/09 at 03:12:31, adamk wrote:
> How would this be already-initialized at this point?

Wondering the same

[neis, 2016-12-09 10:54:53]

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

https://codereview.chromium.org/2561093002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:2511: function_state_->scope()->ForceContextAllocation();
On 2016/12/09 06:02:21, Dan Ehrenberg wrote:
> I take it this is the key fix and the rest is cleanup, right?

Yes this is the main fix.  But I think it was also fishy that we would force the
context allocation quite late, after potentially other stuff had been created.

https://codereview.chromium.org/2561093002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3155: if (IsResumableFunction(kind))
PrepareGeneratorVariables();
On 2016/12/09 06:02:21, Dan Ehrenberg wrote:
> On 2016/12/09 at 03:12:31, adamk wrote:
> > Why wasn't this needed before?
> 
> I believe because the logic was (duplicated) in PrepareAsyncFunctionBody.

Yes.

https://codereview.chromium.org/2561093002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4170: if (function_state_->generator_object_variable() ==
nullptr) {
On 2016/12/09 06:02:21, Dan Ehrenberg wrote:
> On 2016/12/09 at 03:12:31, adamk wrote:
> > How would this be already-initialized at this point?
> 
> Wondering the same

It is initialized by the PrepareGeneratorVariables above when we enter via
ParseFunction -> ParseEagerFunctionBody -> ParseAsyncFunctionBody.

[neis, 2016-12-09 11:38:20]

I made one other change, ptal. That conditional didn't make any sense to me.

[adamk, 2016-12-09 17:44:34]

lgtm, thanks for the explanation(s)

[Dan Ehrenberg, 2016-12-09 21:51:46]

lgtm

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

https://codereview.chromium.org/2561093002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:4170: if (function_state_->generator_object_variable() ==
nullptr) {
Could you put a comment here explaining why this is necessary?

[neis, 2016-12-12 09:32:20]

The CQ bit was checked by neis@chromium.org

[neis, 2016-12-12 09:32:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org, adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2561093002/#ps40001
(title: "Add comment and rebase.")

[commit-bot: I haz the power, 2016-12-12 09:32:22]

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-12-12 09:57:18]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481535140398550,
"parent_rev": "d6abc448fdfe7de923bfc95d047ca7fdfb587790", "commit_rev":
"70440f81ca557fa031315b99105186bbb3339107"}

[commit-bot: I haz the power, 2016-12-12 09:57:21]

Description was changed from

==========
[parsing] Fix context allocation for async functions.

For generator-based functions (e.g. async functions) we force variables to be
context-allocated.  Due to a bug in the parser, this didn't always work
correctly.  For instance, in "async function foo([a]) { ... }" the variable "a"
could become stack-allocated due to context allocation being forced on the wrong
scope.

Besides fixing this, I'm also cleaning up some related code in the async parsing
setup and adding some guards.

R=adamk@chromium.org, littledan@chromium.org
BUG=
==========

to

==========
[parsing] Fix context allocation for async functions.

For generator-based functions (e.g. async functions) we force variables to be
context-allocated.  Due to a bug in the parser, this didn't always work
correctly.  For instance, in "async function foo([a]) { ... }" the variable "a"
could become stack-allocated due to context allocation being forced on the wrong
scope.

Besides fixing this, I'm also cleaning up some related code in the async parsing
setup and adding some guards.

R=adamk@chromium.org, littledan@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2561093002
==========

[commit-bot: I haz the power, 2016-12-12 09:57:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-12 09:57:37]

Description was changed from

==========
[parsing] Fix context allocation for async functions.

For generator-based functions (e.g. async functions) we force variables to be
context-allocated.  Due to a bug in the parser, this didn't always work
correctly.  For instance, in "async function foo([a]) { ... }" the variable "a"
could become stack-allocated due to context allocation being forced on the wrong
scope.

Besides fixing this, I'm also cleaning up some related code in the async parsing
setup and adding some guards.

R=adamk@chromium.org, littledan@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2561093002
==========

to

==========
[parsing] Fix context allocation for async functions.

For generator-based functions (e.g. async functions) we force variables to be
context-allocated.  Due to a bug in the parser, this didn't always work
correctly.  For instance, in "async function foo([a]) { ... }" the variable "a"
could become stack-allocated due to context allocation being forced on the wrong
scope.

Besides fixing this, I'm also cleaning up some related code in the async parsing
setup and adding some guards.

R=adamk@chromium.org, littledan@chromium.org
BUG=

Committed: https://crrev.com/80567914c70ab6538bdfa452913a84e929f3cc25
Cr-Commit-Position: refs/heads/master@{#41635}
==========

[commit-bot: I haz the power, 2016-12-12 09:57:38]

Patchset 3 (id:??) landed as
https://crrev.com/80567914c70ab6538bdfa452913a84e929f3cc25
Cr-Commit-Position: refs/heads/master@{#41635}