Revert "[wasm] Materialize suspender in JS-to-wasm wrapper"

This reverts commit 8cb02753.

Reason for revert: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20TSAN%20-%20isolates/20736/overview

Original change's description:
> [wasm] Materialize suspender in JS-to-wasm wrapper
>
> Instead of creating the Suspender object in JS and passing it to the
> stack-switching js-to-wasm wrapper, the wrapper now automatically
> creates the Suspender object and forwards it as an extra parameter to
> the wasm function. See:
> https://github.com/WebAssembly/js-promise-integration/pull/1/files
>
> R=​ahaas@chromium.org
>
> Bug: v8:12191
> Change-Id: I2badee823f4223a293632f93e7e59f24c49d0820
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3779688
> Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
> Reviewed-by: Andreas Haas <ahaas@chromium.org>
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#81890}

Bug: v8:12191
Change-Id: Id22ed357e3a59bd1569687eadbc9b007d3da995c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3780816
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Nico Hartmann <nicohartmann@chromium.org>
Owners-Override: Nico Hartmann <nicohartmann@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#81891}

src/builtins/x64/builtins-x64.cc

@@ -2999,19 +2999,20 @@ void SaveState(MacroAssembler* masm, Register active_continuation, Register tmp,
   FillJumpBuffer(masm, jmpbuf, suspend);
 }
 
-// Returns the new suspender in rax.
-void AllocateSuspender(MacroAssembler* masm, Register function_data,
-                       Register wasm_instance) {
+// Returns the new continuation in rax.
+void AllocateContinuation(MacroAssembler* masm, Register function_data,
+                          Register wasm_instance, Register suspender) {
   MemOperand GCScanSlotPlace =
       MemOperand(rbp, BuiltinWasmWrapperConstants::kGCScanSlotCountOffset);
-  __ Move(GCScanSlotPlace, 2);
+  __ Move(GCScanSlotPlace, 3);
   __ Push(wasm_instance);
   __ Push(function_data);
+  __ Push(suspender);  // Argument.
   __ LoadAnyTaggedField(
       kContextRegister,
       MemOperand(wasm_instance, wasm::ObjectAccess::ToTagged(
                                     WasmInstanceObject::kNativeContextOffset)));
-  __ CallRuntime(Runtime::kWasmAllocateSuspender);
+  __ CallRuntime(Runtime::kWasmAllocateContinuation);
   __ Pop(function_data);
   __ Pop(wasm_instance);
   static_assert(kReturnRegister0 == rax);
@@ -3173,9 +3174,7 @@ void GenericJSToWasmWrapperHelper(MacroAssembler* masm, bool stack_switch) {
   // The number of parameters according to the signature.
   constexpr int kParamCountOffset =
       BuiltinWasmWrapperConstants::kParamCountOffset;
-  constexpr int kSuspenderOffset =
-      BuiltinWasmWrapperConstants::kSuspenderOffset;
-  constexpr int kReturnCountOffset = kSuspenderOffset - kSystemPointerSize;
+  constexpr int kReturnCountOffset = kParamCountOffset - kSystemPointerSize;
   constexpr int kValueTypesArrayStartOffset =
       kReturnCountOffset - kSystemPointerSize;
   // A boolean flag to check if one of the parameters is a reference. If so, we
@@ -3187,9 +3186,7 @@ void GenericJSToWasmWrapperHelper(MacroAssembler* masm, bool stack_switch) {
   // registers (so no GC scan is needed).
   constexpr int kFunctionDataOffset = kHasRefTypesOffset - kSystemPointerSize;
   constexpr int kLastSpillOffset = kFunctionDataOffset;
-  constexpr int kNumSpillSlots =
-      (-TypedFrameConstants::kFixedFrameSizeFromFp - kLastSpillOffset) >>
-      kSystemPointerSizeLog2;
+  constexpr int kNumSpillSlots = 7;
   __ subq(rsp, Immediate(kNumSpillSlots * kSystemPointerSize));
   // Put the in_parameter count on the stack, we only  need it at the very end
   // when we pop the parameters off the stack.
@@ -3224,21 +3221,17 @@ void GenericJSToWasmWrapperHelper(MacroAssembler* masm, bool stack_switch) {
 
   Label suspend;
   if (stack_switch) {
-    // Set the suspender spill slot to a sentinel value, in case a GC happens
-    // before we set the actual value.
-    __ LoadRoot(kScratchRegister, RootIndex::kUndefinedValue);
-    __ movq(MemOperand(rbp, kSuspenderOffset), kScratchRegister);
     Register active_continuation = rbx;
     __ LoadRoot(active_continuation, RootIndex::kActiveContinuation);
     SaveState(masm, active_continuation, rcx, &suspend);
-    AllocateSuspender(masm, function_data, wasm_instance);
-    Register suspender = rax;  // Fixed.
-    __ movq(MemOperand(rbp, kSuspenderOffset), suspender);
-    Register target_continuation = rax;
-    __ LoadAnyTaggedField(
-        target_continuation,
-        FieldOperand(suspender, WasmSuspenderObject::kContinuationOffset));
+    Register suspender = rax;
+    constexpr int kReceiverOnStackSize = kSystemPointerSize;
+    constexpr int kFirstParamOffset =
+        kFPOnStackSize + kPCOnStackSize + kReceiverOnStackSize;
+    __ movq(suspender, MemOperand(rbp, kFirstParamOffset));
+    AllocateContinuation(masm, function_data, wasm_instance, suspender);
     suspender = no_reg;
+    Register target_continuation = rax;  // fixed
     // Save the old stack's rbp in r9, and use it to access the parameters in
     // the parent frame.
     // We also distribute the spill slots across the two stacks as needed by
@@ -3256,11 +3249,9 @@ void GenericJSToWasmWrapperHelper(MacroAssembler* masm, bool stack_switch) {
     //      +-----------------+           +-----------------+
     //      |kGCScanSlotCount |           |kGCScanSlotCount |
     //      +-----------------+           +-----------------+
-    //      | kInParamCount   |           |      /          |
-    //      +-----------------+           +-----------------+
     //      | kParamCount     |           |      /          |
     //      +-----------------+           +-----------------+
-    //      | kSuspender      |           |      /          |
+    //      | kInParamCount   |           |      /          |
     //      +-----------------+           +-----------------+
     //      |      /          |           | kReturnCount    |
     //      +-----------------+           +-----------------+
@@ -3292,9 +3283,6 @@ void GenericJSToWasmWrapperHelper(MacroAssembler* masm, bool stack_switch) {
     // this marks the base of the stack segment for the stack frame iterator.
     __ EnterFrame(StackFrame::STACK_SWITCH);
     __ subq(rsp, Immediate(kNumSpillSlots * kSystemPointerSize));
-    // Set a sentinel value for the suspender spill slot in the new frame.
-    __ LoadRoot(kScratchRegister, RootIndex::kUndefinedValue);
-    __ movq(MemOperand(rbp, kSuspenderOffset), kScratchRegister);
   }
   Register original_fp = stack_switch ? r9 : rbp;
 
@@ -3436,22 +3424,6 @@ void GenericJSToWasmWrapperHelper(MacroAssembler* masm, bool stack_switch) {
   returns_size = no_reg;
   Register valuetype = r12;
 
-  Label numeric_params_done;
-  if (stack_switch) {
-    // Prepare for materializing the suspender parameter. We don't materialize
-    // it here but in the next loop that processes references. Here we only
-    // adjust the pointers to keep the state consistent:
-    // - Skip the first valuetype in the signature,
-    // - Adjust the param limit which is off by one because of the extra
-    // param in the signature,
-    // - Set HasRefTypes to 1 to ensure that the reference loop is entered.
-    __ addq(valuetypes_array_ptr, Immediate(kValueTypeSize));
-    __ subq(param_limit, Immediate(kSystemPointerSize));
-    __ movq(MemOperand(rbp, kHasRefTypesOffset), Immediate(1));
-    __ cmpq(current_param, param_limit);
-    __ j(equal, &numeric_params_done);
-  }
-
   // -------------------------------------------
   // Param evaluation loop.
   // -------------------------------------------
@@ -3471,7 +3443,7 @@ void GenericJSToWasmWrapperHelper(MacroAssembler* masm, bool stack_switch) {
   __ cmpq(valuetype, Immediate(wasm::kWasmI32.raw_bit_field()));
   __ j(not_equal, &convert_param);
   __ JumpIfNotSmi(param, &convert_param);
-  // Change the param from Smi to int32.
+  // Change the paramfrom Smi to int32.
   __ SmiUntag(param);
   // Zero extend.
   __ movl(param, param);
@@ -3490,7 +3462,6 @@ void GenericJSToWasmWrapperHelper(MacroAssembler* masm, bool stack_switch) {
 
   __ cmpq(current_param, param_limit);
   __ j(not_equal, &loop_through_params);
-  __ bind(&numeric_params_done);
 
   // -------------------------------------------
   // Second loop to handle references.
@@ -3519,17 +3490,6 @@ void GenericJSToWasmWrapperHelper(MacroAssembler* masm, bool stack_switch) {
   __ Move(current_param,
           kFPOnStackSize + kPCOnStackSize + kReceiverOnStackSize);
 
-  if (stack_switch) {
-    // Materialize the suspender param
-    __ movq(param, MemOperand(original_fp, kSuspenderOffset));
-    __ movq(MemOperand(current_int_param_slot, 0), param);
-    __ subq(current_int_param_slot, Immediate(kSystemPointerSize));
-    __ addq(valuetypes_array_ptr, Immediate(kValueTypeSize));
-    __ addq(ref_param_count, Immediate(1));
-    __ cmpq(current_param, param_limit);
-    __ j(equal, &ref_params_done);
-  }
-
   Label ref_loop_through_params;
   Label ref_loop_end;
   // Start of the loop.
@@ -4182,10 +4142,6 @@ void Generate_WasmResumeHelper(MacroAssembler* masm, wasm::OnResume on_resume) {
   __ subq(rsp, Immediate(3 * kSystemPointerSize));
   __ movq(MemOperand(rbp, kParamCountOffset), param_count);
   __ movq(MemOperand(rbp, kInParamCountOffset), param_count);
-  // Set a sentinel value for the spill slot visited by the GC.
-  __ LoadRoot(kScratchRegister, RootIndex::kUndefinedValue);
-  __ movq(MemOperand(rbp, BuiltinWasmWrapperConstants::kSuspenderOffset),
-          kScratchRegister);
 
   param_count = no_reg;
 

src/common/message-template.h

@@ -652,6 +652,7 @@ namespace internal {
   T(WasmTrapStringOffsetOutOfBounds, "string offset out of bounds")            \
   T(WasmTrapStringIsolatedSurrogate,                                           \
     "Failed to encode string as UTF-8: contains unpaired surrogate")           \
+  T(WasmTrapReentrantSuspender, "re-entering an active/suspended suspender")   \
   T(WasmExceptionError, "wasm exception")                                      \
   /* Asm.js validation related */                                              \
   T(AsmJsInvalid, "Invalid asm.js: %")                                         \

src/execution/frame-constants.h

@@ -216,7 +216,6 @@ class BuiltinWasmWrapperConstants : public TypedFrameConstants {
   static constexpr int kInParamCountOffset = TYPED_FRAME_PUSHED_VALUE_OFFSET(1);
   // The number of parameters according to the signature.
   static constexpr int kParamCountOffset = TYPED_FRAME_PUSHED_VALUE_OFFSET(2);
-  static constexpr int kSuspenderOffset = TYPED_FRAME_PUSHED_VALUE_OFFSET(3);
 };
 
 class ConstructFrameConstants : public TypedFrameConstants {

src/execution/frames.cc

@@ -2332,10 +2332,6 @@ void StackSwitchFrame::Iterate(RootVisitor* v) const {
       &Memory<Address>(sp() + scan_count * kSystemPointerSize));
   v->VisitRootPointers(Root::kStackRoots, nullptr, spill_slot_base,
                        spill_slot_limit);
-  // Also visit fixed spill slots that contain references.
-  FullObjectSlot suspender_slot(
-      &Memory<Address>(fp() + BuiltinWasmWrapperConstants::kSuspenderOffset));
-  v->VisitRootPointer(Root::kStackRoots, nullptr, suspender_slot);
 }
 
 // static

src/runtime/runtime-wasm.cc

@@ -797,12 +797,21 @@ void SyncStackLimit(Isolate* isolate) {
 }
 }  // namespace
 
-// Allocate a new suspender, and prepare for stack switching by updating the
+// Allocate a new continuation, and prepare for stack switching by updating the
 // active continuation, active suspender and stack limit.
-RUNTIME_FUNCTION(Runtime_WasmAllocateSuspender) {
+RUNTIME_FUNCTION(Runtime_WasmAllocateContinuation) {
   CHECK(FLAG_experimental_wasm_stack_switching);
   HandleScope scope(isolate);
-  Handle<WasmSuspenderObject> suspender = WasmSuspenderObject::New(isolate);
+  if (!args[0].IsWasmSuspenderObject()) {
+    return ThrowWasmError(isolate, MessageTemplate::kWasmTrapJSTypeError);
+  }
+  Handle<WasmSuspenderObject> suspender =
+      handle(WasmSuspenderObject::cast(args[0]), isolate);
+
+  if (suspender->state() != WasmSuspenderObject::kInactive) {
+    return ThrowWasmError(isolate,
+                          MessageTemplate::kWasmTrapReentrantSuspender);
+  }
 
   // Update the continuation state.
   auto parent = handle(WasmContinuationObject::cast(
@@ -824,7 +833,7 @@ RUNTIME_FUNCTION(Runtime_WasmAllocateSuspender) {
   active_suspender_slot.store(*suspender);
 
   SyncStackLimit(isolate);
-  return *suspender;
+  return *target;
 }
 
 // Update the stack limit after a stack switch, and preserve pending interrupts.

src/runtime/runtime.h

@@ -609,7 +609,7 @@ namespace internal {
   F(WasmDebugBreak, 0, 1)             \
   F(WasmArrayCopy, 5, 1)              \
   F(WasmArrayNewSegment, 5, 1)        \
-  F(WasmAllocateSuspender, 0, 1)      \
+  F(WasmAllocateContinuation, 1, 1)   \
   F(WasmSyncStackLimit, 0, 1)         \
   F(WasmCreateResumePromise, 2, 1)    \
   F(WasmStringNewWtf8, 5, 1)          \

src/wasm/wasm-js.cc

@@ -2022,15 +2022,13 @@ void WebAssemblyFunctionType(const v8::FunctionCallbackInfo<v8::Value>& args) {
         handle(sfi->wasm_exported_function_data(), i_isolate);
     sig = wasm_exported_function->sig();
     if (data->suspend()) {
-      // If this export is suspendable, the first parameter of the original
-      // function is an externref (suspender) which does not appear in the
-      // wrapper function's signature. The wrapper function also returns a
+      // If this export is suspendable, the function returns a
       // promise as an externref instead of the original return type.
       size_t param_count = sig->parameter_count();
       DCHECK_GE(param_count, 1);
       DCHECK_EQ(sig->GetParam(0), i::wasm::kWasmAnyRef);
-      i::wasm::FunctionSig::Builder builder(&zone, 1, param_count - 1);
-      for (size_t i = 1; i < param_count; ++i) {
+      i::wasm::FunctionSig::Builder builder(&zone, 1, param_count);
+      for (size_t i = 0; i < param_count; ++i) {
         builder.AddParam(sig->GetParam(i));
       }
       builder.AddReturn(i::wasm::kWasmAnyRef);

src/wasm/wasm-objects.cc

@@ -1795,8 +1795,10 @@ Handle<WasmContinuationObject> WasmContinuationObject::New(
 Handle<WasmSuspenderObject> WasmSuspenderObject::New(Isolate* isolate) {
   Handle<JSFunction> suspender_cons(
       isolate->native_context()->wasm_suspender_constructor(), isolate);
+  // Suspender objects should be at least as long-lived as the instances of
+  // which it will wrap the imports/exports, allocate in old space too.
   auto suspender = Handle<WasmSuspenderObject>::cast(
-      isolate->factory()->NewJSObject(suspender_cons));
+      isolate->factory()->NewJSObject(suspender_cons, AllocationType::kOld));
   suspender->set_state(kInactive);
   // Instantiate the callable object which resumes this Suspender. This will be
   // used implicitly as the onFulfilled callback of the returned JS promise.

test/mjsunit/mjsunit.status

@@ -1713,7 +1713,6 @@
 ['arch != x64', {
   # Stack switching is only supported on x64.
   'wasm/stack-switching': [SKIP],
-  'wasm/stack-switching-export': [SKIP],
 }],  # arch != x64
 
 ##############################################################################