Deoptimize to the proper target after assignment side effects.

This fixes V8 issue 989.

Before, assignments used the AST ID of the assignment expression to
mark the side effect of the store, which became a target for
deoptimization bailout for code after the assignment.  In effect
contexts this environment included the value of the assignment, which
was unexpected by the unoptimized code.

Now we introduce a new assignment ID for AST node types that include
an assignment (Assignment, CountOperation, and ForInStatement) and use
it for the side effect of the store.

Review URL: http://codereview.chromium.org/5682010

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@5990 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/ast-inl.h

@@ -94,7 +94,8 @@ ForStatement::ForStatement(ZoneStringList* labels)
 
 
 ForInStatement::ForInStatement(ZoneStringList* labels)
-    : IterationStatement(labels), each_(NULL), enumerable_(NULL) {
+    : IterationStatement(labels), each_(NULL), enumerable_(NULL),
+      assignment_id_(GetNextId()) {
 }
 
 

src/ast.cc

@@ -125,17 +125,18 @@ Assignment::Assignment(Token::Value op,
       target_(target),
       value_(value),
       pos_(pos),
-      compound_bailout_id_(kNoNumber),
+      binary_operation_(NULL),
+      compound_load_id_(kNoNumber),
+      assignment_id_(GetNextId()),
       block_start_(false),
       block_end_(false),
       is_monomorphic_(false),
       receiver_types_(NULL) {
   ASSERT(Token::IsAssignmentOp(op));
-  binary_operation_ = is_compound()
-      ? new BinaryOperation(binary_op(), target, value, pos + 1)
-      : NULL;
   if (is_compound()) {
-    compound_bailout_id_ = GetNextId();
+    binary_operation_ =
+        new BinaryOperation(binary_op(), target, value, pos + 1);
+    compound_load_id_ = GetNextId();
   }
 }
 

src/ast.h

@@ -575,11 +575,13 @@ class ForInStatement: public IterationStatement {
   Expression* enumerable() const { return enumerable_; }
 
   // Bailout support.
+  int AssignmentId() const { return assignment_id_; }
   virtual int ContinueId() const { return EntryId(); }
 
  private:
   Expression* each_;
   Expression* enumerable_;
+  int assignment_id_;
 };
 
 
@@ -1426,7 +1428,9 @@ class IncrementOperation: public Expression {
 class CountOperation: public Expression {
  public:
   CountOperation(bool is_prefix, IncrementOperation* increment, int pos)
-      : is_prefix_(is_prefix), increment_(increment), pos_(pos) { }
+      : is_prefix_(is_prefix), increment_(increment), pos_(pos),
+        assignment_id_(GetNextId()) {
+  }
 
   DECLARE_NODE_TYPE(CountOperation)
 
@@ -1446,10 +1450,14 @@ class CountOperation: public Expression {
 
   virtual bool IsInlineable() const;
 
+  // Bailout support.
+  int AssignmentId() const { return assignment_id_; }
+
  private:
   bool is_prefix_;
   IncrementOperation* increment_;
   int pos_;
+  int assignment_id_;
 };
 
 
@@ -1579,7 +1587,8 @@ class Assignment: public Expression {
   }
 
   // Bailout support.
-  int compound_bailout_id() const { return compound_bailout_id_; }
+  int CompoundLoadId() const { return compound_load_id_; }
+  int AssignmentId() const { return assignment_id_; }
 
  private:
   Token::Value op_;
@@ -1587,7 +1596,8 @@ class Assignment: public Expression {
   Expression* value_;
   int pos_;
   BinaryOperation* binary_operation_;
-  int compound_bailout_id_;
+  int compound_load_id_;
+  int assignment_id_;
 
   bool block_start_;
   bool block_end_;

src/full-codegen.h

@@ -481,7 +481,7 @@ class FullCodeGenerator: public AstVisitor {
 
   // Assign to the given expression as if via '='. The right-hand-side value
   // is expected in the accumulator.
-  void EmitAssignment(Expression* expr);
+  void EmitAssignment(Expression* expr, int bailout_ast_id);
 
   // Complete a variable assignment.  The right-hand-side value is expected
   // in the accumulator.

src/hydrogen.cc

@@ -3299,7 +3299,7 @@ void HGraphBuilder::HandlePropertyAssignment(Assignment* expr) {
   Push(value);
   instr->set_position(expr->position());
   AddInstruction(instr);
-  if (instr->HasSideEffects()) AddSimulate(expr->id());
+  if (instr->HasSideEffects()) AddSimulate(expr->AssignmentId());
   ast_context()->ReturnValue(Pop());
 }
 
@@ -3344,7 +3344,10 @@ void HGraphBuilder::HandleCompoundAssignment(Assignment* expr) {
     VISIT_FOR_VALUE(operation);
 
     if (var->is_global()) {
-      HandleGlobalVariableAssignment(var, Top(), expr->position(), expr->id());
+      HandleGlobalVariableAssignment(var,
+                                     Top(),
+                                     expr->position(),
+                                     expr->AssignmentId());
     } else {
       Bind(var, Top());
     }
@@ -3367,7 +3370,7 @@ void HGraphBuilder::HandleCompoundAssignment(Assignment* expr) {
         load = BuildLoadNamedGeneric(obj, prop);
       }
       PushAndAdd(load);
-      if (load->HasSideEffects()) AddSimulate(expr->compound_bailout_id());
+      if (load->HasSideEffects()) AddSimulate(expr->CompoundLoadId());
 
       VISIT_FOR_VALUE(expr->value());
       HValue* right = Pop();
@@ -3379,11 +3382,11 @@ void HGraphBuilder::HandleCompoundAssignment(Assignment* expr) {
 
       HInstruction* store = BuildStoreNamed(obj, instr, prop);
       AddInstruction(store);
-      if (store->HasSideEffects()) AddSimulate(expr->id());
-
       // Drop the simulated receiver and value.  Return the value.
       Drop(2);
-      ast_context()->ReturnValue(instr);
+      Push(instr);
+      if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
+      ast_context()->ReturnValue(Pop());
 
     } else {
       // Keyed property.
@@ -3399,7 +3402,7 @@ void HGraphBuilder::HandleCompoundAssignment(Assignment* expr) {
           ? BuildLoadKeyedFastElement(obj, key, prop)
           : BuildLoadKeyedGeneric(obj, key);
       PushAndAdd(load);
-      if (load->HasSideEffects()) AddSimulate(expr->compound_bailout_id());
+      if (load->HasSideEffects()) AddSimulate(expr->CompoundLoadId());
 
       VISIT_FOR_VALUE(expr->value());
       HValue* right = Pop();
@@ -3413,11 +3416,11 @@ void HGraphBuilder::HandleCompoundAssignment(Assignment* expr) {
           ? BuildStoreKeyedFastElement(obj, key, instr, prop)
           : BuildStoreKeyedGeneric(obj, key, instr);
       AddInstruction(store);
-      if (store->HasSideEffects()) AddSimulate(expr->id());
-
       // Drop the simulated receiver, key, and value.  Return the value.
       Drop(3);
-      ast_context()->ReturnValue(instr);
+      Push(instr);
+      if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
+      ast_context()->ReturnValue(Pop());
     }
 
   } else {
@@ -3443,7 +3446,10 @@ void HGraphBuilder::VisitAssignment(Assignment* expr) {
     // Handle the assignment.
     if (var->is_global()) {
       VISIT_FOR_VALUE(expr->value());
-      HandleGlobalVariableAssignment(var, Top(), expr->position(), expr->id());
+      HandleGlobalVariableAssignment(var,
+                                     Top(),
+                                     expr->position(),
+                                     expr->AssignmentId());
     } else {
       // We allow reference to the arguments object only in assignemtns
       // to local variables to make sure that the arguments object does
@@ -4537,7 +4543,10 @@ void HGraphBuilder::VisitCountOperation(CountOperation* expr) {
     }
 
     if (var->is_global()) {
-      HandleGlobalVariableAssignment(var, instr, expr->position(), expr->id());
+      HandleGlobalVariableAssignment(var,
+                                     instr,
+                                     expr->position(),
+                                     expr->AssignmentId());
     } else {
       ASSERT(var->IsStackAllocated());
       Bind(var, instr);
@@ -4552,9 +4561,8 @@ void HGraphBuilder::VisitCountOperation(CountOperation* expr) {
 
       // Match the full code generator stack by simulating an extra stack
       // element for postfix operations in a value context.
-      if (expr->is_postfix() && !ast_context()->IsEffect()) {
-        Push(graph_->GetConstantUndefined());
-      }
+      bool has_extra = expr->is_postfix() && !ast_context()->IsEffect();
+      if (has_extra) Push(graph_->GetConstantUndefined());
 
       VISIT_FOR_VALUE(prop->obj());
       HValue* obj = Top();
@@ -4570,40 +4578,35 @@ void HGraphBuilder::VisitCountOperation(CountOperation* expr) {
       PushAndAdd(load);
       if (load->HasSideEffects()) AddSimulate(increment->id());
 
-      HValue* value = Pop();
-
+      HValue* before = Pop();
       // There is no deoptimization to after the increment, so we don't need
       // to simulate the expression stack after this instruction.
-      HInstruction* instr = BuildIncrement(value, inc);
-      AddInstruction(instr);
+      HInstruction* after = BuildIncrement(before, inc);
+      AddInstruction(after);
 
-      HInstruction* store = BuildStoreNamed(obj, instr, prop);
+      HInstruction* store = BuildStoreNamed(obj, after, prop);
       AddInstruction(store);
 
-      // Drop simulated receiver and push the result.
-      Drop(1);
-      if (expr->is_prefix()) {
-        Push(instr);
-      } else {
-        if (!ast_context()->IsEffect()) Drop(1);  // Drop simulated zero.
-        Push(value);
-      }
+      // Overwrite the receiver in the bailout environment with the result
+      // of the operation, and the placeholder with the original value if
+      // necessary.
+      environment()->SetExpressionStackAt(0, after);
+      if (has_extra) environment()->SetExpressionStackAt(1, before);
+      if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
+      Drop(has_extra ? 2 : 1);
 
-      if (store->HasSideEffects()) AddSimulate(expr->id());
-      ast_context()->ReturnValue(Pop());
+      ast_context()->ReturnValue(expr->is_postfix() ? before : after);
 
     } else {
       // Keyed property.
 
       // Match the full code generator stack by simulate an extra stack element
       // for postfix operations in a value context.
-      if (expr->is_postfix() && !ast_context()->IsEffect()) {
-        Push(graph_->GetConstantUndefined());
-      }
+      bool has_extra = expr->is_postfix() && !ast_context()->IsEffect();
+      if (has_extra) Push(graph_->GetConstantUndefined());
 
       VISIT_FOR_VALUE(prop->obj());
       VISIT_FOR_VALUE(prop->key());
-
       HValue* obj = environment()->ExpressionStackAt(1);
       HValue* key = environment()->ExpressionStackAt(0);
 
@@ -4616,29 +4619,27 @@ void HGraphBuilder::VisitCountOperation(CountOperation* expr) {
       PushAndAdd(load);
       if (load->HasSideEffects()) AddSimulate(increment->id());
 
-      HValue* value = Pop();
-
+      HValue* before = Pop();
       // There is no deoptimization to after the increment, so we don't need
       // to simulate the expression stack after this instruction.
-      HInstruction* instr = BuildIncrement(value, inc);
-      AddInstruction(instr);
+      HInstruction* after = BuildIncrement(before, inc);
+      AddInstruction(after);
 
       HInstruction* store = is_fast_elements
-          ? BuildStoreKeyedFastElement(obj, key, instr, prop)
-          : new HStoreKeyedGeneric(obj, key, instr);
+          ? BuildStoreKeyedFastElement(obj, key, after, prop)
+          : new HStoreKeyedGeneric(obj, key, after);
       AddInstruction(store);
 
-      // Drop simulated receiver and key and push the result.
-      Drop(2);
-      if (expr->is_prefix()) {
-        Push(instr);
-      } else {
-        if (!ast_context()->IsEffect()) Drop(1);  // Drop simulated zero.
-        Push(value);
-      }
+      // Drop the key from the bailout environment.  Overwrite the receiver
+      // with the result of the operation, and the placeholder with the
+      // original value if necessary.
+      Drop(1);
+      environment()->SetExpressionStackAt(0, after);
+      if (has_extra) environment()->SetExpressionStackAt(1, before);
+      if (store->HasSideEffects()) AddSimulate(expr->AssignmentId());
+      Drop(has_extra ? 2 : 1);
 
-      if (store->HasSideEffects()) AddSimulate(expr->id());
-      ast_context()->ReturnValue(Pop());
+      ast_context()->ReturnValue(expr->is_postfix() ? before : after);
     }
 
   } else {

src/ia32/full-codegen-ia32.cc

@@ -911,7 +911,9 @@ void FullCodeGenerator::VisitForInStatement(ForInStatement* stmt) {
   __ bind(&update_each);
   __ mov(result_register(), ebx);
   // Perform the assignment as if via '='.
-  EmitAssignment(stmt->each());
+  { EffectContext context(this);
+    EmitAssignment(stmt->each(), stmt->AssignmentId());
+  }
 
   // Generate code for the body of the loop.
   Visit(stmt->body());
@@ -1478,7 +1480,7 @@ void FullCodeGenerator::VisitAssignment(Assignment* expr) {
     // For property compound assignments we need another deoptimization
     // point after the property load.
     if (property != NULL) {
-      PrepareForBailoutForId(expr->compound_bailout_id(), TOS_REG);
+      PrepareForBailoutForId(expr->CompoundLoadId(), TOS_REG);
     }
 
     Token::Value op = expr->binary_op();
@@ -1521,6 +1523,8 @@ void FullCodeGenerator::VisitAssignment(Assignment* expr) {
     case VARIABLE:
       EmitVariableAssignment(expr->target()->AsVariableProxy()->var(),
                              expr->op());
+      PrepareForBailoutForId(expr->AssignmentId(), TOS_REG);
+      context()->Plug(eax);
       break;
     case NAMED_PROPERTY:
       EmitNamedPropertyAssignment(expr);
@@ -1849,7 +1853,7 @@ void FullCodeGenerator::EmitBinaryOp(Token::Value op,
 }
 
 
-void FullCodeGenerator::EmitAssignment(Expression* expr) {
+void FullCodeGenerator::EmitAssignment(Expression* expr, int bailout_ast_id) {
   // Invalid left-hand sides are rewritten to have a 'throw
   // ReferenceError' on the left-hand side.
   if (!expr->IsValidLeftHandSide()) {
@@ -1897,6 +1901,8 @@ void FullCodeGenerator::EmitAssignment(Expression* expr) {
       break;
     }
   }
+  PrepareForBailoutForId(bailout_ast_id, TOS_REG);
+  context()->Plug(eax);
 }
 
 
@@ -1969,8 +1975,6 @@ void FullCodeGenerator::EmitVariableAssignment(Variable* var,
     }
     __ bind(&done);
   }
-
-  context()->Plug(eax);
 }
 
 
@@ -2007,10 +2011,10 @@ void FullCodeGenerator::EmitNamedPropertyAssignment(Assignment* expr) {
     __ push(Operand(esp, kPointerSize));  // Receiver is under value.
     __ CallRuntime(Runtime::kToFastProperties, 1);
     __ pop(eax);
-    context()->DropAndPlug(1, eax);
-  } else {
-    context()->Plug(eax);
+    __ Drop(1);
   }
+  PrepareForBailoutForId(expr->AssignmentId(), TOS_REG);
+  context()->Plug(eax);
 }
 
 
@@ -2048,6 +2052,7 @@ void FullCodeGenerator::EmitKeyedPropertyAssignment(Assignment* expr) {
     __ pop(eax);
   }
 
+  PrepareForBailoutForId(expr->AssignmentId(), TOS_REG);
   context()->Plug(eax);
 }
 
@@ -3749,6 +3754,8 @@ void FullCodeGenerator::VisitCountOperation(CountOperation* expr) {
         { EffectContext context(this);
           EmitVariableAssignment(expr->expression()->AsVariableProxy()->var(),
                                  Token::ASSIGN);
+          PrepareForBailoutForId(expr->AssignmentId(), TOS_REG);
+          context.Plug(eax);
         }
         // For all contexts except EffectContext We have the result on
         // top of the stack.
@@ -3759,6 +3766,8 @@ void FullCodeGenerator::VisitCountOperation(CountOperation* expr) {
         // Perform the assignment as if via '='.
         EmitVariableAssignment(expr->expression()->AsVariableProxy()->var(),
                                Token::ASSIGN);
+        PrepareForBailoutForId(expr->AssignmentId(), TOS_REG);
+        context()->Plug(eax);
       }
       break;
     case NAMED_PROPERTY: {
@@ -3766,6 +3775,7 @@ void FullCodeGenerator::VisitCountOperation(CountOperation* expr) {
       __ pop(edx);
       Handle<Code> ic(Builtins::builtin(Builtins::StoreIC_Initialize));
       EmitCallIC(ic, RelocInfo::CODE_TARGET);
+      PrepareForBailoutForId(expr->AssignmentId(), TOS_REG);
       if (expr->is_postfix()) {
         if (!context()->IsEffect()) {
           context()->PlugTOS();
@@ -3780,6 +3790,7 @@ void FullCodeGenerator::VisitCountOperation(CountOperation* expr) {
       __ pop(edx);
       Handle<Code> ic(Builtins::builtin(Builtins::KeyedStoreIC_Initialize));
       EmitCallIC(ic, RelocInfo::CODE_TARGET);
+      PrepareForBailoutForId(expr->AssignmentId(), TOS_REG);
       if (expr->is_postfix()) {
         // Result is on the stack
         if (!context()->IsEffect()) {

test/mjsunit/regress/regress-989.js

+// Copyright 2010 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 bugs when deoptimizing after assignments in effect
+// contexts.
+
+// Bug 989 is that there was an extra value on the expression stack when
+// deoptimizing after an assignment in effect context (the value of the
+// assignment was lingering).  This is hard to observe in the unoptimized
+// code.
+//
+// This test uses comma expressions to put assignments in effect contexts,
+// references to deleted global variables to force deoptimization, and
+// function calls to observe an extra value.
+
+function first(x, y) { return x; }
+var y = 0;
+var o = {};
+o.x = 0;
+o[0] = 0;
+
+// Assignment to global variable.
+x0 = 0;
+function test0() { return first((y = 1, typeof x0), 2); }
+// Call the function once to compile it.
+assertEquals('number', test0());
+// Delete to force deoptimization on the next call.
+delete x0;
+assertEquals('undefined', test0());
+
+// Compound assignment to global variable.
+x1 = 0;
+function test1() { return first((y += 1, typeof x1), 2); }
+assertEquals('number', test1(), 'test1 before');
+delete x1;
+assertEquals('undefined', test1(), 'test1 after');
+
+// Pre and post-increment of global variable.
+x2 = 0;
+function test2() { return first((++y, typeof x2), 2); }
+assertEquals('number', test2(), 'test2 before');
+delete x2;
+assertEquals('undefined', test2(), 'test2 after');
+
+x3 = 0;
+function test3() { return first((y++, typeof x3), 2); }
+assertEquals('number', test3(), 'test3 before');
+delete x3;
+assertEquals('undefined', test3(), 'test3 after');
+
+
+// Assignment, compound assignment, and pre and post-increment of named
+// properties.
+x4 = 0;
+function test4() { return first((o.x = 1, typeof x4), 2); }
+assertEquals('number', test4());
+delete x4;
+assertEquals('undefined', test4());
+
+x5 = 0;
+function test5() { return first((o.x += 1, typeof x5), 2); }
+assertEquals('number', test5());
+delete x5;
+assertEquals('undefined', test5());
+
+x6 = 0;
+function test6() { return first((++o.x, typeof x6), 2); }
+assertEquals('number', test6());
+delete x6;
+assertEquals('undefined', test6());
+
+x7 = 0;
+function test7() { return first((o.x++, typeof x7), 2); }
+assertEquals('number', test7());
+delete x7;
+assertEquals('undefined', test7());
+
+
+// Assignment, compound assignment, and pre and post-increment of indexed
+// properties.
+x8 = 0;
+function test8(index) { return first((o[index] = 1, typeof x8), 2); }
+assertEquals('number', test8());
+delete x8;
+assertEquals('undefined', test8());
+
+x9 = 0;
+function test9(index) { return first((o[index] += 1, typeof x9), 2); }
+assertEquals('number', test9());
+delete x9;
+assertEquals('undefined', test9());
+
+x10 = 0;
+function test10(index) { return first((++o[index], typeof x10), 2); }
+assertEquals('number', test10());
+delete x10;
+assertEquals('undefined', test10());
+
+x11 = 0;
+function test11(index) { return first((o[index]++, typeof x11), 2); }
+assertEquals('number', test11());
+delete x11;
+assertEquals('undefined', test11());