From ece2c03160345995958b3d3dd5aca989a334c6da Mon Sep 17 00:00:00 2001 From: "kmillikin@chromium.org" Date: Mon, 9 Mar 2009 10:51:57 +0000 Subject: [PATCH] Fix issue 263: http://code.google.com/p/v8/issues/detail?id=263 Sharing the code to unlink the exception handler for a try/finally causes us to try to merge virtual frames with different heights (due to statements that keep state on the stack) at the entry to the unlink code. Avoid this by unlinking the handler separately for each exit from the try block. Review URL: http://codereview.chromium.org/39331 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1449 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/codegen-arm.cc | 97 ++++++++++++++++++++----------------- src/codegen-ia32.cc | 85 +++++++++++++++++--------------- test/mjsunit/regress/regress-263.js | 38 +++++++++++++++ 3 files changed, 137 insertions(+), 83 deletions(-) create mode 100644 test/mjsunit/regress/regress-263.js diff --git a/src/codegen-arm.cc b/src/codegen-arm.cc index d4dc67e..3647190 100644 --- a/src/codegen-arm.cc +++ b/src/codegen-arm.cc @@ -2058,22 +2058,25 @@ void CodeGenerator::VisitTryCatch(TryCatch* node) { // Stop the introduced shadowing and count the number of required unlinks. // After shadowing stops, the original labels are unshadowed and the // LabelShadows represent the formerly shadowing labels. - int nof_unlinks = 0; + bool has_unlinks = false; for (int i = 0; i <= nof_escapes; i++) { shadows[i]->StopShadowing(); - if (shadows[i]->is_linked()) nof_unlinks++; + has_unlinks = has_unlinks || shadows[i]->is_linked(); } function_return_is_shadowed_ = function_return_was_shadowed; + // Get an external reference to the handler address. + ExternalReference handler_address(Top::k_handler_address); + + // The next handler address is at kNextIndex in the stack. const int kNextIndex = StackHandlerConstants::kNextOffset / kPointerSize; // If we can fall off the end of the try block, unlink from try chain. if (has_valid_frame()) { - // The next handler address is at kNextIndex in the stack. __ ldr(r1, frame_->ElementAt(kNextIndex)); - __ mov(r3, Operand(ExternalReference(Top::k_handler_address))); + __ mov(r3, Operand(handler_address)); __ str(r1, MemOperand(r3)); frame_->Drop(StackHandlerConstants::kSize / kPointerSize); - if (nof_unlinks > 0) { + if (has_unlinks) { exit.Jump(); } } @@ -2090,7 +2093,7 @@ void CodeGenerator::VisitTryCatch(TryCatch* node) { // Reload sp from the top handler, because some statements that we // break from (eg, for...in) may have left stuff on the stack. - __ mov(r3, Operand(ExternalReference(Top::k_handler_address))); + __ mov(r3, Operand(handler_address)); __ ldr(sp, MemOperand(r3)); // The stack pointer was restored to just below the code slot // (the topmost slot) in the handler. @@ -2128,7 +2131,6 @@ void CodeGenerator::VisitTryFinally(TryFinally* node) { // break/continue from within the try block. enum { FALLING, THROWING, JUMPING }; - JumpTarget unlink(this); JumpTarget try_block(this); JumpTarget finally_block(this); @@ -2179,26 +2181,60 @@ void CodeGenerator::VisitTryFinally(TryFinally* node) { } function_return_is_shadowed_ = function_return_was_shadowed; - // If we can fall off the end of the try block, set the state on the stack - // to FALLING. + // Get an external reference to the handler address. + ExternalReference handler_address(Top::k_handler_address); + + // The next handler address is at kNextIndex in the stack. + const int kNextIndex = StackHandlerConstants::kNextOffset / kPointerSize; + // If we can fall off the end of the try block, unlink from the try + // chain and set the state on the frame to FALLING. if (has_valid_frame()) { - __ mov(r0, Operand(Factory::undefined_value())); // fake TOS + __ ldr(r1, frame_->ElementAt(kNextIndex)); + __ mov(r3, Operand(handler_address)); + __ str(r1, MemOperand(r3)); + frame_->Drop(StackHandlerConstants::kSize / kPointerSize); + + // Fake a top of stack value (unneeded when FALLING) and set the + // state in r2, then jump around the unlink blocks if any. + __ mov(r0, Operand(Factory::undefined_value())); frame_->EmitPush(r0); __ mov(r2, Operand(Smi::FromInt(FALLING))); if (nof_unlinks > 0) { - unlink.Jump(); + finally_block.Jump(); } } - // Generate code to set the state for the (formerly) shadowing labels that - // have been jumped to. + // Generate code to unlink and set the state for the (formerly) + // shadowing labels that have been jumped to. for (int i = 0; i <= nof_escapes; i++) { if (shadows[i]->is_linked()) { + // If we have come from the shadowed return, the return value is + // in (a non-refcounted reference to) r0. We must preserve it + // until it is pushed. + // // Because we can be jumping here (to spilled code) from // unspilled code, we need to reestablish a spilled frame at // this block. shadows[i]->Bind(); frame_->SpillAll(); + + // Reload sp from the top handler, because some statements that + // we break from (eg, for...in) may have left stuff on the + // stack. + __ mov(r3, Operand(handler_address)); + __ ldr(sp, MemOperand(r3)); + // The stack pointer was restored to the address slot in the handler. + ASSERT(StackHandlerConstants::kNextOffset == 1 * kPointerSize); + frame_->Forget(frame_->height() - handler_height + 1); + + // Unlink this handler and drop it from the frame. The next + // handler address is now on top of the frame. + frame_->EmitPop(r1); + __ str(r1, MemOperand(r3)); + // The top (code) and the second (handler) slot have both been + // dropped already. + frame_->Drop(StackHandlerConstants::kSize / kPointerSize - 2); + if (i == kReturnShadowIndex) { // If this label shadowed the function return, materialize the // return value on the stack. @@ -2209,40 +2245,13 @@ void CodeGenerator::VisitTryFinally(TryFinally* node) { frame_->EmitPush(r0); } __ mov(r2, Operand(Smi::FromInt(JUMPING + i))); - unlink.Jump(); + if (--nof_unlinks > 0) { + // If this is not the last unlink block, jump around the next. + finally_block.Jump(); + } } } - // Unlink from try chain; - if (unlink.is_linked()) { - unlink.Bind(); - } - - // Control can reach here via a jump to unlink or by falling off the - // end of the try block (with no unlinks). - if (has_valid_frame()) { - // Preserve TOS result in r0 across stack manipulation. - frame_->EmitPop(r0); - // Reload sp from the top handler, because some statements that we - // break from (eg, for...in) may have left stuff on the stack. - __ mov(r3, Operand(ExternalReference(Top::k_handler_address))); - __ ldr(sp, MemOperand(r3)); - // The stack pointer was restored to just below the code slot (the - // topmost slot) in the handler. - frame_->Forget(frame_->height() - handler_height + 1); - const int kNextIndex = (StackHandlerConstants::kNextOffset - + StackHandlerConstants::kAddressDisplacement) - / kPointerSize; - __ ldr(r1, frame_->ElementAt(kNextIndex)); - __ str(r1, MemOperand(r3)); - ASSERT(StackHandlerConstants::kCodeOffset == 0); - // Drop the rest of the handler (not including the already dropped - // code slot). - frame_->Drop(StackHandlerConstants::kSize / kPointerSize - 1); - // Restore the result to TOS. - frame_->EmitPush(r0); - } - // --- Finally block --- finally_block.Bind(); diff --git a/src/codegen-ia32.cc b/src/codegen-ia32.cc index 649c9e5..28c7eec 100644 --- a/src/codegen-ia32.cc +++ b/src/codegen-ia32.cc @@ -2671,10 +2671,10 @@ void CodeGenerator::VisitTryCatch(TryCatch* node) { // Stop the introduced shadowing and count the number of required unlinks. // After shadowing stops, the original targets are unshadowed and the // ShadowTargets represent the formerly shadowing targets. - int nof_unlinks = 0; + bool has_unlinks = false; for (int i = 0; i <= nof_escapes; i++) { shadows[i]->StopShadowing(); - if (shadows[i]->is_linked()) nof_unlinks++; + has_unlinks = has_unlinks || shadows[i]->is_linked(); } function_return_is_shadowed_ = function_return_was_shadowed; @@ -2692,11 +2692,12 @@ void CodeGenerator::VisitTryCatch(TryCatch* node) { // If we can fall off the end of the try block, unlink from try chain. if (has_valid_frame()) { - // The TOS is the next handler address. - frame_->EmitPop(eax); - __ mov(Operand::StaticVariable(handler_address), eax); + // The next handler address is on top of the frame. Unlink from + // the handler list and drop the rest of this handler from the + // frame. + frame_->EmitPop(Operand::StaticVariable(handler_address)); frame_->Drop(StackHandlerConstants::kSize / kPointerSize - 1); - if (nof_unlinks > 0) { + if (has_unlinks) { exit.Jump(); } } @@ -2745,7 +2746,6 @@ void CodeGenerator::VisitTryFinally(TryFinally* node) { // break/continue from within the try block. enum { FALLING, THROWING, JUMPING }; - JumpTarget unlink(this); JumpTarget try_block(this); JumpTarget finally_block(this); @@ -2797,25 +2797,54 @@ void CodeGenerator::VisitTryFinally(TryFinally* node) { } function_return_is_shadowed_ = function_return_was_shadowed; - // If we can fall off the end of the try block, set the state on the stack - // to FALLING. + // Get an external reference to the handler address. + ExternalReference handler_address(Top::k_handler_address); + + // If we can fall off the end of the try block, unlink from the try + // chain and set the state on the frame to FALLING. if (has_valid_frame()) { - frame_->EmitPush(Immediate(Factory::undefined_value())); // fake TOS + // The next handler address is on top of the frame. + ASSERT(StackHandlerConstants::kNextOffset == 0); + frame_->EmitPop(eax); + __ mov(Operand::StaticVariable(handler_address), eax); + frame_->Drop(StackHandlerConstants::kSize / kPointerSize - 1); + + // Fake a top of stack value (unneeded when FALLING) and set the + // state in ecx, then jump around the unlink blocks if any. + frame_->EmitPush(Immediate(Factory::undefined_value())); __ Set(ecx, Immediate(Smi::FromInt(FALLING))); if (nof_unlinks > 0) { - unlink.Jump(); + finally_block.Jump(); } } - // Generate code to set the state for the (formerly) shadowing targets that - // have been jumped to. + // Generate code to unlink and set the state for the (formerly) + // shadowing targets that have been jumped to. for (int i = 0; i <= nof_escapes; i++) { if (shadows[i]->is_linked()) { + // If we have come from the shadowed return, the return value is + // in (a non-refcounted reference to) eax. We must preserve it + // until it is pushed. + // // Because we can be jumping here (to spilled code) from // unspilled code, we need to reestablish a spilled frame at // this block. shadows[i]->Bind(); frame_->SpillAll(); + + // Reload sp from the top handler, because some statements that + // we break from (eg, for...in) may have left stuff on the + // stack. + __ mov(edx, Operand::StaticVariable(handler_address)); + const int kNextOffset = StackHandlerConstants::kNextOffset + + StackHandlerConstants::kAddressDisplacement; + __ lea(esp, Operand(edx, kNextOffset)); + frame_->Forget(frame_->height() - handler_height); + + // Unlink this handler and drop it from the frame. + frame_->EmitPop(Operand::StaticVariable(handler_address)); + frame_->Drop(StackHandlerConstants::kSize / kPointerSize - 1); + if (i == kReturnShadowIndex) { // If this target shadowed the function return, materialize // the return value on the stack. @@ -2825,35 +2854,13 @@ void CodeGenerator::VisitTryFinally(TryFinally* node) { frame_->EmitPush(Immediate(Factory::undefined_value())); } __ Set(ecx, Immediate(Smi::FromInt(JUMPING + i))); - unlink.Jump(); + if (--nof_unlinks > 0) { + // If this is not the last unlink block, jump around the next. + finally_block.Jump(); + } } } - // Unlink from try chain; be careful not to destroy the TOS. - if (unlink.is_linked()) { - unlink.Bind(); - } - - // Control can reach here via a jump to unlink or by falling off the - // end of the try block (with no unlinks). - if (has_valid_frame()) { - // Reload sp from the top handler, because some statements that we - // break from (eg, for...in) may have left stuff on the stack. - // Preserve the TOS in a register across stack manipulation. - frame_->EmitPop(eax); - ExternalReference handler_address(Top::k_handler_address); - __ mov(edx, Operand::StaticVariable(handler_address)); - const int kNextOffset = StackHandlerConstants::kNextOffset + - StackHandlerConstants::kAddressDisplacement; - __ lea(esp, Operand(edx, kNextOffset)); - frame_->Forget(frame_->height() - handler_height); - - frame_->EmitPop(Operand::StaticVariable(handler_address)); - frame_->Drop(StackHandlerConstants::kSize / kPointerSize - 1); - // Next_sp popped. - frame_->EmitPush(eax); - } - // --- Finally block --- finally_block.Bind(); diff --git a/test/mjsunit/regress/regress-263.js b/test/mjsunit/regress/regress-263.js new file mode 100644 index 0000000..123bde6 --- /dev/null +++ b/test/mjsunit/regress/regress-263.js @@ -0,0 +1,38 @@ +// Copyright 2009 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. + +// Exits via return, break, or continue from within try/finally or +// for/in should not crash or trigger a debug assert. + +// See http://code.google.com/p/v8/issues/detail?id=263 + +function test0() { with({}) for(var x in {}) return; } +test0(); + + +function test1() { with({}) try { } finally { with({}) return; } } +test1(); -- 2.7.4