[compiler][wasm] Fix Wasm linkage

- Fixes a problem when constructing Wasm CallDescriptors, where the
  allocation tries to treat parameters and returns as if they are in the
  same frame. This doesn't work when slots may be aligned in their
  frame. Instead, allocate parameters and returns separately and offset
  return slots by the number of parameter slots.
- Adds argument slot padding in the CallDescriptor lowering case, to
  prepare for when 32 bit targets align stack frames and require
  padding.
- Adds a regression test.

Bug: chromium:1174500
Change-Id: I60d96a94b171a0d27ff61cbab35623976b0c6da8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2683024
Commit-Queue: Bill Budge <bbudge@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72588}

src/compiler/wasm-compiler.cc

@@ -8153,8 +8153,9 @@ class LinkageLocationAllocator {
  public:
   template <size_t kNumGpRegs, size_t kNumFpRegs>
   constexpr LinkageLocationAllocator(const Register (&gp)[kNumGpRegs],
-                                     const DoubleRegister (&fp)[kNumFpRegs])
-      : allocator_(wasm::LinkageAllocator(gp, fp)) {}
+                                     const DoubleRegister (&fp)[kNumFpRegs],
+                                     int slot_offset)
+      : allocator_(wasm::LinkageAllocator(gp, fp)), slot_offset_(slot_offset) {}
 
   LinkageLocation Next(MachineRepresentation rep) {
     MachineType type = MachineType::TypeForRepresentation(rep);
@@ -8168,16 +8169,19 @@ class LinkageLocationAllocator {
       return LinkageLocation::ForRegister(reg_code, type);
     }
     // Cannot use register; use stack slot.
-    int index = -1 - allocator_.NextStackSlot(rep);
+    int index = -1 - (slot_offset_ + allocator_.NextStackSlot(rep));
     return LinkageLocation::ForCallerFrameSlot(index, type);
   }
 
-  void SetStackOffset(int offset) { allocator_.SetStackOffset(offset); }
   int NumStackSlots() const { return allocator_.NumStackSlots(); }
   void EndSlotArea() { allocator_.EndSlotArea(); }
 
  private:
   wasm::LinkageAllocator allocator_;
+  // Since params and returns are in different stack frames, we must allocate
+  // them separately. Parameter slots don't need an offset, but return slots
+  // must be offset to just before the param slots, using this |slot_offset_|.
+  int slot_offset_;
 };
 }  // namespace
 
@@ -8195,8 +8199,8 @@ CallDescriptor* GetWasmCallDescriptor(
                                        fsig->parameter_count() + extra_params);
 
   // Add register and/or stack parameter(s).
-  LinkageLocationAllocator params(wasm::kGpParamRegisters,
-                                  wasm::kFpParamRegisters);
+  LinkageLocationAllocator params(
+      wasm::kGpParamRegisters, wasm::kFpParamRegisters, 0 /* no slot offset */);
 
   // The instance object.
   locations.AddParam(params.Next(MachineRepresentation::kTaggedPointer));
@@ -8232,22 +8236,21 @@ CallDescriptor* GetWasmCallDescriptor(
         kJSFunctionRegister.code(), MachineType::TaggedPointer()));
   }
 
-  // Add return location(s).
-  LinkageLocationAllocator rets(wasm::kGpReturnRegisters,
-                                wasm::kFpReturnRegisters);
-
   int parameter_slots = params.NumStackSlots();
   if (ShouldPadArguments(parameter_slots)) parameter_slots++;
 
-  rets.SetStackOffset(parameter_slots);
+  // Add return location(s).
+  LinkageLocationAllocator rets(wasm::kGpReturnRegisters,
+                                wasm::kFpReturnRegisters, parameter_slots);
 
   const int return_count = static_cast<int>(locations.return_count_);
   for (int i = 0; i < return_count; i++) {
     MachineRepresentation ret = fsig->GetReturn(i).machine_representation();
-    auto l = rets.Next(ret);
-    locations.AddReturn(l);
+    locations.AddReturn(rets.Next(ret));
   }
 
+  int return_slots = rets.NumStackSlots();
+
   const RegList kCalleeSaveRegisters = 0;
   const RegList kCalleeSaveFPRegisters = 0;
 
@@ -8274,7 +8277,7 @@ CallDescriptor* GetWasmCallDescriptor(
       target_type,                        // target MachineType
       target_loc,                         // target location
       locations.Build(),                  // location_sig
-      parameter_slots,                    // stack_parameter_count
+      parameter_slots,                    // parameter slot count
       compiler::Operator::kNoProperties,  // properties
       kCalleeSaveRegisters,               // callee-saved registers
       kCalleeSaveFPRegisters,             // callee-saved fp regs
@@ -8282,7 +8285,7 @@ CallDescriptor* GetWasmCallDescriptor(
       "wasm-call",                        // debug name
       StackArgumentOrder::kDefault,       // order of the arguments in the stack
       0,                                  // allocatable registers
-      rets.NumStackSlots() - parameter_slots);  // stack_return_count
+      return_slots);                      // return slot count
 }
 
 namespace {
@@ -8315,8 +8318,9 @@ CallDescriptor* ReplaceTypeInCallDescriptorWith(
       (call_descriptor->GetInputLocation(call_descriptor->InputCount() - 1) ==
        LinkageLocation::ForRegister(kJSFunctionRegister.code(),
                                     MachineType::TaggedPointer()));
-  LinkageLocationAllocator params(wasm::kGpParamRegisters,
-                                  wasm::kFpParamRegisters);
+  LinkageLocationAllocator params(
+      wasm::kGpParamRegisters, wasm::kFpParamRegisters, 0 /* no slot offset */);
+
   for (size_t i = 0, e = call_descriptor->ParameterCount() -
                          (has_callable_param ? 1 : 0);
        i < e; i++) {
@@ -8334,9 +8338,12 @@ CallDescriptor* ReplaceTypeInCallDescriptorWith(
         kJSFunctionRegister.code(), MachineType::TaggedPointer()));
   }
 
+  int parameter_slots = params.NumStackSlots();
+  if (ShouldPadArguments(parameter_slots)) parameter_slots++;
+
   LinkageLocationAllocator rets(wasm::kGpReturnRegisters,
-                                wasm::kFpReturnRegisters);
-  rets.SetStackOffset(params.NumStackSlots());
+                                wasm::kFpReturnRegisters, parameter_slots);
+
   for (size_t i = 0; i < call_descriptor->ReturnCount(); i++) {
     if (call_descriptor->GetReturnType(i) == input_type) {
       for (size_t j = 0; j < num_replacements; j++) {
@@ -8348,20 +8355,22 @@ CallDescriptor* ReplaceTypeInCallDescriptorWith(
     }
   }
 
-  return zone->New<CallDescriptor>(                    // --
-      call_descriptor->kind(),                         // kind
-      call_descriptor->GetInputType(0),                // target MachineType
-      call_descriptor->GetInputLocation(0),            // target location
-      locations.Build(),                               // location_sig
-      params.NumStackSlots(),                          // stack_parameter_count
-      call_descriptor->properties(),                   // properties
-      call_descriptor->CalleeSavedRegisters(),         // callee-saved registers
-      call_descriptor->CalleeSavedFPRegisters(),       // callee-saved fp regs
-      call_descriptor->flags(),                        // flags
-      call_descriptor->debug_name(),                   // debug name
-      call_descriptor->GetStackArgumentOrder(),        // stack order
-      call_descriptor->AllocatableRegisters(),         // allocatable registers
-      rets.NumStackSlots() - params.NumStackSlots());  // stack_return_count
+  int return_slots = rets.NumStackSlots();
+
+  return zone->New<CallDescriptor>(               // --
+      call_descriptor->kind(),                    // kind
+      call_descriptor->GetInputType(0),           // target MachineType
+      call_descriptor->GetInputLocation(0),       // target location
+      locations.Build(),                          // location_sig
+      parameter_slots,                            // parameter slot count
+      call_descriptor->properties(),              // properties
+      call_descriptor->CalleeSavedRegisters(),    // callee-saved registers
+      call_descriptor->CalleeSavedFPRegisters(),  // callee-saved fp regs
+      call_descriptor->flags(),                   // flags
+      call_descriptor->debug_name(),              // debug name
+      call_descriptor->GetStackArgumentOrder(),   // stack order
+      call_descriptor->AllocatableRegisters(),    // allocatable registers
+      return_slots);                              // return slot count
 }
 }  // namespace
 

test/unittests/wasm/wasm-compiler-unittest.cc

@@ -2,13 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "test/unittests/test-utils.h"
+#include "src/compiler/wasm-compiler.h"
 
 #include "src/codegen/machine-type.h"
 #include "src/codegen/signature.h"
 #include "src/compiler/linkage.h"
-#include "src/compiler/wasm-compiler.h"
 #include "src/wasm/value-type.h"
+#include "src/wasm/wasm-linkage.h"
+#include "test/unittests/test-utils.h"
 
 namespace v8 {
 namespace internal {
@@ -66,6 +67,45 @@ TEST_F(WasmCallDescriptorTest, TestExternRefIsGrouped) {
   }
 }
 
+TEST_F(WasmCallDescriptorTest, Regress_1174500) {
+  // Our test signature should have just enough params and returns to force
+  // 1 param and 1 return to be allocated as stack slots. Use FP registers to
+  // avoid interference with implicit parameters, like the Wasm Instance.
+  constexpr int kParamRegisters = arraysize(kFpParamRegisters);
+  constexpr int kParams = kParamRegisters + 1;
+  constexpr int kReturnRegisters = arraysize(kFpReturnRegisters);
+  constexpr int kReturns = kReturnRegisters + 1;
+  ValueType types[kReturns + kParams];
+  // One S128 return slot which shouldn't be padded unless the arguments area
+  // of the frame requires it.
+  for (int i = 0; i < kReturnRegisters; ++i) types[i] = kWasmF32;
+  types[kReturnRegisters] = kWasmS128;
+  // One F32 parameter slot to misalign the parameter area.
+  for (int i = 0; i < kParamRegisters; ++i) types[kReturns + i] = kWasmF32;
+  types[kReturns + kParamRegisters] = kWasmF32;
+
+  FunctionSig sig(kReturns, kParams, types);
+  compiler::CallDescriptor* desc =
+      compiler::GetWasmCallDescriptor(zone(), &sig);
+
+  // Get the location of our stack parameter slot. Skip the implicit Wasm
+  // instance parameter.
+  compiler::LinkageLocation last_param = desc->GetInputLocation(kParams + 1);
+  EXPECT_TRUE(last_param.IsCallerFrameSlot());
+  EXPECT_EQ(MachineType::Float32(), last_param.GetType());
+  EXPECT_EQ(-1, last_param.GetLocation());
+
+  // The stack return slot should be right above our last parameter, and any
+  // argument padding slots. The return slot itself should not be padded.
+  const int padding = kPadArguments ? 1 : 0;
+  const int first_return_slot = -1 - (padding + 1);
+  compiler::LinkageLocation return_location =
+      desc->GetReturnLocation(kReturns - 1);
+  EXPECT_TRUE(return_location.IsCallerFrameSlot());
+  EXPECT_EQ(MachineType::Simd128(), return_location.GetType());
+  EXPECT_EQ(first_return_slot, return_location.GetLocation());
+}
+
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8