[class] error when accessing unused static private method at debug time

At the moment when the static private method is unused
in source code (either explicitly or through eval) but is accessed
at runtime through the debugger, and there are no other potential
references to the class variable in the source code otherwise,
the reference to the class variable is lost here since the class
variable would not be context-allocated, then we could not rebuild
a proper brand check for it.

For now, a ReferenceError would be thrown and the method is considered
"optimized away", similar to how unused ordinary methods in closures
work. Before this patch it would DCHECK when generating bytecode
for the debugger instead of throwing errors.

Bug: v8:9839, v8:8330
Change-Id: I5d63131a7bdba141d01a3e6459bc27d0f5953c1a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2095637
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66734}

src/common/message-template.h

@@ -426,6 +426,8 @@ namespace internal {
   T(InvalidPrivateMethodWrite, "Private method '%' is not writable")           \
   T(InvalidPrivateGetterAccess, "'%' was defined without a getter")            \
   T(InvalidPrivateSetterAccess, "'%' was defined without a setter")            \
+  T(InvalidUnusedPrivateStaticMethodAccessedByDebugger,                        \
+    "Unused static private method '%' cannot be accessed at debug time")       \
   T(JsonParseUnexpectedEOS, "Unexpected end of JSON input")                    \
   T(JsonParseUnexpectedToken, "Unexpected token % in JSON at position %")      \
   T(JsonParseUnexpectedTokenNumber, "Unexpected number in JSON at position %") \

src/interpreter/bytecode-generator.cc

@@ -4672,16 +4672,37 @@ void BytecodeGenerator::BuildPrivateBrandCheck(Property* property,
   DCHECK(IsPrivateMethodOrAccessorVariableMode(private_name->mode()));
   ClassScope* scope = private_name->scope()->AsClassScope();
   if (private_name->is_static()) {
-    DCHECK_NOT_NULL(scope->class_variable());
     // For static private methods, the only valid receiver is the class.
     // Load the class constructor.
-    BuildVariableLoadForAccumulatorValue(scope->class_variable(),
-                                         HoleCheckMode::kElided);
-    BytecodeLabel return_check;
-    builder()->CompareReference(object).JumpIfTrue(
-        ToBooleanMode::kAlreadyBoolean, &return_check);
-    BuildInvalidPropertyAccess(tmpl, property);
-    builder()->Bind(&return_check);
+    if (scope->class_variable() == nullptr) {
+      // If the static private method has not been used used in source
+      // code (either explicitly or through the presence of eval), but is
+      // accessed by the debugger at runtime, reference to the class variable
+      // is not available since it was not be context-allocated. Therefore we
+      // can't build a branch check, and throw an ReferenceError as if the
+      // method was optimized away.
+      // TODO(joyee): get a reference to the class constructor through
+      // something other than scope->class_variable() in this scenario.
+      RegisterAllocationScope register_scope(this);
+      RegisterList args = register_allocator()->NewRegisterList(2);
+      builder()
+          ->LoadLiteral(Smi::FromEnum(
+              MessageTemplate::
+                  kInvalidUnusedPrivateStaticMethodAccessedByDebugger))
+          .StoreAccumulatorInRegister(args[0])
+          .LoadLiteral(private_name->raw_name())
+          .StoreAccumulatorInRegister(args[1])
+          .CallRuntime(Runtime::kNewError, args)
+          .Throw();
+    } else {
+      BuildVariableLoadForAccumulatorValue(scope->class_variable(),
+                                           HoleCheckMode::kElided);
+      BytecodeLabel return_check;
+      builder()->CompareReference(object).JumpIfTrue(
+          ToBooleanMode::kAlreadyBoolean, &return_check);
+      BuildInvalidPropertyAccess(tmpl, property);
+      builder()->Bind(&return_check);
+    }
   } else {
     BuildVariableLoadForAccumulatorValue(scope->brand(),
                                          HoleCheckMode::kElided);

src/runtime/runtime-internal.cc

@@ -198,6 +198,15 @@ RUNTIME_FUNCTION(Runtime_ThrowAccessedUninitializedVariable) {
       NewReferenceError(MessageTemplate::kAccessedUninitializedVariable, name));
 }
 
+RUNTIME_FUNCTION(Runtime_NewError) {
+  HandleScope scope(isolate);
+  DCHECK_EQ(2, args.length());
+  CONVERT_INT32_ARG_CHECKED(template_index, 0);
+  CONVERT_ARG_HANDLE_CHECKED(Object, arg0, 1);
+  MessageTemplate message_template = MessageTemplateFromInt(template_index);
+  return *isolate->factory()->NewError(message_template, arg0);
+}
+
 RUNTIME_FUNCTION(Runtime_NewTypeError) {
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());

src/runtime/runtime.h

@@ -219,6 +219,7 @@ namespace internal {
   F(GetTemplateObject, 3, 1)                         \
   F(IncrementUseCounter, 1, 1)                       \
   F(BytecodeBudgetInterrupt, 1, 1)                   \
+  F(NewError, 2, 1)                                  \
   F(NewReferenceError, 2, 1)                         \
   F(NewSyntaxError, 2, 1)                            \
   F(NewTypeError, 2, 1)                              \

test/inspector/debugger/class-private-methods-static-nested.js

@@ -38,9 +38,7 @@ InspectorTest.runAsyncTestSuite([
     let {
       params: { callFrames }
     } = await Protocol.Debugger.oncePaused(); // inside A.#method()
-    // TODO(joyee): make it possible to desugar the brand check, which requires
-    // the class variable to be saved, even when the static private
-    // methods/accessors are not referenced in the class body.
+
     let frame = callFrames[0];
     let { result } = await Protocol.Runtime.getProperties({
       objectId: frame.this.objectId

test/inspector/debugger/class-private-methods-static.js

@@ -57,9 +57,6 @@ InspectorTest.runAsyncTestSuite([
     });
     InspectorTest.logMessage(result.privateProperties);
 
-    // TODO(joyee): make it possible to desugar the brand check, which requires
-    // the class variable to be saved, even when the static private
-    // methods/accessors are not referenced in the class body.
     InspectorTest.log('Evaluating A.#inc();');
     ({ result } = await Protocol.Debugger.evaluateOnCallFrame({
       expression: 'A.#inc();',

test/inspector/debugger/class-private-methods-unused-expected.txt

@@ -0,0 +1,83 @@
+Test accessing unused private methods at runtime
+
+Running test: testScopesPaused
+Get privateProperties of A in testStatic()
+[
+    [0] : {
+        name : #staticMethod
+        value : {
+            className : Function
+            description : #staticMethod() { return 1; }
+            objectId : {"injectedScriptId":1,"id":34}
+            type : function
+        }
+    }
+]
+Access A.#staticMethod() in testStatic()
+{
+    exceptionDetails : {
+        columnNumber : 0
+        exception : {
+            className : ReferenceError
+            description : ReferenceError: A is not defined     at eval (eval at testStatic (:1:1), <anonymous>:1:1)     at Function.testStatic (<anonymous>:6:29)     at run (<anonymous>:9:7)     at <anonymous>:1:1
+            objectId : {"injectedScriptId":1,"id":36}
+            subtype : error
+            type : object
+        }
+        exceptionId : 1
+        lineNumber : 0
+        scriptId : 5
+        text : Uncaught
+    }
+    result : {
+        className : ReferenceError
+        description : ReferenceError: A is not defined     at eval (eval at testStatic (:1:1), <anonymous>:1:1)     at Function.testStatic (<anonymous>:6:29)     at run (<anonymous>:9:7)     at <anonymous>:1:1
+        objectId : {"injectedScriptId":1,"id":35}
+        subtype : error
+        type : object
+    }
+}
+Access this.#staticMethod() in testStatic()
+{
+    exceptionDetails : {
+        columnNumber : 0
+        exception : {
+            className : Error
+            description : Error: Unused static private method '#staticMethod' cannot be accessed at debug time     at eval (eval at testStatic (:1:1), <anonymous>:1:1)     at Function.testStatic (<anonymous>:6:29)     at run (<anonymous>:9:7)     at <anonymous>:1:1
+            objectId : {"injectedScriptId":1,"id":38}
+            subtype : error
+            type : object
+        }
+        exceptionId : 2
+        lineNumber : 0
+        scriptId : 6
+        text : Uncaught
+    }
+    result : {
+        className : Error
+        description : Error: Unused static private method '#staticMethod' cannot be accessed at debug time     at eval (eval at testStatic (:1:1), <anonymous>:1:1)     at Function.testStatic (<anonymous>:6:29)     at run (<anonymous>:9:7)     at <anonymous>:1:1
+        objectId : {"injectedScriptId":1,"id":37}
+        subtype : error
+        type : object
+    }
+}
+get privateProperties of a in testInstance()
+[
+    [0] : {
+        name : #instanceMethod
+        value : {
+            className : Function
+            description : #instanceMethod() { return 2; }
+            objectId : {"injectedScriptId":1,"id":61}
+            type : function
+        }
+    }
+]
+Evaluating this.#instanceMethod() in testInstance()
+{
+    result : {
+        description : 2
+        type : number
+        value : 2
+    }
+}

test/inspector/debugger/class-private-methods-unused.js

@@ -0,0 +1,79 @@
+// 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 accessing unused private methods at runtime"
+);
+
+const script = `
+  function run() {
+    class A {
+      #instanceMethod() { return 2; }
+      static #staticMethod() { return 1; }
+      static testStatic() { debugger; }
+      testInstance() { debugger; }
+    };
+    A.testStatic();
+    const a = new A;
+    a.testInstance();
+  }`;
+
+contextGroup.addScript(script);
+
+InspectorTest.runAsyncTestSuite([
+  async function testScopesPaused() {
+    Protocol.Debugger.enable();
+
+    // Do not await here, instead oncePaused should be awaited.
+    Protocol.Runtime.evaluate({ expression: 'run()' });
+
+    InspectorTest.log('Get privateProperties of A in testStatic()');
+    let {
+      params: { callFrames }
+    } = await Protocol.Debugger.oncePaused(); // inside A.testStatic()
+    let frame = callFrames[0];
+    let { result } = await Protocol.Runtime.getProperties({
+      objectId: frame.this.objectId
+    });
+    InspectorTest.logObject(result.privateProperties);
+
+    // Variables not referenced in the source code are currently
+    // considered "optimized away".
+    InspectorTest.log('Access A.#staticMethod() in testStatic()');
+    ({ result } = await Protocol.Debugger.evaluateOnCallFrame({
+      expression: 'A.#staticMethod();',
+      callFrameId: callFrames[0].callFrameId
+    }));
+    InspectorTest.logObject(result);
+
+    InspectorTest.log('Access this.#staticMethod() in testStatic()');
+    ({ result } = await Protocol.Debugger.evaluateOnCallFrame({
+      expression: 'this.#staticMethod();',
+      callFrameId: callFrames[0].callFrameId
+    }));
+    InspectorTest.logObject(result);
+
+    Protocol.Debugger.resume();
+    ({ params: { callFrames } } = await Protocol.Debugger.oncePaused());  // a.testInstatnce();
+    frame = callFrames[0];
+
+    InspectorTest.log('get privateProperties of a in testInstance()');
+    ({ result } = await Protocol.Runtime.getProperties({
+      objectId: frame.this.objectId
+    }));
+    InspectorTest.logObject(result.privateProperties);
+
+    InspectorTest.log('Evaluating this.#instanceMethod() in testInstance()');
+    ({ result } = await Protocol.Debugger.evaluateOnCallFrame({
+      expression: 'this.#instanceMethod();',
+      callFrameId: callFrames[0].callFrameId
+    }));
+    InspectorTest.logObject(result);
+
+    Protocol.Debugger.resume();
+    Protocol.Debugger.disable();
+  }
+]);