From 88192fc01c09da8f1a3376e1ab6f8110cd83335c Mon Sep 17 00:00:00 2001 From: "iposva@chromium.org" Date: Tue, 16 Sep 2008 11:23:02 +0000 Subject: [PATCH] Fix http://code.google.com/p/v8/issues/detail?id=69 : - Simplify the switch statement code generation. - Ensure that the switch value is always popped from the stack. Credit goes to Feng for isolating the issue and proposing a fix. Review URL: http://codereview.chromium.org/2888 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@315 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/codegen-arm.cc | 38 ++++++++++++++++++--------------- src/codegen-ia32.cc | 36 +++++++++++++++++-------------- test/mjsunit/regress/regress-69.js | 43 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 33 deletions(-) create mode 100644 test/mjsunit/regress/regress-69.js diff --git a/src/codegen-arm.cc b/src/codegen-arm.cc index cd66ce4..778f69b 100644 --- a/src/codegen-arm.cc +++ b/src/codegen-arm.cc @@ -2761,19 +2761,14 @@ void ArmCodeGenerator::VisitSwitchStatement(SwitchStatement* node) { Comment cmnt(masm_, "[ case clause"); if (clause->is_default()) { + // Continue matching cases. The program will execute the default case's + // statements if it does not match any of the cases. + __ b(&next); + // Bind the default case label, so we can branch to it when we // have compared against all other cases. ASSERT(default_case.is_unused()); // at most one default clause - - // If the default case is the first (but not only) case, we have - // to jump past it for now. Once we're done with the remaining - // clauses, we'll branch back here. If it isn't the first case, - // we jump past it by avoiding to chain it into the next chain. - if (length > 1) { - if (i == 0) __ b(&next); - __ bind(&default_case); - } - + __ bind(&default_case); } else { __ bind(&next); next.Unuse(); @@ -2782,11 +2777,16 @@ void ArmCodeGenerator::VisitSwitchStatement(SwitchStatement* node) { Load(clause->label()); Comparison(eq, true); Branch(false, &next); - // Entering the case statement -> remove the switch value from the stack - __ pop(r0); } + // Entering the case statement for the first time. Remove the switch value + // from the stack. + __ pop(r0); + // Generate code for the body. + // This is also the target for the fall through from the previous case's + // statements which has to skip over the matching code and the popping of + // the switch value. __ bind(&fall_through); fall_through.Unuse(); VisitStatements(clause->statements()); @@ -2794,11 +2794,15 @@ void ArmCodeGenerator::VisitSwitchStatement(SwitchStatement* node) { } __ bind(&next); - // Reached the end of the case statements -> remove the switch value - // from the stack. - __ pop(r0); // __ Pop(no_reg) - if (default_case.is_bound()) __ b(&default_case); - + // Reached the end of the case statements without matching any of the cases. + if (default_case.is_bound()) { + // A default case exists -> execute its statements. + __ b(&default_case); + } else { + // Remove the switch value from the stack. + __ pop(r0); + } + __ bind(&fall_through); __ bind(node->break_target()); } diff --git a/src/codegen-ia32.cc b/src/codegen-ia32.cc index 94592d1..b6c3ed9 100644 --- a/src/codegen-ia32.cc +++ b/src/codegen-ia32.cc @@ -2917,19 +2917,14 @@ void Ia32CodeGenerator::VisitSwitchStatement(SwitchStatement* node) { Comment cmnt(masm_, "[ case clause"); if (clause->is_default()) { + // Continue matching cases. The program will execute the default case's + // statements if it does not match any of the cases. + __ jmp(&next); + // Bind the default case label, so we can branch to it when we // have compared against all other cases. ASSERT(default_case.is_unused()); // at most one default clause - - // If the default case is the first (but not only) case, we have - // to jump past it for now. Once we're done with the remaining - // clauses, we'll branch back here. If it isn't the first case, - // we jump past it by avoiding to chain it into the next chain. - if (length > 1) { - if (i == 0) __ jmp(&next); - __ bind(&default_case); - } - + __ bind(&default_case); } else { __ bind(&next); next.Unuse(); @@ -2938,11 +2933,16 @@ void Ia32CodeGenerator::VisitSwitchStatement(SwitchStatement* node) { Load(clause->label()); Comparison(equal, true); Branch(false, &next); - // Entering the case statement -> remove the switch value from the stack - __ pop(eax); } + // Entering the case statement for the first time. Remove the switch value + // from the stack. + __ pop(eax); + // Generate code for the body. + // This is also the target for the fall through from the previous case's + // statements which has to skip over the matching code and the popping of + // the switch value. __ bind(&fall_through); fall_through.Unuse(); VisitStatements(clause->statements()); @@ -2950,10 +2950,14 @@ void Ia32CodeGenerator::VisitSwitchStatement(SwitchStatement* node) { } __ bind(&next); - // Reached the end of the case statements -> remove the switch value - // from the stack - __ pop(eax); // Pop(no_reg) - if (default_case.is_bound()) __ jmp(&default_case); + // Reached the end of the case statements without matching any of the cases. + if (default_case.is_bound()) { + // A default case exists -> execute its statements. + __ jmp(&default_case); + } else { + // Remove the switch value from the stack. + __ pop(eax); + } __ bind(&fall_through); __ bind(node->break_target()); diff --git a/test/mjsunit/regress/regress-69.js b/test/mjsunit/regress/regress-69.js new file mode 100644 index 0000000..3fb1f76 --- /dev/null +++ b/test/mjsunit/regress/regress-69.js @@ -0,0 +1,43 @@ +// Copyright 2008 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. + +// This tests a switch statement with only default clause leaves +// balanced stack. It should not trigger the break point when --debug_code +// flag is turned on. +// See issue: http://code.google.com/p/v8/issues/detail?id=69 + +// Flags: --debug-code --expose-gc +function unbalanced_switch(a) { + try { + switch (a) { + default: break; + } + } catch (e) {} + gc(); +} + +unbalanced_switch(1); -- 2.7.4