Save exception and stack trace variables in async catch clauses and use them

runtime/vm/parser.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/parser.h"
 
 #include "lib/invocation_mirror.h"
 #include "platform/utils.h"
 #include "vm/ast_transformer.h"
 #include "vm/bootstrap.h"
@@ skipping 5955 matching lines @@
   stack_trace_param.token_pos = Scanner::kNoSourcePos;
   stack_trace_param.type = &dynamic_type;
   stack_trace_param.name = &Symbols::StackTraceParameter();
 
   AddCatchParamsToScope(
       &exception_param, &stack_trace_param, current_block_->scope);
 
   LocalVariable* context_var = current_block_->scope->LookupVariable(
       Symbols::SavedTryContextVar(), false);
   ASSERT(context_var != NULL);
+
   LocalVariable* exception_var = current_block_->scope->LookupVariable(
       Symbols::ExceptionVar(), false);
   if (exception_param.var != NULL) {
     // Generate code to load the exception object (:exception_var) into
     // the exception variable specified in this block.
     ASSERT(exception_var != NULL);

[hausner, 2015/02/24 20:57:31]

Maybe it makes sense to pull this assert out of the if block, now that we use
the exception_var outside of the if.

Ditto for ASSERT(stack_trace_var != NULL) below.

[regis, 2015/02/24 22:05:13]

Done. Note that it was asserted in the constructor of CatchClauseNode below.

     current_block_->statements->Add(new(Z) StoreLocalNode(
         Scanner::kNoSourcePos,
         exception_param.var,
         new(Z) LoadLocalNode(Scanner::kNoSourcePos, exception_var)));
   }
+
   LocalVariable* stack_trace_var =
       current_block_->scope->LookupVariable(Symbols::StackTraceVar(), false);
   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
     // trace variable specified in this block.
     ASSERT(stack_trace_var != NULL);
     current_block_->statements->Add(new(Z) StoreLocalNode(
         Scanner::kNoSourcePos,
         stack_trace_param.var,
         new(Z) LoadLocalNode(Scanner::kNoSourcePos, stack_trace_var)));
   }
 
+  AddSavedExceptionAndStacktraceToScope(
+      exception_var, stack_trace_var, current_block_->scope);
+
   ASSERT(try_blocks_list_ != NULL);
   ASSERT(innermost_function().IsAsyncClosure() ||
          innermost_function().IsAsyncFunction());
   if ((try_blocks_list_->outer_try_block() != NULL) &&
       (try_blocks_list_->outer_try_block()->try_block()
           ->scope->function_level() ==
        current_block_->scope->function_level())) {
     // We need to unchain three scope levels: catch clause, catch
     // parameters, and the general try block.
     RestoreSavedTryContext(
@@ skipping 2175 matching lines @@
 void Parser::AddCatchParamsToScope(CatchParamDesc* exception_param,
                                    CatchParamDesc* stack_trace_param,
                                    LocalScope* scope) {
   if (exception_param->name != NULL) {
     LocalVariable* var = new(Z) LocalVariable(
         exception_param->token_pos,
         *exception_param->name,
         *exception_param->type);
     var->set_is_final();
     bool added_to_scope = scope->AddVariable(var);
-    ASSERT(added_to_scope);
+    ASSERT(added_to_scope);  // TODO(regis): Why do we check below but not here?

[hausner, 2015/02/24 20:57:31]

FWIW, I don't know either.

[regis, 2015/02/24 22:05:13]

I get it now.
The error can only be reported below when the name of the exception param is
reused for the stack trace param.
I have added a comment.

     exception_param->var = var;
   }
   if (stack_trace_param->name != NULL) {
     LocalVariable* var = new(Z) LocalVariable(
         stack_trace_param->token_pos,
         *stack_trace_param->name,
         *stack_trace_param->type);
     var->set_is_final();
     bool added_to_scope = scope->AddVariable(var);
     if (!added_to_scope) {
       ReportError(stack_trace_param->token_pos,
                   "name '%s' already exists in scope",
                   stack_trace_param->name->ToCString());
-       }
+    }
     stack_trace_param->var = var;
   }
 }
 
 
+// Populate local scope of the catch block with the saved exception and saved
+// stack trace.
+void Parser::AddSavedExceptionAndStacktraceToScope(

[Ivan Posva, 2015/02/24 20:52:39]

These variables are being added unconditionally here. You should assert that we
are in a async/sync*/async* body closure.

[regis, 2015/02/24 22:05:13]

Done.

+    LocalVariable* exception_var,
+    LocalVariable* stack_trace_var,
+    LocalScope* scope) {
+  // Add :saved_exception_var and :saved_stack_trace_var to scope.
+  LocalVariable* saved_exception_var = new (Z) LocalVariable(
+      Scanner::kNoSourcePos,
+      Symbols::SavedExceptionVar(),
+      Type::ZoneHandle(Z, Type::DynamicType()));
+  scope->AddVariable(saved_exception_var);
+  saved_exception_var->set_is_captured();

[Ivan Posva, 2015/02/24 20:52:39]

These variables should be automatically captured as is the rest of the
variables. If not we need to know why they are not being captured by the rest of
the system.
These variables also should be marked as final.

[hausner, 2015/02/24 20:57:31]

Capturing explicitly should not be necessary. After the function is parsed,
there is a recursive pass over all scopes to capture all variables. I think that
would also capture these two variables.

[regis, 2015/02/24 22:05:13]

Removed the explicit capturing and marked as final.

+  LocalVariable* saved_stack_trace_var = new (Z) LocalVariable(
+      Scanner::kNoSourcePos,
+      Symbols::SavedStackTraceVar(),
+      Type::ZoneHandle(Z, Type::DynamicType()));
+  scope->AddVariable(saved_stack_trace_var);
+  saved_stack_trace_var->set_is_captured();
+
+  // Generate code to load the exception object (:exception_var) into
+  // the saved exception variable (:saved_exception_var) used to rethrow.
+  saved_exception_var = current_block_->scope->LookupVariable(
+      Symbols::SavedExceptionVar(), false);
+  ASSERT(saved_exception_var != NULL);
+  ASSERT(exception_var != NULL);
+  current_block_->statements->Add(new(Z) StoreLocalNode(
+      Scanner::kNoSourcePos,
+      saved_exception_var,
+      new(Z) LoadLocalNode(Scanner::kNoSourcePos, exception_var)));
+
+  // Generate code to load the stack trace object (:stack_trace_var) into
+  // the saved stacktrace variable (:saved_stack_trace_var) used to rethrow.
+  saved_stack_trace_var = current_block_->scope->LookupVariable(
+      Symbols::SavedStackTraceVar(), false);
+  ASSERT(saved_stack_trace_var != NULL);
+  ASSERT(stack_trace_var != NULL);
+  current_block_->statements->Add(new(Z) StoreLocalNode(
+      Scanner::kNoSourcePos,
+      saved_stack_trace_var,
+      new(Z) LoadLocalNode(Scanner::kNoSourcePos, stack_trace_var)));
+}
+
+
 SequenceNode* Parser::ParseFinallyBlock() {
   TRACE_PARSER("ParseFinallyBlock");
   OpenBlock();
   ExpectToken(Token::kLBRACE);
 
   // 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() ||
@@ skipping 113 matching lines @@
         stack_trace_param.token_pos = TokenPos();
         stack_trace_param.name = ExpectIdentifier("identifier expected");
       }
       ExpectToken(Token::kRPAREN);
     }
 
     // Create a block containing the catch clause parameters and the
     // following code:
     // 1) Store exception object and stack trace object into user-defined
     //    variables (as needed).
-    // 2) Nested block with source code from catch clause block.
+    // 2) In async code, save exception object and stack trace object into
+    //    captured :saved_exception_var and :saved_stack_trace_var.
+    // 3) Nested block with source code from catch clause block.
     OpenBlock();
     AddCatchParamsToScope(&exception_param, &stack_trace_param,
                           current_block_->scope);
 
     if (exception_param.var != NULL) {
       // Generate code to load the exception object (:exception_var) into
       // the exception variable specified in this block.
       ASSERT(exception_var != NULL);
       current_block_->statements->Add(new(Z) StoreLocalNode(
           catch_pos, exception_param.var, new(Z) LoadLocalNode(
@@ skipping 27 matching lines @@
            current_block_->scope->function_level())) {
         // We need to unchain three scope levels: catch clause, catch
         // parameters, and the general try block.
         RestoreSavedTryContext(
             current_block_->scope->parent()->parent()->parent(),
             try_blocks_list_->outer_try_block()->try_index(),
             current_block_->statements);
       } else {
         parsed_function()->reset_saved_try_ctx_vars();
       }
+      AddSavedExceptionAndStacktraceToScope(
+          exception_var, stack_trace_var, current_block_->scope);
     }
 
     current_block_->statements->Add(ParseNestedStatement(false, NULL));
     catch_blocks.Add(CloseBlock());
 
     const bool is_bad_type =
         exception_param.type->IsMalformed() ||
         exception_param.type->IsMalbounded();
     if (exception_param.type->IsDynamicType() || is_bad_type) {
       // There is no exception type or else it is malformed or malbounded.
@@ skipping 124 matching lines @@
   current_block_->statements->Add(new(Z) StoreLocalNode(
       Scanner::kNoSourcePos,
       async_saved_try_ctx,
       new(Z) LoadLocalNode(Scanner::kNoSourcePos, saved_try_context)));
   parsed_function()->set_saved_try_ctx(saved_try_context);
   parsed_function()->set_async_saved_try_ctx_name(async_saved_try_ctx_name);
 }
 
 
 // Restore the currently relevant :saved_try_context_var on the stack
-// from the captured :async_saved_try_cts_var.
+// from the captured :async_saved_try_ctx_var_.
 // * Try blocks: Set the context variable for this try block.
 // * Catch/finally blocks: Set the context variable for any outer try block (if
 //   existent).
 //
 // Also save the captured variable and the stack variable to be able to set
 // it after a function continues execution (await).
 void Parser::RestoreSavedTryContext(LocalScope* saved_try_context_scope,
                                     int16_t try_index,
                                     SequenceNode* target) {
   LocalVariable* saved_try_ctx = saved_try_context_scope->LookupVariable(
@@ skipping 373 matching lines @@
     // Empty statement, nothing to do.
     ConsumeToken();
   } else if (token == Token::kRETHROW) {
     // Rethrow of current exception.
     ConsumeToken();
     ExpectSemicolon();
     // Check if it is ok to do a rethrow.
     if ((try_blocks_list_ == NULL) || !try_blocks_list_->inside_catch()) {
       ReportError(statement_pos, "rethrow of an exception is not valid here");
     }
-    // The exception and stack trace variables are bound in the block
-    // containing the try.
-    LocalScope* scope = try_blocks_list_->try_block()->scope->parent();
-    ASSERT(scope != NULL);
-    LocalVariable* excp_var =
-        scope->LocalLookupVariable(Symbols::ExceptionVar());
+
+    // If in async code, use :saved_exception_var and :saved_stack_trace_var
+    // instead of :exception_var and :stack_trace_var.
+    LocalVariable* excp_var;
+    LocalVariable* trace_var;
+    if (innermost_function().IsAsyncClosure() ||
+        innermost_function().IsAsyncFunction() ||
+        innermost_function().IsSyncGenClosure() ||
+        innermost_function().IsSyncGenerator()) {
+      // The saved exception and stack trace variables are bound in the block
+      // containing the catch.
+      LocalScope* scope = current_block_->scope;
+      excp_var = scope->LookupVariable(Symbols::SavedExceptionVar(), false);

[Ivan Posva, 2015/02/24 20:52:39]

Why is this not LocalLookupVariable?

[hausner, 2015/02/24 20:57:31]

I think Ivan is right, this should only look up in the innermost scope.
Otherwise you might find the saved exception var of an outer try-catch. Compare
to the else branch below.

[regis, 2015/02/24 22:05:13]

As explained by the comments (obviously not clearly enough), the saved exception
var and saved stack trace var are looked for in the catch scope, not in the try
scope as for the vars of the else part. The try scope is available, but not the
catch scope. It's why I have to start from the current scope. There is no danger
to find these vars in an outer scope, since they are declared unconditionally.
I have updated the comments.

+      trace_var = scope->LookupVariable(Symbols::SavedStackTraceVar(), false);
+    } else {
+      // The exception and stack trace variables are bound in the block
+      // containing the try.
+      LocalScope* scope = try_blocks_list_->try_block()->scope->parent();
+      ASSERT(scope != NULL);
+      excp_var = scope->LocalLookupVariable(Symbols::ExceptionVar());
+      trace_var = scope->LocalLookupVariable(Symbols::StackTraceVar());
+    }
     ASSERT(excp_var != NULL);
-    LocalVariable* trace_var =
-        scope->LocalLookupVariable(Symbols::StackTraceVar());
     ASSERT(trace_var != NULL);
+
     statement = new(Z) ThrowNode(
         statement_pos,
         new(Z) LoadLocalNode(statement_pos, excp_var),
         new(Z) LoadLocalNode(statement_pos, trace_var));
   } else {
     statement = ParseAwaitableExpr(kAllowConst, kConsumeCascades, NULL);
     ExpectSemicolon();
   }
   return statement;
 }
@@ skipping 3614 matching lines @@
 void Parser::SkipQualIdent() {
   ASSERT(IsIdentifier());
   ConsumeToken();
   if (CurrentToken() == Token::kPERIOD) {
     ConsumeToken();  // Consume the kPERIOD token.
     ExpectIdentifier("identifier expected after '.'");
   }
 }
 
 }  // namespace dart