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

@@ -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<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));
+    }
   }
 }
 

src/compiler.cc

@@ -645,6 +645,18 @@ void CodeGenSelector::VisitStatements(ZoneList<Statement*>* 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);
   }

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<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));
+    }
   }
 }
 

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<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));
+    }
   }
 }
 

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);