[Liftoff] Fix register spilling on stack transfer

When moving arguments for calls into the right registers and stack
slots, we were sometimes overwriting stack slots which would still be
used later to load arguments from. This is because we popped the (wasm)
value stack before executing the register moves, hence the stack
transfer would think the values are not being used any more and reuse
the stack slots.
With this CL, we only pop the arguments from the stack after executing
the stack transfer.

R=ahaas@chromium.org

Bug: v8:7366, v8:6600
Change-Id: I3aa5126c82634fd281959075e91e73465c39abaa
Reviewed-on: https://chromium-review.googlesource.com/883802
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50853}

src/wasm/baseline/liftoff-assembler.cc

@@ -420,17 +420,20 @@ void LiftoffAssembler::PrepareCall(wasm::FunctionSig* sig,
   StackTransferRecipe stack_transfers(this);
 
   // Now move all parameter values into the right slot for the call.
-  // Process parameters backward, such that we can just pop values from the
-  // stack.
+  // Don't pop values yet, such that the stack height is still correct when
+  // executing the {stack_transfers}.
+  // Process parameters backwards, such that pushes of caller frame slots are
+  // in the correct order.
   LiftoffRegList param_regs;
+  uint32_t param_base = cache_state_.stack_height() - num_params;
   for (uint32_t i = num_params; i > 0; --i) {
     uint32_t param = i - 1;
     ValueType type = sig->GetParam(param);
     RegClass rc = reg_class_for(type);
     compiler::LinkageLocation loc = call_desc->GetInputLocation(
         param + kFirstActualParameter + kInputShift);
-    const VarState& slot = cache_state_.stack_state.back();
-    uint32_t stack_idx = cache_state_.stack_height() - 1;
+    uint32_t stack_idx = param_base + param;
+    const VarState& slot = cache_state_.stack_state[stack_idx];
     if (loc.IsRegister()) {
       DCHECK(!loc.IsAnyRegister());
       int reg_code = loc.AsRegister();
@@ -441,7 +444,6 @@ void LiftoffAssembler::PrepareCall(wasm::FunctionSig* sig,
       DCHECK(loc.IsCallerFrameSlot());
       PushCallerFrameSlot(slot, stack_idx);
     }
-    cache_state_.stack_state.pop_back();
   }
 
   compiler::LinkageLocation context_loc =
@@ -473,6 +475,10 @@ void LiftoffAssembler::PrepareCall(wasm::FunctionSig* sig,
   // stack grew too large.
   *max_used_spill_slot = stack_transfers.max_used_spill_slot();
 
+  // Pop parameters from the value stack.
+  auto stack_end = cache_state_.stack_state.end();
+  cache_state_.stack_state.erase(stack_end - num_params, stack_end);
+
   // Reset register use counters.
   cache_state_.reset_used_registers();
 

test/mjsunit/regress/wasm/regress-7366.js

+// Copyright 2018 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.
+
+load('test/mjsunit/wasm/wasm-constants.js');
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+const builder = new WasmModuleBuilder();
+builder.addFunction(undefined, kSig_i_iii).addBody([
+  // Return the sum of all arguments.
+  kExprGetLocal, 0, kExprGetLocal, 1, kExprGetLocal, 2, kExprI32Add, kExprI32Add
+]);
+const sig = builder.addType(kSig_i_iii);
+builder.addFunction(undefined, kSig_i_iii)
+    .addBody([
+      ...wasmI32Const(1),         // i32.const 0x1
+      kExprSetLocal, 0,           // set_local 0
+      ...wasmI32Const(4),         // i32.const 0x1
+      kExprSetLocal, 1,           // set_local 1
+      ...wasmI32Const(16),        // i32.const 0x1
+      kExprSetLocal, 2,           // set_local 2
+      kExprLoop, kWasmStmt,       // loop
+      kExprEnd,                   // end
+      kExprGetLocal, 0,           // get_local 0
+      kExprGetLocal, 1,           // get_local 1
+      kExprGetLocal, 2,           // get_local 2
+      kExprI32Const, 0,           // i32.const 0 (func index)
+      kExprCallIndirect, sig, 0,  // call indirect
+    ])
+    .exportAs('main');
+builder.appendToTable([0]);
+const instance = builder.instantiate();
+assertEquals(21, instance.exports.main());