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([]() {
-    SetState(SConfig::GetInstance().bBootToPause ? State::Paused : State::Running);
+  QueueHostJob([force_paused]() {
+    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());
-    gdb_break();
+    GDBStub::InitLocal(_CoreParameter.gdb_socket.data());
+    CPUSetInitialExecutionState(true);
   }
   else
 #endif
       if (_CoreParameter.iGDBPort > 0)
   {
-    gdb_init(_CoreParameter.iGDBPort);
-    // break at next instruction (the first instruction)
-    gdb_break();
+    GDBStub::Init(_CoreParameter.iGDBPort);
+    CPUSetInitialExecutionState(true);
+  }
+  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();