diff --git a/AGENTS.md b/AGENTS.md index ce7071bd40..61fc752a2c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -383,11 +383,14 @@ Two more things found while doing this: `CDisasm::NotifyMapLoaded()`, which locks it internally) will deadlock. Keep such calls outside the queued lambda, same as the modal-dialog and `SendMessage()` rules above. -### Lock ordering: `g_frameMutex` before `Memory::Lock()`, always +### Lock ordering: `g_frameMutex` before `Core_LockAgainstShutdown()`, always -`Memory::Lock()` / `Memory::MemoryInitedLock` (a `recursive_mutex`, `g_shutdownLock` in -`Core/MemMap.cpp`) guards the memory system against being torn down or reinitialized under you. When -a function needs both it and `g_frameMutex`, **take `g_frameMutex` first**. +`Core_LockAgainstShutdown()` / `CoreShutdownLock` (a `recursive_mutex`, `g_shutdownLock` in +`Core/Core.cpp`) is held across `CPU_Shutdown()` and `Memory::Reinit()`, i.e. while the core is +going away. Take it on any thread other than the CPU thread before reading core state - emulated +memory, the symbol map, kernel objects - so none of it is freed mid-read. It was called +`Memory::Lock()` and only covered the memory map; the name misled people into thinking it was about +memory access. When a function needs both it and `g_frameMutex`, **take `g_frameMutex` first**. The CPU thread's order is structural and can't be changed: `NativeFrame()` wraps everything below it in `g_frameMutex`, and several things under there lock memory - `Core_ProcessCPUQueue()` running a @@ -395,30 +398,36 @@ queued WebSocket handler, and `runImDebugger()` -> `ImMemView` -> `DisassembleRa GUI-thread side is the one that has to match. (Getting it backwards deadlocked for real: a paint handler held the memory lock and waited for `g_frameMutex` while the CPU thread did the reverse.) -Also: **a `Core_RunOnCPUThread()` callback does not need `Memory::Lock()`** - teardown only happens +Also: **a `Core_RunOnCPUThread()` callback does not need `Core_LockAgainstShutdown()`** - teardown only happens on the CPU thread itself (`Memory::Shutdown()` via `CPU_Shutdown()` <- `PSP_Shutdown()`, all callers on that thread; `Memory::Reinit()` from `Memory::DoState()` on savestate load). Don't add one. **The general rule behind both of these: never make the CPU thread wait for a thread that is (or may be) waiting on the CPU thread.** `Core_RunOnCPUThread()` blocks until the CPU thread drains the queue, so anything the CPU thread might block on must not be held across such a call. The WebSocket -debugger's `lifecycleLock` hit exactly this - it's held across a whole event handler, and the CPU +debugger's `lifecycleLock` hit exactly this - it was held across a whole event handler, and the CPU thread took it in `Core_NotifyLifecycle(STOPPING)`, so stopping a game with a debugger request in -flight hung both threads. Resolved by draining the queue while waiting for the lock rather than -blocking on it outright (`WebSocketNotifyLifecycle` in `Core/Debugger/WebSocket.cpp`). +flight hung both threads. That lock is gone now; the rule is what's left of it. -`lifecycleLock` itself can't just be deleted, tempting as it looks: about half the subscribers -(`GameSubscriber`, `GPU*Subscriber`, `InputSubscriber`, `MemoryInfoSubscriber`, `ReplaySubscriber`, -`ClientConfigSubscriber`) and all four broadcasters still read core state directly on the WebSocket -thread rather than going through `Core_RunOnCPUThread()`. It's what stops that racing with teardown. -Routing those through the queue is the prerequisite for removing it. +**The WebSocket debugger no longer has any lock guarding it against startup/shutdown, and must not +grow one back.** The invariant instead is: a handler either does its emulator-state access inside +`Core_RunOnCPUThread()` - which serializes it against startup and teardown, since those run on the +CPU thread too - or touches only state that carries its own lock (the log ring buffer, `ctrlMutex`, +`GPUStepping`'s pause-action rendezvous). When adding a subscriber, put the core access in the +queued callback, including the `isAlive()`/`IsValidAddress()` checks: answering those outside it +just means acting on an answer that may already be stale. -The Win32 debugger's paint handlers *do* still need it, though, so don't "simplify" those away: +`game.*` and `cpu.stepping`/`cpu.resume` are pushed rather than polled - `WebSocketDebuggerTick()` +(called from `Core_ProcessCPUQueue()`) notices the transition on the CPU thread, formats the event +there, and drops it in a per-connection mailbox. Don't add a broadcaster that reads emulator state +from the connection's own thread; produce the event on the CPU thread and push it instead. + +The Win32 debugger's GUI-thread readers *do* still need it, so don't "simplify" those away: teardown is not yet fully inside the `g_frameMutex` span. `EmuScreen::render()`'s `PSP_Shutdown()` is inside it, but the ones in `EmuScreen::sendMessage()` (`REQUEST_GAME_RESET`, loading a new game) run from `g_screenManager->sendMessage()` in `NativeFrame()`, which sits *above* where the guard is taken. Closing that hole - moving those shutdowns inside the span, or deferring them to render time - -is the prerequisite for dropping `Memory::Lock()` from the debugger entirely. +is the prerequisite for dropping the shutdown lock from the debugger entirely. 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 diff --git a/Common/ExceptionHandlerSetup.cpp b/Common/ExceptionHandlerSetup.cpp index 6df5cc4a56..e26b9c7d85 100644 --- a/Common/ExceptionHandlerSetup.cpp +++ b/Common/ExceptionHandlerSetup.cpp @@ -20,9 +20,42 @@ #include "Common/MachineContext.h" #include "Common/ExceptionHandlerSetup.h" +#if defined(_MSC_VER) +#include +#include "Common/CommonWindows.h" +#endif + static BadAccessHandler g_badAccessHandler; static void *altStack = nullptr; +void SetupCRT(bool suppressDialogs) { +#if defined(_MSC_VER) + _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF); + + if (suppressDialogs) { + // 1. Redirect CRT assertions/errors/warnings to stderr. + const _HFILE reportTarget = _CRTDBG_FILE_STDERR; + + _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE); + _CrtSetReportFile(_CRT_ASSERT, reportTarget); + + _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE); + _CrtSetReportFile(_CRT_ERROR, reportTarget); + + _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE); + _CrtSetReportFile(_CRT_WARN, reportTarget); + + // 2. Suppress the abort() message box & crash reporting dialogs. + _set_abort_behavior(0, _WRITE_ABORT_MSG | _CALL_REPORTFAULT); + +#if !PPSSPP_PLATFORM(UWP) + // 3. Suppress Windows OS-level "Program has stopped working" modal dialogs. + SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX); +#endif + } +#endif +} + #ifdef MACHINE_CONTEXT_SUPPORTED // We cannot handle exceptions in UWP builds. Bleh. diff --git a/Common/ExceptionHandlerSetup.h b/Common/ExceptionHandlerSetup.h index da47a55687..caaccaee8b 100644 --- a/Common/ExceptionHandlerSetup.h +++ b/Common/ExceptionHandlerSetup.h @@ -21,3 +21,8 @@ void InstallExceptionHandler(BadAccessHandler accessHandler, bool logStackTraceO // Implementation note: This must be a no-op if InstallExceptionHandler hasn't been called. void UninstallExceptionHandler(); + +// MSVC-only, no-op elsewhere. Turns on the debug CRT's leak checking, and with suppressDialogs +// set, routes CRT assertions and abort() to stderr instead of a modal dialog - required for +// anything run non-interactively (headless, the unit tests, CI), where a dialog just hangs. +void SetupCRT(bool suppressDialogs); diff --git a/Core/Core.cpp b/Core/Core.cpp index d841f1ad2e..1aa508c030 100644 --- a/Core/Core.cpp +++ b/Core/Core.cpp @@ -40,6 +40,7 @@ #include "Core/System.h" #include "Core/MemFault.h" #include "Core/Debugger/Breakpoints.h" +#include "Core/Debugger/WebSocket.h" #include "Core/MIPS/MIPS.h" #include "Core/MIPS/MIPSAnalyst.h" #include "Core/HLE/sceKernelModule.h" @@ -113,6 +114,10 @@ void Core_ProcessCPUQueue() { g_cpuThreadIdValid.store(true, std::memory_order_release); }); + // Piggybacking on the one function that's reliably called on the CPU thread both in game + // (Core_RunLoopUntil) and at the menu (NativeFrame) - see WebSocketDebuggerTick(). + WebSocketDebuggerTick(); + std::vector> tasks; { std::lock_guard guard(g_cpuQueueMutex); @@ -133,6 +138,21 @@ void Core_ProcessCPUQueue() { g_cpuQueueCond.notify_all(); } +// See Core.h. Recursive because Memory::Shutdown() nests inside CPU_Shutdown()'s acquire. +static std::recursive_mutex g_shutdownLock; + +CoreShutdownLock::CoreShutdownLock() { + g_shutdownLock.lock(); +} + +CoreShutdownLock::~CoreShutdownLock() { + g_shutdownLock.unlock(); +} + +CoreShutdownLock Core_LockAgainstShutdown() { + return CoreShutdownLock(); +} + // 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; diff --git a/Core/Core.h b/Core/Core.h index 774bde4167..014aa14447 100644 --- a/Core/Core.h +++ b/Core/Core.h @@ -168,6 +168,20 @@ void Core_ReenterDispatcher(); // If you've done things that mess with caches, // even while it's fully running. void Core_RunOnCPUThread(std::function func); +// Held while the core is being torn down (CPU_Shutdown) or its memory map reinitialized. Take it on +// any thread other than the CPU thread before reading core state - emulated memory, the symbol map, +// kernel objects - so none of it can be freed mid-read. Recursive, so nesting is fine. +// +// It is not a lock on memory *access*: it doesn't stop the CPU thread mutating anything, only stop +// it going away. If you also need a stable snapshot, take g_frameMutex first - see the ordering +// rule in AGENTS.md. +class CoreShutdownLock { +public: + CoreShutdownLock(); + ~CoreShutdownLock(); +}; +CoreShutdownLock Core_LockAgainstShutdown(); + // Drains the queue Core_RunOnCPUThread() feeds. Normally called from the top of every // Core_RunLoopUntil() iteration, but that function is only reached while a game is actually // loaded/running (via EmuScreen) - so NativeFrame() (UI/NativeApp.cpp) also calls this directly, diff --git a/Core/Debugger/DisassemblyManager.cpp b/Core/Debugger/DisassemblyManager.cpp index 2bcd1da1b4..897a5f5360 100644 --- a/Core/Debugger/DisassemblyManager.cpp +++ b/Core/Debugger/DisassemblyManager.cpp @@ -28,6 +28,7 @@ #include "Common/Log.h" #include "Common/StringUtils.h" #include "Common/Math/math_util.h" +#include "Core/Core.h" #include "Core/MemMap.h" #include "Core/System.h" #include "Core/MIPS/MIPSDebugInterface.h" @@ -992,7 +993,7 @@ bool GetDisasmAddressText(u32 address, char *dest, size_t bufSize, bool abbrevia // Utilify function from the old debugger. std::string DisassembleRange(u32 start, u32 size, bool displaySymbols, MIPSDebugInterface *debugger) { - auto memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); std::string result; // gather all branch targets without labels diff --git a/Core/Debugger/WebSocket.cpp b/Core/Debugger/WebSocket.cpp index 8aa1f00a4e..d885a5544a 100644 --- a/Core/Debugger/WebSocket.cpp +++ b/Core/Debugger/WebSocket.cpp @@ -15,8 +15,10 @@ // Official git repository and contact information can be found at // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/. +#include #include #include +#include #include "Common/Thread/ThreadUtil.h" #include "Common/TimeUtil.h" @@ -92,9 +94,12 @@ static volatile bool stopRequested = false; static std::mutex stopLock; static std::condition_variable stopCond; -// Prevent threading surprises and obscure crashes by locking startup/shutdown. -static bool lifecycleLockSetup = false; -static std::mutex lifecycleLock; +// There is deliberately no lock guarding debugger handlers against the core being started or torn +// down under them: every handler either does its emulator-state access inside Core_RunOnCPUThread() +// (so it's serialized with startup/shutdown, which also run on the CPU thread), or only touches +// state that carries its own lock - the log ring buffer, ctrlMutex, GPUStepping's rendezvous. +// The lock that used to be here had to be held across a whole handler, including the blocking wait +// inside Core_RunOnCPUThread(), which deadlocked against the CPU thread taking it on STOPPING. static void UpdateConnected(int delta) { std::lock_guard guard(stopLock); @@ -102,40 +107,68 @@ static void UpdateConnected(int delta) { stopCond.notify_all(); } -static void WebSocketNotifyLifecycle(CoreLifecycle stage) { - switch (stage) { - case CoreLifecycle::STARTING: - case CoreLifecycle::STOPPING: - case CoreLifecycle::MEMORY_REINITING: - if (debuggersConnected > 0) { - DEBUG_LOG(Log::System, "Waiting for debugger to complete on shutdown"); - } - // Keep draining the CPU queue while we wait, instead of a plain blocking lock(). We're on - // the CPU thread here, and a debugger thread holding this lock may be parked inside - // Core_RunOnCPUThread() waiting for us to run its callback - so blocking outright means - // neither side can ever move. Core state is still fully alive at this point (STOPPING is - // notified before CPU_Shutdown), so running those callbacks now is safe. - while (!lifecycleLock.try_lock()) { - Core_ProcessCPUQueue(); - sleep_ms(1, "debugger-lifecycle"); - } - break; +// Per-connection mailbox for events the CPU thread produces (cpu.stepping, game.start, ...). +// +// These used to be polled per connection from the WebSocket thread, which meant every connected +// debugger was reading pc, the tick count, the UI state and the param SFO out from under the CPU +// thread on every lap of its loop. Now the CPU thread notices the transition once, formats the +// event, and drops it in here; the connection's own thread just drains and sends. +struct DebuggerEventSink { + std::mutex lock; + std::vector> pending; + // A debugger that connects while the CPU is already stopped still wants to hear about it. + bool needsSteppingPrime = true; - case CoreLifecycle::START_COMPLETE: - case CoreLifecycle::STOPPED: - case CoreLifecycle::MEMORY_REINITED: - lifecycleLock.unlock(); - if (debuggersConnected > 0) { - DEBUG_LOG(Log::System, "Debugger ready for shutdown"); - } - break; + void Push(const char *category, std::string json) { + std::lock_guard guard(lock); + pending.emplace_back(category, std::move(json)); } + + void Take(std::vector> *out) { + std::lock_guard guard(lock); + out->swap(pending); + pending.clear(); + } +}; + +static std::mutex g_sinkLock; +static std::vector g_sinks; + +static void RegisterSink(DebuggerEventSink *sink) { + std::lock_guard guard(g_sinkLock); + g_sinks.push_back(sink); } -static void SetupDebuggerLock() { - if (!lifecycleLockSetup) { - Core_ListenLifecycle(&WebSocketNotifyLifecycle); - lifecycleLockSetup = true; +static void UnregisterSink(DebuggerEventSink *sink) { + std::lock_guard guard(g_sinkLock); + g_sinks.erase(std::remove(g_sinks.begin(), g_sinks.end(), sink), g_sinks.end()); +} + +void WebSocketDebuggerTick() { + // Poll unconditionally, even with nothing connected: these track transitions, and skipping them + // would let the "previous" state go stale and fire a bogus event at whoever connects next. + const std::string gameEvent = GameBroadcaster::PollChange(); + const std::string steppingEvent = SteppingBroadcaster::PollChange(); + + std::lock_guard guard(g_sinkLock); + if (g_sinks.empty()) + return; + + std::string steppingPrime; + for (DebuggerEventSink *sink : g_sinks) { + if (sink->needsSteppingPrime) { + sink->needsSteppingPrime = false; + // Only format it if somebody actually needs it. + if (steppingPrime.empty()) + steppingPrime = SteppingBroadcaster::CurrentState(); + if (!steppingPrime.empty()) + sink->Push("stepping", steppingPrime); + continue; + } + if (!gameEvent.empty()) + sink->Push("game", gameEvent); + if (!steppingEvent.empty()) + sink->Push("stepping", steppingEvent); } } @@ -148,20 +181,19 @@ void HandleDebuggerRequest(const http::ServerRequest &request) { } UpdateConnected(1); - SetupDebuggerLock(); WebSocketClientInfo client_info; auto& disallowed_config = client_info.disallowed; - GameBroadcaster game; LogBroadcaster logger; InputBroadcaster input; - SteppingBroadcaster stepping; + + DebuggerEventSink sink; + RegisterSink(&sink); DebuggerEventHandlerMap eventHandlers; std::vector subscriberData; for (auto init : subscribers) { - std::lock_guard guard(lifecycleLock); subscriberData.push_back(init(eventHandlers)); } @@ -186,7 +218,6 @@ void HandleDebuggerRequest(const http::ServerRequest &request) { DebuggerRequest req(event, ws, root, &client_info); auto eventFunc = eventHandlers.find(event); if (eventFunc != eventHandlers.end()) { - std::lock_guard guard(lifecycleLock); eventFunc->second(req); if (!req.Finish()) { // Poll more frequently for a second in case this triggers something. @@ -206,20 +237,23 @@ void HandleDebuggerRequest(const http::ServerRequest &request) { constexpr float lowActivityPollTimeStep = 1.0f / 60.0f; constexpr float highActivityPollTimeStep = 1.0f / 1000.0f; while (ws->Process(highActivity ? highActivityPollTimeStep : lowActivityPollTimeStep)) { - std::lock_guard guard(lifecycleLock); // These send events that aren't just responses to requests // The client can explicitly ask not to be notified about some events // so we check the client settings first if (!disallowed_config["logger"]) logger.Broadcast(ws); - if (!disallowed_config["game"]) - game.Broadcast(ws); - if (!disallowed_config["stepping"]) - stepping.Broadcast(ws); if (!disallowed_config["input"]) input.Broadcast(ws); + // Whatever the CPU thread queued up for us since last lap. + std::vector> events; + sink.Take(&events); + for (const auto &ev : events) { + if (!disallowed_config[ev.first]) + ws->Send(ev.second); + } + for (size_t i = 0; i < subscribers.size(); ++i) { if (subscriberData[i]) { subscriberData[i]->Broadcast(ws); @@ -235,7 +269,8 @@ void HandleDebuggerRequest(const http::ServerRequest &request) { } } - std::lock_guard guard(lifecycleLock); + UnregisterSink(&sink); + for (size_t i = 0; i < subscribers.size(); ++i) { delete subscriberData[i]; } diff --git a/Core/Debugger/WebSocket.h b/Core/Debugger/WebSocket.h index f05856db20..ef6c823c8b 100644 --- a/Core/Debugger/WebSocket.h +++ b/Core/Debugger/WebSocket.h @@ -24,3 +24,8 @@ class ServerRequest; void HandleDebuggerRequest(const http::ServerRequest &request); // Note: blocks. void StopAllDebuggers(); + +// Notices emulator state changes (cpu.stepping, game.start, ...) and pushes the resulting events to +// connected debuggers, so their own threads never have to read that state themselves. CPU thread +// only; cheap, and safe to call with no debugger connected (it still has to track transitions). +void WebSocketDebuggerTick(); diff --git a/Core/Debugger/WebSocket/BreakpointSubscriber.cpp b/Core/Debugger/WebSocket/BreakpointSubscriber.cpp index 83e9e365a2..1b69eb9109 100644 --- a/Core/Debugger/WebSocket/BreakpointSubscriber.cpp +++ b/Core/Debugger/WebSocket/BreakpointSubscriber.cpp @@ -108,10 +108,6 @@ struct WebSocketCPUBreakpointParams { if (hasCondition) { if (!req.ParamString("condition", &condition)) return false; - if (!initExpression(currentDebugMIPS, condition.c_str(), compiledCondition)) { - req.Fail(StringFromFormat("Could not parse expression syntax: %s", getExpressionError())); - return false; - } } hasLogFormat = req.HasParam("logFormat"); if (hasLogFormat) { @@ -122,6 +118,17 @@ struct WebSocketCPUBreakpointParams { return true; } + // Compiled on the CPU thread rather than in Parse(): resolving symbols in an expression goes + // through g_symbolMap, which is CPU-thread-owned and destroyed on shutdown. + bool CompileCondition(std::string *error) { + if (!hasCondition || condition.empty()) + return true; + if (initExpression(currentDebugMIPS, condition.c_str(), compiledCondition)) + return true; + *error = StringFromFormat("Could not parse expression syntax: %s", getExpressionError()); + return false; + } + void Apply() { if (hasCondition && !condition.empty()) { BreakPointCond cond; @@ -174,10 +181,15 @@ void WebSocketCPUBreakpointAdd(DebuggerRequest &req) { // Route the actual breakpoint manipulation to the CPU thread instead of poking at it directly // from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. + std::string error; Core_RunOnCPUThread([&] { + if (!params.CompileCondition(&error)) + return; g_breakpoints.AddBreakPoint(params.address); params.Apply(); }); + if (!error.empty()) + return req.Fail(error); req.Respond(); } @@ -199,13 +211,18 @@ void WebSocketCPUBreakpointUpdate(DebuggerRequest &req) { // Route the actual breakpoint manipulation to the CPU thread instead of poking at it directly // from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. bool found = false; + std::string error; Core_RunOnCPUThread([&] { + if (!params.CompileCondition(&error)) + return; bool enabled; found = g_breakpoints.IsAddressBreakPoint(params.address, &enabled); if (found) params.Apply(); }); + if (!error.empty()) + return req.Fail(error); if (!found) return req.Fail("Breakpoint not found"); req.Respond(); @@ -349,10 +366,6 @@ struct WebSocketMemoryBreakpointParams { if (hasCondition) { if (!req.ParamString("condition", &condition)) return false; - if (!initExpression(currentDebugMIPS, condition.c_str(), compiledCondition)) { - req.Fail(StringFromFormat("Could not parse expression syntax: %s", getExpressionError())); - return false; - } } hasLogFormat = req.HasParam("logFormat"); if (hasLogFormat) { @@ -381,6 +394,17 @@ struct WebSocketMemoryBreakpointParams { return BreakAction(bits); } + // Compiled on the CPU thread rather than in Parse(): resolving symbols in an expression goes + // through g_symbolMap, which is CPU-thread-owned and destroyed on shutdown. + bool CompileCondition(std::string *error) { + if (!hasCondition || condition.empty()) + return true; + if (initExpression(currentDebugMIPS, condition.c_str(), compiledCondition)) + return true; + *error = StringFromFormat("Could not parse expression syntax: %s", getExpressionError()); + return false; + } + void Apply() { if (hasCondition && !condition.empty()) { BreakPointCond cond; @@ -421,10 +445,15 @@ void WebSocketMemoryBreakpointAdd(DebuggerRequest &req) { // Route the actual breakpoint manipulation to the CPU thread instead of poking at it directly // from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. + std::string error; Core_RunOnCPUThread([&] { + if (!params.CompileCondition(&error)) + return; g_breakpoints.AddMemCheck(params.address, params.end, params.cond, params.Action(true)); params.Apply(); }); + if (!error.empty()) + return req.Fail(error); req.Respond(); } @@ -451,7 +480,10 @@ void WebSocketMemoryBreakpointUpdate(DebuggerRequest &req) { // Route the actual breakpoint manipulation to the CPU thread instead of poking at it directly // from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. bool found = false; + std::string error; Core_RunOnCPUThread([&] { + if (!params.CompileCondition(&error)) + return; MemCheck mc; found = g_breakpoints.GetMemCheck(params.address, params.end, &mc); if (found) { @@ -460,6 +492,8 @@ void WebSocketMemoryBreakpointUpdate(DebuggerRequest &req) { } }); + if (!error.empty()) + return req.Fail(error); if (!found) return req.Fail("Breakpoint not found"); req.Respond(); @@ -590,10 +624,6 @@ struct WebSocketRegBreakpointParams { if (hasCondition) { if (!req.ParamString("condition", &condition)) return false; - if (!initExpression(currentDebugMIPS, condition.c_str(), compiledCondition)) { - req.Fail(StringFromFormat("Could not parse expression syntax: %s", getExpressionError())); - return false; - } } hasLogFormat = req.HasParam("logFormat"); if (hasLogFormat) { @@ -604,6 +634,17 @@ struct WebSocketRegBreakpointParams { return true; } + // Compiled on the CPU thread rather than in Parse(): resolving symbols in an expression goes + // through g_symbolMap, which is CPU-thread-owned and destroyed on shutdown. + bool CompileCondition(std::string *error) { + if (!hasCondition || condition.empty()) + return true; + if (initExpression(currentDebugMIPS, condition.c_str(), compiledCondition)) + return true; + *error = StringFromFormat("Could not parse expression syntax: %s", getExpressionError()); + return false; + } + void Apply() { if (hasCondition && !condition.empty()) { BreakPointCond cond; @@ -661,10 +702,15 @@ void WebSocketRegBreakpointAdd(DebuggerRequest &req) { // Route the actual breakpoint manipulation to the CPU thread instead of poking at it directly // from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. + std::string error; Core_RunOnCPUThread([&] { + if (!params.CompileCondition(&error)) + return; g_breakpoints.AddRegBreakpoint(params.reg); params.Apply(); }); + if (!error.empty()) + return req.Fail(error); req.Respond(); } @@ -681,13 +727,18 @@ void WebSocketRegBreakpointUpdate(DebuggerRequest &req) { // Route the actual breakpoint manipulation to the CPU thread instead of poking at it directly // from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. bool found = false; + std::string error; Core_RunOnCPUThread([&] { + if (!params.CompileCondition(&error)) + return; RegBreakpoint bp; found = g_breakpoints.GetRegBreakpoint(params.reg, &bp); if (found) params.Apply(); }); + if (!error.empty()) + return req.Fail(error); if (!found) return req.Fail("Breakpoint not found"); req.Respond(); diff --git a/Core/Debugger/WebSocket/GPUDisasmSubscriber.cpp b/Core/Debugger/WebSocket/GPUDisasmSubscriber.cpp index 37d050c5f5..18071b98d3 100644 --- a/Core/Debugger/WebSocket/GPUDisasmSubscriber.cpp +++ b/Core/Debugger/WebSocket/GPUDisasmSubscriber.cpp @@ -20,6 +20,7 @@ #include "Common/StringUtils.h" #include "Core/Debugger/WebSocket/GPUDisasmSubscriber.h" #include "Core/Debugger/WebSocket/WebSocketUtils.h" +#include "Core/Core.h" #include "Core/MemMap.h" #include "GPU/GPU.h" #include "GPU/GPUCommon.h" @@ -58,13 +59,6 @@ DebuggerSubscriber *WebSocketGPUDisasmInit(DebuggerEventHandlerMap &map) { // "AAAAAAAA desc" - meant for skimming a display list by eye instead of parsing full JSON, // same idea as memory.disasm's own compact mode. void WebSocketGPUDisplayListDisasm(DebuggerRequest &req) { - if (!gpu) { - return req.Fail("No GPU active (game not booted?)"); - } - if (!Memory::IsActive()) { - return req.Fail("Memory not active"); - } - // Mirrors memory.disasm's own limit - keeps a client typo (e.g. count=0xFFFFFFFF) from // blocking the debugger connection for an unreasonable amount of time. static const uint32_t MAX_RANGE = 10000; @@ -89,7 +83,17 @@ void WebSocketGPUDisplayListDisasm(DebuggerRequest &req) { if (!req.ParamBool("compact", &compact, DebuggerParamType::OPTIONAL)) return; - std::vector ops = gpu->DisassembleOpRange(start, end); + // gpu and the memory it disassembles from are CPU-thread-owned, so do the read over there + // rather than from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. + bool active = false; + std::vector ops; + Core_RunOnCPUThread([&] { + active = gpu && Memory::IsActive(); + if (active) + ops = gpu->DisassembleOpRange(start, end); + }); + if (!active) + return req.Fail("No GPU active (game not booted?)"); JsonWriter &json = req.Respond(); json.pushArray("lines"); diff --git a/Core/Debugger/WebSocket/GPURecordSubscriber.cpp b/Core/Debugger/WebSocket/GPURecordSubscriber.cpp index 4fdb7c2cfd..bad43f8175 100644 --- a/Core/Debugger/WebSocket/GPURecordSubscriber.cpp +++ b/Core/Debugger/WebSocket/GPURecordSubscriber.cpp @@ -17,6 +17,7 @@ #include "Common/Data/Encoding/Base64.h" #include "Common/File/FileUtil.h" +#include "Core/Core.h" #include "Core/Debugger/WebSocket/GPURecordSubscriber.h" #include "Core/Debugger/WebSocket/WebSocketUtils.h" #include "Core/System.h" @@ -44,9 +45,14 @@ DebuggerSubscriber *WebSocketGPURecordInit(DebuggerEventHandlerMap &map) { } WebSocketGPURecordState::~WebSocketGPURecordState() { - // Clear the callback to hopefully avoid a crash. - if (pending_) - gpu->GetRecorder()->ClearCallback(); + // Clear the callback to hopefully avoid a crash. On the CPU thread, since gpu itself is + // destroyed over there - see Core_RunOnCPUThread() in Core.h. + if (pending_) { + Core_RunOnCPUThread([&] { + if (gpu) + gpu->GetRecorder()->ClearCallback(); + }); + } } // Begin recording (gpu.record.dump) @@ -58,16 +64,24 @@ WebSocketGPURecordState::~WebSocketGPURecordState() { // // Note: recording may take a moment. void WebSocketGPURecordState::Dump(DebuggerRequest &req) { - if (PSP_GetBootState() != BootState::Complete) { - return req.Fail("CPU not started"); - } - - bool result = gpu->GetRecorder()->RecordNextFrame([=](const Path &filename) { - lastFilename_ = filename; - pending_ = false; + // gpu is created and destroyed on the CPU thread, so ask it for a recording over there rather + // than dereferencing it from this WebSocket handler thread. + bool started = false; + bool haveGPU = false; + Core_RunOnCPUThread([&] { + haveGPU = PSP_GetBootState() == BootState::Complete && gpu != nullptr; + if (!haveGPU) + return; + started = gpu->GetRecorder()->RecordNextFrame([=](const Path &filename) { + lastFilename_ = filename; + pending_ = false; + }); }); - if (!result) { + if (!haveGPU) { + return req.Fail("CPU not started"); + } + if (!started) { return req.Fail("Recording already in progress"); } diff --git a/Core/Debugger/WebSocket/GPUStatsSubscriber.cpp b/Core/Debugger/WebSocket/GPUStatsSubscriber.cpp index 4da650889f..c0d686b9b0 100644 --- a/Core/Debugger/WebSocket/GPUStatsSubscriber.cpp +++ b/Core/Debugger/WebSocket/GPUStatsSubscriber.cpp @@ -20,6 +20,7 @@ #include "Common/Data/Text/StringWriter.h" #include "Core/Debugger/WebSocket/GPUStatsSubscriber.h" +#include "Core/Core.h" #include "Core/HW/Display.h" #include "Core/System.h" @@ -102,8 +103,10 @@ WebSocketGPUStatsState::WebSocketGPUStatsState() { } WebSocketGPUStatsState::~WebSocketGPUStatsState() { + // PSP_ForceDebugStats bumps a plain counter, so do it on the CPU thread that owns it - see + // Core_RunOnCPUThread() in Core.h. if (forced_) - PSP_ForceDebugStats(false); + Core_RunOnCPUThread([] { PSP_ForceDebugStats(false); }); __DisplayForgetFlip(&WebSocketGPUStatsState::FlipForwarder, this); } @@ -185,7 +188,7 @@ void WebSocketGPUStatsState::Feed(DebuggerRequest &req) { std::lock_guard guard(pendingLock_); sendFeed_ = enable; if (forced_ != enable) { - PSP_ForceDebugStats(enable); + Core_RunOnCPUThread([enable] { PSP_ForceDebugStats(enable); }); forced_ = enable; } } diff --git a/Core/Debugger/WebSocket/GameBroadcaster.cpp b/Core/Debugger/WebSocket/GameBroadcaster.cpp index 755f041728..f6a1cca51b 100644 --- a/Core/Debugger/WebSocket/GameBroadcaster.cpp +++ b/Core/Debugger/WebSocket/GameBroadcaster.cpp @@ -73,22 +73,28 @@ struct GameStatusEvent { // - id: string disc ID (such as ULUS12345.) // - version: string disc version. // - title: string game title. -void GameBroadcaster::Broadcast(net::WebSocketServer *ws) { - // TODO: This is ugly. Callbacks instead? - GlobalUIState state = GetUIState(); - if (prevState_ != state) { - if (state == UISTATE_PAUSEMENU) { - ws->Send(GameStatusEvent{"game.pause"}); - prevState_ = state; - } else if (state == UISTATE_INGAME && prevState_ == UISTATE_PAUSEMENU) { - ws->Send(GameStatusEvent{"game.resume"}); - prevState_ = state; - } else if (state == UISTATE_INGAME && PSP_GetBootState() == BootState::Complete) { - ws->Send(GameStatusEvent{"game.start"}); - prevState_ = state; - } else if (state == UISTATE_MENU && PSP_GetBootState() != BootState::Complete) { - ws->Send(GameStatusEvent{"game.quit"}); - prevState_ = state; - } +std::string GameBroadcaster::PollChange() { + // Tracked globally rather than per connection: this runs on the CPU thread, which owns the state + // being read, and the resulting event is then handed to every connected debugger. + static GlobalUIState prevState = GetUIState(); + + const GlobalUIState state = GetUIState(); + if (prevState == state) + return std::string(); + + const char *ev = nullptr; + if (state == UISTATE_PAUSEMENU) { + ev = "game.pause"; + } else if (state == UISTATE_INGAME && prevState == UISTATE_PAUSEMENU) { + ev = "game.resume"; + } else if (state == UISTATE_INGAME && PSP_GetBootState() == BootState::Complete) { + ev = "game.start"; + } else if (state == UISTATE_MENU && PSP_GetBootState() != BootState::Complete) { + ev = "game.quit"; } + if (!ev) + return std::string(); + + prevState = state; + return GameStatusEvent{ev}; } diff --git a/Core/Debugger/WebSocket/GameBroadcaster.h b/Core/Debugger/WebSocket/GameBroadcaster.h index 63ef3eb346..3539a523a7 100644 --- a/Core/Debugger/WebSocket/GameBroadcaster.h +++ b/Core/Debugger/WebSocket/GameBroadcaster.h @@ -17,20 +17,14 @@ #pragma once -#include "Core/System.h" +#include -namespace net { -class WebSocketServer; -} +namespace GameBroadcaster { -struct GameBroadcaster { -public: - GameBroadcaster() { - prevState_ = GetUIState(); - } +// Notices game start/quit/pause/resume transitions and returns the formatted event for one, or an +// empty string if nothing changed. CPU thread only - it reads the UI state and the param SFO, and +// is what lets connected debuggers hear about this without touching either from their own threads. +// Must be called even when no debugger is connected, so the transition state doesn't go stale. +std::string PollChange(); - void Broadcast(net::WebSocketServer *ws); - -private: - GlobalUIState prevState_; -}; +} // namespace GameBroadcaster diff --git a/Core/Debugger/WebSocket/GameSubscriber.cpp b/Core/Debugger/WebSocket/GameSubscriber.cpp index 007fb12ef7..cf958709aa 100644 --- a/Core/Debugger/WebSocket/GameSubscriber.cpp +++ b/Core/Debugger/WebSocket/GameSubscriber.cpp @@ -31,6 +31,7 @@ #include "Common/System/System.h" #include "Core/Config.h" +#include "Core/Core.h" #include "Core/CoreParameter.h" #include "Core/Debugger/WebSocket/GameSubscriber.h" #include "Core/Debugger/WebSocket/WebSocketUtils.h" @@ -64,15 +65,21 @@ DebuggerSubscriber *WebSocketGameInit(DebuggerEventHandlerMap &map) { // // Response (same event name) with no extra data or error. void WebSocketGameReset(DebuggerRequest &req) { - if (PSP_GetBootState() != BootState::Complete) - return req.Fail("Game not running"); - bool needBreak = false; if (!req.ParamBool("break", &needBreak, DebuggerParamType::OPTIONAL)) return; - if (needBreak) - PSP_CoreParameter().startBreak = true; + // Route the boot-state check and the startBreak write to the CPU thread instead of poking at + // them directly from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. + bool running = false; + Core_RunOnCPUThread([&] { + running = PSP_GetBootState() == BootState::Complete; + if (running && needBreak) + PSP_CoreParameter().startBreak = true; + }); + + if (!running) + return req.Fail("Game not running"); // We can only support async resets here. A lot of the stuff in init must happen on the EmuThread, // and we are not on it here. @@ -92,17 +99,21 @@ void WebSocketGameReset(DebuggerRequest &req) { // - title: string game title. // - paused: boolean, true when gameplay is paused (not the same as stepping.) void WebSocketGameStatus(DebuggerRequest &req) { - JsonWriter &json = req.Respond(); - if (PSP_GetBootState() == BootState::Complete) { - json.pushDict("game"); - json.writeString("id", g_paramSFO.GetDiscID()); - json.writeString("version", g_paramSFO.GetValueString("DISC_VERSION")); - json.writeString("title", g_paramSFO.GetValueString("TITLE")); - json.pop(); - } else { - json.writeNull("game"); - } - json.writeBool("paused", GetUIState() == UISTATE_PAUSEMENU); + // Route the boot state and param SFO reads to the CPU thread instead of poking at them directly + // from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. + Core_RunOnCPUThread([&] { + JsonWriter &json = req.Respond(); + if (PSP_GetBootState() == BootState::Complete) { + json.pushDict("game"); + json.writeString("id", g_paramSFO.GetDiscID()); + json.writeString("version", g_paramSFO.GetValueString("DISC_VERSION")); + json.writeString("title", g_paramSFO.GetValueString("TITLE")); + json.pop(); + } else { + json.writeNull("game"); + } + json.writeBool("paused", GetUIState() == UISTATE_PAUSEMENU); + }); } // Notify debugger version info (version) @@ -121,8 +132,6 @@ void WebSocketGameStatus(DebuggerRequest &req) { // meant to attach to. Ports aren't enough on their own: a leftover process may still be holding // the one you asked for, and you'd never know you were driving the wrong emulator. void WebSocketVersion(DebuggerRequest &req) { - JsonWriter &json = req.Respond(); - std::string version = req.client->version; if (!req.ParamString("version", &version, DebuggerParamType::OPTIONAL_LOOSE)) return; @@ -133,14 +142,19 @@ void WebSocketVersion(DebuggerRequest &req) { req.client->version = version; req.client->name = name; + // fileToStart is CPU-thread-owned (PSP_Shutdown clears it), so read it over there - see + // Core_RunOnCPUThread() in Core.h. The rest is constant and safe to read here. + std::string path; + Core_RunOnCPUThread([&] { + path = PSP_CoreParameter().fileToStart.ToString(); + }); + + JsonWriter &json = req.Respond(); json.writeString("name", "PPSSPP"); json.writeString("version", PPSSPP_GIT_VERSION); - json.writeUint("pid", GetOwnProcessID()); - - const Path &fileToStart = PSP_CoreParameter().fileToStart; - if (fileToStart.empty()) + if (path.empty()) json.writeNull("path"); else - json.writeString("path", fileToStart.ToString()); + json.writeString("path", path); } diff --git a/Core/Debugger/WebSocket/MemoryInfoSubscriber.cpp b/Core/Debugger/WebSocket/MemoryInfoSubscriber.cpp index 29d647a917..93b1790b9f 100644 --- a/Core/Debugger/WebSocket/MemoryInfoSubscriber.cpp +++ b/Core/Debugger/WebSocket/MemoryInfoSubscriber.cpp @@ -18,6 +18,7 @@ #include #include "Core/MIPS/MIPS.h" #include "Core/MIPS/MIPSDebugInterface.h" +#include "Core/Core.h" #include "Core/Debugger/MemBlockInfo.h" #include "Core/Debugger/WebSocket/MemoryInfoSubscriber.h" #include "Core/Debugger/WebSocket/WebSocketUtils.h" @@ -141,11 +142,17 @@ void WebSocketMemoryInfoState::Config(DebuggerRequest &req) { if (!req.ParamBool("detailed", &detailed, DebuggerParamType::OPTIONAL)) return; - JsonWriter &json = req.Respond(); - json.writeBool("detailed", MemBlockInfoDetailed()); + // MemBlockInfo's detail tracking is CPU-thread-owned, so flip it over there rather than from + // this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. + bool nowDetailed = false; + Core_RunOnCPUThread([&] { + if (setDetailed) + UpdateOverride(detailed); + nowDetailed = MemBlockInfoDetailed(); + }); - if (setDetailed) - UpdateOverride(detailed); + JsonWriter &json = req.Respond(); + json.writeBool("detailed", nowDetailed); } static MemBlockFlags FlagFromType(const std::string &type) { @@ -196,9 +203,6 @@ static std::string TypeFromFlag(const MemBlockFlags &flag) { // Note: Only one tag per type is maintained for any given memory address. // Small extent info may be ignored unless detailed tracking enabled (see memory.info.config.) void WebSocketMemoryInfoState::Set(DebuggerRequest &req) { - if (!currentDebugMIPS->isAlive() || !Memory::IsActive()) - return req.Fail("CPU not started"); - std::string type; if (!req.ParamString("type", &type)) return; @@ -213,20 +217,37 @@ void WebSocketMemoryInfoState::Set(DebuggerRequest &req) { uint32_t size; if (!req.ParamU32("size", &size)) return; - uint32_t pc = currentMIPS->pc; - if (!req.ParamU32("pc", &pc, false, DebuggerParamType::OPTIONAL)) + // Defaults to the current PC, but that has to be read on the CPU thread - see below. + const bool hasPC = req.HasParam("pc"); + uint32_t pc = 0; + if (hasPC && !req.ParamU32("pc", &pc, false, DebuggerParamType::OPTIONAL)) return; MemBlockFlags flags = MemBlockFlags::SKIP_MEMCHECK | FlagFromType(type); if (flags == MemBlockFlags::SKIP_MEMCHECK) return req.Fail("Invaid type - expecting write, texture, alloc, suballoc, free, or subfree"); - if (!Memory::IsValidAddress(addr)) - return req.Fail("Invalid address"); - else if (!Memory::IsValidRange(addr, size)) - return req.Fail("Invalid size"); + // Everything below this point reads or writes CPU-thread-owned state, so do it over there - + // see Core_RunOnCPUThread() in Core.h. The validity checks come along for the ride, since + // answering them out here would just mean acting on a stale answer. + const char *failure = nullptr; + Core_RunOnCPUThread([&] { + if (!currentDebugMIPS->isAlive() || !Memory::IsActive()) + failure = "CPU not started"; + else if (!Memory::IsValidAddress(addr)) + failure = "Invalid address"; + else if (!Memory::IsValidRange(addr, size)) + failure = "Invalid size"; + if (failure) + return; + + if (!hasPC) + pc = currentMIPS->pc; + NotifyMemInfoPC(flags, addr, size, pc, tag.c_str(), tag.size()); + }); + if (failure) + return req.Fail(failure); - NotifyMemInfoPC(flags, addr, size, pc, tag.c_str(), tag.size()); req.Respond(); } @@ -251,9 +272,6 @@ void WebSocketMemoryInfoState::Set(DebuggerRequest &req) { // - tag: string tag for this memory extent. // - allocated: boolean, if this extent is marked as allocated (for alloc/suballoc types.) void WebSocketMemoryInfoState::List(DebuggerRequest &req) { - if (!currentDebugMIPS->isAlive() || !Memory::IsActive()) - return req.Fail("CPU not started"); - std::string type; if (!req.ParamString("type", &type, DebuggerParamType::OPTIONAL)) return; @@ -269,16 +287,27 @@ void WebSocketMemoryInfoState::List(DebuggerRequest &req) { if (flags == MemBlockFlags::SKIP_MEMCHECK && req.HasParam("type")) return req.Fail("Invaid type - expecting write, texture, alloc, suballoc, free, or subfree"); - if (!Memory::IsValidAddress(addr)) - return req.Fail("Invalid address"); - else if (!Memory::IsValidRange(addr, size)) - return req.Fail("Invalid size"); - + // The slab maps FindMemInfo walks are CPU-thread-owned, so gather over there rather than from + // this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. + const char *failure = nullptr; std::vector results; - if (flags == MemBlockFlags::SKIP_MEMCHECK) - results = FindMemInfo(addr, size); - else - results = FindMemInfoByFlag(flags, addr, size); + Core_RunOnCPUThread([&] { + if (!currentDebugMIPS->isAlive() || !Memory::IsActive()) + failure = "CPU not started"; + else if (!Memory::IsValidAddress(addr)) + failure = "Invalid address"; + else if (!Memory::IsValidRange(addr, size)) + failure = "Invalid size"; + if (failure) + return; + + if (flags == MemBlockFlags::SKIP_MEMCHECK) + results = FindMemInfo(addr, size); + else + results = FindMemInfoByFlag(flags, addr, size); + }); + if (failure) + return req.Fail(failure); JsonWriter &json = req.Respond(); json.pushArray("extents"); @@ -320,9 +349,6 @@ void WebSocketMemoryInfoState::List(DebuggerRequest &req) { // // Note: may not be fast. void WebSocketMemoryInfoState::Search(DebuggerRequest &req) { - if (!currentDebugMIPS->isAlive() || !Memory::IsActive()) - return req.Fail("CPU not started"); - uint32_t start = 0; if (!req.ParamU32("address", &start, false, DebuggerParamType::OPTIONAL)) return; @@ -346,34 +372,47 @@ void WebSocketMemoryInfoState::Search(DebuggerRequest &req) { std::transform(match.begin(), match.end(), match.begin(), ::tolower); bool found = false; + bool alive = false; MemBlockInfo foundResult; - uint32_t addr = start; - constexpr uint32_t CHUNK_SIZE = 0x1000; - do { - uint32_t chunk_end = addr + CHUNK_SIZE; - if (addr < end && chunk_end >= end) { - chunk_end = end; - } + // The whole scan happens in one trip to the CPU thread - see Core_RunOnCPUThread() in Core.h. + // It can cover a fair bit of memory and so will stall emulation briefly, which is fine for + // something a human triggers by hand. (The chunking below is just to avoid gathering every + // extent in the range before looking at any of them.) + Core_RunOnCPUThread([&] { + alive = currentDebugMIPS->isAlive() && Memory::IsActive(); + if (!alive) + return; - std::vector results; - if (flags == MemBlockFlags::SKIP_MEMCHECK) - results = FindMemInfo(addr, chunk_end - addr); - else - results = FindMemInfoByFlag(flags, addr, chunk_end - addr); - - for (const auto &result : results) { - std::string lowercase = result.tag; - std::transform(lowercase.begin(), lowercase.end(), lowercase.begin(), ::tolower); - - if (lowercase.find(match) != lowercase.npos) { - found = true; - foundResult = result; - break; + uint32_t addr = start; + constexpr uint32_t CHUNK_SIZE = 0x1000; + do { + uint32_t chunk_end = addr + CHUNK_SIZE; + if (addr < end && chunk_end >= end) { + chunk_end = end; } - } - addr = RoundMemAddressUp(chunk_end); - } while (!found && addr != end); + + std::vector results; + if (flags == MemBlockFlags::SKIP_MEMCHECK) + results = FindMemInfo(addr, chunk_end - addr); + else + results = FindMemInfoByFlag(flags, addr, chunk_end - addr); + + for (const auto &result : results) { + std::string lowercase = result.tag; + std::transform(lowercase.begin(), lowercase.end(), lowercase.begin(), ::tolower); + + if (lowercase.find(match) != lowercase.npos) { + found = true; + foundResult = result; + break; + } + } + addr = RoundMemAddressUp(chunk_end); + } while (!found && addr != end); + }); + if (!alive) + return req.Fail("CPU not started"); JsonWriter &json = req.Respond(); if (found) { diff --git a/Core/Debugger/WebSocket/MemorySubscriber.cpp b/Core/Debugger/WebSocket/MemorySubscriber.cpp index 7b260ca2a7..2f63cbf064 100644 --- a/Core/Debugger/WebSocket/MemorySubscriber.cpp +++ b/Core/Debugger/WebSocket/MemorySubscriber.cpp @@ -63,7 +63,7 @@ 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 +// Deliberately does NOT take a CoreShutdownLock: 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. // diff --git a/Core/Debugger/WebSocket/ReplaySubscriber.cpp b/Core/Debugger/WebSocket/ReplaySubscriber.cpp index 9d518630eb..3bc54aaac8 100644 --- a/Core/Debugger/WebSocket/ReplaySubscriber.cpp +++ b/Core/Debugger/WebSocket/ReplaySubscriber.cpp @@ -19,6 +19,7 @@ #include "Common/Data/Encoding/Base64.h" #include "Common/Swap.h" #include "Core/HLE/sceRtc.h" +#include "Core/Core.h" #include "Core/Replay.h" #include "Core/System.h" #include "Core/Debugger/WebSocket/ReplaySubscriber.h" @@ -45,7 +46,12 @@ DebuggerSubscriber *WebSocketReplayInit(DebuggerEventHandlerMap &map) { // // Response (same event name) with no extra data. void WebSocketReplayBegin(DebuggerRequest &req) { - ReplayBeginSave(); + // Replay state is consumed by the CPU thread as it runs, so mutate it over there rather than + // from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h. Same for the rest + // of the handlers below. + Core_RunOnCPUThread([&] { + ReplayBeginSave(); + }); req.Respond(); } @@ -57,7 +63,9 @@ void WebSocketReplayBegin(DebuggerRequest &req) { // // Response (same event name) with no extra data. void WebSocketReplayAbort(DebuggerRequest &req) { - ReplayAbort(); + Core_RunOnCPUThread([&] { + ReplayAbort(); + }); req.Respond(); } @@ -71,11 +79,15 @@ void WebSocketReplayAbort(DebuggerRequest &req) { // - version: unsigned integer, version number of data. // - base64: base64 encode of binary data. void WebSocketReplayFlush(DebuggerRequest &req) { - if (PSP_GetBootState() != BootState::Complete) - return req.Fail("Game not running"); - + bool running = false; std::vector data; - ReplayFlushBlob(&data); + Core_RunOnCPUThread([&] { + running = PSP_GetBootState() == BootState::Complete; + if (running) + ReplayFlushBlob(&data); + }); + if (!running) + return req.Fail("Game not running"); JsonWriter &json = req.Respond(); json.writeInt("version", ReplayVersion()); @@ -90,9 +102,6 @@ void WebSocketReplayFlush(DebuggerRequest &req) { // // Response (same event name) with no extra data. void WebSocketReplayExecute(DebuggerRequest &req) { - if (PSP_GetBootState() != BootState::Complete) - return req.Fail("Game not running"); - uint32_t version = -1; if (!req.ParamU32("version", &version)) return; @@ -101,7 +110,16 @@ void WebSocketReplayExecute(DebuggerRequest &req) { return; std::vector data = Base64Decode(encoded.data(), encoded.size()); - if (!ReplayExecuteBlob(version, data)) + bool running = false; + bool ok = false; + Core_RunOnCPUThread([&] { + running = PSP_GetBootState() == BootState::Complete; + if (running) + ok = ReplayExecuteBlob(version, data); + }); + if (!running) + return req.Fail("Game not running"); + if (!ok) return req.Fail("Invalid replay data or version"); req.Respond(); @@ -115,9 +133,15 @@ void WebSocketReplayExecute(DebuggerRequest &req) { // - executing: boolean if a replay is being executed. // - saving: boolean if a replay is being recorded. void WebSocketReplayStatus(DebuggerRequest &req) { + bool executing = false, saving = false; + Core_RunOnCPUThread([&] { + executing = ReplayIsExecuting(); + saving = ReplayIsSaving(); + }); + JsonWriter &json = req.Respond(); - json.writeBool("executing", ReplayIsExecuting()); - json.writeBool("saving", ReplayIsSaving()); + json.writeBool("executing", executing); + json.writeBool("saving", saving); } // Get the base RTC (real time clock) time for replay data (replay.time.get) @@ -130,11 +154,18 @@ void WebSocketReplayStatus(DebuggerRequest &req) { // Response (same event name): // - value: unsigned integer, may have more than 32 integer bits. void WebSocketReplayTimeGet(DebuggerRequest &req) { - if (PSP_GetBootState() != BootState::Complete) + bool running = false; + uint32_t baseTime = 0; + Core_RunOnCPUThread([&] { + running = PSP_GetBootState() == BootState::Complete; + if (running) + baseTime = RtcBaseTime(); + }); + if (!running) return req.Fail("Game not running"); JsonWriter &json = req.Respond(); - json.writeUint("value", RtcBaseTime()); + json.writeUint("value", baseTime); } // Overwrite the base RTC time (replay.time.set) @@ -144,14 +175,19 @@ void WebSocketReplayTimeGet(DebuggerRequest &req) { // // Response (same event name) with no extra data. void WebSocketReplayTimeSet(DebuggerRequest &req) { - if (PSP_GetBootState() != BootState::Complete) - return req.Fail("Game not running"); - uint32_t value; if (!req.ParamU32("value", &value, false)) { return; } - RtcSetBaseTime((int32_t)value); + bool running = false; + Core_RunOnCPUThread([&] { + running = PSP_GetBootState() == BootState::Complete; + if (running) + RtcSetBaseTime((int32_t)value); + }); + if (!running) + return req.Fail("Game not running"); + req.Respond(); } diff --git a/Core/Debugger/WebSocket/SteppingBroadcaster.cpp b/Core/Debugger/WebSocket/SteppingBroadcaster.cpp index 0f4fe66fa8..76bef1edb0 100644 --- a/Core/Debugger/WebSocket/SteppingBroadcaster.cpp +++ b/Core/Debugger/WebSocket/SteppingBroadcaster.cpp @@ -56,19 +56,33 @@ private: // CPU has resumed from stepping (cpu.resume) // // Sent unexpectedly with no other properties. -void SteppingBroadcaster::Broadcast(net::WebSocketServer *ws) { - if (PSP_GetBootState() == BootState::Complete) { - int steppingCounter = Core_GetSteppingCounter(); - // We ignore CORE_POWERDOWN as a stepping state. - if (coreState == CORE_STEPPING_CPU && steppingCounter != lastCounter_) { - ws->Send(CPUSteppingEvent(Core_GetSteppingReason())); - } else if (prevState_ == CORE_STEPPING_CPU && coreState != CORE_STEPPING_CPU && Core_IsActive()) { - ws->Send(R"({"event":"cpu.resume"})"); - } - lastCounter_ = steppingCounter; - prevState_ = coreState; - } else { - lastCounter_ = -1; - prevState_ = CORE_POWERDOWN; +// Tracked globally rather than per connection: this runs on the CPU thread, which owns the state +// being read, and the resulting event is then handed to every connected debugger. +static CoreState g_prevState = CORE_POWERDOWN; +static int g_lastCounter = 0; + +std::string SteppingBroadcaster::PollChange() { + if (PSP_GetBootState() != BootState::Complete) { + g_lastCounter = -1; + g_prevState = CORE_POWERDOWN; + return std::string(); } + + std::string result; + const int steppingCounter = Core_GetSteppingCounter(); + // We ignore CORE_POWERDOWN as a stepping state. + if (coreState == CORE_STEPPING_CPU && steppingCounter != g_lastCounter) { + result = CPUSteppingEvent(Core_GetSteppingReason()); + } else if (g_prevState == CORE_STEPPING_CPU && coreState != CORE_STEPPING_CPU && Core_IsActive()) { + result = R"({"event":"cpu.resume"})"; + } + g_lastCounter = steppingCounter; + g_prevState = coreState; + return result; +} + +std::string SteppingBroadcaster::CurrentState() { + if (PSP_GetBootState() != BootState::Complete || coreState != CORE_STEPPING_CPU) + return std::string(); + return CPUSteppingEvent(Core_GetSteppingReason()); } diff --git a/Core/Debugger/WebSocket/SteppingBroadcaster.h b/Core/Debugger/WebSocket/SteppingBroadcaster.h index 4ca34f4552..1a697044e9 100644 --- a/Core/Debugger/WebSocket/SteppingBroadcaster.h +++ b/Core/Debugger/WebSocket/SteppingBroadcaster.h @@ -17,21 +17,18 @@ #pragma once -#include "Core/Core.h" +#include -namespace net { -class WebSocketServer; -} +namespace SteppingBroadcaster { -struct SteppingBroadcaster { -public: - SteppingBroadcaster() { - prevState_ = coreState; - } +// Notices the CPU entering or leaving stepping and returns the formatted event for one, or an empty +// string if nothing changed. CPU thread only - it reads pc and the tick count, which is exactly what +// connected debuggers must not do from their own threads. Must be called even when no debugger is +// connected, so the transition state doesn't go stale. +std::string PollChange(); - void Broadcast(net::WebSocketServer *ws); +// The cpu.stepping event describing the state right now, for a debugger that connected while the +// CPU was already stopped. Empty if it isn't stepping. CPU thread only. +std::string CurrentState(); -private: - CoreState prevState_; - int lastCounter_ = 0; -}; +} // namespace SteppingBroadcaster diff --git a/Core/HW/Display.cpp b/Core/HW/Display.cpp index 6f78bebeeb..f21d442d33 100644 --- a/Core/HW/Display.cpp +++ b/Core/HW/Display.cpp @@ -15,6 +15,7 @@ // Official git repository and contact information can be found at // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/. +#include #include #include #include @@ -41,7 +42,9 @@ typedef std::pair FlipListener; static std::vector flipListeners; static uint64_t frameStartTicks; -static int numVBlanks; +// Atomic because the WebSocket debugger reads it from its own thread (input.buttons.press uses +// it to count down frames) while the CPU thread bumps it. +static std::atomic numVBlanks; // hCount is computed now. static int vCount; // The "AccumulatedHcount" can be adjusted, this is the base. diff --git a/Core/MemMap.cpp b/Core/MemMap.cpp index aecb6aeb8f..06d9596569 100644 --- a/Core/MemMap.cpp +++ b/Core/MemMap.cpp @@ -87,7 +87,6 @@ u32 g_PSPModel; static MemMapSetupFlags g_setupFlags; -std::recursive_mutex g_shutdownLock; // We don't declare the IO region in here since its handled by other means. static MemoryView views[] = { @@ -343,6 +342,9 @@ bool Init(MemMapSetupFlags flags) { void Reinit() { _assert_msg_(PSP_GetBootState() == BootState::Complete, "Cannot reinit during startup/shutdown"); Core_NotifyLifecycle(CoreLifecycle::MEMORY_REINITING); + // Held across both halves: between Shutdown() and Init() there is no memory map at all, and a + // reader that only saw Shutdown()'s own acquire could slip into that gap. + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); MemMapSetupFlags flags = g_setupFlags; Shutdown(); Init(flags); @@ -421,7 +423,7 @@ void DoState(PointerWrap &p) { } void Shutdown() { - std::lock_guard guard(g_shutdownLock); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); u32 flags = 0; MemoryMap_Shutdown(); base = nullptr; @@ -432,21 +434,6 @@ bool IsActive() { return base != nullptr; } -// Wanting to avoid include pollution, MemMap.h is included a lot. -MemoryInitedLock::MemoryInitedLock() -{ - g_shutdownLock.lock(); -} -MemoryInitedLock::~MemoryInitedLock() -{ - g_shutdownLock.unlock(); -} - -MemoryInitedLock Lock() -{ - return MemoryInitedLock(); -} - static Opcode Read_Instruction(u32 address, bool resolveReplacements, Opcode inst) { if (!MIPS_IS_EMUHACK(inst.encoding)) { return inst; diff --git a/Core/MemMap.h b/Core/MemMap.h index 2444f0c6e7..56d8201d0c 100644 --- a/Core/MemMap.h +++ b/Core/MemMap.h @@ -118,16 +118,6 @@ void DoState(PointerWrap &p); // False when shutdown has already been called. bool IsActive(); -class MemoryInitedLock { -public: - MemoryInitedLock(); - ~MemoryInitedLock(); -}; - -// This doesn't lock memory access or anything, it just makes sure memory isn't freed. -// Use it when accessing PSP memory from external threads. -MemoryInitedLock Lock(); - // used by JIT to read instructions. Does not resolve replacements. Opcode Read_Opcode_JIT(const u32 _Address); // used by JIT. Reads in the "Locked cache" mode diff --git a/Core/System.cpp b/Core/System.cpp index 348d951835..f8c8382f8d 100644 --- a/Core/System.cpp +++ b/Core/System.cpp @@ -15,6 +15,7 @@ // Official git repository and contact information can be found at // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/. +#include #include "ppsspp_config.h" #ifdef _WIN32 @@ -102,7 +103,9 @@ static GPUBackend gpuBackend; static std::string gpuBackendDevice; static bool g_fileLoggingWasEnabled; -static BootState g_bootState = BootState::Off; +// Atomic because it's read as a fast-fail from the WebSocket debugger's own thread while the +// CPU and loader threads move it along. +static std::atomic g_bootState = BootState::Off; BootState PSP_GetBootState() { return g_bootState; @@ -565,6 +568,12 @@ static bool CPU_Init(FileLoader *fileLoader, IdentifiedFileType type, std::strin } void CPU_Shutdown(bool success) { + // Held across the whole teardown, not just Memory::Shutdown() further down. Everything below + // frees state the debugger UIs read from other threads - kernel objects, the symbol map, the + // memory map - and this is the lock they take to be sure none of it goes away mid-read. See + // Core_LockAgainstShutdown(); it's recursive, so the nested acquire in Memory::Shutdown() is fine. + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); + UninstallExceptionHandler(); GPURecord::Replay_Unload(); diff --git a/Windows/Debugger/CtrlDisAsmView.cpp b/Windows/Debugger/CtrlDisAsmView.cpp index 219f19626e..92bc260672 100644 --- a/Windows/Debugger/CtrlDisAsmView.cpp +++ b/Windows/Debugger/CtrlDisAsmView.cpp @@ -61,9 +61,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. + // duration, which NativeFrame() also holds while it's actually touching that state, and + // Core_LockAgainstShutdown() so the core can't be torn down mid-read. See g_frameMutex in Core.h. std::lock_guard frameGuard(g_frameMutex); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); g_disassemblyManager.analyze(windowStart, g_disassemblyManager.getNthNextAddress(windowStart,visibleRows)-windowStart); } @@ -243,7 +244,7 @@ std::string trimString(std::string input) void CtrlDisAsmView::assembleOpcode(u32 address, const std::string &defaultText) { - Memory::MemoryInitedLock memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); if (!Core_IsStepping()) { MessageBox(wnd,L"Cannot change code while the core is running!",L"Error",MB_OK); return; @@ -461,9 +462,9 @@ void CtrlDisAsmView::onPaint(WPARAM wParam, LPARAM lParam) // 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. + // g_frameMutex first, then Core_LockAgainstShutdown() - never the other way around. See CtrlMemView::onPaint. std::lock_guard frameGuard(g_frameMutex); - Memory::MemoryInitedLock memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); if (!debugger->isAlive() || Achievements::HardcoreModeActive()) return; PAINTSTRUCT ps; @@ -1195,7 +1196,7 @@ void CtrlDisAsmView::onMouseMove(WPARAM wParam, LPARAM lParam, int button) void CtrlDisAsmView::updateStatusBarText() { - auto memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); if (!PSP_IsInited()) return; @@ -1229,7 +1230,7 @@ void CtrlDisAsmView::calculatePixelPositions() void CtrlDisAsmView::search(bool continueSearch) { - auto memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); u32 searchAddress; if (continueSearch == false || searchQuery[0] == 0) diff --git a/Windows/Debugger/CtrlMemView.cpp b/Windows/Debugger/CtrlMemView.cpp index 84cc271500..4d0a7807ff 100644 --- a/Windows/Debugger/CtrlMemView.cpp +++ b/Windows/Debugger/CtrlMemView.cpp @@ -185,10 +185,10 @@ void CtrlMemView::onPaint(WPARAM wParam, LPARAM lParam) { // 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 + // g_frameMutex first, then Core_LockAgainstShutdown() - 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(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); // draw to a bitmap for double buffering PAINTSTRUCT ps; @@ -424,7 +424,7 @@ void CtrlMemView::onKeyDown(WPARAM wParam, LPARAM lParam) { } void CtrlMemView::onChar(WPARAM wParam, LPARAM lParam) { - Memory::MemoryInitedLock memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); if (!PSP_IsInited()) return; @@ -526,7 +526,7 @@ void CtrlMemView::onMouseUp(WPARAM wParam, LPARAM lParam, int button) { case ID_MEMVIEW_COPYVALUE_8: { - auto memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); size_t tempSize = 3 * selectedSize + 1; char *temp = new char[tempSize]; memset(temp, 0, tempSize); @@ -556,7 +556,7 @@ void CtrlMemView::onMouseUp(WPARAM wParam, LPARAM lParam, int button) { case ID_MEMVIEW_COPYVALUE_16: { - auto memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); size_t tempSize = 5 * ((selectedSize + 1) / 2) + 1; char *temp = new char[tempSize]; memset(temp, 0, tempSize); @@ -577,7 +577,7 @@ void CtrlMemView::onMouseUp(WPARAM wParam, LPARAM lParam, int button) { case ID_MEMVIEW_COPYVALUE_32: { - auto memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); size_t tempSize = 9 * ((selectedSize + 3) / 4) + 1; char *temp = new char[tempSize]; memset(temp, 0, tempSize); @@ -598,7 +598,7 @@ void CtrlMemView::onMouseUp(WPARAM wParam, LPARAM lParam, int button) { case ID_MEMVIEW_COPYFLOAT_32: { - auto memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); std::ostringstream stream; stream << (Memory::IsValid4AlignedAddress(curAddress_) ? Memory::ReadUnchecked_Float(curAddress_) : NAN); auto temp_string = stream.str(); @@ -856,7 +856,7 @@ bool CtrlMemView::ParseSearchString(std::string_view query, bool asHex, std::vec std::vector CtrlMemView::searchString(std::string_view searchQuery) { std::vector searchResAddrs; - auto memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); if (!PSP_IsInited()) return searchResAddrs; @@ -893,7 +893,7 @@ std::vector CtrlMemView::searchString(std::string_view searchQuery) { }; void CtrlMemView::search(bool continueSearch) { - auto memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); if (!PSP_IsInited()) return; diff --git a/Windows/Debugger/CtrlRegisterList.cpp b/Windows/Debugger/CtrlRegisterList.cpp index 8d0f05320f..9ea4db02a4 100644 --- a/Windows/Debugger/CtrlRegisterList.cpp +++ b/Windows/Debugger/CtrlRegisterList.cpp @@ -203,6 +203,7 @@ void CtrlRegisterList::onPaint(WPARAM wParam, LPARAM lParam) // 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); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); // 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(); diff --git a/Windows/Debugger/Debugger_Disasm.cpp b/Windows/Debugger/Debugger_Disasm.cpp index cc7740a6ec..63d54d16de 100644 --- a/Windows/Debugger/Debugger_Disasm.cpp +++ b/Windows/Debugger/Debugger_Disasm.cpp @@ -730,6 +730,7 @@ void CDisasm::Show(bool bShow, bool includeToTop) { // 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); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); g_symbolMap->FillSymbolListBox(GetDlgItem(m_hDlg, IDC_FUNCTIONLIST), ST_FUNCTION); deferredSymbolFill_ = false; } @@ -740,6 +741,7 @@ void CDisasm::Show(bool bShow, bool includeToTop) { void CDisasm::NotifyMapLoaded() { if (m_bShowState != SW_HIDE && g_symbolMap) { std::lock_guard frameGuard(g_frameMutex); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); 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 7c8bcefa96..dd93398f4f 100644 --- a/Windows/Debugger/Debugger_Lists.cpp +++ b/Windows/Debugger/Debugger_Lists.cpp @@ -12,6 +12,7 @@ #include "Windows/main.h" #include "Common/Data/Encoding/Utf8.h" #include "Core/Core.h" +#include "Core/MemMap.h" #include "Core/HLE/sceKernelThread.h" enum { TL_NAME, TL_PROGRAMCOUNTER, TL_ENTRYPOINT, TL_PRIORITY, TL_STATE, TL_WAITTYPE, TL_COLUMNCOUNT }; @@ -260,6 +261,7 @@ void CtrlThreadList::reloadThreads() // while it's actually touching that state. See g_frameMutex in Core.h. { std::lock_guard frameGuard(g_frameMutex); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); threads = GetThreadsInfo(); } Update(); @@ -335,6 +337,7 @@ void CtrlBreakpointList::reloadBreakpoints() // with the CPU thread - see g_frameMutex in Core.h. { std::lock_guard frameGuard(g_frameMutex); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); displayedBreakPoints_ = g_breakpoints.GetBreakpoints(); displayedMemChecks_= g_breakpoints.GetMemChecks(); } @@ -747,9 +750,9 @@ void CtrlStackTraceView::loadStackTrace() { // 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. + // g_frameMutex first, then Core_LockAgainstShutdown() - never the other way around. See CtrlMemView::onPaint. std::lock_guard frameGuard(g_frameMutex); - Memory::MemoryInitedLock memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); if (!PSP_IsInited()) return; @@ -845,6 +848,7 @@ void CtrlModuleList::loadModules() // actually touching that state. See g_frameMutex in Core.h. { std::lock_guard frameGuard(g_frameMutex); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); if (g_symbolMap) { modules = g_symbolMap->getAllModules(); } else { @@ -870,6 +874,7 @@ void CtrlWatchList::RefreshValues() { // 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); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); int steppingCounter = Core_GetSteppingCounter(); int changes = false; diff --git a/Windows/Debugger/DumpMemoryWindow.cpp b/Windows/Debugger/DumpMemoryWindow.cpp index 7d03ddbc3d..495e837108 100644 --- a/Windows/Debugger/DumpMemoryWindow.cpp +++ b/Windows/Debugger/DumpMemoryWindow.cpp @@ -88,7 +88,7 @@ INT_PTR CALLBACK DumpMemoryWindow::dlgFunc(HWND hwnd, UINT iMsg, WPARAM wParam, // queued callback. enum class Outcome { NotInited, OpenFailed, Success } outcome = Outcome::NotInited; Core_RunOnCPUThread([&] { - Memory::MemoryInitedLock memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); if (!PSP_IsInited()) return; diff --git a/Windows/GEDebugger/GEDebugger.cpp b/Windows/GEDebugger/GEDebugger.cpp index d39bf1ed87..b446ffc55d 100644 --- a/Windows/GEDebugger/GEDebugger.cpp +++ b/Windows/GEDebugger/GEDebugger.cpp @@ -552,7 +552,7 @@ void CGEDebugger::PreviewToClipboard(const GPUDebugBuffer *dbgBuffer, bool saveA } void CGEDebugger::UpdatePreviews() { - auto memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); if (!PSP_IsInited()) { return; } diff --git a/Windows/GEDebugger/TabVertices.cpp b/Windows/GEDebugger/TabVertices.cpp index 3feaf56c21..cff41a1b11 100644 --- a/Windows/GEDebugger/TabVertices.cpp +++ b/Windows/GEDebugger/TabVertices.cpp @@ -186,7 +186,7 @@ void CtrlVertexList::GetColumnText(wchar_t *dest, size_t destSize, int row, int } int CtrlVertexList::GetRowCount() { - auto memLock = Memory::Lock(); + CoreShutdownLock coreLock = Core_LockAgainstShutdown(); if (!PSP_IsInited()) { return 0; } diff --git a/headless/Headless.cpp b/headless/Headless.cpp index ba2ef8b647..b583eb6a22 100644 --- a/headless/Headless.cpp +++ b/headless/Headless.cpp @@ -38,6 +38,7 @@ #include #endif #include "Common/CPUDetect.h" +#include "Common/ExceptionHandlerSetup.h" #include "Common/File/VFS/VFS.h" #include "Common/File/VFS/ZipFileReader.h" #include "Common/File/VFS/DirectoryReader.h" @@ -520,35 +521,6 @@ public: } }; -// Has a parameter because we might start using this from the main build. -void SetupCRT(bool headless) { -#if defined(_MSC_VER) - _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF); - - if (headless) { - // Suppress abort dialogs and similar. - // 1. Redirect CRT Assertions/Errors/Warnings to stdout/stderr - - const _HFILE reportTarget = _CRTDBG_FILE_STDERR; - - _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE); - _CrtSetReportFile(_CRT_ASSERT, reportTarget); - - _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE); - _CrtSetReportFile(_CRT_ERROR, reportTarget); - - _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE); - _CrtSetReportFile(_CRT_WARN, reportTarget); - - // 2. Suppress the abort() message box & crash reporting dialogs - _set_abort_behavior(0, _WRITE_ABORT_MSG | _CALL_REPORTFAULT); - - // 3. Suppress Windows OS-level "Program has stopped working" modal dialogs - SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX); - } -#endif -} - int main(int argc, const char* argv[]) { PROFILE_INIT(); TimeInit(); diff --git a/libretro/libretro.cpp b/libretro/libretro.cpp index 974f78d50c..62815a7b89 100644 --- a/libretro/libretro.cpp +++ b/libretro/libretro.cpp @@ -1984,7 +1984,7 @@ void System_PostUIMessage(UIMessage message, std::string_view param) {} void System_RunOnMainThread(std::function) {} void NativeFrame(GraphicsContext *graphicsContext) {} void NativeResized() {} - +void WebSocketDebuggerTick() {} // stub to let things link void System_Toast(std::string_view str) {} inline int16_t Clamp16(int32_t sample) { diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index 2859a87dff..b28ecf8a5f 100644 --- a/unittest/UnitTest.cpp +++ b/unittest/UnitTest.cpp @@ -77,6 +77,7 @@ #include "Common/ArmEmitter.h" #include "Common/BitScan.h" #include "Common/CPUDetect.h" +#include "Common/ExceptionHandlerSetup.h" #include "Common/Log.h" #include "Common/StringUtils.h" #include "Core/Config.h" @@ -1762,6 +1763,8 @@ TestItem availableTests[] = { }; int main(int argc, const char *argv[]) { + // Never block on a modal dialog - these get run from CI and from tooling. + SetupCRT(true); SetCurrentThreadName("UnitTest"); TimeInit();