From 79cea44c80e19b9dd426cfc3bcbbf476c716ef3c Mon Sep 17 00:00:00 2001 From: r-log Date: Sat, 4 Jul 2026 13:24:47 +0200 Subject: [PATCH] [Debug] Confine GDB breakpoint stops to the world thread Breakpoint call sites in WorldSocket (ACE network threads) and DatabaseMysql (DB worker thread) could drive EnterStop off the world thread, racing the unguarded RSP state and never actually pausing the world. Latch the world thread id on the first OnWorldUpdate tick and reject EnterBreak from any other thread (the hit is already counted). The same foreign sites also reached GdbBp::Matches / Hit before that guard, racing g_entries (std::vector) against Arm/Disarm on the world thread and incrementing a non-atomic g_hits. Serialise g_entries with a mutex and make g_hits atomic; correct the stale "needs no locking" note. --- src/game/Debug/GdbServer/GdbBreakpoints.cpp | 16 ++++++++++++-- src/game/Debug/GdbServer/GdbBreakpoints.h | 7 +++--- src/game/Debug/GdbServer/GdbServer.cpp | 24 ++++++++++++++++++++- src/game/Debug/GdbServer/GdbServer.h | 6 ++++++ 4 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/game/Debug/GdbServer/GdbBreakpoints.cpp b/src/game/Debug/GdbServer/GdbBreakpoints.cpp index 3659f820e..190a7a8f0 100644 --- a/src/game/Debug/GdbServer/GdbBreakpoints.cpp +++ b/src/game/Debug/GdbServer/GdbBreakpoints.cpp @@ -32,8 +32,10 @@ #include "GdbMonitor.h" #include "GdbServer.h" +#include #include #include +#include #include #include @@ -50,8 +52,12 @@ namespace GdbBp uint64 filter; }; + // g_entries is mutated on the world thread (monitor commands) but + // also iterated by Matches from foreign threads (network/DB call + // sites), so every access must hold g_entriesLock. + std::mutex g_entriesLock; std::vector g_entries; - uint64 g_hits = 0; + std::atomic g_hits{0}; // Canonical lower-case name per Event, indexed by enum value. Keep in // lockstep with the GdbEvent enum in shared/Debug/GdbEvents.h. @@ -75,6 +81,7 @@ namespace GdbBp static_cast(Event::Count), "kEventNames must match the Event enum"); + // Callers must hold g_entriesLock; do not lock here (self-deadlock). void Recount() { uint64 mask = 0; @@ -93,6 +100,7 @@ namespace GdbBp bool Matches(Event e, uint64 detail) { + std::lock_guard lock(g_entriesLock); for (const Entry& entry : g_entries) { if (entry.ev == e && (entry.filter == 0 || entry.filter == detail)) @@ -105,6 +113,7 @@ namespace GdbBp bool Arm(Event e, uint64 filter) { + std::lock_guard lock(g_entriesLock); for (const Entry& entry : g_entries) { if (entry.ev == e && entry.filter == filter) @@ -119,6 +128,7 @@ namespace GdbBp bool Disarm(Event e, uint64 filter) { + std::lock_guard lock(g_entriesLock); for (size_t i = 0; i < g_entries.size(); ++i) { if (g_entries[i].ev == e && g_entries[i].filter == filter) @@ -133,12 +143,14 @@ namespace GdbBp void DisarmAll() { + std::lock_guard lock(g_entriesLock); g_entries.clear(); Recount(); } void List(GdbMon::MonitorWriter& out) { + std::lock_guard lock(g_entriesLock); if (g_entries.empty()) { out.Str(" (no breakpoints armed)\n"); @@ -159,7 +171,7 @@ namespace GdbBp out.Line(); } out.Str(" hits="); - out.U64(g_hits); + out.U64(g_hits.load()); out.Line(); } diff --git a/src/game/Debug/GdbServer/GdbBreakpoints.h b/src/game/Debug/GdbServer/GdbBreakpoints.h index 3f737db91..6275c8af8 100644 --- a/src/game/Debug/GdbServer/GdbBreakpoints.h +++ b/src/game/Debug/GdbServer/GdbBreakpoints.h @@ -50,9 +50,10 @@ namespace GdbMon { class MonitorWriter; } * inspect game state via the `monitor` surface, then resume. * * The hot-path guard is a single relaxed atomic load of a per-event bitmask, - * so call sites cost effectively nothing when their event is not armed. All - * arming and matching happens on the world thread (monitor dispatch + game - * code), so the registry needs no locking. + * so call sites cost effectively nothing when their event is not armed. + * Arming happens on the world thread (monitor dispatch), but matching and + * hit-counting can also run from foreign threads (network/DB call sites), so + * the registry is serialised with a mutex and the hit counter is atomic. */ namespace GdbBp { diff --git a/src/game/Debug/GdbServer/GdbServer.cpp b/src/game/Debug/GdbServer/GdbServer.cpp index db216a224..1f42d7c63 100644 --- a/src/game/Debug/GdbServer/GdbServer.cpp +++ b/src/game/Debug/GdbServer/GdbServer.cpp @@ -311,10 +311,24 @@ void GdbServer::EnterStop(const char* reason) void GdbServer::EnterBreak(const char* reason) { + if (!m_enabled) + { + return; + } + // World-thread-only: EnterStop mutates unguarded RSP state and pauses + // the world by blocking this very thread. A breakpoint raised from the + // DB worker or an ACE network thread must not enter it; the hit was + // already counted by the caller, so just return. The acquire load pairs + // with the release store in OnWorldUpdate. + if (!m_worldThreadLatched.load(std::memory_order_acquire) || + std::this_thread::get_id() != m_worldThreadId) + { + return; + } // Only break when a debugger is attached — otherwise no one could resume // the server and the world would hang. Never nest stops (a breakpoint may // fire from a command executed while already stopped). - if (!m_enabled || m_inStop || !DebuggerAttached()) + if (m_inStop || !DebuggerAttached()) { return; } @@ -328,6 +342,14 @@ void GdbServer::OnWorldUpdate() return; } + // This is the world thread: latch its identity on the first tick so + // EnterBreak can reject breakpoints raised from foreign threads. + if (!m_worldThreadLatched.load(std::memory_order_acquire)) + { + m_worldThreadId = std::this_thread::get_id(); + m_worldThreadLatched.store(true, std::memory_order_release); + } + if (m_resetPending.exchange(false)) { GdbRsp::ResetSession(); diff --git a/src/game/Debug/GdbServer/GdbServer.h b/src/game/Debug/GdbServer/GdbServer.h index 35d235046..5bd4dab7f 100644 --- a/src/game/Debug/GdbServer/GdbServer.h +++ b/src/game/Debug/GdbServer/GdbServer.h @@ -36,6 +36,7 @@ #include #include #include +#include #include /** @@ -136,6 +137,11 @@ class GdbServer // True while inside the stop loop; blocks a breakpoint that fires from // a command run during the stop from nesting another stop. bool m_inStop = false; + // World-thread identity, latched on the first OnWorldUpdate tick so + // EnterBreak can reject breakpoints raised from foreign threads. The + // id is written once before the latch flag is released. + std::atomic m_worldThreadLatched{false}; + std::thread::id m_worldThreadId; // Set by AttachRsp (network thread); consumed by OnWorldUpdate so all // RSP engine state mutation happens on the world thread. std::atomic m_resetPending{false};