[turbofan] Don't mix up holders when inlining API callbacks

Holder in 'object where the property was found' sense is different from
the holder object needed for calling API callbacks (see
FunctionCallbackInfo::Holder()).

Bug: v8:13284
Change-Id: I08dd625de6cc7ba33aec8cea4ebe28c884755455
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3913285Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83386}

src/compiler/access-info.cc

@@ -139,18 +139,20 @@ PropertyAccessInfo PropertyAccessInfo::FastDataConstant(
 
 // static
 PropertyAccessInfo PropertyAccessInfo::FastAccessorConstant(
-    Zone* zone, MapRef receiver_map, base::Optional<ObjectRef> constant,
-    base::Optional<JSObjectRef> holder) {
-  return PropertyAccessInfo(zone, kFastAccessorConstant, holder, constant, {},
-                            {{receiver_map}, zone});
+    Zone* zone, MapRef receiver_map, base::Optional<JSObjectRef> holder,
+    base::Optional<ObjectRef> constant,
+    base::Optional<JSObjectRef> api_holder) {
+  return PropertyAccessInfo(zone, kFastAccessorConstant, holder, constant,
+                            api_holder, {} /* name */, {{receiver_map}, zone});
 }
 
 // static
 PropertyAccessInfo PropertyAccessInfo::ModuleExport(Zone* zone,
                                                     MapRef receiver_map,
                                                     CellRef cell) {
-  return PropertyAccessInfo(zone, kModuleExport, {}, cell, {},
-                            {{receiver_map}, zone});
+  return PropertyAccessInfo(zone, kModuleExport, {} /* holder */,
+                            cell /* constant */, {} /* api_holder */,
+                            {} /* name */, {{receiver_map}, zone});
 }
 
 // static
@@ -170,9 +172,11 @@ PropertyAccessInfo PropertyAccessInfo::DictionaryProtoDataConstant(
 // static
 PropertyAccessInfo PropertyAccessInfo::DictionaryProtoAccessorConstant(
     Zone* zone, MapRef receiver_map, base::Optional<JSObjectRef> holder,
-    ObjectRef constant, NameRef property_name) {
+    ObjectRef constant, base::Optional<JSObjectRef> api_holder,
+    NameRef property_name) {
   return PropertyAccessInfo(zone, kDictionaryProtoAccessorConstant, holder,
-                            constant, property_name, {{receiver_map}, zone});
+                            constant, api_holder, property_name,
+                            {{receiver_map}, zone});
 }
 
 PropertyAccessInfo::PropertyAccessInfo(Zone* zone)
@@ -196,12 +200,13 @@ PropertyAccessInfo::PropertyAccessInfo(
 
 PropertyAccessInfo::PropertyAccessInfo(
     Zone* zone, Kind kind, base::Optional<JSObjectRef> holder,
-    base::Optional<ObjectRef> constant, base::Optional<NameRef> name,
-    ZoneVector<MapRef>&& lookup_start_object_maps)
+    base::Optional<ObjectRef> constant, base::Optional<JSObjectRef> api_holder,
+    base::Optional<NameRef> name, ZoneVector<MapRef>&& lookup_start_object_maps)
     : kind_(kind),
       lookup_start_object_maps_(lookup_start_object_maps),
       constant_(constant),
       holder_(holder),
+      api_holder_(api_holder),
       unrecorded_dependencies_(zone),
       field_representation_(Representation::None()),
       field_type_(Type::Any()),
@@ -538,8 +543,8 @@ PropertyAccessInfo AccessorAccessInfoHelper(
     DCHECK(!map.is_dictionary_map());
 
     // HasProperty checks don't call getter/setters, existence is sufficient.
-    return PropertyAccessInfo::FastAccessorConstant(zone, receiver_map, {},
-                                                    holder);
+    return PropertyAccessInfo::FastAccessorConstant(zone, receiver_map, holder,
+                                                    {}, {});
   }
   Handle<Object> maybe_accessors = get_accessors();
   if (!maybe_accessors->IsAccessorPair()) {
@@ -553,6 +558,7 @@ PropertyAccessInfo AccessorAccessInfoHelper(
   base::Optional<ObjectRef> accessor_ref = TryMakeRef(broker, accessor);
   if (!accessor_ref.has_value()) return PropertyAccessInfo::Invalid(zone);
 
+  base::Optional<JSObjectRef> api_holder_ref;
   if (!accessor->IsJSFunction()) {
     CallOptimization optimization(broker->local_isolate_or_isolate(), accessor);
     if (!optimization.is_simple_api_call() ||
@@ -561,24 +567,22 @@ PropertyAccessInfo AccessorAccessInfoHelper(
       return PropertyAccessInfo::Invalid(zone);
     }
 
-    CallOptimization::HolderLookup lookup;
-    Handle<JSObject> holder_handle = broker->CanonicalPersistentHandle(
+    CallOptimization::HolderLookup holder_lookup;
+    Handle<JSObject> api_holder = broker->CanonicalPersistentHandle(
         optimization.LookupHolderOfExpectedType(
             broker->local_isolate_or_isolate(), receiver_map.object(),
-            &lookup));
-    if (lookup == CallOptimization::kHolderNotFound) {
+            &holder_lookup));
+    if (holder_lookup == CallOptimization::kHolderNotFound) {
       return PropertyAccessInfo::Invalid(zone);
     }
-    DCHECK_IMPLIES(lookup == CallOptimization::kHolderIsReceiver,
-                   holder_handle.is_null());
-    DCHECK_IMPLIES(lookup == CallOptimization::kHolderFound,
-                   !holder_handle.is_null());
+    DCHECK_IMPLIES(holder_lookup == CallOptimization::kHolderIsReceiver,
+                   api_holder.is_null());
+    DCHECK_IMPLIES(holder_lookup == CallOptimization::kHolderFound,
+                   !api_holder.is_null());
 
-    if (holder_handle.is_null()) {
-      holder = {};
-    } else {
-      holder = TryMakeRef(broker, holder_handle);
-      if (!holder.has_value()) return PropertyAccessInfo::Invalid(zone);
+    if (!api_holder.is_null()) {
+      api_holder_ref = TryMakeRef(broker, api_holder);
+      if (!api_holder_ref.has_value()) return PropertyAccessInfo::Invalid(zone);
     }
   }
   if (access_mode == AccessMode::kLoad) {
@@ -596,11 +600,12 @@ PropertyAccessInfo AccessorAccessInfoHelper(
   }
 
   if (map.is_dictionary_map()) {
+    CHECK(!api_holder_ref.has_value());
     return PropertyAccessInfo::DictionaryProtoAccessorConstant(
-        zone, receiver_map, holder, accessor_ref.value(), name);
+        zone, receiver_map, holder, accessor_ref.value(), api_holder_ref, name);
   } else {
     return PropertyAccessInfo::FastAccessorConstant(
-        zone, receiver_map, accessor_ref.value(), holder);
+        zone, receiver_map, holder, accessor_ref.value(), api_holder_ref);
   }
 }
 

src/compiler/access-info.h

@@ -85,8 +85,9 @@ class PropertyAccessInfo final {
       base::Optional<JSObjectRef> holder,
       base::Optional<MapRef> transition_map);
   static PropertyAccessInfo FastAccessorConstant(
-      Zone* zone, MapRef receiver_map, base::Optional<ObjectRef> constant,
-      base::Optional<JSObjectRef> holder);
+      Zone* zone, MapRef receiver_map, base::Optional<JSObjectRef> holder,
+      base::Optional<ObjectRef> constant,
+      base::Optional<JSObjectRef> api_holder);
   static PropertyAccessInfo ModuleExport(Zone* zone, MapRef receiver_map,
                                          CellRef cell);
   static PropertyAccessInfo StringLength(Zone* zone, MapRef receiver_map);
@@ -96,7 +97,7 @@ class PropertyAccessInfo final {
       InternalIndex dict_index, NameRef name);
   static PropertyAccessInfo DictionaryProtoAccessorConstant(
       Zone* zone, MapRef receiver_map, base::Optional<JSObjectRef> holder,
-      ObjectRef constant, NameRef name);
+      ObjectRef constant, base::Optional<JSObjectRef> api_holder, NameRef name);
 
   bool Merge(PropertyAccessInfo const* that, AccessMode access_mode,
              Zone* zone) V8_WARN_UNUSED_RESULT;
@@ -127,12 +128,20 @@ class PropertyAccessInfo final {
   ConstFieldInfo GetConstFieldInfo() const;
 
   Kind kind() const { return kind_; }
+
+  // The object where the property definition was found.
   base::Optional<JSObjectRef> holder() const {
     // TODO(neis): There was a CHECK here that tries to protect against
     // using the access info without recording its dependencies first.
     // Find a more suitable place for it.
     return holder_;
   }
+  // For accessor properties when the callback is an API function with a
+  // signature, this is the value that will be passed to the callback as
+  // FunctionCallbackInfo::Holder().
+  // Don't mix it up with holder in a "object where the property was found"
+  // sense.
+  base::Optional<JSObjectRef> api_holder() const { return api_holder_; }
   base::Optional<MapRef> transition_map() const {
     DCHECK(!HasDictionaryHolder());
     return transition_map_;
@@ -180,6 +189,7 @@ class PropertyAccessInfo final {
                      ZoneVector<MapRef>&& lookup_start_object_maps);
   PropertyAccessInfo(Zone* zone, Kind kind, base::Optional<JSObjectRef> holder,
                      base::Optional<ObjectRef> constant,
+                     base::Optional<JSObjectRef> api_holder,
                      base::Optional<NameRef> name,
                      ZoneVector<MapRef>&& lookup_start_object_maps);
   PropertyAccessInfo(Kind kind, base::Optional<JSObjectRef> holder,
@@ -198,6 +208,7 @@ class PropertyAccessInfo final {
   ZoneVector<MapRef> lookup_start_object_maps_;
   base::Optional<ObjectRef> constant_;
   base::Optional<JSObjectRef> holder_;
+  base::Optional<JSObjectRef> api_holder_;
 
   // Members only used for fast mode holders:
   ZoneVector<CompilationDependency const*> unrecorded_dependencies_;

src/compiler/js-native-context-specialization.cc

@@ -2510,10 +2510,11 @@ Node* JSNativeContextSpecialization::InlinePropertyGetterCall(
     if (receiver != lookup_start_object) {
       return nullptr;
     }
-    Node* holder = access_info.holder().has_value()
-                       ? jsgraph()->Constant(access_info.holder().value())
+    Node* api_holder =
+        access_info.api_holder().has_value()
+            ? jsgraph()->Constant(access_info.api_holder().value())
             : receiver;
-    value = InlineApiCall(receiver, holder, frame_state, nullptr, effect,
+    value = InlineApiCall(receiver, api_holder, frame_state, nullptr, effect,
                           control, constant.AsFunctionTemplateInfo());
   }
   // Remember to rewire the IfException edge if this is inside a try-block.
@@ -2544,10 +2545,11 @@ void JSNativeContextSpecialization::InlinePropertySetterCall(
         target, receiver, value, feedback, context, frame_state, *effect,
         *control);
   } else {
-    Node* holder = access_info.holder().has_value()
-                       ? jsgraph()->Constant(access_info.holder().value())
+    Node* api_holder =
+        access_info.api_holder().has_value()
+            ? jsgraph()->Constant(access_info.api_holder().value())
             : receiver;
-    InlineApiCall(receiver, holder, frame_state, value, effect, control,
+    InlineApiCall(receiver, api_holder, frame_state, value, effect, control,
                   constant.AsFunctionTemplateInfo());
   }
   // Remember to rewire the IfException edge if this is inside a try-block.
@@ -2562,8 +2564,9 @@ void JSNativeContextSpecialization::InlinePropertySetterCall(
 }
 
 Node* JSNativeContextSpecialization::InlineApiCall(
-    Node* receiver, Node* holder, Node* frame_state, Node* value, Node** effect,
-    Node** control, FunctionTemplateInfoRef const& function_template_info) {
+    Node* receiver, Node* api_holder, Node* frame_state, Node* value,
+    Node** effect, Node** control,
+    FunctionTemplateInfoRef const& function_template_info) {
   if (!function_template_info.call_code().has_value()) {
     TRACE_BROKER_MISSING(broker(), "call code for function template info "
                                        << function_template_info);
@@ -2592,9 +2595,8 @@ Node* JSNativeContextSpecialization::InlineApiCall(
 
   // Add CallApiCallbackStub's register argument as well.
   Node* context = jsgraph()->Constant(native_context());
-  Node* inputs[11] = {
-      code,    function_reference, jsgraph()->Constant(argc), data, holder,
-      receiver};
+  Node* inputs[11] = {code, function_reference, jsgraph()->Constant(argc),
+                      data, api_holder,         receiver};
   int index = 6 + argc;
   inputs[index++] = context;
   inputs[index++] = frame_state;

src/compiler/js-native-context-specialization.h

@@ -181,7 +181,7 @@ class V8_EXPORT_PRIVATE JSNativeContextSpecialization final
                                 Node** control,
                                 ZoneVector<Node*>* if_exceptions,
                                 PropertyAccessInfo const& access_info);
-  Node* InlineApiCall(Node* receiver, Node* holder, Node* frame_state,
+  Node* InlineApiCall(Node* receiver, Node* api_holder, Node* frame_state,
                       Node* value, Node** effect, Node** control,
                       FunctionTemplateInfoRef const& function_template_info);