CallIC and KeyedCallIC not wrapping this.

src/ic.cc

 // Copyright 2006-2009 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 417 matching lines @@
     StackFrameLocator locator;
     JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);
     int index = frame->ComputeExpressionsCount() - (argc + 1);
     frame->SetExpression(index, *target);
   }
 
   return *delegate;
 }
 
 
-void CallICBase::ReceiverToObject(Handle<Object> object) {
-  HandleScope scope;
-  Handle<Object> receiver(object);
+void CallICBase::ReceiverToObjectIfRequired(Handle<Object> callee,
+                                            Handle<Object> object) {
+  if (callee->IsJSFunction()) {
+    JSFunction* function = JSFunction::cast(*callee);
+    if (function->shared()->strict_mode() || function->IsBuiltin()) {
+      // Do not wrap receiver for strict mode functions or for builtins.
+      return;
+    }
+  }
 
-  // Change the receiver to the result of calling ToObject on it.
-  const int argc = this->target()->arguments_count();
-  StackFrameLocator locator;
-  JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);
-  int index = frame->ComputeExpressionsCount() - (argc + 1);
-  frame->SetExpression(index, *Factory::ToObject(object));
+  // And only wrap string, number or boolean.
+  if (object->IsString() || object->IsNumber() || object->IsBoolean()) {

[Martin Maly, 2011/02/18 00:55:38]

I don't understand why the object was double-wrapped in a handle. Based on my
understanding it is not necessary but if it was necessary I wonder why the
Handle<Object> object which arrives straight from the IC. I am assuming GC will
discover that object by walking the stack.

[Mads Ager (chromium), 2011/02/18 07:22:48]

Yes, there is no reason to rewrap the incoming objects.

+    // Change the receiver to the result of calling ToObject on it.
+    const int argc = this->target()->arguments_count();
+    StackFrameLocator locator;
+    JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);
+    int index = frame->ComputeExpressionsCount() - (argc + 1);
+    frame->SetExpression(index, *Factory::ToObject(object));
+  }
 }
 
 
 MaybeObject* CallICBase::LoadFunction(State state,
                                       Code::ExtraICState extra_ic_state,
                                       Handle<Object> object,
                                       Handle<String> name) {
   // If the object is undefined or null it's illegal to try to get any
   // of its properties; throw a TypeError in that case.
   if (object->IsUndefined() || object->IsNull()) {
     return TypeError("non_object_property_call", object, name);
   }
 
-  if (object->IsString() || object->IsNumber() || object->IsBoolean()) {
-    ReceiverToObject(object);
-  }
-
   // Check if the name is trivially convertible to an index and get
   // the element if so.
   uint32_t index;
   if (name->AsArrayIndex(&index)) {
     Object* result;
     { MaybeObject* maybe_result = object->GetElement(index);
       if (!maybe_result->ToObject(&result)) return maybe_result;
     }
 
     if (result->IsJSFunction()) return result;
@@ skipping 18 matching lines @@
     return TypeError("undefined_method", object, name);
   }
 
   // Lookup is valid: Update inline cache and stub cache.
   if (FLAG_use_ic) {
     UpdateCaches(&lookup, state, extra_ic_state, object, name);
   }
 
   // Get the property.
   PropertyAttributes attr;
-  Object* result;
+  HandleScope scope;
+  Handle<Object> result;

[Mads Ager (chromium), 2011/02/18 07:22:48]

I would move the object_result out here (maybe call it result and call the
handle result_handle.) Then only create the HandleScope after the GetProperty
and ToObject part.

Then you can add a comment that you wrap the result in a handle because
ReceiverToObjectIfRequired can cause a GC.

It would be nice to just use a handlified version of GetProperty but that would
redo work that you already did, so I would probably leave the raw pointers stuff
in here.

[Martin Maly, 2011/02/19 01:26:13]

Done.

   { MaybeObject* maybe_result =
         object->GetProperty(*object, &lookup, *name, &attr);
-    if (!maybe_result->ToObject(&result)) return maybe_result;
+    Object* object_result;
+    if (!maybe_result->ToObject(&object_result)) return maybe_result;
+    result = Handle<Object>(object_result);
   }
+
+  // Make receiver an object if the callee requires it. Strict mode or builtin
+  // functions do not wrap the receiver, non-strict functions and objects
+  // called as functions do.
+  ReceiverToObjectIfRequired(result, object);
+
   if (lookup.type() == INTERCEPTOR) {

[Mads Ager (chromium), 2011/02/18 07:22:48]

It looks like you can move the wrapping below this exception throwing step as
well. If you throw an exception from here, there is no reason to wrap the
receiver. However, the more important part is that LookupResults are not GC
safe. I believe it is safe for the things you are using here (not accessing any
heap object pointers stored on the LookupResult). In general you have to be
careful with LookupResults. To avoid having to think about it I would move the
wrapping to after the final checks on lookup.

[Martin Maly, 2011/02/19 01:26:13]

Done.

     // If the object does not have the requested property, check which
     // exception we need to throw.
     if (attr == ABSENT) {
       if (IsContextual(object)) {
         return ReferenceError("not_defined", name);
       }
       return TypeError("undefined_method", object, name);
     }
   }
 
-  ASSERT(result != Heap::the_hole_value());
+  ASSERT(*result != Heap::the_hole_value());

[Mads Ager (chromium), 2011/02/18 07:22:48]

ASSERT(!result->IsTheHole());

[Martin Maly, 2011/02/19 01:26:13]

Done.

 
   if (result->IsJSFunction()) {
 #ifdef ENABLE_DEBUGGER_SUPPORT
     // Handle stepping into a function if step into is active.
     if (Debug::StepInActive()) {
       // Protect the result in a handle as the debugger can allocate and might
       // cause GC.
-      HandleScope scope;
-      Handle<JSFunction> function(JSFunction::cast(result));
+      Handle<JSFunction> function(JSFunction::cast(*result));

[Martin Maly, 2011/02/18 00:55:38]

No HandleScope needed here, I believe ... the Handle will simply use the one
above (line 508).

[Mads Ager (chromium), 2011/02/18 07:22:48]

Yes, that's fine.

       Debug::HandleStepIn(function, object, fp(), false);
       return *function;
     }
 #endif
 
-    return result;
+    return *result;
   }
 
   // Try to find a suitable function delegate for the object at hand.
-  result = TryCallAsFunction(result);
-  MaybeObject* answer = result;
-  if (!result->IsJSFunction()) {
-    answer = TypeError("property_not_function", object, name);
-  }
-  return answer;
+  result = Handle<Object>(TryCallAsFunction(*result));
+  if (result->IsJSFunction()) return *result;

[Martin Maly, 2011/02/18 00:55:38]

I am a bit hazy on using
"return handle.EscapeFrom(&scope)"
vs.
"return *handle;"
I saw both in the codebase...

[Mads Ager (chromium), 2011/02/18 07:22:48]

This method returns a MaybeObject which is a raw object pointer. It can either
be a Smi, an Object pointer or a failure. The caller will deal with failures
(either throwing exceptions or garbage collecting or just returning the result).
So here, you just have to extract the raw Object* from the handle and return
that. This is done with "return *handle".

handle.EscapeFrom(&scope) is used rarely. It is used in situations where you
have a local handle scope and you want to return one of those handles to the
caller (who has his own handle scope). If you just return the handle you will
get into trouble because that handle belongs to your local handle scope. Once
the local handle scope gets destructed as part of returning, the GC no longer
knows about the value in your handle and you are no better off than with a raw
pointer. handle.EscapeFrom(&scope) moves the handle from the current scope to
the outer scope which makes it safe to return it. The outer scope is still
active after returning and the GC will therefore update the pointer in the
handle.

+
+  return TypeError("property_not_function", object, name);
 }
 
 
 bool CallICBase::TryUpdateExtraICState(LookupResult* lookup,
                                        Handle<Object> object,
                                        Code::ExtraICState* extra_ic_state) {
   ASSERT(kind_ == Code::CALL_IC);
   if (lookup->type() != CONSTANT_FUNCTION) return false;
   JSFunction* function = lookup->GetConstantFunction();
   if (!function->shared()->HasBuiltinFunctionId()) return false;
 
   // Fetch the arguments passed to the called function.
   const int argc = target()->arguments_count();
   Address entry = Top::c_entry_fp(Top::GetCurrentThread());
   Address fp = Memory::Address_at(entry + ExitFrameConstants::kCallerFPOffset);
   Arguments args(argc + 1,
                  &Memory::Object_at(fp +
                                     StandardFrameConstants::kCallerSPOffset +
                                     argc * kPointerSize));
   switch (function->shared()->builtin_function_id()) {
     case kStringCharCodeAt:
     case kStringCharAt:
       if (object->IsString()) {
         String* string = String::cast(*object);
-        // Check that there's the right wrapper in the receiver slot.
-        ASSERT(string == JSValue::cast(args[0])->value());
+        // Check there's the right string value or wrapper in the receiver slot.
+        ASSERT(string == args[0] || string == JSValue::cast(args[0])->value());
         // If we're in the default (fastest) state and the index is
         // out of bounds, update the state to record this fact.
         if (*extra_ic_state == DEFAULT_STRING_STUB &&
             argc >= 1 && args[1]->IsNumber()) {
           double index;
           if (args[1]->IsSmi()) {
             index = Smi::cast(args[1])->value();
           } else {
             ASSERT(args[1]->IsHeapNumber());
             index = DoubleToInteger(HeapNumber::cast(args[1])->value());
@@ skipping 188 matching lines @@
     return CallICBase::LoadFunction(state,
                                     Code::kNoExtraICState,
                                     object,
                                     Handle<String>::cast(key));
   }
 
   if (object->IsUndefined() || object->IsNull()) {
     return TypeError("non_object_property_call", object, key);
   }
 
-  if (object->IsString() || object->IsNumber() || object->IsBoolean()) {
-    ReceiverToObject(object);
-  }
-
   if (FLAG_use_ic && state != MEGAMORPHIC && !object->IsAccessCheckNeeded()) {
     int argc = target()->arguments_count();
     InLoopFlag in_loop = target()->ic_in_loop();
     MaybeObject* maybe_code = StubCache::ComputeCallMegamorphic(
         argc, in_loop, Code::KEYED_CALL_IC);
     Object* code;
     if (maybe_code->ToObject(&code)) {
       set_target(Code::cast(code));
 #ifdef DEBUG
       TraceIC(
           "KeyedCallIC", key, state, target(), in_loop ? " (in-loop)" : "");
 #endif
     }
   }
-  Object* result;
+
+  HandleScope scope;
+  Handle<Object> result;

[Mads Ager (chromium), 2011/02/18 07:22:48]

This case can be rewritten to just use handles. You don't need any of the
maybe_object stuff below and you can just call the handlified version of
GetObjectProperty (defined in handles.[h,cc]:

Handle<Object> result = GetObjectProperty(object, key);

[Martin Maly, 2011/02/19 01:26:13]

Done.

   { MaybeObject* maybe_result = Runtime::GetObjectProperty(object, key);
-    if (!maybe_result->ToObject(&result)) return maybe_result;
+    Object* object_result;
+    if (!maybe_result->ToObject(&object_result)) return maybe_result;
+    result = Handle<Object>(object_result);

[Martin Maly, 2011/02/18 00:55:38]

Is there an established V8 pattern to export objects out of the nested scope and
Handle-ify it?

   }
-  if (result->IsJSFunction()) return result;
-  result = TryCallAsFunction(result);
-  MaybeObject* answer = result;
-  if (!result->IsJSFunction()) {
-    answer = TypeError("property_not_function", object, key);
-  }
-  return answer;
+
+  // Make receiver an object if the callee requires it. Strict mode or builtin
+  // functions do not wrap the receiver, non-strict functions and objects
+  // called as functions do.
+  ReceiverToObjectIfRequired(result, object);
+
+  if (result->IsJSFunction()) return *result;
+  result = Handle<Object>(TryCallAsFunction(*result));
+  if (result->IsJSFunction()) return *result;
+
+  return TypeError("property_not_function", object, key);

[Martin Maly, 2011/02/18 00:55:38]

This sequence seemed cleaner, get rid of "answer" altogether.

[Mads Ager (chromium), 2011/02/18 07:22:48]

Yes, thanks for cleaning that up! The original code was very strange. :)

 }
 
 
 #ifdef DEBUG
 #define TRACE_IC_NAMED(msg, name) \
   if (FLAG_trace_ic) PrintF(msg, *(name)->ToCString())
 #else
 #define TRACE_IC_NAMED(msg, name)
 #endif
 
@@ skipping 1469 matching lines @@
 #undef ADDR
 };
 
 
 Address IC::AddressFromUtilityId(IC::UtilityId id) {
   return IC_utilities[id];
 }
 
 
 } }  // namespace v8::internal