Legacy Win32 debugger: fix the painting problem with a frame-scoped mutex

Debugger windows (register list, disassembly view, memory view, breakpoint/
thread/module/stack lists, watch list) read CPU-thread-owned state directly
from the GUI thread's WM_PAINT/list-fill handlers, racing against the CPU
thread. Routing every read through Core_RunOnCPUThread would be too slow for
something invoked continuously on paint/list-refresh.

Add g_frameMutex (Core.h/Core.cpp), held by NativeFrame() only across the
span where it actually touches that state (running the CPU, processing
breakpoints, running the ImGui debugger) - not across input handling or the
present/frame-pacing waits. Debugger windows now hold the same mutex while
reading, giving synchronized reads without the round-trip cost of queuing
to the CPU thread.

CtrlRegisterList::onPaint() goes back to always reading live values (now
safe under the lock) and grays them out by color alone while the core is
running, rather than the earlier snapshot-caching approach.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hqm11k99viLfbJm2MkH4BH

Core/Core.cpp

@@ -129,6 +129,10 @@ static void Core_ProcessCPUQueue() {
 	g_cpuQueueCond.notify_all();
 }
 
+// See Core.h for the rationale. Held by NativeFrame() (in NativeApp.cpp) around the span where it
+// actually touches CPU-thread-owned debugger state.
+std::mutex g_frameMutex;
+
 // This is so that external threads can wait for the CPU to become inactive.
 static std::condition_variable m_InactiveCond;
 static std::mutex m_hInactiveMutex;

Core/Core.h

@@ -19,6 +19,7 @@
 
 #include <cstdint>
 #include <functional>
+#include <mutex>
 #include <string>
 #include <string_view>
 
@@ -164,6 +165,22 @@ void Core_RunLoopUntil(u64 globalticks);
 // even while it's fully running.
 void Core_RunOnCPUThread(std::function<void()> func);
 
+// Guards CPU-thread-owned debugger state (breakpoints, symbol map, registers, memory, etc.)
+// against concurrent unsynchronized reads from other threads' paint handlers.
+//
+// Held by NativeFrame() for the span where it actually touches that state: running the CPU
+// (Core_RunLoopUntil(), including draining Core_RunOnCPUThread()'s queue), processing breakpoints,
+// and running the ImGui debugger. Not held for the rest of NativeFrame (input handling, present/
+// vsync waits, frame pacing, etc).
+//
+// A paint handler on another thread (e.g. a legacy Win32 debugger window) that wants to read that
+// state directly - without the overhead/latency of routing through Core_RunOnCPUThread(), which
+// would be too heavy for something called on every WM_PAINT - should hold this lock for the
+// duration of the read instead. Since WM_PAINT only fires reactively rather than every frame, and
+// NativeFrame's locked span is normally just a couple of milliseconds, this should rarely block
+// for long.
+extern std::mutex g_frameMutex;
+
 extern volatile CoreState coreState;
 extern volatile bool coreStatePending;
 

UI/NativeApp.cpp

@@ -1257,29 +1257,37 @@ void NativeFrame(GraphicsContext *graphicsContext) {
 
 	g_requestManager.ProcessRequests();
 
-	g_breakpoints.Frame();
+	// Guards the span where we actually touch CPU-thread-owned debugger state (breakpoints,
+	// symbol map, registers, memory, etc.) against unsynchronized reads from other threads' paint
+	// handlers - see g_frameMutex in Core.h.
+	ScreenRenderFlags renderFlags = ScreenRenderFlags::NONE;
+	{
+		std::lock_guard<std::mutex> emuStateGuard(g_frameMutex);
 
-	// Apply the UIContext bounds as a 2D transformation matrix.
-	// NOTE: We compensate for the Y and Z conventions in the shaders, so we can use the same matrices in all backends.
-	Matrix4x4 ortho = ComputeOrthoMatrix(g_display.dp_xres, g_display.dp_yres, g_draw->GetDeviceCaps().coordConvention);
+		g_breakpoints.Frame();
 
-	// Can be overridden by sceDisplay which may pass true for the second argument.
-	g_frameTiming.ComputePresentMode(g_draw, false);
+		// Apply the UIContext bounds as a 2D transformation matrix.
+		// NOTE: We compensate for the Y and Z conventions in the shaders, so we can use the same matrices in all backends.
+		Matrix4x4 ortho = ComputeOrthoMatrix(g_display.dp_xres, g_display.dp_yres, g_draw->GetDeviceCaps().coordConvention);
 
-	ui_draw2d.PushDrawMatrix(ortho);
+		// Can be overridden by sceDisplay which may pass true for the second argument.
+		g_frameTiming.ComputePresentMode(g_draw, false);
 
-	g_screenManager->getUIContext()->SetTintSaturation(g_Config.fUITint, g_Config.fUISaturation);
+		ui_draw2d.PushDrawMatrix(ortho);
 
-	// All actual rendering (and also emulation) happens in this render() call.
-	ScreenRenderFlags renderFlags = g_screenManager->render();
-	if (g_screenManager->getUIContext()->Text()) {
-		g_screenManager->getUIContext()->Text()->OncePerFrame();
+		g_screenManager->getUIContext()->SetTintSaturation(g_Config.fUITint, g_Config.fUISaturation);
+
+		// All actual rendering (and also emulation) happens in this render() call.
+		renderFlags = g_screenManager->render();
+		if (g_screenManager->getUIContext()->Text()) {
+			g_screenManager->getUIContext()->Text()->OncePerFrame();
+		}
+
+		ui_draw2d.PopDrawMatrix();
+
+		runImDebugger(g_draw);
+		renderImDebugger(g_draw);
 	}
-
-	ui_draw2d.PopDrawMatrix();
-
-	runImDebugger(g_draw);
-	renderImDebugger(g_draw);
 	g_draw->EndFrame();
 
 	// This, between EndFrame and Present, is where we should actually wait to do present time management.

Windows/Debugger/CtrlDisAsmView.cpp

@@ -60,6 +60,10 @@ void CtrlDisAsmView::deinit()
 
 void CtrlDisAsmView::scanVisibleFunctions()
 {
+	// Reads live memory/symbol state to detect function boundaries - hold g_frameMutex for the
+	// duration, which NativeFrame() also holds while it's actually touching that state. See
+	// g_frameMutex in Core.h.
+	std::lock_guard<std::mutex> frameGuard(g_frameMutex);
 	g_disassemblyManager.analyze(windowStart, g_disassemblyManager.getNthNextAddress(windowStart,visibleRows)-windowStart);
 }
 
@@ -453,9 +457,14 @@ void CtrlDisAsmView::drawArguments(HDC hdc, const DisassemblyLineInfo &line, int
 
 void CtrlDisAsmView::onPaint(WPARAM wParam, LPARAM lParam)
 {
-	auto memLock = Memory::Lock();
+	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.
+	std::lock_guard<std::mutex> frameGuard(g_frameMutex);
+
 	PAINTSTRUCT ps;
 	HDC actualHdc = BeginPaint(wnd, &ps);
 	HDC hdc = CreateCompatibleDC(actualHdc);

Windows/Debugger/CtrlMemView.cpp

@@ -181,7 +181,11 @@ void CtrlMemView::onPaint(WPARAM wParam, LPARAM lParam) {
 	if (Achievements::HardcoreModeActive())
 		return;
 
-	auto memLock = Memory::Lock();
+	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.
+	std::lock_guard<std::mutex> frameGuard(g_frameMutex);
 
 	// draw to a bitmap for double buffering
 	PAINTSTRUCT ps;	

Windows/Debugger/CtrlRegisterList.cpp

@@ -199,6 +199,14 @@ void CtrlRegisterList::onPaint(WPARAM wParam, LPARAM lParam)
 
 	int numRows=rect.bottom/rowHeight;
 
+	// Reading live CPU-thread-owned register 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.
+	std::lock_guard<std::mutex> frameGuard(g_frameMutex);
+	// The values are a moving target while the core is running - gray them out rather than trying
+	// to highlight "changes" that are really just noise at that point.
+	bool running = !Core_IsStepping();
+
 	for (int i=0; i<numRows; i++)
 	{
 		int rowY1 = rowHeight*(i+1);
@@ -233,7 +241,7 @@ void CtrlRegisterList::onPaint(WPARAM wParam, LPARAM lParam)
 				changedCat0Regs[j] = v != lastCat0Values[j];
 				lastCat0Values[j] = v;
 			}
-			
+
 			changedCat0Regs[REGISTER_PC] = cpu->GetPC() != lastCat0Values[REGISTER_PC];
 			lastCat0Values[REGISTER_PC] = cpu->GetPC();
 			changedCat0Regs[REGISTER_HI] = cpu->GetHi() != lastCat0Values[REGISTER_HI];
@@ -250,12 +258,13 @@ void CtrlRegisterList::onPaint(WPARAM wParam, LPARAM lParam)
 		{
 			char temp[256];
 			int temp_len = snprintf(temp, sizeof(temp), "%s", cpu->GetRegName(category, i).c_str());
-			SetTextColor(hdc,0x600000);
+			SetTextColor(hdc, running ? 0x808080 : 0x600000);
 			TextOutA(hdc,17,rowY1,temp,temp_len);
-			SetTextColor(hdc,0x000000);
 
 			cpu->PrintRegValue(category, i, temp, sizeof(temp));
-			if (category == 0 && changedCat0Regs[i])
+			if (running)
+				SetTextColor(hdc, 0x808080);
+			else if (category == 0 && changedCat0Regs[i])
 				SetTextColor(hdc, 0x0000FF);
 			else
 				SetTextColor(hdc,0x004000);
@@ -286,10 +295,12 @@ void CtrlRegisterList::onPaint(WPARAM wParam, LPARAM lParam)
 				break;
 			}
 
-			SetTextColor(hdc,0x600000);
+			SetTextColor(hdc, running ? 0x808080 : 0x600000);
 			TextOutA(hdc,17,rowY1,temp,len);
 			len = snprintf(temp, sizeof(temp), "%08X",value);
-			if (changedCat0Regs[i])
+			if (running)
+				SetTextColor(hdc, 0x808080);
+			else if (changedCat0Regs[i])
 				SetTextColor(hdc, 0x0000FF);
 			else
 				SetTextColor(hdc,0x004000);

Windows/Debugger/CtrlRegisterList.h

@@ -28,7 +28,7 @@ class CtrlRegisterList {
 	int category = 0;
 
 	int oldSelection = 0;
-	
+
 	bool selecting = false;
 	bool hasFocus = false;
 	MIPSDebugInterface *cpu = nullptr;

Windows/Debugger/Debugger_Disasm.cpp

@@ -726,6 +726,10 @@ void CDisasm::SetDebugMode(bool _bDebug, bool switchPC)
 void CDisasm::Show(bool bShow, bool includeToTop) {
 	if (deferredSymbolFill_ && bShow) {
 		if (g_symbolMap) {
+			// Reading the live symbol map 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.
+			std::lock_guard<std::mutex> frameGuard(g_frameMutex);
 			g_symbolMap->FillSymbolListBox(GetDlgItem(m_hDlg, IDC_FUNCTIONLIST), ST_FUNCTION);
 			deferredSymbolFill_ = false;
 		}
@@ -735,6 +739,7 @@ void CDisasm::Show(bool bShow, bool includeToTop) {
 
 void CDisasm::NotifyMapLoaded() {
 	if (m_bShowState != SW_HIDE && g_symbolMap) {
+		std::lock_guard<std::mutex> frameGuard(g_frameMutex);
 		g_symbolMap->FillSymbolListBox(GetDlgItem(m_hDlg, IDC_FUNCTIONLIST), ST_FUNCTION);
 	} else {
 		deferredSymbolFill_ = true;

Windows/Debugger/Debugger_Lists.cpp

@@ -255,7 +255,13 @@ void CtrlThreadList::OnRightClick(int itemIndex, int column, const POINT& point)
 
 void CtrlThreadList::reloadThreads()
 {
-	threads = GetThreadsInfo();
+	// Reading live kernel thread 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.
+	{
+		std::lock_guard<std::mutex> frameGuard(g_frameMutex);
+		threads = GetThreadsInfo();
+	}
 	Update();
 }
 
@@ -325,9 +331,13 @@ bool CtrlBreakpointList::WindowMessage(UINT msg, WPARAM wParam, LPARAM lParam, L
 
 void CtrlBreakpointList::reloadBreakpoints()
 {
-	// Update the items we're displaying from the debugger.
-	displayedBreakPoints_ = g_breakpoints.GetBreakpoints();
-	displayedMemChecks_= g_breakpoints.GetMemChecks();
+	// Update the items we're displaying from the debugger. g_frameMutex guards this against races
+	// with the CPU thread - see g_frameMutex in Core.h.
+	{
+		std::lock_guard<std::mutex> frameGuard(g_frameMutex);
+		displayedBreakPoints_ = g_breakpoints.GetBreakpoints();
+		displayedMemChecks_= g_breakpoints.GetMemChecks();
+	}
 
 	for (int i = 0; i < GetRowCount(); i++)
 	{
@@ -744,11 +754,16 @@ void CtrlStackTraceView::OnDoubleClick(int itemIndex, int column)
 }
 
 void CtrlStackTraceView::loadStackTrace() {
-	auto memLock = Memory::Lock();
+	Memory::MemoryInitedLock memLock = Memory::Lock();
 	if (!PSP_IsInited())
 		return;
 
-	auto threads = GetThreadsInfo();
+	// 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.
+	std::lock_guard<std::mutex> frameGuard(g_frameMutex);
+
+	std::vector<DebugThreadInfo> threads = GetThreadsInfo();
 
 	u32 entry = 0, stackTop = 0;
 	for (size_t i = 0; i < threads.size(); i++)
@@ -835,10 +850,16 @@ void CtrlModuleList::OnDoubleClick(int itemIndex, int column)
 
 void CtrlModuleList::loadModules()
 {
-	if (g_symbolMap) {
-		modules = g_symbolMap->getAllModules();
-	} else {
-		modules.clear();
+	// Reading the live symbol map 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.
+	{
+		std::lock_guard<std::mutex> frameGuard(g_frameMutex);
+		if (g_symbolMap) {
+			modules = g_symbolMap->getAllModules();
+		} else {
+			modules.clear();
+		}
 	}
 	Update();
 }
@@ -855,6 +876,11 @@ CtrlWatchList::CtrlWatchList(HWND hwnd, DebugInterface *cpu)
 }
 
 void CtrlWatchList::RefreshValues() {
+	// Evaluating watch expressions reads live registers/memory here on the GUI thread, which would
+	// otherwise race with the CPU thread - hold g_frameMutex for the duration, which NativeFrame()
+	// also holds while it's actually touching that state. See g_frameMutex in Core.h.
+	std::lock_guard<std::mutex> frameGuard(g_frameMutex);
+
 	int steppingCounter = Core_GetSteppingCounter();
 	int changes = false;
 	for (auto &watch : watches_) {

Windows/Debugger/Debugger_MemoryDlg.cpp

@@ -6,6 +6,7 @@
 #include "Common/Data/Encoding/Utf8.h"
 #include "Common/System/Display.h"
 
+#include "Core/Core.h"
 #include "Core/Debugger/MemBlockInfo.h"
 #include "Core/Debugger/SymbolMap.h"
 #include "Core/MIPS/MIPSDebugInterface.h"
@@ -131,10 +132,15 @@ void CMemoryDlg::searchBoxRedraw(const std::vector<u32> &results) {
 
 
 void CMemoryDlg::NotifyMapLoaded() {
-	if (m_hDlg && g_symbolMap)
+	if (m_hDlg && g_symbolMap) {
+		// Reading the live symbol map 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.
+		std::lock_guard<std::mutex> frameGuard(g_frameMutex);
 		g_symbolMap->FillSymbolListBox(symListHdl, ST_DATA);
-	else
+	} else {
 		mapLoadPending_ = true;
+	}
 	Update();
 }
 
@@ -221,6 +227,7 @@ BOOL CMemoryDlg::DlgProc(UINT message, WPARAM wParam, LPARAM lParam) {
 
 	case WM_DEB_UPDATE:
 		if (mapLoadPending_ && m_hDlg && g_symbolMap) {
+			std::lock_guard<std::mutex> frameGuard(g_frameMutex);
 			g_symbolMap->FillSymbolListBox(symListHdl, ST_DATA);
 			mapLoadPending_ = false;
 		}