[Liftoff] Fix stack pointer corruption

During a C call, a previous value of the stack pointer is stored in a
platform specific callee saved register. Loading the out argument of the
C call might overwrite the value in that register, if the destination
register collides with the platform specific register. Hence, do first
use that register to restore the previous stack pointer, and only then
load the out argument.
Similarly, when pushing arguments to the stack, do first push all
values and then set the platform specific register in order to avoid
overwriting an argument value held in that register.

Drive-by: Fix offset computations for parameters pushed to the stack
for c calls.

R=titzer@chromium.org

Bug: chromium:820802,chromium:820896,chromium:820807,v8:6600
Change-Id: If4567467b7912454f0bd2cad5927233c98894b03
Reviewed-on: https://chromium-review.googlesource.com/959064Reviewed-by: Ben Titzer <titzer@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51916}

src/wasm/baseline/arm/liftoff-assembler-arm.h

@@ -220,19 +220,19 @@ void LiftoffAssembler::PrepareCCall(wasm::FunctionSig* sig,
   BAILOUT("PrepareCCall");
 }
 
-void LiftoffAssembler::SetCCallRegParamAddr(Register dst, uint32_t param_offset,
+void LiftoffAssembler::SetCCallRegParamAddr(Register dst, int param_byte_offset,
                                             ValueType type) {
   BAILOUT("SetCCallRegParamAddr");
 }
 
-void LiftoffAssembler::SetCCallStackParamAddr(uint32_t stack_param_idx,
-                                              uint32_t param_offset,
+void LiftoffAssembler::SetCCallStackParamAddr(int stack_param_idx,
+                                              int param_byte_offset,
                                               ValueType type) {
   BAILOUT("SetCCallStackParamAddr");
 }
 
 void LiftoffAssembler::LoadCCallOutArgument(LiftoffRegister dst, ValueType type,
-                                            uint32_t num_lowered_args) {
+                                            int param_byte_offset) {
   BAILOUT("LoadCCallOutArgument");
 }
 

src/wasm/baseline/arm64/liftoff-assembler-arm64.h

@@ -220,19 +220,19 @@ void LiftoffAssembler::PrepareCCall(wasm::FunctionSig* sig,
   BAILOUT("PrepareCCall");
 }
 
-void LiftoffAssembler::SetCCallRegParamAddr(Register dst, uint32_t param_offset,
+void LiftoffAssembler::SetCCallRegParamAddr(Register dst, int param_byte_offset,
                                             ValueType type) {
   BAILOUT("SetCCallRegParamAddr");
 }
 
-void LiftoffAssembler::SetCCallStackParamAddr(uint32_t stack_param_idx,
-                                              uint32_t param_offset,
+void LiftoffAssembler::SetCCallStackParamAddr(int stack_param_idx,
+                                              int param_byte_offset,
                                               ValueType type) {
   BAILOUT("SetCCallStackParamAddr");
 }
 
 void LiftoffAssembler::LoadCCallOutArgument(LiftoffRegister dst, ValueType type,
-                                            uint32_t num_lowered_args) {
+                                            int param_byte_offset) {
   BAILOUT("LoadCCallOutArgument");
 }
 

src/wasm/baseline/ia32/liftoff-assembler-ia32.h

@@ -888,43 +888,47 @@ void LiftoffAssembler::DropStackSlotsAndRet(uint32_t num_stack_slots) {
 void LiftoffAssembler::PrepareCCall(wasm::FunctionSig* sig,
                                     const LiftoffRegister* args,
                                     ValueType out_argument_type) {
-  // Save current sp, such that we compute pointers to the values pushed above.
-  mov(liftoff::kCCallLastArgAddrReg, esp);
+  int pushed_bytes = 0;
   for (ValueType param_type : sig->parameters()) {
+    pushed_bytes += RoundUp<kPointerSize>(WasmOpcodes::MemSize(param_type));
     liftoff::push(this, *args++, param_type);
   }
   if (out_argument_type != kWasmStmt) {
-    int size = WasmOpcodes::MemSize(out_argument_type);
-    sub(esp, Immediate(std::max(kPointerSize, size)));
-  }
+    int size = RoundUp<kPointerSize>(WasmOpcodes::MemSize(out_argument_type));
+    sub(esp, Immediate(size));
+    pushed_bytes += size;
+  }
+  // Save the original sp (before the first push), such that we can later
+  // compute pointers to the pushed values. Do this only *after* pushing the
+  // values, because {kCCallLastArgAddrReg} might collide with an arg register.
+  lea(liftoff::kCCallLastArgAddrReg, Operand(esp, pushed_bytes));
   constexpr Register kScratch = ecx;
   static_assert(kScratch != liftoff::kCCallLastArgAddrReg, "collision");
-  PrepareCallCFunction(static_cast<uint32_t>(sig->parameter_count()), kScratch);
+  int num_c_call_arguments = static_cast<int>(sig->parameter_count()) +
+                             (out_argument_type != kWasmStmt);
+  PrepareCallCFunction(num_c_call_arguments, kScratch);
 }
 
-void LiftoffAssembler::SetCCallRegParamAddr(Register dst, uint32_t param_offset,
+void LiftoffAssembler::SetCCallRegParamAddr(Register dst, int param_byte_offset,
                                             ValueType type) {
   // Check that we don't accidentally override kCCallLastArgAddrReg.
   DCHECK_NE(liftoff::kCCallLastArgAddrReg, dst);
-  int offset =
-      kPointerSize * static_cast<int>(param_offset + 1 + needs_reg_pair(type));
-  lea(dst, Operand(liftoff::kCCallLastArgAddrReg, -offset));
+  lea(dst, Operand(liftoff::kCCallLastArgAddrReg, -param_byte_offset));
 }
 
-void LiftoffAssembler::SetCCallStackParamAddr(uint32_t stack_param_idx,
-                                              uint32_t param_offset,
+void LiftoffAssembler::SetCCallStackParamAddr(int stack_param_idx,
+                                              int param_byte_offset,
                                               ValueType type) {
   static constexpr Register kScratch = ecx;
-  SetCCallRegParamAddr(kScratch, param_offset, type);
-  mov(Operand(esp, param_offset * kPointerSize), kScratch);
+  SetCCallRegParamAddr(kScratch, param_byte_offset, type);
+  mov(Operand(esp, stack_param_idx * kPointerSize), kScratch);
 }
 
 void LiftoffAssembler::LoadCCallOutArgument(LiftoffRegister dst, ValueType type,
-                                            uint32_t num_lowered_args) {
+                                            int param_byte_offset) {
   // Check that we don't accidentally override kCCallLastArgAddrReg.
   DCHECK_NE(LiftoffRegister(liftoff::kCCallLastArgAddrReg), dst);
-  int offset = kPointerSize * num_lowered_args;
-  Operand src(liftoff::kCCallLastArgAddrReg, -offset);
+  Operand src(liftoff::kCCallLastArgAddrReg, -param_byte_offset);
   liftoff::Load(this, dst, src, type);
 }
 

src/wasm/baseline/liftoff-assembler.h

@@ -450,12 +450,12 @@ class LiftoffAssembler : public TurboAssembler {
   // the out argument.
   inline void PrepareCCall(wasm::FunctionSig* sig, const LiftoffRegister* args,
                            ValueType out_argument_type);
-  inline void SetCCallRegParamAddr(Register dst, uint32_t param_offset,
+  inline void SetCCallRegParamAddr(Register dst, int param_byte_offset,
                                    ValueType type);
-  inline void SetCCallStackParamAddr(uint32_t stack_param_idx,
-                                     uint32_t param_offset, ValueType type);
+  inline void SetCCallStackParamAddr(int stack_param_idx, int param_byte_offset,
+                                     ValueType type);
   inline void LoadCCallOutArgument(LiftoffRegister dst, ValueType type,
-                                   uint32_t num_lowered_args);
+                                   int param_byte_offset);
   inline void CallC(ExternalReference ext_ref, uint32_t num_params);
   inline void FinishCCall();
 

src/wasm/baseline/liftoff-compiler.cc

@@ -489,24 +489,25 @@ class LiftoffCompiler {
 
     // The arguments to the c function are pointers to the stack slots we just
     // pushed.
-    uint32_t num_stack_params = 0;
-    uint32_t input_idx = 1;  // Input 0 is the call target.
-    uint32_t num_lowered_args = 0;
+    int num_stack_params = 0;   // Number of stack parameters.
+    int input_idx = 1;          // Input 0 is the call target.
+    int param_byte_offset = 0;  // Byte offset into the pushed arguments.
     auto add_argument = [&](ValueType arg_type) {
       compiler::LinkageLocation loc =
           call_descriptor->GetInputLocation(input_idx);
+      param_byte_offset +=
+          RoundUp<kPointerSize>(WasmOpcodes::MemSize(arg_type));
+      ++input_idx;
       if (loc.IsRegister()) {
         Register reg = Register::from_code(loc.AsRegister());
         // Load address of that parameter to the register.
-        __ SetCCallRegParamAddr(reg, num_lowered_args, arg_type);
+        __ SetCCallRegParamAddr(reg, param_byte_offset, arg_type);
       } else {
         DCHECK(loc.IsCallerFrameSlot());
-        __ SetCCallStackParamAddr(num_stack_params, num_lowered_args, arg_type);
+        __ SetCCallStackParamAddr(num_stack_params, param_byte_offset,
+                                  arg_type);
         ++num_stack_params;
       }
-      num_lowered_args +=
-          RoundUp<kPointerSize>(WasmOpcodes::MemSize(arg_type)) / kPointerSize;
-      ++input_idx;
     };
     for (ValueType arg_type : sig->parameters()) {
       add_argument(arg_type);
@@ -517,7 +518,12 @@ class LiftoffCompiler {
     DCHECK_EQ(input_idx, call_descriptor->InputCount());
 
     // Now execute the call.
-    __ CallC(ext_ref, num_lowered_args);
+    uint32_t c_call_arg_count =
+        static_cast<uint32_t>(sig->parameter_count()) + has_out_argument;
+    __ CallC(ext_ref, c_call_arg_count);
+
+    // Reset the stack pointer.
+    __ FinishCCall();
 
     // Load return value.
     const LiftoffRegister* next_result_reg = result_regs;
@@ -537,11 +543,8 @@ class LiftoffCompiler {
     // Load potential return value from output argument.
     if (has_out_argument) {
       __ LoadCCallOutArgument(*next_result_reg, out_argument_type,
-                              num_lowered_args);
+                              param_byte_offset);
     }
-
-    // Reset the stack pointer.
-    __ FinishCCall();
   }
 
   template <ValueType type, class EmitFn>

src/wasm/baseline/mips/liftoff-assembler-mips.h

@@ -567,19 +567,19 @@ void LiftoffAssembler::PrepareCCall(wasm::FunctionSig* sig,
   BAILOUT("PrepareCCall");
 }
 
-void LiftoffAssembler::SetCCallRegParamAddr(Register dst, uint32_t param_offset,
+void LiftoffAssembler::SetCCallRegParamAddr(Register dst, int param_byte_offset,
                                             ValueType type) {
   BAILOUT("SetCCallRegParamAddr");
 }
 
-void LiftoffAssembler::SetCCallStackParamAddr(uint32_t stack_param_idx,
-                                              uint32_t param_offset,
+void LiftoffAssembler::SetCCallStackParamAddr(int stack_param_idx,
+                                              int param_byte_offset,
                                               ValueType type) {
   BAILOUT("SetCCallStackParamAddr");
 }
 
 void LiftoffAssembler::LoadCCallOutArgument(LiftoffRegister dst, ValueType type,
-                                            uint32_t num_lowered_args) {
+                                            int param_byte_offset) {
   BAILOUT("LoadCCallOutArgument");
 }
 

src/wasm/baseline/mips64/liftoff-assembler-mips64.h

@@ -512,19 +512,19 @@ void LiftoffAssembler::PrepareCCall(wasm::FunctionSig* sig,
   BAILOUT("PrepareCCall");
 }
 
-void LiftoffAssembler::SetCCallRegParamAddr(Register dst, uint32_t param_offset,
+void LiftoffAssembler::SetCCallRegParamAddr(Register dst, int param_byte_offset,
                                             ValueType type) {
   BAILOUT("SetCCallRegParamAddr");
 }
 
-void LiftoffAssembler::SetCCallStackParamAddr(uint32_t stack_param_idx,
-                                              uint32_t param_offset,
+void LiftoffAssembler::SetCCallStackParamAddr(int stack_param_idx,
+                                              int param_byte_offset,
                                               ValueType type) {
   BAILOUT("SetCCallStackParamAddr");
 }
 
 void LiftoffAssembler::LoadCCallOutArgument(LiftoffRegister dst, ValueType type,
-                                            uint32_t num_lowered_args) {
+                                            int param_byte_offset) {
   BAILOUT("LoadCCallOutArgument");
 }
 

src/wasm/baseline/ppc/liftoff-assembler-ppc.h

@@ -220,19 +220,19 @@ void LiftoffAssembler::PrepareCCall(wasm::FunctionSig* sig,
   BAILOUT("PrepareCCall");
 }
 
-void LiftoffAssembler::SetCCallRegParamAddr(Register dst, uint32_t param_offset,
+void LiftoffAssembler::SetCCallRegParamAddr(Register dst, int param_byte_offset,
                                             ValueType type) {
   BAILOUT("SetCCallRegParamAddr");
 }
 
-void LiftoffAssembler::SetCCallStackParamAddr(uint32_t stack_param_idx,
-                                              uint32_t param_offset,
+void LiftoffAssembler::SetCCallStackParamAddr(int stack_param_idx,
+                                              int param_byte_offset,
                                               ValueType type) {
   BAILOUT("SetCCallStackParamAddr");
 }
 
 void LiftoffAssembler::LoadCCallOutArgument(LiftoffRegister dst, ValueType type,
-                                            uint32_t num_lowered_args) {
+                                            int param_byte_offset) {
   BAILOUT("LoadCCallOutArgument");
 }
 

src/wasm/baseline/s390/liftoff-assembler-s390.h

@@ -220,19 +220,19 @@ void LiftoffAssembler::PrepareCCall(wasm::FunctionSig* sig,
   BAILOUT("PrepareCCall");
 }
 
-void LiftoffAssembler::SetCCallRegParamAddr(Register dst, uint32_t param_offset,
+void LiftoffAssembler::SetCCallRegParamAddr(Register dst, int param_byte_offset,
                                             ValueType type) {
   BAILOUT("SetCCallRegParamAddr");
 }
 
-void LiftoffAssembler::SetCCallStackParamAddr(uint32_t stack_param_idx,
-                                              uint32_t param_offset,
+void LiftoffAssembler::SetCCallStackParamAddr(int stack_param_idx,
+                                              int param_byte_offset,
                                               ValueType type) {
   BAILOUT("SetCCallStackParamAddr");
 }
 
 void LiftoffAssembler::LoadCCallOutArgument(LiftoffRegister dst, ValueType type,
-                                            uint32_t num_lowered_args) {
+                                            int param_byte_offset) {
   BAILOUT("LoadCCallOutArgument");
 }
 

src/wasm/baseline/x64/liftoff-assembler-x64.h

@@ -824,39 +824,41 @@ void LiftoffAssembler::DropStackSlotsAndRet(uint32_t num_stack_slots) {
 void LiftoffAssembler::PrepareCCall(wasm::FunctionSig* sig,
                                     const LiftoffRegister* args,
                                     ValueType out_argument_type) {
-  // Save current sp, such that we compute pointers to the values pushed above.
-  movq(liftoff::kCCallLastArgAddrReg, rsp);
   for (ValueType param_type : sig->parameters()) {
     liftoff::push(this, *args++, param_type);
   }
   if (out_argument_type != kWasmStmt) {
-    int size = WasmOpcodes::MemSize(out_argument_type);
-    subq(rsp, Immediate(std::max(kPointerSize, size)));
+    subq(rsp, Immediate(kPointerSize));
   }
-  PrepareCallCFunction(static_cast<uint32_t>(sig->parameter_count()));
+  // Save the original sp (before the first push), such that we can later
+  // compute pointers to the pushed values. Do this only *after* pushing the
+  // values, because {kCCallLastArgAddrReg} might collide with an arg register.
+  int num_c_call_arguments = static_cast<int>(sig->parameter_count()) +
+                             (out_argument_type != kWasmStmt);
+  int pushed_bytes = kPointerSize * num_c_call_arguments;
+  leaq(liftoff::kCCallLastArgAddrReg, Operand(rsp, pushed_bytes));
+  PrepareCallCFunction(num_c_call_arguments);
 }
 
-void LiftoffAssembler::SetCCallRegParamAddr(Register dst, uint32_t param_offset,
+void LiftoffAssembler::SetCCallRegParamAddr(Register dst, int param_byte_offset,
                                             ValueType type) {
   // Check that we don't accidentally override kCCallLastArgAddrReg.
   DCHECK_NE(liftoff::kCCallLastArgAddrReg, dst);
-  int offset = kPointerSize * static_cast<int>(param_offset + 1);
-  leaq(dst, Operand(liftoff::kCCallLastArgAddrReg, -offset));
+  leaq(dst, Operand(liftoff::kCCallLastArgAddrReg, -param_byte_offset));
 }
 
-void LiftoffAssembler::SetCCallStackParamAddr(uint32_t stack_param_idx,
-                                              uint32_t param_offset,
+void LiftoffAssembler::SetCCallStackParamAddr(int stack_param_idx,
+                                              int param_byte_offset,
                                               ValueType type) {
   // On x64, all C call arguments fit in registers.
   UNREACHABLE();
 }
 
 void LiftoffAssembler::LoadCCallOutArgument(LiftoffRegister dst, ValueType type,
-                                            uint32_t num_lowered_args) {
+                                            int param_byte_offset) {
   // Check that we don't accidentally override kCCallLastArgAddrReg.
   DCHECK_NE(LiftoffRegister(liftoff::kCCallLastArgAddrReg), dst);
-  int offset = kPointerSize * num_lowered_args;
-  Operand src(liftoff::kCCallLastArgAddrReg, -offset);
+  Operand src(liftoff::kCCallLastArgAddrReg, -param_byte_offset);
   liftoff::Load(this, dst, src, type);
 }
 

test/mjsunit/regress/wasm/regress-820802.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.addMemory(16, 32);
+builder.addGlobal(kWasmI32, 0);
+const sig0 = makeSig([kWasmI32, kWasmI32, kWasmI32], [kWasmI32]);
+builder.addFunction(undefined, sig0).addBody([
+  kExprI32Const, 1,  // i32.const 1
+  kExprI32Const, 0,  // i32.const 0
+  kExprI32Const, 3,  // i32.const 3
+  kExprI32GeU,       // i32.ge_u
+  kExprI32Rol,       // i32.rol
+]);
+builder.addExport('main', 0);
+const instance = builder.instantiate();
+assertEquals(1, instance.exports.main());