[class] maintain private brand information on SFI

When an empty class is nested inside a class with private instance
methods, like this:

  class Outer {
    constructor() {}
    #method() {}
    factory() {
      class Inner {
        constructor() {  }
      }
      return Inner;
    }
    run(obj) {
      obj.#method();
    }
  }

The bytecode generator previously generate private brand
initialization for the constructor of Inner by mistake,
because during scope chain serialization/deserialization,
the outer scopes of Inner and factory() are not allocated
or serialized (as they are empty). In the eyes of the bytecode
generator, it then appeared as if Outer is the direct outer
scope of Inner's constructor.

In order to work around this information loss, in this patch
we rely on SharedFunctionInfo instead of the Context/ScopeInfo
chain to maintain the information about private brand initialization.
This is done by shrinking expected_nof_properties to 8 bits and
freeing 8 bits for a second bitfield on the SFI.

Design doc: https://docs.google.com/document/d/14maU596YbHcWR7XR-_iXM_ANhAAmiuRlJZysM61lqaE/edit#
Bug: v8:9839, v8:8330, v8:10098

Change-Id: I4370a0459bfc0da388052ad5a91aac59582d811d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2056889
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66575}

src/ast/ast.cc

@@ -278,17 +278,6 @@ std::unique_ptr<char[]> FunctionLiteral::GetDebugName() const {
   return result;
 }
 
-bool FunctionLiteral::requires_brand_initialization() const {
-  Scope* outer = scope_->outer_scope();
-
-  // If there are no variables declared in the outer scope other than
-  // the class name variable, the outer class scope may be elided when
-  // the function is deserialized after preparsing.
-  if (!outer->is_class_scope()) return false;
-
-  return outer->AsClassScope()->brand() != nullptr;
-}
-
 bool FunctionLiteral::private_name_lookup_skips_outer_class() const {
   return scope()->private_name_lookup_skips_outer_class();
 }

src/ast/ast.h

@@ -2218,7 +2218,12 @@ class FunctionLiteral final : public Expression {
     return RequiresInstanceMembersInitializer::decode(bit_field_);
   }
 
-  bool requires_brand_initialization() const;
+  void set_class_scope_has_private_brand(bool value) {
+    bit_field_ = ClassScopeHasPrivateBrandField::update(bit_field_, value);
+  }
+  bool class_scope_has_private_brand() const {
+    return ClassScopeHasPrivateBrandField::decode(bit_field_);
+  }
 
   bool private_name_lookup_skips_outer_class() const;
 
@@ -2270,7 +2275,9 @@ class FunctionLiteral final : public Expression {
       HasDuplicateParameters::Next<BailoutReason, 8>;
   using RequiresInstanceMembersInitializer =
       DontOptimizeReasonField::Next<bool, 1>;
-  using HasBracesField = RequiresInstanceMembersInitializer::Next<bool, 1>;
+  using ClassScopeHasPrivateBrandField =
+      RequiresInstanceMembersInitializer::Next<bool, 1>;
+  using HasBracesField = ClassScopeHasPrivateBrandField::Next<bool, 1>;
   using OneshotIIFEBit = HasBracesField::Next<bool, 1>;
 
   // expected_property_count_ is the sum of instance fields and properties.

src/ast/prettyprinter.cc

@@ -828,6 +828,9 @@ const char* AstPrinter::PrintProgram(FunctionLiteral* program) {
     if (program->requires_instance_members_initializer()) {
       Print(" REQUIRES INSTANCE FIELDS INITIALIZER\n");
     }
+    if (program->class_scope_has_private_brand()) {
+      Print(" CLASS SCOPE HAS PRIVATE BRAND\n");
+    }
     PrintParameters(program->scope());
     PrintDeclarations(program->scope()->declarations());
     PrintStatements(program->body());

src/codegen/compiler.cc

@@ -511,6 +511,9 @@ void SetSharedFunctionFlagsFromLiteral(FunctionLiteral* literal,
   if (literal->dont_optimize_reason() != BailoutReason::kNoReason) {
     shared_info.DisableOptimization(literal->dont_optimize_reason());
   }
+
+  shared_info.set_class_scope_has_private_brand(
+      literal->class_scope_has_private_brand());
   shared_info.set_is_safe_to_skip_arguments_adaptor(
       literal->SafeToSkipArgumentsAdaptor());
 }

src/diagnostics/objects-printer.cc

@@ -1364,6 +1364,9 @@ void SharedFunctionInfo::SharedFunctionInfoPrint(std::ostream& os) {  // NOLINT
   if (HasInferredName()) {
     os << "\n - inferred name: " << Brief(inferred_name());
   }
+  if (class_scope_has_private_brand()) {
+    os << "\n - class_scope_has_private_brand";
+  }
   os << "\n - kind: " << kind();
   os << "\n - syntax kind: " << syntax_kind();
   if (needs_home_object()) {

src/interpreter/bytecode-generator.cc

@@ -1331,7 +1331,7 @@ void BytecodeGenerator::GenerateBytecodeBody() {
 
   // The derived constructor case is handled in VisitCallSuper.
   if (IsBaseConstructor(function_kind())) {
-    if (literal->requires_brand_initialization()) {
+    if (literal->class_scope_has_private_brand()) {
       BuildPrivateBrandInitialization(builder()->Receiver());
     }
 
@@ -4998,7 +4998,7 @@ void BytecodeGenerator::VisitCallSuper(Call* expr) {
   Register instance = register_allocator()->NewRegister();
   builder()->StoreAccumulatorInRegister(instance);
 
-  if (info()->literal()->requires_brand_initialization()) {
+  if (info()->literal()->class_scope_has_private_brand()) {
     BuildPrivateBrandInitialization(instance);
   }
 

src/objects/objects.cc

@@ -4971,6 +4971,7 @@ void SharedFunctionInfo::Init(ReadOnlyRoots ro_roots, int unique_id) {
   // All flags default to false or 0, except ConstructAsBuiltinBit just because
   // we're using the kIllegal builtin.
   set_flags(ConstructAsBuiltinBit::encode(true));
+  set_flags2(0);
 
   UpdateFunctionMapIndex();
 
@@ -5365,6 +5366,10 @@ void SharedFunctionInfo::InitFromFunctionLiteral(
                  IsClassConstructor(lit->kind()));
   shared_info->set_requires_instance_members_initializer(
       lit->requires_instance_members_initializer());
+  DCHECK_IMPLIES(lit->class_scope_has_private_brand(),
+                 IsClassConstructor(lit->kind()));
+  shared_info->set_class_scope_has_private_brand(
+      lit->class_scope_has_private_brand());
 
   shared_info->set_is_toplevel(is_toplevel);
   DCHECK(shared_info->outer_scope_info().IsTheHole());
@@ -5438,7 +5443,11 @@ uint16_t SharedFunctionInfo::get_property_estimate_from_literal(
 
 void SharedFunctionInfo::UpdateExpectedNofPropertiesFromEstimate(
     FunctionLiteral* literal) {
-  set_expected_nof_properties(get_property_estimate_from_literal(literal));
+  // Limit actual estimate to fit in a 8 bit field, we will never allocate
+  // more than this in any case.
+  STATIC_ASSERT(JSObject::kMaxInObjectProperties <= kMaxUInt8);
+  int estimate = get_property_estimate_from_literal(literal);
+  set_expected_nof_properties(std::min(estimate, kMaxUInt8));
 }
 
 void SharedFunctionInfo::UpdateAndFinalizeExpectedNofPropertiesFromEstimate(

src/objects/shared-function-info-inl.h

@@ -110,11 +110,12 @@ INT_ACCESSORS(SharedFunctionInfo, unique_id, kUniqueIdOffset)
 UINT16_ACCESSORS(SharedFunctionInfo, length, kLengthOffset)
 UINT16_ACCESSORS(SharedFunctionInfo, internal_formal_parameter_count,
                  kFormalParameterCountOffset)
-UINT16_ACCESSORS(SharedFunctionInfo, expected_nof_properties,
-                 kExpectedNofPropertiesOffset)
+UINT8_ACCESSORS(SharedFunctionInfo, expected_nof_properties,
+                kExpectedNofPropertiesOffset)
 UINT16_ACCESSORS(SharedFunctionInfo, raw_function_token_offset,
                  kFunctionTokenOffsetOffset)
 RELAXED_INT32_ACCESSORS(SharedFunctionInfo, flags, kFlagsOffset)
+UINT8_ACCESSORS(SharedFunctionInfo, flags2, kFlags2Offset)
 
 bool SharedFunctionInfo::HasSharedName() const {
   Object value = name_or_scope_info();
@@ -184,6 +185,9 @@ int SharedFunctionInfo::function_token_position() const {
   }
 }
 
+BIT_FIELD_ACCESSORS(SharedFunctionInfo, flags2, class_scope_has_private_brand,
+                    SharedFunctionInfo::ClassScopeHasPrivateBrandBit)
+
 BIT_FIELD_ACCESSORS(SharedFunctionInfo, flags, syntax_kind,
                     SharedFunctionInfo::FunctionSyntaxKindBits)
 

src/objects/shared-function-info.h

@@ -172,6 +172,7 @@ class SharedFunctionInfo : public HeapObject {
  public:
   NEVER_READ_ONLY_SPACE
   DEFINE_TORQUE_GENERATED_SHARED_FUNCTION_INFO_FLAGS()
+  DEFINE_TORQUE_GENERATED_SHARED_FUNCTION_INFO_FLAGS2()
 
   // This initializes the SharedFunctionInfo after allocation. It must
   // initialize all fields, and leave the SharedFunctionInfo in a state where
@@ -279,7 +280,7 @@ class SharedFunctionInfo : public HeapObject {
 
   // [expected_nof_properties]: Expected number of properties for the
   // function. The value is only reliable when the function has been compiled.
-  DECL_UINT16_ACCESSORS(expected_nof_properties)
+  DECL_UINT8_ACCESSORS(expected_nof_properties)
 
   // [function_literal_id] - uniquely identifies the FunctionLiteral this
   // SharedFunctionInfo represents within its script, or -1 if this
@@ -402,6 +403,11 @@ class SharedFunctionInfo : public HeapObject {
 
   // [flags] Bit field containing various flags about the function.
   DECL_INT32_ACCESSORS(flags)
+  DECL_UINT8_ACCESSORS(flags2)
+
+  // True if the outer class scope contains a private brand for
+  // private instance methdos.
+  DECL_BOOLEAN_ACCESSORS(class_scope_has_private_brand)
 
   // Is this function a top-level function (scripts, evals).
   DECL_BOOLEAN_ACCESSORS(is_toplevel)

src/objects/shared-function-info.tq

@@ -42,6 +42,10 @@ bitfield struct SharedFunctionInfoFlags extends uint32 {
   private_name_lookup_skips_outer_class: bool: 1 bit;
 }
 
+bitfield struct SharedFunctionInfoFlags2 extends uint8 {
+  class_scope_has_private_brand: bool: 1 bit;
+}
+
 extern class SharedFunctionInfo extends HeapObject {
   weak function_data: Object;
   name_or_scope_info: String|NoSharedNameSentinel|ScopeInfo;
@@ -49,9 +53,9 @@ extern class SharedFunctionInfo extends HeapObject {
   script_or_debug_info: Script|DebugInfo|Undefined;
   length: int16;
   formal_parameter_count: uint16;
-  // Currently set to uint16, can be set to uint8 to save space.
-  expected_nof_properties: uint16;
   function_token_offset: int16;
+  expected_nof_properties: uint8;
+  flags2: SharedFunctionInfoFlags2;
   flags: SharedFunctionInfoFlags;
   function_literal_id: int32;
   @if(V8_SFI_HAS_UNIQUE_ID) unique_id: int32;

src/parsing/parse-info.cc

@@ -81,6 +81,7 @@ void ParseInfo::SetFunctionInfo(T function) {
   set_function_syntax_kind(function->syntax_kind());
   set_requires_instance_members_initializer(
       function->requires_instance_members_initializer());
+  set_class_scope_has_private_brand(function->class_scope_has_private_brand());
   set_toplevel(function->is_toplevel());
   set_is_oneshot_iife(function->is_oneshot_iife());
 }

src/parsing/parse-info.h

@@ -93,6 +93,8 @@ class V8_EXPORT_PRIVATE ParseInfo {
   FLAG_ACCESSOR(kRequiresInstanceMembersInitializer,
                 requires_instance_members_initializer,
                 set_requires_instance_members_initializer)
+  FLAG_ACCESSOR(kClassScopeHasPrivateBrand, class_scope_has_private_brand,
+                set_class_scope_has_private_brand)
   FLAG_ACCESSOR(kMightAlwaysOpt, might_always_opt, set_might_always_opt)
   FLAG_ACCESSOR(kAllowNativeSyntax, allow_natives_syntax,
                 set_allow_natives_syntax)
@@ -333,6 +335,7 @@ class V8_EXPORT_PRIVATE ParseInfo {
     kAllowHarmonyNullish = 1u << 28,
     kAllowHarmonyTopLevelAwait = 1u << 29,
     kREPLMode = 1u << 30,
+    kClassScopeHasPrivateBrand = 1u << 31,
   };
 
   //------------- Inputs to parsing and scope analysis -----------------------

src/parsing/parser.cc

@@ -961,6 +961,8 @@ FunctionLiteral* Parser::DoParseFunction(Isolate* isolate, ParseInfo* info,
     if (has_error()) return nullptr;
     result->set_requires_instance_members_initializer(
         info->requires_instance_members_initializer());
+    result->set_class_scope_has_private_brand(
+        info->class_scope_has_private_brand());
     if (info->is_oneshot_iife()) {
       result->mark_as_oneshot_iife();
     }
@@ -2972,6 +2974,10 @@ Expression* Parser::RewriteClassLiteral(ClassScope* block_scope,
         class_info->instance_fields->length());
   }
 
+  if (class_info->requires_brand) {
+    class_info->constructor->set_class_scope_has_private_brand(true);
+  }
+
   ClassLiteral* class_literal = factory()->NewClassLiteral(
       block_scope, class_info->extends, class_info->constructor,
       class_info->public_members, class_info->private_members,

test/inspector/debugger/class-private-methods-empty-inner-expected.txt

@@ -0,0 +1,4 @@
+Test empty inner classes with private instance methods in the outer class
+
+Running test: testScopesPaused
+undefined

test/inspector/debugger/class-private-methods-empty-inner.js

@@ -0,0 +1,44 @@
+// Copyright 2020 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: --harmony-private-methods
+
+let {session, contextGroup, Protocol} = InspectorTest.start(
+    'Test empty inner classes with private instance methods in the outer class');
+
+contextGroup.addScript(`
+function run() {
+  class Outer {
+    #method() {}
+
+    factory() {
+      return class Inner {
+        fn() {
+          debugger;
+        }
+      };
+    }
+  };
+
+  const a = new Outer();
+  const Inner = a.factory();
+  (new Inner).fn();
+}`);
+
+InspectorTest.runAsyncTestSuite([async function testScopesPaused() {
+  Protocol.Debugger.enable();
+  Protocol.Runtime.evaluate({expression: 'run()'});
+
+  let {params: {callFrames}} =
+      await Protocol.Debugger.oncePaused();  // inside fn()
+  let frame = callFrames[0];
+
+  let {result} =
+      await Protocol.Runtime.getProperties({objectId: frame.this.objectId});
+
+  InspectorTest.logObject(result.privateProperties);
+
+  Protocol.Debugger.resume();
+  Protocol.Debugger.disable();
+}]);

test/mjsunit/harmony/private-methods-empty-inner.js

@@ -0,0 +1,30 @@
+// Copyright 2020 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: --harmony-private-methods
+// This tests that empty inner classes don't assign private brands of outer
+// classes in their instances after scope chain deserialization.
+
+'use strict';
+
+class Outer {
+  constructor() {}
+#method(){}
+  factory() {
+    class Inner {
+      constructor() {}
+    }
+    return Inner;
+  }
+  run(obj) {
+    obj.#method();
+  }
+}
+
+const instance = new Outer();
+const Inner = instance.factory();
+// It should not pass the brand check.
+assertThrows(() => instance.run(new Inner()), TypeError);
+// It should pass the brand check.
+instance.run(new Outer());