Fix returning from async functions.

This CL deals with handling of return statements in functions modified as
async. These functions contain a so-called async closure (with the actual body)
that should not just return when encountering a return statement, but rather call
the completer first and then return (null).

Previously the last return node has been replaced with a completer.complete call.
This does not work for return statements in nested blocks or try-catch-finally
blocks.

Instead of replacing return statements we now call the completer after
resolving all steps necessary for return nodes, i.e., right after inlining all
finally blocks.

We also need "continuation" returns later on that do not complete the
completer. We differentiate by whether a return node has its scope set or not.

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

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

[Michael Lippautz (Google), 2014-08-12 18:21:57]

adding the scope to return nodes seems ugly, any suggestions?

[hausner, 2014-08-12 20:03:56]

Holding off with the review until you make the change we discussed offline.

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

https://codereview.chromium.org/460763002/diff/180001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:521: explicit ReturnNode(intptr_t token_pos, LocalScope*
return_scope)
I appreciate your attempt to use a long name, but 'scope' is good enough here.
return_scope can be misleading: is it the scope we return to?

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

PTAL, scope is now added through SequenceNode::Add() to return nodes.

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

https://codereview.chromium.org/460763002/diff/180001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:521: explicit ReturnNode(intptr_t token_pos, LocalScope*
return_scope)
On 2014/08/12 20:03:56, hausner wrote:
> I appreciate your attempt to use a long name, but 'scope' is good enough here.
> return_scope can be misleading: is it the scope we return to?

Done.

[hausner, 2014-08-12 20:51:27]

LGTMwC

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

https://codereview.chromium.org/460763002/diff/220001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:261: void Add(AstNode* node) {
This should now maybe go into the .cc file. Then you can also avoid the
re-ordering of class declarations.

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

https://codereview.chromium.org/460763002/diff/220001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1055: // We distinguish those types by whether
a scope() has been set or not.
Is it possible to have a return without a sequence node? As soon as you insert a
ReturnNode into a SequenceNode, it will have a scope...

https://codereview.chromium.org/460763002/diff/220001/tests/language/async_co...
File tests/language/async_control_structures_test.dart (right):

https://codereview.chromium.org/460763002/diff/220001/tests/language/async_co...
tests/language/async_control_structures_test.dart:24: return 3;
Maybe a comment why there is a return statement here that never gets executed?

[Michael Lippautz (Google), 2014-08-12 21:13:57]

Thanks, waiting for a backend guy :)

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

https://codereview.chromium.org/460763002/diff/220001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:261: void Add(AstNode* node) {
On 2014/08/12 20:51:26, hausner wrote:
> This should now maybe go into the .cc file. Then you can also avoid the
> re-ordering of class declarations.

Done.

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

https://codereview.chromium.org/460763002/diff/220001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1055: // We distinguish those types by whether
a scope() has been set or not.
On 2014/08/12 20:51:26, hausner wrote:
> Is it possible to have a return without a sequence node? As soon as you insert
a
> ReturnNode into a SequenceNode, it will have a scope...

True. I added a flag for this that is always true for now. Once we add the
"continuation" returns this flag will differentiate those kinds of returns.

https://codereview.chromium.org/460763002/diff/220001/tests/language/async_co...
File tests/language/async_control_structures_test.dart (right):

https://codereview.chromium.org/460763002/diff/220001/tests/language/async_co...
tests/language/async_control_structures_test.dart:24: return 3;
On 2014/08/12 20:51:26, hausner wrote:
> Maybe a comment why there is a return statement here that never gets executed?

Done.

[srdjan, 2014-08-12 21:39:25]

lgtm

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

https://codereview.chromium.org/460763002/diff/240001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:566: // Returns true if the return node is not used to return
from a continuation.
Simplify: returns false if the return node is used to return from a
continuation. TODO(..): implement ... so that it is clear that it will be set to
false later.

[Michael Lippautz (Google), 2014-08-12 21:50:44]

Committed patchset #5 manually as 39167 (presubmit successful).

[Michael Lippautz (Google), 2014-08-12 21:50:51]

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

https://codereview.chromium.org/460763002/diff/240001/runtime/vm/ast.h#newcod...
runtime/vm/ast.h:566: // Returns true if the return node is not used to return
from a continuation.
On 2014/08/12 21:39:24, srdjan wrote:
> Simplify: returns false if the return node is used to return from a
> continuation. TODO(..): implement ... so that it is clear that it will be set
to
> false later.

Done.

[Florian Schneider, 2014-08-13 11:10:29]

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

https://codereview.chromium.org/460763002/diff/280001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1069: arguments->Add(PushArgument(rcv_value));
I'm not sure this works. Is this code path exercised by your tests?

You need to follow a stack discipline when pushing the arguments. Bind pushes on
the expression stack, PushArgument consumes the top value.

Do(BuildStoreExprTemp(return_value));

Value* rcv_value = Bind(BuildLoadLocal(*rcv_var));
arguments->Add(PushArgument(rcv_value));

Value* returned_value = Bind(BuildLoadExprTemp());
arguments->Add(PushArgument(returned_value));

In this case you could also just swap the two PushArgument calls, because
evaluating the arguments does not have side-effects.

[Florian Schneider, 2014-08-13 11:26:36]

On 2014/08/13 11:10:29, Florian Schneider wrote:
>
https://codereview.chromium.org/460763002/diff/280001/runtime/vm/flow_graph_b...
> File runtime/vm/flow_graph_builder.cc (right):
> 
>
https://codereview.chromium.org/460763002/diff/280001/runtime/vm/flow_graph_b...
> runtime/vm/flow_graph_builder.cc:1069:
arguments->Add(PushArgument(rcv_value));
> I'm not sure this works. Is this code path exercised by your tests?
> 
> You need to follow a stack discipline when pushing the arguments. Bind pushes
on
> the expression stack, PushArgument consumes the top value.
> 
> Do(BuildStoreExprTemp(return_value));
> 
> Value* rcv_value = Bind(BuildLoadLocal(*rcv_var));
> arguments->Add(PushArgument(rcv_value));
> 
> Value* returned_value = Bind(BuildLoadExprTemp());
> arguments->Add(PushArgument(returned_value));
> 
> In this case you could also just swap the two PushArgument calls, because
> evaluating the arguments does not have side-effects.

This is probably the reason it does not work for optimized code.

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

On 2014/08/13 11:26:36, Florian Schneider wrote:
> On 2014/08/13 11:10:29, Florian Schneider wrote:
> >
>
https://codereview.chromium.org/460763002/diff/280001/runtime/vm/flow_graph_b...
> > File runtime/vm/flow_graph_builder.cc (right):
> > 
> >
>
https://codereview.chromium.org/460763002/diff/280001/runtime/vm/flow_graph_b...
> > runtime/vm/flow_graph_builder.cc:1069:
> arguments->Add(PushArgument(rcv_value));
> > I'm not sure this works. Is this code path exercised by your tests?
> > 
> > You need to follow a stack discipline when pushing the arguments. Bind
pushes
> on
> > the expression stack, PushArgument consumes the top value.
> > 
> > Do(BuildStoreExprTemp(return_value));
> > 
> > Value* rcv_value = Bind(BuildLoadLocal(*rcv_var));
> > arguments->Add(PushArgument(rcv_value));
> > 
> > Value* returned_value = Bind(BuildLoadExprTemp());
> > arguments->Add(PushArgument(returned_value));
> > 
> > In this case you could also just swap the two PushArgument calls, because
> > evaluating the arguments does not have side-effects.
> 
> This is probably the reason it does not work for optimized code.

Thanksa lot! Yes, this also fixes the problem w/ optimized code.

See https://codereview.chromium.org/467323002/