CPU: Implement memory breakpoints/watchpoints

src/core/cpu_core.cpp

@@ -48,7 +48,12 @@ static bool IsCop0ExecutionBreakpointUnmasked();
 static void Cop0ExecutionBreakpointCheck();
 template<MemoryAccessType type>
 static void Cop0DataBreakpointCheck(VirtualMemoryAddress address);
-static void BreakpointCheck();
+
+static BreakpointList& GetBreakpointList(BreakpointType type);
+static bool CheckBreakpointList(BreakpointType type, VirtualMemoryAddress address);
+static void ExecutionBreakpointCheck();
+template<MemoryAccessType type>
+static void MemoryBreakpointCheck(VirtualMemoryAddress address);
 
 #ifdef _DEBUG
 static void TracePrintInstruction();
@@ -94,10 +99,11 @@ static bool s_log_file_opened = false;
 static bool s_trace_to_log = false;
 
 static constexpr u32 INVALID_BREAKPOINT_PC = UINT32_C(0xFFFFFFFF);
-static std::vector<Breakpoint> s_breakpoints;
+static std::array<std::vector<Breakpoint>, static_cast<u32>(BreakpointType::Count)> s_breakpoints;
 static u32 s_breakpoint_counter = 1;
 static u32 s_last_breakpoint_check_pc = INVALID_BREAKPOINT_PC;
-static u8 s_single_step = 0; // 0 - off, 1 - executing one, 2 - done
+static bool s_single_step = false;
+static bool s_break_after_instruction = false;
 } // namespace CPU
 
 bool CPU::IsTraceEnabled()
@@ -156,10 +162,12 @@ void CPU::Initialize()
   g_state.cop0_regs.PRID = UINT32_C(0x00000002);
 
   g_state.use_debug_dispatcher = false;
-  s_breakpoints.clear();
+  for (BreakpointList& bps : s_breakpoints)
+    bps.clear();
   s_breakpoint_counter = 1;
   s_last_breakpoint_check_pc = INVALID_BREAKPOINT_PC;
-  s_single_step = 0;
+  s_single_step = false;
+  s_break_after_instruction = false;
 
   UpdateMemoryPointers();
 
@@ -1436,7 +1444,10 @@ restart_instruction:
     {
       const VirtualMemoryAddress addr = ReadReg(inst.i.rs) + inst.i.imm_sext32();
       if constexpr (debug)
+      {
         Cop0DataBreakpointCheck<MemoryAccessType::Read>(addr);
+        MemoryBreakpointCheck<MemoryAccessType::Read>(addr);
+      }
 
       u8 value;
       if (!ReadMemoryByte(addr, &value))
@@ -1455,7 +1466,10 @@ restart_instruction:
     {
       const VirtualMemoryAddress addr = ReadReg(inst.i.rs) + inst.i.imm_sext32();
       if constexpr (debug)
+      {
         Cop0DataBreakpointCheck<MemoryAccessType::Read>(addr);
+        MemoryBreakpointCheck<MemoryAccessType::Read>(addr);
+      }
 
       u16 value;
       if (!ReadMemoryHalfWord(addr, &value))
@@ -1473,7 +1487,10 @@ restart_instruction:
     {
       const VirtualMemoryAddress addr = ReadReg(inst.i.rs) + inst.i.imm_sext32();
       if constexpr (debug)
+      {
         Cop0DataBreakpointCheck<MemoryAccessType::Read>(addr);
+        MemoryBreakpointCheck<MemoryAccessType::Read>(addr);
+      }
 
       u32 value;
       if (!ReadMemoryWord(addr, &value))
@@ -1490,7 +1507,10 @@ restart_instruction:
     {
       const VirtualMemoryAddress addr = ReadReg(inst.i.rs) + inst.i.imm_sext32();
       if constexpr (debug)
+      {
         Cop0DataBreakpointCheck<MemoryAccessType::Read>(addr);
+        MemoryBreakpointCheck<MemoryAccessType::Read>(addr);
+      }
 
       u8 value;
       if (!ReadMemoryByte(addr, &value))
@@ -1508,7 +1528,10 @@ restart_instruction:
     {
       const VirtualMemoryAddress addr = ReadReg(inst.i.rs) + inst.i.imm_sext32();
       if constexpr (debug)
+      {
         Cop0DataBreakpointCheck<MemoryAccessType::Read>(addr);
+        MemoryBreakpointCheck<MemoryAccessType::Read>(addr);
+      }
 
       u16 value;
       if (!ReadMemoryHalfWord(addr, &value))
@@ -1528,7 +1551,10 @@ restart_instruction:
       const VirtualMemoryAddress addr = ReadReg(inst.i.rs) + inst.i.imm_sext32();
       const VirtualMemoryAddress aligned_addr = addr & ~UINT32_C(3);
       if constexpr (debug)
+      {
         Cop0DataBreakpointCheck<MemoryAccessType::Read>(addr);
+        MemoryBreakpointCheck<MemoryAccessType::Read>(addr);
+      }
 
       u32 aligned_value;
       if (!ReadMemoryWord(aligned_addr, &aligned_value))
@@ -1560,7 +1586,10 @@ restart_instruction:
     {
       const VirtualMemoryAddress addr = ReadReg(inst.i.rs) + inst.i.imm_sext32();
       if constexpr (debug)
+      {
         Cop0DataBreakpointCheck<MemoryAccessType::Write>(addr);
+        MemoryBreakpointCheck<MemoryAccessType::Write>(addr);
+      }
 
       const u32 value = ReadReg(inst.i.rt);
       WriteMemoryByte(addr, value);
@@ -1574,7 +1603,10 @@ restart_instruction:
     {
       const VirtualMemoryAddress addr = ReadReg(inst.i.rs) + inst.i.imm_sext32();
       if constexpr (debug)
+      {
         Cop0DataBreakpointCheck<MemoryAccessType::Write>(addr);
+        MemoryBreakpointCheck<MemoryAccessType::Write>(addr);
+      }
 
       const u32 value = ReadReg(inst.i.rt);
       WriteMemoryHalfWord(addr, value);
@@ -1588,7 +1620,10 @@ restart_instruction:
     {
       const VirtualMemoryAddress addr = ReadReg(inst.i.rs) + inst.i.imm_sext32();
       if constexpr (debug)
+      {
         Cop0DataBreakpointCheck<MemoryAccessType::Write>(addr);
+        MemoryBreakpointCheck<MemoryAccessType::Write>(addr);
+      }
 
       const u32 value = ReadReg(inst.i.rt);
       WriteMemoryWord(addr, value);
@@ -1604,7 +1639,10 @@ restart_instruction:
       const VirtualMemoryAddress addr = ReadReg(inst.i.rs) + inst.i.imm_sext32();
       const VirtualMemoryAddress aligned_addr = addr & ~UINT32_C(3);
       if constexpr (debug)
+      {
         Cop0DataBreakpointCheck<MemoryAccessType::Write>(aligned_addr);
+        MemoryBreakpointCheck<MemoryAccessType::Write>(aligned_addr);
+      }
 
       const u32 reg_value = ReadReg(inst.i.rt);
       const u8 shift = (Truncate8(addr) & u8(3)) * u8(8);
@@ -1933,7 +1971,7 @@ void CPU::DispatchInterrupt()
 
 bool CPU::UpdateDebugDispatcherFlag()
 {
-  const bool has_any_breakpoints = !s_breakpoints.empty() || (s_single_step != 0);
+  const bool has_any_breakpoints = HasAnyBreakpoints() || s_single_step;
 
   // TODO: cop0 breakpoints
   const auto& dcic = g_state.cop0_regs.dcic;
@@ -1962,12 +2000,26 @@ void CPU::ExitExecution()
 
 bool CPU::HasAnyBreakpoints()
 {
-  return !s_breakpoints.empty();
+  return (GetBreakpointList(BreakpointType::Execute).size() + GetBreakpointList(BreakpointType::Read).size() +
+          GetBreakpointList(BreakpointType::Write).size()) > 0;
 }
 
-bool CPU::HasBreakpointAtAddress(VirtualMemoryAddress address)
+ALWAYS_INLINE CPU::BreakpointList& CPU::GetBreakpointList(BreakpointType type)
 {
-  for (const Breakpoint& bp : s_breakpoints)
+  return s_breakpoints[static_cast<size_t>(type)];
+}
+
+const char* CPU::GetBreakpointTypeName(BreakpointType type)
+{
+  static constexpr std::array<const char*, static_cast<u32>(BreakpointType::Count)> names = {
+    {TRANSLATE_NOOP("DebuggerWindow", "Execute"), TRANSLATE_NOOP("DebuggerWindow", "Read"),
+     TRANSLATE_NOOP("DebuggerWindow", "Write")}};
+  return Host::TranslateToCString("DebuggerWindow", names[static_cast<size_t>(type)]);
+}
+
+bool CPU::HasBreakpointAtAddress(BreakpointType type, VirtualMemoryAddress address)
+{
+  for (const Breakpoint& bp : GetBreakpointList(type))
   {
     if (bp.address == address)
       return true;
@@ -1976,68 +2028,79 @@ bool CPU::HasBreakpointAtAddress(VirtualMemoryAddress address)
   return false;
 }
 
-CPU::BreakpointList CPU::GetBreakpointList(bool include_auto_clear, bool include_callbacks)
+CPU::BreakpointList CPU::CopyBreakpointList(bool include_auto_clear, bool include_callbacks)
 {
   BreakpointList bps;
-  bps.reserve(s_breakpoints.size());
 
-  for (const Breakpoint& bp : s_breakpoints)
+  size_t total = 0;
+  for (const BreakpointList& bplist : s_breakpoints)
+    total += bplist.size();
+
+  bps.reserve(total);
+
+  for (const BreakpointList& bplist : s_breakpoints)
   {
-    if (bp.callback && !include_callbacks)
+    for (const Breakpoint& bp : bplist)
-      continue;
+    {
-    if (bp.auto_clear && !include_auto_clear)
+      if (bp.callback && !include_callbacks)
-      continue;
+        continue;
+      if (bp.auto_clear && !include_auto_clear)
+        continue;
 
-    bps.push_back(bp);
+      bps.push_back(bp);
+    }
   }
 
   return bps;
 }
 
-bool CPU::AddBreakpoint(VirtualMemoryAddress address, bool auto_clear, bool enabled)
+bool CPU::AddBreakpoint(BreakpointType type, VirtualMemoryAddress address, bool auto_clear, bool enabled)
 {
-  if (HasBreakpointAtAddress(address))
+  if (HasBreakpointAtAddress(type, address))
     return false;
 
-  Log_InfoPrintf("Adding breakpoint at %08X, auto clear = %u", address, static_cast<unsigned>(auto_clear));
+  Log_InfoFmt("Adding {} breakpoint at {:08X}, auto clear = %u", GetBreakpointTypeName(type), address,
+              static_cast<unsigned>(auto_clear));
 
-  Breakpoint bp{address, nullptr, auto_clear ? 0 : s_breakpoint_counter++, 0, auto_clear, enabled};
+  Breakpoint bp{address, nullptr, auto_clear ? 0 : s_breakpoint_counter++, 0, type, auto_clear, enabled};
-  s_breakpoints.push_back(std::move(bp));
+  GetBreakpointList(type).push_back(std::move(bp));
   if (UpdateDebugDispatcherFlag())
     System::InterruptExecution();
 
   if (!auto_clear)
   {
-    Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerMessage", "Added breakpoint at 0x%08X."), address);
+    Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerWindow", "Added breakpoint at 0x%08X."), address);
   }
 
   return true;
 }
 
-bool CPU::AddBreakpointWithCallback(VirtualMemoryAddress address, BreakpointCallback callback)
+bool CPU::AddBreakpointWithCallback(BreakpointType type, VirtualMemoryAddress address, BreakpointCallback callback)
 {
-  if (HasBreakpointAtAddress(address))
+  if (HasBreakpointAtAddress(type, address))
     return false;
 
-  Log_InfoPrintf("Adding breakpoint with callback at %08X", address);
+  Log_InfoFmt("Adding {} breakpoint with callback at {:08X}", GetBreakpointTypeName(type), address);
 
-  Breakpoint bp{address, callback, 0, 0, false, true};
+  Breakpoint bp{address, callback, 0, 0, type, false, true};
-  s_breakpoints.push_back(std::move(bp));
+  GetBreakpointList(type).push_back(std::move(bp));
   if (UpdateDebugDispatcherFlag())
     System::InterruptExecution();
   return true;
 }
 
-bool CPU::RemoveBreakpoint(VirtualMemoryAddress address)
+bool CPU::RemoveBreakpoint(BreakpointType type, VirtualMemoryAddress address)
 {
-  auto it = std::find_if(s_breakpoints.begin(), s_breakpoints.end(),
+  BreakpointList& bplist = GetBreakpointList(type);
-                         [address](const Breakpoint& bp) { return bp.address == address; });
+  auto it =
-  if (it == s_breakpoints.end())
+    std::find_if(bplist.begin(), bplist.end(), [address](const Breakpoint& bp) { return bp.address == address; });
+  if (it == bplist.end())
     return false;
 
-  Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerMessage", "Removed breakpoint at 0x%08X."), address);
+  Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerWindow", "Removed %s breakpoint at 0x%08X."),
+                                       GetBreakpointTypeName(type), address);
 
-  s_breakpoints.erase(it);
+  bplist.erase(it);
   if (UpdateDebugDispatcherFlag())
     System::InterruptExecution();
 
@@ -2049,7 +2112,8 @@ bool CPU::RemoveBreakpoint(VirtualMemoryAddress address)
 
 void CPU::ClearBreakpoints()
 {
-  s_breakpoints.clear();
+  for (BreakpointList& bplist : s_breakpoints)
+    bplist.clear();
   s_breakpoint_counter = 0;
   s_last_breakpoint_check_pc = INVALID_BREAKPOINT_PC;
   if (UpdateDebugDispatcherFlag())
@@ -2068,7 +2132,7 @@ bool CPU::AddStepOverBreakpoint()
 
   if (!IsCallInstruction(inst))
   {
-    Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerMessage", "0x%08X is not a call instruction."), g_state.pc);
+    Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerWindow", "0x%08X is not a call instruction."), g_state.pc);
     return false;
   }
 
@@ -2077,7 +2141,7 @@ bool CPU::AddStepOverBreakpoint()
 
   if (IsBranchInstruction(inst))
   {
-    Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerMessage", "Can't step over double branch at 0x%08X"),
+    Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerWindow", "Can't step over double branch at 0x%08X"),
                                          g_state.pc);
     return false;
   }
@@ -2085,9 +2149,9 @@ bool CPU::AddStepOverBreakpoint()
   // skip the delay slot
   bp_pc += sizeof(Instruction);
 
-  Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerMessage", "Stepping over to 0x%08X."), bp_pc);
+  Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerWindow", "Stepping over to 0x%08X."), bp_pc);
 
-  return AddBreakpoint(bp_pc, true);
+  return AddBreakpoint(BreakpointType::Execute, bp_pc, true);
 }
 
 bool CPU::AddStepOutBreakpoint(u32 max_instructions_to_search)
@@ -2102,42 +2166,36 @@ bool CPU::AddStepOutBreakpoint(u32 max_instructions_to_search)
     if (!SafeReadInstruction(ret_pc, &inst.bits))
     {
       Host::ReportFormattedDebuggerMessage(
-        TRANSLATE("DebuggerMessage", "Instruction read failed at %08X while searching for function end."), ret_pc);
+        TRANSLATE("DebuggerWindow", "Instruction read failed at %08X while searching for function end."), ret_pc);
       return false;
     }
 
     if (IsReturnInstruction(inst))
     {
-      Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerMessage", "Stepping out to 0x%08X."), ret_pc);
+      Host::ReportFormattedDebuggerMessage(TRANSLATE("DebuggerWindow", "Stepping out to 0x%08X."), ret_pc);
 
-      return AddBreakpoint(ret_pc, true);
+      return AddBreakpoint(BreakpointType::Execute, ret_pc, true);
     }
   }
 
   Host::ReportFormattedDebuggerMessage(
-    TRANSLATE("DebuggerMessage", "No return instruction found after %u instructions for step-out at %08X."),
+    TRANSLATE("DebuggerWindow", "No return instruction found after %u instructions for step-out at %08X."),
     max_instructions_to_search, g_state.pc);
 
   return false;
 }
 
-ALWAYS_INLINE_RELEASE void CPU::BreakpointCheck()
+ALWAYS_INLINE_RELEASE bool CPU::CheckBreakpointList(BreakpointType type, VirtualMemoryAddress address)
 {
-  const u32 pc = g_state.pc;
+  BreakpointList& bplist = GetBreakpointList(type);
+  size_t count = bplist.size();
+  if (count == 0) [[likely]]
+    return false;
 
-  if (pc == s_last_breakpoint_check_pc && !s_single_step) [[unlikely]]
+  for (size_t i = 0; i < count;)
   {
-    // we don't want to trigger the same breakpoint which just paused us repeatedly.
+    Breakpoint& bp = bplist[i];
-    return;
+    if (!bp.enabled || bp.address != address)
-  }
-
-  s_last_breakpoint_check_pc = pc;
-
-  u32 count = static_cast<u32>(s_breakpoints.size());
-  for (u32 i = 0; i < count;)
-  {
-    Breakpoint& bp = s_breakpoints[i];
-    if (!bp.enabled || bp.address != pc)
     {
       i++;
       continue;
@@ -2145,12 +2203,14 @@ ALWAYS_INLINE_RELEASE void CPU::BreakpointCheck()
 
     bp.hit_count++;
 
+    const u32 pc = g_state.pc;
+
     if (bp.callback)
     {
       // if callback returns false, the bp is no longer recorded
-      if (!bp.callback(pc))
+      if (!bp.callback(BreakpointType::Execute, pc, address))
       {
-        s_breakpoints.erase(s_breakpoints.begin() + i);
+        bplist.erase(bplist.begin() + i);
         count--;
         UpdateDebugDispatcherFlag();
       }
@@ -2166,41 +2226,60 @@ ALWAYS_INLINE_RELEASE void CPU::BreakpointCheck()
       if (bp.auto_clear)
       {
         Host::ReportFormattedDebuggerMessage("Stopped execution at 0x%08X.", pc);
-        s_breakpoints.erase(s_breakpoints.begin() + i);
+        bplist.erase(bplist.begin() + i);
         count--;
         UpdateDebugDispatcherFlag();
       }
       else
       {
-        Host::ReportFormattedDebuggerMessage("Hit breakpoint %u at 0x%08X.", bp.number, pc);
+        Host::ReportFormattedDebuggerMessage("Hit %s breakpoint %u at 0x%08X.", GetBreakpointTypeName(type), bp.number,
+                                             address);
         i++;
       }
 
-      s_single_step = 0;
+      return true;
-      ExitExecution();
     }
   }
 
-  // single step - we want to break out after this instruction, so set a pending exit
+  return false;
-  // the bp check happens just before execution, so this is fine
+}
-  if (s_single_step != 0) [[unlikely]]
+
+ALWAYS_INLINE_RELEASE void CPU::ExecutionBreakpointCheck()
+{
+  if (s_single_step) [[unlikely]]
   {
-    // not resetting this will break double single stepping, because we broke on the next instruction.
+    // single step ignores breakpoints, since it stops anyway
-    s_last_breakpoint_check_pc = INVALID_BREAKPOINT_PC;
+    s_single_step = false;
-
+    s_break_after_instruction = true;
-    if (s_single_step == 2)
+    Host::ReportFormattedDebuggerMessage("Stepped to 0x%08X.", g_state.npc);
-    {
+    return;
-      s_single_step = 0;
-      System::PauseSystem(true);
-
-      Host::ReportFormattedDebuggerMessage("Stepped to 0x%08X.", g_state.pc);
-      UpdateDebugDispatcherFlag();
-      ExitExecution();
-    }
-
-    // if there's a bp on the instruction we're about to execute, we should've paused already
-    s_single_step = 2;
   }
+
+  if (s_breakpoints[static_cast<u32>(BreakpointType::Execute)].empty()) [[likely]]
+    return;
+
+  const u32 pc = g_state.pc;
+  if (pc == s_last_breakpoint_check_pc) [[unlikely]]
+  {
+    // we don't want to trigger the same breakpoint which just paused us repeatedly.
+    return;
+  }
+
+  s_last_breakpoint_check_pc = pc;
+
+  if (CheckBreakpointList(BreakpointType::Execute, pc)) [[unlikely]]
+  {
+    s_single_step = false;
+    ExitExecution();
+  }
+}
+
+template<MemoryAccessType type>
+ALWAYS_INLINE_RELEASE void CPU::MemoryBreakpointCheck(VirtualMemoryAddress address)
+{
+  const BreakpointType bptype = (type == MemoryAccessType::Read) ? BreakpointType::Read : BreakpointType::Write;
+  if (CheckBreakpointList(bptype, address))
+    s_break_after_instruction = true;
 }
 
 template<PGXPMode pgxp_mode, bool debug>
@@ -2215,7 +2294,7 @@ template<PGXPMode pgxp_mode, bool debug>
       if constexpr (debug)
       {
         Cop0ExecutionBreakpointCheck();
-        BreakpointCheck();
+        ExecutionBreakpointCheck();
       }
 
       g_state.pending_ticks++;
@@ -2258,6 +2337,17 @@ template<PGXPMode pgxp_mode, bool debug>
 
       // next load delay
       UpdateLoadDelay();
+
+      if constexpr (debug)
+      {
+        if (s_break_after_instruction)
+        {
+          s_break_after_instruction = false;
+          System::PauseSystem(true);
+          UpdateDebugDispatcherFlag();
+          ExitExecution();
+        }
+      }
     }
   }
 }
@@ -2334,7 +2424,7 @@ void CPU::Execute()
 
 void CPU::SetSingleStepFlag()
 {
-  s_single_step = 1;
+  s_single_step = true;
   if (UpdateDebugDispatcherFlag())
     System::InterruptExecution();
 }

src/core/cpu_core.h

@@ -191,8 +191,17 @@ bool IsTraceEnabled();
 void StartTrace();
 void StopTrace();
 
+// Breakpoint types - execute => breakpoint, read/write => watchpoints
+enum class BreakpointType : u8
+{
+  Execute,
+  Read,
+  Write,
+  Count
+};
+
 // Breakpoint callback - if the callback returns false, the breakpoint will be removed.
-using BreakpointCallback = bool (*)(VirtualMemoryAddress address);
+using BreakpointCallback = bool (*)(BreakpointType type, VirtualMemoryAddress pc, VirtualMemoryAddress memaddr);
 
 struct Breakpoint
 {
@@ -200,6 +209,7 @@ struct Breakpoint
   BreakpointCallback callback;
   u32 number;
   u32 hit_count;
+  BreakpointType type;
   bool auto_clear;
   bool enabled;
 };
@@ -207,12 +217,13 @@ struct Breakpoint
 using BreakpointList = std::vector<Breakpoint>;
 
 // Breakpoints
+const char* GetBreakpointTypeName(BreakpointType type);
 bool HasAnyBreakpoints();
-bool HasBreakpointAtAddress(VirtualMemoryAddress address);
+bool HasBreakpointAtAddress(BreakpointType type, VirtualMemoryAddress address);
-BreakpointList GetBreakpointList(bool include_auto_clear = false, bool include_callbacks = false);
+BreakpointList CopyBreakpointList(bool include_auto_clear = false, bool include_callbacks = false);
-bool AddBreakpoint(VirtualMemoryAddress address, bool auto_clear = false, bool enabled = true);
+bool AddBreakpoint(BreakpointType type, VirtualMemoryAddress address, bool auto_clear = false, bool enabled = true);
-bool AddBreakpointWithCallback(VirtualMemoryAddress address, BreakpointCallback callback);
+bool AddBreakpointWithCallback(BreakpointType type, VirtualMemoryAddress address, BreakpointCallback callback);
-bool RemoveBreakpoint(VirtualMemoryAddress address);
+bool RemoveBreakpoint(BreakpointType type, VirtualMemoryAddress address);
 void ClearBreakpoints();
 bool AddStepOverBreakpoint();
 bool AddStepOutBreakpoint(u32 max_instructions_to_search = 1000);

src/core/gdb_protocol.cpp

@@ -219,7 +219,7 @@ static std::optional<std::string> Cmd$z1(const std::string_view& data)
 {
   auto address = StringUtil::FromChars<VirtualMemoryAddress>(data, 16);
   if (address) {
-    CPU::RemoveBreakpoint(*address);
+    CPU::RemoveBreakpoint(CPU::BreakpointType::Execute, *address);
     return { "OK" };
   }
   else {
@@ -232,7 +232,7 @@ static std::optional<std::string> Cmd$Z1(const std::string_view& data)
 {
   auto address = StringUtil::FromChars<VirtualMemoryAddress>(data, 16);
   if (address) {
-    CPU::AddBreakpoint(*address, false);
+    CPU::AddBreakpoint(CPU::BreakpointType::Execute, *address, false);
     return { "OK" };
   }
   else {

src/duckstation-qt/CMakeLists.txt

@@ -64,6 +64,7 @@ set(SRCS
   coverdownloaddialog.cpp
   coverdownloaddialog.h
   coverdownloaddialog.ui
+  debuggeraddbreakpointdialog.ui
   debuggermodels.cpp
   debuggermodels.h
   debuggerwindow.cpp

src/duckstation-qt/debuggeraddbreakpointdialog.ui

@@ -0,0 +1,93 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ui version="4.0">
+ <class>DebuggerAddBreakpointDialog</class>
+ <widget class="QDialog" name="DebuggerAddBreakpointDialog">
+  <property name="geometry">
+   <rect>
+    <x>0</x>
+    <y>0</y>
+    <width>400</width>
+    <height>149</height>
+   </rect>
+  </property>
+  <property name="windowTitle">
+   <string>Add Breakpoint</string>
+  </property>
+  <layout class="QFormLayout" name="formLayout">
+   <item row="0" column="0">
+    <widget class="QLabel" name="label">
+     <property name="text">
+      <string>Address:</string>
+     </property>
+    </widget>
+   </item>
+   <item row="0" column="1">
+    <widget class="QLineEdit" name="address"/>
+   </item>
+   <item row="1" column="0">
+    <widget class="QLabel" name="label_2">
+     <property name="text">
+      <string>Type:</string>
+     </property>
+    </widget>
+   </item>
+   <item row="1" column="1">
+    <layout class="QVBoxLayout" name="verticalLayout">
+     <item>
+      <widget class="QRadioButton" name="execute">
+       <property name="text">
+        <string>Execute</string>
+       </property>
+       <property name="checked">
+        <bool>true</bool>
+       </property>
+      </widget>
+     </item>
+     <item>
+      <widget class="QRadioButton" name="read">
+       <property name="text">
+        <string>Read</string>
+       </property>
+      </widget>
+     </item>
+     <item>
+      <widget class="QRadioButton" name="write">
+       <property name="text">
+        <string>Write</string>
+       </property>
+      </widget>
+     </item>
+    </layout>
+   </item>
+   <item row="2" column="0" colspan="2">
+    <widget class="QDialogButtonBox" name="buttonBox">
+     <property name="orientation">
+      <enum>Qt::Horizontal</enum>
+     </property>
+     <property name="standardButtons">
+      <set>QDialogButtonBox::Cancel|QDialogButtonBox::Ok</set>
+     </property>
+    </widget>
+   </item>
+  </layout>
+ </widget>
+ <resources/>
+ <connections>
+  <connection>
+   <sender>buttonBox</sender>
+   <signal>rejected()</signal>
+   <receiver>DebuggerAddBreakpointDialog</receiver>
+   <slot>reject()</slot>
+   <hints>
+    <hint type="sourcelabel">
+     <x>316</x>
+     <y>260</y>
+    </hint>
+    <hint type="destinationlabel">
+     <x>286</x>
+     <y>274</y>
+    </hint>
+   </hints>
+  </connection>
+ </connections>
+</ui>

src/duckstation-qt/debuggermodels.cpp

@@ -1,4 +1,4 @@
-// SPDX-FileCopyrightText: 2019-2022 Connor McLaughlin <stenzek@gmail.com>
+// SPDX-FileCopyrightText: 2019-2024 Connor McLaughlin <stenzek@gmail.com>
 // SPDX-License-Identifier: (GPL-3.0 OR CC-BY-NC-ND-4.0)
 
 #include "debuggermodels.h"
@@ -13,6 +13,8 @@
 #include <QtGui/QIcon>
 #include <QtGui/QPalette>
 #include <QtWidgets/QApplication>
+#include <QtWidgets/QMessageBox>
+#include <QtWidgets/QPushButton>
 
 static constexpr int NUM_COLUMNS = 5;
 static constexpr int STACK_RANGE = 128;
@@ -449,3 +451,44 @@ void DebuggerStackModel::invalidateView()
   beginResetModel();
   endResetModel();
 }
+
+DebuggerAddBreakpointDialog::DebuggerAddBreakpointDialog(QWidget* parent /*= nullptr*/) : QDialog(parent)
+{
+  m_ui.setupUi(this);
+  connect(m_ui.buttonBox->button(QDialogButtonBox::Ok), &QAbstractButton::clicked, this,
+          &DebuggerAddBreakpointDialog::okClicked);
+}
+
+DebuggerAddBreakpointDialog::~DebuggerAddBreakpointDialog() = default;
+
+void DebuggerAddBreakpointDialog::okClicked()
+{
+  const QString address_str = m_ui.address->text();
+  m_address = 0;
+  bool ok = false;
+
+  if (!address_str.isEmpty())
+  {
+    if (address_str.startsWith("0x"))
+      m_address = address_str.mid(2).toUInt(&ok, 16);
+    else
+      m_address = address_str.toUInt(&ok, 16);
+
+    if (!ok)
+    {
+      QMessageBox::critical(
+        this, qApp->translate("DebuggerWindow", "Error"),
+        qApp->translate("DebuggerWindow", "Invalid address. It should be in hex (0x12345678 or 12345678)"));
+      return;
+    }
+
+    if (m_ui.read->isChecked())
+      m_type = CPU::BreakpointType::Read;
+    else if (m_ui.write->isChecked())
+      m_type = CPU::BreakpointType::Write;
+    else
+      m_type = CPU::BreakpointType::Execute;
+
+    accept();
+  }
+}

src/duckstation-qt/debuggermodels.h

@@ -1,27 +1,32 @@
-// SPDX-FileCopyrightText: 2019-2022 Connor McLaughlin <stenzek@gmail.com>
+// SPDX-FileCopyrightText: 2019-2024 Connor McLaughlin <stenzek@gmail.com>
 // SPDX-License-Identifier: (GPL-3.0 OR CC-BY-NC-ND-4.0)
 
 #pragma once
+
+#include "ui_debuggeraddbreakpointdialog.h"
+
 #include "core/bus.h"
 #include "core/cpu_core.h"
 #include "core/cpu_types.h"
+
 #include <QtCore/QAbstractListModel>
 #include <QtCore/QAbstractTableModel>
 #include <QtGui/QPixmap>
+#include <QtWidgets/QDialog>
 #include <map>
 
-class DebuggerCodeModel : public QAbstractTableModel
+class DebuggerCodeModel final : public QAbstractTableModel
 {
   Q_OBJECT
 
 public:
   DebuggerCodeModel(QObject* parent = nullptr);
-  virtual ~DebuggerCodeModel();
+  ~DebuggerCodeModel() override;
 
-  virtual int rowCount(const QModelIndex& parent = QModelIndex()) const override;
+  int rowCount(const QModelIndex& parent = QModelIndex()) const override;
-  virtual int columnCount(const QModelIndex& parent = QModelIndex()) const override;
+  int columnCount(const QModelIndex& parent = QModelIndex()) const override;
-  virtual QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
+  QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
-  virtual QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override;
+  QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override;
 
   // Returns the row for this instruction pointer
   void resetCodeView(VirtualMemoryAddress start_address);
@@ -51,39 +56,59 @@ private:
   QPixmap m_breakpoint_pixmap;
 };
 
-class DebuggerRegistersModel : public QAbstractListModel
+class DebuggerRegistersModel final : public QAbstractListModel
 {
   Q_OBJECT
 
 public:
   DebuggerRegistersModel(QObject* parent = nullptr);
-  virtual ~DebuggerRegistersModel();
+  ~DebuggerRegistersModel() override;
 
-  virtual int rowCount(const QModelIndex& parent = QModelIndex()) const override;
+  int rowCount(const QModelIndex& parent = QModelIndex()) const override;
-  virtual int columnCount(const QModelIndex& parent = QModelIndex()) const override;
+  int columnCount(const QModelIndex& parent = QModelIndex()) const override;
-  virtual QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
+  QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
-  virtual QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override;
+  QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override;
 
   void updateValues();
   void saveCurrentValues();
 
 private:
-  std::array<u32 ,CPU::NUM_DEBUGGER_REGISTER_LIST_ENTRIES> m_reg_values = {};
+  std::array<u32, CPU::NUM_DEBUGGER_REGISTER_LIST_ENTRIES> m_reg_values = {};
   std::array<u32, CPU::NUM_DEBUGGER_REGISTER_LIST_ENTRIES> m_old_reg_values = {};
 };
 
-class DebuggerStackModel : public QAbstractListModel
+class DebuggerStackModel final : public QAbstractListModel
 {
   Q_OBJECT
 
 public:
   DebuggerStackModel(QObject* parent = nullptr);
-  virtual ~DebuggerStackModel();
+  ~DebuggerStackModel() override;
 
-  virtual int rowCount(const QModelIndex& parent = QModelIndex()) const override;
+  int rowCount(const QModelIndex& parent = QModelIndex()) const override;
-  virtual int columnCount(const QModelIndex& parent = QModelIndex()) const override;
+  int columnCount(const QModelIndex& parent = QModelIndex()) const override;
-  virtual QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
+  QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
-  virtual QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override;
+  QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override;
 
   void invalidateView();
 };
+
+class DebuggerAddBreakpointDialog final : public QDialog
+{
+  Q_OBJECT
+
+public:
+  DebuggerAddBreakpointDialog(QWidget* parent = nullptr);
+  ~DebuggerAddBreakpointDialog() override;
+
+  u32 getAddress() const { return m_address; }
+  CPU::BreakpointType getType() const { return m_type; }
+
+private Q_SLOTS:
+  void okClicked();
+
+private:
+  Ui::DebuggerAddBreakpointDialog m_ui;
+  u32 m_address = 0;
+  CPU::BreakpointType m_type = CPU::BreakpointType::Execute;
+};

src/duckstation-qt/debuggerwindow.cpp

@@ -10,6 +10,7 @@
 #include "core/cpu_core_private.h"
 
 #include <QtCore/QSignalBlocker>
+#include <QtGui/QCursor>
 #include <QtGui/QFontDatabase>
 #include <QtWidgets/QFileDialog>
 #include <QtWidgets/QMessageBox>
@@ -41,7 +42,6 @@ void DebuggerWindow::onSystemPaused()
 {
   setUIEnabled(true, true);
   refreshAll();
-  refreshBreakpointList();
 
   {
     QSignalBlocker sb(m_ui.actionPause);
@@ -111,7 +111,7 @@ void DebuggerWindow::onRunToCursorTriggered()
     return;
   }
 
-  CPU::AddBreakpoint(addr.value(), true, true);
+  CPU::AddBreakpoint(CPU::BreakpointType::Execute, addr.value(), true, true);
   g_emu_thread->setSystemPaused(false);
 }
 
@@ -163,18 +163,11 @@ void DebuggerWindow::onFollowAddressTriggered()
 
 void DebuggerWindow::onAddBreakpointTriggered()
 {
-  std::optional<VirtualMemoryAddress> address =
+  DebuggerAddBreakpointDialog dlg(this);
-    QtUtils::PromptForAddress(this, windowTitle(), tr("Enter code address:"), true);
+  if (!dlg.exec())
-  if (!address.has_value())
     return;
 
-  if (CPU::HasBreakpointAtAddress(address.value()))
+  addBreakpoint(dlg.getType(), dlg.getAddress());
-  {
-    QMessageBox::critical(this, windowTitle(), tr("A breakpoint already exists at this address."));
-    return;
-  }
-
-  toggleBreakpoint(address.value());
 }
 
 void DebuggerWindow::onToggleBreakpointTriggered()
@@ -191,6 +184,22 @@ void DebuggerWindow::onClearBreakpointsTriggered()
   clearBreakpoints();
 }
 
+void DebuggerWindow::onBreakpointListContextMenuRequested()
+{
+  const QList<QTreeWidgetItem*> selected = m_ui.breakpointsWidget->selectedItems();
+  if (selected.size() != 1)
+    return;
+
+  const QTreeWidgetItem* item = selected[0];
+  const u32 address = item->data(1, Qt::UserRole).toUInt();
+  const CPU::BreakpointType type = static_cast<CPU::BreakpointType>(item->data(2, Qt::UserRole).toUInt());
+
+  QMenu menu(this);
+  connect(menu.addAction(tr("&Remove")), &QAction::triggered, this,
+          [this, address, type]() { removeBreakpoint(type, address); });
+  menu.exec(QCursor::pos());
+}
+
 void DebuggerWindow::onStepIntoActionTriggered()
 {
   Assert(System::IsPaused());
@@ -268,7 +277,7 @@ void DebuggerWindow::onCodeViewContextMenuRequested(const QPoint& pt)
   action = menu.addAction(QIcon::fromTheme("debugger-go-to-cursor"), tr("&Run To Cursor"));
   connect(action, &QAction::triggered, this, [address]() {
     Host::RunOnCPUThread([address]() {
-      CPU::AddBreakpoint(address, true, true);
+      CPU::AddBreakpoint(CPU::BreakpointType::Execute, address, true, true);
       g_emu_thread->setSystemPaused(false);
     });
   });
@@ -426,6 +435,7 @@ void DebuggerWindow::setupAdditionalUi()
   m_ui.stackView->setFont(fixedFont);
 
   m_ui.codeView->setContextMenuPolicy(Qt::CustomContextMenu);
+  m_ui.breakpointsWidget->setContextMenuPolicy(Qt::CustomContextMenu);
 
   setCentralWidget(nullptr);
   delete m_ui.centralwidget;
@@ -454,6 +464,8 @@ void DebuggerWindow::connectSignals()
   connect(m_ui.actionClose, &QAction::triggered, this, &DebuggerWindow::close);
   connect(m_ui.codeView, &QTreeView::activated, this, &DebuggerWindow::onCodeViewItemActivated);
   connect(m_ui.codeView, &QTreeView::customContextMenuRequested, this, &DebuggerWindow::onCodeViewContextMenuRequested);
+  connect(m_ui.breakpointsWidget, &QTreeWidget::customContextMenuRequested, this,
+          &DebuggerWindow::onBreakpointListContextMenuRequested);
 
   connect(m_ui.memoryRegionRAM, &QRadioButton::clicked, [this]() { setMemoryViewRegion(Bus::MemoryRegion::RAM); });
   connect(m_ui.memoryRegionEXP1, &QRadioButton::clicked, [this]() { setMemoryViewRegion(Bus::MemoryRegion::EXP1); });
@@ -492,7 +504,8 @@ void DebuggerWindow::createModels()
 
   m_ui.breakpointsWidget->setColumnWidth(0, 50);
   m_ui.breakpointsWidget->setColumnWidth(1, 80);
-  m_ui.breakpointsWidget->setColumnWidth(2, 40);
+  m_ui.breakpointsWidget->setColumnWidth(2, 50);
+  m_ui.breakpointsWidget->setColumnWidth(3, 40);
   m_ui.breakpointsWidget->setRootIsDecorated(false);
 }
 
@@ -553,19 +566,19 @@ void DebuggerWindow::setMemoryViewRegion(Bus::MemoryRegion region)
 void DebuggerWindow::toggleBreakpoint(VirtualMemoryAddress address)
 {
   Host::RunOnCPUThread([this, address]() {
-    const bool new_bp_state = !CPU::HasBreakpointAtAddress(address);
+    const bool new_bp_state = !CPU::HasBreakpointAtAddress(CPU::BreakpointType::Execute, address);
     if (new_bp_state)
     {
-      if (!CPU::AddBreakpoint(address, false))
+      if (!CPU::AddBreakpoint(CPU::BreakpointType::Execute, address, false))
         return;
     }
     else
     {
-      if (!CPU::RemoveBreakpoint(address))
+      if (!CPU::RemoveBreakpoint(CPU::BreakpointType::Execute, address))
         return;
     }
 
-    QtHost::RunOnUIThread([this, address, new_bp_state, bps = CPU::GetBreakpointList()]() {
+    QtHost::RunOnUIThread([this, address, new_bp_state, bps = CPU::CopyBreakpointList()]() {
       m_code_model->setBreakpointState(address, new_bp_state);
       refreshBreakpointList(bps);
     });
@@ -618,7 +631,8 @@ bool DebuggerWindow::scrollToMemoryAddress(VirtualMemoryAddress address)
 
 void DebuggerWindow::refreshBreakpointList()
 {
-  refreshBreakpointList(CPU::GetBreakpointList());
+  Host::RunOnCPUThread(
+    [this]() { QtHost::RunOnUIThread([this, bps = CPU::CopyBreakpointList()]() { refreshBreakpointList(bps); }); });
 }
 
 void DebuggerWindow::refreshBreakpointList(const CPU::BreakpointList& bps)
@@ -633,7 +647,50 @@ void DebuggerWindow::refreshBreakpointList(const CPU::BreakpointList& bps)
     item->setCheckState(0, bp.enabled ? Qt::Checked : Qt::Unchecked);
     item->setText(0, QString::asprintf("%u", bp.number));
     item->setText(1, QString::asprintf("0x%08X", bp.address));
-    item->setText(2, QString::asprintf("%u", bp.hit_count));
+    item->setText(2, QString::fromUtf8(CPU::GetBreakpointTypeName(bp.type)));
+    item->setText(3, QString::asprintf("%u", bp.hit_count));
+    item->setData(0, Qt::UserRole, bp.number);
+    item->setData(1, Qt::UserRole, bp.address);
+    item->setData(2, Qt::UserRole, static_cast<u32>(bp.type));
     m_ui.breakpointsWidget->addTopLevelItem(item);
   }
 }
+
+void DebuggerWindow::addBreakpoint(CPU::BreakpointType type, u32 address)
+{
+  Host::RunOnCPUThread([this, address, type]() {
+    const bool result = CPU::AddBreakpoint(type, address);
+    QtHost::RunOnUIThread([this, address, type, result, bps = CPU::CopyBreakpointList()]() {
+      if (!result)
+      {
+        QMessageBox::critical(this, windowTitle(),
+                              tr("Failed to add breakpoint. A breakpoint may already exist at this address."));
+        return;
+      }
+
+      if (type == CPU::BreakpointType::Execute)
+        m_code_model->setBreakpointState(address, true);
+
+      refreshBreakpointList(bps);
+    });
+  });
+}
+
+void DebuggerWindow::removeBreakpoint(CPU::BreakpointType type, u32 address)
+{
+  Host::RunOnCPUThread([this, address, type]() {
+    const bool result = CPU::RemoveBreakpoint(type, address);
+    QtHost::RunOnUIThread([this, address, type, result, bps = CPU::CopyBreakpointList()]() {
+      if (!result)
+      {
+        QMessageBox::critical(this, windowTitle(), tr("Failed to remove breakpoint. This breakpoint may not exist."));
+        return;
+      }
+
+      if (type == CPU::BreakpointType::Execute)
+        m_code_model->setBreakpointState(address, false);
+
+      refreshBreakpointList(bps);
+    });
+  });
+}

src/duckstation-qt/debuggerwindow.h

@@ -55,6 +55,7 @@ private Q_SLOTS:
   void onAddBreakpointTriggered();
   void onToggleBreakpointTriggered();
   void onClearBreakpointsTriggered();
+  void onBreakpointListContextMenuRequested();
   void onStepIntoActionTriggered();
   void onStepOverActionTriggered();
   void onStepOutActionTriggered();
@@ -78,6 +79,8 @@ private:
   bool scrollToMemoryAddress(VirtualMemoryAddress address);
   void refreshBreakpointList();
   void refreshBreakpointList(const CPU::BreakpointList& bps);
+  void addBreakpoint(CPU::BreakpointType type, u32 address);
+  void removeBreakpoint(CPU::BreakpointType type, u32 address);
 
   Ui::DebuggerWindow m_ui;
 

src/duckstation-qt/debuggerwindow.ui

@@ -6,7 +6,7 @@
    <rect>
     <x>0</x>
     <y>0</y>
-    <width>1210</width>
+    <width>1270</width>
     <height>800</height>
    </rect>
   </property>
@@ -233,7 +233,7 @@
    </property>
    <property name="maximumSize">
     <size>
-     <width>200</width>
+     <width>290</width>
      <height>524287</height>
     </size>
    </property>
@@ -269,6 +269,11 @@
       <string>Address</string>
      </property>
     </column>
+    <column>
+     <property name="text">
+      <string>Type</string>
+     </property>
+    </column>
     <column>
      <property name="text">
       <string>Hit Count</string>

src/duckstation-qt/duckstation-qt.vcxproj

@@ -332,6 +332,9 @@
     <QtUi Include="setupwizarddialog.ui">
       <FileType>Document</FileType>
     </QtUi>
+    <QtUi Include="debuggeraddbreakpointdialog.ui">
+      <FileType>Document</FileType>
+    </QtUi>
     <None Include="translations\duckstation-qt_es-es.ts" />
     <None Include="translations\duckstation-qt_tr.ts" />
   </ItemGroup>

src/duckstation-qt/duckstation-qt.vcxproj.filters

@@ -292,6 +292,7 @@
     <QtUi Include="coverdownloaddialog.ui" />
     <QtUi Include="setupwizarddialog.ui" />
     <QtUi Include="controllerledsettingsdialog.ui" />
+    <QtUi Include="debuggeraddbreakpointdialog.ui" />
   </ItemGroup>
   <ItemGroup>
     <Natvis Include="qt5.natvis" />