From c128b8d9de2b2ebce4ef016e444231db3086fc5d Mon Sep 17 00:00:00 2001 From: "kasperl@chromium.org" Date: Fri, 24 Oct 2008 10:59:40 +0000 Subject: [PATCH] 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 | 37 ++++++--- src/contexts.h | 32 +++++--- src/objects.cc | 2 +- src/runtime.cc | 118 +++++++++++++--------------- test/mjsunit/regress/regress-124.js | 3 + 5 files changed, 105 insertions(+), 87 deletions(-) diff --git a/src/contexts.cc b/src/contexts.cc index 81ad8d2c5..6aaa4ccf3 100644 --- a/src/contexts.cc +++ b/src/contexts.cc @@ -27,6 +27,7 @@ #include "v8.h" +#include "bootstrapper.h" #include "debug.h" #include "scopeinfo.h" @@ -99,19 +100,19 @@ Handle Context::Lookup(Handle name, ContextLookupFlags flags, } // check extension/with object - Handle context_ext(context->extension()); - if (*context_ext != NULL) { + if (context->has_extension()) { + Handle extension = Handle(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 Context::Lookup(Handle name, ContextLookupFlags flags, // proceed with enclosing context if (context->IsGlobalContext()) { follow_context_chain = false; - } else if (context->previous() != NULL) { - context = Handle(context->previous()); - } else { - ASSERT(context->is_function_context()); + } else if (context->is_function_context()) { context = Handle(Context::cast(context->closure()->context())); + } else { + context = Handle(context->previous()); } } while (follow_context_chain); @@ -196,8 +196,23 @@ Handle Context::Lookup(Handle name, ContextLookupFlags flags, if (FLAG_trace_contexts) { PrintF("=> no property/slot found\n"); } - return Handle(reinterpret_cast(NULL)); + return Handle::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 diff --git a/src/contexts.h b/src/contexts.h index 2e6c39ad5..30aa5bb9f 100644 --- a/src/contexts.h +++ b/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(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(get(PREVIOUS_INDEX)); + Object* result = unchecked_previous(); + ASSERT(IsBootstrappingOrContext(result)); + return reinterpret_cast(result); } void set_previous(Context* context) { set(PREVIOUS_INDEX, context); } - JSObject* extension() { - return reinterpret_cast(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(get(GLOBAL_INDEX)); + Object* result = get(GLOBAL_INDEX); + ASSERT(IsBootstrappingOrGlobalObject(result)); + return reinterpret_cast(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 Lookup(Handle 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 diff --git a/src/objects.cc b/src/objects.cc index 5be631b43..0be6d283f 100644 --- a/src/objects.cc +++ b/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); } } diff --git a/src/runtime.cc b/src/runtime.cc index c19e3ee1a..dcd2b0182 100644 --- a/src/runtime.cc +++ b/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 context_ext; - if (context->extension() != NULL) { + if (context->has_extension()) { // The function context's extension context exists - use it. context_ext = Handle(context->extension()); } else { @@ -3347,7 +3347,7 @@ static Object* Runtime_LookupContext(Arguments args) { Handle 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(x) | - (reinterpret_cast(y) << 32); + (reinterpret_cast(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 = args.at(0); - Handle name(String::cast(args[1])); + Handle name = args.at(1); int index; PropertyAttributes attributes; @@ -3427,29 +3413,31 @@ static ObjPair LoadContextSlotHelper(Arguments args, bool throw_error) { Handle 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 receiver = - Handle(ComputeContextSlotReceiver(*holder)); - Handle value; - if (holder->IsContext()) { - value = Handle(Context::cast(*holder)->get(index)); - } else { - // Arguments object. - value = Handle(JSObject::cast(*holder)->GetElement(index)); - } - return MakePair(Unhole(*value, attributes), *receiver); - } - - if (*holder != NULL) { - ASSERT(Handle::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::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 context_ext; - if (*holder != NULL) { + if (!holder.is_null()) { // The property exists in the extension context. context_ext = Handle::cast(holder); } else { @@ -4508,7 +4496,7 @@ static Object* Runtime_GetFrameDetails(Arguments args) { Handle 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->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 ext(JSObject::cast(function_context->extension())); Handle keys = GetKeysInFixedArrayFor(ext); diff --git a/test/mjsunit/regress/regress-124.js b/test/mjsunit/regress/regress-124.js index 0c7e3005a..81526b0ed 100644 --- a/test/mjsunit/regress/regress-124.js +++ b/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); -- 2.34.1