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();