CrossCraft Classic jumps to 0xae870000 a few seconds in, after a couple of
button presses, and dies. It runs fine on a real PSP.
The jump is a Zig interface call - std.mem.Allocator is {ptr, vtable} and the
crash is `lw t9, 0(vtable); jalr t9`. The vtable pointer had come out as
0x089191E0 instead of 0x089291E0, exactly 64KB low, so t9 was loaded from the
middle of compiler_rt.udivmod and the "function pointer" was really the
instruction word `sw a3,0(s4)`.
That pointer is built by a lui/addiu pair, and which LO16 completes a given
HI16 decides one thing: whether the high half carries. We were pairing each
HI16 with the next non-HI16 relocation in the table, which can't work in
general. A PRX relocation carries no symbol index - r_info holds segment
numbers - so the table cannot express the pairing. prxgen does order it so a
HI16 is followed by a LO16 for the same symbol, but not necessarily by *its*
LO16: LLVM schedules several luis together and their addius come back
permuted, here even earlier in the table than the lui they belong to. Two
LO16s for one symbol usually yield the same high half, which is why this has
gone unnoticed for so long - it only bites when their low halves land on
opposite sides of 0x8000. This game has two .rodata references in one
function that do exactly that, and the loader swapped them.
Pair the way the compiler generated it instead: the lui loads a register, and
the instruction completing it is the next one using that register as a base -
unless another lui reloads that register first, in which case the candidate
belongs to that one. Falls back to the old scan when the register trail goes
cold (the value gets copied before use), so nothing that worked before stops.
Checked exhaustively against an oracle: the pre-link ELF still has a symbol,
and thus a section, for every relocation, so any pairing whose result lands
outside that section is wrong. Over this module's 8589 HI16 relocations the
old rule puts 22 outside their section; the new one puts 0, and never picks a
LO16 belonging to a different symbol. Both the register match and the reload
guard are needed - without the guard it scores worse than the old rule.
Impact is narrow. On GCC-built PRXs (pspautotests) the chosen LO16 differs for
2-4 relocations per module and the emitted instruction changes in none of
them. On this LLVM-built module 449 pairings differ and 46 emitted addresses
change, i.e. 46 pointers that were quietly 64KB off.
pspautotests 314/314, unit tests 51/51.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
It stopped being about memory when CPU_Shutdown started holding it across the
whole teardown - it's what keeps kernel objects, the symbol map and the memory
map from being freed while another thread reads them. The old name invited the
reading that it locks memory *access*, which it has never done.
Memory::Reinit() now holds it across both halves rather than relying on
Memory::Shutdown()'s own acquire: between Shutdown() and Init() there is no
memory map at all, and a reader could slip into that gap.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
It was only held across Memory::Shutdown(), but everything else in there frees
state the debugger UIs read from other threads - kernel objects
(__KernelShutdown), the symbol map, replacements - so a Win32 debugger window
painting while a game is reset could read freed memory. It's recursive, so the
nested acquire in Memory::Shutdown() is unaffected.
No lock-order risk: on the paths where CPU_Shutdown already runs under
g_frameMutex it now takes these in the same frame-then-shutdown order the GUI
side uses, and on the paths where it doesn't (EmuScreen::sendMessage,
ProcessScreenSwitches - both above where NativeFrame takes the guard) it takes
only this one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
Nothing needs it any more. Every handler either does its emulator-state access
inside Core_RunOnCPUThread(), which serializes it against startup and shutdown
because those run on the CPU thread too, or only touches state that carries its
own lock - the log ring buffer, ctrlMutex, GPUStepping's rendezvous.
Good riddance: it had to be held across an entire handler, including the
blocking wait inside Core_RunOnCPUThread(), so the CPU thread taking it on
STOPPING deadlocked against a debugger request in flight. That needed a
drain-while-waiting workaround, which now goes away with it.
Verified with the instrumented shutdown repro from that fix, which still exits
cleanly with no lock at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
Three things still touched CPU-thread-owned state from the WebSocket thread:
- Breakpoint conditions were compiled in Parse(), and resolving symbols in an
expression goes through g_symbolMap, which is destroyed on shutdown. Compiled
inside the queued callback now, before anything is mutated, so a bad
expression still fails without leaving a breakpoint behind.
- gpu.record.dump dereferenced the gpu pointer, which is created and destroyed
on the CPU thread.
- gpu.stats.feed bumped PSP_ForceDebugStats' plain counter.
Also makes g_bootState atomic - it's read as a fast-fail from the debugger
thread all over while the CPU and loader threads move it along.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
GameBroadcaster and SteppingBroadcaster ran per connection on the WebSocket
thread, so every connected debugger was reading pc, the tick count, coreState,
the UI state and the param SFO out from under the CPU thread on every lap of its
loop - up to 1000 times a second in high-activity mode.
Inverted: the CPU thread notices the transition once in WebSocketDebuggerTick(),
formats the event there, and drops it into a per-connection mailbox that the
connection's own thread drains and sends. Same events, same conditions, no core
reads off the CPU thread, and no per-connection polling of emulator state.
The tick hangs off Core_ProcessCPUQueue(), the one function reliably called on
the CPU thread both in game (Core_RunLoopUntil) and at the menu (NativeFrame).
It polls even with nothing connected, since skipping would let the "previous
state" go stale and fire a bogus event at whoever connects next.
Behavior preserved including the awkward bit: a debugger that connects while the
CPU is already stopped still gets an immediate cpu.stepping, which used to fall
out of SteppingBroadcaster's counter starting at 0. That's now an explicit
per-connection prime instead of an accident.
Part of removing the WebSocket debugger's lifecycleLock.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
The WebSocket debugger reads it from its own thread (input.buttons.press counts
down frames against it) while the CPU thread bumps it.
Note the input subscriber and broadcaster need no other changes for thread
safety: __CtrlUpdateButtons, __CtrlSetAnalogXY, __CtrlPeekButtons and
__CtrlPeekAnalog all take ctrlMutex internally, so routing them through
Core_RunOnCPUThread() would only add a blocking round trip.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
memory.info.list/search walk MemBlockInfo's slab maps and memory.info.set writes
to them, all straight from the WebSocket thread. Route through
Core_RunOnCPUThread(), pulling the isAlive/IsValidAddress checks into the same
trip - checking them outside it only tells you what was true a moment ago.
memory.info.config now reports the value after applying 'detailed' rather than
before, which is what the docs always claimed.
Part of removing the WebSocket debugger's lifecycleLock.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
replay.* mutates and reads replay/RTC state that the CPU thread consumes as it
runs, and gpu.displaylist.disasm reads through the gpu pointer and emulated
memory. Both did it straight from the WebSocket thread. Route through
Core_RunOnCPUThread(), and fold the "is a game running" checks into the same
trip rather than testing before it, where the answer could already be stale.
Part of removing the WebSocket debugger's lifecycleLock.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
game.status, game.reset and version all read PSP_GetBootState(), g_paramSFO,
GetUIState() and PSP_CoreParameter() straight from the WebSocket thread, where
they race with a game being torn down or booted. Route them through
Core_RunOnCPUThread() like the other subscribers already do.
Part of removing the WebSocket debugger's lifecycleLock, which is currently what
stops these racing with teardown.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
The WebSocket thread holds lifecycleLock across a whole event handler, and
handlers do their real work through Core_RunOnCPUThread(), which blocks until
the CPU thread drains the queue. Meanwhile PSP_Shutdown() ->
Core_NotifyLifecycle(STOPPING) took that same lock on the CPU thread. So the
debugger thread waited for the CPU thread while the CPU thread waited for the
lock the debugger thread was holding, and neither ever moved.
Drain the CPU queue while waiting for the lock instead of blocking on it. Core
state is still alive at STOPPING (it's notified before CPU_Shutdown), so running
those queued callbacks then is safe, and it lets the debugger thread finish and
release the lock.
Verified with a temporary instrumented build - a 3s sleep inside a handler while
holding lifecycleLock, arranged to overlap the game's shutdown - which hangs
reliably on the old code and exits cleanly with this change.
lifecycleLock stays for now: roughly half the subscribers and all the
broadcasters still read core state directly on the WebSocket thread instead of
going through the queue, and this is what keeps that from racing with teardown.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
Reported hang: the CPU thread held g_frameMutex (NativeFrame) and blocked on
g_shutdownLock inside a queued memory.read, while the GUI thread held
g_shutdownLock (CtrlMemView::onPaint) and blocked on g_frameMutex. Textbook
ABBA.
The CPU thread's order is structural - NativeFrame wraps everything below it in
g_frameMutex, and both Core_ProcessCPUQueue() and runImDebugger() ->
DisassembleRange() lock memory from under there - so the GUI side is the one
that has to match. Swaps the three handlers that had it backwards
(CtrlMemView::onPaint, CtrlDisAsmView::onPaint, CtrlStackTraceView::
loadStackTrace) to take g_frameMutex first. They already took both locks, so
this is ordering only, and g_shutdownLock is recursive so nesting is fine.
Also drops the Memory::MemoryInitedLock from the WebSocket LockMemory(), which
is what made the CPU thread want that lock in the first place. It was guarding
against another thread tearing down the memory system, but that doesn't happen:
Memory::Shutdown() is only reached via CPU_Shutdown() <- PSP_Shutdown(), whose
callers all run on the CPU thread, and Memory::Reinit() runs from
Memory::DoState() on savestate load, likewise. WebSocket.cpp additionally holds
lifecycleLock across the whole handler and takes it on STOPPING.
Note this second part isn't sufficient on its own - ImMemView's copy-disassembly
path also locks memory from inside the frame span - which is why the ordering
fix is the real one.
Not removing Memory::Lock() from the Win32 paint handlers: teardown isn't fully
inside the g_frameMutex span yet. EmuScreen::render()'s PSP_Shutdown() is, but
the ones in EmuScreen::sendMessage() (game reset, loading a new game) run from
g_screenManager->sendMessage(), above where NativeFrame takes the guard. Closing
that is the prerequisite, and is left for later.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
step-over, step-out and run-until plant a one-shot breakpoint at the address
they want execution to return to. Keeping it in breakPoints_ alongside the
user's own meant the two kept colliding:
- Adding a log-only user breakpoint at the same address hijacked the temporary
one. AddBreakPoint() didn't match across temp-ness so both existed, and then
ChangeBreakPoint() looked up "the first enabled breakpoint at this address" -
a log-only breakpoint isn't enabled, so the temporary one won and had its
action overwritten to log-only. It lost PAUSE and the step never came back.
- RemoveBreakPoint() erased up to two entries per address to catch an
overlapping temporary one, so deleting either deleted both - including the
interpreter's cleanup path in CheckExecBreakpoints() taking the user's
breakpoint with it.
- ExecBreakPoint() handled one breakpoint per address, so with both at the same
address only one of them did anything: the step completed but the user's log
line never printed.
- Nothing dropped it when something *else* stopped us first, so an interrupted
step left a breakpoint armed at an address nobody was waiting for anymore,
which later fired as a phantom stop.
It's a single TempBreakPoint member now, invisible to the breakpoint lists and
untouched by user edits. One is enough: step over/out and cross-thread step into
all require the CPU to already be stepping and resume it immediately, so only
one can be in flight, and run-until now replaces rather than stacking (two
pending run-untils had no coherent meaning, and the loser stayed armed).
Behavior follows what other debuggers do. Both breakpoints at an address are
evaluated independently and their actions combine, so a log-only breakpoint
logs without stopping and still lets the step finish. Core_Break() drops the
temporary breakpoint on any stop, whatever the reason - the same way gdb deletes
its step-resume breakpoint and lldb discards the thread plan.
Two things to be careful of, both covered by the new TempBreakpoints test:
HasBreakPoints() has to account for it, or the interpreter's checked run loop
and the JIT skip breakpoint checking entirely and a step with no user
breakpoints set never returns; and IsAddressBreakPoint() (user-facing, for the
lists and disassembly markers) is now separate from NeedsBreakCheckAt() (what
the JIT frontends and interpreter ask), since only the latter should see it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
ChangeBreakPointAddress() moves the breakpoint keeping its action, condition and
log format, invalidates both ends, refuses to land on an existing breakpoint,
and resets the hit count since it belonged to the old address. The edit form now
works on a copy of the address and commits on deactivation rather than per
keystroke, so typing one address doesn't churn through every prefix of it.
The breakpoint edit form assigned straight to bp.addr and then invalidated the
icache at "bp.addr - 4, 8" - which by then is the *new* address - need both.
Also clear the selection after Delete in both edit forms - the reference into
the vector is dangling from that point on. Harmless today, but only because
nothing happens to touch it below.
Covered by a new Breakpoints unit test (verified to fail without the duplicate
check and the hit reset).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
AddLabel() won't overwrite an existing label. That's deliberate and right for
bulk import - a real ELF symbol name shouldn't lose to the analyzer's later
z_un_* - but wrong when someone is explicitly naming an address, so a second
hle.data.add at the same address silently kept the old name. The response echoed
the requested name back either way, so there was no sign anything had been
ignored.
Force the requested name in with SetLabelName() now, except when a function
starts at that address and owns the label - renaming that function isn't what
"label this data" should mean, and it would undo the care hle.data.remove takes
not to destroy it. Either way the response now reports the name the symbol
actually ended up with rather than the one that was asked for.
Also, in the ImDebugger memcheck edit form: the Enabled checkbox didn't mark the
memcheck as changed, and the condition combo marked it changed on every frame
the popup was open rather than when a condition was actually picked (Selectable
returns true only on click, BeginCombo stays true while open).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
memory.readString could kill the connection: it copied raw emulated memory
straight into a JSON string, so any address not holding valid UTF-8 produced an
invalid WebSocket text frame.
hle.data.remove wiped the name of a function sharing the address. Labels are
shared between data and function symbols, so removing the data label left the
function showing up in hle.func.list with an empty name.
hle.data.add silently did nothing outside a loaded module. GetModuleIndex()
returns -1 for e.g. a heap or stack address, and symbols under that index never
reach the active maps - so the add reported success while the symbol was
invisible to list, and rename/remove then failed with "No data symbol found".
Falls back to module index 0 ("no module, absolute address"), which is the right
answer for a label the user put somewhere after a memory.search.
hle.thread.list reported the thread's stack base address in a field called
initialStackSize. Renamed to initialStack, matching the SceKernelThreadInfo
field it comes from.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
Decoding a GE display list previously meant memory.read-ing the raw bytes
and hand-decoding each 32-bit command word against GPU/ge_constants.h's
GECommand enum - which is exactly what it took to find this session's
actual headline VSH boot finding (a display list that clears the screen
once, sets up per-icon render state 6 times, and never issues a single
further draw call - see docs/VSHBootInvestigation.md Attempt 22). That
manual process is real, repeatable, and error-prone by hand; PPSSPP
already has a proper GE disassembler (GPU/GeDisasm.cpp's
GeDisassembleOp(), and GPUCommon::DisassembleOpRange() built on top of
it) used by the ImGui/Windows GE debugger UI - it just wasn't reachable
from the WebSocket API.
New Core/Debugger/WebSocket/GPUDisasmSubscriber.cpp exposes
gpu->DisassembleOpRange() as gpu.displaylist.disasm, mirroring
memory.disasm's own parameter conventions (address+count or
address+end, capped at 10000 commands) and compact mode (one string per
command, "AAAAAAAA desc", instead of the full {address,cmd,op,desc}
object) added in the previous commit. GE command words live in normal
guest RAM like CPU code, so - unlike gpu.buffer.* - this doesn't require
the CPU/GPU to be paused first, matching memory.disasm's own live-read
behavior.
Added to all 6 build systems that compile the WebSocket debugger
(CMakeLists.txt, Core.vcxproj(.filters), UWP's CoreUWP.vcxproj(.filters),
android/jni/Android.mk - libretro doesn't build any Debugger/WebSocket
files at all, so nothing to add there).
Verified live via PPSSPPHeadless + wsdbg against a real demo ELF: both
compact and full-JSON modes correctly decode real GE command words (NOP/
NOP_FF) with no errors. UnitTest.exe all: 49/49 passed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
Two real gaps hit repeatedly while investigating the VSH boot path (see
docs/VSHBootInvestigation.md):
- memory.disasm's response is the full per-field JSON (type, address,
addressSize, encoding, macroEncoding, backgroundColor, name, params,
symbol, function, dataSymbol, breakpoint, isCurrentPC, branch,
relevantData, conditionMet, dataAccess - ~15 fields per line). Reading
disassembly by hand meant writing a throwaway script each time to reduce
this down to "ADDR: name params" - and at least once, a bug in one of
those scripts produced misleading output that wasn't caught immediately.
Added compact=true: returns "lines" as an array of plain strings
("M AAAAAAAA [symbol: ]name params", M = '>' for current PC, '*'/'o'
for an enabled/disabled breakpoint) instead, computed once correctly
here instead of ad hoc every time.
- memory.searchDisasm already existed but only ever returned the first
match - genuinely limiting for "find every caller of this address"
call-graph-style queries, which came up directly while trying to trace
which function builds VSH's GE display list. Added findAll=true: scans
the whole range and returns every match in a new "addresses" array
(capped at 1000), instead of stopping at the first. Default behavior
(address: first match or null) is unchanged for existing callers.
Verified live via PPSSPPHeadless + wsdbg: compact mode against a real
demo ELF's entry point produces clean, correctly-marked text lines;
findAll=true against the same range found all 11 jal instructions instead
of just the first. UnitTest.exe all: 49/49 passed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
Into()'s same-thread branch called Core_RequestCPUStep(CPUStepType::Into, 1)
without checking its return value. Core_RequestCPUStep() can genuinely
fail (a step/run request is already queued this host frame - see its own
"Can't submit two steps in one host frame" ERROR_LOG) - on failure, no
step happens and no cpu.stepping event ever fires, but cpu.stepInto's own
contract is "no immediate response, a cpu.stepping event follows", so a
rejected request looked identical to a request still in flight: nothing
to distinguish "wait longer" from "this silently failed, nothing is ever
coming." This is part of the same failure family as the delay-slot race
just fixed in PrepareResume() (previous commit) - Core_RequestCPUStep()'s
one-at-a-time guard rejecting a step no caller in this file checked for.
Now calls req.Fail() on rejection so the client gets an explicit answer
instead of an indefinite wait. Updated the cpu.stepInto doc comment to
note the new (retryable) failure mode.
Verified via UnitTest.exe all (49/49).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
DebuggerLogListener buffers up to 1024 log messages between polls of the
WebSocket event loop (up to 1000Hz under high activity, 60Hz otherwise -
see WebSocket.cpp). A source that logs faster than that - a log-only
breakpoint hit thousands of times in a tight loop is a real example, not
hypothetical, see docs/VSHBootInvestigation.md's Attempt 22/24 - can wrap
the ring buffer before GetMessages() ever reads the oldest entries,
silently losing them. From the client's side this was indistinguishable
from the breakpoint just not firing at all, which cost real debugging time
this session tracking down a red herring before finding the real
mechanism.
GetMessages() already detected the overflow case internally (the
`read_ + BUFFER_SIZE < count_` branch) to avoid returning garbage, but
never reported how many messages were actually lost. Now synthesizes a
warning LogMessage ("N log message(s) dropped - client polling too slow
for this volume") and prepends it to the batch whenever this happens, so
a real gap is visibly distinguishable from "this just never got logged."
Verified via UnitTest.exe all (49/49) and a live PPSSPPHeadless + wsdbg
session confirming normal (non-overflow) log relay still works
end-to-end.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
BreakPoint (cpu.breakpoint.*) had no hit-count tracking at all, unlike
MemCheck (memory.breakpoint.*), which already tracks numHits. This made it
genuinely hard to tell "this breakpoint is never being reached" apart from
"it's being reached but I'm not seeing the log/pause where I'm looking" -
directly informed by repeatedly hitting exactly that ambiguity while
debugging the VSH boot path this session (see docs/VSHBootInvestigation.md).
Added BreakPoint::numHits, incremented in BreakpointManager::ExecBreakPoint()
whenever a breakpoint's address is hit and any condition passes (matching
MemCheck::Apply()'s existing semantics - counts real triggers, not just
"execution passed through here"). Exposed as a new "hits" field in
cpu.breakpoint.list's response.
Verified live via PPSSPPHeadless + wsdbg: hits reads 0 before the CPU
resumes, 1 after the breakpoint fires once. UnitTest.exe all: 49/49 passed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
PrepareResume() used Core_RequestCPUStep(CPUStepType::Into, 1) to step past a
delay slot instruction before deciding whether to add a breakpoint and call
Core_Resume() - but Core_RequestCPUStep() only queues that step for
Core_ProcessStepping() to perform later (on the next iteration of the normal
stepping-mode loop). Every caller (Into's cross-thread branch, Over, Out,
RunUntil, HLE) immediately inspected currentMIPS->pc/inDelaySlot right after
PrepareResume() returned to decide what to do next - reading stale,
pre-step state, since the queued step hadn't run yet.
Worse: those callers then call Core_Resume(), which sets coreState back to
CORE_RUNNING_CPU. Core_ProcessStepping() only processes g_cpuStepCommand
when coreState is CORE_STEPPING_CPU/STEPPING_GE/RUNNING_GE, so once resumed,
the queued step is never processed at all - not just late, silently dropped,
leaving g_cpuStepCommand permanently set until the next Core_Break() resets
it. Any cpu.step*/cpu.runUntil request a client issues in that window (CPU
resumed running, breakpoint not yet hit again) hits
Core_RequestCPUStep()'s "Can't submit two steps in one host frame" guard and
is silently ignored, since none of these call sites check its return value -
this is the "step-out sometimes just doesn't do anything" flakiness reported
against this file.
PrepareResume() is only ever called from within a Core_RunOnCPUThread()
callback, so it's always already running on the CPU thread - safe to
single-step synchronously (currentMIPS->SingleStep(), matching how
Core_PerformCPUStep()'s own CPUStepType::Into case does it) instead of
queuing an async request whose completion every caller then assumes without
verifying.
Verified via UnitTest.exe all (49/49). Attempted to force a live repro via
wsdbg against a delay-slot jal in a demo ELF; wasn't able to reliably
trigger the failure window externally (by the time a client's next command
arrives, the CPU has typically already reached its next breakpoint and
Core_Break() has cleaned up the stale state first) - the race window is
real per the code trace above but appears to be narrow enough that it
mainly shows up under real usage timing (a slow-to-reach next breakpoint,
or a fast follow-up command from a script/UI), not simple synchronous
scripting. The fix is unconditionally more correct regardless: it replaces
a fire-and-forget async request every caller immediately assumed had
already completed with a direct synchronous call that actually has by the
time the next line runs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
The interpreter's hot-path breakpoint check in
RunUntilDowncountZeroWithChecks called Core_Break() unconditionally
whenever IsAddressBreakPoint() was true - true for any non-ignored
breakpoint, log-only included - instead of routing through
BreakpointManager::ExecBreakPoint(), which is what actually respects
BREAK_ACTION_LOG vs BREAK_ACTION_PAUSE. So a cpu.breakpoint.add with
log=true and enabled=false still paused on hit, contradicting its own
documented behavior.
The JIT backends and IR interpreter don't have this bug - they already
route through ExecBreakPoint() via JitBreakpoint()/IRRunBreakpoint()
and check the result for BREAK_ACTION_PAUSE. Only this one plain
interpreter loop had its own unconditional inline check instead.
Verified: a log-only breakpoint now logs without pausing (325 hits
logged, then execution continued past it normally); a normal enabled
breakpoint still pauses; cpu.stepOver (which relies on temporary
breakpoints, still correctly removed only when they actually pause)
still steps over calls correctly; all 49 unit tests pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
(cherry picked from commit 726db5e4ea3651db0eb2de13c622c033dcc95699)
Two diagnostic experiments while chasing the divide-by-zero break from the
previous commits (see docs/VSHBootInvestigation.md "Attempt 8"/"Attempt 9"):
- Unregistered MMIO reads now return a distinctive poison value
(0x1337BEEF) instead of 0, so a future trace can tell at a glance when a
value traces back to an unimplemented register instead of looking like an
ordinary zero.
- COP0 register 9 (Count) now returns a live CoreTiming-derived value
instead of a static shadow-array read, matching how real hardware free-
runs it regardless of software writes.
Neither change altered the reboot.bin free-run's outcome at all (identical
break, same PC) - ruling out both as the source of the zero divisor traced
in the previous commit. Kept anyway: both are straightforwardly more
correct/useful than what was there before, independent of this specific
bug.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
(cherry picked from commit ac446449d9031b628d7d9c4dbc101638840f8434)
The struct and its API only handle GPR indices today, but the naming
should stay general since this is expected to grow to cover other
register files too (e.g. FPU registers like $f10). Pure rename - no
behavior change:
- Core/Debugger/Breakpoints.{h,cpp}: RegBreakpoint struct, all
BreakpointManager Add/Remove/Change/Get/Exec/Has/Find*RegBreakpoint*
methods, regBreakpoints_/regBreakpointMask_ members.
- Core/Core.{h,cpp}: BreakReason::RegBreakpoint, "cpu.regBreakpoint"
break-reason string.
- Core/Debugger/WebSocket/BreakpointSubscriber.{h,cpp}: WebSocket
events cpu.gprBreakpoint.* -> cpu.regBreakpoint.*, matching
Add/Update/Remove/List handlers and params struct.
- Core/MIPS/MIPSTables.cpp: local variable names in the interpreter's
per-instruction breakpoint check.
- docs/WebSocketDebugger.md updated to match.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
Breakpoints.cpp's memcheck-matching NotCached() helpers (used by both the
interpreter's real-time FindMemCheckInRange and the JIT's precomputed
UpdateCachedMemCheckRanges/GetMemCheckRanges) only ever normalized away the
uncached bit (0x40000000), never the kernel bit (0x80000000) - so a
memcheck registered on one kernel/user address alias silently didn't match
a write made through the other. This is a real, general bug (any kernel
code writing through the 0x88xxxxxx-style mirror could dodge a memcheck
set on the corresponding 0x08xxxxxx address), not specific to any one
investigation.
Extended NotCached(u32) to also strip the kernel bit, and added a
NotKernel(MemCheck) counterpart so UpdateCachedMemCheckRanges now expands
each non-VRAM memcheck into all four kernel/uncached combinations instead
of two. VRAM intentionally excluded, matching IsValidAddress's existing
"no kernel-flagged VRAM" comment. cpu.breakpoint (PC) breakpoints are
unchanged - out of scope here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
The scratchpad (PSP's repurposed-cache scratch RAM, 0x00010000+) was only
mirrored for user-mode access (cached 0x00010000, uncached 0x40010000) -
kernel-mode code sees it at 0x80010000/0xC0010000 (the kernel bit,
0x80000000, is independent of and combinable with the uncached bit,
0x40000000 - not "the uncached bit" as an earlier doc note in this branch
mistakenly called it). Missing entirely from MemMap.cpp's views[] table,
causing a real SIGSEGV the first time kernel-mode code (flash0:/reboot.bin)
touched it.
Adding the two missing views wasn't sufficient: the scratchpad range check
is duplicated eight times (IsValidAddress/IsValid2AlignedAddress/
IsValid4AlignedAddress/MaxSizeAtAddress in MemMap.h, and four more in
MemMapFunctions.cpp), and all eight used a mask (0xBFFFC000) that cleared
the uncached bit but kept the kernel bit, rejecting 0x80010000 as invalid
before ever reaching the now-mapped memory. Fixed all eight to 0x3FFFC000,
matching Memory::MEMVIEW32_MASK (which the JIT backends already used
correctly for the equivalent runtime check).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
