Fix completion of try..finally.

R=rossberg BUG=v8:2529 LOG=Y Review URL: https://codereview.chromium.org/1375203004 Cr-Commit-Position: refs/heads/master@{#31051}
2015-10-01 06:59:36 -07:00 · 2015-10-01 06:59:36 -07:00 · cf82eea6d7
commit cf82eea6d7
parent 90998947bc
7 changed files with 125 additions and 49 deletions
--- a/src/ast.h
+++ b/src/ast.h
@ -462,6 +462,10 @@ class Block final : public BreakableStatement {
    statements_.Add(statement, zone);
  }

+  void InsertStatementAt(int index, Statement* statement, Zone* zone) {
+    statements_.InsertAt(index, statement, zone);
+  }
+
  ZoneList<Statement*>* statements() { return &statements_; }
  bool ignore_completion_value() const { return ignore_completion_value_; }

@ -3190,6 +3194,8 @@ class AstNodeFactory final BASE_EMBEDDED {
        parser_zone_(ast_value_factory->zone()),
        ast_value_factory_(ast_value_factory) {}

+  AstValueFactory* ast_value_factory() const { return ast_value_factory_; }
+
  VariableDeclaration* NewVariableDeclaration(
      VariableProxy* proxy, VariableMode mode, Scope* scope, int pos,
      bool is_class_declaration = false, int declaration_group_start = -1) {
--- a/src/parser.cc
+++ b/src/parser.cc
@ -1933,7 +1933,7 @@ Statement* Parser::ParseSubStatement(ZoneList<const AstRawString*>* labels,
            factory()->NewBlock(labels, 1, false, RelocInfo::kNoPosition);
        Target target(&this->target_stack_, result);
        Statement* statement = ParseStatementAsUnlabelled(labels, CHECK_OK);
-        if (result) result->AddStatement(statement, zone());
+        if (result) result->statements()->Add(statement, zone());
        return result;
      }
    }
@ -2361,7 +2361,7 @@ Block* Parser::ParseBlock(ZoneList<const AstRawString*>* labels, bool* ok) {
  while (peek() != Token::RBRACE) {
    Statement* stat = ParseStatement(NULL, CHECK_OK);
    if (stat && !stat->IsEmpty()) {
-      result->AddStatement(stat, zone());
+      result->statements()->Add(stat, zone());
    }
  }
  Expect(Token::RBRACE, CHECK_OK);
@ -2390,7 +2390,7 @@ Block* Parser::ParseScopedBlock(ZoneList<const AstRawString*>* labels,
    while (peek() != Token::RBRACE) {
      Statement* stat = ParseStatementListItem(CHECK_OK);
      if (stat && !stat->IsEmpty()) {
-        body->AddStatement(stat, zone());
+        body->statements()->Add(stat, zone());
      }
    }
  }
@ -3029,12 +3029,12 @@ Statement* Parser::ParseSwitchStatement(ZoneList<const AstRawString*>* labels,
      tag->position());
  Statement* tag_statement =
      factory()->NewExpressionStatement(tag_assign, RelocInfo::kNoPosition);
-  switch_block->AddStatement(tag_statement, zone());
+  switch_block->statements()->Add(tag_statement, zone());

  // make statement: undefined;
  // This is needed so the tag isn't returned as the value, in case the switch
  // statements don't have a value.
-  switch_block->AddStatement(
+  switch_block->statements()->Add(
      factory()->NewExpressionStatement(
          factory()->NewUndefinedLiteral(RelocInfo::kNoPosition),
          RelocInfo::kNoPosition),
@ -3064,7 +3064,7 @@ Statement* Parser::ParseSwitchStatement(ZoneList<const AstRawString*>* labels,
      cases->Add(clause, zone());
    }
    switch_statement->Initialize(tag_read, cases);
-    cases_block->AddStatement(switch_statement, zone());
+    cases_block->statements()->Add(switch_statement, zone());
  }
  Expect(Token::RBRACE, CHECK_OK);

@ -3072,7 +3072,7 @@ Statement* Parser::ParseSwitchStatement(ZoneList<const AstRawString*>* labels,
  cases_scope = cases_scope->FinalizeBlockScope();
  cases_block->set_scope(cases_scope);

-  switch_block->AddStatement(cases_block, zone());
+  switch_block->statements()->Add(cases_block, zone());

  return switch_block;
 }
@ -3163,7 +3163,7 @@ TryStatement* Parser::ParseTryStatement(bool* ok) {
        factory()->NewTryCatchStatement(try_block, catch_scope, catch_variable,
                                        catch_block, RelocInfo::kNoPosition);
    try_block = factory()->NewBlock(NULL, 1, false, RelocInfo::kNoPosition);
-    try_block->AddStatement(statement, zone());
+    try_block->statements()->Add(statement, zone());
    catch_block = NULL;  // Clear to indicate it's been handled.
  }

@ -3384,7 +3384,7 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
                                           RelocInfo::kNoPosition);

  // Add statement: let/const x = i.
-  outer_block->AddStatement(init, zone());
+  outer_block->statements()->Add(init, zone());

  const AstRawString* temp_name = ast_value_factory()->dot_for_string();

@ -3398,7 +3398,7 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
        Token::ASSIGN, temp_proxy, proxy, RelocInfo::kNoPosition);
    Statement* assignment_statement = factory()->NewExpressionStatement(
        assignment, RelocInfo::kNoPosition);
-    outer_block->AddStatement(assignment_statement, zone());
+    outer_block->statements()->Add(assignment_statement, zone());
    temps.Add(temp, zone());
  }

@ -3412,11 +3412,11 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
        Token::ASSIGN, first_proxy, const1, RelocInfo::kNoPosition);
    Statement* assignment_statement =
        factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition);
-    outer_block->AddStatement(assignment_statement, zone());
+    outer_block->statements()->Add(assignment_statement, zone());
  }

  // make statement: undefined;
-  outer_block->AddStatement(
+  outer_block->statements()->Add(
      factory()->NewExpressionStatement(
          factory()->NewUndefinedLiteral(RelocInfo::kNoPosition),
          RelocInfo::kNoPosition),
@ -3429,7 +3429,7 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
  // in this function that looks up break targets.
  ForStatement* outer_loop =
      factory()->NewForStatement(NULL, RelocInfo::kNoPosition);
-  outer_block->AddStatement(outer_loop, zone());
+  outer_block->statements()->Add(outer_loop, zone());

  outer_block->set_scope(for_scope);
  scope_ = inner_scope;
@ -3456,7 +3456,7 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
        factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition);
    DCHECK(init->position() != RelocInfo::kNoPosition);
    proxy->var()->set_initializer_position(init->position());
-    ignore_completion_block->AddStatement(assignment_statement, zone());
+    ignore_completion_block->statements()->Add(assignment_statement, zone());
  }

  // Make statement: if (first == 1) { first = 0; } else { next; }
@ -3482,7 +3482,7 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
    }
    Statement* clear_first_or_next = factory()->NewIfStatement(
        compare, clear_first, next, RelocInfo::kNoPosition);
-    ignore_completion_block->AddStatement(clear_first_or_next, zone());
+    ignore_completion_block->statements()->Add(clear_first_or_next, zone());
  }

  Variable* flag = scope_->NewTemporary(temp_name);
@ -3494,9 +3494,9 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
        Token::ASSIGN, flag_proxy, const1, RelocInfo::kNoPosition);
    Statement* assignment_statement =
        factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition);
-    ignore_completion_block->AddStatement(assignment_statement, zone());
+    ignore_completion_block->statements()->Add(assignment_statement, zone());
  }
-  inner_block->AddStatement(ignore_completion_block, zone());
+  inner_block->statements()->Add(ignore_completion_block, zone());
  // Make cond expression for main loop: flag == 1.
  Expression* flag_cond = NULL;
  {
@ -3548,7 +3548,7 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
  // and ensures that any break or continue statements in body point to
  // the right place.
  loop->Initialize(NULL, flag_cond, compound_next_statement, body_or_stop);
-  inner_block->AddStatement(loop, zone());
+  inner_block->statements()->Add(loop, zone());

  // Make statement: if (flag == 1) { break; }
  {
@ -3565,7 +3565,7 @@ Statement* Parser::DesugarLexicalBindingsInForStatement(
    Statement* empty = factory()->NewEmptyStatement(RelocInfo::kNoPosition);
    Statement* if_flag_break =
        factory()->NewIfStatement(compare, stop, empty, RelocInfo::kNoPosition);
-    inner_block->AddStatement(if_flag_break, zone());
+    inner_block->statements()->Add(if_flag_break, zone());
  }

  inner_scope->set_end_position(scanner()->location().end_pos);
@ -3646,7 +3646,7 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
              each_beg_pos, each_end_pos);
          init_block = factory()->NewBlock(
              nullptr, 2, true, parsing_result.descriptor.declaration_pos);
-          init_block->AddStatement(
+          init_block->statements()->Add(
              factory()->NewExpressionStatement(
                  factory()->NewAssignment(
                      Token::ASSIGN, single_var,
@ -3709,8 +3709,8 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
              CHECK_OK);
        }

-        body_block->AddStatement(each_initialization_block, zone());
-        body_block->AddStatement(body, zone());
+        body_block->statements()->Add(each_initialization_block, zone());
+        body_block->statements()->Add(body, zone());
        VariableProxy* temp_proxy =
            factory()->NewVariableProxy(temp, each_beg_pos, each_end_pos);
        InitializeForEachStatement(loop, temp_proxy, enumerable, body_block);
@ -3746,7 +3746,7 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
        for_scope = for_scope->FinalizeBlockScope();
        // Parsed for-in loop w/ variable declarations.
        if (init_block != nullptr) {
-          init_block->AddStatement(loop, zone());
+          init_block->statements()->Add(loop, zone());
          if (for_scope != nullptr) {
            init_block->set_scope(for_scope);
          }
@ -3864,8 +3864,8 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
      DCHECK(init != NULL);
      Block* block =
          factory()->NewBlock(NULL, 2, false, RelocInfo::kNoPosition);
-      block->AddStatement(init, zone());
-      block->AddStatement(loop, zone());
+      block->statements()->Add(init, zone());
+      block->statements()->Add(loop, zone());
      block->set_scope(for_scope);
      loop->Initialize(NULL, cond, next, body);
      result = block;
@ -4544,11 +4544,11 @@ Block* Parser::BuildParameterInitializationBlock(

      loop->Initialize(init, cond, next, body);

-      init_block->AddStatement(
+      init_block->statements()->Add(
          factory()->NewExpressionStatement(init_array, RelocInfo::kNoPosition),
          zone());

-      init_block->AddStatement(loop, zone());
+      init_block->statements()->Add(loop, zone());

      descriptor.initialization_pos = pos;
    }
@ -4579,7 +4579,7 @@ Block* Parser::BuildParameterInitializationBlock(
      if (param_scope != nullptr) {
        CheckConflictingVarDeclarations(param_scope, CHECK_OK);
      }
-      init_block->AddStatement(param_block, zone());
+      init_block->statements()->Add(param_block, zone());
    }
  }
  return init_block;
--- a/src/pattern-rewriter.cc
+++ b/src/pattern-rewriter.cc
@ -171,7 +171,7 @@ void Parser::PatternRewriter::VisitVariableProxy(VariableProxy* pattern) {
    }

    if (initialize != NULL) {
-      block_->AddStatement(
+      block_->statements()->Add(
          factory()->NewExpressionStatement(initialize, RelocInfo::kNoPosition),
          zone());
    }
@ -189,7 +189,7 @@ void Parser::PatternRewriter::VisitVariableProxy(VariableProxy* pattern) {
    DCHECK_NOT_NULL(value);
    Assignment* assignment = factory()->NewAssignment(
        descriptor_->init_op, proxy, value, descriptor_->initialization_pos);
-    block_->AddStatement(
+    block_->statements()->Add(
        factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition),
        zone());
    value = NULL;
@ -205,7 +205,7 @@ void Parser::PatternRewriter::VisitVariableProxy(VariableProxy* pattern) {
    VariableProxy* proxy = initialization_scope->NewUnresolved(factory(), name);
    Assignment* assignment = factory()->NewAssignment(
        descriptor_->init_op, proxy, value, descriptor_->initialization_pos);
-    block_->AddStatement(
+    block_->statements()->Add(
        factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition),
        zone());
  }
@ -220,7 +220,7 @@ Variable* Parser::PatternRewriter::CreateTempVar(Expression* value) {
        Token::ASSIGN, factory()->NewVariableProxy(temp), value,
        RelocInfo::kNoPosition);

-    block_->AddStatement(
+    block_->statements()->Add(
        factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition),
        zone());
  }
@ -231,8 +231,8 @@ Variable* Parser::PatternRewriter::CreateTempVar(Expression* value) {
 void Parser::PatternRewriter::VisitObjectLiteral(ObjectLiteral* pattern) {
  auto temp = CreateTempVar(current_value_);

-  block_->AddStatement(descriptor_->parser->BuildAssertIsCoercible(temp),
-                       zone());
+  block_->statements()->Add(descriptor_->parser->BuildAssertIsCoercible(temp),
+                            zone());

  for (ObjectLiteralProperty* property : *pattern->properties()) {
    RecurseIntoSubpattern(
@ -264,12 +264,13 @@ void Parser::PatternRewriter::VisitArrayLiteral(ArrayLiteral* node) {
    // }
    auto next_block =
        factory()->NewBlock(nullptr, 2, true, RelocInfo::kNoPosition);
-    next_block->AddStatement(factory()->NewExpressionStatement(
-                                 descriptor_->parser->BuildIteratorNextResult(
-                                     factory()->NewVariableProxy(iterator),
-                                     result, RelocInfo::kNoPosition),
-                                 RelocInfo::kNoPosition),
-                             zone());
+    next_block->statements()->Add(
+        factory()->NewExpressionStatement(
+            descriptor_->parser->BuildIteratorNextResult(
+                factory()->NewVariableProxy(iterator), result,
+                RelocInfo::kNoPosition),
+            RelocInfo::kNoPosition),
+        zone());

    auto assign_to_done = factory()->NewAssignment(
        Token::ASSIGN, factory()->NewVariableProxy(done),
@ -287,7 +288,7 @@ void Parser::PatternRewriter::VisitArrayLiteral(ArrayLiteral* node) {
                                        RelocInfo::kNoPosition),
            RelocInfo::kNoPosition),
        RelocInfo::kNoPosition);
-    next_block->AddStatement(
+    next_block->statements()->Add(
        factory()->NewExpressionStatement(
            factory()->NewAssignment(Token::ASSIGN,
                                     factory()->NewVariableProxy(v), next_value,
@ -301,7 +302,7 @@ void Parser::PatternRewriter::VisitArrayLiteral(ArrayLiteral* node) {
                                     RelocInfo::kNoPosition),
        next_block, factory()->NewEmptyStatement(RelocInfo::kNoPosition),
        RelocInfo::kNoPosition);
-    block_->AddStatement(if_statement, zone());
+    block_->statements()->Add(if_statement, zone());

    if (!(value->IsLiteral() && value->AsLiteral()->raw_value()->IsTheHole())) {
      RecurseIntoSubpattern(value, factory()->NewVariableProxy(v));
@ -334,8 +335,7 @@ void Parser::PatternRewriter::VisitArrayLiteral(ArrayLiteral* node) {
                                          RelocInfo::kNoPosition),
        factory()->NewEmptyStatement(RelocInfo::kNoPosition),
        RelocInfo::kNoPosition);
-    block_->AddStatement(if_statement, zone());
-
+    block_->statements()->Add(if_statement, zone());

    RecurseIntoSubpattern(spread->expression(),
                          factory()->NewVariableProxy(array));
--- a/src/rewriter.cc
+++ b/src/rewriter.cc
@ -13,12 +13,13 @@ namespace internal {

 class Processor: public AstVisitor {
 public:
-  Processor(Isolate* isolate, Variable* result,
+  Processor(Isolate* isolate, Scope* scope, Variable* result,
            AstValueFactory* ast_value_factory)
      : result_(result),
        result_assigned_(false),
        replacement_(nullptr),
        is_set_(false),
+        scope_(scope),
        factory_(ast_value_factory) {
    InitializeAstVisitor(isolate, ast_value_factory->zone());
  }
@ -28,6 +29,7 @@ class Processor: public AstVisitor {
  void Process(ZoneList<Statement*>* statements);
  bool result_assigned() const { return result_assigned_; }

+  Scope* scope() { return scope_; }
  AstNodeFactory* factory() { return &factory_; }

 private:
@ -49,8 +51,10 @@ class Processor: public AstVisitor {
  // was hoping for.
  bool is_set_;

+  Scope* scope_;
  AstNodeFactory factory_;

+  // Returns ".result = value"
  Expression* SetResult(Expression* value) {
    result_assigned_ = true;
    VariableProxy* result_proxy = factory()->NewVariableProxy(result_);
@ -167,9 +171,31 @@ void Processor::VisitTryCatchStatement(TryCatchStatement* node) {

 void Processor::VisitTryFinallyStatement(TryFinallyStatement* node) {
  // Rewrite both try and finally block (in reverse order).
+  bool set_after = is_set_;
+  is_set_ = true;  // Don't normally need to assign in finally block.
  Visit(node->finally_block());
  node->set_finally_block(replacement_->AsBlock());
-  Visit(node->try_block());  // Exception will not be caught.
+  {  // Save .result value at the beginning of the finally block and restore it
+     // at the end again: ".backup = .result; ...; .result = .backup"
+     // This is necessary because the finally block does not normally contribute
+     // to the completion value.
+    Variable* backup = scope()->NewTemporary(
+        factory()->ast_value_factory()->dot_result_string());
+    Expression* backup_proxy = factory()->NewVariableProxy(backup);
+    Expression* result_proxy = factory()->NewVariableProxy(result_);
+    Expression* save = factory()->NewAssignment(
+        Token::ASSIGN, backup_proxy, result_proxy, RelocInfo::kNoPosition);
+    Expression* restore = factory()->NewAssignment(
+        Token::ASSIGN, result_proxy, backup_proxy, RelocInfo::kNoPosition);
+    node->finally_block()->statements()->InsertAt(
+        0, factory()->NewExpressionStatement(save, RelocInfo::kNoPosition),
+        zone());
+    node->finally_block()->statements()->Add(
+        factory()->NewExpressionStatement(restore, RelocInfo::kNoPosition),
+        zone());
+  }
+  is_set_ = set_after;
+  Visit(node->try_block());
  node->set_try_block(replacement_->AsBlock());
  replacement_ = node;
 }
@ -260,7 +286,8 @@ bool Rewriter::Rewrite(ParseInfo* info) {
        scope->NewTemporary(info->ast_value_factory()->dot_result_string());
    // The name string must be internalized at this point.
    DCHECK(!result->name().is_null());
-    Processor processor(info->isolate(), result, info->ast_value_factory());
+    Processor processor(info->isolate(), scope, result,
+                        info->ast_value_factory());
    processor.Process(body);
    if (processor.HasStackOverflow()) return false;

--- a/test/mjsunit/regress/regress-2529.js
+++ b/test/mjsunit/regress/regress-2529.js
@ -0,0 +1,43 @@
+// Copyright 2015 the V8 project authors. All rights reserved.
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+//       notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+//       copyright notice, this list of conditions and the following
+//       disclaimer in the documentation and/or other materials provided
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+
+// Regression test for v8 bug 2529.
+
+function makeScript(s) {
+  return 'while(true) { try { "try"; break } finally { "finally" }; ' + s + ' }';
+}
+
+var s1 = makeScript('');
+var s2 = makeScript('y = "done"');
+var s3 = makeScript('if (true) 2; else var x = 3;');
+var s4 = makeScript('if (true) 2; else 3;');
+
+assertEquals("try", eval(s1));
+assertEquals("try", eval(s2));
+assertEquals("try", eval(s3));
+assertEquals("try", eval(s4));
--- a/test/webkit/eval-throw-return-expected.txt
+++ b/test/webkit/eval-throw-return-expected.txt
@ -31,7 +31,7 @@ PASS eval("1; try { foo = [2,3,throwFunc(), 4]; } catch (e){}") is 1
 PASS eval("1; try { 2; throw \"\"; } catch (e){}") is 1
 PASS eval("1; try { 2; throwFunc(); } catch (e){}") is 1
 PASS eval("1; try { 2; throwFunc(); } catch (e){3;} finally {}") is 3
-PASS eval("1; try { 2; throwFunc(); } catch (e){3;} finally {4;}") is 4
+PASS eval("1; try { 2; throwFunc(); } catch (e){3;} finally {4;}") is 3
 PASS eval("function blah() { 1; }\n blah();") is undefined
 PASS eval("var x = 1;") is undefined
 PASS eval("if (true) { 1; } else { 2; }") is 1
--- a/test/webkit/eval-throw-return.js
+++ b/test/webkit/eval-throw-return.js
@ -41,7 +41,7 @@ shouldBe('eval("1; try { foo = [2,3,throwFunc(), 4]; } catch (e){}")', "1");
 shouldBe('eval("1; try { 2; throw \\"\\"; } catch (e){}")', "1");
 shouldBe('eval("1; try { 2; throwFunc(); } catch (e){}")', "1");
 shouldBe('eval("1; try { 2; throwFunc(); } catch (e){3;} finally {}")', "3");
-shouldBe('eval("1; try { 2; throwFunc(); } catch (e){3;} finally {4;}")', "4");
+shouldBe('eval("1; try { 2; throwFunc(); } catch (e){3;} finally {4;}")', "3");
 shouldBe('eval("function blah() { 1; }\\n blah();")', "undefined");
 shouldBe('eval("var x = 1;")', "undefined");
 shouldBe('eval("if (true) { 1; } else { 2; }")', "1");