From 1af840ad4c8fab5ad03a1fa6631b42eee42a6b4b Mon Sep 17 00:00:00 2001 From: "kmillikin@chromium.org" Date: Mon, 2 May 2011 11:35:51 +0000 Subject: [PATCH] Be more discriminating about uses of the arguments object in optimized code. Because we track the value of the arguments object, we need to check values whenever plugged into a forbidden value context. It is not enough to check at only variable references as we did previously. R=fschneider@chromium.org BUG=1351 TEST=regress-1351.js Review URL: http://codereview.chromium.org/6902202 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@7739 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/hydrogen.cc | 32 +++++++++++++-------------- src/hydrogen.h | 24 ++++++++++++++++----- test/mjsunit/regress/regress-1351.js | 42 ++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 22 deletions(-) create mode 100644 test/mjsunit/regress/regress-1351.js diff --git a/src/hydrogen.cc b/src/hydrogen.cc index ef7e363..67ff14d 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -1943,6 +1943,9 @@ void EffectContext::ReturnValue(HValue* value) { void ValueContext::ReturnValue(HValue* value) { // The value is tracked in the bailout environment, and communicated // through the environment as the result of the expression. + if (!arguments_allowed() && value->CheckFlag(HValue::kIsArguments)) { + owner()->Bailout("bad value context for arguments value"); + } owner()->Push(value); } @@ -1959,6 +1962,9 @@ void EffectContext::ReturnInstruction(HInstruction* instr, int ast_id) { void ValueContext::ReturnInstruction(HInstruction* instr, int ast_id) { + if (!arguments_allowed() && instr->CheckFlag(HValue::kIsArguments)) { + owner()->Bailout("bad value context for arguments object value"); + } owner()->AddInstruction(instr); owner()->Push(instr); if (instr->HasSideEffects()) owner()->AddSimulate(ast_id); @@ -1985,6 +1991,9 @@ void TestContext::BuildBranch(HValue* value) { // property by always adding an empty block on the outgoing edges of this // branch. HGraphBuilder* builder = owner(); + if (value->CheckFlag(HValue::kIsArguments)) { + builder->Bailout("arguments object value in a test context"); + } HBasicBlock* empty_true = builder->graph()->CreateBasicBlock(); HBasicBlock* empty_false = builder->graph()->CreateBasicBlock(); HTest* test = new(zone()) HTest(value, empty_true, empty_false); @@ -2026,14 +2035,14 @@ void HGraphBuilder::VisitForEffect(Expression* expr) { } -void HGraphBuilder::VisitForValue(Expression* expr) { - ValueContext for_value(this); +void HGraphBuilder::VisitForValue(Expression* expr, ArgumentsAllowedFlag flag) { + ValueContext for_value(this, flag); Visit(expr); } void HGraphBuilder::VisitForTypeOf(Expression* expr) { - ValueContext for_value(this); + ValueContext for_value(this, ARGUMENTS_NOT_ALLOWED); for_value.set_for_typeof(true); Visit(expr); } @@ -2879,9 +2888,6 @@ void HGraphBuilder::VisitVariableProxy(VariableProxy* expr) { if (variable == NULL) { return Bailout("reference to rewritten variable"); } else if (variable->IsStackAllocated()) { - if (environment()->Lookup(variable)->CheckFlag(HValue::kIsArguments)) { - return Bailout("unsupported context for arguments object"); - } ast_context()->ReturnValue(environment()->Lookup(variable)); } else if (variable->IsContextSlot()) { if (variable->mode() == Variable::CONST) { @@ -3451,19 +3457,11 @@ void HGraphBuilder::VisitAssignment(Assignment* expr) { // Handle the assignment. if (var->IsStackAllocated()) { - HValue* value = NULL; - // Handle stack-allocated variables on the right-hand side directly. // We do not allow the arguments object to occur in a context where it // may escape, but assignments to stack-allocated locals are - // permitted. Handling such assignments here bypasses the check for - // the arguments object in VisitVariableProxy. - Variable* rhs_var = expr->value()->AsVariableProxy()->AsVariable(); - if (rhs_var != NULL && rhs_var->IsStackAllocated()) { - value = environment()->Lookup(rhs_var); - } else { - CHECK_ALIVE(VisitForValue(expr->value())); - value = Pop(); - } + // permitted. + CHECK_ALIVE(VisitForValue(expr->value(), ARGUMENTS_ALLOWED)); + HValue* value = Pop(); Bind(var, value); ast_context()->ReturnValue(value); diff --git a/src/hydrogen.h b/src/hydrogen.h index a7dba01..f65386f 100644 --- a/src/hydrogen.h +++ b/src/hydrogen.h @@ -439,6 +439,11 @@ class HEnvironment: public ZoneObject { class HGraphBuilder; +enum ArgumentsAllowedFlag { + ARGUMENTS_NOT_ALLOWED, + ARGUMENTS_ALLOWED +}; + // This class is not BASE_EMBEDDED because our inlining implementation uses // new and delete. class AstContext { @@ -497,13 +502,18 @@ class EffectContext: public AstContext { class ValueContext: public AstContext { public: - explicit ValueContext(HGraphBuilder* owner) - : AstContext(owner, Expression::kValue) { + explicit ValueContext(HGraphBuilder* owner, ArgumentsAllowedFlag flag) + : AstContext(owner, Expression::kValue), flag_(flag) { } virtual ~ValueContext(); virtual void ReturnValue(HValue* value); virtual void ReturnInstruction(HInstruction* instr, int ast_id); + + bool arguments_allowed() { return flag_ == ARGUMENTS_ALLOWED; } + + private: + ArgumentsAllowedFlag flag_; }; @@ -666,6 +676,8 @@ class HGraphBuilder: public AstVisitor { void Push(HValue* value) { environment()->Push(value); } HValue* Pop() { return environment()->Pop(); } + void Bailout(const char* reason); + private: // Type of a member function that generates inline code for a native function. typedef void (HGraphBuilder::*InlineFunctionGenerator)(CallRuntime* call); @@ -720,8 +732,6 @@ class HGraphBuilder: public AstVisitor { INLINE_RUNTIME_FUNCTION_LIST(INLINE_FUNCTION_GENERATOR_DECLARATION) #undef INLINE_FUNCTION_GENERATOR_DECLARATION - void Bailout(const char* reason); - void PreProcessOsrEntry(IterationStatement* statement); // True iff. we are compiling for OSR and the statement is the entry. bool HasOsrEntryAt(IterationStatement* statement); @@ -751,7 +761,11 @@ class HGraphBuilder: public AstVisitor { void Drop(int n) { environment()->Drop(n); } void Bind(Variable* var, HValue* value) { environment()->Bind(var, value); } - void VisitForValue(Expression* expr); + // The value of the arguments object is allowed in some but not most value + // contexts. (It's allowed in all effect contexts and disallowed in all + // test contexts.) + void VisitForValue(Expression* expr, + ArgumentsAllowedFlag flag = ARGUMENTS_NOT_ALLOWED); void VisitForTypeOf(Expression* expr); void VisitForEffect(Expression* expr); void VisitForControl(Expression* expr, diff --git a/test/mjsunit/regress/regress-1351.js b/test/mjsunit/regress/regress-1351.js new file mode 100644 index 0000000..656b19f --- /dev/null +++ b/test/mjsunit/regress/regress-1351.js @@ -0,0 +1,42 @@ +// Copyright 2011 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. + +// Flags: --allow-natives-syntax + +// Test that the arguments value is does not escape when it appears as +// an intermediate value in an expression. + +function h() { } + +function f() { + var a = null; + h(a = arguments); +} + +f(); +%OptimizeFunctionOnNextCall(f); +f(); -- 2.7.4