[compiler] Fix typing JSLoadNamed of private brands

syg · V8 LUCI CQ · commit d19a707d148c · 2021-12-22T16:32:44.000Z
Private method loads are compiled to a named load of a private brand, which always loads a BlockContext. This BlockContext holds the private methods common to all instances of a class. TurboFan currently considers JSLoadNamed to be of Type::NonInternal(). Private methods break this assumption, since BlockContext is of Type::OtherInternal(). This CL changes the typing of JSLoadNamed of private brands to be Type::OtherInternal(). Bug: v8:12500 Change-Id: I91f39747bf9422bd419d299f44152f567d8be8db Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3351167 Reviewed-by: Maya Lekova <mslekova@chromium.org> Commit-Queue: Shu-yu Guo <syg@chromium.org> Cr-Commit-Position: refs/heads/main@{#78431}
diff --git a/src/compiler/access-info.cc b/src/compiler/access-info.cc
@@ -431,8 +431,9 @@ bool AccessInfoFactory::ComputeElementAccessInfos(
 }
 
 PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
-    MapRef receiver_map, MapRef map, base::Optional<JSObjectRef> holder,
-    InternalIndex descriptor, AccessMode access_mode) const {
+    MapRef receiver_map, MapRef map, NameRef name,
+    base::Optional<JSObjectRef> holder, InternalIndex descriptor,
+    AccessMode access_mode) const {
   DCHECK(descriptor.is_found());
   // TODO(jgruber,v8:7790): Use DescriptorArrayRef instead.
   Handle<DescriptorArray> descriptors = map.instance_descriptors().object();
@@ -449,7 +450,10 @@ PropertyAccessInfo AccessInfoFactory::ComputeDataFieldAccessInfo(
   }
   FieldIndex field_index = FieldIndex::ForPropertyIndex(*map.object(), index,
                                                         details_representation);
-  Type field_type = Type::NonInternal();
+  // Private brands are used when loading private methods, which are stored in a
+  // BlockContext, an internal object.
+  Type field_type = name.object()->IsPrivateBrand() ? Type::OtherInternal()
+                                                    : Type::NonInternal();
   base::Optional<MapRef> field_map;
 
   ZoneVector<CompilationDependency const*> unrecorded_dependencies(zone());
@@ -842,8 +846,8 @@ PropertyAccessInfo AccessInfoFactory::ComputePropertyAccessInfo(
       }
       if (details.location() == PropertyLocation::kField) {
         if (details.kind() == PropertyKind::kData) {
-          return ComputeDataFieldAccessInfo(receiver_map, map, holder, index,
-                                            access_mode);
+          return ComputeDataFieldAccessInfo(receiver_map, map, name, holder,
+                                            index, access_mode);
         } else {
           DCHECK_EQ(PropertyKind::kAccessor, details.kind());
           // TODO(turbofan): Add support for general accessors?
diff --git a/src/compiler/access-info.h b/src/compiler/access-info.h
@@ -289,8 +289,9 @@ class AccessInfoFactory final {
                                       base::Optional<JSObjectRef> holder,
                                       PropertyAttributes attrs) const;
   PropertyAccessInfo ComputeDataFieldAccessInfo(
-      MapRef receiver_map, MapRef map, base::Optional<JSObjectRef> holder,
-      InternalIndex descriptor, AccessMode access_mode) const;
+      MapRef receiver_map, MapRef map, NameRef name,
+      base::Optional<JSObjectRef> holder, InternalIndex descriptor,
+      AccessMode access_mode) const;
   PropertyAccessInfo ComputeAccessorDescriptorAccessInfo(
       MapRef receiver_map, NameRef name, MapRef map,
       base::Optional<JSObjectRef> holder, InternalIndex descriptor,
diff --git a/src/compiler/typer.cc b/src/compiler/typer.cc
@@ -1323,7 +1323,18 @@ Type Typer::Visitor::TypeJSGetTemplateObject(Node* node) {
 
 Type Typer::Visitor::TypeJSLoadProperty(Node* node) { return Type::Any(); }
 
-Type Typer::Visitor::TypeJSLoadNamed(Node* node) { return Type::NonInternal(); }
+Type Typer::Visitor::TypeJSLoadNamed(Node* node) {
+#ifdef DEBUG
+  // Loading of private methods is compiled to a named load of a BlockContext
+  // via a private brand, which is an internal object. However, native context
+  // specialization should always apply for those cases, so assert that the name
+  // is not a private brand here. Otherwise Type::NonInternal() is wrong.
+  JSLoadNamedNode n(node);
+  NamedAccess const& p = n.Parameters();
+  DCHECK(!p.name(typer_->broker()).object()->IsPrivateBrand());
+#endif
+  return Type::NonInternal();
+}
 
 Type Typer::Visitor::TypeJSLoadNamedFromSuper(Node* node) {
   return Type::NonInternal();
diff --git a/test/mjsunit/compiler/inline-private-method.js b/test/mjsunit/compiler/inline-private-method.js
@@ -0,0 +1,22 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+class A {
+  a() { this.#b() }
+  #b() {}
+}
+
+function InlinePrivateMethod() {
+  for (let i = 0; i < 10; i++) {
+    new A().a();
+  }
+}
+
+%PrepareFunctionForOptimization(A);
+%PrepareFunctionForOptimization(InlinePrivateMethod);
+InlinePrivateMethod();
+%OptimizeFunctionOnNextCall(InlinePrivateMethod);
+InlinePrivateMethod();
