diff --git a/Core/Core.cpp b/Core/Core.cpp index 295288ba0a..cb5a5f8d79 100644 --- a/Core/Core.cpp +++ b/Core/Core.cpp @@ -190,6 +190,9 @@ volatile bool coreStatePending = false; static bool powerSaving = false; static bool g_breakAfterFrame = false; static BreakReason g_breakReason = BreakReason::None; +// Detail about the breakpoint that caused the current break, if it was one. Guarded by g_stepMutex +// alongside g_cpuStepCommand, which is what it belongs to. +static BreakpointHit g_breakHit; static MIPSExceptionInfo g_exceptionInfo; @@ -587,7 +590,7 @@ static bool Core_ProcessStepping(MIPSDebugInterface *cpu) { } // Free-threaded (hm, possibly except tracing). -void Core_Break(BreakReason reason, u32 relatedAddress) { +void Core_Break(BreakReason reason, u32 relatedAddress, const BreakpointHit *hit) { const CoreState state = coreState; if (state != CORE_RUNNING_CPU) { if (state == CORE_STEPPING_CPU) { @@ -628,6 +631,12 @@ void Core_Break(BreakReason reason, u32 relatedAddress) { CoreTiming::SetBreakDeadlineUs(0); g_breakReason = reason; + // Cleared rather than left alone when there's no hit, so the detail from an earlier + // breakpoint can't be reported against, say, the user pressing pause afterwards. + if (hit) + g_breakHit = *hit; + else + g_breakHit = BreakpointHit{}; g_cpuStepCommand.type = CPUStepType::None; g_cpuStepCommand.reason = reason; g_cpuStepCommand.relatedAddr = relatedAddress; @@ -694,6 +703,7 @@ SteppingReason Core_GetSteppingReason() { // genuinely nothing to report. r.reason = g_cpuStepCommand.reason; r.relatedAddress = g_cpuStepCommand.relatedAddr; + r.hit = g_breakHit; return r; } diff --git a/Core/Core.h b/Core/Core.h index ffba3caf63..5f0865b6cf 100644 --- a/Core/Core.h +++ b/Core/Core.h @@ -20,6 +20,7 @@ #include #include #include +#include #include #include "Common/CommonTypes.h" @@ -77,8 +78,45 @@ enum class BreakReason { }; const char *BreakReasonToString(BreakReason reason); +enum class BreakpointKind { + None, + Exec, + Memory, + Register, +}; + +// What tripped a breakpoint, captured where it happened. +// +// All three kinds know a lot more than the single address Core_Break() carries - which register, +// which byte of a watched range, read or write, how big - and until this existed it was formatted +// straight into a log line and discarded, so a debugger over the wire couldn't see any of it. For +// a memcheck in particular the address that reached the client was the start of the watched range, +// not the address actually touched. +struct BreakpointHit { + BreakpointKind kind = BreakpointKind::None; + u32 pc = 0; // The instruction responsible. + u32 address = 0; // Exec: the instruction itself. Memory: the address actually accessed. + int size = 0; // Memory only, in bytes. + bool write = false; // Memory only. + int reg = -1; // Register only: a GPR index. + // Which breakpoint this was, so a client can match it against cpu.breakpoint.list and friends. + // For a memcheck that's the watched range, which is exactly what 'address' is not. + u32 rangeStart = 0; + u32 rangeEnd = 0; + u32 numHits = 0; + bool logged = false; // Had the LOG action. + bool paused = false; // Had the PAUSE action, so the CPU stopped for it. + std::string condition; // Empty when unconditional. + // Memory only: who performed the access - "interpret", "CPU", "HLE", or an allocation tag. + // Copied rather than kept as a pointer; callers pass buffers that are gone by the time the + // event gets formatted. + std::string source; +}; + // Async, called from gui -void Core_Break(BreakReason reason, u32 relatedAddress = 0); +// hit is optional detail for the breakpoint kinds, forwarded to the debugger. Only stored when +// the break actually takes effect, so a rejected Core_Break() can't leave a stale one behind. +void Core_Break(BreakReason reason, u32 relatedAddress = 0, const BreakpointHit *hit = nullptr); // Resumes execution. Works both when stepping the CPU and the GE. void Core_Resume(); @@ -98,6 +136,8 @@ int Core_GetSteppingCounter(); struct SteppingReason { BreakReason reason; u32 relatedAddress = 0; + // Only filled in when the break came from a breakpoint - kind is None otherwise. + BreakpointHit hit; }; SteppingReason Core_GetSteppingReason(); diff --git a/Core/Debugger/Breakpoints.cpp b/Core/Debugger/Breakpoints.cpp index de051cfa7e..9282a0d7c8 100644 --- a/Core/Debugger/Breakpoints.cpp +++ b/Core/Debugger/Breakpoints.cpp @@ -20,6 +20,7 @@ #include "Common/System/System.h" #include "Common/Log.h" #include "Core/Core.h" +#include "Core/Debugger/WebSocket.h" #include "Core/Debugger/Breakpoints.h" #include "Core/Debugger/MemBlockInfo.h" #include "Core/Debugger/SymbolMap.h" @@ -62,8 +63,32 @@ BreakAction MemCheck::Apply(u32 addr, bool write, int size, u32 pc) { BreakAction MemCheck::Action(u32 addr, bool write, int size, u32 pc, const char *reason) { // Conditions have always already been checked if we get here. Log(addr, write, size, pc, reason); + + BreakpointHit hit; + if (WebSocketDebuggerHasClients() || (action & BREAK_ACTION_PAUSE)) { + hit.kind = BreakpointKind::Memory; + hit.pc = pc; + hit.address = addr; + hit.size = size; + hit.write = write; + hit.rangeStart = start; + hit.rangeEnd = end; + // This is a copy of the stored memcheck, taken after Apply() bumped the count, so it's + // already the post-hit value. + hit.numHits = numHits; + hit.logged = (action & BREAK_ACTION_LOG) != 0; + hit.paused = (action & BREAK_ACTION_PAUSE) != 0; + if (hasCondition) + hit.condition = condition.expressionString; + if (reason) + hit.source = reason; + WebSocketNotifyBreakpointHit(hit); + } + if (action & BREAK_ACTION_PAUSE) { - Core_Break(BreakReason::MemoryBreakpoint, start); + // relatedAddress stays the range start for compatibility - the address actually touched + // is in the hit, which is the whole point of it. + Core_Break(BreakReason::MemoryBreakpoint, start, &hit); } return action; } @@ -329,6 +354,7 @@ BreakAction BreakpointManager::ExecBreakPoint(u32 addr) { return BREAK_ACTION_NONE; BreakAction result = BREAK_ACTION_NONE; + BreakpointHit hit; size_t bp = FindBreakpoint(addr); if (bp != INVALID_BREAKPOINT) { @@ -342,6 +368,20 @@ BreakAction BreakpointManager::ExecBreakPoint(u32 addr) { if (condPassed) { ++info.numHits; + if (action != BREAK_ACTION_NONE && (WebSocketDebuggerHasClients() || (action & BREAK_ACTION_PAUSE))) { + hit.kind = BreakpointKind::Exec; + hit.pc = addr; + hit.address = addr; + hit.rangeStart = addr; + hit.rangeEnd = addr; + hit.numHits = info.numHits; + hit.logged = (action & BREAK_ACTION_LOG) != 0; + hit.paused = (action & BREAK_ACTION_PAUSE) != 0; + if (info.hasCond) + hit.condition = info.cond.expressionString; + WebSocketNotifyBreakpointHit(hit); + } + if (action & BREAK_ACTION_LOG) { if (info.logFormat.empty()) { NOTICE_LOG(Log::JIT, "BKP PC=%08x (%s)", addr, g_symbolMap->GetDescription(addr).c_str()); @@ -367,7 +407,9 @@ BreakAction BreakpointManager::ExecBreakPoint(u32 addr) { } if (result & BREAK_ACTION_PAUSE) { - Core_Break(BreakReason::CpuBreakpoint, addr); + // hit stays kind None when only the temporary breakpoint fired - there's no user + // breakpoint to describe in that case, just a step completing. + Core_Break(BreakReason::CpuBreakpoint, addr, hit.kind != BreakpointKind::None ? &hit : nullptr); System_Notify(SystemNotification::DISASSEMBLY); } @@ -698,6 +740,20 @@ BreakAction BreakpointManager::ExecRegBreakpoint(int reg, u32 pc) { ++info.numHits; + BreakpointHit hit; + if (WebSocketDebuggerHasClients() || (info.result & BREAK_ACTION_PAUSE)) { + hit.kind = BreakpointKind::Register; + hit.pc = pc; + hit.address = pc; + hit.reg = reg; + hit.numHits = info.numHits; + hit.logged = (info.result & BREAK_ACTION_LOG) != 0; + hit.paused = (info.result & BREAK_ACTION_PAUSE) != 0; + if (info.hasCond) + hit.condition = info.cond.expressionString; + WebSocketNotifyBreakpointHit(hit); + } + if (info.result & BREAK_ACTION_LOG) { if (info.logFormat.empty()) { NOTICE_LOG(Log::JIT, "BKP reg write r%d, PC=%08x (%s)", reg, pc, g_symbolMap->GetDescription(pc).c_str()); @@ -708,7 +764,7 @@ BreakAction BreakpointManager::ExecRegBreakpoint(int reg, u32 pc) { } } if ((info.result & BREAK_ACTION_PAUSE) && g_breakpoints.CheckSkipFirst() != pc) { - Core_Break(BreakReason::RegBreakpoint, pc); + Core_Break(BreakReason::RegBreakpoint, pc, &hit); } return info.result; diff --git a/Core/Debugger/WebSocket.cpp b/Core/Debugger/WebSocket.cpp index f8ba7c34b3..e0d9ecf999 100644 --- a/Core/Debugger/WebSocket.cpp +++ b/Core/Debugger/WebSocket.cpp @@ -16,6 +16,7 @@ // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/. #include +#include #include #include #include @@ -117,6 +118,13 @@ static void UpdateConnected(int delta) { // 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. +// A log-only breakpoint in a hot loop can produce events far faster than a connection drains them +// (the drain runs once per lap of ws->Process, so at best a few hundred times a second). Without a +// cap the queue grows without bound and the connection falls further and further behind. Dropping +// is the only sane answer; cpu.breakpoint.hit carries a sequence number so a client can tell +// exactly how many it missed rather than silently believing it saw everything. +static constexpr size_t MAX_PENDING_EVENTS = 4096; + struct DebuggerEventSink { std::mutex lock; std::vector> pending; @@ -125,6 +133,8 @@ struct DebuggerEventSink { void Push(const char *category, std::string json) { std::lock_guard guard(lock); + if (pending.size() >= MAX_PENDING_EVENTS) + return; pending.emplace_back(category, std::move(json)); } @@ -137,15 +147,47 @@ struct DebuggerEventSink { static std::mutex g_sinkLock; static std::vector g_sinks; +// Mirrors g_sinks.size() so the breakpoint path can check "is anyone listening" with one relaxed +// load, instead of taking g_sinkLock on every single breakpoint hit. +static std::atomic g_sinkCount{ 0 }; static void RegisterSink(DebuggerEventSink *sink) { std::lock_guard guard(g_sinkLock); g_sinks.push_back(sink); + g_sinkCount.store((int)g_sinks.size(), std::memory_order_relaxed); } 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()); + g_sinkCount.store((int)g_sinks.size(), std::memory_order_relaxed); +} + +bool WebSocketDebuggerHasClients() { + return g_sinkCount.load(std::memory_order_relaxed) != 0; +} + +void WebSocketNotifyBreakpointHit(const BreakpointHit &hit) { + // Counts hits produced, not hits delivered, so a gap in what a client receives tells it how + // many were dropped by the cap in Push(). + static uint64_t g_hitSequence = 0; + + std::lock_guard guard(g_sinkLock); + if (g_sinks.empty()) + return; + + // Formatted once here on the CPU thread, then shared - same rule as the other pushed events: + // a connection's own thread must never be the one reading emulator state. + JsonWriter j; + j.begin(); + j.writeString("event", "cpu.breakpoint.hit"); + j.writeFloat("sequence", (double)++g_hitSequence); + WriteBreakpointHit(j, hit); + j.end(); + const std::string json = j.str(); + + for (DebuggerEventSink *sink : g_sinks) + sink->Push("breakpoint", json); } void WebSocketDebuggerTick() { @@ -193,7 +235,7 @@ void HandleDebuggerRequest(const http::ServerRequest &request) { // as a side effect of operator[] the first time each category actually broadcasts - which // meant "game" and "stepping" were rejected as unsupported until one happened to fire, even // though they're documented and valid. Keep in sync with the Broadcast calls further down. - for (const char *category : { "logger", "input", "game", "stepping" }) + for (const char *category : { "logger", "input", "game", "stepping", "breakpoint" }) disallowed_config[category] = false; LogBroadcaster logger; diff --git a/Core/Debugger/WebSocket.h b/Core/Debugger/WebSocket.h index ef6c823c8b..d6f9569a73 100644 --- a/Core/Debugger/WebSocket.h +++ b/Core/Debugger/WebSocket.h @@ -29,3 +29,15 @@ void StopAllDebuggers(); // 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(); + +struct BreakpointHit; + +// Whether it's worth building a BreakpointHit at all. False whenever no debugger is connected, +// which is the overwhelmingly common case and the one that must stay free - a log-only breakpoint +// in a hot loop runs this per hit. +bool WebSocketDebuggerHasClients(); + +// Reports a breakpoint hit to every connected debugger as "cpu.breakpoint.hit", whether or not it +// stopped the CPU. That's what makes log-only breakpoints usable for automation: previously their +// only trace was a log line. CPU thread only. +void WebSocketNotifyBreakpointHit(const BreakpointHit &hit); diff --git a/Core/Debugger/WebSocket/BreakpointSubscriber.cpp b/Core/Debugger/WebSocket/BreakpointSubscriber.cpp index 1b69eb9109..4dbe2d43d0 100644 --- a/Core/Debugger/WebSocket/BreakpointSubscriber.cpp +++ b/Core/Debugger/WebSocket/BreakpointSubscriber.cpp @@ -24,6 +24,60 @@ #include "Core/Debugger/WebSocket/WebSocketUtils.h" #include "Core/MIPS/MIPSDebugInterface.h" +static const char *BreakpointKindToString(BreakpointKind kind) { + switch (kind) { + case BreakpointKind::Exec: return "exec"; + case BreakpointKind::Memory: return "memory"; + case BreakpointKind::Register: return "register"; + default: return "none"; + } +} + +void WriteBreakpointHit(JsonWriter &json, const BreakpointHit &hit) { + json.pushDict("hit"); + json.writeString("kind", BreakpointKindToString(hit.kind)); + json.writeUint("pc", hit.pc); + json.writeUint("address", hit.address); + json.writeUint("hits", hit.numHits); + json.writeBool("logged", hit.logged); + json.writeBool("paused", hit.paused); + if (hit.condition.empty()) + json.writeNull("condition"); + else + json.writeString("condition", hit.condition); + + // Resolved here rather than left to the client: it's one symbol map lookup at break time, and + // it saves a round trip at exactly the moment the client is trying to show something. + const std::string symbol = g_symbolMap->GetDescription(hit.address); + if (symbol.empty()) + json.writeNull("symbol"); + else + json.writeString("symbol", symbol); + + if (hit.kind == BreakpointKind::Memory) { + json.writeInt("size", hit.size); + json.writeString("access", hit.write ? "write" : "read"); + json.writeString("source", hit.source); + } + if (hit.kind == BreakpointKind::Register) { + json.writeInt("register", hit.reg); + json.writeString("registerName", MIPSDebugInterface::GetRegName(0, hit.reg)); + } + + // Which breakpoint it was, as opposed to what was touched. Those differ for every memcheck + // with a range, and this is what matches the entries in cpu.breakpoint.list / etc. A register + // breakpoint isn't identified by an address at all, so it gets no range rather than a + // meaningless zero one - "register" above is its identity. + if (hit.kind == BreakpointKind::Exec || hit.kind == BreakpointKind::Memory) { + json.pushDict("breakpoint"); + json.writeUint("start", hit.rangeStart); + json.writeUint("end", hit.rangeEnd); + json.end(); + } + + json.end(); +} + DebuggerSubscriber *WebSocketBreakpointInit(DebuggerEventHandlerMap &map) { // No need to bind or alloc state, these are all global. map["cpu.breakpoint.add"] = &WebSocketCPUBreakpointAdd; diff --git a/Core/Debugger/WebSocket/BreakpointSubscriber.h b/Core/Debugger/WebSocket/BreakpointSubscriber.h index 9f70e0337e..cf4f8dbed4 100644 --- a/Core/Debugger/WebSocket/BreakpointSubscriber.h +++ b/Core/Debugger/WebSocket/BreakpointSubscriber.h @@ -35,3 +35,10 @@ void WebSocketRegBreakpointAdd(DebuggerRequest &req); void WebSocketRegBreakpointUpdate(DebuggerRequest &req); void WebSocketRegBreakpointRemove(DebuggerRequest &req); void WebSocketRegBreakpointList(DebuggerRequest &req); + +struct BreakpointHit; + +// Writes the "hit" object describing what tripped a breakpoint. Shared by the cpu.breakpoint.hit +// broadcast and by cpu.stepping, so a client can parse the two the same way and the field set +// can't drift between them. +void WriteBreakpointHit(JsonWriter &json, const BreakpointHit &hit); diff --git a/Core/Debugger/WebSocket/SteppingBroadcaster.cpp b/Core/Debugger/WebSocket/SteppingBroadcaster.cpp index 76bef1edb0..ceb4b07dbf 100644 --- a/Core/Debugger/WebSocket/SteppingBroadcaster.cpp +++ b/Core/Debugger/WebSocket/SteppingBroadcaster.cpp @@ -17,12 +17,15 @@ #include "Core/Core.h" #include "Core/CoreTiming.h" +#include "Core/Debugger/WebSocket/BreakpointSubscriber.h" #include "Core/Debugger/WebSocket/SteppingBroadcaster.h" #include "Core/Debugger/WebSocket/WebSocketUtils.h" #include "Core/MIPS/MIPS.h" #include "Core/System.h" struct CPUSteppingEvent { + // By value: the SteppingReason this is built from is a temporary at every call site, and it + // carries strings now, so binding a reference to it is asking for trouble later. CPUSteppingEvent(const SteppingReason &reason) : reason_(reason) { } @@ -37,12 +40,17 @@ struct CPUSteppingEvent { j.writeString("reason", BreakReasonToString(reason_.reason)); j.writeUint("relatedAddress", reason_.relatedAddress); } + // Present only when a breakpoint was what stopped us, so its absence is the test rather + // than some "kind": "none" the client would have to check for. + if (reason_.hit.kind != BreakpointKind::None) { + WriteBreakpointHit(j, reason_.hit); + } j.end(); return j.str(); } private: - const SteppingReason &reason_; + const SteppingReason reason_; }; // CPU has begun stepping (cpu.stepping) diff --git a/docs/WebSocketDebugger.md b/docs/WebSocketDebugger.md index 697654502d..f3e789ee47 100644 --- a/docs/WebSocketDebugger.md +++ b/docs/WebSocketDebugger.md @@ -121,16 +121,83 @@ Sent without you asking, whenever the underlying state changes: | `game.pause` / `game.resume` | User opens/leaves the pause menu | `GameBroadcaster.cpp` | | `cpu.stepping` | CPU enters a stepping/break state | `SteppingBroadcaster.cpp` | | `cpu.resume` | CPU resumes from stepping | `SteppingBroadcaster.cpp` | +| `cpu.breakpoint.hit` | Any breakpoint trips, whether or not it stops the CPU | `WebSocket.cpp` | | `input.buttons` | Any emulated button changes state | `InputBroadcaster.cpp` | | `input.analog` | An analog stick position changes | `InputBroadcaster.cpp` | A client can opt out of specific broadcast categories with -`broadcast.config.set` (`{"disallowed": {"logger": true, "game": true, "stepping": true, "input": true}}`), +`broadcast.config.set` (`{"disallowed": {"logger": true, "game": true, "stepping": true, "input": true, "breakpoint": true}}`), see `ClientConfigSubscriber.cpp`. `gpu.stats.feed` (see below) works the same way for periodic GPU stats. `client.config.set` in the same file carries per-connection settings that aren't about broadcasts - currently just `acknowledgeDeferred`, described under "Message protocol" above. +### Breakpoint hits + +`cpu.breakpoint.hit` fires every time a breakpoint's condition passes and it has +some action set - including **log-only breakpoints, which never stop the CPU**. +That's what makes them usable for automation: before this event existed, a +log-only breakpoint's only trace was a line in the log stream. + +```json +{ + "event": "cpu.breakpoint.hit", + "sequence": 1, + "hit": { + "kind": "exec", + "pc": 142876568, + "address": 142876568, + "hits": 1, + "logged": true, + "paused": false, + "condition": null, + "symbol": "rendering.mesh.Mesh(rendering.Vertex.PspVertex).draw", + "breakpoint": { "start": 142876568, "end": 142876568 } + } +} +``` + +The same `hit` object is attached to `cpu.stepping` when a breakpoint is what +stopped the CPU, so both can be parsed the same way. It is **absent** when the +break came from something else (the user pausing, a savestate load, an +exception), so test for its presence rather than for a `kind`. + +Fields common to every kind: + +| Field | Meaning | +|---|---| +| `kind` | `"exec"`, `"memory"` or `"register"` | +| `pc` | The instruction responsible | +| `address` | Exec: the instruction. Memory: the address **actually accessed** | +| `hits` | Total times this breakpoint has tripped, matching `*.list` | +| `logged` / `paused` | Which actions it had - `paused` false means the CPU kept running | +| `condition` | The condition expression, or `null` | +| `symbol` | Symbol at `address`, or `null` - resolved here to save a round trip | +| `breakpoint` | `{start, end}` identifying which breakpoint fired. Absent for `"register"`, whose identity is the register, not an address | + +Extra fields for `"memory"`: + +| Field | Meaning | +|---|---| +| `size` | Bytes accessed | +| `access` | `"read"` or `"write"` | +| `source` | Who performed it - `"interpret"`, `"CPU"`, `"HLE"`, or an allocation tag such as `"ThreadFillStack"` | + +Extra fields for `"register"`: `register` (GPR index) and `registerName` +(e.g. `"a0"`). + +Note `address` and `breakpoint.start` are **not** the same thing for a memory +breakpoint watching a range - the first is the byte touched, the second is the +range being watched. On `cpu.stepping` the legacy `relatedAddress` field keeps +reporting the range start; `hit.address` is the accurate one. + +`sequence` counts hits *produced*, not delivered. A connection whose queue backs +up (easy to do with a log-only breakpoint in a hot loop - one can produce tens +of thousands of hits per second) drops events rather than growing without bound, +so a gap in `sequence` tells a client exactly how many it missed. Turning the +`breakpoint` category off via `broadcast.config.set` avoids the traffic +entirely. + ## Request/response event catalog Full details (parameters, response shape) are documented as comments above diff --git a/libretro/libretro.cpp b/libretro/libretro.cpp index 62815a7b89..fa61e2b317 100644 --- a/libretro/libretro.cpp +++ b/libretro/libretro.cpp @@ -1984,7 +1984,11 @@ 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 +// Stubs to let things link - libretro builds Core.cpp and Breakpoints.cpp, which call into the +// WebSocket debugger, but doesn't build Core/Debugger/WebSocket.cpp itself. +void WebSocketDebuggerTick() {} +bool WebSocketDebuggerHasClients() { return false; } +void WebSocketNotifyBreakpointHit(const BreakpointHit &hit) {} void System_Toast(std::string_view str) {} inline int16_t Clamp16(int32_t sample) {