Improve code for looking up in context slots in runtime.cc and

use safe casting operations to slot access on contexts when
possible.


git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@588 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/contexts.cc

@@ -27,6 +27,7 @@
 
 #include "v8.h"
 
+#include "bootstrapper.h"
 #include "debug.h"
 #include "scopeinfo.h"
 
@@ -99,19 +100,19 @@ Handle<Object> Context::Lookup(Handle<String> name, ContextLookupFlags flags,
     }
 
     // check extension/with object
-    Handle<JSObject> context_ext(context->extension());
-    if (*context_ext != NULL) {
+    if (context->has_extension()) {
+      Handle<JSObject> extension = Handle<JSObject>(context->extension());
       if ((flags & FOLLOW_PROTOTYPE_CHAIN) == 0) {
-        *attributes = context_ext->GetLocalPropertyAttribute(*name);
+        *attributes = extension->GetLocalPropertyAttribute(*name);
       } else {
-        *attributes = context_ext->GetPropertyAttribute(*name);
+        *attributes = extension->GetPropertyAttribute(*name);
       }
       if (*attributes != ABSENT) {
         // property found
         if (FLAG_trace_contexts) {
-          PrintF("=> found property in context object %p\n", *context_ext);
+          PrintF("=> found property in context object %p\n", *extension);
         }
-        return context_ext;
+        return extension;
       }
     }
 
@@ -184,11 +185,10 @@ Handle<Object> Context::Lookup(Handle<String> name, ContextLookupFlags flags,
     // proceed with enclosing context
     if (context->IsGlobalContext()) {
       follow_context_chain = false;
-    } else if (context->previous() != NULL) {
-      context = Handle<Context>(context->previous());
-    } else {
-      ASSERT(context->is_function_context());
+    } else if (context->is_function_context()) {
       context = Handle<Context>(Context::cast(context->closure()->context()));
+    } else {
+      context = Handle<Context>(context->previous());
     }
   } while (follow_context_chain);
 
@@ -196,8 +196,23 @@ Handle<Object> Context::Lookup(Handle<String> name, ContextLookupFlags flags,
   if (FLAG_trace_contexts) {
     PrintF("=> no property/slot found\n");
   }
-  return Handle<Object>(reinterpret_cast<JSObject*>(NULL));
+  return Handle<Object>::null();
 }
 
 
+#ifdef DEBUG
+bool Context::IsBootstrappingOrContext(Object* object) {
+  // During bootstrapping we allow all objects to pass as
+  // contexts. This is necessary to fix circular dependencies.
+  return Bootstrapper::IsActive() || object->IsContext();
+}
+
+
+bool Context::IsBootstrappingOrGlobalObject(Object* object) {
+  // During bootstrapping we allow all objects to pass as global
+  // objects. This is necessary to fix circular dependencies.
+  return Bootstrapper::IsActive() || object->IsGlobalObject();
+}
+#endif
+
 } }  // namespace v8::internal

src/contexts.h

@@ -220,23 +220,24 @@ class Context: public FixedArray {
   JSFunction* closure() { return JSFunction::cast(get(CLOSURE_INDEX)); }
   void set_closure(JSFunction* closure) { set(CLOSURE_INDEX, closure); }
 
-  Context* fcontext() {
-    return reinterpret_cast<Context*>(get(FCONTEXT_INDEX));
-  }
+  Context* fcontext() { return Context::cast(get(FCONTEXT_INDEX)); }
   void set_fcontext(Context* context) { set(FCONTEXT_INDEX, context); }
 
   Context* previous() {
-    return reinterpret_cast<Context*>(get(PREVIOUS_INDEX));
+    Object* result = unchecked_previous();
+    ASSERT(IsBootstrappingOrContext(result));
+    return reinterpret_cast<Context*>(result);
   }
   void set_previous(Context* context) { set(PREVIOUS_INDEX, context); }
 
-  JSObject* extension() {
-    return reinterpret_cast<JSObject*>(get(EXTENSION_INDEX));
-  }
+  bool has_extension() { return unchecked_extension() != NULL; }
+  JSObject* extension() { return JSObject::cast(unchecked_extension()); }
   void set_extension(JSObject* object) { set(EXTENSION_INDEX, object); }
 
   GlobalObject* global() {
-    return reinterpret_cast<GlobalObject*>(get(GLOBAL_INDEX));
+    Object* result = get(GLOBAL_INDEX);
+    ASSERT(IsBootstrappingOrGlobalObject(result));
+    return reinterpret_cast<GlobalObject*>(result);
   }
   void set_global(GlobalObject* global) { set(GLOBAL_INDEX, global); }
 
@@ -251,7 +252,7 @@ class Context: public FixedArray {
   Context* global_context();
 
   // Tells if this is a function context (as opposed to a 'with' context).
-  bool is_function_context() { return previous() == NULL; }
+  bool is_function_context() { return unchecked_previous() == NULL; }
 
   // Tells whether the global context is marked with out of memory.
   bool has_out_of_memory() {
@@ -293,7 +294,7 @@ class Context: public FixedArray {
   //    and the name is the property name, and the property exists.
   //    attributes != ABSENT.
   //
-  // 4) index_ < 0 && result.deref() == NULL:
+  // 4) index_ < 0 && result.is_null():
   //    there was no context found with the corresponding property.
   //    attributes == ABSENT.
   Handle<Object> Lookup(Handle<String> name, ContextLookupFlags flags,
@@ -303,6 +304,17 @@ class Context: public FixedArray {
   static int SlotOffset(int index) {
     return kHeaderSize + index * kPointerSize - kHeapObjectTag;
   }
+
+ private:
+  // Unchecked access to the slots.
+  Object* unchecked_previous() { return get(PREVIOUS_INDEX); }
+  Object* unchecked_extension() { return get(EXTENSION_INDEX); }
+
+#ifdef DEBUG
+  // Bootstrapping-aware type checks.
+  static bool IsBootstrappingOrContext(Object* object);
+  static bool IsBootstrappingOrGlobalObject(Object* object);
+#endif
 };
 
 } }  // namespace v8::internal

src/objects.cc

@@ -2139,7 +2139,7 @@ bool JSObject::ReferencesObject(Object* obj) {
     }
 
     // Check the context extension if any.
-    if (context->extension() != NULL) {
+    if (context->has_extension()) {
       return context->extension()->ReferencesObject(obj);
     }
   }

src/runtime.cc

@@ -504,7 +504,7 @@ static Object* Runtime_DeclareContextSlot(Arguments args) {
     // "declared" in the function context's extension context, or in the
     // global context.
     Handle<JSObject> context_ext;
-    if (context->extension() != NULL) {
+    if (context->has_extension()) {
       // The function context's extension context exists - use it.
       context_ext = Handle<JSObject>(context->extension());
     } else {
@@ -3347,7 +3347,7 @@ static Object* Runtime_LookupContext(Arguments args) {
   Handle<Object> holder =
       context->Lookup(name, flags, &index, &attributes);
 
-  if (index < 0 && *holder != NULL) {
+  if (index < 0 && !holder.is_null()) {
     ASSERT(holder->IsJSObject());
     return *holder;
   }
@@ -3364,62 +3364,48 @@ static Object* Runtime_LookupContext(Arguments args) {
 // compiler to do the right thing.
 //
 // TODO(1236026): This is a non-portable hack that should be removed.
-typedef uint64_t ObjPair;
-ObjPair MakePair(Object* x, Object* y) {
+typedef uint64_t ObjectPair;
+static inline ObjectPair MakePair(Object* x, Object* y) {
   return reinterpret_cast<uint32_t>(x) |
-         (reinterpret_cast<ObjPair>(y) << 32);
+      (reinterpret_cast<ObjectPair>(y) << 32);
 }
 
 
-static Object* Unhole(Object* x, PropertyAttributes attributes) {
+static inline Object* Unhole(Object* x, PropertyAttributes attributes) {
   ASSERT(!x->IsTheHole() || (attributes & READ_ONLY) != 0);
   USE(attributes);
   return x->IsTheHole() ? Heap::undefined_value() : x;
 }
 
 
-static Object* ComputeContextSlotReceiver(Object* holder) {
-  // If the "property" we were looking for is a local variable or an
-  // argument in a context, the receiver is the global object; see
-  // ECMA-262, 3rd., 10.1.6 and 10.2.3.
-  // Contexts and global objects are most common.
-  if (holder->IsContext()) {
-    return Context::cast(holder)->global()->global_receiver();
-  }
-  if (holder->IsGlobalObject()) {
-    // If the holder is a global object, we have to be careful to wrap
-    // it in its proxy if necessary.
-    return GlobalObject::cast(holder)->global_receiver();
-  }
-
+static JSObject* ComputeReceiverForNonGlobal(JSObject* holder) {
+  ASSERT(!holder->IsGlobalObject());
   Context* top = Top::context();
-  // TODO(125): Find a better - and faster way - of checking for
-  // arguments and context extension objects. This kinda sucks.
+  // Get the context extension function.
   JSFunction* context_extension_function =
       top->global_context()->context_extension_function();
-  JSObject* arguments_boilerplate =
-      top->global_context()->arguments_boilerplate();
-  JSFunction* arguments_function =
-      JSFunction::cast(arguments_boilerplate->map()->constructor());
-  // If the holder is an arguments object or a context extension then the
-  // receiver is also the global object;
-  Object* constructor = HeapObject::cast(holder)->map()->constructor();
-  if (constructor == context_extension_function ||
-      constructor == arguments_function) {
-    return Top::context()->global()->global_receiver();
-  }
-
-  return holder;
+  // If the holder isn't a context extension object, we just return it
+  // as the receiver. This allows arguments objects to be used as
+  // receivers, but only if they are put in the context scope chain
+  // explicitly via a with-statement.
+  Object* constructor = holder->map()->constructor();
+  if (constructor != context_extension_function) return holder;
+  // Fall back to using the global object as the receiver if the
+  // property turns out to be a local variable allocated in a context
+  // extension object - introduced via eval.
+  return top->global()->global_receiver();
 }
 
 
-static ObjPair LoadContextSlotHelper(Arguments args, bool throw_error) {
+static ObjectPair LoadContextSlotHelper(Arguments args, bool throw_error) {
   HandleScope scope;
   ASSERT(args.length() == 2);
 
-  if (!args[0]->IsContext()) return MakePair(IllegalOperation(), NULL);
+  if (!args[0]->IsContext() || !args[1]->IsString()) {
+    return MakePair(IllegalOperation(), NULL);
+  }
   Handle<Context> context = args.at<Context>(0);
-  Handle<String> name(String::cast(args[1]));
+  Handle<String> name = args.at<String>(1);
 
   int index;
   PropertyAttributes attributes;
@@ -3427,29 +3413,31 @@ static ObjPair LoadContextSlotHelper(Arguments args, bool throw_error) {
   Handle<Object> holder =
       context->Lookup(name, flags, &index, &attributes);
 
+  // If the index is non-negative, the slot has been found in a local
+  // variable or a parameter. Read it from the context object or the
+  // arguments object.
   if (index >= 0) {
-    Handle<Object> receiver =
-        Handle<Object>(ComputeContextSlotReceiver(*holder));
-    Handle<Object> value;
-    if (holder->IsContext()) {
-      value = Handle<Object>(Context::cast(*holder)->get(index));
-    } else {
-      // Arguments object.
-      value = Handle<Object>(JSObject::cast(*holder)->GetElement(index));
-    }
-    return MakePair(Unhole(*value, attributes), *receiver);
-  }
-
-  if (*holder != NULL) {
-    ASSERT(Handle<JSObject>::cast(holder)->HasProperty(*name));
-    // Note: As of 5/29/2008, GetProperty does the "unholing" and so
-    // this call here is redundant. We left it anyway, to be explicit;
-    // also it's not clear why GetProperty should do the unholing in
-    // the first place.
-    return MakePair(
-        Unhole(Handle<JSObject>::cast(holder)->GetProperty(*name),
-               attributes),
-        ComputeContextSlotReceiver(*holder));
+    // If the "property" we were looking for is a local variable or an
+    // argument in a context, the receiver is the global object; see
+    // ECMA-262, 3rd., 10.1.6 and 10.2.3.
+    JSObject* receiver = Top::context()->global()->global_receiver();
+    Object* value = (holder->IsContext())
+        ? Context::cast(*holder)->get(index)
+        : JSObject::cast(*holder)->GetElement(index);
+    return MakePair(Unhole(value, attributes), receiver);
+  }
+
+  // If the holder is found, we read the property from it.
+  if (!holder.is_null() && holder->IsJSObject()) {
+    JSObject* object = JSObject::cast(*holder);
+    ASSERT(object->HasProperty(*name));
+    JSObject* receiver = (object->IsGlobalObject())
+        ? GlobalObject::cast(object)->global_receiver()
+        : ComputeReceiverForNonGlobal(object);
+    // No need to unhole the value here. This is taken care of by the
+    // GetProperty function.
+    Object* value = object->GetProperty(*name);
+    return MakePair(value, receiver);
   }
 
   if (throw_error) {
@@ -3464,12 +3452,12 @@ static ObjPair LoadContextSlotHelper(Arguments args, bool throw_error) {
 }
 
 
-static ObjPair Runtime_LoadContextSlot(Arguments args) {
+static ObjectPair Runtime_LoadContextSlot(Arguments args) {
   return LoadContextSlotHelper(args, true);
 }
 
 
-static ObjPair Runtime_LoadContextSlotNoReferenceError(Arguments args) {
+static ObjectPair Runtime_LoadContextSlotNoReferenceError(Arguments args) {
   return LoadContextSlotHelper(args, false);
 }
 
@@ -3508,7 +3496,7 @@ static Object* Runtime_StoreContextSlot(Arguments args) {
   // It is either in an JSObject extension context or it was not found.
   Handle<JSObject> context_ext;
 
-  if (*holder != NULL) {
+  if (!holder.is_null()) {
     // The property exists in the extension context.
     context_ext = Handle<JSObject>::cast(holder);
   } else {
@@ -4508,7 +4496,7 @@ static Object* Runtime_GetFrameDetails(Arguments args) {
       Handle<String> name = info.LocalName(i);
       // Traverse the context chain to the function context as all local
       // variables stored in the context will be on the function context.
-      while (context->previous() != NULL) {
+      while (!context->is_function_context()) {
         context = Handle<Context>(context->previous());
       }
       ASSERT(context->is_function_context());
@@ -5024,7 +5012,7 @@ static Object* Runtime_DebugEvaluate(Arguments args) {
   }
   // Finally copy any properties from the function context extension. This will
   // be variables introduced by eval.
-  if (function_context->extension() != NULL &&
+  if (function_context->has_extension() &&
       !function_context->IsGlobalContext()) {
     Handle<JSObject> ext(JSObject::cast(function_context->extension()));
     Handle<FixedArray> keys = GetKeysInFixedArrayFor(ext);

test/mjsunit/regress/regress-124.js

@@ -49,6 +49,9 @@ function F(f) {
 
   // Receiver should be the arguments object here.
   assertEquals("[object Object]", eval("arguments[0]()"));
+  with (arguments) {
+    assertEquals("[object Object]", toString());
+  }
 }
 
 F(Object.prototype.toString);