[compiler] Fix extra arguments position when reversed stack

When the interface descriptor of a builtin uses DEFINE_JS_PARAMETERS, the extra stack arguments must be positioned just above the return address, otherwise we would need to calculate its offset depending on the actual number of the arguments, we currently use a fixed offset to access them in CSA.

Therefore, these extra arguments are either the first arguments when V8_REVERSE_JSARGS is enabled or otherwise the last arguments.

Change-Id: If38ac7fd7f0079fc0e4fdccdb6cfb26e0425eb84
Bug: v8:10825
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2379854Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Auto-Submit: Victor Gomes <victorgomes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69714}

src/builtins/base.tq

@@ -608,6 +608,7 @@ extern macro CodeStubAssembler::AllocateNameDictionary(constexpr int32):
 
 extern builtin ToObject(Context, JSAny): JSReceiver;
 extern macro ToObject_Inline(Context, JSAny): JSReceiver;
+extern macro IsUndefined(Object): bool;
 extern macro IsNullOrUndefined(Object): bool;
 extern macro IsString(HeapObject): bool;
 extern macro IsSeqOneByteString(HeapObject): bool;

src/builtins/ic-callable.tq

@@ -53,6 +53,12 @@ macro TransitionToMegamorphic(implicit context: Context)(
 macro CollectCallFeedback(
     maybeTarget: JSAny, context: Context,
     maybeFeedbackVector: Undefined|FeedbackVector, slotId: uintptr): void {
+  // TODO(v8:9891): Remove this assert once all callers are ported to Torque.
+  // This assert ensures correctness of maybeFeedbackVector's type which can
+  // be easily broken for calls from CSA.
+  assert(
+      IsUndefined(maybeFeedbackVector) ||
+      Is<FeedbackVector>(maybeFeedbackVector));
   const feedbackVector =
       Cast<FeedbackVector>(maybeFeedbackVector) otherwise return;
   IncrementCallCount(feedbackVector, slotId);
@@ -97,6 +103,12 @@ macro CollectCallFeedback(
 macro CollectInstanceOfFeedback(
     maybeTarget: JSAny, context: Context,
     maybeFeedbackVector: Undefined|FeedbackVector, slotId: uintptr): void {
+  // TODO(v8:9891): Remove this assert once all callers are ported to Torque.
+  // This assert ensures correctness of maybeFeedbackVector's type which can
+  // be easily broken for calls from CSA.
+  assert(
+      IsUndefined(maybeFeedbackVector) ||
+      Is<FeedbackVector>(maybeFeedbackVector));
   const feedbackVector =
       Cast<FeedbackVector>(maybeFeedbackVector) otherwise return;
   // Note: The call count is not incremented.
@@ -134,6 +146,12 @@ macro CollectConstructFeedback(implicit context: Context)(
     maybeFeedbackVector: Undefined|FeedbackVector,
     slotId: uintptr): never labels ConstructGeneric,
     ConstructArray(AllocationSite) {
+  // TODO(v8:9891): Remove this assert once all callers are ported to Torque.
+  // This assert ensures correctness of maybeFeedbackVector's type which can
+  // be easily broken for calls from CSA.
+  assert(
+      IsUndefined(maybeFeedbackVector) ||
+      Is<FeedbackVector>(maybeFeedbackVector));
   const feedbackVector = Cast<FeedbackVector>(maybeFeedbackVector)
       otherwise goto ConstructGeneric;
   IncrementCallCount(feedbackVector, slotId);

src/builtins/internal.tq

@@ -14,11 +14,17 @@ extern runtime BytecodeBudgetInterruptFromCode(implicit context: Context)(
 builtin GetTemplateObject(
     context: Context, shared: SharedFunctionInfo,
     description: TemplateObjectDescription, slot: uintptr,
-    maybeFeedbackVector: FeedbackVector|Undefined): JSArray {
+    maybeFeedbackVector: Undefined|FeedbackVector): JSArray {
   // TODO(jgruber): Consider merging with the GetTemplateObject bytecode
   // handler; the current advantage of the split implementation is that the
   // bytecode can skip most work if feedback exists.
 
+  // TODO(v8:9891): Remove this assert once all callers are ported to Torque.
+  // This assert ensures correctness of maybeFeedbackVector's type which can
+  // be easily broken for calls from CSA.
+  assert(
+      IsUndefined(maybeFeedbackVector) ||
+      Is<FeedbackVector>(maybeFeedbackVector));
   try {
     const vector =
         Cast<FeedbackVector>(maybeFeedbackVector) otherwise CallRuntime;

src/builtins/iterator.tq

@@ -50,9 +50,16 @@ extern builtin IterableToFixedArrayWithSymbolLookupSlow(
 
 transitioning builtin GetIteratorWithFeedback(
     context: Context, receiver: JSAny, loadSlot: TaggedIndex,
-    callSlot: TaggedIndex, feedback: Undefined|FeedbackVector): JSAny {
+    callSlot: TaggedIndex,
+    maybeFeedbackVector: Undefined|FeedbackVector): JSAny {
+  // TODO(v8:9891): Remove this assert once all callers are ported to Torque.
+  // This assert ensures correctness of maybeFeedbackVector's type which can
+  // be easily broken for calls from CSA.
+  assert(
+      IsUndefined(maybeFeedbackVector) ||
+      Is<FeedbackVector>(maybeFeedbackVector));
   let iteratorMethod: JSAny;
-  typeswitch (feedback) {
+  typeswitch (maybeFeedbackVector) {
     case (Undefined): {
       iteratorMethod = GetProperty(receiver, IteratorSymbolConstant());
     }
@@ -64,7 +71,7 @@ transitioning builtin GetIteratorWithFeedback(
   // TODO(v8:10047): Use TaggedIndex here once TurboFan supports it.
   const callSlotSmi: Smi = TaggedIndexToSmi(callSlot);
   return CallIteratorWithFeedback(
-      context, receiver, iteratorMethod, callSlotSmi, feedback);
+      context, receiver, iteratorMethod, callSlotSmi, maybeFeedbackVector);
 }
 
 transitioning builtin CallIteratorWithFeedback(

src/codegen/interface-descriptors.h

@@ -503,6 +503,10 @@ STATIC_ASSERT(kMaxTFSBuiltinRegisterParams <= kMaxBuiltinRegisterParams);
   DEFINE_RESULT_AND_PARAMETER_TYPES(MachineType::AnyTagged() /* result */, \
                                     ##__VA_ARGS__)
 
+// When the extra arguments described here are located in the stack, they are
+// just above the return address in the frame. Therefore, they are either the
+// first arguments when V8_REVERSE_JSARGS is enabled, or otherwise the last
+// arguments.
 #define DEFINE_JS_PARAMETERS(...)                           \
   static constexpr int kDescriptorFlags =                   \
       CallInterfaceDescriptorData::kAllowVarArgs;           \

src/compiler/js-generic-lowering.cc

@@ -529,9 +529,14 @@ void JSGenericLowering::LowerJSCreateArguments(Node* node) {
 void JSGenericLowering::LowerJSCreateArray(Node* node) {
   CreateArrayParameters const& p = CreateArrayParametersOf(node->op());
   int const arity = static_cast<int>(p.arity());
+  auto interface_descriptor = ArrayConstructorDescriptor{};
   auto call_descriptor = Linkage::GetStubCallDescriptor(
-      zone(), ArrayConstructorDescriptor{}, arity + 1,
-      CallDescriptor::kNeedsFrameState, node->op()->properties());
+      zone(), interface_descriptor, arity + 1, CallDescriptor::kNeedsFrameState,
+      node->op()->properties());
+  // If this fails, we might need to update the parameter reordering code
+  // to ensure that the additional arguments passed via stack are pushed
+  // between top of stack and JS arguments.
+  DCHECK_EQ(interface_descriptor.GetStackParameterCount(), 0);
   Node* stub_code = jsgraph()->ArrayConstructorStubConstant();
   Node* stub_arity = jsgraph()->Int32Constant(arity);
   MaybeHandle<AllocationSite> const maybe_site = p.site();
@@ -780,6 +785,10 @@ void JSGenericLowering::LowerJSConstructForwardVarargs(Node* node) {
   int const arg_count = static_cast<int>(p.arity() - 2);
   CallDescriptor::Flags flags = FrameStateFlagForCall(node);
   Callable callable = CodeFactory::ConstructForwardVarargs(isolate());
+  // If this fails, we might need to update the parameter reordering code
+  // to ensure that the additional arguments passed via stack are pushed
+  // between top of stack and JS arguments.
+  DCHECK_EQ(callable.descriptor().GetStackParameterCount(), 0);
   auto call_descriptor = Linkage::GetStubCallDescriptor(
       zone(), callable.descriptor(), arg_count + 1, flags);
   Node* stub_code = jsgraph()->HeapConstant(callable.code());
@@ -808,12 +817,20 @@ void JSGenericLowering::LowerJSConstruct(Node* node) {
         arg_count + kReceiver + kMaybeFeedbackVector;
     Callable callable =
         Builtins::CallableFor(isolate(), Builtins::kConstruct_WithFeedback);
+    // If this fails, we might need to update the parameter reordering code
+    // to ensure that the additional arguments passed via stack are pushed
+    // between top of stack and JS arguments.
+    DCHECK_EQ(callable.descriptor().GetStackParameterCount(),
+              kMaybeFeedbackVector);
     auto call_descriptor = Linkage::GetStubCallDescriptor(
         zone(), callable.descriptor(), stack_argument_count, flags);
     Node* stub_code = jsgraph()->HeapConstant(callable.code());
     Node* stub_arity = jsgraph()->Int32Constant(arg_count);
     Node* slot = jsgraph()->Int32Constant(p.feedback().index());
     Node* receiver = jsgraph()->UndefinedConstant();
+#ifdef V8_REVERSE_JSARGS
+    Node* feedback_vector = node->RemoveInput(n.FeedbackVectorIndex());
+#endif
     // Register argument inputs are followed by stack argument inputs (such as
     // feedback_vector). Both are listed in ascending order. Note that
     // the receiver is implicitly placed on the stack and is thus inserted
@@ -822,10 +839,16 @@ void JSGenericLowering::LowerJSConstruct(Node* node) {
     node->InsertInput(zone(), 0, stub_code);
     node->InsertInput(zone(), 3, stub_arity);
     node->InsertInput(zone(), 4, slot);
+#ifdef V8_REVERSE_JSARGS
+    node->InsertInput(zone(), 5, feedback_vector);
+    node->InsertInput(zone(), 6, receiver);
+    // After: {code, target, new_target, arity, slot, vector, receiver,
+    // ...args}.
+#else
     node->InsertInput(zone(), 5, receiver);
-
     // After: {code, target, new_target, arity, slot, receiver, ...args,
     // vector}.
+#endif
 
     NodeProperties::ChangeOp(node, common()->Call(call_descriptor));
   } else {
@@ -864,12 +887,19 @@ void JSGenericLowering::LowerJSConstructWithArrayLike(Node* node) {
         arg_count - kArgumentList + kReceiver + kMaybeFeedbackVector;
     Callable callable = Builtins::CallableFor(
         isolate(), Builtins::kConstructWithArrayLike_WithFeedback);
+    // If this fails, we might need to update the parameter reordering code
+    // to ensure that the additional arguments passed via stack are pushed
+    // between top of stack and JS arguments.
+    DCHECK_EQ(callable.descriptor().GetStackParameterCount(),
+              kMaybeFeedbackVector);
     auto call_descriptor = Linkage::GetStubCallDescriptor(
         zone(), callable.descriptor(), stack_argument_count, flags);
     Node* stub_code = jsgraph()->HeapConstant(callable.code());
     Node* receiver = jsgraph()->UndefinedConstant();
     Node* slot = jsgraph()->Int32Constant(p.feedback().index());
-
+#ifdef V8_REVERSE_JSARGS
+    Node* feedback_vector = node->RemoveInput(n.FeedbackVectorIndex());
+#endif
     // Register argument inputs are followed by stack argument inputs (such as
     // feedback_vector). Both are listed in ascending order. Note that
     // the receiver is implicitly placed on the stack and is thus inserted
@@ -877,16 +907,26 @@ void JSGenericLowering::LowerJSConstructWithArrayLike(Node* node) {
     // TODO(jgruber): Implement a simpler way to specify these mutations.
     node->InsertInput(zone(), 0, stub_code);
     node->InsertInput(zone(), 4, slot);
+#ifdef V8_REVERSE_JSARGS
+    node->InsertInput(zone(), 5, feedback_vector);
+    node->InsertInput(zone(), 6, receiver);
+    // After: {code, target, new_target, arguments_list, slot, vector,
+    // receiver}.
+#else
     node->InsertInput(zone(), 5, receiver);
-
     // After: {code, target, new_target, arguments_list, slot, receiver,
     // vector}.
+#endif
 
     NodeProperties::ChangeOp(node, common()->Call(call_descriptor));
   } else {
     const int stack_argument_count = arg_count - kArgumentList + kReceiver;
     Callable callable =
         Builtins::CallableFor(isolate(), Builtins::kConstructWithArrayLike);
+    // If this fails, we might need to update the parameter reordering code
+    // to ensure that the additional arguments passed via stack are pushed
+    // between top of stack and JS arguments.
+    DCHECK_EQ(callable.descriptor().GetStackParameterCount(), 0);
     auto call_descriptor = Linkage::GetStubCallDescriptor(
         zone(), callable.descriptor(), stack_argument_count, flags);
     Node* stub_code = jsgraph()->HeapConstant(callable.code());
@@ -918,6 +958,11 @@ void JSGenericLowering::LowerJSConstructWithSpread(Node* node) {
         arg_count + kReceiver + kMaybeFeedbackVector;
     Callable callable = Builtins::CallableFor(
         isolate(), Builtins::kConstructWithSpread_WithFeedback);
+    // If this fails, we might need to update the parameter reordering code
+    // to ensure that the additional arguments passed via stack are pushed
+    // between top of stack and JS arguments.
+    DCHECK_EQ(callable.descriptor().GetStackParameterCount(),
+              kTheSpread + kMaybeFeedbackVector);
     auto call_descriptor = Linkage::GetStubCallDescriptor(
         zone(), callable.descriptor(), stack_argument_count, flags);
     Node* stub_code = jsgraph()->HeapConstant(callable.code());
@@ -927,6 +972,10 @@ void JSGenericLowering::LowerJSConstructWithSpread(Node* node) {
     // on the stack here.
     Node* stub_arity = jsgraph()->Int32Constant(arg_count - kTheSpread);
     Node* receiver = jsgraph()->UndefinedConstant();
+#ifdef V8_REVERSE_JSARGS
+    Node* feedback_vector = node->RemoveInput(n.FeedbackVectorIndex());
+    Node* spread = node->RemoveInput(n.LastArgumentIndex());
+#endif
 
     // Register argument inputs are followed by stack argument inputs (such as
     // feedback_vector). Both are listed in ascending order. Note that
@@ -936,15 +985,26 @@ void JSGenericLowering::LowerJSConstructWithSpread(Node* node) {
     node->InsertInput(zone(), 0, stub_code);
     node->InsertInput(zone(), 3, stub_arity);
     node->InsertInput(zone(), 4, slot);
+#ifdef V8_REVERSE_JSARGS
+    node->InsertInput(zone(), 5, spread);
+    node->InsertInput(zone(), 6, feedback_vector);
+    node->InsertInput(zone(), 7, receiver);
+    // After: {code, target, new_target, arity, slot, spread, vector, receiver,
+    // ...args}.
+#else
     node->InsertInput(zone(), 5, receiver);
-
     // After: {code, target, new_target, arity, slot, receiver, ...args, spread,
     // vector}.
+#endif
 
     NodeProperties::ChangeOp(node, common()->Call(call_descriptor));
   } else {
     const int stack_argument_count = arg_count + kReceiver - kTheSpread;
     Callable callable = CodeFactory::ConstructWithSpread(isolate());
+    // If this fails, we might need to update the parameter reordering code
+    // to ensure that the additional arguments passed via stack are pushed
+    // between top of stack and JS arguments.
+    DCHECK_EQ(callable.descriptor().GetStackParameterCount(), 0);
     auto call_descriptor = Linkage::GetStubCallDescriptor(
         zone(), callable.descriptor(), stack_argument_count, flags);
     Node* stub_code = jsgraph()->HeapConstant(callable.code());
@@ -1098,6 +1158,11 @@ void JSGenericLowering::LowerJSCallWithSpread(Node* node) {
         arg_count - kTheSpread + kReceiver + kMaybeFeedbackVector;
     Callable callable = Builtins::CallableFor(
         isolate(), Builtins::kCallWithSpread_WithFeedback);
+    // If this fails, we might need to update the parameter reordering code
+    // to ensure that the additional arguments passed via stack are pushed
+    // between top of stack and JS arguments.
+    DCHECK_EQ(callable.descriptor().GetStackParameterCount(),
+              kMaybeFeedbackVector);
     auto call_descriptor = Linkage::GetStubCallDescriptor(
         zone(), callable.descriptor(), stack_argument_count, flags);
     Node* stub_code = jsgraph()->HeapConstant(callable.code());
@@ -1114,22 +1179,29 @@ void JSGenericLowering::LowerJSCallWithSpread(Node* node) {
 
     // Shuffling inputs.
     // Before: {target, receiver, ...args, spread, vector}.
-
+#ifdef V8_REVERSE_JSARGS
+    Node* feedback_vector = node->RemoveInput(n.FeedbackVectorIndex());
+#endif
     Node* spread = node->RemoveInput(n.LastArgumentIndex());
-
-    // Now: {target, receiver, ...args, vector}.
-
     node->InsertInput(zone(), 0, stub_code);
     node->InsertInput(zone(), 2, stub_arity);
     node->InsertInput(zone(), 3, spread);
     node->InsertInput(zone(), 4, slot);
-
+#ifdef V8_REVERSE_JSARGS
+    node->InsertInput(zone(), 5, feedback_vector);
+    // After: {code, target, arity, spread, slot, vector, receiver, ...args}.
+#else
     // After: {code, target, arity, spread, slot, receiver, ...args, vector}.
+#endif
 
     NodeProperties::ChangeOp(node, common()->Call(call_descriptor));
   } else {
     const int stack_argument_count = arg_count - kTheSpread + kReceiver;
     Callable callable = CodeFactory::CallWithSpread(isolate());
+    // If this fails, we might need to update the parameter reordering code
+    // to ensure that the additional arguments passed via stack are pushed
+    // between top of stack and JS arguments.
+    DCHECK_EQ(callable.descriptor().GetStackParameterCount(), 0);
     auto call_descriptor = Linkage::GetStubCallDescriptor(
         zone(), callable.descriptor(), stack_argument_count, flags);
     Node* stub_code = jsgraph()->HeapConstant(callable.code());