Revision: 3429 Author: [email protected] Date: Mon Dec 7 05:31:47 2009 Log: 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 http://code.google.com/p/v8/source/detail?r=3429 Modified: /branches/bleeding_edge/src/arm/fast-codegen-arm.cc /branches/bleeding_edge/src/compiler.cc /branches/bleeding_edge/src/ia32/fast-codegen-ia32.cc /branches/bleeding_edge/src/x64/fast-codegen-x64.cc /branches/bleeding_edge/test/mjsunit/regress/regress-540.js ======================================= --- /branches/bleeding_edge/src/arm/fast-codegen-arm.cc Fri Dec 4 06:30:27 2009 +++ /branches/bleeding_edge/src/arm/fast-codegen-arm.cc Mon Dec 7 05:31:47 2009 @@ -414,78 +414,98 @@ 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); - } - break; - default: - UNREACHABLE(); + } else { + __ LoadRoot(r0, Heap::kTheHoleValueRootIndex); + } + + Handle<Code> 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)); + } } } ======================================= --- /branches/bleeding_edge/src/compiler.cc Fri Dec 4 06:30:27 2009 +++ /branches/bleeding_edge/src/compiler.cc Mon Dec 7 05:31:47 2009 @@ -645,6 +645,18 @@ 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); } ======================================= --- /branches/bleeding_edge/src/ia32/fast-codegen-ia32.cc Fri Dec 4 06:30:27 2009 +++ /branches/bleeding_edge/src/ia32/fast-codegen-ia32.cc Mon Dec 7 05:31:47 2009 @@ -412,46 +412,24 @@ 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 @@ __ 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); - } - break; - default: - UNREACHABLE(); + } else { + __ Set(eax, Immediate(Factory::the_hole_value())); + } + + Handle<Code> 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)); + } } } ======================================= --- /branches/bleeding_edge/src/x64/fast-codegen-x64.cc Fri Dec 4 06:30:27 2009 +++ /branches/bleeding_edge/src/x64/fast-codegen-x64.cc Mon Dec 7 05:31:47 2009 @@ -420,73 +420,97 @@ 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); - } - break; - default: - UNREACHABLE(); + } else { + __ LoadRoot(rax, Heap::kTheHoleValueRootIndex); + } + + Handle<Code> 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)); + } } } ======================================= --- /branches/bleeding_edge/test/mjsunit/regress/regress-540.js Fri Dec 4 03:59:09 2009 +++ /branches/bleeding_edge/test/mjsunit/regress/regress-540.js Mon Dec 7 05:31:47 2009 @@ -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); -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
