[wasm] Extend debug side table for registers

This extends the debug side table to also store register locations in
addition to constants and stack values.
Previously, every value that was not constant was assumed to be spilled
to the stack. This made sense, because without breakpoints we would only
emit debug side table entries at call sites, where all registers are
spilled.
With breakpoints, this changes. At break locations, values might be live
in registers.

The logic to decide whether a value will live in the register or on the
stack is extended, because we sometimes generate the debug side table
entry at a point where the registers are not spilled yet. The debug side
table entry creation needs to account for that, and assume that these
registers will still be spilled.

R=thibaudm@chromium.org

Bug: v8:10147, v8:10222
Change-Id: I3b020dfaa29fc007047663706ee286180a996bfd
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2066960
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66407}

src/wasm/baseline/liftoff-compiler.cc

@@ -155,6 +155,17 @@ constexpr Condition GetCompareCondition(WasmOpcode opcode) {
 // Builds a {DebugSideTable}.
 class DebugSideTableBuilder {
  public:
+  enum AssumeSpilling {
+    // All register values will be spilled before the pc covered by the debug
+    // side table entry. Register slots will be marked as stack slots in the
+    // generated debug side table entry.
+    kAssumeSpilling,
+    // Register slots will be written out as they are.
+    kAllowRegisters,
+    // Register slots cannot appear since we already spilled.
+    kDidSpill
+  };
+
   class EntryBuilder {
    public:
     explicit EntryBuilder(int pc_offset,
@@ -176,7 +187,8 @@ class DebugSideTableBuilder {
   // Adds a new entry, and returns a pointer to a builder for modifying that
   // entry ({stack_height} includes {num_locals}).
   EntryBuilder* NewEntry(int pc_offset, int num_locals, int stack_height,
-                         LiftoffAssembler::VarState* stack_state) {
+                         LiftoffAssembler::VarState* stack_state,
+                         AssumeSpilling assume_spilling) {
     DCHECK_LE(num_locals, stack_height);
     // Record stack types.
     std::vector<DebugSideTable::Entry::Value> values(stack_height);
@@ -184,12 +196,25 @@ class DebugSideTableBuilder {
       const auto& slot = stack_state[i];
       values[i].type = slot.type();
       values[i].stack_offset = slot.offset();
-      if (slot.is_const()) {
-        values[i].kind = DebugSideTable::Entry::kConstant;
-        values[i].i32_const = slot.i32_const();
-      } else {
-        values[i].kind = DebugSideTable::Entry::kStack;
-        values[i].stack_offset = slot.offset();
+      switch (slot.loc()) {
+        case kIntConst:
+          values[i].kind = DebugSideTable::Entry::kConstant;
+          values[i].i32_const = slot.i32_const();
+          break;
+        case kRegister:
+          DCHECK_NE(kDidSpill, assume_spilling);
+          if (assume_spilling == kAllowRegisters) {
+            values[i].kind = DebugSideTable::Entry::kRegister;
+            values[i].reg_code =
+                slot.is_fp_reg() ? slot.fp_reg().code() : slot.gp_reg().code();
+            break;
+          }
+          DCHECK_EQ(kAssumeSpilling, assume_spilling);
+          V8_FALLTHROUGH;
+        case kStack:
+          values[i].kind = DebugSideTable::Entry::kStack;
+          values[i].stack_offset = slot.offset();
+          break;
       }
     }
     entries_.emplace_back(pc_offset, std::move(values));
@@ -463,9 +488,9 @@ class LiftoffCompiler {
 
   void StackCheck(WasmCodePosition position) {
     if (!FLAG_wasm_stack_checks || !env_->runtime_exception_support) return;
-    out_of_line_code_.push_back(
-        OutOfLineCode::StackCheck(position, __ cache_state()->used_registers,
-                                  RegisterDebugSideTableEntry()));
+    out_of_line_code_.push_back(OutOfLineCode::StackCheck(
+        position, __ cache_state()->used_registers,
+        RegisterDebugSideTableEntry(DebugSideTableBuilder::kAssumeSpilling)));
     OutOfLineCode& ool = out_of_line_code_.back();
     Register limit_address = __ GetUnusedRegister(kGpReg).gp();
     LOAD_INSTANCE_FIELD(limit_address, StackLimitAddress, kSystemPointerSize);
@@ -651,7 +676,7 @@ class LiftoffCompiler {
     // installing a new Liftoff code object.
     source_position_table_builder_.AddPosition(
         __ pc_offset(), SourcePosition(decoder->position()), false);
-    RegisterDebugSideTableEntry();
+    RegisterDebugSideTableEntry(DebugSideTableBuilder::kAllowRegisters);
     safepoint_table_builder_.DefineSafepoint(&asm_, Safepoint::kNoLazyDeopt);
   }
 
@@ -1706,8 +1731,9 @@ class LiftoffCompiler {
     DCHECK_EQ(pc != 0, stub == WasmCode::kThrowWasmTrapMemOutOfBounds &&
                            env_->use_trap_handler);
 
-    out_of_line_code_.push_back(
-        OutOfLineCode::Trap(stub, position, pc, RegisterDebugSideTableEntry()));
+    out_of_line_code_.push_back(OutOfLineCode::Trap(
+        stub, position, pc,
+        RegisterDebugSideTableEntry(DebugSideTableBuilder::kAssumeSpilling)));
     return out_of_line_code_.back().label.get();
   }
 
@@ -1970,7 +1996,7 @@ class LiftoffCompiler {
     if (input.gp() != param_reg) __ Move(param_reg, input.gp(), kWasmI32);
 
     __ CallRuntimeStub(WasmCode::kWasmMemoryGrow);
-    RegisterDebugSideTableEntry();
+    RegisterDebugSideTableEntry(DebugSideTableBuilder::kDidSpill);
     safepoint_table_builder_.DefineSafepoint(&asm_, Safepoint::kNoLazyDeopt);
 
     if (kReturnRegister0 != result.gp()) {
@@ -1980,12 +2006,13 @@ class LiftoffCompiler {
     __ PushRegister(kWasmI32, result);
   }
 
-  DebugSideTableBuilder::EntryBuilder* RegisterDebugSideTableEntry() {
+  DebugSideTableBuilder::EntryBuilder* RegisterDebugSideTableEntry(
+      DebugSideTableBuilder::AssumeSpilling assume_spilling) {
     if (V8_LIKELY(!debug_sidetable_builder_)) return nullptr;
     int stack_height = static_cast<int>(__ cache_state()->stack_height());
     return debug_sidetable_builder_->NewEntry(
         __ pc_offset(), __ num_locals(), stack_height,
-        __ cache_state()->stack_state.begin());
+        __ cache_state()->stack_state.begin(), assume_spilling);
   }
 
   void CallDirect(FullDecoder* decoder,
@@ -2050,7 +2077,7 @@ class LiftoffCompiler {
           __ pc_offset(), SourcePosition(decoder->position()), false);
     }
 
-    RegisterDebugSideTableEntry();
+    RegisterDebugSideTableEntry(DebugSideTableBuilder::kDidSpill);
     safepoint_table_builder_.DefineSafepoint(&asm_, Safepoint::kNoLazyDeopt);
 
     __ FinishCall(imm.sig, call_descriptor);
@@ -2190,7 +2217,7 @@ class LiftoffCompiler {
           __ pc_offset(), SourcePosition(decoder->position()), false);
     }
 
-    RegisterDebugSideTableEntry();
+    RegisterDebugSideTableEntry(DebugSideTableBuilder::kDidSpill);
     safepoint_table_builder_.DefineSafepoint(&asm_, Safepoint::kNoLazyDeopt);
 
     __ FinishCall(imm.sig, call_descriptor);

src/wasm/wasm-debug.cc

@@ -707,6 +707,23 @@ class DebugInfoImpl {
                        int64_t{debug_side_table_entry->i32_constant(index)});
     }
 
+    if (debug_side_table_entry->is_register(index)) {
+      // TODO(clemensb): Implement by loading from the frame of the
+      // WasmDebugBreak builtin. The current values are just placeholders.
+      switch (type) {
+        case kWasmI32:
+          return WasmValue(int32_t{-11});
+        case kWasmI64:
+          return WasmValue(int64_t{-11});
+        case kWasmF32:
+          return WasmValue(float{-11});
+        case kWasmF64:
+          return WasmValue(double{-11});
+        default:
+          UNIMPLEMENTED();
+      }
+    }
+
     // Otherwise load the value from the stack.
     Address stack_address =
         stack_frame_base - debug_side_table_entry->stack_offset(index);

src/wasm/wasm-debug.h

@@ -40,13 +40,13 @@ class DebugSideTable {
  public:
   class Entry {
    public:
-    // TODO(clemensb): Add |kRegister|.
-    enum ValueKind : int8_t { kConstant, kStack };
+    enum ValueKind : int8_t { kConstant, kRegister, kStack };
     struct Value {
       ValueType type;
       ValueKind kind;
       union {
         int32_t i32_const;  // if kind == kConstant
+        int reg_code;       // if kind == kRegister
         int stack_offset;   // if kind == kStack
       };
     };
@@ -75,11 +75,20 @@ class DebugSideTable {
       return values_[index].kind == kConstant;
     }
 
+    bool is_register(int index) const {
+      return values_[index].kind == kRegister;
+    }
+
     int32_t i32_constant(int index) const {
       DCHECK_EQ(kConstant, values_[index].kind);
       return values_[index].i32_const;
     }
 
+    int32_t register_code(int index) const {
+      DCHECK_EQ(kRegister, values_[index].kind);
+      return values_[index].reg_code;
+    }
+
    private:
     int pc_offset_;
     std::vector<Value> values_;

test/cctest/wasm/test-liftoff-inspection.cc

@@ -64,23 +64,29 @@ class LiftoffCompileEnvironment {
   std::unique_ptr<DebugSideTable> GenerateDebugSideTable(
       std::initializer_list<ValueType> return_types,
       std::initializer_list<ValueType> param_types,
-      std::initializer_list<uint8_t> raw_function_bytes) {
+      std::initializer_list<uint8_t> raw_function_bytes,
+      std::vector<int> breakpoints = {}) {
     auto test_func = AddFunction(return_types, param_types, raw_function_bytes);
 
-    CompilationEnv env = module_builder_.CreateCompilationEnv();
-    std::unique_ptr<DebugSideTable> debug_side_table =
-        GenerateLiftoffDebugSideTable(CcTest::i_isolate()->allocator(), &env,
-                                      test_func.body);
-    // Check that {ExecuteLiftoffCompilation} provides the same debug side
-    // table.
+    CompilationEnv env = module_builder_.CreateCompilationEnv(
+        breakpoints.empty() ? TestingModuleBuilder::kNoDebug
+                            : TestingModuleBuilder::kDebug);
     WasmFeatures detected;
     std::unique_ptr<DebugSideTable> debug_side_table_via_compilation;
-    ExecuteLiftoffCompilation(CcTest::i_isolate()->allocator(), &env,
-                              test_func.body, 0, nullptr, &detected, {},
-                              &debug_side_table_via_compilation);
-    CheckTableEquals(*debug_side_table, *debug_side_table_via_compilation);
+    ExecuteLiftoffCompilation(
+        CcTest::i_isolate()->allocator(), &env, test_func.body, 0, nullptr,
+        &detected, VectorOf(breakpoints), &debug_side_table_via_compilation);
+
+    // If there are no breakpoint, then {ExecuteLiftoffCompilation} should
+    // provide the same debug side table.
+    if (breakpoints.empty()) {
+      std::unique_ptr<DebugSideTable> debug_side_table =
+          GenerateLiftoffDebugSideTable(CcTest::i_isolate()->allocator(), &env,
+                                        test_func.body);
+      CheckTableEquals(*debug_side_table, *debug_side_table_via_compilation);
+    }
 
-    return debug_side_table;
+    return debug_side_table_via_compilation;
   }
 
  private:
@@ -108,6 +114,9 @@ class LiftoffCompileEnvironment {
       case DebugSideTable::Entry::kConstant:
         CHECK_EQ(a.i32_const, b.i32_const);
         break;
+      case DebugSideTable::Entry::kRegister:
+        CHECK_EQ(a.reg_code, b.reg_code);
+        break;
       case DebugSideTable::Entry::kStack:
         CHECK_EQ(a.stack_offset, b.stack_offset);
         break;
@@ -196,10 +205,16 @@ std::ostream& operator<<(std::ostream& out, const DebugSideTableEntry& entry) {
   const char* comma = "";
   for (auto& v : entry.values) {
     out << comma << ValueTypes::TypeName(v.type) << " ";
-    if (v.kind == DebugSideTable::Entry::kConstant) {
-      out << "const:" << v.i32_const;
-    } else {
-      out << "stack";
+    switch (v.kind) {
+      case DebugSideTable::Entry::kConstant:
+        out << "const:" << v.i32_const;
+        break;
+      case DebugSideTable::Entry::kRegister:
+        out << "reg";
+        break;
+      case DebugSideTable::Entry::kStack:
+        out << "stack";
+        break;
     }
     comma = ", ";
   }
@@ -220,6 +235,12 @@ DebugSideTable::Entry::Value Constant(ValueType type, int32_t constant) {
   value.i32_const = constant;
   return value;
 }
+DebugSideTable::Entry::Value Register(ValueType type) {
+  DebugSideTable::Entry::Value value;
+  value.type = type;
+  value.kind = DebugSideTable::Entry::kRegister;
+  return value;
+}
 DebugSideTable::Entry::Value Stack(ValueType type) {
   DebugSideTable::Entry::Value value;
   value.type = type;
@@ -379,6 +400,29 @@ TEST(Liftoff_debug_side_table_trap) {
       debug_side_table.get());
 }
 
+TEST(Liftoff_breakpoint_simple) {
+  LiftoffCompileEnvironment env;
+  // Set two breakpoints. At both locations, values are live in registers.
+  auto debug_side_table = env.GenerateDebugSideTable(
+      {kWasmI32}, {kWasmI32, kWasmI32},
+      {WASM_I32_ADD(WASM_GET_LOCAL(0), WASM_GET_LOCAL(1))},
+      {
+          1,  // break at beginning of function (first local.get)
+          5   // break at i32.add
+      });
+  CheckDebugSideTable(
+      {
+          // First break point, locals in registers.
+          {Register(kWasmI32), Register(kWasmI32)},
+          // Second break point, locals and two stack values in registers.
+          {Register(kWasmI32), Register(kWasmI32), Register(kWasmI32),
+           Register(kWasmI32)},
+          // OOL stack check, locals spilled, stack empty.
+          {Stack(kWasmI32), Stack(kWasmI32)},
+      },
+      debug_side_table.get());
+}
+
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8

test/cctest/wasm/wasm-run-utils.cc

@@ -305,14 +305,18 @@ uint32_t TestingModuleBuilder::AddPassiveElementSegment(
   return index;
 }
 
-CompilationEnv TestingModuleBuilder::CreateCompilationEnv() {
+CompilationEnv TestingModuleBuilder::CreateCompilationEnv(
+    AssumeDebugging debug) {
   // This is a hack so we don't need to call
   // trap_handler::IsTrapHandlerEnabled().
   const bool is_trap_handler_enabled =
       V8_TRAP_HANDLER_SUPPORTED && i::FLAG_wasm_trap_handler;
   return {test_module_ptr_,
           is_trap_handler_enabled ? kUseTrapHandler : kNoTrapHandler,
-          runtime_exception_support_, enabled_features_, lower_simd()};
+          runtime_exception_support_,
+          enabled_features_,
+          lower_simd(),
+          debug};
 }
 
 const WasmGlobal* TestingModuleBuilder::AddGlobal(ValueType type) {

test/cctest/wasm/wasm-run-utils.h

@@ -222,7 +222,8 @@ class TestingModuleBuilder {
 
   void SetExecutable() { native_module_->SetExecutable(true); }
 
-  CompilationEnv CreateCompilationEnv();
+  enum AssumeDebugging : bool { kDebug = true, kNoDebug = false };
+  CompilationEnv CreateCompilationEnv(AssumeDebugging = kNoDebug);
 
   ExecutionTier execution_tier() const { return execution_tier_; }