[turbofan] Handle class constructor

Handling of class constructors was moved from CallFunction to Call
in [1].
When reducing calls with spread we forward varargs directly to
CallFunction, if we are spreading to inlined arguments or arguments of
the outermost function.
In that case we didn't check for class constructors and therefore didn't
raise an exception.
This CL adds checks for class constructors to all JSCall* nodes in
JSCallReducer that missed them before.

[1] https://crrev.com/c/3186434

Bug: chromium:1260623
Change-Id: Id39cdfd09ff5aae804ae30d96909518e408c9613
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3229369
Commit-Queue: Patrick Thier <pthier@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77472}

src/builtins/arm/builtins-arm.cc

@@ -2281,7 +2281,7 @@ void Builtins::Generate_CallFunction(MacroAssembler* masm,
   //  -- r0 : the number of arguments
   //  -- r1 : the function to call (checked to be a JSFunction)
   // -----------------------------------
-  __ AssertFunction(r1);
+  __ AssertCallableFunction(r1);
 
   __ ldr(r2, FieldMemOperand(r1, JSFunction::kSharedFunctionInfoOffset));
 

src/builtins/arm64/builtins-arm64.cc

@@ -2646,7 +2646,7 @@ void Builtins::Generate_CallFunction(MacroAssembler* masm,
   //  -- x0 : the number of arguments
   //  -- x1 : the function to call (checked to be a JSFunction)
   // -----------------------------------
-  __ AssertFunction(x1);
+  __ AssertCallableFunction(x1);
 
   __ LoadTaggedPointerField(
       x2, FieldMemOperand(x1, JSFunction::kSharedFunctionInfoOffset));

src/builtins/ia32/builtins-ia32.cc

@@ -2445,7 +2445,7 @@ void Builtins::Generate_CallFunction(MacroAssembler* masm,
   //  -- edi : the function to call (checked to be a JSFunction)
   // -----------------------------------
   StackArgumentsAccessor args(eax);
-  __ AssertFunction(edi, edx);
+  __ AssertCallableFunction(edi, edx);
 
   __ mov(edx, FieldOperand(edi, JSFunction::kSharedFunctionInfoOffset));
 

src/builtins/x64/builtins-x64.cc

@@ -2375,7 +2375,7 @@ void Builtins::Generate_CallFunction(MacroAssembler* masm,
   // -----------------------------------
 
   StackArgumentsAccessor args(rax);
-  __ AssertFunction(rdi);
+  __ AssertCallableFunction(rdi);
 
   __ LoadTaggedPointerField(
       rdx, FieldOperand(rdi, JSFunction::kSharedFunctionInfoOffset));

src/codegen/arm/macro-assembler-arm.cc

@@ -2218,6 +2218,20 @@ void MacroAssembler::AssertFunction(Register object) {
   Check(ls, AbortReason::kOperandIsNotAFunction);
 }
 
+void MacroAssembler::AssertCallableFunction(Register object) {
+  if (!FLAG_debug_code) return;
+  ASM_CODE_COMMENT(this);
+  STATIC_ASSERT(kSmiTag == 0);
+  tst(object, Operand(kSmiTagMask));
+  Check(ne, AbortReason::kOperandIsASmiAndNotAFunction);
+  push(object);
+  LoadMap(object, object);
+  CompareInstanceTypeRange(object, object, FIRST_CALLABLE_JS_FUNCTION_TYPE,
+                           LAST_CALLABLE_JS_FUNCTION_TYPE);
+  pop(object);
+  Check(ls, AbortReason::kOperandIsNotACallableFunction);
+}
+
 void MacroAssembler::AssertBoundFunction(Register object) {
   if (!FLAG_debug_code) return;
   ASM_CODE_COMMENT(this);

src/codegen/arm/macro-assembler-arm.h

@@ -851,6 +851,10 @@ class V8_EXPORT_PRIVATE MacroAssembler : public TurboAssembler {
   // Abort execution if argument is not a JSFunction, enabled via --debug-code.
   void AssertFunction(Register object);
 
+  // Abort execution if argument is not a callable JSFunction, enabled via
+  // --debug-code.
+  void AssertCallableFunction(Register object);
+
   // Abort execution if argument is not a JSBoundFunction,
   // enabled via --debug-code.
   void AssertBoundFunction(Register object);

src/codegen/arm64/macro-assembler-arm64.cc

@@ -1551,6 +1551,19 @@ void MacroAssembler::AssertFunction(Register object) {
   Check(ls, AbortReason::kOperandIsNotAFunction);
 }
 
+void MacroAssembler::AssertCallableFunction(Register object) {
+  if (!FLAG_debug_code) return;
+  ASM_CODE_COMMENT(this);
+  AssertNotSmi(object, AbortReason::kOperandIsASmiAndNotAFunction);
+
+  UseScratchRegisterScope temps(this);
+  Register temp = temps.AcquireX();
+  LoadMap(temp, object);
+  CompareInstanceTypeRange(temp, temp, FIRST_CALLABLE_JS_FUNCTION_TYPE,
+                           LAST_CALLABLE_JS_FUNCTION_TYPE);
+  Check(ls, AbortReason::kOperandIsNotACallableFunction);
+}
+
 void MacroAssembler::AssertBoundFunction(Register object) {
   if (!FLAG_debug_code) return;
   ASM_CODE_COMMENT(this);

src/codegen/arm64/macro-assembler-arm64.h

@@ -1857,6 +1857,10 @@ class V8_EXPORT_PRIVATE MacroAssembler : public TurboAssembler {
   // Abort execution if argument is not a JSFunction, enabled via --debug-code.
   void AssertFunction(Register object);
 
+  // Abort execution if argument is not a callable JSFunction, enabled via
+  // --debug-code.
+  void AssertCallableFunction(Register object);
+
   // Abort execution if argument is not a JSGeneratorObject (or subclass),
   // enabled via --debug-code.
   void AssertGeneratorObject(Register object);

src/codegen/bailout-reason.h

@@ -48,6 +48,7 @@ namespace internal {
   V(kOperandIsNotAConstructor, "Operand is not a constructor")                 \
   V(kOperandIsNotAFixedArray, "Operand is not a fixed array")                  \
   V(kOperandIsNotAFunction, "Operand is not a function")                       \
+  V(kOperandIsNotACallableFunction, "Operand is not a callable function")      \
   V(kOperandIsNotAGeneratorObject, "Operand is not a generator object")        \
   V(kOperandIsNotACodeT, "Operand is not a CodeT")                             \
   V(kOperandIsNotASmi, "Operand is not a smi")                                 \

src/codegen/ia32/macro-assembler-ia32.cc

@@ -777,6 +777,21 @@ void MacroAssembler::AssertFunction(Register object, Register scratch) {
   }
 }
 
+void MacroAssembler::AssertCallableFunction(Register object, Register scratch) {
+  if (FLAG_debug_code) {
+    ASM_CODE_COMMENT(this);
+    test(object, Immediate(kSmiTagMask));
+    Check(not_equal, AbortReason::kOperandIsASmiAndNotAFunction);
+    Push(object);
+    LoadMap(object, object);
+    CmpInstanceTypeRange(object, scratch, scratch,
+                         FIRST_CALLABLE_JS_FUNCTION_TYPE,
+                         LAST_CALLABLE_JS_FUNCTION_TYPE);
+    Pop(object);
+    Check(below_equal, AbortReason::kOperandIsNotACallableFunction);
+  }
+}
+
 void MacroAssembler::AssertBoundFunction(Register object) {
   if (FLAG_debug_code) {
     ASM_CODE_COMMENT(this);

src/codegen/ia32/macro-assembler-ia32.h

@@ -566,6 +566,10 @@ class V8_EXPORT_PRIVATE MacroAssembler : public TurboAssembler {
   // Abort execution if argument is not a JSFunction, enabled via --debug-code.
   void AssertFunction(Register object, Register scratch);
 
+  // Abort execution if argument is not a callable JSFunction, enabled via
+  // --debug-code.
+  void AssertCallableFunction(Register object, Register scratch);
+
   // Abort execution if argument is not a Constructor, enabled via --debug-code.
   void AssertConstructor(Register object);
 

src/codegen/x64/macro-assembler-x64.cc

@@ -2418,6 +2418,19 @@ void MacroAssembler::AssertFunction(Register object) {
   Check(below_equal, AbortReason::kOperandIsNotAFunction);
 }
 
+void MacroAssembler::AssertCallableFunction(Register object) {
+  if (!FLAG_debug_code) return;
+  ASM_CODE_COMMENT(this);
+  testb(object, Immediate(kSmiTagMask));
+  Check(not_equal, AbortReason::kOperandIsASmiAndNotAFunction);
+  Push(object);
+  LoadMap(object, object);
+  CmpInstanceTypeRange(object, object, FIRST_CALLABLE_JS_FUNCTION_TYPE,
+                       LAST_CALLABLE_JS_FUNCTION_TYPE);
+  Pop(object);
+  Check(below_equal, AbortReason::kOperandIsNotACallableFunction);
+}
+
 void MacroAssembler::AssertBoundFunction(Register object) {
   if (!FLAG_debug_code) return;
   ASM_CODE_COMMENT(this);

src/codegen/x64/macro-assembler-x64.h

@@ -809,6 +809,10 @@ class V8_EXPORT_PRIVATE MacroAssembler : public TurboAssembler {
   // Abort execution if argument is not a JSFunction, enabled via --debug-code.
   void AssertFunction(Register object);
 
+  // Abort execution if argument is not a callable JSFunction, enabled via
+  // --debug-code.
+  void AssertCallableFunction(Register object);
+
   // Abort execution if argument is not a JSBoundFunction,
   // enabled via --debug-code.
   void AssertBoundFunction(Register object);

src/compiler/js-call-reducer.cc

@@ -4482,7 +4482,8 @@ Reduction JSCallReducer::ReduceJSCall(Node* node,
   // Debug::PrepareFunctionForDebugExecution()).
   if (shared.HasBreakInfo()) return NoChange();
 
-  // Raise a TypeError if the {target} is a "classConstructor".
+  // Class constructors are callable, but [[Call]] will raise an exception.
+  // See ES6 section 9.2.1 [[Call]] ( thisArgument, argumentsList ).
   if (IsClassConstructor(shared.kind())) {
     NodeProperties::ReplaceValueInputs(node, target);
     NodeProperties::ChangeOp(
@@ -4886,10 +4887,51 @@ TNode<Object> JSCallReducerAssembler::ReduceJSCallWithArrayLikeOrSpreadOfEmpty(
       .Value();
 }
 
+namespace {
+
+// Check if the target is a class constructor.
+// We need to check all cases where the target will be typed as Function
+// to prevent later optimizations from using the CallFunction trampoline,
+// skipping the instance type check.
+bool TargetIsClassConstructor(Node* node, JSHeapBroker* broker) {
+  Node* target = NodeProperties::GetValueInput(node, 0);
+  base::Optional<SharedFunctionInfoRef> shared;
+  HeapObjectMatcher m(target);
+  if (m.HasResolvedValue()) {
+    ObjectRef target_ref = m.Ref(broker);
+    if (target_ref.IsJSFunction()) {
+      JSFunctionRef function = target_ref.AsJSFunction();
+      shared = function.shared();
+    }
+  } else if (target->opcode() == IrOpcode::kJSCreateClosure) {
+    CreateClosureParameters const& ccp =
+        JSCreateClosureNode{target}.Parameters();
+    shared = ccp.shared_info(broker);
+  } else if (target->opcode() == IrOpcode::kCheckClosure) {
+    FeedbackCellRef cell = MakeRef(broker, FeedbackCellOf(target->op()));
+    shared = cell.shared_function_info();
+  }
+
+  if (shared.has_value() && IsClassConstructor(shared->kind())) return true;
+
+  return false;
+}
+
+}  // namespace
+
 Reduction JSCallReducer::ReduceJSCallWithArrayLike(Node* node) {
   JSCallWithArrayLikeNode n(node);
   CallParameters const& p = n.Parameters();
   DCHECK_EQ(p.arity_without_implicit_args(), 1);  // The arraylike object.
+  // Class constructors are callable, but [[Call]] will raise an exception.
+  // See ES6 section 9.2.1 [[Call]] ( thisArgument, argumentsList ).
+  if (TargetIsClassConstructor(node, broker())) {
+    NodeProperties::ReplaceValueInputs(node, n.target());
+    NodeProperties::ChangeOp(
+        node, javascript()->CallRuntime(
+                  Runtime::kThrowConstructorNonCallableError, 1));
+    return Changed(node);
+  }
   return ReduceCallOrConstructWithArrayLikeOrSpread(
       node, n.ArgumentCount(), n.LastArgumentIndex(), p.frequency(),
       p.feedback(), p.speculation_mode(), p.feedback_relation(), n.target(),
@@ -4900,6 +4942,15 @@ Reduction JSCallReducer::ReduceJSCallWithSpread(Node* node) {
   JSCallWithSpreadNode n(node);
   CallParameters const& p = n.Parameters();
   DCHECK_GE(p.arity_without_implicit_args(), 1);  // At least the spread.
+  // Class constructors are callable, but [[Call]] will raise an exception.
+  // See ES6 section 9.2.1 [[Call]] ( thisArgument, argumentsList ).
+  if (TargetIsClassConstructor(node, broker())) {
+    NodeProperties::ReplaceValueInputs(node, n.target());
+    NodeProperties::ChangeOp(
+        node, javascript()->CallRuntime(
+                  Runtime::kThrowConstructorNonCallableError, 1));
+    return Changed(node);
+  }
   return ReduceCallOrConstructWithArrayLikeOrSpread(
       node, n.ArgumentCount(), n.LastArgumentIndex(), p.frequency(),
       p.feedback(), p.speculation_mode(), p.feedback_relation(), n.target(),

src/compiler/js-typed-lowering.cc

@@ -1662,6 +1662,10 @@ Reduction JSTypedLowering::ReduceJSCallForwardVarargs(Node* node) {
     // Compute flags for the call.
     CallDescriptor::Flags flags = CallDescriptor::kNeedsFrameState;
     // Patch {node} to an indirect call via CallFunctionForwardVarargs.
+    // It is safe to call CallFunction instead of Call, as we already checked
+    // that the target is a function that is not a class constructor in
+    // JSCallReduer.
+    // TODO(pthier): We shouldn't blindly rely on checks made in another pass.
     Callable callable = CodeFactory::CallFunctionForwardVarargs(isolate());
     node->InsertInput(graph()->zone(), 0,
                       jsgraph()->HeapConstant(callable.code()));
@@ -1722,6 +1726,8 @@ Reduction JSTypedLowering::ReduceJSCall(Node* node) {
 
     // Class constructors are callable, but [[Call]] will raise an exception.
     // See ES6 section 9.2.1 [[Call]] ( thisArgument, argumentsList ).
+    // We need to check here in addition to JSCallReducer for Realms.
+    // TODO(pthier): Consolidate all the class constructor checks.
     if (IsClassConstructor(shared->kind())) return NoChange();
 
     // Check if we need to convert the {receiver}, but bailout if it would
@@ -1816,6 +1822,8 @@ Reduction JSTypedLowering::ReduceJSCall(Node* node) {
     // Compute flags for the call.
     CallDescriptor::Flags flags = CallDescriptor::kNeedsFrameState;
     // Patch {node} to an indirect call via the CallFunction builtin.
+    // It is safe to call CallFunction instead of Call, as we already checked
+    // that the target is a function that is not a class constructor.
     Callable callable = CodeFactory::CallFunction(isolate(), convert_mode);
     node->InsertInput(graph()->zone(), 0,
                       jsgraph()->HeapConstant(callable.code()));

test/mjsunit/regress/regress-crbug-1260623.js

+// 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
+
+// Check calling a class constructor via Reflect.apply.
+const c = class C { };
+function newC(arg1) {
+  return Reflect.apply(c, arg1, arguments);
+}
+%PrepareFunctionForOptimization(newC);
+assertThrows(newC, TypeError);
+assertThrows(newC, TypeError);
+%OptimizeFunctionOnNextCall(newC);
+assertThrows(newC, TypeError);
+
+// Check calling a class constructor with forwarded rest arguments to closure.
+function newD(...args) {
+  class D {}
+  D(...args);
+}
+%PrepareFunctionForOptimization(newD);
+assertThrows(newD, TypeError);
+assertThrows(newD, TypeError);
+%OptimizeFunctionOnNextCall(newD);
+assertThrows(newD, TypeError);