Allocate generator result objects before unwinding try handlers

When a generator suspends, it saves its state out to the heap and
unwinds try handlers but doesn't pop anything off the stack.  Instead it
relies on no GC happening between the suspend and the return from the
generator.  However this was not the case: boxing the result object
could cause GC, which would try to traverse the stack but would
misinterpret words from unwound try handlers as heap objects.

This CL changes to allocate the result objects before the suspend.  It
also removes the generators-iteration skip introduced in r15065.

R=mstarzinger@chromium.org
TEST=mjsunit/harmony/generators-iteration
BUG=

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

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

src/arm/full-codegen-arm.cc

@@ -1991,8 +1991,12 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
   VisitForStackValue(expr->expression());
 
   switch (expr->yield_kind()) {
-    case Yield::INITIAL:
+    case Yield::SUSPEND:
-    case Yield::SUSPEND: {
+      // Pop value from top-of-stack slot; box result into result register.
+      EmitCreateIteratorResult(false);
+      __ push(result_register());
+      // Fall through.
+    case Yield::INITIAL: {
       VisitForStackValue(expr->generator_object());
       __ CallRuntime(Runtime::kSuspendJSGeneratorObject, 1);
       __ ldr(context_register(),
@@ -2001,12 +2005,8 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
       Label resume;
       __ CompareRoot(result_register(), Heap::kTheHoleValueRootIndex);
       __ b(ne, &resume);
-      if (expr->yield_kind() == Yield::SUSPEND) {
+      __ pop(result_register());
-        EmitReturnIteratorResult(false);
+      EmitReturnSequence();
-      } else {
-        __ pop(result_register());
-        EmitReturnSequence();
-      }
 
       __ bind(&resume);
       context()->Plug(result_register());
@@ -2018,7 +2018,10 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
       __ mov(r1, Operand(Smi::FromInt(JSGeneratorObject::kGeneratorClosed)));
       __ str(r1, FieldMemOperand(result_register(),
                                  JSGeneratorObject::kContinuationOffset));
-      EmitReturnIteratorResult(true);
+      // Pop value from top-of-stack slot, box result into result register.
+      EmitCreateIteratorResult(true);
+      EmitUnwindBeforeReturn();
+      EmitReturnSequence();
       break;
     }
 
@@ -2048,10 +2051,10 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
 
       // try { received = yield result.value }
       __ bind(&l_try);
-      __ pop(r0);                                        // result.value
+      EmitCreateIteratorResult(false);                   // pop and box to r0
       __ PushTryHandler(StackHandler::CATCH, expr->index());
       const int handler_size = StackHandlerConstants::kSize;
-      __ push(r0);                                       // result.value
+      __ push(r0);                                       // result
       __ ldr(r3, MemOperand(sp, (0 + 1) * kPointerSize + handler_size));  // g
       __ push(r3);                                       // g
       __ CallRuntime(Runtime::kSuspendJSGeneratorObject, 1);
@@ -2059,7 +2062,8 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
              MemOperand(fp, StandardFrameConstants::kContextOffset));
       __ CompareRoot(r0, Heap::kTheHoleValueRootIndex);
       __ b(ne, &l_resume);
-      EmitReturnIteratorResult(false);
+      __ pop(r0);                                        // result
+      EmitReturnSequence();
       __ bind(&l_resume);                                // received in r0
       __ PopTryHandler();
 
@@ -2214,13 +2218,20 @@ void FullCodeGenerator::EmitGeneratorResume(Expression *generator,
 }
 
 
-void FullCodeGenerator::EmitReturnIteratorResult(bool done) {
+void FullCodeGenerator::EmitCreateIteratorResult(bool done) {
   Label gc_required;
   Label allocated;
 
   Handle<Map> map(isolate()->native_context()->generator_result_map());
 
   __ Allocate(map->instance_size(), r0, r2, r3, &gc_required, TAG_OBJECT);
+  __ jmp(&allocated);
+
+  __ bind(&gc_required);
+  __ Push(Smi::FromInt(map->instance_size()));
+  __ CallRuntime(Runtime::kAllocateInNewSpace, 1);
+  __ ldr(context_register(),
+         MemOperand(fp, StandardFrameConstants::kContextOffset));
 
   __ bind(&allocated);
   __ mov(r1, Operand(map));
@@ -2240,26 +2251,6 @@ void FullCodeGenerator::EmitReturnIteratorResult(bool done) {
   // root set.
   __ RecordWriteField(r0, JSGeneratorObject::kResultValuePropertyOffset,
                       r2, r3, kLRHasBeenSaved, kDontSaveFPRegs);
-
-  if (done) {
-    // Exit all nested statements.
-    NestedStatement* current = nesting_stack_;
-    int stack_depth = 0;
-    int context_length = 0;
-    while (current != NULL) {
-      current = current->Exit(&stack_depth, &context_length);
-    }
-    __ Drop(stack_depth);
-  }
-
-  EmitReturnSequence();
-
-  __ bind(&gc_required);
-  __ Push(Smi::FromInt(map->instance_size()));
-  __ CallRuntime(Runtime::kAllocateInNewSpace, 1);
-  __ ldr(context_register(),
-         MemOperand(fp, StandardFrameConstants::kContextOffset));
-  __ jmp(&allocated);
 }
 
 

src/full-codegen.cc

@@ -1230,13 +1230,7 @@ void FullCodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
 }
 
 
-void FullCodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
+void FullCodeGenerator::EmitUnwindBeforeReturn() {
-  Comment cmnt(masm_, "[ ReturnStatement");
-  SetStatementPosition(stmt);
-  Expression* expr = stmt->expression();
-  VisitForAccumulatorValue(expr);
-
-  // Exit all nested statements.
   NestedStatement* current = nesting_stack_;
   int stack_depth = 0;
   int context_length = 0;
@@ -1244,7 +1238,15 @@ void FullCodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
     current = current->Exit(&stack_depth, &context_length);
   }
   __ Drop(stack_depth);
+}
 
+
+void FullCodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
+  Comment cmnt(masm_, "[ ReturnStatement");
+  SetStatementPosition(stmt);
+  Expression* expr = stmt->expression();
+  VisitForAccumulatorValue(expr);
+  EmitUnwindBeforeReturn();
   EmitReturnSequence();
 }
 

src/full-codegen.h

@@ -410,10 +410,10 @@ class FullCodeGenerator: public AstVisitor {
   // this has to be a separate pass _before_ populating or executing any module.
   void AllocateModules(ZoneList<Declaration*>* declarations);
 
-  // Generator code to return a fresh iterator result object.  The "value"
+  // Generate code to create an iterator result object.  The "value" property is
-  // property is set to a value popped from the stack, and "done" is set
+  // set to a value popped from the stack, and "done" is set according to the
-  // according to the argument.
+  // argument.  The result object is left in the result register.
-  void EmitReturnIteratorResult(bool done);
+  void EmitCreateIteratorResult(bool done);
 
   // Try to perform a comparison as a fast inlined literal compare if
   // the operands allow it.  Returns true if the compare operations
@@ -472,6 +472,11 @@ class FullCodeGenerator: public AstVisitor {
   void EmitProfilingCounterDecrement(int delta);
   void EmitProfilingCounterReset();
 
+  // Emit code to pop values from the stack associated with nested statements
+  // like try/catch, try/finally, etc, running the finallies and unwinding the
+  // handlers as needed.
+  void EmitUnwindBeforeReturn();
+
   // Platform-specific return sequence
   void EmitReturnSequence();
 

src/ia32/full-codegen-ia32.cc

@@ -1950,8 +1950,12 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
   VisitForStackValue(expr->expression());
 
   switch (expr->yield_kind()) {
-    case Yield::INITIAL:
+    case Yield::SUSPEND:
-    case Yield::SUSPEND: {
+      // Pop value from top-of-stack slot; box result into result register.
+      EmitCreateIteratorResult(false);
+      __ push(result_register());
+      // Fall through.
+    case Yield::INITIAL: {
       VisitForStackValue(expr->generator_object());
       __ CallRuntime(Runtime::kSuspendJSGeneratorObject, 1);
       __ mov(context_register(),
@@ -1960,12 +1964,8 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
       Label resume;
       __ CompareRoot(result_register(), Heap::kTheHoleValueRootIndex);
       __ j(not_equal, &resume);
-      if (expr->yield_kind() == Yield::SUSPEND) {
+      __ pop(result_register());
-        EmitReturnIteratorResult(false);
+      EmitReturnSequence();
-      } else {
-        __ pop(result_register());
-        EmitReturnSequence();
-      }
 
       __ bind(&resume);
       context()->Plug(result_register());
@@ -1977,7 +1977,10 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
       __ mov(FieldOperand(result_register(),
                           JSGeneratorObject::kContinuationOffset),
              Immediate(Smi::FromInt(JSGeneratorObject::kGeneratorClosed)));
-      EmitReturnIteratorResult(true);
+      // Pop value from top-of-stack slot, box result into result register.
+      EmitCreateIteratorResult(true);
+      EmitUnwindBeforeReturn();
+      EmitReturnSequence();
       break;
     }
 
@@ -2006,17 +2009,18 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
 
       // try { received = yield result.value }
       __ bind(&l_try);
-      __ pop(eax);                                       // result.value
+      EmitCreateIteratorResult(false);                    // pop and box to eax
       __ PushTryHandler(StackHandler::CATCH, expr->index());
       const int handler_size = StackHandlerConstants::kSize;
-      __ push(eax);                                      // result.value
+      __ push(eax);                                      // result
       __ push(Operand(esp, (0 + 1) * kPointerSize + handler_size));  // g
       __ CallRuntime(Runtime::kSuspendJSGeneratorObject, 1);
       __ mov(context_register(),
              Operand(ebp, StandardFrameConstants::kContextOffset));
       __ CompareRoot(eax, Heap::kTheHoleValueRootIndex);
       __ j(not_equal, &l_resume);
-      EmitReturnIteratorResult(false);
+      __ pop(eax);                                       // result
+      EmitReturnSequence();
       __ bind(&l_resume);                                // received in eax
       __ PopTryHandler();
 
@@ -2169,13 +2173,20 @@ void FullCodeGenerator::EmitGeneratorResume(Expression *generator,
 }
 
 
-void FullCodeGenerator::EmitReturnIteratorResult(bool done) {
+void FullCodeGenerator::EmitCreateIteratorResult(bool done) {
   Label gc_required;
   Label allocated;
 
   Handle<Map> map(isolate()->native_context()->generator_result_map());
 
   __ Allocate(map->instance_size(), eax, ecx, edx, &gc_required, TAG_OBJECT);
+  __ jmp(&allocated);
+
+  __ bind(&gc_required);
+  __ Push(Smi::FromInt(map->instance_size()));
+  __ CallRuntime(Runtime::kAllocateInNewSpace, 1);
+  __ mov(context_register(),
+         Operand(ebp, StandardFrameConstants::kContextOffset));
 
   __ bind(&allocated);
   __ mov(ebx, map);
@@ -2194,26 +2205,6 @@ void FullCodeGenerator::EmitReturnIteratorResult(bool done) {
   // root set.
   __ RecordWriteField(eax, JSGeneratorObject::kResultValuePropertyOffset,
                       ecx, edx, kDontSaveFPRegs);
-
-  if (done) {
-    // Exit all nested statements.
-    NestedStatement* current = nesting_stack_;
-    int stack_depth = 0;
-    int context_length = 0;
-    while (current != NULL) {
-      current = current->Exit(&stack_depth, &context_length);
-    }
-    __ Drop(stack_depth);
-  }
-
-  EmitReturnSequence();
-
-  __ bind(&gc_required);
-  __ Push(Smi::FromInt(map->instance_size()));
-  __ CallRuntime(Runtime::kAllocateInNewSpace, 1);
-  __ mov(context_register(),
-         Operand(ebp, StandardFrameConstants::kContextOffset));
-  __ jmp(&allocated);
 }
 
 

src/x64/full-codegen-x64.cc

@@ -1974,8 +1974,12 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
   VisitForStackValue(expr->expression());
 
   switch (expr->yield_kind()) {
-    case Yield::INITIAL:
+    case Yield::SUSPEND:
-    case Yield::SUSPEND: {
+      // Pop value from top-of-stack slot; box result into result register.
+      EmitCreateIteratorResult(false);
+      __ push(result_register());
+      // Fall through.
+    case Yield::INITIAL: {
       VisitForStackValue(expr->generator_object());
       __ CallRuntime(Runtime::kSuspendJSGeneratorObject, 1);
       __ movq(context_register(),
@@ -1984,12 +1988,8 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
       Label resume;
       __ CompareRoot(result_register(), Heap::kTheHoleValueRootIndex);
       __ j(not_equal, &resume);
-      if (expr->yield_kind() == Yield::SUSPEND) {
+      __ pop(result_register());
-        EmitReturnIteratorResult(false);
+      EmitReturnSequence();
-      } else {
-        __ pop(result_register());
-        EmitReturnSequence();
-      }
 
       __ bind(&resume);
       context()->Plug(result_register());
@@ -2001,7 +2001,10 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
       __ Move(FieldOperand(result_register(),
                            JSGeneratorObject::kContinuationOffset),
               Smi::FromInt(JSGeneratorObject::kGeneratorClosed));
-      EmitReturnIteratorResult(true);
+      // Pop value from top-of-stack slot, box result into result register.
+      EmitCreateIteratorResult(true);
+      EmitUnwindBeforeReturn();
+      EmitReturnSequence();
       break;
     }
 
@@ -2031,17 +2034,18 @@ void FullCodeGenerator::VisitYield(Yield* expr) {
 
       // try { received = yield result.value }
       __ bind(&l_try);
-      __ pop(rax);                                       // result.value
+      EmitCreateIteratorResult(false);                    // pop and box to rax
       __ PushTryHandler(StackHandler::CATCH, expr->index());
       const int handler_size = StackHandlerConstants::kSize;
-      __ push(rax);                                      // result.value
+      __ push(rax);                                      // result
       __ push(Operand(rsp, (0 + 1) * kPointerSize + handler_size));  // g
       __ CallRuntime(Runtime::kSuspendJSGeneratorObject, 1);
       __ movq(context_register(),
               Operand(rbp, StandardFrameConstants::kContextOffset));
       __ CompareRoot(rax, Heap::kTheHoleValueRootIndex);
       __ j(not_equal, &l_resume);
-      EmitReturnIteratorResult(false);
+      __ pop(rax);                                       // result
+      EmitReturnSequence();
       __ bind(&l_resume);                                // received in rax
       __ PopTryHandler();
 
@@ -2195,13 +2199,20 @@ void FullCodeGenerator::EmitGeneratorResume(Expression *generator,
 }
 
 
-void FullCodeGenerator::EmitReturnIteratorResult(bool done) {
+void FullCodeGenerator::EmitCreateIteratorResult(bool done) {
   Label gc_required;
   Label allocated;
 
   Handle<Map> map(isolate()->native_context()->generator_result_map());
 
   __ Allocate(map->instance_size(), rax, rcx, rdx, &gc_required, TAG_OBJECT);
+  __ jmp(&allocated);
+
+  __ bind(&gc_required);
+  __ Push(Smi::FromInt(map->instance_size()));
+  __ CallRuntime(Runtime::kAllocateInNewSpace, 1);
+  __ movq(context_register(),
+          Operand(rbp, StandardFrameConstants::kContextOffset));
 
   __ bind(&allocated);
   __ Move(rbx, map);
@@ -2222,26 +2233,6 @@ void FullCodeGenerator::EmitReturnIteratorResult(bool done) {
   // root set.
   __ RecordWriteField(rax, JSGeneratorObject::kResultValuePropertyOffset,
                       rcx, rdx, kDontSaveFPRegs);
-
-  if (done) {
-    // Exit all nested statements.
-    NestedStatement* current = nesting_stack_;
-    int stack_depth = 0;
-    int context_length = 0;
-    while (current != NULL) {
-      current = current->Exit(&stack_depth, &context_length);
-    }
-    __ Drop(stack_depth);
-  }
-
-  EmitReturnSequence();
-
-  __ bind(&gc_required);
-  __ Push(Smi::FromInt(map->instance_size()));
-  __ CallRuntime(Runtime::kAllocateInNewSpace, 1);
-  __ movq(context_register(),
-          Operand(rbp, StandardFrameConstants::kContextOffset));
-  __ jmp(&allocated);
 }
 
 

test/mjsunit/mjsunit.status

@@ -46,10 +46,6 @@ regress/regress-crbug-160010: SKIP
 # Deferred stack trace formatting is temporarily disabled.
 stack-traces-gc: PASS || FAIL
 
-##############################################################################
-# TODO(wingo): Re-enable when GC bug from r15060 is solved.
-harmony/generators-iteration: SKIP
-
 ##############################################################################
 # Too slow in debug mode with --stress-opt mode.
 compiler/regress-stacktrace-methods: PASS, SKIP if $mode == debug