Document the debugger threading model in AGENTS.md

This commit is contained in:
Henrik Rydgård committed 2026-08-08 17:22:12 +02:00
1 parent f72709feb0
commit a39efbdaec
1 file changed
+67
+67
View File
@@ -209,6 +209,73 @@ first - send `cpu.stepping` and `cpu.resume` to pause/unpause.
Alternatively use the headless build, Windows/{arch}/Debug/PPSSPPHeadless.exe or build/PPSSPPHeadless on CMake-based
platforms. Where arch is x64 or ARM64.
## Debugger threading model (Core_RunOnCPUThread / g_frameMutex)
CPU-thread-owned debugger state (`g_breakpoints`, `g_symbolMap`, `g_disassemblyManager`, registers,
memory, kernel threads) used to be touched directly from other threads (the WebSocket handler
thread, and the legacy Win32 debugger's message-pump thread) with no synchronization. Two
mechanisms now exist for doing this safely - pick based on whether you're mutating or just reading:
- **`Core_RunOnCPUThread(func)`** (`Core.h`/`Core.cpp`) - queues `func` to run on the CPU thread,
blocking the caller until it's done. Use for *mutations* (breakpoint add/remove, register/memory
writes, symbol map edits, stepping requests, thread wake/kill). Drained at the top of every
`Core_RunLoopUntil()` iteration, so it's reached whether the CPU is running or stepping/paused;
runs immediately if already called from the CPU thread. Two hard rules learned the hard way:
never put a modal Win32 dialog call (`MessageBox`, `DialogBoxParam`, `InputBox_GetString`,
`SomeDialog::exec()`) inside the queued lambda - it would block the CPU thread on user input, so
split the function into "read/decide", "show modal", "mutate" pieces instead. And never call
`SendMessage()` targeting one of your own GUI-thread windows from inside the lambda - the calling
GUI thread is blocked waiting on the CPU thread rather than pumping messages, so a cross-thread
`SendMessage()` back to it deadlocks; keep such calls outside the lambda instead.
- **`g_frameMutex`** (`Core.h`/`Core.cpp`) - a plain `std::mutex`, held by `NativeFrame()`
(`UI/NativeApp.cpp`) only across the span where it actually touches that state
(`g_breakpoints.Frame()` through `g_screenManager->render()` - where `Core_RunLoopUntil()`/actual
CPU stepping happens - through `runImDebugger`/`renderImDebugger`), not across input handling or
the present/frame-pacing waits. Use for *reads* invoked very frequently (`WM_PAINT`, a list
reload triggered on every debugger-state-changed notification) where routing through
`Core_RunOnCPUThread` would be too slow/heavy. The legacy Win32 debugger windows do this now -
see `CtrlRegisterList::onPaint`, `CtrlDisAsmView::onPaint`, `CtrlMemView::onPaint`,
`CtrlBreakpointList::reloadBreakpoints`, `CtrlThreadList::reloadThreads`, etc. in
`Windows/Debugger/*.cpp`.
Architecture fact that makes `g_frameMutex` correct: regardless of graphics backend, `NativeFrame()`
(and thus CPU emulation via `Core_RunLoopUntil()`, and the Dear ImGui debugger) always runs on the
*same* thread - see `Core/EmuThread.cpp`. When a backend needs its own thread for actual graphics
API calls (`GraphicsContext::NeedsSeparateEmuThread()` - true for OpenGL, SDL, headless, libretro,
Qt; false for D3D11/Vulkan on Windows), the *original* thread stays behind purely to pump
`graphicsContext->ThreadFrame()` (i.e. just executes queued graphics API calls), and a *newly
spawned* thread takes over `NativeFrame()`/game logic/CPU duty. So `UI/ImDebugger/*.cpp` is always
safe to read/write this state directly, without either mechanism - it's always on the same thread
as `Core_RunLoopUntil()`. The legacy Win32 debugger is different: its dialogs are pumped by the
*original* `WinMain` message-loop thread, which is a genuinely separate OS thread from whichever
thread ends up running `NativeFrame()`/CPU, regardless of backend (this split happens one level
above the `NeedsSeparateEmuThread()` branch) - that's the whole reason it needed this mechanism.
**Known gaps, found while auditing whether `BreakpointManager`'s/`SymbolMap`'s own internal mutexes
could finally be removed** (as of 2026-08-08, not yet addressed): `Windows/MainWindowMenu.cpp` and
`Windows/MainWindow.cpp` (outside `Windows/Debugger/`) have their own direct, unguarded
`g_symbolMap` mutations/reads (Load/Save/Clear Symbol Map menu actions, `getAllModules()` for a
module list) on the `WinMain` thread. `Qt/mainwindow.cpp` has the same pattern on the Qt UI thread
(Qt always sets `NeedsSeparateEmuThread() = true`, so it's a separate thread from the CPU there
too). `Windows/main.cpp` has an existing comment ("internal locking is performed here") explicitly
documenting current reliance on `SymbolMap`'s own mutex at its `SortSymbols()` call sites. Until
these are covered the same way, **`SymbolMap`'s internal locking can't be removed**.
**`BreakpointManager`'s internal locking looks fully redundant** after this session's work (every
touchpoint across the whole codebase was audited - WebSocket subscribers, `Windows/Debugger/*.cpp`,
`Core/Core.cpp`'s free-threaded `Core_Break`/`Core_Resume`, `UI/ImDebugger/*.cpp`, JIT/interpreter
backends, `Core/Debugger/MemBlockInfo.cpp` - and each is covered by one of the two mechanisms above
or is already on the CPU/NativeFrame thread) - but removal hasn't actually been attempted yet; do
it as its own careful, separately-tested step, not bundled with unrelated changes.
Painting-problem design history, in case a similar tradeoff comes up elsewhere: routing every paint
through `Core_RunOnCPUThread` was rejected as too slow for something invoked continuously. A
per-window snapshot/cache with a per-row-rechecked `Core_IsStepping()` guard was tried first and
worked, but still had a narrow TOCTOU race (the CPU could resume between the check and that row's
reads) and the per-row-recheck pattern itself wasn't liked. Settled on `g_frameMutex` instead -
simpler, and actually race-free rather than just lower-risk. `CtrlRegisterList` shows live values
always now, grayed out by color alone (not cached) while the core is running, since a
constantly-moving value isn't meaningful to read closely anyway.
## Debugging and breakpoint considerations
It might be worth trying the interpreter - all types of breakpoints are the most reliable with this CPU backend.