Merge pull request #10142 from aldelaro5/gdb-stub-rework

Rework the entire logic of the GDB stub

CMakeLists.txt

@@ -59,7 +59,6 @@ option(ENCODE_FRAMEDUMPS "Encode framedumps in AVI format" ON)
 
 option(ENABLE_GPROF "Enable gprof profiling (must be using Debug build)" OFF)
 option(FASTLOG "Enable all logs" OFF)
-option(GDBSTUB "Enable gdb stub for remote debugging." ON)
 option(OPROFILING "Enable profiling" OFF)
 
 # TODO: Add DSPSpy
@@ -389,10 +388,6 @@ if(FASTLOG)
   add_definitions(-DDEBUGFAST)
 endif()
 
-if(GDBSTUB)
-  add_definitions(-DUSE_GDBSTUB)
-endif()
-
 if(ENABLE_VTUNE)
   set(VTUNE_DIR "/opt/intel/vtune_amplifier")
   add_definitions(-DUSE_VTUNE)

Source/Core/Core/CMakeLists.txt

@@ -452,6 +452,8 @@ add_library(core
   PowerPC/JitCommon/JitCache.h
   PowerPC/JitInterface.cpp
   PowerPC/JitInterface.h
+  PowerPC/GDBStub.cpp
+  PowerPC/GDBStub.h
   PowerPC/MMU.cpp
   PowerPC/MMU.h
   PowerPC/PowerPC.cpp
@@ -698,13 +700,6 @@ if(TARGET Hidapi::Hidapi)
   target_compile_definitions(core PRIVATE -DHAVE_HIDAPI=1)
 endif()
 
-if(GDBSTUB)
-  target_sources(core PRIVATE
-    PowerPC/GDBStub.cpp
-    PowerPC/GDBStub.h
-  )
-endif()
-
 if(UNIX)
   target_sources(core PRIVATE
     MemoryWatcher.cpp

Source/Core/Core/ConfigManager.cpp

@@ -131,12 +131,10 @@ void SConfig::SaveGeneralSettings(IniFile& ini)
 
   general->Set("WirelessMac", m_WirelessMac);
 
-#ifdef USE_GDBSTUB
 #ifndef _WIN32
   general->Set("GDBSocket", gdb_socket);
 #endif
   general->Set("GDBPort", iGDBPort);
-#endif
 }
 
 void SConfig::SaveInterfaceSettings(IniFile& ini)
@@ -349,12 +347,10 @@ void SConfig::LoadGeneralSettings(IniFile& ini)
 
   general->Get("ShowLag", &m_ShowLag, false);
   general->Get("ShowFrameCount", &m_ShowFrameCount, false);
-#ifdef USE_GDBSTUB
 #ifndef _WIN32
   general->Get("GDBSocket", &gdb_socket, "");
 #endif
   general->Get("GDBPort", &(iGDBPort), -1);
-#endif
 
   m_ISOFolder.clear();
   int numISOPaths;
@@ -689,11 +685,9 @@ void SConfig::LoadDefaults()
   bAutomaticStart = false;
   bBootToPause = false;
 
-#ifdef USE_GDBSTUB
   iGDBPort = -1;
 #ifndef _WIN32
   gdb_socket = "";
-#endif
 #endif
 
   cpu_core = PowerPC::DefaultCPUCore();

Source/Core/Core/ConfigManager.h

@@ -70,11 +70,9 @@ struct SConfig
 
   // Settings
   bool bEnableDebugging = false;
-#ifdef USE_GDBSTUB
   int iGDBPort;
 #ifndef _WIN32
   std::string gdb_socket;
-#endif
 #endif
   bool bAutomaticStart = false;
   bool bBootToPause = false;

Source/Core/Core/Core.cpp

@@ -63,15 +63,12 @@
 #include "Core/NetPlayClient.h"
 #include "Core/NetPlayProto.h"
 #include "Core/PatchEngine.h"
+#include "Core/PowerPC/GDBStub.h"
 #include "Core/PowerPC/JitInterface.h"
 #include "Core/PowerPC/PowerPC.h"
 #include "Core/State.h"
 #include "Core/WiiRoot.h"
 
-#ifdef USE_GDBSTUB
-#include "Core/PowerPC/GDBStub.h"
-#endif
-
 #ifdef USE_MEMORYWATCHER
 #include "Core/MemoryWatcher.h"
 #endif
@@ -316,12 +313,13 @@ void UndeclareAsCPUThread()
 }
 
 // For the CPU Thread only.
-static void CPUSetInitialExecutionState()
+static void CPUSetInitialExecutionState(bool force_paused = false)
 {
   // The CPU starts in stepping state, and will wait until a new state is set before executing.
   // SetState must be called on the host thread, so we defer it for later.
-  QueueHostJob([]() {
+  QueueHostJob([force_paused]() {
-    SetState(SConfig::GetInstance().bBootToPause ? State::Paused : State::Running);
+    bool paused = SConfig::GetInstance().bBootToPause || force_paused;
+    SetState(paused ? State::Paused : State::Running);
     Host_UpdateDisasmDialog();
     Host_UpdateMainFrame();
     Host_Message(HostMessageID::WMUserCreate);
@@ -363,24 +361,23 @@ static void CpuThread(const std::optional<std::string>& savestate_path, bool del
   }
 
   s_is_started = true;
-  CPUSetInitialExecutionState();
-
-#ifdef USE_GDBSTUB
 #ifndef _WIN32
   if (!_CoreParameter.gdb_socket.empty())
   {
-    gdb_init_local(_CoreParameter.gdb_socket.data());
+    GDBStub::InitLocal(_CoreParameter.gdb_socket.data());
-    gdb_break();
+    CPUSetInitialExecutionState(true);
   }
   else
 #endif
       if (_CoreParameter.iGDBPort > 0)
   {
-    gdb_init(_CoreParameter.iGDBPort);
+    GDBStub::Init(_CoreParameter.iGDBPort);
-    // break at next instruction (the first instruction)
+    CPUSetInitialExecutionState(true);
-    gdb_break();
+  }
+  else
+  {
+    CPUSetInitialExecutionState();
   }
-#endif
 
   // Enter CPU run loop. When we leave it - we are done.
   CPU::Run();
@@ -393,6 +390,13 @@ static void CpuThread(const std::optional<std::string>& savestate_path, bool del
 
   if (_CoreParameter.bFastmem)
     EMM::UninstallExceptionHandler();
+
+  if (GDBStub::IsActive())
+  {
+    GDBStub::Deinit();
+    INFO_LOG_FMT(GDB_STUB, "Killed by CPU shutdown");
+    return;
+  }
 }
 
 static void FifoPlayerThread(const std::optional<std::string>& savestate_path,
@@ -653,11 +657,9 @@ static void EmuThread(std::unique_ptr<BootParameters> boot, WindowSystemInfo wsi
     cpuThreadFunc(savestate_path, delete_savestate);
   }
 
-#ifdef USE_GDBSTUB
   INFO_LOG_FMT(CONSOLE, "{}", StopMessage(true, "Stopping GDB ..."));
-  gdb_deinit();
+  GDBStub::Deinit();
   INFO_LOG_FMT(CONSOLE, "{}", StopMessage(true, "GDB stopped."));
-#endif
 }
 
 // Set or get the running state

Source/Core/Core/HW/CPU.cpp

@@ -12,6 +12,7 @@
 #include "Common/Event.h"
 #include "Core/Core.h"
 #include "Core/Host.h"
+#include "Core/PowerPC/GDBStub.h"
 #include "Core/PowerPC/PowerPC.h"
 #include "VideoCommon/Fifo.h"
 
@@ -96,6 +97,7 @@ void Run()
     s_state_cpu_cvar.wait(state_lock, [] { return !s_state_paused_and_locked; });
     ExecutePendingJobs(state_lock);
 
+    Common::Event gdb_step_sync_event;
     switch (s_state)
     {
     case State::Running:
@@ -129,8 +131,26 @@ void Run()
 
     case State::Stepping:
       // Wait for step command.
-      s_state_cpu_cvar.wait(state_lock, [&state_lock] {
+      s_state_cpu_cvar.wait(state_lock, [&state_lock, &gdb_step_sync_event] {
         ExecutePendingJobs(state_lock);
+        state_lock.unlock();
+        if (GDBStub::IsActive() && GDBStub::HasControl())
+        {
+          GDBStub::SendSignal(GDBStub::Signal::Sigtrap);
+          GDBStub::ProcessCommands(true);
+          // If we are still going to step, emulate the fact we just sent a step command
+          if (GDBStub::HasControl())
+          {
+            // Make sure the previous step by gdb was serviced
+            if (s_state_cpu_step_instruction_sync &&
+                s_state_cpu_step_instruction_sync != &gdb_step_sync_event)
+              s_state_cpu_step_instruction_sync->Set();
+
+            s_state_cpu_step_instruction = true;
+            s_state_cpu_step_instruction_sync = &gdb_step_sync_event;
+          }
+        }
+        state_lock.lock();
         return s_state_cpu_step_instruction || !IsStepping();
       });
       if (!IsStepping())
@@ -282,6 +302,12 @@ void Break()
   RunAdjacentSystems(false);
 }
 
+void Continue()
+{
+  CPU::EnableStepping(false);
+  Core::CallOnStateChangedCallbacks(Core::State::Running);
+}
+
 bool PauseAndLock(bool do_lock, bool unpause_on_unlock, bool control_adjacent)
 {
   // NOTE: This is protected by s_stepping_lock.

Source/Core/Core/HW/CPU.h

@@ -53,6 +53,9 @@ void EnableStepping(bool stepping);
 //   should not be used by the Host.
 void Break();
 
+// This should only be called from the CPU thread
+void Continue();
+
 // Shorthand for GetState() == State::Stepping.
 // WARNING: State::PowerDown will return false, not just State::Running.
 bool IsStepping();

Source/Core/Core/PowerPC/GDBStub.h

@@ -6,38 +6,23 @@
 #pragma once
 
 #include "Common/CommonTypes.h"
+#include "Core/CoreTiming.h"
 
-#ifndef MSG_WAITALL
+namespace GDBStub
-#define MSG_WAITALL (8)
-#endif
-
-typedef enum
 {
-  GDB_SIGTRAP = 5,
+enum class Signal
-  GDB_SIGTERM = 15,
-} gdb_signals;
-
-typedef enum
 {
-  GDB_BP_TYPE_NONE = 0,
+  Sigtrap = 5,
-  GDB_BP_TYPE_X,
+  Sigterm = 15,
-  GDB_BP_TYPE_R,
+};
-  GDB_BP_TYPE_W,
-  GDB_BP_TYPE_A
-} gdb_bp_type;
 
-void gdb_init(u32 port);
+void Init(u32 port);
-void gdb_init_local(const char* socket);
+void InitLocal(const char* socket);
-void gdb_deinit();
+void Deinit();
-bool gdb_active();
+bool IsActive();
-void gdb_break();
+bool HasControl();
+void TakeControl();
 
-void gdb_handle_exception();
+void ProcessCommands(bool loop_until_continue);
-int gdb_signal(u32 signal);
+void SendSignal(Signal signal);
-
+}  // namespace GDBStub
-int gdb_bp_x(u32 addr);
-int gdb_bp_r(u32 addr);
-int gdb_bp_w(u32 addr);
-int gdb_bp_a(u32 addr);
-
-bool gdb_add_bp(u32 type, u32 addr, u32 len);

Source/Core/Core/PowerPC/Interpreter/Interpreter.cpp

@@ -20,15 +20,12 @@
 #include "Core/HLE/HLE.h"
 #include "Core/HW/CPU.h"
 #include "Core/Host.h"
+#include "Core/PowerPC/GDBStub.h"
 #include "Core/PowerPC/Interpreter/ExceptionUtils.h"
 #include "Core/PowerPC/MMU.h"
 #include "Core/PowerPC/PPCTables.h"
 #include "Core/PowerPC/PowerPC.h"
 
-#ifdef USE_GDBSTUB
-#include "Core/PowerPC/GDBStub.h"
-#endif
-
 namespace
 {
 u32 last_pc;
@@ -152,16 +149,6 @@ int Interpreter::SingleStepInner()
     return PPCTables::GetOpInfo(m_prev_inst)->numCycles;
   }
 
-#ifdef USE_GDBSTUB
-  if (gdb_active() && gdb_bp_x(PC))
-  {
-    Host_UpdateDisasmDialog();
-
-    gdb_signal(GDB_SIGTRAP);
-    gdb_handle_exception();
-  }
-#endif
-
   NPC = PC + sizeof(UGeckoInstruction);
   m_prev_inst.hex = PowerPC::Read_Opcode(PC);
 
@@ -306,6 +293,8 @@ void Interpreter::Run()
 #endif
             INFO_LOG_FMT(POWERPC, "Hit Breakpoint - {:08x}", PC);
             CPU::Break();
+            if (GDBStub::IsActive())
+              GDBStub::TakeControl();
             if (PowerPC::breakpoints.IsTempBreakPoint(PC))
               PowerPC::breakpoints.Remove(PC);
 

Source/Core/Core/PowerPC/MMU.cpp

@@ -18,15 +18,12 @@
 #include "Core/HW/MMIO.h"
 #include "Core/HW/Memmap.h"
 #include "Core/HW/ProcessorInterface.h"
+#include "Core/PowerPC/GDBStub.h"
 #include "Core/PowerPC/JitInterface.h"
 #include "Core/PowerPC/PowerPC.h"
 
 #include "VideoCommon/VideoBackendBase.h"
 
-#ifdef USE_GDBSTUB
-#include "Core/PowerPC/GDBStub.h"
-#endif
-
 namespace PowerPC
 {
 // EFB RE
@@ -522,6 +519,9 @@ static void Memcheck(u32 address, u64 var, bool write, size_t size)
 
   CPU::Break();
 
+  if (GDBStub::IsActive())
+    GDBStub::TakeControl();
+
   // Fake a DSI so that all the code that tests for it in order to skip
   // the rest of the instruction will apply.  (This means that
   // watchpoints will stop the emulator before the offending load/store,

Source/Core/Core/PowerPC/PowerPC.cpp

@@ -25,6 +25,7 @@
 #include "Core/HW/SystemTimers.h"
 #include "Core/Host.h"
 #include "Core/PowerPC/CPUCoreBase.h"
+#include "Core/PowerPC/GDBStub.h"
 #include "Core/PowerPC/Interpreter/Interpreter.h"
 #include "Core/PowerPC/JitInterface.h"
 #include "Core/PowerPC/MMU.h"
@@ -611,7 +612,11 @@ void CheckBreakPoints()
     return;
 
   if (PowerPC::breakpoints.IsBreakPointBreakOnHit(PC))
+  {
     CPU::Break();
+    if (GDBStub::IsActive())
+      GDBStub::TakeControl();
+  }
   if (PowerPC::breakpoints.IsBreakPointLogOnHit(PC))
   {
     NOTICE_LOG_FMT(MEMMAP,