Fix VM bug with try-catch inside of try-finally.

There were two exception handlers with the same index added to the code so that only the one added
last was executed. In case of an exception that means that a re-throw may be omitted, causing invalid
control flow.

Also, add assertion to ensure unique try-index for exception handlers.

BUG=#25333
R=hausner@google.com

Review URL: https://codereview.chromium.org/1569523002 .

runtime/vm/ast.h

@@ -1921,13 +1921,15 @@ class TryCatchNode : public AstNode {
                const LocalVariable* context_var,
                CatchClauseNode* catch_block,
                SequenceNode* finally_block,
-               intptr_t try_index)
+               intptr_t try_index,
+               SequenceNode* rethrow_clause)
       : AstNode(token_pos),
         try_block_(try_block),
         context_var_(*context_var),
         catch_block_(catch_block),
         finally_block_(finally_block),
-        try_index_(try_index) {
+        try_index_(try_index),
+        rethrow_clause_(rethrow_clause) {
     ASSERT(try_block_ != NULL);
     ASSERT(context_var != NULL);
     ASSERT(catch_block_ != NULL);
@@ -1939,6 +1941,8 @@ class TryCatchNode : public AstNode {
   const LocalVariable& context_var() const { return context_var_; }
   intptr_t try_index() const { return try_index_; }
 
+  SequenceNode* rethrow_clause() const { return rethrow_clause_; }
+
   virtual void VisitChildren(AstNodeVisitor* visitor) const {
     try_block_->Visit(visitor);
     if (catch_block_ != NULL) {
@@ -1957,6 +1961,7 @@ class TryCatchNode : public AstNode {
   CatchClauseNode* catch_block_;
   SequenceNode* finally_block_;
   const intptr_t try_index_;
+  SequenceNode* rethrow_clause_;
 
   DISALLOW_COPY_AND_ASSIGN(TryCatchNode);
 };

runtime/vm/code_descriptors.h

@@ -102,6 +102,7 @@ class ExceptionHandlerList : public ZoneAllocated {
       AddPlaceHolder();
     }
     list_[try_index].outer_try_index = outer_try_index;
+    ASSERT(list_[try_index].pc_offset == ExceptionHandlers::kInvalidPcOffset);
     list_[try_index].pc_offset = pc_offset;
     ASSERT(handler_types.IsZoneHandle());
     list_[try_index].handler_types = &handler_types;

runtime/vm/flow_graph_builder.cc

@@ -4220,7 +4220,7 @@ void EffectGraphVisitor::VisitCatchClauseNode(CatchClauseNode* node) {
 
 void EffectGraphVisitor::VisitTryCatchNode(TryCatchNode* node) {
   InlineBailout("EffectGraphVisitor::VisitTryCatchNode (exception)");
-  intptr_t original_handler_index = owner()->try_index();
+  const intptr_t original_handler_index = owner()->try_index();
   const intptr_t try_handler_index = node->try_index();
   ASSERT(try_handler_index != original_handler_index);
   owner()->set_try_index(try_handler_index);
@@ -4292,13 +4292,14 @@ void EffectGraphVisitor::VisitTryCatchNode(TryCatchNode* node) {
   }
 
   if (finally_block != NULL) {
+    ASSERT(node->rethrow_clause() != NULL);
     // Create a handler for the code in the catch block, containing the
     // code in the finally block.
     owner()->set_try_index(original_handler_index);
     EffectGraphVisitor for_finally(owner());
     for_finally.BuildRestoreContext(catch_block->context_var());
 
-    finally_block->Visit(&for_finally);
+    node->rethrow_clause()->Visit(&for_finally);
     if (for_finally.is_open()) {
       // Rethrow the exception.  Manually build the graph for rethrow.
       Value* exception = for_finally.Bind(

runtime/vm/flow_graph_compiler.cc

@@ -273,7 +273,7 @@ void FlowGraphCompiler::InitCompiler() {
                             isolate()->GetCompilerStream(),
                             "InitCompiler");
   pc_descriptors_list_ = new(zone()) DescriptorList(64);
-  exception_handlers_list_ = new(zone())ExceptionHandlerList();
+  exception_handlers_list_ = new(zone()) ExceptionHandlerList();
   block_info_.Clear();
   // Conservative detection of leaf routines used to remove the stack check
   // on function entry.

runtime/vm/parser.cc

@@ -6465,7 +6465,8 @@ SequenceNode* Parser::CloseAsyncGeneratorTryBlock(SequenceNode *body) {
                           context_var,
                           catch_clause,
                           finally_clause,
-                          try_index);
+                          try_index,
+                          finally_clause);
   current_block_->statements->Add(try_catch_node);
   return CloseBlock();
 }
@@ -6581,7 +6582,8 @@ SequenceNode* Parser::CloseAsyncTryBlock(SequenceNode* try_block) {
       context_var,
       catch_clause,
       NULL,  // No finally clause.
-      try_index);
+      try_index,
+      NULL);  // No rethrow-finally clause.
   current_block_->statements->Add(try_catch_node);
   return CloseBlock();
 }
@@ -8936,7 +8938,8 @@ AstNode* Parser::ParseAwaitForStatement(String* label_name) {
                          context_var,
                          catch_clause,
                          finally_clause,
-                         try_index);
+                         try_index,
+                         finally_clause);
 
   ASSERT(current_block_ == loop_block);
   loop_block->statements->Add(try_catch_node);
@@ -9762,6 +9765,7 @@ AstNode* Parser::ParseTryStatement(String* label_name) {
   // of an existing outer try. Generate a finally clause to this purpose if it
   // is not declared.
   SequenceNode* finally_clause = NULL;
+  SequenceNode* rethrow_clause = NULL;
   const bool parse = CurrentToken() == Token::kFINALLY;
   if (parse || (is_async && (try_stack_ != NULL))) {
     if (parse) {
@@ -9795,6 +9799,21 @@ AstNode* Parser::ParseTryStatement(String* label_name) {
         stack_trace_var,
         is_async ? saved_exception_var : exception_var,
         is_async ? saved_stack_trace_var : stack_trace_var);
+    if (finally_clause != NULL) {
+      // Re-parse to create a duplicate of finally clause to avoid unintended
+      // sharing of try-indices if the finally-block contains a try-catch.
+      // The flow graph builder emits two copies of the finally-block if the
+      // try-block has a normal exit: one for the exception- and one for the
+      // non-exception case (see EffectGraphVisitor::VisitTryCatchNode)
+      tokens_iterator_.SetCurrentPosition(finally_pos);
+      rethrow_clause = EnsureFinallyClause(
+          parse,
+          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(
@@ -9814,7 +9833,8 @@ AstNode* Parser::ParseTryStatement(String* label_name) {
   // on the try/catch, close the block that's embedding the try statement
   // and attach the label to it.
   AstNode* try_catch_node = new(Z) TryCatchNode(
-      try_pos, try_block, context_var, catch_clause, finally_clause, try_index);
+      try_pos, try_block, context_var, catch_clause, finally_clause, try_index,
+      rethrow_clause);
 
   if (try_label != NULL) {
     current_block_->statements->Add(try_catch_node);

tests/language/try_finally_regress_25333_test.dart

@@ -0,0 +1,65 @@
+// Copyright (c) 2016, 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.
+
+// Test correct handling of try-catch inside try-finally.
+
+import "package:expect/expect.dart";
+
+void main() {
+  print("== test1 ==");
+  bool caught = false;
+  try {
+    test1();
+    print("Unexpected 1"); // should never go here
+    Expect.isTrue(false);
+  } catch (e) {
+    caught = true;
+    print("main catch 1: $e");
+    Expect.equals(e, "Ball");
+  }
+  Expect.isTrue(caught);
+  print("== test2 ==");
+  caught = false;
+  try {
+    test2();
+    print("Unexpected 2"); // should never go here
+    Expect.isTrue(false);
+  } catch (e) {
+    caught = true;
+    print("main catch 2: $e");
+    Expect.equals(e, "Ball");
+  }
+  Expect.isTrue(caught);
+}
+
+void test1() {
+  try {
+    throw "Ball";
+  } finally {
+    try {
+      throw "Frisbee";
+    } catch(e) {
+      print("test 1 catch: $e");
+      Expect.equals(e, "Frisbee");
+    }
+  }
+}
+
+void test2() {
+  try {
+    throwError(); // call a method that throws an error
+  } finally {
+    try {
+      throw "Frisbee";
+    } catch(e) {
+      print("test 2 catch: $e");
+      Expect.equals(e, "Frisbee");
+    }
+  }
+}
+
+
+void throwError() {
+  throw "Ball";
+}