From: wingo@igalia.com Date: Wed, 12 Jun 2013 11:02:51 +0000 (+0000) Subject: Allocate generator result objects before unwinding try handlers X-Git-Tag: upstream/4.7.83~13896 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=418ddc800a2a4ca793e78e4f58e47ab003b37814;p=platform%2Fupstream%2Fv8.git 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 --- diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc index 8b24bf1..0e603b4 100644 --- a/src/arm/full-codegen-arm.cc +++ b/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) { - EmitReturnIteratorResult(false); - } else { - __ pop(result_register()); - EmitReturnSequence(); - } + __ 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(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); } diff --git a/src/full-codegen.cc b/src/full-codegen.cc index bad634c..4d5d5da 100644 --- a/src/full-codegen.cc +++ b/src/full-codegen.cc @@ -1230,13 +1230,7 @@ void FullCodeGenerator::VisitBreakStatement(BreakStatement* stmt) { } -void FullCodeGenerator::VisitReturnStatement(ReturnStatement* stmt) { - Comment cmnt(masm_, "[ ReturnStatement"); - SetStatementPosition(stmt); - Expression* expr = stmt->expression(); - VisitForAccumulatorValue(expr); - - // Exit all nested statements. +void FullCodeGenerator::EmitUnwindBeforeReturn() { 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(); } diff --git a/src/full-codegen.h b/src/full-codegen.h index 68263a5..6d84ef1 100644 --- a/src/full-codegen.h +++ b/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* declarations); - // Generator code to return a fresh iterator result object. The "value" - // property is set to a value popped from the stack, and "done" is set - // according to the argument. - void EmitReturnIteratorResult(bool done); + // Generate code to create an iterator result object. The "value" property is + // set to a value popped from the stack, and "done" is set according to the + // argument. The result object is left in the result register. + 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(); diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc index c77faaa..26a64d7 100644 --- a/src/ia32/full-codegen-ia32.cc +++ b/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) { - EmitReturnIteratorResult(false); - } else { - __ pop(result_register()); - EmitReturnSequence(); - } + __ 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(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); } diff --git a/src/x64/full-codegen-x64.cc b/src/x64/full-codegen-x64.cc index e9fe2a8..4068af3 100644 --- a/src/x64/full-codegen-x64.cc +++ b/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) { - EmitReturnIteratorResult(false); - } else { - __ pop(result_register()); - EmitReturnSequence(); - } + __ 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(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); } diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index 8d6274b..585d503 100644 --- a/test/mjsunit/mjsunit.status +++ b/test/mjsunit/mjsunit.status @@ -47,10 +47,6 @@ regress/regress-crbug-160010: SKIP 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 compiler/regress-funcaller: PASS, SKIP if $mode == debug