From 81511e7a08b2f8269acae4279cbb58908584ddd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 17 Aug 2026 00:09:41 +0200 Subject: [PATCH] Fix deadlock between the Win32 debugger's paint handlers and the CPU thread Reported hang: the CPU thread held g_frameMutex (NativeFrame) and blocked on g_shutdownLock inside a queued memory.read, while the GUI thread held g_shutdownLock (CtrlMemView::onPaint) and blocked on g_frameMutex. Textbook ABBA. The CPU thread's order is structural - NativeFrame wraps everything below it in g_frameMutex, and both Core_ProcessCPUQueue() and runImDebugger() -> DisassembleRange() lock memory from under there - so the GUI side is the one that has to match. Swaps the three handlers that had it backwards (CtrlMemView::onPaint, CtrlDisAsmView::onPaint, CtrlStackTraceView:: loadStackTrace) to take g_frameMutex first. They already took both locks, so this is ordering only, and g_shutdownLock is recursive so nesting is fine. Also drops the Memory::MemoryInitedLock from the WebSocket LockMemory(), which is what made the CPU thread want that lock in the first place. It was guarding against another thread tearing down the memory system, but that doesn't happen: Memory::Shutdown() is only reached via CPU_Shutdown() <- PSP_Shutdown(), whose callers all run on the CPU thread, and Memory::Reinit() runs from Memory::DoState() on savestate load, likewise. WebSocket.cpp additionally holds lifecycleLock across the whole handler and takes it on STOPPING. Note this second part isn't sufficient on its own - ImMemView's copy-disassembly path also locks memory from inside the frame span - which is why the ordering fix is the real one. Not removing Memory::Lock() from the Win32 paint handlers: teardown isn't fully inside the g_frameMutex span yet. EmuScreen::render()'s PSP_Shutdown() is, but the ones in EmuScreen::sendMessage() (game reset, loading a new game) run from g_screenManager->sendMessage(), above where NativeFrame takes the guard. Closing that is the prerequisite, and is left for later. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9 --- Core/Debugger/WebSocket/MemorySubscriber.cpp | 11 ++++------- Windows/Debugger/CtrlDisAsmView.cpp | 7 ++++--- Windows/Debugger/CtrlMemView.cpp | 5 ++++- Windows/Debugger/Debugger_Lists.cpp | 9 +++++---- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/Core/Debugger/WebSocket/MemorySubscriber.cpp b/Core/Debugger/WebSocket/MemorySubscriber.cpp index 7854ea87f0..7b260ca2a7 100644 --- a/Core/Debugger/WebSocket/MemorySubscriber.cpp +++ b/Core/Debugger/WebSocket/MemorySubscriber.cpp @@ -53,7 +53,6 @@ struct AutoDisabledReplacements { AutoDisabledReplacements &operator =(const AutoDisabledReplacements &) = delete; ~AutoDisabledReplacements(); - Memory::MemoryInitedLock *lock = nullptr; std::map replacements; std::vector emuhacks; bool saved = false; @@ -64,12 +63,13 @@ struct AutoDisabledReplacements { // by the time this is called, so nothing else can be concurrently executing MIPS code or touching the // JIT's emuhack ops on this thread while we hold onto them below. // +// Deliberately does NOT take a Memory::MemoryInitedLock: memory teardown only ever happens on the +// CPU thread too, so there's nothing to guard against, and taking it here deadlocked against the +// Win32 debugger's paint handlers. See the lock ordering section in AGENTS.md. +// // Important: Only use keepReplacements=false when reading, not writing. static AutoDisabledReplacements LockMemory(bool keepReplacements) { AutoDisabledReplacements result; - // This still guards against a *different* thread (not the CPU thread) tearing down or - // reinitializing the memory system underneath us, e.g. during shutdown. - result.lock = new Memory::MemoryInitedLock(); if (!keepReplacements) { result.saved = true; // Okay, save so we can restore later. @@ -82,8 +82,6 @@ static AutoDisabledReplacements LockMemory(bool keepReplacements) { } AutoDisabledReplacements::AutoDisabledReplacements(AutoDisabledReplacements &&other) { - lock = other.lock; - other.lock = nullptr; replacements = std::move(other.replacements); emuhacks = std::move(other.emuhacks); saved = other.saved; @@ -96,7 +94,6 @@ AutoDisabledReplacements::~AutoDisabledReplacements() { MIPSComp::jit->RestoreSavedEmuHackOps(emuhacks); RestoreSavedReplacements(replacements); } - delete lock; } // Read a byte from memory (memory.read_u8) diff --git a/Windows/Debugger/CtrlDisAsmView.cpp b/Windows/Debugger/CtrlDisAsmView.cpp index fc0c4872f9..219f19626e 100644 --- a/Windows/Debugger/CtrlDisAsmView.cpp +++ b/Windows/Debugger/CtrlDisAsmView.cpp @@ -457,13 +457,14 @@ void CtrlDisAsmView::drawArguments(HDC hdc, const DisassemblyLineInfo &line, int void CtrlDisAsmView::onPaint(WPARAM wParam, LPARAM lParam) { - Memory::MemoryInitedLock memLock = Memory::Lock(); - if (!debugger->isAlive() || Achievements::HardcoreModeActive()) return; - // Reading live disassembly/symbol/breakpoint state here on the GUI thread would otherwise race // with the CPU thread - hold g_frameMutex for the duration of the read, which NativeFrame() // also holds while it's actually touching that state. See g_frameMutex in Core.h. + // + // g_frameMutex first, then Memory::Lock() - never the other way around. See CtrlMemView::onPaint. std::lock_guard frameGuard(g_frameMutex); + Memory::MemoryInitedLock memLock = Memory::Lock(); + if (!debugger->isAlive() || Achievements::HardcoreModeActive()) return; PAINTSTRUCT ps; HDC actualHdc = BeginPaint(wnd, &ps); diff --git a/Windows/Debugger/CtrlMemView.cpp b/Windows/Debugger/CtrlMemView.cpp index e249710f51..84cc271500 100644 --- a/Windows/Debugger/CtrlMemView.cpp +++ b/Windows/Debugger/CtrlMemView.cpp @@ -181,11 +181,14 @@ void CtrlMemView::onPaint(WPARAM wParam, LPARAM lParam) { if (Achievements::HardcoreModeActive()) return; - Memory::MemoryInitedLock memLock = Memory::Lock(); // Reading live memory/tracking state here on the GUI thread would otherwise race with the CPU // thread - hold g_frameMutex for the duration of the read, which NativeFrame() also holds // while it's actually touching that state. See g_frameMutex in Core.h. + // + // g_frameMutex first, then Memory::Lock() - never the other way around. The CPU thread has no + // choice but that order (NativeFrame wraps everything below it), so this side has to match. std::lock_guard frameGuard(g_frameMutex); + Memory::MemoryInitedLock memLock = Memory::Lock(); // draw to a bitmap for double buffering PAINTSTRUCT ps; diff --git a/Windows/Debugger/Debugger_Lists.cpp b/Windows/Debugger/Debugger_Lists.cpp index bbaaa97a2b..7c8bcefa96 100644 --- a/Windows/Debugger/Debugger_Lists.cpp +++ b/Windows/Debugger/Debugger_Lists.cpp @@ -743,14 +743,15 @@ void CtrlStackTraceView::OnDoubleClick(int itemIndex, int column) } void CtrlStackTraceView::loadStackTrace() { - Memory::MemoryInitedLock memLock = Memory::Lock(); - if (!PSP_IsInited()) - return; - // Reading live thread/register/stack state here on the GUI thread would otherwise race with // the CPU thread - hold g_frameMutex for the duration of the read, which NativeFrame() also // holds while it's actually touching that state. See g_frameMutex in Core.h. + // + // g_frameMutex first, then Memory::Lock() - never the other way around. See CtrlMemView::onPaint. std::lock_guard frameGuard(g_frameMutex); + Memory::MemoryInitedLock memLock = Memory::Lock(); + if (!PSP_IsInited()) + return; std::vector threads = GetThreadsInfo();