Await it!

Await it!

Add support for awaiting futures. This leaves us with a non-structural CFG.

TODOs:
* Forwarding exceptions through futures in async functions and rethrowing them using await.
* Known bug with nested contexts (will be fixed in following CL)
* await is not recognized at all expressions specified (yet)

BUG=
R=hausner@google.com, srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39559

[Michael Lippautz (Google), 2014-08-19 18:01:58]

RFC -- help needed

This approach works for an example without nested contexts.

The example attached to the CL however creates a nested context using a loop and
does not work. A cannot see the problem in the flow graph, but it should be
around saving/restoring the context and passing (through a temporary) the
continuation result. The problem is after resuming the result does still contain
a Future, and not the passed value.

(Also, I commented out the ASSERTs for exceptions as I need to check whether to
not capture them or capturing is safe.)

Obviously it's still rough around the edges...

run with:
out/DebugIA32/dart --enable_async tests/language/await_future_test.dart

[Michael Lippautz (Google), 2014-08-20 02:03:32]

PTAL, +hausner

The bug was caused by not properly dereferencing a nested context when copying
over the continuation result.

The implementation now follows what I have presented. Next step is to add tests.

[srdjan, 2014-08-20 17:50:07]

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.h (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.h:51: // scope.
Please fix comment.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.h:53: // We also need the jump counter in the FGB.
Please use FlowGraphBuilder instead of FGB

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:1473: // FGB is at top context level, but the
await target has possibly been
ditto for FGB

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:3731: if (node ==
owner()->parsed_function()->node_sequence() &&
Add parentheses.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:3757: if (node ==
owner()->parsed_function()->node_sequence() &&
ditto

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.h (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.h:182: int32_t next_await_counter() { return
jump_cnt_++; }
Why does it need to be 32 bits (int32_t), could it be just intptr_t?

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:5569: ArgumentListNode* empty_args = new(I)
ArgumentListNode(Scanner::kNoSourcePos);
The prevalent type is new (I) (with space), please adapt code.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc#new...
runtime/vm/scopes.cc:609: VariableAt(i)->name().raw() ==
Symbols::ExceptionVar().raw()) {
Please add parentheses.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc#new...
runtime/vm/scopes.cc:611: // interrupted by await.
s/there/their/ ?

https://codereview.chromium.org/484933003/diff/80001/tests/language/await_fut...
File tests/language/await_future_test.dart (right):

https://codereview.chromium.org/484933003/diff/80001/tests/language/await_fut...
tests/language/await_future_test.dart:5: // VMOptions=--enable_async
"--optimization-counter-threshold=5" as well?

[hausner, 2014-08-20 19:47:49]

More comments to chew on. I think this CL will be good after the next round.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:90: // transformer.
should never be visited by the transformer?

Why not list this node in the FOR_EACH_UNREACHABLE_NODE macro above?

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:121: AwaitMarkerNode* await_marker =
A comment would be helpful here, explaining what the AwaitMarker node is for.
Maybe it makes sense to describe the purpose of these nodes in ast.h rather than
here, might make more sense. These nodes are not straight-forward to understand
like an IfNode or StaticCall node.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:130:
continuation_return->set_is_regular_return(false);
It would be nice to have a more descriptive name than "is_regular_return". Is it
a yield? (That word will probably be used later on for streams, though.) Or is
it a suspend instead of return?

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.h (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.h:183: 
@srdjan: I am fine with int32_t. This value is not expected to be more than
single digit. Let's not burn 7 bytes on 64bit platforms. In fact, I think we
could use int16_t but we'd probably not save anything due to alignment of other
fields.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:5429:
current_block_->scope->LookupVariable(Symbols::Completer(), false);
Could you add a comment why the variable is looked up? I presume it's to mark it
captured as a side effect of the lookup.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:5560: AstNode* null_literal = new(I) LiteralNode(
You should not need to initialize these to null explicitly. The runtime will do
this automatically for all locals (and context variables).

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:5620:
body->scope()->LookupVariable(Symbols::AwaitJumpVar(), false);
A comment would be nice explaining why you are looking up the variables.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc#new...
runtime/vm/scopes.cc:610: // Don't capture those variables as there scope of use
can not be
there -> their

I'm not sure what the "scope of use" of a variable is.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc#new...
runtime/vm/scopes.cc:615: VariableAt(i)->set_is_captured();
Why do you need to set the variable as captured again?

https://codereview.chromium.org/484933003/diff/80001/tests/language/await_fut...
File tests/language/await_future_test.dart (right):

https://codereview.chromium.org/484933003/diff/80001/tests/language/await_fut...
tests/language/await_future_test.dart:21: print(result);
Use Expect.equals to verify that the result is correct?

[Michael Lippautz (Google), 2014-08-20 20:56:07]

PTAL

Good catch with optimizations. I forgot to set the backwards pointer in one
place. It now compiles a proper SSA and executes optimized code.

Addressed all comments and added another test with try/finally and
optimizations.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:90: // transformer.
On 2014/08/20 19:47:48, hausner wrote:
> should never be visited by the transformer?
> 
> Why not list this node in the FOR_EACH_UNREACHABLE_NODE macro above?

Done.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:121: AwaitMarkerNode* await_marker =
On 2014/08/20 19:47:48, hausner wrote:
> A comment would be helpful here, explaining what the AwaitMarker node is for.
> Maybe it makes sense to describe the purpose of these nodes in ast.h rather
than
> here, might make more sense. These nodes are not straight-forward to
understand
> like an IfNode or StaticCall node.

Done in ast.h

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:130:
continuation_return->set_is_regular_return(false);
On 2014/08/20 19:47:48, hausner wrote:
> It would be nice to have a more descriptive name than "is_regular_return". Is
it
> a yield? (That word will probably be used later on for streams, though.) Or is
> it a suspend instead of return?

As discussed offline: ReturnNodes will get a type() and for now they can be
kRegular (user dart code) or kContinuation (continuation returns). 

We can extend this once we have yield.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.h (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.h:51: // scope.
On 2014/08/20 17:50:06, srdjan wrote:
> Please fix comment.

Done.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.h:53: // We also need the jump counter in the FGB.
On 2014/08/20 17:50:06, srdjan wrote:
> Please use FlowGraphBuilder instead of FGB

Done.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:1473: // FGB is at top context level, but the
await target has possibly been
On 2014/08/20 17:50:06, srdjan wrote:
> ditto for FGB

Done.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:3731: if (node ==
owner()->parsed_function()->node_sequence() &&
On 2014/08/20 17:50:06, srdjan wrote:
> Add parentheses.

Done.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:3757: if (node ==
owner()->parsed_function()->node_sequence() &&
On 2014/08/20 17:50:06, srdjan wrote:
> ditto

Done.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.h (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.h:182: int32_t next_await_counter() { return
jump_cnt_++; }
On 2014/08/20 17:50:06, srdjan wrote:
> Why does it need to be 32 bits (int32_t), could it be just intptr_t?

Done.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.h:183: 
On 2014/08/20 19:47:48, hausner wrote:
> @srdjan: I am fine with int32_t. This value is not expected to be more than
> single digit. Let's not burn 7 bytes on 64bit platforms. In fact, I think we
> could use int16_t but we'd probably not save anything due to alignment of
other
> fields.

intptr_t it is

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:5429:
current_block_->scope->LookupVariable(Symbols::Completer(), false);
On 2014/08/20 19:47:48, hausner wrote:
> Could you add a comment why the variable is looked up? I presume it's to mark
it
> captured as a side effect of the lookup.

Yes, it's about capturing it. Moved into CloseAsyncClosure() and added a
comment.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:5560: AstNode* null_literal = new(I) LiteralNode(
On 2014/08/20 19:47:48, hausner wrote:
> You should not need to initialize these to null explicitly. The runtime will
do
> this automatically for all locals (and context variables).

Done.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:5569: ArgumentListNode* empty_args = new(I)
ArgumentListNode(Scanner::kNoSourcePos);
On 2014/08/20 17:50:06, srdjan wrote:
> The prevalent type is new (I) (with space), please adapt code.

Done. I have seen "new(I)" and "new (I)" all around our codebase. Will write new
code with "new (I)".

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:5620:
body->scope()->LookupVariable(Symbols::AwaitJumpVar(), false);
On 2014/08/20 19:47:48, hausner wrote:
> A comment would be nice explaining why you are looking up the variables.

Added a comment in the source.

Currently, all variables from all scopes are captured anyways. This lookup
ensures that if we do something smarter later on we will still have the
necessary internal variables available.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc#new...
runtime/vm/scopes.cc:609: VariableAt(i)->name().raw() ==
Symbols::ExceptionVar().raw()) {
On 2014/08/20 17:50:06, srdjan wrote:
> Please add parentheses.

Done.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc#new...
runtime/vm/scopes.cc:610: // Don't capture those variables as there scope of use
can not be
On 2014/08/20 19:47:48, hausner wrote:
> there -> their
> 
> I'm not sure what the "scope of use" of a variable is.

There is no way to interrupt those variable's scopes with an await expression.
In other words: They will be assigned variables and it is not possible to reach
an await until they are read.

I didn't want to break the FlowGraphBuilder's assumption on them lying on the
stack. (flow_graph_builder.h:3896-3897)

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc#new...
runtime/vm/scopes.cc:611: // interrupted by await.
On 2014/08/20 17:50:06, srdjan wrote:
> s/there/their/ ?

Done.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc#new...
runtime/vm/scopes.cc:615: VariableAt(i)->set_is_captured();
On 2014/08/20 19:47:48, hausner wrote:
> Why do you need to set the variable as captured again?

CaptureVariable() conveniently manages regular capturing of variables but does
not mark variables on the same context level as captured. However, for await we
also need to capture those.

Example:
  for (int i = 0; ...; ...) {...}

"i" needs to be captured, but CaptureVariable() sees that it's on the same scope
level (somewhere in the for loop) and won't do anything here.

https://codereview.chromium.org/484933003/diff/80001/tests/language/await_fut...
File tests/language/await_future_test.dart (right):

https://codereview.chromium.org/484933003/diff/80001/tests/language/await_fut...
tests/language/await_future_test.dart:5: // VMOptions=--enable_async
On 2014/08/20 17:50:06, srdjan wrote:
> "--optimization-counter-threshold=5" as well?

Done.

https://codereview.chromium.org/484933003/diff/80001/tests/language/await_fut...
tests/language/await_future_test.dart:21: print(result);
On 2014/08/20 19:47:49, hausner wrote:
> Use Expect.equals to verify that the result is correct? 

Done.

[hausner, 2014-08-20 21:42:53]

LGTM

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc#new...
runtime/vm/scopes.cc:615: VariableAt(i)->set_is_captured();
Maybe a one-liner comment might help a future reader who has the same question.

On 2014/08/20 20:56:07, Michael Lippautz wrote:
> On 2014/08/20 19:47:48, hausner wrote:
> > Why do you need to set the variable as captured again?
> 
> CaptureVariable() conveniently manages regular capturing of variables but does
> not mark variables on the same context level as captured. However, for await
we
> also need to capture those.
> 
> Example:
>   for (int i = 0; ...; ...) {...}
> 
> "i" needs to be captured, but CaptureVariable() sees that it's on the same
scope
> level (somewhere in the for loop) and won't do anything here.

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:177: // 2) kTargetForContinuation: A marker indicating an entry
point for the last
Last as in "most recent" or "previous".

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:95: //     AwaitMarker(kNewcontinuationState);
This does not compile, right? Should be a capital C and qualified:
AwaitMarkerNode::kNewContinuationState

[srdjan, 2014-08-20 23:08:42]

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:586: enum Type {
I would rename the enum since we have a class Type already (see object.h). Maybe
ReturnType?

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:637: Type type_;
Add printing of type to ReturnNode printing.

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.h (right):

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:52: // needed variables to get their correspondig
aliases.
s/correspondig/corresponding/

[Florian Schneider, 2014-08-21 11:09:29]

dbc:

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:2194: StoreLocalNode* store_jump_cnt = new(I)
StoreLocalNode(
Why do you create an AST node first, only to visit it immediately with the
EffectGraphVisitor.

Instead, you can simplify the code by appending IL instructions directly:

Value* value = Bind(new ConstantInstr(Smi::ZoneHandle(I, Smi::New(jump_cnt)));

Do(new StoreLocalInstr(*jump_var, jmp_cnt_val))

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:2206: } else if (node->type() ==
AwaitMarkerNode::kTargetForContinuation) {
No need for "else if" if you return inside the if-part.

if (x) {
  ...
  return;
}

if (y) {

}

[Michael Lippautz (Google), 2014-08-21 16:39:14]

Addressed all comments.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:2194: StoreLocalNode* store_jump_cnt = new(I)
StoreLocalNode(
On 2014/08/21 11:09:29, Florian Schneider wrote:
> Why do you create an AST node first, only to visit it immediately with the
> EffectGraphVisitor.
> 
> Instead, you can simplify the code by appending IL instructions directly:
> 
> Value* value = Bind(new ConstantInstr(Smi::ZoneHandle(I, Smi::New(jump_cnt)));
> 
> Do(new StoreLocalInstr(*jump_var, jmp_cnt_val))

Yes and no :)

I inlined the IL instructions, but used BuildStoreLocal() instead of
StoreLocalInstr() directly since the variable lives in a context.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:2206: } else if (node->type() ==
AwaitMarkerNode::kTargetForContinuation) {
On 2014/08/21 11:09:29, Florian Schneider wrote:
> No need for "else if" if you return inside the if-part.
> 
> if (x) {
>   ...
>   return;
> }
> 
> if (y) {
> 
> }

Done.

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/484933003/diff/80001/runtime/vm/scopes.cc#new...
runtime/vm/scopes.cc:615: VariableAt(i)->set_is_captured();
On 2014/08/20 21:42:52, hausner wrote:
> Maybe a one-liner comment might help a future reader who has the same
question.
> 
> On 2014/08/20 20:56:07, Michael Lippautz wrote:
> > On 2014/08/20 19:47:48, hausner wrote:
> > > Why do you need to set the variable as captured again?
> > 
> > CaptureVariable() conveniently manages regular capturing of variables but
does
> > not mark variables on the same context level as captured. However, for await
> we
> > also need to capture those.
> > 
> > Example:
> >   for (int i = 0; ...; ...) {...}
> > 
> > "i" needs to be captured, but CaptureVariable() sees that it's on the same
> scope
> > level (somewhere in the for loop) and won't do anything here.
> 

Done.

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:177: // 2) kTargetForContinuation: A marker indicating an entry
point for the last
On 2014/08/20 21:42:52, hausner wrote:
> Last as in "most recent" or "previous".

done: most recent

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:586: enum Type {
On 2014/08/20 23:08:42, srdjan wrote:
> I would rename the enum since we have a class Type already (see object.h).
Maybe
> ReturnType?

Done.

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:637: Type type_;
On 2014/08/20 23:08:42, srdjan wrote:
> Add printing of type to ReturnNode printing.

Done.

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:95: //     AwaitMarker(kNewcontinuationState);
On 2014/08/20 21:42:53, hausner wrote:
> This does not compile, right? Should be a capital C and qualified:
> AwaitMarkerNode::kNewContinuationState

Fixed in the comment.

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.h (right):

https://codereview.chromium.org/484933003/diff/100001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.h:52: // needed variables to get their correspondig
aliases.
On 2014/08/20 23:08:42, srdjan wrote:
> s/correspondig/corresponding/

Done.

[Michael Lippautz (Google), 2014-08-25 16:46:13]

PTAL; update contains fixes for try/catch/finally

As discussed offline for try/catch/finally we don't capture the
:saved_try_context_var but rather leave it on the stack. (If you find a way to
exactly know the ctx level after a throw we can change this.) Instead we add
additional captured variables and restore the stack variables from those.

[hausner, 2014-08-25 20:26:14]

Front-end LGTMwC. Would be nice if one of the back-enders could take a final
look, too.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5526: //   var :await_saved_try_ctx_var_<x>;
is the name :await_saved_try_ctx_var_ or :async_saved_try_ctx_var_?

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5527: Type& dynamic_type = Type::ZoneHandle(I,
Type::DynamicType());
const

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5551: const String& cnt_str = String::ZoneHandle(
cnt_str can be a regular handle, need not be a zone handle.

Also, the code pattern of building a string or symbol from this prefix and a
given integer appears in 3 places. Maybe you can factor it out into a short
static inline function with parameters isolate and index.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7423: 
Could you add a short comment describing what this helper function does?

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7431: const String& cnt_str = String::ZoneHandle(
cnt_str probably doesn't need to be a ZoneHandle; a regular Handle is
sufficient.

[srdjan, 2014-08-26 15:54:34]

https://codereview.chromium.org/484933003/diff/140001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/484933003/diff/140001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:3764: ASSERT(ctx_var != NULL &&
ctx_var->is_captured());
Add parantheses above

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:97: //     return;  // (is_regular_return() ==
false)
There is no more is_regular_return(); please adjust comment.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1464: ASSERT(continuation_result != NULL &&
continuation_result->is_captured());
Add parentheses

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1465: ASSERT(old_ctx != NULL &&
old_ctx->is_captured());
ditto

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1466: // Before resoring the continuation
context we need to temporary save the
s/resoring/restoring/

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:2191: ASSERT(ctx_var != NULL &&
ctx_var->is_captured());
parentheses

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7509: const String& cnt_str = String::ZoneHandle(
Regular handle: use ZoneHandle-s only if the variable escapes its scope.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/scopes.cc#ne...
runtime/vm/scopes.cc:625: 
two instead of three empty lines

[Michael Lippautz (Google), 2014-08-26 16:45:49]

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:97: //     return;  // (is_regular_return() ==
false)
On 2014/08/26 15:54:33, srdjan wrote:
> There is no more is_regular_return(); please adjust comment.

Done.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1464: ASSERT(continuation_result != NULL &&
continuation_result->is_captured());
On 2014/08/26 15:54:33, srdjan wrote:
> Add parentheses

Done.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1465: ASSERT(old_ctx != NULL &&
old_ctx->is_captured());
On 2014/08/26 15:54:33, srdjan wrote:
> ditto

Done.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1466: // Before resoring the continuation
context we need to temporary save the
On 2014/08/26 15:54:33, srdjan wrote:
> s/resoring/restoring/

Done.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:2191: ASSERT(ctx_var != NULL &&
ctx_var->is_captured());
On 2014/08/26 15:54:33, srdjan wrote:
> parentheses

Done.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5526: //   var :await_saved_try_ctx_var_<x>;
On 2014/08/25 20:26:14, hausner wrote:
> is the name :await_saved_try_ctx_var_ or :async_saved_try_ctx_var_?

Done. Using :async_...

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5527: Type& dynamic_type = Type::ZoneHandle(I,
Type::DynamicType());
On 2014/08/25 20:26:14, hausner wrote:
> const

Done.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5551: const String& cnt_str = String::ZoneHandle(
On 2014/08/25 20:26:14, hausner wrote:
> cnt_str can be a regular handle, need not be a zone handle.
> 
> Also, the code pattern of building a string or symbol from this prefix and a
> given integer appears in 3 places. Maybe you can factor it out into a short
> static inline function with parameters isolate and index. 

Done: BuildAsyncSavedTryContextName()

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7423: 
On 2014/08/25 20:26:14, hausner wrote:
> Could you add a short comment describing what this helper function does?

Done.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7431: const String& cnt_str = String::ZoneHandle(
On 2014/08/25 20:26:14, hausner wrote:
> cnt_str probably doesn't need to be a ZoneHandle; a regular Handle is
> sufficient.

Done.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7509: const String& cnt_str = String::ZoneHandle(
On 2014/08/26 15:54:33, srdjan wrote:
> Regular handle: use ZoneHandle-s only if the variable escapes its scope.

Done. Code is now refactored but uses a regular handle for constructing the
string.

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/484933003/diff/160001/runtime/vm/scopes.cc#ne...
runtime/vm/scopes.cc:625: 
On 2014/08/26 15:54:33, srdjan wrote:
> two instead of three empty lines

Done.

[srdjan, 2014-08-26 17:10:33]

lgtm

[Michael Lippautz (Google), 2014-08-26 17:36:06]

Committed patchset #11 manually as 39559 (presubmit successful).