Fix async machinery for finally clauses (issues 22445, 22300).

runtime/vm/parser.cc

Index: runtime/vm/parser.cc
===================================================================
--- runtime/vm/parser.cc	(revision 44206)
+++ runtime/vm/parser.cc	(working copy)
@@ -5985,6 +5985,7 @@
 
   // The try-block (closure body code) has been parsed. We are now
   // generating the code for the catch block.
+  LocalScope* try_scope = current_block_->scope;
   try_blocks_list_->enter_catch();
   OpenBlock();  // Catch handler list.
   OpenBlock();  // Catch block.
@@ -6006,12 +6007,12 @@
 
   // Generate code to save the exception object and stack trace
   // in local variables.
-  LocalVariable* context_var = current_block_->scope->LookupVariable(
-      Symbols::SavedTryContextVar(), false);
+  LocalVariable* context_var = try_scope->LocalLookupVariable(
+      Symbols::SavedTryContextVar());
   ASSERT(context_var != NULL);
 
-  LocalVariable* exception_var = current_block_->scope->LookupVariable(
-      Symbols::ExceptionVar(), false);
+  LocalVariable* exception_var = try_scope->LocalLookupVariable(
+      Symbols::ExceptionVar());
   ASSERT(exception_var != NULL);
   if (exception_param.var != NULL) {
     // Generate code to load the exception object (:exception_var) into
@@ -6023,7 +6024,7 @@
   }
 
   LocalVariable* stack_trace_var =
-      current_block_->scope->LookupVariable(Symbols::StackTraceVar(), false);
+      try_scope->LocalLookupVariable(Symbols::StackTraceVar());
   ASSERT(stack_trace_var != NULL);
   if (stack_trace_param.var != NULL) {
     // A stack trace variable is specified in this block, so generate code
@@ -6034,11 +6035,12 @@
         stack_trace_param.var,
         new(Z) LoadLocalNode(Scanner::kNoSourcePos, stack_trace_var)));
   }
-  LocalVariable* saved_exception_var = current_block_->scope->LookupVariable(
-      Symbols::SavedExceptionVar(), false);
-  LocalVariable* saved_stack_trace_var = current_block_->scope->LookupVariable(
-      Symbols::SavedStackTraceVar(), false);
-  SaveExceptionAndStacktrace(exception_var,
+  LocalVariable* saved_exception_var = try_scope->LocalLookupVariable(

[hausner, 2015/03/03 23:19:11]

Nit: the initializer value for the variable may now fit on one line, so the line
can wrap after the =. Dirro below.

[regis, 2015/03/03 23:47:09]

It does not.

+      Symbols::SavedExceptionVar());
+  LocalVariable* saved_stack_trace_var = try_scope->LocalLookupVariable(
+      Symbols::SavedStackTraceVar());
+  SaveExceptionAndStacktrace(current_block_->statements,
+                             exception_var,
                              stack_trace_var,
                              saved_exception_var,
                              saved_stack_trace_var);
@@ -6139,6 +6141,7 @@
   ASSERT(try_blocks_list_ != NULL);
   ASSERT(try_blocks_list_->outer_try_block() == NULL);
   ASSERT(innermost_function().IsAsyncClosure());
+  LocalScope* try_scope = current_block_->scope;
 
   try_blocks_list_->enter_catch();
 
@@ -6158,12 +6161,12 @@
   AddCatchParamsToScope(
       &exception_param, &stack_trace_param, current_block_->scope);
 
-  LocalVariable* context_var = current_block_->scope->LookupVariable(
-      Symbols::SavedTryContextVar(), false);
+  LocalVariable* context_var = try_scope->LocalLookupVariable(

[hausner, 2015/03/03 23:19:11]

Nit: line break after =

[regis, 2015/03/03 23:47:09]

ditto

+      Symbols::SavedTryContextVar());
   ASSERT(context_var != NULL);
 
-  LocalVariable* exception_var = current_block_->scope->LookupVariable(
-      Symbols::ExceptionVar(), false);
+  LocalVariable* exception_var = try_scope->LocalLookupVariable(
+      Symbols::ExceptionVar());
   if (exception_param.var != NULL) {
     // Generate code to load the exception object (:exception_var) into
     // the exception variable specified in this block.
@@ -6175,7 +6178,7 @@
   }
 
   LocalVariable* stack_trace_var =
-      current_block_->scope->LookupVariable(Symbols::StackTraceVar(), false);
+      try_scope->LocalLookupVariable(Symbols::StackTraceVar());
   if (stack_trace_param.var != NULL) {
     // A stack trace variable is specified in this block, so generate code
     // to load the stack trace object (:stack_trace_var) into the stack
@@ -6186,11 +6189,12 @@
         stack_trace_param.var,
         new(Z) LoadLocalNode(Scanner::kNoSourcePos, stack_trace_var)));
   }
-  LocalVariable* saved_exception_var = current_block_->scope->LookupVariable(
-      Symbols::SavedExceptionVar(), false);
-  LocalVariable* saved_stack_trace_var = current_block_->scope->LookupVariable(
-      Symbols::SavedStackTraceVar(), false);
-  SaveExceptionAndStacktrace(exception_var,
+  LocalVariable* saved_exception_var = try_scope->LocalLookupVariable(
+      Symbols::SavedExceptionVar());
+  LocalVariable* saved_stack_trace_var = try_scope->LocalLookupVariable(
+      Symbols::SavedStackTraceVar());
+  SaveExceptionAndStacktrace(current_block_->statements,
+                             exception_var,
                              stack_trace_var,
                              saved_exception_var,
                              saved_stack_trace_var);
@@ -6250,58 +6254,23 @@
 }
 
 
-// Wrap the body of the async or arync* closure in a try/catch block.
+// Wrap the body of the async or async* closure in a try/catch block.
 void Parser::OpenAsyncTryBlock() {
   ASSERT(innermost_function().IsAsyncClosure() ||
          innermost_function().IsAsyncGenClosure());
+  LocalVariable* context_var = NULL;
+  LocalVariable* exception_var = NULL;
+  LocalVariable* stack_trace_var = NULL;
+  LocalVariable* saved_exception_var = NULL;
+  LocalVariable* saved_stack_trace_var = NULL;
+  SetupExceptionVariables(current_block_->scope,
+                          true,
+                          &context_var,
+                          &exception_var,
+                          &stack_trace_var,
+                          &saved_exception_var,
+                          &saved_stack_trace_var);
 
-  const Type& dynamic_type = Type::ZoneHandle(Z, Type::DynamicType());
-  LocalVariable* context_var =
-      current_block_->scope->LocalLookupVariable(Symbols::SavedTryContextVar());
-  if (context_var == NULL) {
-    context_var = new(Z) LocalVariable(
-        TokenPos(),
-        Symbols::SavedTryContextVar(),
-        dynamic_type);
-    current_block_->scope->AddVariable(context_var);
-  }
-  LocalVariable* exception_var =
-      current_block_->scope->LocalLookupVariable(Symbols::ExceptionVar());
-  if (exception_var == NULL) {
-    exception_var = new(Z) LocalVariable(
-        TokenPos(),
-        Symbols::ExceptionVar(),
-        dynamic_type);
-    current_block_->scope->AddVariable(exception_var);
-  }
-  LocalVariable* stack_trace_var =
-      current_block_->scope->LocalLookupVariable(Symbols::StackTraceVar());
-  if (stack_trace_var == NULL) {
-    stack_trace_var = new(Z) LocalVariable(
-        TokenPos(),
-        Symbols::StackTraceVar(),
-        dynamic_type);
-    current_block_->scope->AddVariable(stack_trace_var);
-  }
-  LocalVariable* saved_exception_var =
-      current_block_->scope->LocalLookupVariable(Symbols::SavedExceptionVar());
-  if (saved_exception_var == NULL) {
-    saved_exception_var = new(Z) LocalVariable(
-        TokenPos(),
-        Symbols::SavedExceptionVar(),
-        dynamic_type);
-    current_block_->scope->AddVariable(saved_exception_var);
-  }
-  LocalVariable* saved_stack_trace_var =
-      current_block_->scope->LocalLookupVariable(Symbols::SavedStackTraceVar());
-  if (saved_stack_trace_var == NULL) {
-    saved_stack_trace_var = new(Z) LocalVariable(
-        TokenPos(),
-        Symbols::SavedStackTraceVar(),
-        dynamic_type);
-    current_block_->scope->AddVariable(saved_stack_trace_var);
-  }
-
   // Open the try block.
   OpenBlock();
   // This is the outermost try-catch in the function.
@@ -8689,7 +8658,8 @@
 // the saved exception variable (:saved_exception_var) used to rethrow.
 // Generate code to load the stack trace object (:stack_trace_var) into
 // the saved stacktrace variable (:saved_stack_trace_var) used to rethrow.
-void Parser::SaveExceptionAndStacktrace(LocalVariable* exception_var,
+void Parser::SaveExceptionAndStacktrace(SequenceNode* statements,
+                                        LocalVariable* exception_var,
                                         LocalVariable* stack_trace_var,
                                         LocalVariable* saved_exception_var,
                                         LocalVariable* saved_stack_trace_var) {
@@ -8702,7 +8672,7 @@
 
   ASSERT(saved_exception_var != NULL);
   ASSERT(exception_var != NULL);
-  current_block_->statements->Add(new(Z) StoreLocalNode(
+  statements->Add(new(Z) StoreLocalNode(
       Scanner::kNoSourcePos,
       saved_exception_var,
       new(Z) LoadLocalNode(Scanner::kNoSourcePos, exception_var)));
@@ -8709,7 +8679,7 @@
 
   ASSERT(saved_stack_trace_var != NULL);
   ASSERT(stack_trace_var != NULL);
-  current_block_->statements->Add(new(Z) StoreLocalNode(
+  statements->Add(new(Z) StoreLocalNode(
       Scanner::kNoSourcePos,
       saved_stack_trace_var,
       new(Z) LoadLocalNode(Scanner::kNoSourcePos, stack_trace_var)));
@@ -8716,7 +8686,12 @@
 }
 
 
-SequenceNode* Parser::ParseFinallyBlock() {
+SequenceNode* Parser::ParseFinallyBlock(
+    bool is_async,
+    LocalVariable* exception_var,
+    LocalVariable* stack_trace_var,
+    LocalVariable* rethrow_exception_var,
+    LocalVariable* rethrow_stack_trace_var) {
   TRACE_PARSER("ParseFinallyBlock");
   OpenBlock();
   ExpectToken(Token::kLBRACE);
@@ -8724,13 +8699,7 @@
   // In case of async closures we need to restore the saved try index of an
   // outer try block (if it exists).  The current try block has already been
   // removed from the stack of try blocks.
-  if ((innermost_function().IsAsyncClosure() ||
-       innermost_function().IsAsyncFunction() ||
-       innermost_function().IsSyncGenClosure() ||
-       innermost_function().IsSyncGenerator() ||
-       innermost_function().IsAsyncGenerator() ||
-       innermost_function().IsAsyncGenClosure()) &&
-      (try_blocks_list_ != NULL)) {
+  if (is_async && (try_blocks_list_ != NULL)) {
     LocalScope* scope = try_blocks_list_->try_block()->scope;
     if (scope->function_level() == current_block_->scope->function_level()) {
       current_block_->statements->Add(
@@ -8737,6 +8706,11 @@
           AwaitTransformer::RestoreSavedTryContext(
               Z, scope->parent(), try_blocks_list_->try_index()));
     }
+    SaveExceptionAndStacktrace(current_block_->statements,

[hausner, 2015/03/03 23:19:11]

Strictly speaking, this is only necessary if the finally block contains an await
or yield statement that suspends execution of the function. Correct? If so, it
might be worth adding a comment that we might want to "optimize" this at some
time in the future.

[regis, 2015/03/03 23:47:09]

Yes, I added a comment and a TODO(hausner) :-)

I also fixed a potential bug:
The call to RestoreSavedTryContext is only done if there is an outer try block,
but the saving of the exception variables is unconditional (even without an
outer try).

+                               exception_var,
+                               stack_trace_var,
+                               rethrow_exception_var,
+                               rethrow_stack_trace_var);
   }
 
   ParseStatementSequence();
@@ -8805,6 +8779,7 @@
 
 SequenceNode* Parser::ParseCatchClauses(
     intptr_t handler_pos,
+    bool is_async,
     LocalVariable* exception_var,
     LocalVariable* stack_trace_var,
     LocalVariable* rethrow_exception_var,
@@ -8882,31 +8857,6 @@
     // declarations in the body to shadow the catch parameters.
     CheckToken(Token::kLBRACE);
 
-    // In case of async closures we need to restore the saved try index of an
-    // outer try block (if it exists).
-    ASSERT(try_blocks_list_ != NULL);
-    if (innermost_function().IsAsyncClosure() ||
-        innermost_function().IsAsyncFunction() ||
-        innermost_function().IsSyncGenClosure() ||
-        innermost_function().IsSyncGenerator() ||
-        innermost_function().IsAsyncGenerator() ||
-        innermost_function().IsAsyncGenClosure()) {
-      const TryBlocks* try_block = try_blocks_list_->outer_try_block();
-      if (try_block != NULL) {
-        LocalScope* scope = try_block->try_block()->scope;
-        if (scope->function_level() ==
-            current_block_->scope->function_level()) {
-          current_block_->statements->Add(
-              AwaitTransformer::RestoreSavedTryContext(
-                  Z, scope->parent(), try_block->try_index()));
-        }
-      }
-      SaveExceptionAndStacktrace(exception_var,
-                                 stack_trace_var,
-                                 rethrow_exception_var,
-                                 rethrow_stack_trace_var);
-    }
-
     current_block_->statements->Add(ParseNestedStatement(false, NULL));
     catch_blocks.Add(CloseBlock());
 
@@ -9000,28 +8950,29 @@
         type_test->token_pos(), type_test, catch_block, current));
     current = CloseBlock();
   }
-  // Restore :saved_try_context_var before executing the catch clauses.
+  // In case of async closures, restore :saved_try_context_var before executing
+  // the catch clauses.
   if (current != NULL) {

[hausner, 2015/03/03 23:19:11]

Can this be simplified to if (is_async && (current != NULL))?

[regis, 2015/03/03 23:47:09]

Done.

     ASSERT(try_blocks_list_ != NULL);
-    if (innermost_function().IsAsyncClosure() ||
-        innermost_function().IsAsyncFunction() ||
-        innermost_function().IsSyncGenClosure() ||
-        innermost_function().IsSyncGenerator() ||
-        innermost_function().IsAsyncGenerator() ||
-        innermost_function().IsAsyncGenClosure()) {
+    if (is_async) {
+      SequenceNode* async_code = new(Z) SequenceNode(handler_pos, NULL);
       const TryBlocks* try_block = try_blocks_list_->outer_try_block();
       if (try_block != NULL) {
         LocalScope* scope = try_block->try_block()->scope;
         if (scope->function_level() ==
             current_block_->scope->function_level()) {
-          SequenceNode* restore_code = new(Z) SequenceNode(handler_pos, NULL);
-          restore_code->Add(
+          async_code->Add(
               AwaitTransformer::RestoreSavedTryContext(
                   Z, scope->parent(), try_block->try_index()));
-          restore_code->Add(current);
-          current = restore_code;
         }
       }
+      SaveExceptionAndStacktrace(async_code,

[Ivan Posva, 2015/03/03 22:57:57]

Please add comment what is being saved here and where this code is in relation
to the rest.

[regis, 2015/03/03 23:47:09]

Done.

+                                 exception_var,
+                                 stack_trace_var,
+                                 rethrow_exception_var,
+                                 rethrow_stack_trace_var);
+      async_code->Add(current);
+      current = async_code;
     }
   }
   return current;
@@ -9052,80 +9003,100 @@
 }
 
 
-AstNode* Parser::ParseTryStatement(String* label_name) {
-  TRACE_PARSER("ParseTryStatement");
-
-  // We create three variables for exceptions here:
-  // ':saved_try_context_var' - Used to save the context before the start of
-  //                            the try block. The context register is
-  //                            restored from this variable before
-  //                            processing the catch block handler.
-  // ':exception_var' - Used to save the current exception object that was
-  //                    thrown.
-  // ':stack_trace_var' - Used to save the current stack trace object which
-  //                      the stack trace was copied into when an exception
-  //                      was thrown.
-  // :exception_var and :stack_trace_var get set with the exception object
-  // and the stack trace object when an exception is thrown.  These three
-  // implicit variables can never be captured.
+// We create three variables for exceptions:
+// ':saved_try_context_var' - Used to save the context before the start of
+//                            the try block. The context register is
+//                            restored from this variable before
+//                            processing the catch block handler.
+// ':exception_var' - Used to save the current exception object that was
+//                    thrown.
+// ':stack_trace_var' - Used to save the current stack trace object which
+//                      the stack trace was copied into when an exception
+//                      was thrown.
+// :exception_var and :stack_trace_var get set with the exception object
+// and the stack trace object when an exception is thrown.  These three
+// implicit variables can never be captured.
+//
+// In case of async code, we create two additional variables:
+// ':saved_exception_var' - Used to capture the exception object above.
+// ':saved_stack_trace_var' - Used to capture the stack trace object above.
+void Parser::SetupExceptionVariables(LocalScope* try_scope,
+                                     bool is_async,
+                                     LocalVariable** context_var,
+                                     LocalVariable** exception_var,
+                                     LocalVariable** stack_trace_var,
+                                     LocalVariable** saved_exception_var,
+                                     LocalVariable** saved_stack_trace_var) {
   const Type& dynamic_type = Type::ZoneHandle(Z, Type::DynamicType());
   // Consecutive try statements share the same set of variables.
-  LocalVariable* context_var =
-      current_block_->scope->LocalLookupVariable(Symbols::SavedTryContextVar());
-  if (context_var == NULL) {
-    context_var = new(Z) LocalVariable(
+  *context_var = try_scope->LocalLookupVariable(Symbols::SavedTryContextVar());
+  if (*context_var == NULL) {
+    *context_var = new(Z) LocalVariable(
         TokenPos(),
         Symbols::SavedTryContextVar(),
         dynamic_type);
-    current_block_->scope->AddVariable(context_var);
+    try_scope->AddVariable(*context_var);
   }
-  LocalVariable* exception_var =
-      current_block_->scope->LocalLookupVariable(Symbols::ExceptionVar());
-  if (exception_var == NULL) {
-    exception_var = new(Z) LocalVariable(
+  *exception_var = try_scope->LocalLookupVariable(Symbols::ExceptionVar());
+  if (*exception_var == NULL) {
+    *exception_var = new(Z) LocalVariable(
         TokenPos(),
         Symbols::ExceptionVar(),
         dynamic_type);
-    current_block_->scope->AddVariable(exception_var);
+    try_scope->AddVariable(*exception_var);
   }
-  LocalVariable* stack_trace_var =
-      current_block_->scope->LocalLookupVariable(Symbols::StackTraceVar());
-  if (stack_trace_var == NULL) {
-    stack_trace_var = new(Z) LocalVariable(
+  *stack_trace_var = try_scope->LocalLookupVariable(Symbols::StackTraceVar());
+  if (*stack_trace_var == NULL) {
+    *stack_trace_var = new(Z) LocalVariable(
         TokenPos(),
         Symbols::StackTraceVar(),
         dynamic_type);
-    current_block_->scope->AddVariable(stack_trace_var);
+    try_scope->AddVariable(*stack_trace_var);
   }
-  LocalVariable* saved_exception_var = NULL;
-  LocalVariable* saved_stack_trace_var = NULL;
-  const bool is_async = innermost_function().IsAsyncClosure() ||
-                        innermost_function().IsAsyncFunction() ||
-                        innermost_function().IsSyncGenClosure() ||
-                        innermost_function().IsSyncGenerator() ||
-                        innermost_function().IsAsyncGenClosure() ||
-                        innermost_function().IsAsyncGenerator();
   if (is_async) {
-    saved_exception_var = current_block_->scope->LocalLookupVariable(
+    *saved_exception_var = try_scope->LocalLookupVariable(
         Symbols::SavedExceptionVar());
-    if (saved_exception_var == NULL) {
-      saved_exception_var = new(Z) LocalVariable(
+    if (*saved_exception_var == NULL) {
+      *saved_exception_var = new(Z) LocalVariable(
           TokenPos(),
           Symbols::SavedExceptionVar(),
           dynamic_type);
-      current_block_->scope->AddVariable(saved_exception_var);
+      try_scope->AddVariable(*saved_exception_var);
     }
-    saved_stack_trace_var = current_block_->scope->LocalLookupVariable(
+    *saved_stack_trace_var = try_scope->LocalLookupVariable(
         Symbols::SavedStackTraceVar());
-    if (saved_stack_trace_var == NULL) {
-      saved_stack_trace_var = new(Z) LocalVariable(
+    if (*saved_stack_trace_var == NULL) {
+      *saved_stack_trace_var = new(Z) LocalVariable(
           TokenPos(),
           Symbols::SavedStackTraceVar(),
           dynamic_type);
-      current_block_->scope->AddVariable(saved_stack_trace_var);
+      try_scope->AddVariable(*saved_stack_trace_var);
     }
   }
+}
 
+
+AstNode* Parser::ParseTryStatement(String* label_name) {
+  TRACE_PARSER("ParseTryStatement");
+  const bool is_async = innermost_function().IsAsyncClosure() ||
+                        innermost_function().IsAsyncFunction() ||
+                        innermost_function().IsSyncGenClosure() ||
+                        innermost_function().IsSyncGenerator() ||
+                        innermost_function().IsAsyncGenClosure() ||
+                        innermost_function().IsAsyncGenerator();
+  LocalVariable* context_var = NULL;
+  LocalVariable* exception_var = NULL;
+  LocalVariable* stack_trace_var = NULL;
+  LocalVariable* saved_exception_var = NULL;
+  LocalVariable* saved_stack_trace_var = NULL;
+  SetupExceptionVariables(current_block_->scope,
+                          is_async,
+                          &context_var,
+                          &exception_var,
+                          &stack_trace_var,
+                          &saved_exception_var,
+                          &saved_stack_trace_var);
+
   const intptr_t try_pos = TokenPos();
   ConsumeToken();  // Consume the 'try'.
 
@@ -9161,10 +9132,14 @@
       GrowableObjectArray::Handle(Z, GrowableObjectArray::New());
   bool needs_stack_trace = false;
   SequenceNode* catch_handler_list =
-      ParseCatchClauses(handler_pos, exception_var, stack_trace_var,
+      ParseCatchClauses(handler_pos,
+                        is_async,
+                        exception_var,
+                        stack_trace_var,
                         is_async ? saved_exception_var : exception_var,
                         is_async ? saved_stack_trace_var : stack_trace_var,
-                        handler_types, &needs_stack_trace);
+                        handler_types,
+                        &needs_stack_trace);
 
   TryBlocks* inner_try_block = PopTryBlock();
   const intptr_t try_index = inner_try_block->try_index();
@@ -9183,7 +9158,12 @@
     AstNode* node_to_inline =
         inner_try_block->GetNodeToInlineFinally(node_index);
     while (node_to_inline != NULL) {
-      finally_block = ParseFinallyBlock();
+      finally_block = ParseFinallyBlock(
+          is_async,
+          exception_var,
+          stack_trace_var,
+          is_async ? saved_exception_var : exception_var,
+          is_async ? saved_stack_trace_var : stack_trace_var);
       InlinedFinallyNode* node = new(Z) InlinedFinallyNode(finally_pos,
                                                            finally_block,
                                                            context_var,
@@ -9193,7 +9173,12 @@
       node_to_inline = inner_try_block->GetNodeToInlineFinally(node_index);
       tokens_iterator_.SetCurrentPosition(finally_pos);
     }
-    finally_block = ParseFinallyBlock();
+    finally_block = ParseFinallyBlock(
+        is_async,
+        exception_var,
+        stack_trace_var,
+        is_async ? saved_exception_var : exception_var,
+        is_async ? saved_stack_trace_var : stack_trace_var);
   }
 
   CatchClauseNode* catch_clause = new(Z) CatchClauseNode(