diff --git a/Core/Core.cpp b/Core/Core.cpp index 337185e003..fda3ae5d31 100644 --- a/Core/Core.cpp +++ b/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; diff --git a/Core/Core.h b/Core/Core.h index 493701c127..a52efcc869 100644 --- a/Core/Core.h +++ b/Core/Core.h @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -164,6 +165,22 @@ void Core_RunLoopUntil(u64 globalticks); // even while it's fully running. void Core_RunOnCPUThread(std::function 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; diff --git a/UI/NativeApp.cpp b/UI/NativeApp.cpp index 3d5a07acf4..976adabad4 100644 --- a/UI/NativeApp.cpp +++ b/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 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. diff --git a/Windows/Debugger/CtrlDisAsmView.cpp b/Windows/Debugger/CtrlDisAsmView.cpp index e4588abfe5..d77313c4ce 100644 --- a/Windows/Debugger/CtrlDisAsmView.cpp +++ b/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 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 frameGuard(g_frameMutex); + PAINTSTRUCT ps; HDC actualHdc = BeginPaint(wnd, &ps); HDC hdc = CreateCompatibleDC(actualHdc); diff --git a/Windows/Debugger/CtrlMemView.cpp b/Windows/Debugger/CtrlMemView.cpp index fcde41806e..c2bce68e93 100644 --- a/Windows/Debugger/CtrlMemView.cpp +++ b/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 frameGuard(g_frameMutex); // draw to a bitmap for double buffering PAINTSTRUCT ps; diff --git a/Windows/Debugger/CtrlRegisterList.cpp b/Windows/Debugger/CtrlRegisterList.cpp index fe0874707a..8d0f05320f 100644 --- a/Windows/Debugger/CtrlRegisterList.cpp +++ b/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 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; iGetPC() != 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); diff --git a/Windows/Debugger/CtrlRegisterList.h b/Windows/Debugger/CtrlRegisterList.h index 211e6203c7..5bc31d9b41 100644 --- a/Windows/Debugger/CtrlRegisterList.h +++ b/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; diff --git a/Windows/Debugger/Debugger_Disasm.cpp b/Windows/Debugger/Debugger_Disasm.cpp index 7d4493c695..04ebace40d 100644 --- a/Windows/Debugger/Debugger_Disasm.cpp +++ b/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 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 frameGuard(g_frameMutex); g_symbolMap->FillSymbolListBox(GetDlgItem(m_hDlg, IDC_FUNCTIONLIST), ST_FUNCTION); } else { deferredSymbolFill_ = true; diff --git a/Windows/Debugger/Debugger_Lists.cpp b/Windows/Debugger/Debugger_Lists.cpp index 7e2f160a1e..5c3ea40f6c 100644 --- a/Windows/Debugger/Debugger_Lists.cpp +++ b/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 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 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 frameGuard(g_frameMutex); + + std::vector 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 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 frameGuard(g_frameMutex); + int steppingCounter = Core_GetSteppingCounter(); int changes = false; for (auto &watch : watches_) { diff --git a/Windows/Debugger/Debugger_MemoryDlg.cpp b/Windows/Debugger/Debugger_MemoryDlg.cpp index cc485ca0d7..51cd435898 100644 --- a/Windows/Debugger/Debugger_MemoryDlg.cpp +++ b/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 &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 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 frameGuard(g_frameMutex); g_symbolMap->FillSymbolListBox(symListHdl, ST_DATA); mapLoadPending_ = false; }