CPU: Make single step go through the "normal" execution path

That way it exits and re-enters the dynarec as expected.

src/core/cpu_core.cpp

@@ -1,12 +1,8 @@
-// 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 "cpu_core.h"
 #include "bus.h"
-#include "common/align.h"
-#include "common/fastjmp.h"
-#include "common/file_system.h"
-#include "common/log.h"
 #include "cpu_code_cache_private.h"
 #include "cpu_core_private.h"
 #include "cpu_disasm.h"
@@ -18,7 +14,14 @@
 #include "settings.h"
 #include "system.h"
 #include "timing_event.h"
+
 #include "util/state_wrapper.h"
+
+#include "common/align.h"
+#include "common/fastjmp.h"
+#include "common/file_system.h"
+#include "common/log.h"
+
 #include <cstdio>
 
 Log_SetChannel(CPU::Core);
@@ -45,7 +48,7 @@ static bool IsCop0ExecutionBreakpointUnmasked();
 static void Cop0ExecutionBreakpointCheck();
 template<MemoryAccessType type>
 static void Cop0DataBreakpointCheck(VirtualMemoryAddress address);
-static bool BreakpointCheck();
+static void BreakpointCheck();
 
 #ifdef _DEBUG
 static void TracePrintInstruction();
@@ -94,8 +97,7 @@ static constexpr u32 INVALID_BREAKPOINT_PC = UINT32_C(0xFFFFFFFF);
 static std::vector<Breakpoint> s_breakpoints;
 static u32 s_breakpoint_counter = 1;
 static u32 s_last_breakpoint_check_pc = INVALID_BREAKPOINT_PC;
-static bool s_single_step = false;
+static u8 s_single_step = 0; // 0 - off, 1 - executing one, 2 - done
-static bool s_single_step_done = false;
 } // namespace CPU
 
 bool CPU::IsTraceEnabled()
@@ -157,7 +159,7 @@ void CPU::Initialize()
   s_breakpoints.clear();
   s_breakpoint_counter = 1;
   s_last_breakpoint_check_pc = INVALID_BREAKPOINT_PC;
-  s_single_step = false;
+  s_single_step = 0;
 
   UpdateMemoryPointers();
 
@@ -1932,7 +1934,7 @@ void CPU::DispatchInterrupt()
 
 bool CPU::UpdateDebugDispatcherFlag()
 {
-  const bool has_any_breakpoints = !s_breakpoints.empty();
+  const bool has_any_breakpoints = !s_breakpoints.empty() || (s_single_step != 0);
 
   // TODO: cop0 breakpoints
   const auto& dcic = g_state.cop0_regs.dcic;
@@ -2120,29 +2122,18 @@ bool CPU::AddStepOutBreakpoint(u32 max_instructions_to_search)
   return false;
 }
 
-bool CPU::BreakpointCheck()
+ALWAYS_INLINE_RELEASE void CPU::BreakpointCheck()
 {
   const u32 pc = g_state.pc;
 
-  // single step - we want to break out after this instruction, so set a pending exit
+  if (pc == s_last_breakpoint_check_pc && !s_single_step) [[unlikely]]
-  // the bp check happens just before execution, so this is fine
-  if (s_single_step)
-  {
-    if (s_single_step_done)
-      ExitExecution();
-    else
-      s_single_step_done = true;
-
-    s_last_breakpoint_check_pc = pc;
-    return false;
-  }
-
-  if (pc == s_last_breakpoint_check_pc)
   {
     // we don't want to trigger the same breakpoint which just paused us repeatedly.
-    return false;
+    return;
   }
 
+  s_last_breakpoint_check_pc = pc;
+
   u32 count = static_cast<u32>(s_breakpoints.size());
   for (u32 i = 0; i < count;)
   {
@@ -2186,12 +2177,31 @@ bool CPU::BreakpointCheck()
         i++;
       }
 
+      s_single_step = 0;
       ExitExecution();
     }
   }
 
-  s_last_breakpoint_check_pc = pc;
+  // single step - we want to break out after this instruction, so set a pending exit
-  return System::IsPaused();
+  // the bp check happens just before execution, so this is fine
+  if (s_single_step != 0) [[unlikely]]
+  {
+    // not resetting this will break double single stepping, because we broke on the next instruction.
+    s_last_breakpoint_check_pc = INVALID_BREAKPOINT_PC;
+
+    if (s_single_step == 2)
+    {
+      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;
+  }
 }
 
 template<PGXPMode pgxp_mode, bool debug>
@@ -2206,12 +2216,7 @@ template<PGXPMode pgxp_mode, bool debug>
       if constexpr (debug)
       {
         Cop0ExecutionBreakpointCheck();
-
+        BreakpointCheck();
-        if (BreakpointCheck())
-        {
-          // continue is measurably faster than break on msvc for some reason
-          continue;
-        }
       }
 
       g_state.pending_ticks++;
@@ -2280,8 +2285,17 @@ void CPU::Execute()
   if (fastjmp_set(&s_jmp_buf) != 0)
   {
     // Before we return, set npc to pc so that we can switch from recs to int.
+    // We'll also need to fetch the next instruction to execute.
     if (exec_mode != CPUExecutionMode::Interpreter && !use_debug_dispatcher)
-      g_state.npc = g_state.pc;
+    {
+      if (!SafeReadInstruction(g_state.pc, &g_state.next_instruction.bits)) [[unlikely]]
+      {
+        g_state.next_instruction.bits = 0;
+        Log_ErrorFmt("Failed to read current instruction from 0x{:08X}", g_state.pc);
+      }
+
+      g_state.npc = g_state.pc + sizeof(Instruction);
+    }
 
     return;
   }
@@ -2319,13 +2333,11 @@ void CPU::Execute()
   }
 }
 
-void CPU::SingleStep()
+void CPU::SetSingleStepFlag()
 {
-  s_single_step = true;
+  s_single_step = 1;
-  s_single_step_done = false;
+  if (UpdateDebugDispatcherFlag())
-  if (fastjmp_set(&s_jmp_buf) == 0)
+    System::InterruptExecution();
-    ExecuteDebug();
-  Host::ReportFormattedDebuggerMessage("Stepped to 0x%08X.", g_state.pc);
 }
 
 template<PGXPMode pgxp_mode>

src/core/cpu_core.h

@@ -1,11 +1,14 @@
-// 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 "common/bitfield.h"
+
 #include "cpu_types.h"
 #include "gte_types.h"
 #include "types.h"
+
+#include "common/bitfield.h"
+
 #include <array>
 #include <optional>
 #include <string>
@@ -127,7 +130,6 @@ void ExecutionModeChanged();
 
 /// Executes interpreter loop.
 void Execute();
-void SingleStep();
 
 // Forces an early exit from the CPU dispatcher.
 void ExitExecution();
@@ -214,6 +216,7 @@ bool RemoveBreakpoint(VirtualMemoryAddress address);
 void ClearBreakpoints();
 bool AddStepOverBreakpoint();
 bool AddStepOutBreakpoint(u32 max_instructions_to_search = 1000);
+void SetSingleStepFlag();
 
 extern bool TRACE_EXECUTION;
 

src/core/system.cpp

@@ -1932,19 +1932,11 @@ void System::Throttle()
 
 void System::SingleStepCPU()
 {
-  s_frame_timer.Reset();
+  CPU::SetSingleStepFlag();
-  s_system_executing = true;
 
-  g_gpu->RestoreDeviceContext();
+  // If this gets called when the system is executing, we're not going to end up here..
-
+  if (IsPaused())
-  CPU::SingleStep();
+    PauseSystem(false);
-
-  g_gpu->FlushRender();
-  SPU::GeneratePendingSamples();
-
-  InvalidateDisplay();
-
-  s_system_executing = false;
 }
 
 void System::IncrementInternalFrameNumber()