[wasm] Fix registers spilled in DebugBreak frame

The set of registers to spill was wrong. Instead of spilling wasm
parameter registers (like the WasmCompileLazy builtin), we should spill
all registers that are being used as Liftoff cache registers.
This CL defines platform-specific WasmDebugBreakFrameConstants which
hold the set of registers to spill. This set is used in the builtin, and
will later be used for inspecting the spilled registers.

In order to iterate bit sets more easily in both direction (MSB to LSB
or LSB to MSB), we add a base::bits::IterateBits{,Backwards} method
which provides the respective iterators.

R=jkummerow@chromium.org
CC=thibaudm@chromium.org

Bug: v8:10222
Change-Id: I73ecbdff9b29e244c478b404063c0c9ee25bc821
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2102570
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66715}

BUILD.gn

@@ -3635,6 +3635,7 @@ v8_component("v8_libbase") {
     "src/base/atomicops_internals_std.h",
     "src/base/base-export.h",
     "src/base/bit-field.h",
+    "src/base/bits-iterator.h",
     "src/base/bits.cc",
     "src/base/bits.h",
     "src/base/bounded-page-allocator.cc",

src/base/bits-iterator.h

@@ -0,0 +1,58 @@
+// Copyright 2020 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.
+
+#ifndef V8_BASE_BITS_ITERATOR_H_
+#define V8_BASE_BITS_ITERATOR_H_
+
+#include <type_traits>
+
+#include "src/base/bits.h"
+#include "src/base/iterator.h"
+
+namespace v8 {
+namespace base {
+namespace bits {
+
+template <typename T, bool kMSBFirst = false>
+class BitsIterator : public iterator<std::forward_iterator_tag, int> {
+  STATIC_ASSERT(std::is_integral<T>::value);
+
+ public:
+  explicit BitsIterator(T bits) : bits_(bits) {}
+
+  int operator*() const {
+    return kMSBFirst ? 8 * sizeof(T) - 1 - CountLeadingZeros(bits_)
+                     : CountTrailingZeros(bits_);
+  }
+
+  BitsIterator& operator++() {
+    bits_ &= ~(T{1} << **this);
+    return *this;
+  }
+
+  bool operator==(BitsIterator other) { return bits_ == other.bits_; }
+  bool operator!=(BitsIterator other) { return bits_ != other.bits_; }
+
+ private:
+  T bits_;
+};
+
+// Returns an iterable over the bits in {bits}, from LSB to MSB.
+template <typename T>
+auto IterateBits(T bits) {
+  return make_iterator_range(BitsIterator<T>{bits}, BitsIterator<T>{0});
+}
+
+// Returns an iterable over the bits in {bits}, from MSB to LSB.
+template <typename T>
+auto IterateBitsBackwards(T bits) {
+  return make_iterator_range(BitsIterator<T, true>{bits},
+                             BitsIterator<T, true>{0});
+}
+
+}  // namespace bits
+}  // namespace base
+}  // namespace v8
+
+#endif  // V8_BASE_BITS_ITERATOR_H_

src/builtins/arm/builtins-arm.cc

@@ -2505,15 +2505,14 @@ void Builtins::Generate_WasmDebugBreak(MacroAssembler* masm) {
 
     // Save all parameter registers. They might hold live values, we restore
     // them after the runtime call.
-    RegList gp_regs = 0;
-    for (Register reg : wasm::kGpParamRegisters) gp_regs |= reg.bit();
-    constexpr DwVfpRegister lowest_fp_reg = wasm::kFpParamRegisters[0];
-    constexpr DwVfpRegister highest_fp_reg =
-        wasm::kFpParamRegisters[arraysize(wasm::kFpParamRegisters) - 1];
-    STATIC_ASSERT(arraysize(wasm::kFpParamRegisters) ==
-                  highest_fp_reg.code() - lowest_fp_reg.code() + 1);
+    constexpr DwVfpRegister lowest_fp_reg = DwVfpRegister::from_code(
+        WasmDebugBreakFrameConstants::kFirstPushedFpReg);
+    constexpr DwVfpRegister highest_fp_reg = DwVfpRegister::from_code(
+        WasmDebugBreakFrameConstants::kLastPushedFpReg);
 
-    __ stm(db_w, sp, gp_regs);
+    // Store gp parameter registers.
+    __ stm(db_w, sp, WasmDebugBreakFrameConstants::kPushedGpRegs);
+    // Store fp parameter registers.
     __ vstm(db_w, sp, lowest_fp_reg, highest_fp_reg);
 
     // Initialize the JavaScript context with 0. CEntry will use it to
@@ -2523,7 +2522,7 @@ void Builtins::Generate_WasmDebugBreak(MacroAssembler* masm) {
 
     // Restore registers.
     __ vldm(ia_w, sp, lowest_fp_reg, highest_fp_reg);
-    __ ldm(ia_w, sp, gp_regs);
+    __ ldm(ia_w, sp, WasmDebugBreakFrameConstants::kPushedGpRegs);
   }
   __ Ret();
 }

src/builtins/arm64/builtins-arm64.cc

@@ -3021,19 +3021,8 @@ void Builtins::Generate_WasmDebugBreak(MacroAssembler* masm) {
 
     // Save all parameter registers. They might hold live values, we restore
     // them after the runtime call.
-    RegList gp_regs = 0;
-    for (Register reg : wasm::kGpParamRegisters) gp_regs |= reg.bit();
-    // The size of {gp_regs} must be a multiple of 2, so that the total pushed
-    // size is a multiple of 16 bytes.
-    if (NumRegs(gp_regs) % 2) {
-      // Just add any unset register.
-      gp_regs |= uint64_t{1} << base::bits::CountTrailingZeros(~gp_regs);
-    }
-    RegList fp_regs = 0;
-    for (DoubleRegister reg : wasm::kFpParamRegisters) fp_regs |= reg.bit();
-
-    __ PushXRegList(gp_regs);
-    __ PushDRegList(fp_regs);
+    __ PushXRegList(WasmDebugBreakFrameConstants::kPushedGpRegs);
+    __ PushDRegList(WasmDebugBreakFrameConstants::kPushedFpRegs);
 
     // Initialize the JavaScript context with 0. CEntry will use it to
     // set the current context on the isolate.
@@ -3041,8 +3030,8 @@ void Builtins::Generate_WasmDebugBreak(MacroAssembler* masm) {
     __ CallRuntime(Runtime::kWasmDebugBreak, 0);
 
     // Restore registers.
-    __ PopDRegList(fp_regs);
-    __ PopXRegList(gp_regs);
+    __ PopDRegList(WasmDebugBreakFrameConstants::kPushedFpRegs);
+    __ PopXRegList(WasmDebugBreakFrameConstants::kPushedGpRegs);
   }
   __ Ret();
 }

src/builtins/ia32/builtins-ia32.cc

@@ -5,6 +5,7 @@
 #if V8_TARGET_ARCH_IA32
 
 #include "src/api/api-arguments.h"
+#include "src/base/bits-iterator.h"
 #include "src/base/iterator.h"
 #include "src/codegen/code-factory.h"
 // For interpreter_entry_return_pc_offset. TODO(jkummerow): Drop.
@@ -2676,18 +2677,19 @@ void Builtins::Generate_WasmDebugBreak(MacroAssembler* masm) {
 
     // Save all parameter registers. They might hold live values, we restore
     // them after the runtime call.
-    // TODO(clemensb): Make these registers available for inspection.
-    for (Register reg : wasm::kGpParamRegisters) {
-      __ Push(reg);
+    for (int reg_code : base::bits::IterateBitsBackwards(
+             WasmDebugBreakFrameConstants::kPushedGpRegs)) {
+      __ Push(Register::from_code(reg_code));
     }
 
     constexpr int kFpStackSize =
-        kSimd128Size * arraysize(wasm::kFpParamRegisters);
+        kSimd128Size * WasmDebugBreakFrameConstants::kNumPushedFpRegisters;
     __ AllocateStackSpace(kFpStackSize);
-    int offset = 0;
-    for (DoubleRegister reg : wasm::kFpParamRegisters) {
-      __ movdqu(Operand(esp, offset), reg);
-      offset += kSimd128Size;
+    int offset = kFpStackSize;
+    for (int reg_code : base::bits::IterateBitsBackwards(
+             WasmDebugBreakFrameConstants::kPushedFpRegs)) {
+      offset -= kSimd128Size;
+      __ movdqu(Operand(esp, offset), DoubleRegister::from_code(reg_code));
     }
 
     // Initialize the JavaScript context with 0. CEntry will use it to
@@ -2696,13 +2698,15 @@ void Builtins::Generate_WasmDebugBreak(MacroAssembler* masm) {
     __ CallRuntime(Runtime::kWasmDebugBreak, 0);
 
     // Restore registers.
-    for (DoubleRegister reg : base::Reversed(wasm::kFpParamRegisters)) {
-      offset -= kSimd128Size;
-      __ movdqu(reg, Operand(esp, offset));
+    for (int reg_code :
+         base::bits::IterateBits(WasmDebugBreakFrameConstants::kPushedFpRegs)) {
+      __ movdqu(DoubleRegister::from_code(reg_code), Operand(esp, offset));
+      offset += kSimd128Size;
     }
     __ add(esp, Immediate(kFpStackSize));
-    for (Register reg : base::Reversed(wasm::kGpParamRegisters)) {
-      __ Pop(reg);
+    for (int reg_code :
+         base::bits::IterateBits(WasmDebugBreakFrameConstants::kPushedGpRegs)) {
+      __ Pop(Register::from_code(reg_code));
     }
   }
 

src/builtins/x64/builtins-x64.cc

@@ -5,6 +5,7 @@
 #if V8_TARGET_ARCH_X64
 
 #include "src/api/api-arguments.h"
+#include "src/base/bits-iterator.h"
 #include "src/base/iterator.h"
 #include "src/codegen/code-factory.h"
 // For interpreter_entry_return_pc_offset. TODO(jkummerow): Drop.
@@ -2857,17 +2858,19 @@ void Builtins::Generate_WasmDebugBreak(MacroAssembler* masm) {
 
     // Save all parameter registers. They might hold live values, we restore
     // them after the runtime call.
-    for (Register reg : wasm::kGpParamRegisters) {
-      __ Push(reg);
+    for (int reg_code : base::bits::IterateBitsBackwards(
+             WasmDebugBreakFrameConstants::kPushedGpRegs)) {
+      __ Push(Register::from_code(reg_code));
     }
 
     constexpr int kFpStackSize =
-        kSimd128Size * arraysize(wasm::kFpParamRegisters);
+        kSimd128Size * WasmDebugBreakFrameConstants::kNumPushedFpRegisters;
     __ AllocateStackSpace(kFpStackSize);
-    int offset = 0;
-    for (DoubleRegister reg : wasm::kFpParamRegisters) {
-      __ movdqu(Operand(rsp, offset), reg);
-      offset += kSimd128Size;
+    int offset = kFpStackSize;
+    for (int reg_code : base::bits::IterateBitsBackwards(
+             WasmDebugBreakFrameConstants::kPushedFpRegs)) {
+      offset -= kSimd128Size;
+      __ movdqu(Operand(rsp, offset), DoubleRegister::from_code(reg_code));
     }
 
     // Initialize the JavaScript context with 0. CEntry will use it to
@@ -2876,13 +2879,15 @@ void Builtins::Generate_WasmDebugBreak(MacroAssembler* masm) {
     __ CallRuntime(Runtime::kWasmDebugBreak, 0);
 
     // Restore registers.
-    for (DoubleRegister reg : base::Reversed(wasm::kFpParamRegisters)) {
-      offset -= kSimd128Size;
-      __ movdqu(reg, Operand(rsp, offset));
+    for (int reg_code :
+         base::bits::IterateBits(WasmDebugBreakFrameConstants::kPushedFpRegs)) {
+      __ movdqu(DoubleRegister::from_code(reg_code), Operand(rsp, offset));
+      offset += kSimd128Size;
     }
     __ addq(rsp, Immediate(kFpStackSize));
-    for (Register reg : base::Reversed(wasm::kGpParamRegisters)) {
-      __ Pop(reg);
+    for (int reg_code :
+         base::bits::IterateBits(WasmDebugBreakFrameConstants::kPushedGpRegs)) {
+      __ Pop(Register::from_code(reg_code));
     }
   }
 

src/execution/arm/frame-constants-arm.h

@@ -5,6 +5,7 @@
 #ifndef V8_EXECUTION_ARM_FRAME_CONSTANTS_ARM_H_
 #define V8_EXECUTION_ARM_FRAME_CONSTANTS_ARM_H_
 
+#include "src/base/bits.h"
 #include "src/base/macros.h"
 #include "src/execution/frame-constants.h"
 
@@ -36,6 +37,23 @@ class WasmCompileLazyFrameConstants : public TypedFrameConstants {
       kNumberOfSavedFpParamRegs * kDoubleSize;
 };
 
+// Frame constructed by the {WasmDebugBreak} builtin.
+// After pushing the frame type marker, the builtin pushes all Liftoff cache
+// registers (see liftoff-assembler-defs.h).
+class WasmDebugBreakFrameConstants : public TypedFrameConstants {
+ public:
+  // {r0, r1, r2, r3, r4, r5, r6, r8, r9}
+  static constexpr uint32_t kPushedGpRegs = 0b1101111111;
+  // {d0 .. d12}
+  static constexpr int kFirstPushedFpReg = 0;
+  static constexpr int kLastPushedFpReg = 12;
+
+  static constexpr int kNumPushedGpRegisters =
+      base::bits::CountPopulation(kPushedGpRegs);
+  static constexpr int kNumPushedFpRegisters =
+      kLastPushedFpReg - kFirstPushedFpReg + 1;
+};
+
 }  // namespace internal
 }  // namespace v8
 

src/execution/arm64/frame-constants-arm64.h

@@ -5,6 +5,7 @@
 #ifndef V8_EXECUTION_ARM64_FRAME_CONSTANTS_ARM64_H_
 #define V8_EXECUTION_ARM64_FRAME_CONSTANTS_ARM64_H_
 
+#include "src/base/bits.h"
 #include "src/base/macros.h"
 #include "src/common/globals.h"
 #include "src/execution/frame-constants.h"
@@ -87,6 +88,24 @@ class WasmCompileLazyFrameConstants : public TypedFrameConstants {
       kNumberOfSavedFpParamRegs * kDoubleSize;
 };
 
+// Frame constructed by the {WasmDebugBreak} builtin.
+// After pushing the frame type marker, the builtin pushes all Liftoff cache
+// registers (see liftoff-assembler-defs.h).
+class WasmDebugBreakFrameConstants : public TypedFrameConstants {
+ public:
+  // {x0 .. x28} \ {x16, x17, x18, x26, x27}
+  static constexpr uint32_t kPushedGpRegs =
+      (1 << 29) - 1 - (1 << 16) - (1 << 17) - (1 << 18) - (1 << 26) - (1 << 27);
+  // {d0 .. d29}; {d15} is not used, but we still keep it for alignment reasons
+  // (the frame size needs to be a multiple of 16).
+  static constexpr uint32_t kPushedFpRegs = (1 << 30) - 1;
+
+  static constexpr int kNumPushedGpRegisters =
+      base::bits::CountPopulation(kPushedGpRegs);
+  static constexpr int kNumPushedFpRegisters =
+      base::bits::CountPopulation(kPushedFpRegs);
+};
+
 }  // namespace internal
 }  // namespace v8
 

src/execution/frame-constants.h

@@ -163,7 +163,7 @@ class OptimizedBuiltinFrameConstants : public StandardFrameConstants {
   static constexpr int kFixedSlotCount = kFixedFrameSize / kSystemPointerSize;
 };
 
-// TypedFrames have a SMI type maker value below the saved FP/constant pool to
+// TypedFrames have a type maker value below the saved FP/constant pool to
 // distinguish them from StandardFrames, which have a context in that position
 // instead.
 //
@@ -205,8 +205,7 @@ class TypedFrameConstants : public CommonFrameConstants {
       StandardFrameConstants::kFixedFrameSizeAboveFp + kFixedFrameSizeFromFp;
   static constexpr int kFixedSlotCount = kFixedFrameSize / kSystemPointerSize;
   static constexpr int kFirstPushedFrameValueOffset =
-      -StandardFrameConstants::kCPSlotSize - kFrameTypeSize -
-      kSystemPointerSize;
+      -kFixedFrameSizeFromFp - kSystemPointerSize;
 };
 
 #define TYPED_FRAME_PUSHED_VALUE_OFFSET(x) \

src/execution/frames.cc

@@ -2050,7 +2050,7 @@ void WasmDebugBreakFrame::Print(StringStream* accumulator, PrintMode mode,
 Address WasmDebugBreakFrame::GetCallerStackPointer() const {
   // WasmDebugBreak does not receive any arguments, hence the stack pointer of
   // the caller is at a fixed offset from the frame pointer.
-  return fp() + StandardFrameConstants::kCallerSPOffset;
+  return fp() + WasmDebugBreakFrameConstants::kCallerSPOffset;
 }
 
 Code WasmCompileLazyFrame::unchecked_code() const { return Code(); }

src/execution/ia32/frame-constants-ia32.h

@@ -5,6 +5,7 @@
 #ifndef V8_EXECUTION_IA32_FRAME_CONSTANTS_IA32_H_
 #define V8_EXECUTION_IA32_FRAME_CONSTANTS_IA32_H_
 
+#include "src/base/bits.h"
 #include "src/base/macros.h"
 #include "src/execution/frame-constants.h"
 
@@ -45,6 +46,22 @@ class WasmCompileLazyFrameConstants : public TypedFrameConstants {
       kNumberOfSavedFpParamRegs * kSimd128Size;
 };
 
+// Frame constructed by the {WasmDebugBreak} builtin.
+// After pushing the frame type marker, the builtin pushes all Liftoff cache
+// registers (see liftoff-assembler-defs.h).
+class WasmDebugBreakFrameConstants : public TypedFrameConstants {
+ public:
+  // {eax, ecx, edx, esi, edi}
+  static constexpr uint32_t kPushedGpRegs = 0b11000111;
+  // {xmm0, xmm1, xmm2, xmm3, xmm4, xmm5, xmm6}
+  static constexpr uint32_t kPushedFpRegs = 0b01111111;
+
+  static constexpr int kNumPushedGpRegisters =
+      base::bits::CountPopulation(kPushedGpRegs);
+  static constexpr int kNumPushedFpRegisters =
+      base::bits::CountPopulation(kPushedFpRegs);
+};
+
 }  // namespace internal
 }  // namespace v8
 

src/execution/x64/frame-constants-x64.h

@@ -5,6 +5,7 @@
 #ifndef V8_EXECUTION_X64_FRAME_CONSTANTS_X64_H_
 #define V8_EXECUTION_X64_FRAME_CONSTANTS_X64_H_
 
+#include "src/base/bits.h"
 #include "src/base/macros.h"
 #include "src/execution/frame-constants.h"
 
@@ -54,6 +55,22 @@ class WasmCompileLazyFrameConstants : public TypedFrameConstants {
       kNumberOfSavedFpParamRegs * kSimd128Size;
 };
 
+// Frame constructed by the {WasmDebugBreak} builtin.
+// After pushing the frame type marker, the builtin pushes all Liftoff cache
+// registers (see liftoff-assembler-defs.h).
+class WasmDebugBreakFrameConstants : public TypedFrameConstants {
+ public:
+  // {rax, rcx, rdx, rbx, rsi, rdi, r9}
+  static constexpr uint32_t kPushedGpRegs = 0b1011001111;
+  // {xmm0, xmm1, xmm2, xmm3, xmm4, xmm5, xmm6, xmm7}
+  static constexpr uint32_t kPushedFpRegs = 0b11111111;
+
+  static constexpr int kNumPushedGpRegisters =
+      base::bits::CountPopulation(kPushedGpRegs);
+  static constexpr int kNumPushedFpRegisters =
+      base::bits::CountPopulation(kPushedFpRegs);
+};
+
 }  // namespace internal
 }  // namespace v8