From 0bf2822266e6e08c41d60cd1bc1f45e85aa45eb4 Mon Sep 17 00:00:00 2001 From: "kmillikin@chromium.org" Date: Mon, 7 Dec 2009 13:31:47 +0000 Subject: [PATCH] The toplevel code generator assumed that declarations did not shadow parameters. This could case the initial value to be lost or worse, a crash. Fix by handling the case of a declaration shadowing both stack-allocated parameters and those in the arguments object. This is related to V8 issue 540. http://code.google.com/p/v8/issues/detail?id=540 BUG=29565 Review URL: http://codereview.chromium.org/469006 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@3429 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/fast-codegen-arm.cc | 148 ++++++++++++++++++++---------------- src/compiler.cc | 12 +++ src/ia32/fast-codegen-ia32.cc | 136 +++++++++++++++++++-------------- src/x64/fast-codegen-x64.cc | 142 ++++++++++++++++++++-------------- test/mjsunit/regress/regress-540.js | 17 ++++- 5 files changed, 273 insertions(+), 182 deletions(-) diff --git a/src/arm/fast-codegen-arm.cc b/src/arm/fast-codegen-arm.cc index 03e00db..51ac229 100644 --- a/src/arm/fast-codegen-arm.cc +++ b/src/arm/fast-codegen-arm.cc @@ -414,78 +414,98 @@ void FastCodeGenerator::VisitDeclaration(Declaration* decl) { Variable* var = decl->proxy()->var(); ASSERT(var != NULL); // Must have been resolved. Slot* slot = var->slot(); - ASSERT(slot != NULL); // No global declarations here. - - // We have 3 cases for slots: LOOKUP, LOCAL, CONTEXT. - switch (slot->type()) { - case Slot::LOOKUP: { - __ mov(r2, Operand(var->name())); - // Declaration nodes are always introduced in one of two modes. - ASSERT(decl->mode() == Variable::VAR || decl->mode() == Variable::CONST); - PropertyAttributes attr = decl->mode() == Variable::VAR ? - NONE : READ_ONLY; - __ mov(r1, Operand(Smi::FromInt(attr))); - // Push initial value, if any. - // Note: For variables we must not push an initial value (such as - // 'undefined') because we may have a (legal) redeclaration and we - // must not destroy the current value. - if (decl->mode() == Variable::CONST) { - __ mov(r0, Operand(Factory::the_hole_value())); - __ stm(db_w, sp, cp.bit() | r2.bit() | r1.bit() | r0.bit()); - } else if (decl->fun() != NULL) { - __ stm(db_w, sp, cp.bit() | r2.bit() | r1.bit()); - Visit(decl->fun()); // Initial value for function decl. - } else { - __ mov(r0, Operand(Smi::FromInt(0))); // No initial value! - __ stm(db_w, sp, cp.bit() | r2.bit() | r1.bit() | r0.bit()); - } - __ CallRuntime(Runtime::kDeclareContextSlot, 4); - break; - } - case Slot::LOCAL: - if (decl->mode() == Variable::CONST) { - __ mov(r0, Operand(Factory::the_hole_value())); - __ str(r0, MemOperand(fp, SlotOffset(var->slot()))); - } else if (decl->fun() != NULL) { - Visit(decl->fun()); - __ pop(r0); - __ str(r0, MemOperand(fp, SlotOffset(var->slot()))); - } - break; - case Slot::CONTEXT: - // The variable in the decl always resides in the current context. - ASSERT(function_->scope()->ContextChainLength(slot->var()->scope()) == 0); - if (decl->mode() == Variable::CONST) { - __ mov(r0, Operand(Factory::the_hole_value())); + Property* prop = var->AsProperty(); + + if (slot != NULL) { + switch (slot->type()) { + case Slot::PARAMETER: // Fall through. + case Slot::LOCAL: + if (decl->mode() == Variable::CONST) { + __ LoadRoot(ip, Heap::kTheHoleValueRootIndex); + __ str(ip, MemOperand(fp, SlotOffset(var->slot()))); + } else if (decl->fun() != NULL) { + Visit(decl->fun()); + __ pop(ip); + __ str(ip, MemOperand(fp, SlotOffset(var->slot()))); + } + break; + + case Slot::CONTEXT: + // The variable in the decl always resides in the current context. + ASSERT_EQ(0, function_->scope()->ContextChainLength(var->scope())); if (FLAG_debug_code) { // Check if we have the correct context pointer. - __ ldr(r1, CodeGenerator::ContextOperand(cp, - Context::FCONTEXT_INDEX)); + __ ldr(r1, + CodeGenerator::ContextOperand(cp, Context::FCONTEXT_INDEX)); __ cmp(r1, cp); __ Check(eq, "Unexpected declaration in current context."); } - __ str(r0, CodeGenerator::ContextOperand(cp, slot->index())); - // No write barrier since the_hole_value is in old space. - ASSERT(!Heap::InNewSpace(*Factory::the_hole_value())); - } else if (decl->fun() != NULL) { + if (decl->mode() == Variable::CONST) { + __ LoadRoot(ip, Heap::kTheHoleValueRootIndex); + __ str(ip, CodeGenerator::ContextOperand(cp, slot->index())); + // No write barrier since the_hole_value is in old space. + } else if (decl->fun() != NULL) { + Visit(decl->fun()); + __ pop(r0); + __ str(r0, CodeGenerator::ContextOperand(cp, slot->index())); + int offset = Context::SlotOffset(slot->index()); + __ mov(r2, Operand(offset)); + // We know that we have written a function, which is not a smi. + __ RecordWrite(cp, r2, r0); + } + break; + + case Slot::LOOKUP: { + __ mov(r2, Operand(var->name())); + // Declaration nodes are always introduced in one of two modes. + ASSERT(decl->mode() == Variable::VAR || + decl->mode() == Variable::CONST); + PropertyAttributes attr = + (decl->mode() == Variable::VAR) ? NONE : READ_ONLY; + __ mov(r1, Operand(Smi::FromInt(attr))); + // Push initial value, if any. + // Note: For variables we must not push an initial value (such as + // 'undefined') because we may have a (legal) redeclaration and we + // must not destroy the current value. + if (decl->mode() == Variable::CONST) { + __ LoadRoot(r0, Heap::kTheHoleValueRootIndex); + __ stm(db_w, sp, cp.bit() | r2.bit() | r1.bit() | r0.bit()); + } else if (decl->fun() != NULL) { + __ stm(db_w, sp, cp.bit() | r2.bit() | r1.bit()); + Visit(decl->fun()); // Initial value for function decl. + } else { + __ mov(r0, Operand(Smi::FromInt(0))); // No initial value! + __ stm(db_w, sp, cp.bit() | r2.bit() | r1.bit() | r0.bit()); + } + __ CallRuntime(Runtime::kDeclareContextSlot, 4); + break; + } + } + + } else if (prop != NULL) { + if (decl->fun() != NULL || decl->mode() == Variable::CONST) { + // We are declaring a function or constant that rewrites to a + // property. Use (keyed) IC to set the initial value. + ASSERT_EQ(Expression::kValue, prop->obj()->context()); + Visit(prop->obj()); + ASSERT_EQ(Expression::kValue, prop->key()->context()); + Visit(prop->key()); + + if (decl->fun() != NULL) { + ASSERT_EQ(Expression::kValue, decl->fun()->context()); Visit(decl->fun()); __ pop(r0); - if (FLAG_debug_code) { - // Check if we have the correct context pointer. - __ ldr(r1, CodeGenerator::ContextOperand(cp, - Context::FCONTEXT_INDEX)); - __ cmp(r1, cp); - __ Check(eq, "Unexpected declaration in current context."); - } - __ str(r0, CodeGenerator::ContextOperand(cp, slot->index())); - int offset = Context::SlotOffset(slot->index()); - __ mov(r2, Operand(offset)); - // We know that we have written a function, which is not a smi. - __ RecordWrite(cp, r2, r0); + } else { + __ LoadRoot(r0, Heap::kTheHoleValueRootIndex); } - break; - default: - UNREACHABLE(); + + Handle ic(Builtins::builtin(Builtins::KeyedStoreIC_Initialize)); + __ Call(ic, RelocInfo::CODE_TARGET); + + // Value in r0 is ignored (declarations are statements). Receiver + // and key on stack are discarded. + __ add(sp, sp, Operand(2 * kPointerSize)); + } } } diff --git a/src/compiler.cc b/src/compiler.cc index 7f65d46..1f3f01a 100644 --- a/src/compiler.cc +++ b/src/compiler.cc @@ -645,6 +645,18 @@ void CodeGenSelector::VisitStatements(ZoneList* stmts) { void CodeGenSelector::VisitDeclaration(Declaration* decl) { + Property* prop = decl->proxy()->AsProperty(); + if (prop != NULL) { + // Property rewrites are shared, ensure we are not changing its + // expression context state. + ASSERT(prop->obj()->context() == Expression::kUninitialized || + prop->obj()->context() == Expression::kValue); + ASSERT(prop->key()->context() == Expression::kUninitialized || + prop->key()->context() == Expression::kValue); + ProcessExpression(prop->obj(), Expression::kValue); + ProcessExpression(prop->key(), Expression::kValue); + } + if (decl->fun() != NULL) { ProcessExpression(decl->fun(), Expression::kValue); } diff --git a/src/ia32/fast-codegen-ia32.cc b/src/ia32/fast-codegen-ia32.cc index 2601b6a..e7e7533 100644 --- a/src/ia32/fast-codegen-ia32.cc +++ b/src/ia32/fast-codegen-ia32.cc @@ -412,46 +412,24 @@ void FastCodeGenerator::VisitDeclaration(Declaration* decl) { Variable* var = decl->proxy()->var(); ASSERT(var != NULL); // Must have been resolved. Slot* slot = var->slot(); - ASSERT(slot != NULL); // No global declarations here. - - // We have 3 cases for slots: LOOKUP, LOCAL, CONTEXT. - switch (slot->type()) { - case Slot::LOOKUP: { - __ push(esi); - __ push(Immediate(var->name())); - // Declaration nodes are always introduced in one of two modes. - ASSERT(decl->mode() == Variable::VAR || decl->mode() == Variable::CONST); - PropertyAttributes attr = - (decl->mode() == Variable::VAR) ? NONE : READ_ONLY; - __ push(Immediate(Smi::FromInt(attr))); - // Push initial value, if any. - // Note: For variables we must not push an initial value (such as - // 'undefined') because we may have a (legal) redeclaration and we - // must not destroy the current value. - if (decl->mode() == Variable::CONST) { - __ push(Immediate(Factory::the_hole_value())); - } else if (decl->fun() != NULL) { - Visit(decl->fun()); - } else { - __ push(Immediate(Smi::FromInt(0))); // No initial value! - } - __ CallRuntime(Runtime::kDeclareContextSlot, 4); - break; - } - case Slot::LOCAL: - if (decl->mode() == Variable::CONST) { - __ mov(Operand(ebp, SlotOffset(var->slot())), - Immediate(Factory::the_hole_value())); - } else if (decl->fun() != NULL) { - Visit(decl->fun()); - __ pop(Operand(ebp, SlotOffset(var->slot()))); - } - break; - case Slot::CONTEXT: - // The variable in the decl always resides in the current context. - ASSERT(function_->scope()->ContextChainLength(slot->var()->scope()) == 0); - if (decl->mode() == Variable::CONST) { - __ mov(eax, Immediate(Factory::the_hole_value())); + Property* prop = var->AsProperty(); + + if (slot != NULL) { + switch (slot->type()) { + case Slot::PARAMETER: // Fall through. + case Slot::LOCAL: + if (decl->mode() == Variable::CONST) { + __ mov(Operand(ebp, SlotOffset(var->slot())), + Immediate(Factory::the_hole_value())); + } else if (decl->fun() != NULL) { + Visit(decl->fun()); + __ pop(Operand(ebp, SlotOffset(var->slot()))); + } + break; + + case Slot::CONTEXT: + // The variable in the decl always resides in the current context. + ASSERT_EQ(0, function_->scope()->ContextChainLength(var->scope())); if (FLAG_debug_code) { // Check if we have the correct context pointer. __ mov(ebx, @@ -459,26 +437,70 @@ void FastCodeGenerator::VisitDeclaration(Declaration* decl) { __ cmp(ebx, Operand(esi)); __ Check(equal, "Unexpected declaration in current context."); } - __ mov(CodeGenerator::ContextOperand(esi, slot->index()), eax); - // No write barrier since the_hole_value is in old space. - ASSERT(!Heap::InNewSpace(*Factory::the_hole_value())); - } else if (decl->fun() != NULL) { + if (decl->mode() == Variable::CONST) { + __ mov(eax, Immediate(Factory::the_hole_value())); + __ mov(CodeGenerator::ContextOperand(esi, slot->index()), eax); + // No write barrier since the hole value is in old space. + } else if (decl->fun() != NULL) { + Visit(decl->fun()); + __ pop(eax); + __ mov(CodeGenerator::ContextOperand(esi, slot->index()), eax); + int offset = Context::SlotOffset(slot->index()); + __ RecordWrite(esi, offset, eax, ecx); + } + break; + + case Slot::LOOKUP: { + __ push(esi); + __ push(Immediate(var->name())); + // Declaration nodes are always introduced in one of two modes. + ASSERT(decl->mode() == Variable::VAR || + decl->mode() == Variable::CONST); + PropertyAttributes attr = + (decl->mode() == Variable::VAR) ? NONE : READ_ONLY; + __ push(Immediate(Smi::FromInt(attr))); + // Push initial value, if any. + // Note: For variables we must not push an initial value (such as + // 'undefined') because we may have a (legal) redeclaration and we + // must not destroy the current value. + if (decl->mode() == Variable::CONST) { + __ push(Immediate(Factory::the_hole_value())); + } else if (decl->fun() != NULL) { + Visit(decl->fun()); + } else { + __ push(Immediate(Smi::FromInt(0))); // No initial value! + } + __ CallRuntime(Runtime::kDeclareContextSlot, 4); + break; + } + } + + } else if (prop != NULL) { + if (decl->fun() != NULL || decl->mode() == Variable::CONST) { + // We are declaring a function or constant that rewrites to a + // property. Use (keyed) IC to set the initial value. + ASSERT_EQ(Expression::kValue, prop->obj()->context()); + Visit(prop->obj()); + ASSERT_EQ(Expression::kValue, prop->key()->context()); + Visit(prop->key()); + + if (decl->fun() != NULL) { + ASSERT_EQ(Expression::kValue, decl->fun()->context()); Visit(decl->fun()); __ pop(eax); - if (FLAG_debug_code) { - // Check if we have the correct context pointer. - __ mov(ebx, - CodeGenerator::ContextOperand(esi, Context::FCONTEXT_INDEX)); - __ cmp(ebx, Operand(esi)); - __ Check(equal, "Unexpected declaration in current context."); - } - __ mov(CodeGenerator::ContextOperand(esi, slot->index()), eax); - int offset = Context::SlotOffset(slot->index()); - __ RecordWrite(esi, offset, eax, ecx); + } else { + __ Set(eax, Immediate(Factory::the_hole_value())); } - break; - default: - UNREACHABLE(); + + Handle ic(Builtins::builtin(Builtins::KeyedStoreIC_Initialize)); + __ call(ic, RelocInfo::CODE_TARGET); + // Absence of a test eax instruction following the call + // indicates that none of the load was inlined. + + // Value in eax is ignored (declarations are statements). Receiver + // and key on stack are discarded. + __ add(Operand(esp), Immediate(2 * kPointerSize)); + } } } diff --git a/src/x64/fast-codegen-x64.cc b/src/x64/fast-codegen-x64.cc index 4f8f3b1..cff2524 100644 --- a/src/x64/fast-codegen-x64.cc +++ b/src/x64/fast-codegen-x64.cc @@ -420,73 +420,97 @@ void FastCodeGenerator::VisitDeclaration(Declaration* decl) { Variable* var = decl->proxy()->var(); ASSERT(var != NULL); // Must have been resolved. Slot* slot = var->slot(); - ASSERT(slot != NULL); // No global declarations here. - - // We have 3 cases for slots: LOOKUP, LOCAL, CONTEXT. - switch (slot->type()) { - case Slot::LOOKUP: { - __ push(rsi); - __ Push(var->name()); - // Declaration nodes are always introduced in one of two modes. - ASSERT(decl->mode() == Variable::VAR || decl->mode() == Variable::CONST); - PropertyAttributes attr = decl->mode() == Variable::VAR ? - NONE : READ_ONLY; - __ Push(Smi::FromInt(attr)); - // Push initial value, if any. - // Note: For variables we must not push an initial value (such as - // 'undefined') because we may have a (legal) redeclaration and we - // must not destroy the current value. - if (decl->mode() == Variable::CONST) { - __ Push(Factory::the_hole_value()); - } else if (decl->fun() != NULL) { - Visit(decl->fun()); - } else { - __ Push(Smi::FromInt(0)); // no initial value! - } - __ CallRuntime(Runtime::kDeclareContextSlot, 4); - break; - } - case Slot::LOCAL: - if (decl->mode() == Variable::CONST) { - __ Move(Operand(rbp, SlotOffset(var->slot())), - Factory::the_hole_value()); - } else if (decl->fun() != NULL) { - Visit(decl->fun()); - __ pop(Operand(rbp, SlotOffset(var->slot()))); - } - break; - case Slot::CONTEXT: - // The variable in the decl always resides in the current context. - ASSERT(function_->scope()->ContextChainLength(slot->var()->scope()) == 0); - if (decl->mode() == Variable::CONST) { - __ Move(rax, Factory::the_hole_value()); + Property* prop = var->AsProperty(); + + if (slot != NULL) { + switch (slot->type()) { + case Slot::PARAMETER: // Fall through. + case Slot::LOCAL: + if (decl->mode() == Variable::CONST) { + __ LoadRoot(kScratchRegister, Heap::kTheHoleValueRootIndex); + __ movq(Operand(rbp, SlotOffset(var->slot())), kScratchRegister); + } else if (decl->fun() != NULL) { + Visit(decl->fun()); + __ pop(Operand(rbp, SlotOffset(var->slot()))); + } + break; + + case Slot::CONTEXT: + // The variable in the decl always resides in the current context. + ASSERT_EQ(0, function_->scope()->ContextChainLength(var->scope())); if (FLAG_debug_code) { // Check if we have the correct context pointer. - __ movq(rbx, CodeGenerator::ContextOperand(rsi, - Context::FCONTEXT_INDEX)); + __ movq(rbx, + CodeGenerator::ContextOperand(rsi, Context::FCONTEXT_INDEX)); __ cmpq(rbx, rsi); __ Check(equal, "Unexpected declaration in current context."); } - __ movq(CodeGenerator::ContextOperand(rsi, slot->index()), rax); - // No write barrier since the_hole_value is in old space. - ASSERT(!Heap::InNewSpace(*Factory::the_hole_value())); - } else if (decl->fun() != NULL) { + if (decl->mode() == Variable::CONST) { + __ LoadRoot(kScratchRegister, Heap::kTheHoleValueRootIndex); + __ movq(CodeGenerator::ContextOperand(rsi, slot->index()), + kScratchRegister); + // No write barrier since the hole value is in old space. + } else if (decl->fun() != NULL) { + Visit(decl->fun()); + __ pop(rax); + __ movq(CodeGenerator::ContextOperand(rsi, slot->index()), rax); + int offset = Context::SlotOffset(slot->index()); + __ RecordWrite(rsi, offset, rax, rcx); + } + break; + + case Slot::LOOKUP: { + __ push(rsi); + __ Push(var->name()); + // Declaration nodes are always introduced in one of two modes. + ASSERT(decl->mode() == Variable::VAR || + decl->mode() == Variable::CONST); + PropertyAttributes attr = + (decl->mode() == Variable::VAR) ? NONE : READ_ONLY; + __ Push(Smi::FromInt(attr)); + // Push initial value, if any. + // Note: For variables we must not push an initial value (such as + // 'undefined') because we may have a (legal) redeclaration and we + // must not destroy the current value. + if (decl->mode() == Variable::CONST) { + __ PushRoot(Heap::kTheHoleValueRootIndex); + } else if (decl->fun() != NULL) { + Visit(decl->fun()); + } else { + __ Push(Smi::FromInt(0)); // no initial value! + } + __ CallRuntime(Runtime::kDeclareContextSlot, 4); + break; + } + } + + } else if (prop != NULL) { + if (decl->fun() != NULL || decl->mode() == Variable::CONST) { + // We are declaring a function or constant that rewrites to a + // property. Use (keyed) IC to set the initial value. + ASSERT_EQ(Expression::kValue, prop->obj()->context()); + Visit(prop->obj()); + ASSERT_EQ(Expression::kValue, prop->key()->context()); + Visit(prop->key()); + + if (decl->fun() != NULL) { + ASSERT_EQ(Expression::kValue, decl->fun()->context()); Visit(decl->fun()); __ pop(rax); - if (FLAG_debug_code) { - // Check if we have the correct context pointer. - __ movq(rbx, CodeGenerator::ContextOperand(rsi, - Context::FCONTEXT_INDEX)); - __ cmpq(rbx, rsi); - __ Check(equal, "Unexpected declaration in current context."); - } - __ movq(CodeGenerator::ContextOperand(rsi, slot->index()), rax); - int offset = Context::SlotOffset(slot->index()); - __ RecordWrite(rsi, offset, rax, rcx); + } else { + __ LoadRoot(rax, Heap::kTheHoleValueRootIndex); } - break; - default: - UNREACHABLE(); + + Handle ic(Builtins::builtin(Builtins::KeyedStoreIC_Initialize)); + __ call(ic, RelocInfo::CODE_TARGET); + + // Absence of a test rax instruction following the call + // indicates that none of the load was inlined. + + // Value in rax is ignored (declarations are statements). Receiver + // and key on stack are discarded. + __ addq(rsp, Immediate(2 * kPointerSize)); + } } } diff --git a/test/mjsunit/regress/regress-540.js b/test/mjsunit/regress/regress-540.js index 31e0b7a..c40fa2c 100644 --- a/test/mjsunit/regress/regress-540.js +++ b/test/mjsunit/regress/regress-540.js @@ -29,6 +29,19 @@ // See http://code.google.com/p/v8/issues/detail?id=540 function f(x, y) { eval(x); return y(); } -assertEquals(1, f("function y() { return 1; }", - function () { return 0; })); +var result = f("function y() { return 1; }", function () { return 0; }) +assertEquals(1, result); +result = + (function (x) { + function x() { return 3; } + return x(); + })(function () { return 2; }); +assertEquals(3, result); + +result = + (function (x) { + function x() { return 5; } + return arguments[0](); + })(function () { return 4; }); +assertEquals(5, result); -- 2.7.4