[async-functions] support await expressions in finally statements

[async-functions] support await expressions in finally statements

Changes:

- Async functions no longer desugar return statements in Parser::RewriteReturn. Instead, return desugaring happens in BytecodeGenerator::BuildReturn(). This enables evaluation of ResolvePromise() to wait until all finally blocks are evaluated

- Parameter initialization block (created in Parser::BuildParameterInitializationBlock()) is now stored as a separate member of FunctionLiteral, rather than part of the function body.

- The `Scope::function_var_` assignment (in named function expressions) happens in BytecodeGenerator rather than as part of the function body. This variable is created at the end of ParseFunctionBody in parser-base.h, and is guaranteed to be allocated if there are any inner lazily parsed function scopes which may capture the variable, or any eval calls which may capture the variable.

- The above 2 changes allow for generators and async functions to be desugared in BytecodeGenerator, rather than in the Parser, allowing them to evaluate parameter initialization as part of a try-block, with a catch handler (in the case of async functions) and a finally block which closes the generator.

- BytecodeGenerator::VisitTryCatchStatement and VisitTryFinallyStatement defer to BuildTryCatch and BuildTryFinally, to allow this code to be reused by the async function/generator desugarings in BytecodeGenerator::GenerateBytecodeBody()

- BytecodeGenerator::VisitYield() now defers to BytecodeGenerator::BuildYield() and BytecodeGenerator::BuildAwait(), depending on what type of yield it is, which in turn perform different operations to suspend the generator. BuildYield() is also used to desugar the initial yield in generators and modules. Because of this, ast-numbering.cc ensures that modules and generator have at least one yield point, and syntactic yields begin at yield_id=1 rather than 0.

Because await desugaring happens in BytecodeGenerator rather than in Parser::RewriteAwaitExpression(), BytecodeGenerator tracks the current catch_prediction via member catch_prediction_, in the same way that ast-numbering.cc does this. For each catch block generated (via BuildTryCatch), if the catch prediction bit is not HandlerTable::UNCAUGHT, BytecodeGenerator::catch_prediction_ is updated for the course of the catch block, and restored to its previous value afterwards. This results in BytecodeGenerator selecting the correct Await builtin, depending on whether it occurs at the top-level of an async function, or within a syntactic catch try scope.

BUG=v8:5896
R=gsathya@chromium.org, rmcilroy@chromium.org, neis@chromium.org

[caitp, 2017-01-27 19:30:08]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 19:30:12]

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

[caitp, 2017-01-27 19:32:25]

hi,

I've hacked together a fix for this which seems to work locally.

It's a bit broken in some ways, since I decided to move the await rewriting to
bytecode-generator and only got half way with it (so the CAUGHT vs UNCAUGHT
catch prediction stuff is no longer present, which will probably fail some
tests).

The InternalVariable thing is kind of a hack to allow some of the parser
desugaring to keep working in the short term.

If the approach is reasonable, I'll turn it into a more fully formed solution
next week

[commit-bot: I haz the power, 2017-01-27 19:45:51]

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

[commit-bot: I haz the power, 2017-01-27 19:45:52]

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

[caitp, 2017-01-29 22:42:08]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-29 22:42:15]

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-01-29 22:46:25]

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

[commit-bot: I haz the power, 2017-01-29 22:46:26]

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

[caitp, 2017-01-29 22:48:59]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-29 22:49:05]

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-01-29 22:54:35]

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

[commit-bot: I haz the power, 2017-01-29 22:54:35]

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

[caitp, 2017-01-29 23:07:12]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-29 23:07:24]

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-01-29 23:25:31]

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

[commit-bot: I haz the power, 2017-01-29 23:25:32]

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

[caitp, 2017-01-30 04:17:36]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 04:17:46]

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

[caitp, 2017-01-30 04:20:29]

I think the last test failures are related to Promise debug events, but will
definitely want to get some feedback on the fix before spending any more time
trying to solve that problem.

https://codereview.chromium.org/2654423004/diff/20001/src/ast/ast-numbering.cc
File src/ast/ast-numbering.cc (right):

https://codereview.chromium.org/2654423004/diff/20001/src/ast/ast-numbering.c...
src/ast/ast-numbering.cc:652: // crankshaft.
I'm guessing there isn't a whole lot of cases where the function-named variable
is actually used in benchmarks. If I'm wrong, it wouldn't be too hard to
implement the function-name assignment to the legacy pipeline

https://codereview.chromium.org/2654423004/diff/20001/src/runtime/runtime-gen...
File src/runtime/runtime-generator.cc (right):

https://codereview.chromium.org/2654423004/diff/20001/src/runtime/runtime-gen...
src/runtime/runtime-generator.cc:35:
generator->set_resume_mode(JSGeneratorObject::kNext);
afaik this isn't needed now, removing..

https://codereview.chromium.org/2654423004/diff/40001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2654423004/diff/40001/src/ast/ast.h#newcode2530
src/ast/ast.h:2530: YieldType yield_type() const { return
YieldTypeField::decode(bit_field_); }
Await gets desugared in the bytecode generator now, rather than in the parser.
So this information helps generate the correct bytecode

https://codereview.chromium.org/2654423004/diff/40001/src/ast/ast.h#newcode2593
src/ast/ast.h:2593: void set_parameter_init_block(Block* block) {
parameter_init_block_ = block; }
Separating this from the function's "body" allows BytecodeGenerator to have more
control over how the code is generated, including wrapping parameter
initialization in a try/catch/finally block. Doing all of this in bytecode fixes
a TODO of Dan's

https://codereview.chromium.org/2654423004/diff/40001/src/ast/ast.h#newcode2970
src/ast/ast.h:2970: class InternalVariable final : public Expression {
Originally this was used for a lot of different things, but since moving much of
the parser desugaring into BytecodeGenerator, it's now only used for
function.sent.

This class can be eliminated if function.sent just gets its own AST node
instead.

https://codereview.chromium.org/2654423004/diff/40001/src/ast/ast.h#newcode3510
src/ast/ast.h:3510: Yield* NewYield(Expression* expression, int pos,
Yield no longer takes a generator object as a parameter, since it's always the
"own" generator object that yields, and that can be accessed via the
BytecodeGenerator.

[commit-bot: I haz the power, 2017-01-30 04:31:37]

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

[commit-bot: I haz the power, 2017-01-30 04:31:37]

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

[caitp, 2017-01-30 13:31:37]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 13:31:49]

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-01-30 13:33:11]

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

[commit-bot: I haz the power, 2017-01-30 13:33:12]

Dry run: Try jobs failed on following builders:
  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_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_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/16572)

[caitp, 2017-01-30 13:43:55]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 13:43:58]

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-01-30 14:04:14]

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

[commit-bot: I haz the power, 2017-01-30 14:04:15]

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

[caitp, 2017-01-30 15:48:31]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 15:48:44]

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-01-30 15:53:52]

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

[commit-bot: I haz the power, 2017-01-30 15:53:52]

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

[caitp, 2017-01-30 15:55:53]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 15:56:03]

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-01-30 16:12:30]

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

[commit-bot: I haz the power, 2017-01-30 16:12:31]

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

[Dan Ehrenberg, 2017-01-30 16:18:07]

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

[Dan Ehrenberg, 2017-01-30 16:18:08]

Could you separate out desugaring in the interpreter from addressing this bug in
separate patches? It would be nice to have one pure-refactoring patch that
doesn't change any behavior and one (much smaller) which fixes the bug. Could be
ordered either way, as you prefer. It would also help review if both of these
patches included some more details in the commit message describing how they
work (since this stuff is complicated).

[caitp, 2017-01-30 16:45:32]

Description was changed from

==========
[async-functions] support await expressions in finally statements

BUG=v8:5896
R=gsathya@chromium.org, rmcilroy@chromium.org, neis@chromium.org
==========

to

==========
[async-functions] support await expressions in finally statements

Changes:

- Async functions no longer desugar return statements in Parser::RewriteReturn.
Instead, return desugaring happens in BytecodeGenerator::BuildReturn(). This
enables evaluation of ResolvePromise() to wait until all finally blocks are
evaluated

- Parameter initialization block (created in
Parser::BuildParameterInitializationBlock()) is now stored as a separate member
of FunctionLiteral, rather than part of the function body.

- The `Scope::function_var_` assignment (in named function expressions) happens
in BytecodeGenerator rather than as part of the function body. This variable is
created at the end of ParseFunctionBody in parser-base.h, and is guaranteed to
be allocated if there are any inner lazily parsed function scopes which may
capture the variable, or any eval calls which may capture the variable.

- The above 2 changes allow for generators and async functions to be desugared
in BytecodeGenerator, rather than in the Parser, allowing them to evaluate
parameter initialization as part of a try-block, with a catch handler (in the
case of async functions) and a finally block which closes the generator.

- BytecodeGenerator::VisitTryCatchStatement and VisitTryFinallyStatement defer
to BuildTryCatch and BuildTryFinally, to allow this code to be reused by the
async function/generator desugarings in
BytecodeGenerator::GenerateBytecodeBody()

- BytecodeGenerator::VisitYield() now defers to BytecodeGenerator::BuildYield()
and BytecodeGenerator::BuildAwait(), depending on what type of yield it is,
which in turn perform different operations to suspend the generator.
BuildYield() is also used to desugar the initial yield in generators and
modules. Because of this, ast-numbering.cc ensures that modules and generator
have at least one yield point, and syntactic yields begin at yield_id=1 rather
than 0.

Because await desugaring happens in BytecodeGenerator rather than in
Parser::RewriteAwaitExpression(), BytecodeGenerator tracks the current
catch_prediction via member catch_prediction_, in the same way that
ast-numbering.cc does this. For each catch block generated (via BuildTryCatch),
if the catch prediction bit is not HandlerTable::UNCAUGHT,
BytecodeGenerator::catch_prediction_ is updated for the course of the catch
block, and restored to its previous value afterwards. This results in
BytecodeGenerator selecting the correct Await builtin, depending on whether it
occurs at the top-level of an async function, or within a syntactic catch try
scope.

BUG=v8:5896
R=gsathya@chromium.org, rmcilroy@chromium.org, neis@chromium.org
==========

[adamk, 2017-01-30 21:10:45]

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

[adamk, 2017-01-30 21:10:46]

From talking to neis about this this morning, it's not clear to me why moving
logic to the bytecode generator is necessary for fixing this bug. Can we not fix
the bug using the current AST-desugaring approach, and leave the rest of the
work as a separate concern?

[caitp, 2017-01-30 22:23:01]

On 2017/01/30 21:10:46, adamk wrote:
> From talking to neis about this this morning, it's not clear to me why moving
> logic to the bytecode generator is necessary for fixing this bug. Can we not
fix
> the bug using the current AST-desugaring approach, and leave the rest of the
> work as a separate concern?

I don't think there is a good way to do that using the AST desugaring. Since my
work is blocked on fixing this, and fits in well with the bytecode approach
anyways, I think this is the way to go.

What I can do is start with a separate "do it in BytecodeGenrerator without
fixing the bug " patch, and add a smaller fix on top of that. Does that make
people happy?

[rmcilroy, 2017-01-31 20:47:09]

On 2017/01/30 22:23:01, caitp wrote:
> On 2017/01/30 21:10:46, adamk wrote:
> > From talking to neis about this this morning, it's not clear to me why
moving
> > logic to the bytecode generator is necessary for fixing this bug. Can we not
> fix
> > the bug using the current AST-desugaring approach, and leave the rest of the
> > work as a separate concern?
> 
> I don't think there is a good way to do that using the AST desugaring. Since
my
> work is blocked on fixing this, and fits in well with the bytecode approach
> anyways, I think this is the way to go.
> 
> What I can do is start with a separate "do it in BytecodeGenrerator without
> fixing the bug " patch, and add a smaller fix on top of that. Does that make
> people happy?

I would definitly prefer a CL which is just moving where the desugaring happens.
I'd also like to understand why it's necessary to move the desugaring to the
bytecode generator? This adds a lot of complexity and I'm not clear on the
benefits. 

I'm traveling this week, so I'm unlikely to get to this review until next week.

[rmcilroy, 2017-01-31 20:50:58]

On 2017/01/31 20:47:09, rmcilroy (travelling till 3rd) wrote:
> On 2017/01/30 22:23:01, caitp wrote:
> > On 2017/01/30 21:10:46, adamk wrote:
> > > From talking to neis about this this morning, it's not clear to me why
> moving
> > > logic to the bytecode generator is necessary for fixing this bug. Can we
not
> > fix
> > > the bug using the current AST-desugaring approach, and leave the rest of
the
> > > work as a separate concern?
> > 
> > I don't think there is a good way to do that using the AST desugaring. Since
> my
> > work is blocked on fixing this, and fits in well with the bytecode approach
> > anyways, I think this is the way to go.
> > 
> > What I can do is start with a separate "do it in BytecodeGenrerator without
> > fixing the bug " patch, and add a smaller fix on top of that. Does that make
> > people happy?
> 
> I would definitly prefer a CL which is just moving where the desugaring
happens.
> I'd also like to understand why it's necessary to move the desugaring to the
> bytecode generator? This adds a lot of complexity and I'm not clear on the
> benefits. 
> 
> I'm traveling this week, so I'm unlikely to get to this review until next
week.

Just realized you've already split this CL. Could you please close this CL if it
no longer needs reviewing.

[caitp, 2017-01-31 21:40:59]

On 2017/01/31 20:47:09, rmcilroy (travelling till 3rd) wrote:
> I would definitly prefer a CL which is just moving where the desugaring
happens.
> I'd also like to understand why it's necessary to move the desugaring to the
> bytecode generator? This adds a lot of complexity and I'm not clear on the
> benefits. 
> 
> I'm traveling this week, so I'm unlikely to get to this review until next
week.

Moving the desugaring of `return` to the BytecodeGenerator allows us to reuse
the state machine which already exists, and defers return commands until after
finally blocks have been completed.

Moving the rest of the desugaring to bytecode is an easy fix for removing the
two implicit try/catch with just one in async functions, which has been on a
todo list for a while.