Fix accessor lookup in crankshaft.

src/hydrogen.cc

 // Copyright 2012 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 4644 matching lines @@
 
   HRegExpLiteral* instr = new(zone()) HRegExpLiteral(context,
                                                      literals,
                                                      expr->pattern(),
                                                      expr->flags(),
                                                      expr->literal_index());
   return ast_context()->ReturnInstruction(instr, expr->id());
 }
 
 
+static void LookupInPrototypes(Handle<Map> map,
+                               Handle<String> name,
+                               LookupResult* lookup) {
+  while (map->prototype()->IsJSObject()) {
+    Handle<JSObject> holder(JSObject::cast(map->prototype()));
+    if (!holder->HasFastProperties()) break;
+    map = Handle<Map>(holder->map());
+    map->LookupDescriptor(*holder, *name, lookup);
+    if (lookup->IsFound()) return;
+  }
+  lookup->NotFound();
+}
+
+
+// Tries to find a JavaScript accessor of the given name in the prototype chain
+// starting at the given map. Return true iff there is one, including the
+// corresponding AccessorPair plus its holder (which could be null when the
+// accessor is found directly in the given map).
+static bool LookupAccessorPair(Handle<Map> map,
+                               Handle<String> name,
+                               Handle<AccessorPair>* accessors,
+                               Handle<JSObject>* holder) {
+  LookupResult lookup(map->GetIsolate());
+
+  // Check for a JavaScript accessor directly in the map.
+  map->LookupDescriptor(NULL, *name, &lookup);
+  if (lookup.IsPropertyCallbacks()) {
+    Handle<Object> callback(lookup.GetValueFromMap(*map));
+    if (!callback->IsAccessorPair()) return false;
+    *accessors = Handle<AccessorPair>::cast(callback);
+    *holder = Handle<JSObject>();
+    return true;
+  }
+
+  // Everything else, e.g. a field, can't be an accessor call.
+  if (lookup.IsFound()) return false;
+
+  // Check for a JavaScript accessor somewhere in the proto chain.
+  LookupInPrototypes(map, name, &lookup);
+  if (lookup.IsPropertyCallbacks()) {
+    Handle<Object> callback(lookup.GetValue());
+    if (!callback->IsAccessorPair()) return false;
+    *accessors = Handle<AccessorPair>::cast(callback);
+    *holder = Handle<JSObject>(lookup.holder());
+    return true;
+  }
+
+  // We haven't found a JavaScript accessor anywhere.
+  return false;
+}
+
+
+static bool LookupGetter(Handle<Map> map,
+                         Handle<String> name,
+                         Handle<JSFunction>* getter,
+                         Handle<JSObject>* holder) {
+  Handle<AccessorPair> accessors;
+  if (LookupAccessorPair(map, name, &accessors, holder) &&
+      (*accessors)->getter()->IsJSFunction()) {

[Michael Starzinger, 2012/08/16 09:55:58]

No need to do open the handle here, that will be done implicitly, just use ...

accessors->getter()->IsJSFunction()

[Sven Panne, 2012/08/16 10:47:40]

Done.

+    *getter = Handle<JSFunction>(JSFunction::cast((*accessors)->getter()));

[Michael Starzinger, 2012/08/16 09:55:58]

Likewise.

[Sven Panne, 2012/08/16 10:47:40]

Done.

+    return true;
+  }
+  return false;
+}
+
+
+static bool LookupSetter(Handle<Map> map,
+                         Handle<String> name,
+                         Handle<JSFunction>* setter,
+                         Handle<JSObject>* holder) {
+  Handle<AccessorPair> accessors;
+  if (LookupAccessorPair(map, name, &accessors, holder) &&
+      (*accessors)->setter()->IsJSFunction()) {

[Michael Starzinger, 2012/08/16 09:55:58]

Likewise.

[Sven Panne, 2012/08/16 10:47:40]

Done.

+    *setter = Handle<JSFunction>(JSFunction::cast((*accessors)->setter()));

[Michael Starzinger, 2012/08/16 09:55:58]

Likewise.

[Sven Panne, 2012/08/16 10:47:40]

Done.

+    return true;
+  }
+  return false;
+}
+
+
 // Determines whether the given array or object literal boilerplate satisfies
 // all limits to be considered for fast deep-copying and computes the total
 // size of all objects that are part of the graph.
 static bool IsFastLiteral(Handle<JSObject> boilerplate,
                           int max_depth,
                           int* max_properties,
                           int* total_size) {
   ASSERT(max_depth >= 0 && *max_properties >= 0);
   if (max_depth == 0) return false;
 
@@ skipping 105 matching lines @@
             CHECK_ALIVE(VisitForValue(value));
             HValue* value = Pop();
             Handle<Map> map = property->GetReceiverType();
             Handle<String> name = property->key()->AsPropertyName();
             HInstruction* store;
             if (map.is_null()) {
               // If we don't know the monomorphic type, do a generic store.
               CHECK_ALIVE(store = BuildStoreNamedGeneric(literal, name, value));
             } else {
 #if DEBUG
-              Handle<AccessorPair> accessors;
+              Handle<JSFunction> setter;
               Handle<JSObject> holder;
-              ASSERT(!LookupAccessorPair(map, name, &accessors, &holder));
+              ASSERT(!LookupSetter(map, name, &setter, &holder));
 #endif
               CHECK_ALIVE(store = BuildStoreNamedMonomorphic(literal,
                                                              name,
                                                              value,
                                                              map));
             }
             AddInstruction(store);
             if (store->HasObservableSideEffects()) AddSimulate(key->id());
           } else {
             CHECK_ALIVE(VisitForEffect(value));
@@ skipping 228 matching lines @@
   HValue* context = environment()->LookupContext();
   return new(zone()) HStoreNamedGeneric(
                          context,
                          object,
                          name,
                          value,
                          function_strict_mode_flag());
 }
 
 
-static void LookupInPrototypes(Handle<Map> map,
-                               Handle<String> name,
-                               LookupResult* lookup) {
-  while (map->prototype()->IsJSObject()) {
-    Handle<JSObject> holder(JSObject::cast(map->prototype()));
-    if (!holder->HasFastProperties()) break;
-    map = Handle<Map>(holder->map());
-    map->LookupDescriptor(*holder, *name, lookup);
-    if (lookup->IsFound()) return;
-  }
-  lookup->NotFound();
-}
-
-
 HInstruction* HGraphBuilder::BuildCallSetter(HValue* object,
                                              HValue* value,
                                              Handle<Map> map,
-                                             Handle<AccessorPair> accessors,
+                                             Handle<JSFunction> setter,
                                              Handle<JSObject> holder) {
-  Handle<JSFunction> setter(JSFunction::cast(accessors->setter()));
   AddCheckConstantFunction(holder, object, map, true);
   AddInstruction(new(zone()) HPushArgument(object));
   AddInstruction(new(zone()) HPushArgument(value));
   return new(zone()) HCallConstantFunction(setter, 2);
 }
 
 
 HInstruction* HGraphBuilder::BuildStoreNamedMonomorphic(HValue* object,
                                                         Handle<String> name,
                                                         HValue* value,
@@ skipping 159 matching lines @@
 
     HInstruction* instr = NULL;
     SmallMapList* types = expr->GetReceiverTypes();
     bool monomorphic = expr->IsMonomorphic();
     Handle<Map> map;
     if (monomorphic) {
       map = types->first();
       if (map->is_dictionary_map()) monomorphic = false;
     }
     if (monomorphic) {
-      Handle<AccessorPair> accessors;
+      Handle<JSFunction> setter;
       Handle<JSObject> holder;
-      if (LookupAccessorPair(map, name, &accessors, &holder)) {
-        Handle<JSFunction> setter(JSFunction::cast(accessors->setter()));
+      if (LookupSetter(map, name, &setter, &holder)) {
         AddCheckConstantFunction(holder, object, map, true);
         if (FLAG_inline_accessors && TryInlineSetter(setter, expr, value)) {
           return;
         }
         Drop(2);
         AddInstruction(new(zone()) HPushArgument(object));
         AddInstruction(new(zone()) HPushArgument(value));
         instr = new(zone()) HCallConstantFunction(setter, 2);
       } else {
         Drop(2);
@@ skipping 166 matching lines @@
       Handle<Map> map;
       HInstruction* load;
       bool monomorphic = prop->IsMonomorphic();
       if (monomorphic) {
         map = prop->GetReceiverTypes()->first();
         // We can't generate code for a monomorphic dict mode load so
         // just pretend it is not monomorphic.
         if (map->is_dictionary_map()) monomorphic = false;
       }
       if (monomorphic) {
-        Handle<AccessorPair> accessors;
+        Handle<JSFunction> getter;
         Handle<JSObject> holder;
-        if (LookupAccessorPair(map, name, &accessors, &holder)) {
-          load = BuildCallGetter(object, map, accessors, holder);
+        if (LookupGetter(map, name, &getter, &holder)) {
+          load = BuildCallGetter(object, map, getter, holder);
         } else {
           load = BuildLoadNamedMonomorphic(object, name, prop, map);
         }
       } else {
         load = BuildLoadNamedGeneric(object, name, prop);
       }
       PushAndAdd(load);
       if (load->HasObservableSideEffects()) AddSimulate(prop->LoadId());
 
       CHECK_ALIVE(VisitForValue(expr->value()));
       HValue* right = Pop();
       HValue* left = Pop();
 
       HInstruction* instr = BuildBinaryOperation(operation, left, right);
       PushAndAdd(instr);
       if (instr->HasObservableSideEffects()) AddSimulate(operation->id());
 
       HInstruction* store;
       if (!monomorphic) {
         // If we don't know the monomorphic type, do a generic store.
         CHECK_ALIVE(store = BuildStoreNamedGeneric(object, name, instr));
       } else {
-        Handle<AccessorPair> accessors;
+        Handle<JSFunction> setter;
         Handle<JSObject> holder;
         // Because we re-use the load type feedback, there might be no setter.
-        if (LookupAccessorPair(map, name, &accessors, &holder) &&
-            accessors->setter()->IsJSFunction()) {
-          store = BuildCallSetter(object, instr, map, accessors, holder);
+        if (LookupSetter(map, name, &setter, &holder)) {
+          store = BuildCallSetter(object, instr, map, setter, holder);
         } else {
           CHECK_ALIVE(store = BuildStoreNamedMonomorphic(object,
                                                          name,
                                                          instr,
                                                          map));
         }
       }
       AddInstruction(store);
       // Drop the simulated receiver and value.  Return the value.
       Drop(2);
@@ skipping 223 matching lines @@
     AddInstruction(new(zone()) HSoftDeoptimize);
     current_block()->MarkAsDeoptimizing();
   }
   HValue* context = environment()->LookupContext();
   return new(zone()) HLoadNamedGeneric(context, object, name);
 }
 
 
 HInstruction* HGraphBuilder::BuildCallGetter(HValue* object,
                                              Handle<Map> map,
-                                             Handle<AccessorPair> accessors,
+                                             Handle<JSFunction> getter,
                                              Handle<JSObject> holder) {
-  Handle<JSFunction> getter(JSFunction::cast(accessors->getter()));
   AddCheckConstantFunction(holder, object, map, true);
   AddInstruction(new(zone()) HPushArgument(object));
   return new(zone()) HCallConstantFunction(getter, 1);
 }
 
 
-bool HGraphBuilder::LookupAccessorPair(Handle<Map> map,
-                                       Handle<String> name,
-                                       Handle<AccessorPair>* accessors,
-                                       Handle<JSObject>* holder) {
-  LookupResult lookup(isolate());
-
-  // Check for a JavaScript accessor directly in the map.
-  map->LookupDescriptor(NULL, *name, &lookup);
-  if (lookup.IsPropertyCallbacks()) {
-    Handle<Object> callback(lookup.GetValueFromMap(*map));
-    if (!callback->IsAccessorPair()) return false;
-    *accessors = Handle<AccessorPair>::cast(callback);
-    *holder = Handle<JSObject>();
-    return true;
-  }
-
-  // Everything else, e.g. a field, can't be an accessor call.
-  if (lookup.IsFound()) return false;
-
-  // Check for a JavaScript accessor somewhere in the proto chain.
-  LookupInPrototypes(map, name, &lookup);
-  if (lookup.IsPropertyCallbacks()) {
-    Handle<Object> callback(lookup.GetValue());
-    if (!callback->IsAccessorPair()) return false;
-    *accessors = Handle<AccessorPair>::cast(callback);
-    *holder = Handle<JSObject>(lookup.holder());
-    return true;
-  }
-
-  // We haven't found a JavaScript accessor anywhere.
-  return false;
-}
-
-
 HInstruction* HGraphBuilder::BuildLoadNamedMonomorphic(HValue* object,
                                                        Handle<String> name,
                                                        Property* expr,
                                                        Handle<Map> map) {
   // Handle a load from a known field.
   ASSERT(!map->is_dictionary_map());
   LookupResult lookup(isolate());
   map->LookupDescriptor(NULL, *name, &lookup);
   if (lookup.IsField()) {
     return BuildLoadNamedField(object, map, &lookup, true);
@@ skipping 626 matching lines @@
     Handle<String> name = expr->key()->AsLiteral()->AsPropertyName();
     SmallMapList* types = expr->GetReceiverTypes();
 
     bool monomorphic = expr->IsMonomorphic();
     Handle<Map> map;
     if (expr->IsMonomorphic()) {
       map = types->first();
       if (map->is_dictionary_map()) monomorphic = false;
     }
     if (monomorphic) {
-      Handle<AccessorPair> accessors;
+      Handle<JSFunction> getter;
       Handle<JSObject> holder;
-      if (LookupAccessorPair(map, name, &accessors, &holder)) {
+      if (LookupGetter(map, name, &getter, &holder)) {
         AddCheckConstantFunction(holder, Top(), map, true);
-        Handle<JSFunction> getter(JSFunction::cast(accessors->getter()));
         if (FLAG_inline_accessors && TryInlineGetter(getter, expr)) return;
         AddInstruction(new(zone()) HPushArgument(Pop()));
         instr = new(zone()) HCallConstantFunction(getter, 1);
       } else {
         instr = BuildLoadNamedMonomorphic(Pop(), name, expr, map);
       }
     } else if (types != NULL && types->length() > 1) {
       return HandlePolymorphicLoadNamedField(expr, Pop(), types, name);
     } else {
       instr = BuildLoadNamedGeneric(Pop(), name, expr);
@@ skipping 1451 matching lines @@
 
       Handle<String> name = prop->key()->AsLiteral()->AsPropertyName();
       Handle<Map> map;
       HInstruction* load;
       bool monomorphic = prop->IsMonomorphic();
       if (monomorphic) {
         map = prop->GetReceiverTypes()->first();
         if (map->is_dictionary_map()) monomorphic = false;
       }
       if (monomorphic) {
-        Handle<AccessorPair> accessors;
+        Handle<JSFunction> getter;
         Handle<JSObject> holder;
-        if (LookupAccessorPair(map, name, &accessors, &holder)) {
-          load = BuildCallGetter(object, map, accessors, holder);
+        if (LookupGetter(map, name, &getter, &holder)) {
+          load = BuildCallGetter(object, map, getter, holder);
         } else {
           load = BuildLoadNamedMonomorphic(object, name, prop, map);
         }
       } else {
         load = BuildLoadNamedGeneric(object, name, prop);
       }
       PushAndAdd(load);
       if (load->HasObservableSideEffects()) AddSimulate(prop->LoadId());
 
       after = BuildIncrement(returns_original_input, expr);
       input = Pop();
 
       HInstruction* store;
       if (!monomorphic) {
         // If we don't know the monomorphic type, do a generic store.
         CHECK_ALIVE(store = BuildStoreNamedGeneric(object, name, after));
       } else {
-        Handle<AccessorPair> accessors;
+        Handle<JSFunction> setter;
         Handle<JSObject> holder;
         // Because we re-use the load type feedback, there might be no setter.

[Michael Starzinger, 2012/08/16 09:55:58]

We should remove this comment, because there might be no setter for other
reasons as well. This is misleading.

[Sven Panne, 2012/08/16 10:47:40]

Done.

-        if (LookupAccessorPair(map, name, &accessors, &holder) &&
-            accessors->setter()->IsJSFunction()) {
-          store = BuildCallSetter(object, after, map, accessors, holder);
+        if (LookupSetter(map, name, &setter, &holder)) {
+          store = BuildCallSetter(object, after, map, setter, holder);
         } else {
           CHECK_ALIVE(store = BuildStoreNamedMonomorphic(object,
                                                          name,
                                                          after,
                                                          map));
         }
       }
       AddInstruction(store);
 
       // Overwrite the receiver in the bailout environment with the result
@@ skipping 1766 matching lines @@
     }
   }
 
 #ifdef DEBUG
   if (graph_ != NULL) graph_->Verify(false);  // No full verify.
   if (allocator_ != NULL) allocator_->Verify();
 #endif
 }
 
 } }  // namespace v8::internal