Remove arguments shadow from scopes, stop rewriting parameters.

arguments/src/scopes.cc

Index: arguments/src/scopes.cc
diff --git a/arguments/src/scopes.cc b/arguments/src/scopes.cc
index cffbef6d1f8455bd89eb1caabde85d669f8d5331..6458a630e5a772c4e6907d5c60a075566fd4354d 100644
--- a/arguments/src/scopes.cc
+++ b/arguments/src/scopes.cc
@@ -155,24 +155,6 @@ Scope::Scope(Scope* inner_scope, SerializedScopeInfo* scope_info)
   }
 
   AddInnerScope(inner_scope);
-
-  // This scope's arguments shadow (if present) is context-allocated if an inner
-  // scope accesses this one's parameters.  Allocate the arguments_shadow_
-  // variable if necessary.
-  Variable::Mode mode;
-  int arguments_shadow_index =
-      scope_info_->ContextSlotIndex(Heap::arguments_shadow_symbol(), &mode);
-  if (arguments_shadow_index >= 0) {
-    ASSERT(mode == Variable::INTERNAL);
-    arguments_shadow_ = new Variable(this,
-                                     Factory::arguments_shadow_symbol(),
-                                     Variable::INTERNAL,
-                                     true,
-                                     Variable::ARGUMENTS);
-    arguments_shadow_->set_rewrite(
-        new Slot(arguments_shadow_, Slot::CONTEXT, arguments_shadow_index));
-    arguments_shadow_->set_is_used(true);
-  }
 }
 
 
@@ -265,52 +247,33 @@ Variable* Scope::LocalLookup(Handle<String> name) {
   if (result != NULL || !resolved()) {
     return result;
   }
-  // If the scope is resolved, we can find a variable in serialized scope info.
-
-  // We should never lookup 'arguments' in this scope
-  // as it is implicitly present in any scope.
+  // If the scope is resolved, we can find a variable in serialized scope
+  // info.
+  //
+  // We should never lookup 'arguments' in this scope as it is implicitly
+  // present in every scope.
   ASSERT(*name != *Factory::arguments_symbol());
-
-  // Assert that there is no local slot with the given name.
+  // There should be no local slot with the given name.
   ASSERT(scope_info_->StackSlotIndex(*name) < 0);
 
   // Check context slot lookup.
   Variable::Mode mode;
   int index = scope_info_->ContextSlotIndex(*name, &mode);
-  if (index >= 0) {
-    Variable* var =
-        variables_.Declare(this, name, mode, true, Variable::NORMAL);
-    var->set_rewrite(new Slot(var, Slot::CONTEXT, index));
-    return var;
-  }
-
-  index = scope_info_->ParameterIndex(*name);
-  if (index >= 0) {
-    // ".arguments" must be present in context slots.
-    ASSERT(arguments_shadow_ != NULL);
-    Variable* var =
-        variables_.Declare(this, name, Variable::VAR, true, Variable::NORMAL);
-    Property* rewrite =
-        new Property(new VariableProxy(arguments_shadow_),
-                     new Literal(Handle<Object>(Smi::FromInt(index))),
-                     RelocInfo::kNoPosition,
-                     Property::SYNTHETIC);
-    rewrite->set_is_arguments_access(true);
-    var->set_rewrite(rewrite);
-    return var;
-  }
-
-  index = scope_info_->FunctionContextSlotIndex(*name);
-  if (index >= 0) {
-    // Check that there is no local slot with the given name.
-    ASSERT(scope_info_->StackSlotIndex(*name) < 0);
-    Variable* var =
-        variables_.Declare(this, name, Variable::VAR, true, Variable::NORMAL);

[Kevin Millikin (Chromium), 2011/03/17 13:47:46]

I made these variables all have mode VAR, but I thought that this one used to be
CONST.

-    var->set_rewrite(new Slot(var, Slot::CONTEXT, index));
-    return var;
+  if (index < 0) {
+    // Check parameters.
+    mode = Variable::VAR;
+    index = scope_info_->ParameterIndex(*name);
+    if (index < 0) {
+      // Check the function name.
+      index = scope_info_->FunctionContextSlotIndex(*name);
+      if (index < 0) return NULL;
+    }
   }
 
-  return NULL;
+  Variable* var =
+      variables_.Declare(this, name, mode, true, Variable::NORMAL);
+  var->set_rewrite(new Slot(var, Slot::CONTEXT, index));
+  return var;
 }
 
 
@@ -890,36 +853,17 @@ void Scope::AllocateParameterLocals() {
   Variable* arguments = LocalLookup(Factory::arguments_symbol());
   ASSERT(arguments != NULL);  // functions have 'arguments' declared implicitly
 
-  // Parameters are rewritten to arguments[i] if 'arguments' is used in
-  // a non-strict mode function. Strict mode code doesn't alias arguments.
-  bool rewrite_parameters = false;
+  bool uses_nonstrict_arguments = false;
 
   if (MustAllocate(arguments) && !HasArgumentsParameter()) {
     // 'arguments' is used. Unless there is also a parameter called
-    // 'arguments', we must be conservative and access all parameters via
-    // the arguments object: The i'th parameter is rewritten into
-    // '.arguments[i]' (*). If we have a parameter named 'arguments', a
-    // (new) value is always assigned to it via the function
-    // invocation. Then 'arguments' denotes that specific parameter value
-    // and cannot be used to access the parameters, which is why we don't
-    // need to rewrite in that case.
-    //
-    // (*) Instead of having a parameter called 'arguments', we may have an
-    // assignment to 'arguments' in the function body, at some arbitrary
-    // point in time (possibly through an 'eval()' call!). After that
-    // assignment any re-write of parameters would be invalid (was bug
-    // 881452). Thus, we introduce a shadow '.arguments'
-    // variable which also points to the arguments object. For rewrites we
-    // use '.arguments' which remains valid even if we assign to
-    // 'arguments'. To summarize: If we need to rewrite, we allocate an
-    // 'arguments' object dynamically upon function invocation. The compiler
-    // introduces 2 local variables 'arguments' and '.arguments', both of
-    // which originally point to the arguments object that was
-    // allocated. All parameters are rewritten into property accesses via
-    // the '.arguments' variable. Thus, any changes to properties of
-    // 'arguments' are reflected in the variables and vice versa. If the
-    // 'arguments' variable is changed, '.arguments' still points to the
-    // correct arguments object and the rewrites still work.
+    // 'arguments', we must be conservative and allocate all parameters to
+    // the context assuming they will be captured by the arguments object.
+    // (*). If we have a parameter named 'arguments', a (new) value is

[Mads Ager (chromium), 2011/03/17 14:28:10]

The (*) is not gone. :-)

[Kevin Millikin (Chromium), 2011/03/17 14:35:04]

It is now.

+    // always assigned to it via the function invocation. Then 'arguments'
+    // denotes that specific parameter value and cannot be used to access
+    // the parameters, which is why we don't need to allocate an arguments
+    // object in that case.
 
     // We are using 'arguments'. Tell the code generator that is needs to
     // allocate the arguments object by setting 'arguments_'.
@@ -928,75 +872,31 @@ void Scope::AllocateParameterLocals() {
     // In strict mode 'arguments' does not alias formal parameters.
     // Therefore in strict mode we allocate parameters as if 'arguments'
     // were not used.
-    rewrite_parameters = !is_strict_mode();
-  }
-
-  if (rewrite_parameters) {
-    // We also need the '.arguments' shadow variable. Declare it and create
-    // and bind the corresponding proxy. It's ok to declare it only now
-    // because it's a local variable that is allocated after the parameters
-    // have been allocated.
-    //
-    // Note: This is "almost" at temporary variable but we cannot use
-    // NewTemporary() because the mode needs to be INTERNAL since this
-    // variable may be allocated in the heap-allocated context (temporaries
-    // are never allocated in the context).
-    arguments_shadow_ = new Variable(this,
-                                     Factory::arguments_shadow_symbol(),
-                                     Variable::INTERNAL,
-                                     true,
-                                     Variable::ARGUMENTS);
-    arguments_shadow_->set_is_used(true);
-    temps_.Add(arguments_shadow_);
-
-    // Allocate the parameters by rewriting them into '.arguments[i]' accesses.
-    for (int i = 0; i < params_.length(); i++) {
-      Variable* var = params_[i];
-      ASSERT(var->scope() == this);
-      if (MustAllocate(var)) {
-        if (MustAllocateInContext(var)) {
-          // It is ok to set this only now, because arguments is a local
-          // variable that is allocated after the parameters have been
-          // allocated.
-          arguments_shadow_->MarkAsAccessedFromInnerScope();
-        }
-        Property* rewrite =
-            new Property(new VariableProxy(arguments_shadow_),
-                         new Literal(Handle<Object>(Smi::FromInt(i))),
-                         RelocInfo::kNoPosition,
-                         Property::SYNTHETIC);
-        rewrite->set_is_arguments_access(true);
-        var->set_rewrite(rewrite);
-      }
+    uses_nonstrict_arguments = !is_strict_mode();
+  }
+
+  // The same parameter may occur multiple times in the parameters_ list.
+  // If it does, and if it is not copied into the context object, it must
+  // receive the highest parameter index for that parameter; thus iteration
+  // order is relevant!
+  for (int i = params_.length() - 1; i >= 0; --i) {
+    Variable* var = params_[i];
+    ASSERT(var->scope() == this);
+    if (uses_nonstrict_arguments) {
+      // Give the parameter a use from an inner scope, to force allocation
+      // to the context.
+      var->MarkAsAccessedFromInnerScope();
     }
 
-  } else {
-    // The arguments object is not used, so we can access parameters directly.
-    // The same parameter may occur multiple times in the parameters_ list.
-    // If it does, and if it is not copied into the context object, it must
-    // receive the highest parameter index for that parameter; thus iteration
-    // order is relevant!
-    for (int i = 0; i < params_.length(); i++) {
-      Variable* var = params_[i];
-      ASSERT(var->scope() == this);
-      if (MustAllocate(var)) {
-        if (MustAllocateInContext(var)) {
-          ASSERT(var->rewrite() == NULL ||
-                 (var->AsSlot() != NULL &&
-                  var->AsSlot()->type() == Slot::CONTEXT));
-          if (var->rewrite() == NULL) {
-            // Only set the heap allocation if the parameter has not
-            // been allocated yet.
-            AllocateHeapSlot(var);
-          }
-        } else {
-          ASSERT(var->rewrite() == NULL ||
-                 (var->AsSlot() != NULL &&
-                  var->AsSlot()->type() == Slot::PARAMETER));
-          // Set the parameter index always, even if the parameter
-          // was seen before! (We need to access the actual parameter
-          // supplied for the last occurrence of a multiply declared
-          // parameter.)
+    if (MustAllocate(var)) {
+      if (MustAllocateInContext(var)) {
+        ASSERT(var->rewrite() == NULL || var->IsContextSlot());
+        if (var->rewrite() == NULL) {
+          AllocateHeapSlot(var);
+        }
+      } else {
+        ASSERT(var->rewrite() == NULL || var->IsParameter());
+        if (var->rewrite() == NULL) {
           var->set_rewrite(new Slot(var, Slot::PARAMETER, i));
         }
       }