diff --git a/.github/workflows/c-cpp.yml b/.github/workflows/c-cpp.yml index fcbf7f72..9439df6e 100644 --- a/.github/workflows/c-cpp.yml +++ b/.github/workflows/c-cpp.yml @@ -811,8 +811,15 @@ jobs: # capability macros and includes). They are covered instead by # c89-lint's per-arch pass, which compiles them the way the # build does. + # + # src/jerry/dsp_exec_untimed.h is the same kind of fragment: a + # function body dsp.c includes twice, using dsp.c's statics. + # Standalone it is nothing but undeclared identifiers. Its code + # is still linted, through dsp.c, because HeaderFilterRegex + # covers src/jerry. FILES=$(git diff --name-only --diff-filter=d "$BASE"...HEAD -- '*.c' '*.h' \ | grep -Ev '^(src/m68000/|src/bios/jag.*\.c$|libretro-common/|src/core/version\.h$|src/(tom|jerry)/[a-z0-9_]+_simd_(neon|sse2|scalar)\.(c|h)$)' \ + | grep -v '^src/jerry/dsp_exec_untimed\.h$' \ | grep -E '^(src/|libretro\.c$)' || true) if [ -z "$FILES" ]; then echo "No relevant C files changed; skipping clang-tidy." diff --git a/docs/gdb-stub-design.md b/docs/gdb-stub-design.md index 15901c40..f8e0e865 100644 --- a/docs/gdb-stub-design.md +++ b/docs/gdb-stub-design.md @@ -416,6 +416,37 @@ Five layers, in increasing cost: (digest-verified, C89-clean) rather than merged -- 0.7% did not justify an extracted-body header in the emulator's hottest function. + **Update (issue #1054, 2026-10): the DSP half is now specialised.** + By October develop had split DSPExec() into timed and untimed loops, + and the untimed loop is now built twice from `src/jerry/dsp_exec_untimed.h`: + a plain instance with no GDB code, and an armed instance that DSPExec() + enters only while `gdbArmedDSP` is set. Measured on an M4 Max, A/B/B/A, + load 2-4 unless noted: + + | Arms | ROM | Median | Best-run | p | + |---|---|---|---|---| + | develop vs DSP check compiled out (the ceiling) | yarc | +2.7% | +2.3% | 0.0003 | + | develop vs port | yarc | +2.6% | +3.5% | 0.0006 | + | develop vs port (repeat, 5-min load ~10) | yarc | +1.6% | +3.3% | 0.0092 | + | develop vs port, both DSP-armed (load rose 2 -> 17) | yarc | +2.5% | +0.3% | 0.1078 | + | develop vs port | Tempest 2000 | -0.2% | +0.2% | 0.7415 | + | develop vs port | Doom | -0.8% | -0.4% | 0.6801 | + | develop vs ceiling | Tempest 2000 | -1.0% | -1.3% | 0.0007 | + | develop vs ceiling | Doom | -0.6% | -1.3% | 0.0168 | + | ceiling vs port | yarc | +1.1% | +0.9% | 0.0018 | + + Read the last three rows before quoting a cause. Compiling the check + out makes real games about 1% *slower*, and the port beats the ceiling + on yarc although its plain loop is the same C. Code layout moves this + loop by about +/-1%, the same size as the check. So the claim is + narrow: the default DSP path carries no GDB code, yarc gains, and games + are neutral. A "the check cost 2%" reading is not supported. + + One consequence for the control arms: after this change + `VJ_GDB_STUB_DISABLE_DSP_HOOK=1` compares two nearly identical plain + loops, so the DSP row of a future attribution run should read about 0. + That does not mean the arm is broken. + **Next step is real-hardware profiling, not more host measurement.** Every number here comes from an arm64 Mac that never dropped below load 5. The residual should be re-attributed on a real target (tvOS/A-series, @@ -429,7 +460,8 @@ Five layers, in increasing cost: confident-looking p-value. Shipped-by-default stands for now at a measured ~2% cost, deliberately, - pending those numbers. + pending those numbers. (Since #1054 the default DSP path carries no GDB + code at all; see the update above.) diff --git a/src/jerry/dsp.c b/src/jerry/dsp.c index a83a8d28..abe0bcf9 100644 --- a/src/jerry/dsp.c +++ b/src/jerry/dsp.c @@ -2445,276 +2445,39 @@ static int32_t DSPIdleLoopProbe(int32_t cycles, uint32_t head, uint32_t jrAddr) static DSP_NOINLINE void DSPExecTimed(int32_t cycles); static DSP_NOINLINE void DSPExecUntimed(int32_t cycles); +#if !defined(VJ_GDB_STUB_DISABLE_HOOKS) && !defined(VJ_GDB_STUB_DISABLE_DSP_HOOK) +static DSP_NOINLINE void DSPExecUntimedArmed(int32_t cycles); +#endif /* Timing models (pipeline or dram_timing) run a separate loop, picked * once per call, so the untimed loop below and the opcode handlers it - * dispatches are exactly develop's (issue #313 / #892). Both loops are - * out of line so neither can merge into, or re-allocate, the other. */ + * dispatches are exactly develop's (issue #313 / #892). The untimed loop + * is itself built twice, with and without the GDB breakpoint test (issue + * #1054, see dsp_exec_untimed.h), so the default path carries no debugger + * code. All loops are out of line so none can merge into, or + * re-allocate, another. */ void DSPExec(int32_t cycles) { if (vjs.gpuPipelineTiming || busArbiter.enabled) DSPExecTimed(cycles); +#if !defined(VJ_GDB_STUB_DISABLE_HOOKS) && !defined(VJ_GDB_STUB_DISABLE_DSP_HOOK) + else if (gdbArmedDSP) + DSPExecUntimedArmed(cycles); +#endif else DSPExecUntimed(cycles); } -static DSP_NOINLINE void DSPExecUntimed(int32_t cycles) -{ - int idleSkipActive; - int gdbArmedSlice; - /* Issue #811 late load write-back, armed for exactly the next - * instruction (see dsp_ldx_slot). */ - uint32_t *ldxArmed = NULL; - uint32_t ldxVal = 0, ldxPc = 0; - /* Issue #811 stale indexed-store data after a divide (see - * dsp_div_store_fixups), armed for exactly the next instruction. */ - uint32_t *divArmed = NULL; - uint32_t divOld = 0, divPc = 0; - -#ifdef DSP_SINGLE_STEPPING - if (dsp_control & 0x18) - { - cycles = 1; - dsp_control &= ~0x10; - } -#endif - /* No return between here and VJP_LEAVE (perf_iface.h). */ - VJP_ENTER(VJP_DSP); - - dsp_releaseTimeSlice_flag = 0; - - /* Same shape as GPUExec's cached locals (issue #532): a hot global - * read per emulated instruction with an opaque opcode call in - * between. Measured (issue #652): this removes part of the DSP's - * share of the GDB hook cost; ~2%% remains and is NOT explained -- - * see docs/gdb-stub-design.md. Only GDBHalt() can re-arm inside a - * slice. */ - gdbArmedSlice = gdbArmedDSP; - dsp_in_exec++; - - /* Idle-loop fast-forward gates (issue #569). Every one of these adds - * per-instruction state the affine extrapolation does not model, so - * any of them turns the whole thing off: - * - vjs.riscIdleSkip: the user-facing core option itself; - * - blitMemoMode / blitMemoRecording: the JaguarRead and JaguarWrite - * families note reads and writes for the memo (blit_memo.h:64-65, - * src/core/jaguar.c:1139), and skipping loads would change what - * the memo records; - * - riscClockScalePct is deliberately NOT a gate (#378 overclock - * presets): SCALE_RISC_CYCLES is applied where the budget is handed out (jaguar.c), not inside - * DSPExec, so `cycles` here is already in the same currency the - * measured per-iteration cost was charged in -- cost and opcost - * are measured from live `cycles` deltas, never recomputed, and - * nothing per-instruction in this function depends on the scale. - * Scaling `n` or `cost` again would double-scale. The GPU has - * one scale-dependent per-instruction term (gpu_stall_scale_accum); - * see the note in GPUExec; - * - busArbiter.enabled (the dram_timing option): bus occupancy is - * accounted per access. Belt-and-braces here -- dsp.c never - * touches busArbiter and JaguarReadLong charges it only for - * `who == OP` -- but a DSP-side arbiter charge added later must - * not silently invalidate this; - * - vjs.gpuPipelineTiming: drives the DSP pipeline/gateway model - * (issue #313), which runs in DSPExecTimed() and never reaches - * this loop -- kept in the gate as belt-and-braces; - * - DSP_CTRL single-step / single-go: the slice is one instruction; - * - vjtrace armed or a memory watch installed (VJ_TRACE builds - * only): VJT_PCHIST_DSP in this loop and VJT_WATCH_RD on the - * DRAM read path are per-instruction / per-read side effects. - * This has to be a RUNTIME check, not `#ifdef VJ_TRACE`: the - * whole test ABI is built with -DVJ_TRACE (Makefile:158-161), - * so a compile-time gate would make every headless harness -- - * including the A/B that proves this exact -- silently measure - * a disabled feature. Both flags default off, so the cost is - * two loads per slice on trace builds and nothing at all on - * release builds. */ - idleSkipActive = vjs.riscIdleSkip - && !blitMemoMode && !blitMemoRecording - && !busArbiter.enabled - && !vjs.gpuPipelineTiming - && !(dsp_control & 0x18); -#ifdef VJ_TRACE - if (vjtrace_armed || vjtrace_nwatch) - idleSkipActive = 0; -#endif -#if !defined(VJ_GDB_STUB_DISABLE_HOOKS) && !defined(VJ_GDB_STUB_DISABLE_IDLE_GATE) - /* GDB stub (issue #652): DSPIdleLoopProbe() extrapolates the PC - * forward over many idle-loop iterations without visiting each one, - * exactly the class of per-instruction side effect the #569 idle-skip - * gate list above already disables for (blit memo, busArbiter, - * vjtrace watch). A DSP breakpoint or pending step would be stepped - * clean over. This is a RUNTIME check, same reasoning as the VJ_TRACE - * block above: gdbArmedDSP defaults to 0 and costs one load + branch - * when nothing is armed. VJ_GDB_STUB_DISABLE_HOOKS exists only for - * the Phase 2 A/B perf measurement. */ - if (gdbArmedDSP) - idleSkipActive = 0; -#endif - /* Probe + reject memo are re-derived from scratch every call, so - * nothing here reaches a savestate and nothing survives a slice. */ - idleProbeStage = 0; - idleMemoCount = 0; - idleMemoNext = 0; - dsp_ldx_slot = NULL; - - /* An armed late write-back runs one instruction past the budget so a - * load/overwrite pair never straddles a slice (see dsp_ldx_slot); - * likewise a divide/indexed-store pair (see dsp_div_store_fixups). */ - while ((cycles > 0 || ldxArmed || divArmed) && DSP_RUNNING) - { - uint16_t opcode; - uint32_t index; - uint32_t pcThis; -#ifdef VJ_TRACE - VJT_PCHIST_DSP(dsp_pc); -#endif - - /* If IMASK was cleared, see if any other interrupts are pending -- - * but not until the D_FLAGS store that cleared it has retired, so - * the instruction still in the pipeline behind it (typically the - * epilogue's `jump` back to the interrupted code) runs first. */ - if (IMASKCleared && dspFlagsRetireDelay == 0) - { - DSPHandleIRQsNP(); - IMASKCleared = false; - } - - /* PC escape bail-out. When the DSP PC has wandered into a - * region that doesn't contain executable code -- register - * space at $F00000-$F1FFFF outside DSP local SRAM, or the - * unmapped territory above $E40000 -- every "fetched opcode" - * is bus-default 0xFFFF garbage that decodes to a near-zero- - * cost opcode. The inner loop then burns the entire - * timeslice without making progress, hanging the frontend. - * - * Wolf3D headless triage caught this in v2.3.0: the runtime - * watchdog logged `dsp_pc_escape pc=$00FFF004E8` (PC top-byte - * corrupted) and the harness wedged for 12+ minutes per - * frame. An earlier dsp-diag snapshot showed PC=$0006EE in - * RAM at frame 48 -- that's the upstream bug (separate from - * this bail-out: $0006EE is *valid* RAM and decodes to real - * opcodes; it's where the DSP eventually drifts INTO bad - * territory that triggers the wedge). Drain cycles here and - * let the runtime watchdog (src/core/crash_detect.c) log the - * actual escape PC. DSP_RUNNING is left alone so games that - * legitimately stop the DSP via DSPGO=0 are unaffected. - * - * Valid execution regions match JaguarReadX address decoding - * (src/core/jaguar.c): anything <= $E3FFFF (main RAM mirrored - * 4x for the bottom 8MB, cart ROM, boot ROM) plus DSP local - * SRAM. Earlier versions of this check used `<= 0x1FFFFF` - * and would have false-flagged DSP code running from a RAM - * mirror at $200000-$7FFFFF or from cart ROM at $800000+. - * Caught by Copilot review on PR #182. */ - if (!((dsp_pc <= 0x00E3FFFF) || - (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000))) - { - cycles = 0; - break; - } - - pcThis = dsp_pc; +#define DSP_UNTIMED_NAME DSPExecUntimed +#define DSP_UNTIMED_GDB 0 +#include "dsp_exec_untimed.h" #if !defined(VJ_GDB_STUB_DISABLE_HOOKS) && !defined(VJ_GDB_STUB_DISABLE_DSP_HOOK) - /* GDB stub (issue #652): one load of a hot global, one - * never-taken branch when no DSP breakpoint/step is armed -- see - * docs/gdb-stub-design.md "Breakpoint detection". GDBHalt() - * blocks in place (does not unwind this loop) until the client - * continues, steps, or disconnects. VJ_GDB_STUB_DISABLE_HOOKS - * exists only for the Phase 2 A/B perf measurement -- see the - * comment in src/core/jaguar.c's M68KInstructionHook(). */ - if (gdbArmedSlice && GDBCheckPC(GDB_TGT_DSP, pcThis)) - { - GDBHalt(GDB_TGT_DSP, GDB_STOP_BREAKPOINT, pcThis); - gdbArmedSlice = gdbArmedDSP; - } +#define DSP_UNTIMED_NAME DSPExecUntimedArmed +#define DSP_UNTIMED_GDB 1 +#include "dsp_exec_untimed.h" #endif - if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000) - { - uint32_t off = dsp_pc - DSP_WORK_RAM_BASE; - opcode = ((uint16_t)dsp_ram_8[off] << 8) | (uint16_t)dsp_ram_8[off + 1]; - } - else - opcode = DSPReadWord(dsp_pc, DSP); - index = opcode >> 10; - dsp_opcode_first_parameter = (opcode >> 5) & 0x1F; - dsp_opcode_second_parameter = opcode & 0x1F; - dsp_pc += 2; - dsp_exec_opcode_count++; - if (divArmed && pcThis == divPc - && &dsp_reg[dsp_opcode_second_parameter] == divArmed) - { - /* Issue #811: indexed store right behind a divide into its - * data register -- the quotient has not been written back - * yet, so the store sees the old contents. */ - uint32_t quot = *divArmed; - - *divArmed = divOld; - dsp_executeOpcode(index); - *divArmed = quot; - dsp_div_store_fixups++; - divArmed = NULL; - } - else if (index == 21) - { - uint32_t *dst = &dsp_reg[dsp_opcode_second_parameter]; - uint32_t old = *dst; - - dsp_executeOpcode(index); - divArmed = DSPDivStorePeek(dst, &divPc); - divOld = old; - } - else - { - dsp_executeOpcode(index); - divArmed = NULL; - } - cycles -= dsp_opcode_cycles[index]; - - /* Issue #811: the previous instruction was an external load and - * this one overwrote its target -- the load data lands last. */ - if (ldxArmed) - { - if (pcThis == ldxPc - && &dsp_reg[dsp_opcode_second_parameter] == ldxArmed) - { - *ldxArmed = ldxVal; - dsp_load_wb_fixups++; - } - ldxArmed = NULL; - } - if (dsp_ldx_slot) - { - ldxArmed = DSPLoadOverwritePeek(&ldxVal, &ldxPc); - dsp_ldx_slot = NULL; - } - - /* Idle-loop fast-forward (issue #569). A taken `jr` that landed - * on or behind its own address, at most 8 words back, is the only - * candidate; the unsigned difference rejects a not-taken jr - * (pc = pcThis + 2) and every forward branch in one compare. */ - if (idleSkipActive && index == 53 - && (uint32_t)(pcThis - dsp_pc) <= 14) - cycles = DSPIdleLoopProbe(cycles, dsp_pc, pcThis); - - /* Age out a D_FLAGS store once the instruction that was already - * behind it in the pipeline has run (see DSPWriteLong, D_FLAGS - * case). Retiring re-opens interrupt recognition and commits the - * register-bank switch the store selected. */ - if (dspFlagsRetireDelay) - { - dspFlagsRetireDelay--; - if (dspFlagsRetireDelay == 0) - DSPUpdateRegisterBanks(); - } - } - - dsp_in_exec--; - - VJP_LEAVE(VJP_DSP); -} - INLINE static void dsp_executeOpcode(uint32_t index) { #ifdef __GNUC__ @@ -2929,7 +2692,7 @@ INLINE static void dsp_opcode_jump(void) * PC before applying the branch target. This replaces the old * recursive DSPExec(1) call, avoiding full function-call overhead, * redundant IRQ checks, and pipeline-state save/restore. */ - if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000) + if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000 - 1) { uint32_t off = dsp_pc - DSP_WORK_RAM_BASE; ds_opcode = ((uint16_t)dsp_ram_8[off] << 8) | (uint16_t)dsp_ram_8[off + 1]; @@ -2976,7 +2739,7 @@ INLINE static void dsp_opcode_jr(void) /* Inline delay-slot: fetch-decode-execute one instruction at current * PC before applying the branch target. Same rationale as in * dsp_opcode_jump above. */ - if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000) + if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000 - 1) { uint32_t off = dsp_pc - DSP_WORK_RAM_BASE; ds_opcode = ((uint16_t)dsp_ram_8[off] << 8) | (uint16_t)dsp_ram_8[off + 1]; @@ -4811,7 +4574,7 @@ static void dsp_opcode_jump_t(void) dspFlagsRetireDelay = 0; DSPUpdateRegisterBanks(); } - if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000) + if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000 - 1) { uint32_t off = dsp_pc - DSP_WORK_RAM_BASE; ds_opcode = ((uint16_t)dsp_ram_8[off] << 8) | (uint16_t)dsp_ram_8[off + 1]; @@ -4860,7 +4623,7 @@ static void dsp_opcode_jr_t(void) dspFlagsRetireDelay = 0; DSPUpdateRegisterBanks(); } - if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000) + if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000 - 1) { uint32_t off = dsp_pc - DSP_WORK_RAM_BASE; ds_opcode = ((uint16_t)dsp_ram_8[off] << 8) | (uint16_t)dsp_ram_8[off + 1]; @@ -4964,7 +4727,7 @@ static DSP_NOINLINE void DSPExecTimed(int32_t cycles) } #endif - if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000) + if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000 - 1) { uint32_t off = dsp_pc - DSP_WORK_RAM_BASE; opcode = ((uint16_t)dsp_ram_8[off] << 8) | (uint16_t)dsp_ram_8[off + 1]; diff --git a/src/jerry/dsp_exec_untimed.h b/src/jerry/dsp_exec_untimed.h new file mode 100644 index 00000000..37978bd1 --- /dev/null +++ b/src/jerry/dsp_exec_untimed.h @@ -0,0 +1,277 @@ +/* dsp_exec_untimed.h -- DSPExecUntimed(), the default DSP inner loop. + * + * Included TWICE from dsp.c (issue #1054), once with DSP_UNTIMED_GDB 0 + * and once with 1, to make two separate out-of-line functions: + * + * DSPExecUntimed no GDB code at all -- the path every user runs; + * DSPExecUntimedArmed per-instruction breakpoint test, idle skip off. + * + * DSPExec() picks one per slice from gdbArmedDSP, so a DSP breakpoint + * set while a plain slice is on the stack (from a watchpoint halt, say) + * takes effect at the next slice. That is the same once-per-slice + * sampling the single loop already had: it cached gdbArmedDSP in a local + * at slice entry and re-read it only after its own GDBHalt(). + * + * The includer defines DSP_UNTIMED_NAME and DSP_UNTIMED_GDB; this file + * #undefs both. dsp.c is the only includer; no include guard, by design. + */ +static DSP_NOINLINE void DSP_UNTIMED_NAME(int32_t cycles) +{ + int idleSkipActive; +#if DSP_UNTIMED_GDB + int gdbArmedSlice; +#endif + /* Issue #811 late load write-back, armed for exactly the next + * instruction (see dsp_ldx_slot). */ + uint32_t *ldxArmed = NULL; + uint32_t ldxVal = 0, ldxPc = 0; + /* Issue #811 stale indexed-store data after a divide (see + * dsp_div_store_fixups), armed for exactly the next instruction. */ + uint32_t *divArmed = NULL; + uint32_t divOld = 0, divPc = 0; + +#ifdef DSP_SINGLE_STEPPING + if (dsp_control & 0x18) + { + cycles = 1; + dsp_control &= ~0x10; + } +#endif + /* No return between here and VJP_LEAVE (perf_iface.h). */ + VJP_ENTER(VJP_DSP); + + dsp_releaseTimeSlice_flag = 0; + +#if DSP_UNTIMED_GDB + /* Same shape as GPUExec's cached locals (issue #532): a hot global + * read per emulated instruction with an opaque opcode call in + * between. Only GDBHalt() can re-arm inside a slice. */ + gdbArmedSlice = gdbArmedDSP; +#endif + dsp_in_exec++; + + /* Idle-loop fast-forward gates (issue #569). Every one of these adds + * per-instruction state the affine extrapolation does not model, so + * any of them turns the whole thing off: + * - vjs.riscIdleSkip: the user-facing core option itself; + * - blitMemoMode / blitMemoRecording: the JaguarRead and JaguarWrite + * families note reads and writes for the memo (blit_memo.h:64-65, + * src/core/jaguar.c:1139), and skipping loads would change what + * the memo records; + * - riscClockScalePct is deliberately NOT a gate (#378 overclock + * presets): SCALE_RISC_CYCLES is applied where the budget is handed out (jaguar.c), not inside + * DSPExec, so `cycles` here is already in the same currency the + * measured per-iteration cost was charged in -- cost and opcost + * are measured from live `cycles` deltas, never recomputed, and + * nothing per-instruction in this function depends on the scale. + * Scaling `n` or `cost` again would double-scale. The GPU has + * one scale-dependent per-instruction term (gpu_stall_scale_accum); + * see the note in GPUExec; + * - busArbiter.enabled (the dram_timing option): bus occupancy is + * accounted per access. Belt-and-braces here -- dsp.c never + * touches busArbiter and JaguarReadLong charges it only for + * `who == OP` -- but a DSP-side arbiter charge added later must + * not silently invalidate this; + * - vjs.gpuPipelineTiming: drives the DSP pipeline/gateway model + * (issue #313), which runs in DSPExecTimed() and never reaches + * this loop -- kept in the gate as belt-and-braces; + * - DSP_CTRL single-step / single-go: the slice is one instruction; + * - vjtrace armed or a memory watch installed (VJ_TRACE builds + * only): VJT_PCHIST_DSP in this loop and VJT_WATCH_RD on the + * DRAM read path are per-instruction / per-read side effects. + * This has to be a RUNTIME check, not `#ifdef VJ_TRACE`: the + * whole test ABI is built with -DVJ_TRACE (Makefile:158-161), + * so a compile-time gate would make every headless harness -- + * including the A/B that proves this exact -- silently measure + * a disabled feature. Both flags default off, so the cost is + * two loads per slice on trace builds and nothing at all on + * release builds. */ + idleSkipActive = vjs.riscIdleSkip + && !blitMemoMode && !blitMemoRecording + && !busArbiter.enabled + && !vjs.gpuPipelineTiming + && !(dsp_control & 0x18); +#ifdef VJ_TRACE + if (vjtrace_armed || vjtrace_nwatch) + idleSkipActive = 0; +#endif +#if DSP_UNTIMED_GDB && !defined(VJ_GDB_STUB_DISABLE_IDLE_GATE) + /* GDB stub (issue #652): DSPIdleLoopProbe() extrapolates the PC + * forward over many idle-loop iterations without visiting each one, + * exactly the class of per-instruction side effect the #569 idle-skip + * gate list above already disables for (blit memo, busArbiter, + * vjtrace watch). A DSP breakpoint or pending step would be stepped + * clean over. DSPExec() only enters this instance with gdbArmedDSP + * set, so the gate is unconditional here and absent from the plain + * instance. */ + idleSkipActive = 0; +#endif + /* Probe + reject memo are re-derived from scratch every call, so + * nothing here reaches a savestate and nothing survives a slice. */ + idleProbeStage = 0; + idleMemoCount = 0; + idleMemoNext = 0; + dsp_ldx_slot = NULL; + + /* An armed late write-back runs one instruction past the budget so a + * load/overwrite pair never straddles a slice (see dsp_ldx_slot); + * likewise a divide/indexed-store pair (see dsp_div_store_fixups). */ + while ((cycles > 0 || ldxArmed || divArmed) && DSP_RUNNING) + { + uint16_t opcode; + uint32_t index; + uint32_t pcThis; +#ifdef VJ_TRACE + VJT_PCHIST_DSP(dsp_pc); +#endif + + /* If IMASK was cleared, see if any other interrupts are pending -- + * but not until the D_FLAGS store that cleared it has retired, so + * the instruction still in the pipeline behind it (typically the + * epilogue's `jump` back to the interrupted code) runs first. */ + if (IMASKCleared && dspFlagsRetireDelay == 0) + { + DSPHandleIRQsNP(); + IMASKCleared = false; + } + + /* PC escape bail-out. When the DSP PC has wandered into a + * region that doesn't contain executable code -- register + * space at $F00000-$F1FFFF outside DSP local SRAM, or the + * unmapped territory above $E40000 -- every "fetched opcode" + * is bus-default 0xFFFF garbage that decodes to a near-zero- + * cost opcode. The inner loop then burns the entire + * timeslice without making progress, hanging the frontend. + * + * Wolf3D headless triage caught this in v2.3.0: the runtime + * watchdog logged `dsp_pc_escape pc=$00FFF004E8` (PC top-byte + * corrupted) and the harness wedged for 12+ minutes per + * frame. An earlier dsp-diag snapshot showed PC=$0006EE in + * RAM at frame 48 -- that's the upstream bug (separate from + * this bail-out: $0006EE is *valid* RAM and decodes to real + * opcodes; it's where the DSP eventually drifts INTO bad + * territory that triggers the wedge). Drain cycles here and + * let the runtime watchdog (src/core/crash_detect.c) log the + * actual escape PC. DSP_RUNNING is left alone so games that + * legitimately stop the DSP via DSPGO=0 are unaffected. + * + * Valid execution regions match JaguarReadX address decoding + * (src/core/jaguar.c): anything <= $E3FFFF (main RAM mirrored + * 4x for the bottom 8MB, cart ROM, boot ROM) plus DSP local + * SRAM. Earlier versions of this check used `<= 0x1FFFFF` + * and would have false-flagged DSP code running from a RAM + * mirror at $200000-$7FFFFF or from cart ROM at $800000+. + * Caught by Copilot review on PR #182. */ + if (!((dsp_pc <= 0x00E3FFFF) || + (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000))) + { + cycles = 0; + break; + } + + pcThis = dsp_pc; + +#if DSP_UNTIMED_GDB + /* GDB stub (issue #652): checked after the IRQ dispatch and the + * PC-escape bail, so a breakpoint on an ISR's first instruction + * still fires. GDBHalt() blocks in place (does not unwind this + * loop) until the client continues, steps, or disconnects; the + * client may disarm meanwhile, which the refresh below picks up. */ + if (gdbArmedSlice && GDBCheckPC(GDB_TGT_DSP, pcThis)) + { + GDBHalt(GDB_TGT_DSP, GDB_STOP_BREAKPOINT, pcThis); + gdbArmedSlice = gdbArmedDSP; + } +#endif + + /* `- 1`: the fetch reads off and off + 1, so the last byte of + * local RAM (only reachable at an odd PC) goes through + * DSPReadWord(), which aligns, instead of reading past + * dsp_ram_8 (cppcheck arrayIndexOutOfBoundsCond). */ + if (dsp_pc >= DSP_WORK_RAM_BASE && dsp_pc < DSP_WORK_RAM_BASE + 0x2000 - 1) + { + uint32_t off = dsp_pc - DSP_WORK_RAM_BASE; + opcode = ((uint16_t)dsp_ram_8[off] << 8) | (uint16_t)dsp_ram_8[off + 1]; + } + else + opcode = DSPReadWord(dsp_pc, DSP); + index = opcode >> 10; + dsp_opcode_first_parameter = (opcode >> 5) & 0x1F; + dsp_opcode_second_parameter = opcode & 0x1F; + dsp_pc += 2; + dsp_exec_opcode_count++; + if (divArmed && pcThis == divPc + && &dsp_reg[dsp_opcode_second_parameter] == divArmed) + { + /* Issue #811: indexed store right behind a divide into its + * data register -- the quotient has not been written back + * yet, so the store sees the old contents. */ + uint32_t quot = *divArmed; + + *divArmed = divOld; + dsp_executeOpcode(index); + *divArmed = quot; + dsp_div_store_fixups++; + divArmed = NULL; + } + else if (index == 21) + { + uint32_t *dst = &dsp_reg[dsp_opcode_second_parameter]; + uint32_t old = *dst; + + dsp_executeOpcode(index); + divArmed = DSPDivStorePeek(dst, &divPc); + divOld = old; + } + else + { + dsp_executeOpcode(index); + divArmed = NULL; + } + cycles -= dsp_opcode_cycles[index]; + + /* Issue #811: the previous instruction was an external load and + * this one overwrote its target -- the load data lands last. */ + if (ldxArmed) + { + if (pcThis == ldxPc + && &dsp_reg[dsp_opcode_second_parameter] == ldxArmed) + { + *ldxArmed = ldxVal; + dsp_load_wb_fixups++; + } + ldxArmed = NULL; + } + if (dsp_ldx_slot) + { + ldxArmed = DSPLoadOverwritePeek(&ldxVal, &ldxPc); + dsp_ldx_slot = NULL; + } + + /* Idle-loop fast-forward (issue #569). A taken `jr` that landed + * on or behind its own address, at most 8 words back, is the only + * candidate; the unsigned difference rejects a not-taken jr + * (pc = pcThis + 2) and every forward branch in one compare. */ + if (idleSkipActive && index == 53 + && (uint32_t)(pcThis - dsp_pc) <= 14) + cycles = DSPIdleLoopProbe(cycles, dsp_pc, pcThis); + + /* Age out a D_FLAGS store once the instruction that was already + * behind it in the pipeline has run (see DSPWriteLong, D_FLAGS + * case). Retiring re-opens interrupt recognition and commits the + * register-bank switch the store selected. */ + if (dspFlagsRetireDelay) + { + dspFlagsRetireDelay--; + if (dspFlagsRetireDelay == 0) + DSPUpdateRegisterBanks(); + } + } + + dsp_in_exec--; + + VJP_LEAVE(VJP_DSP); +} + +#undef DSP_UNTIMED_NAME +#undef DSP_UNTIMED_GDB