[class] Fix super call evaluation order

Fix super calls so that arguments are evaluated before the
super constructor is checked to be in fact a constructor.

A new bytecode is introduced to split the IsConstructor check
out from the current GetSuperConstructor bytecode.

Bug: v8:10111
Change-Id: I3af99e32a34d99493806bb01b547d6f671cdc9de
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2493077
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70881}

src/builtins/builtins-definitions.h

@@ -196,7 +196,6 @@ namespace internal {
   TFC(PlainPrimitiveToNumber, TypeConversionNoContext)                         \
   TFC(ToNumberConvertBigInt, TypeConversion)                                   \
   TFC(Typeof, Typeof)                                                          \
-  TFC(GetSuperConstructor, Typeof)                                             \
   TFC(BigIntToI64, BigIntToI64)                                                \
   TFC(BigIntToI32Pair, BigIntToI32Pair)                                        \
   TFC(I64ToBigInt, I64ToBigInt)                                                \

src/builtins/builtins-object-gen.cc

@@ -1174,13 +1174,6 @@ TF_BUILTIN(OrdinaryHasInstance, ObjectBuiltinsAssembler) {
   Return(OrdinaryHasInstance(context, constructor, object));
 }
 
-TF_BUILTIN(GetSuperConstructor, ObjectBuiltinsAssembler) {
-  auto object = Parameter<JSFunction>(Descriptor::kObject);
-  auto context = Parameter<Context>(Descriptor::kContext);
-
-  Return(GetSuperConstructor(context, object));
-}
-
 TF_BUILTIN(CreateGeneratorObject, ObjectBuiltinsAssembler) {
   auto closure = Parameter<JSFunction>(Descriptor::kClosure);
   auto receiver = Parameter<Object>(Descriptor::kReceiver);

src/codegen/code-stub-assembler.cc

@@ -12362,28 +12362,10 @@ TNode<String> CodeStubAssembler::Typeof(SloppyTNode<Object> value) {
   return result_var.value();
 }
 
-TNode<Object> CodeStubAssembler::GetSuperConstructor(
-    TNode<Context> context, TNode<JSFunction> active_function) {
-  Label is_not_constructor(this, Label::kDeferred), out(this);
-  TVARIABLE(Object, result);
-
+TNode<HeapObject> CodeStubAssembler::GetSuperConstructor(
+    TNode<JSFunction> active_function) {
   TNode<Map> map = LoadMap(active_function);
-  TNode<HeapObject> prototype = LoadMapPrototype(map);
-  TNode<Map> prototype_map = LoadMap(prototype);
-  GotoIfNot(IsConstructorMap(prototype_map), &is_not_constructor);
-
-  result = prototype;
-  Goto(&out);
-
-  BIND(&is_not_constructor);
-  {
-    CallRuntime(Runtime::kThrowNotSuperConstructor, context, prototype,
-                active_function);
-    Unreachable();
-  }
-
-  BIND(&out);
-  return result.value();
+  return LoadMapPrototype(map);
 }
 
 TNode<JSReceiver> CodeStubAssembler::SpeciesConstructor(

src/codegen/code-stub-assembler.h

@@ -3378,8 +3378,7 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
 
   TNode<String> Typeof(SloppyTNode<Object> value);
 
-  TNode<Object> GetSuperConstructor(TNode<Context> context,
-                                    TNode<JSFunction> active_function);
+  TNode<HeapObject> GetSuperConstructor(TNode<JSFunction> active_function);
 
   TNode<JSReceiver> SpeciesConstructor(
       SloppyTNode<Context> context, SloppyTNode<Object> object,

src/compiler/bytecode-graph-builder.cc

@@ -2899,6 +2899,31 @@ void BytecodeGraphBuilder::VisitThrowSuperAlreadyCalledIfNotHole() {
                          Runtime::kThrowSuperAlreadyCalledError);
 }
 
+void BytecodeGraphBuilder::VisitThrowIfNotSuperConstructor() {
+  Node* constructor =
+      environment()->LookupRegister(bytecode_iterator().GetRegisterOperand(0));
+  Node* check_is_constructor =
+      NewNode(simplified()->ObjectIsConstructor(), constructor);
+  NewBranch(check_is_constructor, BranchHint::kTrue);
+  {
+    SubEnvironment sub_environment(this);
+    NewIfFalse();
+    BuildLoopExitsForFunctionExit(bytecode_analysis().GetInLivenessFor(
+        bytecode_iterator().current_offset()));
+    Node* node =
+        NewNode(javascript()->CallRuntime(Runtime::kThrowNotSuperConstructor),
+                constructor, GetFunctionClosure());
+    environment()->RecordAfterState(node, Environment::kAttachFrameState);
+    Node* control = NewNode(common()->Throw());
+    MergeControlToLeaveFunction(control);
+  }
+  NewIfTrue();
+
+  constructor = NewNode(common()->TypeGuard(Type::Callable()), constructor);
+  environment()->BindRegister(bytecode_iterator().GetRegisterOperand(0),
+                              constructor);
+}
+
 void BytecodeGraphBuilder::BuildUnaryOp(const Operator* op) {
   DCHECK(JSOperator::IsUnaryWithFeedback(op->opcode()));
   PrepareEagerCheckpoint();

src/compiler/js-generic-lowering.cc

@@ -7,6 +7,7 @@
 #include "src/ast/ast.h"
 #include "src/builtins/builtins-constructor.h"
 #include "src/codegen/code-factory.h"
+#include "src/compiler/access-builder.h"
 #include "src/compiler/common-operator.h"
 #include "src/compiler/js-graph.h"
 #include "src/compiler/js-heap-broker.h"
@@ -15,6 +16,7 @@
 #include "src/compiler/node-properties.h"
 #include "src/compiler/operator-properties.h"
 #include "src/compiler/processed-feedback.h"
+#include "src/compiler/simplified-operator.h"
 #include "src/objects/feedback-cell.h"
 #include "src/objects/feedback-vector.h"
 #include "src/objects/scope-info.h"
@@ -480,7 +482,21 @@ void JSGenericLowering::LowerJSDeleteProperty(Node* node) {
 }
 
 void JSGenericLowering::LowerJSGetSuperConstructor(Node* node) {
-  ReplaceWithBuiltinCall(node, Builtins::kGetSuperConstructor);
+  Node* active_function = NodeProperties::GetValueInput(node, 0);
+  Node* effect = NodeProperties::GetEffectInput(node);
+  Node* control = NodeProperties::GetControlInput(node);
+
+  Node* function_map = effect = graph()->NewNode(
+      jsgraph()->simplified()->LoadField(AccessBuilder::ForMap()),
+      active_function, effect, control);
+
+  RelaxControls(node);
+  node->ReplaceInput(0, function_map);
+  node->ReplaceInput(1, effect);
+  node->ReplaceInput(2, control);
+  node->TrimInputCount(3);
+  NodeProperties::ChangeOp(node, jsgraph()->simplified()->LoadField(
+                                     AccessBuilder::ForMapPrototype()));
 }
 
 void JSGenericLowering::LowerJSHasInPrototypeChain(Node* node) {

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

@@ -360,13 +360,12 @@ Reduction JSNativeContextSpecialization::ReduceJSGetSuperConstructor(
     TRACE_BROKER_MISSING(broker(), "data for map " << function_map);
     return NoChange();
   }
-  ObjectRef function_prototype = function_map.prototype();
+  HeapObjectRef function_prototype = function_map.prototype();
 
   // We can constant-fold the super constructor access if the
   // {function}s map is stable, i.e. we can use a code dependency
   // to guard against [[Prototype]] changes of {function}.
-  if (function_map.is_stable() && function_prototype.IsHeapObject() &&
-      function_prototype.AsHeapObject().map().is_constructor()) {
+  if (function_map.is_stable()) {
     dependencies()->DependOnStableMap(function_map);
     Node* value = jsgraph()->Constant(function_prototype);
     ReplaceWithValue(node, value);

src/compiler/js-operator.cc

@@ -709,7 +709,7 @@ ForInParameters const& ForInParametersOf(const Operator* op) {
   V(PromiseResolve, Operator::kNoProperties, 2, 1)                       \
   V(RejectPromise, Operator::kNoDeopt | Operator::kNoThrow, 3, 1)        \
   V(ResolvePromise, Operator::kNoDeopt | Operator::kNoThrow, 2, 1)       \
-  V(GetSuperConstructor, Operator::kNoWrite, 1, 1)                       \
+  V(GetSuperConstructor, Operator::kNoWrite | Operator::kNoThrow, 1, 1)  \
   V(ParseInt, Operator::kNoProperties, 2, 1)                             \
   V(RegExpTest, Operator::kNoProperties, 2, 1)
 

src/compiler/serializer-for-background-compilation.cc

@@ -101,6 +101,7 @@ namespace compiler {
   V(IncBlockCounter)                  \
   V(ResumeGenerator)                  \
   V(SuspendGenerator)                 \
+  V(ThrowIfNotSuperConstructor)       \
   V(ThrowSuperAlreadyCalledIfNotHole) \
   V(ThrowSuperNotCalledIfHole)        \
   V(ToObject)

src/compiler/typer.cc

@@ -576,6 +576,10 @@ Type Typer::Visitor::ObjectIsCallable(Type type, Typer* t) {
 Type Typer::Visitor::ObjectIsConstructor(Type type, Typer* t) {
   // TODO(turbofan): Introduce a Type::Constructor?
   CHECK(!type.IsNone());
+  if (type.IsHeapConstant() &&
+      type.AsHeapConstant()->Ref().map().is_constructor()) {
+    return t->singleton_true_;
+  }
   if (!type.Maybe(Type::Callable())) return t->singleton_false_;
   return Type::Boolean();
 }
@@ -1434,7 +1438,7 @@ Type Typer::Visitor::JSOrdinaryHasInstanceTyper(Type lhs, Type rhs, Typer* t) {
 }
 
 Type Typer::Visitor::TypeJSGetSuperConstructor(Node* node) {
-  return Type::Callable();
+  return Type::NonInternal();
 }
 
 // JS context operators.

src/compiler/verifier.cc

@@ -772,7 +772,7 @@ void Verifier::Visitor::Check(Node* node, const AllNodes& all) {
       // We don't check the input for Type::Function because this_function can
       // be context-allocated.
       CheckValueInputIs(node, 0, Type::Any());
-      CheckTypeIs(node, Type::Callable());
+      CheckTypeIs(node, Type::NonInternal());
       break;
 
     case IrOpcode::kJSHasContextExtension:

src/interpreter/bytecode-array-builder.cc

@@ -1330,6 +1330,12 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::ThrowSuperAlreadyCalledIfNotHole() {
   return *this;
 }
 
+BytecodeArrayBuilder& BytecodeArrayBuilder::ThrowIfNotSuperConstructor(
+    Register constructor) {
+  OutputThrowIfNotSuperConstructor(constructor);
+  return *this;
+}
+
 BytecodeArrayBuilder& BytecodeArrayBuilder::Debugger() {
   OutputDebugger();
   return *this;

src/interpreter/bytecode-array-builder.h

@@ -459,6 +459,7 @@ class V8_EXPORT_PRIVATE BytecodeArrayBuilder final {
   BytecodeArrayBuilder& ThrowReferenceErrorIfHole(const AstRawString* name);
   BytecodeArrayBuilder& ThrowSuperNotCalledIfHole();
   BytecodeArrayBuilder& ThrowSuperAlreadyCalledIfNotHole();
+  BytecodeArrayBuilder& ThrowIfNotSuperConstructor(Register constructor);
 
   // Debugger.
   BytecodeArrayBuilder& Debugger();

src/interpreter/bytecode-generator.cc

@@ -5019,6 +5019,9 @@ void BytecodeGenerator::VisitCallSuper(Call* expr) {
     // First generate the array containing all arguments.
     BuildCreateArrayLiteral(args, nullptr);
 
+    // Check if the constructor is in fact a constructor.
+    builder()->ThrowIfNotSuperConstructor(constructor);
+
     // Now pass that array to %reflect_construct.
     RegisterList construct_args = register_allocator()->NewRegisterList(3);
     builder()->StoreAccumulatorInRegister(construct_args[1]);
@@ -5028,6 +5031,10 @@ void BytecodeGenerator::VisitCallSuper(Call* expr) {
   } else {
     RegisterList args_regs = register_allocator()->NewGrowableRegisterList();
     VisitArguments(args, &args_regs);
+
+    // Check if the constructor is in fact a constructor.
+    builder()->ThrowIfNotSuperConstructor(constructor);
+
     // The new target is loaded into the accumulator from the
     // {new.target} variable.
     VisitForAccumulatorValue(super->new_target_var());

src/interpreter/bytecodes.h

@@ -345,6 +345,7 @@ namespace interpreter {
   V(ThrowReferenceErrorIfHole, AccumulatorUse::kRead, OperandType::kIdx)       \
   V(ThrowSuperNotCalledIfHole, AccumulatorUse::kRead)                          \
   V(ThrowSuperAlreadyCalledIfNotHole, AccumulatorUse::kRead)                   \
+  V(ThrowIfNotSuperConstructor, AccumulatorUse::kNone, OperandType::kReg)      \
                                                                                \
   /* Generators */                                                             \
   V(SwitchOnGeneratorState, AccumulatorUse::kNone, OperandType::kReg,          \

src/interpreter/interpreter-generator.cc

@@ -1340,8 +1340,7 @@ IGNITION_HANDLER(DeletePropertySloppy, InterpreterAssembler) {
 // The result is stored in register |reg|.
 IGNITION_HANDLER(GetSuperConstructor, InterpreterAssembler) {
   TNode<JSFunction> active_function = CAST(GetAccumulator());
-  TNode<Context> context = GetContext();
-  TNode<Object> result = GetSuperConstructor(context, active_function);
+  TNode<Object> result = GetSuperConstructor(active_function);
   StoreRegisterAtOperandIndex(result, 0);
   Dispatch();
 }
@@ -2722,7 +2721,7 @@ IGNITION_HANDLER(ThrowSuperNotCalledIfHole, InterpreterAssembler) {
 
 // ThrowSuperAlreadyCalledIfNotHole
 //
-// Throws SuperAleradyCalled exception if the value in the accumulator is not
+// Throws SuperAlreadyCalled exception if the value in the accumulator is not
 // TheHole.
 IGNITION_HANDLER(ThrowSuperAlreadyCalledIfNotHole, InterpreterAssembler) {
   TNode<Object> value = GetAccumulator();
@@ -2740,6 +2739,31 @@ IGNITION_HANDLER(ThrowSuperAlreadyCalledIfNotHole, InterpreterAssembler) {
   }
 }
 
+// ThrowIfNotSuperConstructor <constructor>
+//
+// Throws an exception if the value in |constructor| is not in fact a
+// constructor.
+IGNITION_HANDLER(ThrowIfNotSuperConstructor, InterpreterAssembler) {
+  TNode<HeapObject> constructor = CAST(LoadRegisterAtOperandIndex(0));
+  TNode<Context> context = GetContext();
+
+  Label is_not_constructor(this, Label::kDeferred);
+  TNode<Map> constructor_map = LoadMap(constructor);
+  GotoIfNot(IsConstructorMap(constructor_map), &is_not_constructor);
+  Dispatch();
+
+  BIND(&is_not_constructor);
+  {
+    TNode<JSFunction> function =
+        CAST(LoadRegister(Register::function_closure()));
+    CallRuntime(Runtime::kThrowNotSuperConstructor, context, constructor,
+                function);
+    // We shouldn't ever return from a throw.
+    Abort(AbortReason::kUnexpectedReturnFromThrow);
+    Unreachable();
+  }
+}
+
 // Debugger
 //
 // Call runtime to handle debugger statement.

src/runtime/runtime-classes.cc

@@ -2,15 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "src/runtime/runtime-utils.h"
-
 #include <stdlib.h>
+
 #include <limits>
 
 #include "src/builtins/accessors.h"
 #include "src/common/message-template.h"
 #include "src/debug/debug.h"
 #include "src/execution/arguments-inl.h"
+#include "src/execution/frames-inl.h"
 #include "src/execution/isolate-inl.h"
 #include "src/logging/counters.h"
 #include "src/logging/log.h"
@@ -20,6 +20,7 @@
 #include "src/objects/lookup-inl.h"
 #include "src/objects/smi.h"
 #include "src/objects/struct-inl.h"
+#include "src/runtime/runtime-utils.h"
 #include "src/runtime/runtime.h"
 
 namespace v8 {

test/cctest/interpreter/bytecode_expectations/ClassAndSuperClass.golden

@@ -104,12 +104,13 @@ snippet: "
 "
 frame size: 6
 parameter count: 1
-bytecode array length: 39
+bytecode array length: 41
 bytecodes: [
                 B(Mov), R(closure), R(1),
   /*  118 S> */ B(Ldar), R(1),
                 B(GetSuperConstructor), R(3),
                 B(LdaSmi), I8(1),
+                B(ThrowIfNotSuperConstructor), R(3),
                 B(Star), R(4),
                 B(Ldar), R(0),
   /*  118 E> */ B(Construct), R(3), R(4), U8(1), U8(0),
@@ -146,11 +147,12 @@ snippet: "
 "
 frame size: 5
 parameter count: 1
-bytecode array length: 35
+bytecode array length: 37
 bytecodes: [
                 B(Mov), R(closure), R(1),
   /*  117 S> */ B(Ldar), R(1),
                 B(GetSuperConstructor), R(3),
+                B(ThrowIfNotSuperConstructor), R(3),
                 B(Ldar), R(0),
   /*  117 E> */ B(Construct), R(3), R(0), U8(0), U8(0),
                 B(Star), R(4),

test/cctest/interpreter/bytecode_expectations/SuperCallAndSpread.golden

@@ -19,13 +19,14 @@ snippet: "
 "
 frame size: 5
 parameter count: 1
-bytecode array length: 18
+bytecode array length: 20
 bytecodes: [
   /*   93 E> */ B(CreateRestParameter),
                 B(Star), R(2),
                 B(Mov), R(closure), R(1),
   /*   93 S> */ B(Ldar), R(1),
                 B(GetSuperConstructor), R(4),
+                B(ThrowIfNotSuperConstructor), R(4),
                 B(Ldar), R(0),
   /*   93 E> */ B(ConstructWithSpread), R(4), R(2), U8(1), U8(0),
   /*   93 S> */ B(Return),
@@ -50,7 +51,7 @@ snippet: "
 "
 frame size: 9
 parameter count: 1
-bytecode array length: 39
+bytecode array length: 41
 bytecodes: [
   /*  128 E> */ B(CreateRestParameter),
                 B(Star), R(3),
@@ -60,6 +61,7 @@ bytecodes: [
                 B(GetSuperConstructor), R(5),
                 B(LdaSmi), I8(1),
                 B(Star), R(6),
+  /*  152 E> */ B(ThrowIfNotSuperConstructor), R(5),
                 B(Ldar), R(0),
                 B(Mov), R(3), R(7),
   /*  140 E> */ B(ConstructWithSpread), R(5), R(6), U8(2), U8(0),
@@ -91,7 +93,7 @@ snippet: "
 "
 frame size: 11
 parameter count: 1
-bytecode array length: 118
+bytecode array length: 120
 bytecodes: [
   /*  128 E> */ B(CreateRestParameter),
                 B(Star), R(3),
@@ -129,6 +131,7 @@ bytecodes: [
                 B(JumpLoop), U8(33), I8(0),
                 B(LdaSmi), I8(1),
                 B(StaInArrayLiteral), R(7), R(6), U8(1),
+                B(ThrowIfNotSuperConstructor), R(5),
                 B(Mov), R(5), R(6),
                 B(Mov), R(0), R(8),
   /*  140 E> */ B(CallJSRuntime), U8(%reflect_construct), R(6), U8(3),

test/mjsunit/regress/regress-6248.js

@@ -23,4 +23,4 @@ assertSame(sentinelObject, new C());
 assertSame(sentinelObject, new C());
 %OptimizeFunctionOnNextCall(C)
 assertSame(sentinelObject, new C());
-assertFalse(evaluatedArg);
+assertTrue(evaluatedArg);

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

@@ -9,7 +9,9 @@ class D extends C { constructor() { super(...unresolved, 75) } }
 D.__proto__ = null;
 
 %PrepareFunctionForOptimization(D);
-assertThrows(() => new D(), TypeError);
-assertThrows(() => new D(), TypeError);
+// ReferenceError because argument evaluation happens before calling the super
+// constructor.
+assertThrows(() => new D(), ReferenceError);
+assertThrows(() => new D(), ReferenceError);
 %OptimizeFunctionOnNextCall(D);
-assertThrows(() => new D(), TypeError);
+assertThrows(() => new D(), ReferenceError);

test/test262/test262.status

@@ -571,10 +571,6 @@
   # https://bugs.chromium.org/p/v8/issues/detail?id=9818
   'built-ins/AsyncFunction/proto-from-ctor-realm': [FAIL],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=10111
-  # super() should evaluate arguments before checking IsConstructable
-  'language/expressions/super/call-proto-not-ctor': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=10381
   'built-ins/Array/prototype/concat/arg-length-near-integer-limit': [FAIL],
 

test/unittests/interpreter/bytecode-array-builder-unittest.cc

@@ -277,6 +277,9 @@ TEST_F(BytecodeArrayBuilderTest, AllBytecodesGenerated) {
   // Emit GetSuperConstructor.
   builder.GetSuperConstructor(reg);
 
+  // Constructor check for GetSuperConstructor.
+  builder.ThrowIfNotSuperConstructor(reg);
+
   // Hole checks.
   builder.ThrowReferenceErrorIfHole(name)
       .ThrowSuperAlreadyCalledIfNotHole()