Build and verify Tekken 3 TAS from a clean clone - #29
Merged
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
40 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/src/dirty_ram_interp.c">
<violation number="1" location="runtime/src/dirty_ram_interp.c:2767">
P1: When source mode takes an IRQ inside a loop whose resume PC is mid-function clean text, this condition removes the only instruction bound and the loop has no safe exit. The player can spin forever instead of failing closed; retain a finite cap and halt with the documented precise-slice diagnostic when no dispatchable PC is reached.</violation>
</file>
<file name="runtime/include/source_gpu_command_projection.h">
<violation number="1" location="runtime/include/source_gpu_command_projection.h:210">
P1: When a valid GP0(C0) transfer uses zero height for 512 rows, this leaves `transfer_words` zero and never enters phase 8, so GPUSTAT ready-to-send and GPUREAD progression are missing. Expand zero height for both A0 and C0.</violation>
</file>
<file name="runtime/src/source_gpu_runtime.c">
<violation number="1" location="runtime/src/source_gpu_runtime.c:86">
P1: When GP1(04) selects DMA direction 1, this expression clears GPUSTAT bit 25 because `1 & 2` is zero. Source-mode polling or DMA setup can then wait forever for a request that the native GPU reports as asserted; map direction 1 to bit 25, and make directions 2 and 3 mirror bits 28 and 27 respectively.</violation>
</file>
<file name="runtime/src/memory.c">
<violation number="1" location="runtime/src/memory.c:1665">
P1: When `CP0.SR.IsC` is set, a write to `0xFFFE0130` no longer updates `cache_ctrl` because this early branch returns before the cache-control handler below. Preserve the cache-control special case before diverting other stores to the isolated-cache model.</violation>
</file>
<file name="runtime/src/sio.c">
<violation number="1" location="runtime/src/sio.c:1438">
P2: The `sio_dualshock_rumble_test` target now fails to link because this new call has no implementation in that test target. Provide a test stub or link the implementation that owns `debug_server_update_poll` before merging.</violation>
</file>
<file name="runtime/include/gpu_render.h">
<violation number="1" location="runtime/include/gpu_render.h:62">
P2: When source-GPU mode runs with the OpenGL or Vulkan backend selected, these facade calls return failure because the source path is not part of the backend vtable, and `gpu.c` aborts the process. Force software for source-GPU mode before backend selection, or add a defined source-operation dispatch/fallback for hardware backends.</violation>
</file>
<file name="runtime/src/cdrom.c">
<violation number="1" location="runtime/src/cdrom.c:3611">
P2: When `PSX_CD_EXPLICIT_SEEK_MODEL` differs between save and load, this conditional byte changes the CD snapshot wire layout without changing the boot-state version, so the load rejects the CD section after partially restoring the machine. Make the snapshot format profile-independent or bump `BOOT_STATE_VERSION` and `BOOT_STATE_VERSION_MIN_READ` so incompatible states are rejected before any sections are applied.
(Based on your team's feedback about savestate wire changes.)</violation>
</file>
<file name="runtime/include/input_route_raster_clock.h">
<violation number="1" location="runtime/include/input_route_raster_clock.h:77">
P2: When GP1 reset occurs during the short interlaced field, this branch switches to progressive mode but leaves `lines` at 262. The next progressive frame can wrap and schedule its VBlank one scanline early; restore the derived line count to 263 during reset.</violation>
</file>
<file name="runtime/src/spu.c">
<violation number="1" location="runtime/src/spu.c:923">
P3: Because `voice_next_sample` returns through `source_voice_sample` first, this `source_key_timing` block can never execute. Remove the unreachable branch so the inactive-voice path does not promise source behavior that cannot run.</violation>
<violation number="2" location="runtime/src/spu.c:1921">
P2: When a v6 savestate is loaded under a different `PSX_GPU_DMA_MODEL`, this profile-dependent SPU wire size passes header validation but fails at the SPU section after earlier sections have already been restored. Bump `BOOT_STATE_VERSION` and `BOOT_STATE_VERSION_MIN_READ` (or encode the profile/layout in the header) before changing the snapshot wire layout.
(Based on your team's feedback about savestate wire versioning.)</violation>
</file>
<file name="runtime/src/debug_server.c">
<violation number="1" location="runtime/src/debug_server.c:14795">
P2: When `PSX_INPUT_HISTOGRAM_RANGE` starts at frame 0, the configuration already opens the frame-0 interval before execution. This call closes and reopens it at the same frame, so the next VBlank attempts to create `instructions-000000.tsv` again and aborts the replay; skip this boundary call for the initial frame.</violation>
</file>
<file name="runtime/src/gpu_sw_renderer.c">
<violation number="1" location="runtime/src/gpu_sw_renderer.c:349">
P2: Textured source draws currently store the texel STP bit as the framebuffer mask bit. This makes later mask-checking draws skip pixels unexpectedly; discard the texel bit after using it for blending and let `g_mask_set_bit` control the stored bit.</violation>
</file>
<file name="runtime/src/timers.c">
<violation number="1" location="runtime/src/timers.c:289">
P2: When the exact/COSIM cycle path advances across a timer2 deadline, this guard exits the process instead of splitting and servicing the source timer. Make the timer2 advance path consume deadline-sized chunks (or include this deadline in the exact scheduler) so source timer2 remains usable outside the production fast path.</violation>
</file>
<file name="runtime/include/source_gpu_polygon_projection.h">
<violation number="1" location="runtime/include/source_gpu_polygon_projection.h:36">
P2: When a valid triangle spans 512 scanlines, this returns success with zero work and `sw_draw_source_triangle` drops the primitive without drawing it. Remove the span rejection so the clipped rows are walked; coordinate admission already handles the supported range.</violation>
</file>
<file name="runtime/src/main.cpp">
<violation number="1" location="runtime/src/main.cpp:7046">
P2: When a preloaded input route is combined with solo self-check replay, the route observer records an input but this branch never acknowledges its SIO delivery, so the next vblank aborts verification. Skip route polling during replay or explicitly reconcile the observer state before the boundary.</violation>
</file>
<file name="runtime/src/psx_cycles.c">
<violation number="1" location="runtime/src/psx_cycles.c:144">
P2: When the source GPU runtime is active, idle-loop skipping can jump past a GPU service deadline because the idle observation bound does not include `source_gpu_runtime_cycles_to_event()`. Include the source-GPU deadline in `devices_cycles_to_next_idle_event()` so raster, frame-return, and source-DMA callbacks remain on their scheduled cycle.</violation>
</file>
<file name="runtime/tests/test_critical_exception.c">
<violation number="1" location="runtime/tests/test_critical_exception.c:11">
P2: `test_critical_exception.c` compiles only itself plus an `#include` of `../src/traps.c`, yet `psx_syscall`'s enabled path calls `source_gpu_runtime_active()` (traps.c line ~1076). Main sets `enabled` from runtime `argv`, so that branch is reachable and the call cannot be eliminated by LTO; no definition of `source_gpu_runtime_active` exists in this translation unit, so `critical_exception_test` fails to link with an undefined reference and the test can never run. Every comparable test (test_dma_cd_source.c, test_dma_completion_deadline.c, test_dma_otc_timing.c, ...) defines `int source_gpu_runtime_active(void) { return 0; }`. Add the same stub so the executable links and the assertions are actually exercised.</violation>
</file>
<file name="runtime/tests/test_source_gpu_reset_projection.c">
<violation number="1" location="runtime/tests/test_source_gpu_reset_projection.c:19">
P2: In the kind==0 branch, the polygon is never rejected as unsupported, so `assert(!source_gpu_command_write(&s,0x10000))` fails. The 0x10000 word is not parsed as an unsupported command: it is consumed as the third vertex of the 0x28 triangle. `source_gpu_command_length(0x28000000)` is 4 (stride 1 for opcode 0x28), so `process` only runs the polygon branch once `count` reaches 4. `source_gpu_command_polygon_cost` checks `clip_y0>511 || clip_y1>511 || ((display_mode&0x24)==0x24 && !(draw_mode&0x400) && !field_valid)`; in kind 0 display_mode is 0 and clip_y1 is 63 (=0xe407ffff>>10), so nothing rejects, and the degenerate triangle (area 0) returns cost 0. The write therefore returns 1 with no error, and the assertion aborts the test on every run. Mask rejection is never triggered by `mask_bits=1` in the polygon path (mask only changes the cost term `masked_or_blended`).</violation>
</file>
<file name="runtime/tests/test_irq_cache_block.c">
<violation number="1" location="runtime/tests/test_irq_cache_block.c:49">
P2: The ternary `cpu->pc=taken?0x80000554:0x80000554` has identical branches, so the just-computed `taken` result never influences the branch. The BNE is always treated as falling through to 0x80000554, so the test never exercises the taken path that the comment (`ordinary BNE with one delay slot`) implies it verifies. Give the taken branch its real target (computed from the branch offset and whether `taken` is true) instead of the duplicated fall-through address.</violation>
</file>
<file name="runtime/tests/test_cdrom_random_tape.c">
<violation number="1" location="runtime/tests/test_cdrom_random_tape.c:23">
P2: The vector/load verification in this test never runs under any registered test invocation. Both `add_test(NAME cdrom_random_tape_test ...)` in runtime/CMakeLists.txt:78 and the `tas_cdrom_random_tape_*` registration in tools/tasreplays/tests/CMakeLists.txt invoke the executable with no arguments, so argc is always 1. The `if (argc==2)` block (cd_tape_load of a real tape plus the 22 source vectors) and the `if (argc==3 ... --reject)` block are therefore dead code, and cd_tape_load is never exercised at all. Either make the registered tests pass a generated tape file and a reject fixture as arguments, or emit a clear failure when the program runs with no tape path instead of silently skipping the file's core verification.
Separately, the manual header-level checks are correct: cursor deltas, restore identity/count/cursor guards, UINT32_MAX masking, and exhaustion behavior all match the cdrom_random_tape.h implementation.</violation>
</file>
<file name="runtime/include/timer2_source_clock.h">
<violation number="1" location="runtime/include/timer2_source_clock.h:70">
P2: When mode `0x100` is selected with target or overflow IRQ enabled, `timer2_source_cpu` intentionally performs no update but `timer2_source_next` still schedules the skipped timer’s target/overflow deadline. The runtime then performs unnecessary device servicing at that deadline forever; return the periodic cap for this encoding as well.</violation>
</file>
<file name="recompiler/src/full_function_emitter.cpp">
<violation number="1" location="recompiler/src/full_function_emitter.cpp:828">
P2: When a branch's delay-slot address is also a block leader, this scan stops before that instruction even though the emitter executes it inline. The resulting `count * 8u` budget can miss an event deadline by one instruction, so include the always-executed delay slot in the slice budget.</violation>
</file>
<file name="runtime/CMakeLists.txt">
<violation number="1" location="runtime/CMakeLists.txt:83">
P2: CTest never runs the new source-clock or source-reset fixtures, so regressions in these paths can pass the build and test suite unnoticed. Register each with its generated tape argument, or explicitly mark it disabled and document the external-fixture requirement.</violation>
</file>
<file name="runtime/src/gpu.c">
<violation number="1" location="runtime/src/gpu.c:2287">
P2: When a savestate is loaded after continuing a mono-polyline, the restored `gp0_state` can describe one vertex while these unsaved counters retain the post-save value. `gpu_dma_source_ll_ready()` can then admit or reject the next linked-list transfer incorrectly; include both admission fields in the GPU snapshot wire state (and its versioned layout).</violation>
</file>
<file name="runtime/include/source_cpu_block_bound.h">
<violation number="1" location="runtime/include/source_cpu_block_bound.h:17">
P2: The full BIOS emitter passes `count * 8u` as the slice budget, but this helper treats the argument as an instruction count. Near a deadline, it scans and bounds up to eight times past the block, unnecessarily routing otherwise safe blocks through the precise interpreter; pass the actual instruction count separately from the cycle budget.</violation>
</file>
<file name="runtime/include/timer1_source_clock.h">
<violation number="1" location="runtime/include/timer1_source_clock.h:60">
P2: When the guest writes an unused Timer1 subregister such as `+0xC` while `counter == target`, this line reasserts the target flag and can reset the counter, even though native timer MMIO ignores that write. Restrict the match check to valid register offsets.</violation>
</file>
<file name="runtime/tests/test_timer1_raster_integration.c">
<violation number="1" location="runtime/tests/test_timer1_raster_integration.c:44">
P2: The second `advance(&r, edge)` is a no-op: `edge` is an absolute cycle computed as `now + delta`, and the first `advance(&r, edge)` sets `now=edge` (advance assigns `now=to`). So the second `advance(&r, edge)` advances 0 cycles and never reaches or crosses a new H edge after the `timers_write(0x1f801110,7)`. The comment says "At the next H edge a write at the same timestamp follows that edge", but no ordering is actually exercised and `assert(timers_read(0x1f801110)==7)` passes trivially. Recompute the next edge from the state after the write (or from the current `phase`/`remaining`) so the advance actually crosses a subsequent H edge, and update the expected count accordingly.</violation>
</file>
<file name="runtime/tests/test_irq_prefetch.c">
<violation number="1" location="runtime/tests/test_irq_prefetch.c:50">
P2: The fixed-size 0x2000-byte guest-image read ignores the `fread` return value. The sibling `test_irq_cache_block.c` (and `test_source_gpu_*.c`, `test_bios_syscall_resume.c`) assert the read count for the same kind of load, so this should validate it too. A truncated or empty committed guest file then runs stale/uninitialized RAM through `dirty_ram_mark_executable_range` and the interpreter instead of failing loudly, producing misleading JSON or a deep-interpreter crash that costs time to debug.</violation>
</file>
<file name="runtime/tests/test_source_gpu_command_projection.c">
<violation number="1" location="runtime/tests/test_source_gpu_command_projection.c:102">
P2: In the ordinary-command loop, after setup() sets budget=-2 and write() queues one word, `update(129)` refills and pops that word, so `assert(s.count==0 && s.budget==-2)` is internally inconsistent. With budget=-2, update() computes missing=256-(-2)=258, threshold (258+1)/2=129, so elapsed 129>=129 makes budget=256; process() then pops the queued word and charges 2 (ordinary-environment command), leaving budget=254, count=0. If count==0 the word was popped (budget is 254), and if budget stayed -2 the word was not popped (count would be 1). The expected budget should be 254, not -2; as written the assertion (and every iteration of the c loop) fails.</violation>
</file>
<file name=".github/workflows/tasreplays.yml">
<violation number="1" location=".github/workflows/tasreplays.yml:4">
P2: Both the `pull_request` and `push` triggers are scoped to `branches: [tasreplays]`. This workflow is added on the tasreplays branch and is meant to merge into `main`, but once it lands, no push or PR targeting `main` (or any other branch) will trigger it, so the asset-free TAS source checks (`tas_bk2_intake`, `tas_replay_recipe`) stop running entirely after the branch merges. Broaden the branch scope (e.g. include `main` or drop the branch filter) so the committed checks keep running after merge.</violation>
</file>
<file name="tools/tasreplays/tekken3.py">
<violation number="1" location="tools/tasreplays/tekken3.py:324">
P3: When `--movie` is corrupt or not a ZIP, setup raises an uncaught `zipfile.BadZipFile` instead of reporting the controlled `ERROR:` failure. Catch `zipfile.BadZipFile` in the CLI handler or normalize it in `prepare_movie`.</violation>
</file>
<file name="runtime/tests/test_dma_gpu_linked_list_source.c">
<violation number="1" location="runtime/tests/test_dma_gpu_linked_list_source.c:100">
P2: When this fixture is compiled without `PSX_TEST_SOURCE_LL_IMPLEMENTED` (the `#else` branch at the top deliberately sets `PSX_GPU_DMA_MODEL=""`), the shared main-loop vote still asserts `expected=ready&&n<=49` against IRQ/busy state that only holds for the linked-list source model. In the default (non-source) DMA model ch2 with sync-mode 2 goes through `schedule_delayed_complete(2, execute_ch2_gpu(), ...)`, so at `try_execute(2)` return the completion IRQ has not fired, the busy bit is still set, and `irqs==expected` (for ready&&n<=49) is false. The non-LL configuration therefore compiles but always fails its main-loop vote; the only config the external driver seems to build defines `PSX_TEST_SOURCE_LL_IMPLEMENTED`, making the `#else` path a broken, untested configuration. Guard the `expected`-voting loop with the same `#ifdef`, or drop the empty-model branch.</violation>
</file>
<file name="runtime/tests/test_dma_completion_deadline.c">
<violation number="1" location="runtime/tests/test_dma_completion_deadline.c:142">
P3: The `masked` matrix dimension has no observable effect on the test outcome. It only sets `i_mask` to 0 or 8, but nothing in this fixture reads `i_mask` to gate interrupt delivery: `psx_irq_raise` increments the `irqs` counter unconditionally, `timely` checks only `irqs==1`, and `dma_cycles_to_internal_event` doesn't consult the mask. Every `m=0` and `m=1` iteration therefore executes identically, so the test gives false confidence that masked-IRQ delivery is covered. Drop the `masked` loop and the `i_mask` write, or make the mask actually gate the assertion (e.g. assert DMA completion is not observed when masked).</violation>
</file>
<file name="runtime/tests/test_timer1_source_clock.c">
<violation number="1" location="runtime/tests/test_timer1_source_clock.c:10">
P2: The timer1 writes are wrapped inside assert() arguments (lines 11-13 and the IRQ-rejection checks at lines 24-25), so when the test is compiled with NDEBUG the entire write call is compiled out and never executes. Under NDEBUG the state machine stays at its reset/before value, the per-row state compares fire, and the test fails while the two IRQ-rejection writes and their memcmp checks silently vanish. Keep the side-effecting call in its own statement and assert only its result, matching the repo's CHECK-with-failures pattern used by the other authored clock fixtures (e.g. test_cdrom_source_clock.c).</violation>
</file>
<file name="runtime/include/source_cpu_boundary_probe.h">
<violation number="1" location="runtime/include/source_cpu_boundary_probe.h:3">
P3: This header uses `uint32_t`, `uint64_t`, `CPUState`, `g_psx_icache_tv`, `g_psx_icache_active`, and `dma_cpu_read_penalty()` but only includes `<stdio.h>` and `<stdlib.h>`. It compiles today only because the sole includer, `runtime/src/source_gpu_runtime.c`, includes `source_gpu_runtime.h`, `psx_icache.h`, and `dma.h` before it. Since the probe functions are `static`, any second translation unit that includes this header gets its own copy and opens the same `<dir>/cpu-return.tsv` / `<dir>/cpu-boundary.tsv` with `fopen(path,"wx")`, so the second TU's exclusive open fails and calls `abort()`. Make the header self-contained (include `<stdint.h>` and the headers that declare `CPUState`, the `g_psx_icache_*` symbols, and `dma_cpu_read_penalty`) so it is robust to include order and multiple inclusion sites.</violation>
</file>
<file name="runtime/tests/test_muldiv_deferred.c">
<violation number="1" location="runtime/tests/test_muldiv_deferred.c:31">
P2: All fixture verification in this test relies on assert(): the 25,944-case loop checks want_clock/want_deadline/want_give, feof, and cases only via assert. Under an NDEBUG build (CMake Release adds -DNDEBUG) those assertions compile out, so the test prints "PASS ..." and returns 0 without verifying anything. Since this test exists to verify the muldiv/deferred timing, that is a silent false pass. Additionally assert(f)/assert(argc==2) are also stripped, so a failed fixture open dereferences NULL in fscanf rather than failing cleanly. Guard against NDEBUG (e.g. #error at the top) or replace the fixture assertions with explicit printf-FAIL + return 1 checks like the first synthetic check already does.</violation>
</file>
<file name="runtime/tests/test_icache_isolated_store.c">
<violation number="1" location="runtime/tests/test_icache_isolated_store.c:29">
P2: The fetch-timing sub-test hardcodes 0x27a0/0x27b0 and asserts that exactly the 0x27a0 line was invalidated (refill +7) while the adjacent 0x27b0 line stays warm. That only holds when the current vector's store address falls inside 0x27a0's 16-byte line, because the tags were initialized to 0x2000+i*4, so index (0x27a0&0xFFC)>>2 already equals 0x27a0 and is a hit unless the store invalidated it. The guard `status && (biu&0x804)==0x804` selects every IsC/tag-test vector regardless of `addr`, so any matrix vector with a store address outside the 0x27a0 line makes `psx_cycle_count!=7` (and the read_absorb checks) fail spuriously. Add an explicit check that the vector's store line is 0x27a0's (and not 0x27b0's) before running the fetch-timing assertions, or drive the assertion addresses from `addr`; otherwise the run result depends on the external generator happening to emit only a matching address.</violation>
</file>
<file name="runtime/tests/test_timer2_source_scheduler.c">
<violation number="1" location="runtime/tests/test_timer2_source_scheduler.c:37">
P2: This fixture never enables the source timer2 scheduler it claims to test. timers_init() only arms source_timer2 when the process environment sets both PSX_TIMER2_MODEL=octoshock-2.2.2 and PSX_TIMER1_MODEL (timers.c:117-124), and the build must additionally define PSX_ENABLE_BLOCK_CYCLES. The test calls timers_init() directly and reads vectors from stdin, with no setenv and no CMake registration in this repo (registration is only a comment in check_test_registration.cmake pointing at the external tas-replay-research/verify_timer2.py harness). Run bare, source_timer2_enabled stays 0, so the whole main loop silently verifies the legacy timer2 path instead of the source scheduler, while the 'restore'/'timer0-irq' probes return 9 instead of being rejected — with no diagnostic that the intended path was skipped. Initialize the source profile inside the fixture, or assert the env vars are set (and abort otherwise), so a bare run cannot silently exercise the wrong path.</violation>
</file>
<file name="docs/TAS_ACCURACY.md">
<violation number="1" location="docs/TAS_ACCURACY.md:9">
P3: Missing whitespace between numerals and the following word throughout this prose makes it hard to read. Examples: "contains7,974 inputs", "all512 4KiB RAM-page hashes ... reference;28 full 2MiB snapshots", "wins Arcade at8.80 seconds", "All57 resulting C files", "bounded at8,400 frames:7,974 original inputs plus426 neutral inputs", and "stopped the research tail after return10,704". Insert a space after each number (and after each comma-separated digit group) before the following word.</violation>
</file>
<file name="runtime/tests/test_source_gpu_environment.c">
<violation number="1" location="runtime/tests/test_source_gpu_environment.c:13">
P3: `to(t)` truncates the cycle delta on the cast: `psx_advance_cycles((uint32_t)(t-psx_get_cycle_count()))`. The assert only guards the lower bound (`t>=count`), not the upper one, so any stamp gap of more than 2^32 cycles (~126 s at 33.868 MHz) silently wraps and advances the wrong amount, corrupting the very timing this verification test is meant to check. Since this harness exists to catch timing deviations, widen the delta before advancing instead of truncating.</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
| * Keep the legacy guard outside this explicit profile. Saturate the | ||
| * diagnostic iteration count rather than overflowing on a guest spin. */ | ||
| const int source_owned_slice = source_gpu_runtime_active(); | ||
| for (uint32_t i = 0; source_owned_slice || i < MAX_PRECISE_INSNS; |
There was a problem hiding this comment.
P1: When source mode takes an IRQ inside a loop whose resume PC is mid-function clean text, this condition removes the only instruction bound and the loop has no safe exit. The player can spin forever instead of failing closed; retain a finite cap and halt with the documented precise-slice diagnostic when no dispatchable PC is reached.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/src/dirty_ram_interp.c, line 2767:
<comment>When source mode takes an IRQ inside a loop whose resume PC is mid-function clean text, this condition removes the only instruction bound and the loop has no safe exit. The player can spin forever instead of failing closed; retain a finite cap and halt with the documented precise-slice diagnostic when no dispatchable PC is reached.</comment>
<file context>
@@ -2573,10 +2755,18 @@ static void psx_run_precise(CPUState *cpu, uint32_t bcyc, int deadline_entry) {
+ * Keep the legacy guard outside this explicit profile. Saturate the
+ * diagnostic iteration count rather than overflowing on a guest spin. */
+ const int source_owned_slice = source_gpu_runtime_active();
+ for (uint32_t i = 0; source_owned_slice || i < MAX_PRECISE_INSNS;
+ i += i != UINT32_MAX) {
+ if ((source_owned_slice || !irq_taken) && precise_irq_before(cpu,pc)) {
</file context>
| unsigned width=s->queue[2]&1023u; | ||
| unsigned height=(s->queue[2]>>16)&(command==0xa0?511u:1023u); | ||
| if(!width)width=1024; | ||
| if(command==0xa0 && !height)height=512; |
There was a problem hiding this comment.
P1: When a valid GP0(C0) transfer uses zero height for 512 rows, this leaves transfer_words zero and never enters phase 8, so GPUSTAT ready-to-send and GPUREAD progression are missing. Expand zero height for both A0 and C0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/include/source_gpu_command_projection.h, line 210:
<comment>When a valid GP0(C0) transfer uses zero height for 512 rows, this leaves `transfer_words` zero and never enters phase 8, so GPUSTAT ready-to-send and GPUREAD progression are missing. Expand zero height for both A0 and C0.</comment>
<file context>
@@ -0,0 +1,290 @@
+ unsigned width=s->queue[2]&1023u;
+ unsigned height=(s->queue[2]>>16)&(command==0xa0?511u:1023u);
+ if(!width)width=1024;
+ if(command==0xa0 && !height)height=512;
+ if(command==0xc0 && height>512)height&=511u;
+ s->dispatch.kind=SOURCE_GPU_DISPATCH_COMMAND;s->dispatch.count=3;
</file context>
| int source_gpu_runtime_active(void) {return enabled;} | ||
| int source_gpu_runtime_ready(void) {return enabled?source_gpu_command_ready(&command_state):-2;} | ||
| uint32_t source_gpu_runtime_status_bits(void) { | ||
| uint32_t bits=(command_state.dma_direction&2u)?1u<<25:0; |
There was a problem hiding this comment.
P1: When GP1(04) selects DMA direction 1, this expression clears GPUSTAT bit 25 because 1 & 2 is zero. Source-mode polling or DMA setup can then wait forever for a request that the native GPU reports as asserted; map direction 1 to bit 25, and make directions 2 and 3 mirror bits 28 and 27 respectively.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/src/source_gpu_runtime.c, line 86:
<comment>When GP1(04) selects DMA direction 1, this expression clears GPUSTAT bit 25 because `1 & 2` is zero. Source-mode polling or DMA setup can then wait forever for a request that the native GPU reports as asserted; map direction 1 to bit 25, and make directions 2 and 3 mirror bits 28 and 27 respectively.</comment>
<file context>
@@ -0,0 +1,128 @@
+int source_gpu_runtime_active(void) {return enabled;}
+int source_gpu_runtime_ready(void) {return enabled?source_gpu_command_ready(&command_state):-2;}
+uint32_t source_gpu_runtime_status_bits(void) {
+ uint32_t bits=(command_state.dma_direction&2u)?1u<<25:0;
+ if(!command_state.phase && !command_state.count && command_state.budget>=0)bits|=1u<<26;
+ if(source_gpu_command_ready(&command_state)>0)bits|=1u<<28;
</file context>
| static void psx_write_word_raw(uint32_t addr, uint32_t val) { | ||
| g_guest_store_count++; | ||
| /* Isolated stores target the cache before memory-address routing. */ | ||
| if (sr_ptr && (*sr_ptr & 0x10000u)) { |
There was a problem hiding this comment.
P1: When CP0.SR.IsC is set, a write to 0xFFFE0130 no longer updates cache_ctrl because this early branch returns before the cache-control handler below. Preserve the cache-control special case before diverting other stores to the isolated-cache model.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/src/memory.c, line 1665:
<comment>When `CP0.SR.IsC` is set, a write to `0xFFFE0130` no longer updates `cache_ctrl` because this early branch returns before the cache-control handler below. Preserve the cache-control special case before diverting other stores to the isolated-cache model.</comment>
<file context>
@@ -1628,6 +1661,11 @@ void psx_write_word(uint32_t addr, uint32_t val) {
static void psx_write_word_raw(uint32_t addr, uint32_t val) {
g_guest_store_count++;
+ /* Isolated stores target the cache before memory-address routing. */
+ if (sr_ptr && (*sr_ptr & 0x10000u)) {
+ psx_icache_isolated_store(addr, cache_ctrl);
+ return;
</file context>
Suggested change
| if (sr_ptr && (*sr_ptr & 0x10000u)) { | |
| if (addr != 0xFFFE0130u && sr_ptr && (*sr_ptr & 0x10000u)) { |
| /* Read poll. Analog (or in-config) uses the 8-byte format with the | ||
| * four stick axes; a plain digital pad uses the 4-byte format. */ | ||
| const uint16_t btn = pad_buttons[lp]; | ||
| const uint16_t btn = debug_server_update_poll(lp, pad_buttons[lp], pad_analog[lp]); |
There was a problem hiding this comment.
P2: The sio_dualshock_rumble_test target now fails to link because this new call has no implementation in that test target. Provide a test stub or link the implementation that owns debug_server_update_poll before merging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/src/sio.c, line 1438:
<comment>The `sio_dualshock_rumble_test` target now fails to link because this new call has no implementation in that test target. Provide a test stub or link the implementation that owns `debug_server_update_poll` before merging.</comment>
<file context>
@@ -1415,7 +1435,7 @@ static void pad_process_byte(uint8_t tx_byte) {
/* Read poll. Analog (or in-config) uses the 8-byte format with the
* four stick axes; a plain digital pad uses the 4-byte format. */
- const uint16_t btn = pad_buttons[lp];
+ const uint16_t btn = debug_server_update_poll(lp, pad_buttons[lp], pad_analog[lp]);
pad_response[0] = cur_id;
pad_response[1] = 0x5A;
</file context>
| unsigned failures=0,cases=0; | ||
| uint64_t starts[]={0,123,54337406};unsigned counts[]={1,7,17},futures[]={0,2,7,100}; | ||
| for(unsigned s=0;s<3;s++)for(unsigned w=0;w<3;w++)for(unsigned t=0;t<4;t++) | ||
| for(int e=0;e<2;e++)for(int m=0;m<2;m++){cases++;failures+=run_case(starts[s],counts[w],futures[t],e,m);} |
There was a problem hiding this comment.
P3: The masked matrix dimension has no observable effect on the test outcome. It only sets i_mask to 0 or 8, but nothing in this fixture reads i_mask to gate interrupt delivery: psx_irq_raise increments the irqs counter unconditionally, timely checks only irqs==1, and dma_cycles_to_internal_event doesn't consult the mask. Every m=0 and m=1 iteration therefore executes identically, so the test gives false confidence that masked-IRQ delivery is covered. Drop the masked loop and the i_mask write, or make the mask actually gate the assertion (e.g. assert DMA completion is not observed when masked).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/tests/test_dma_completion_deadline.c, line 142:
<comment>The `masked` matrix dimension has no observable effect on the test outcome. It only sets `i_mask` to 0 or 8, but nothing in this fixture reads `i_mask` to gate interrupt delivery: `psx_irq_raise` increments the `irqs` counter unconditionally, `timely` checks only `irqs==1`, and `dma_cycles_to_internal_event` doesn't consult the mask. Every `m=0` and `m=1` iteration therefore executes identically, so the test gives false confidence that masked-IRQ delivery is covered. Drop the `masked` loop and the `i_mask` write, or make the mask actually gate the assertion (e.g. assert DMA completion is not observed when masked).</comment>
<file context>
@@ -0,0 +1,155 @@
+ unsigned failures=0,cases=0;
+ uint64_t starts[]={0,123,54337406};unsigned counts[]={1,7,17},futures[]={0,2,7,100};
+ for(unsigned s=0;s<3;s++)for(unsigned w=0;w<3;w++)for(unsigned t=0;t<4;t++)
+ for(int e=0;e<2;e++)for(int m=0;m<2;m++){cases++;failures+=run_case(starts[s],counts[w],futures[t],e,m);}
+ printf("RESULT cases=%u failures=%u\n",cases,failures);return failures?1:0;
+}
</file context>
| @@ -0,0 +1,59 @@ | |||
| #ifndef PSX_SOURCE_CPU_BOUNDARY_PROBE_H | |||
| #define PSX_SOURCE_CPU_BOUNDARY_PROBE_H | |||
| #include <stdio.h> | |||
There was a problem hiding this comment.
P3: This header uses uint32_t, uint64_t, CPUState, g_psx_icache_tv, g_psx_icache_active, and dma_cpu_read_penalty() but only includes <stdio.h> and <stdlib.h>. It compiles today only because the sole includer, runtime/src/source_gpu_runtime.c, includes source_gpu_runtime.h, psx_icache.h, and dma.h before it. Since the probe functions are static, any second translation unit that includes this header gets its own copy and opens the same <dir>/cpu-return.tsv / <dir>/cpu-boundary.tsv with fopen(path,"wx"), so the second TU's exclusive open fails and calls abort(). Make the header self-contained (include <stdint.h> and the headers that declare CPUState, the g_psx_icache_* symbols, and dma_cpu_read_penalty) so it is robust to include order and multiple inclusion sites.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/include/source_cpu_boundary_probe.h, line 3:
<comment>This header uses `uint32_t`, `uint64_t`, `CPUState`, `g_psx_icache_tv`, `g_psx_icache_active`, and `dma_cpu_read_penalty()` but only includes `<stdio.h>` and `<stdlib.h>`. It compiles today only because the sole includer, `runtime/src/source_gpu_runtime.c`, includes `source_gpu_runtime.h`, `psx_icache.h`, and `dma.h` before it. Since the probe functions are `static`, any second translation unit that includes this header gets its own copy and opens the same `<dir>/cpu-return.tsv` / `<dir>/cpu-boundary.tsv` with `fopen(path,"wx")`, so the second TU's exclusive open fails and calls `abort()`. Make the header self-contained (include `<stdint.h>` and the headers that declare `CPUState`, the `g_psx_icache_*` symbols, and `dma_cpu_read_penalty`) so it is robust to include order and multiple inclusion sites.</comment>
<file context>
@@ -0,0 +1,59 @@
+#ifndef PSX_SOURCE_CPU_BOUNDARY_PROBE_H
+#define PSX_SOURCE_CPU_BOUNDARY_PROBE_H
+#include <stdio.h>
+#include <stdlib.h>
+/* One passive snapshot per original-model frontend return. Unlike input-route
</file context>
Comment on lines
+923
to
+926
| if (source_key_timing) { | ||
| if (source_play_delay[idx]) source_play_delay[idx]--; | ||
| else adsr_run(idx, v); | ||
| } |
There was a problem hiding this comment.
P3: Because voice_next_sample returns through source_voice_sample first, this source_key_timing block can never execute. Remove the unreachable branch so the inactive-voice path does not promise source behavior that cannot run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/src/spu.c, line 923:
<comment>Because `voice_next_sample` returns through `source_voice_sample` first, this `source_key_timing` block can never execute. Remove the unreachable branch so the inactive-voice path does not promise source behavior that cannot run.</comment>
<file context>
@@ -804,9 +817,115 @@ typedef char spu_shadow_voice_count_check[
+ * voices start in Attack; a cancelled key-on still leaves their
+ * envelope clock running. Keep this readback behavior independent
+ * of our silent-voice decoder optimization. */
+ if (source_key_timing) {
+ if (source_play_delay[idx]) source_play_delay[idx]--;
+ else adsr_run(idx, v);
</file context>
Suggested change
| if (source_key_timing) { | |
| if (source_play_delay[idx]) source_play_delay[idx]--; | |
| else adsr_run(idx, v); | |
| } | |
| /* Source mode dispatches above; this path is normal-mode only. */ |
| @@ -0,0 +1,66 @@ | |||
| # TAS accuracy profile and Tekken 3 validation | |||
There was a problem hiding this comment.
P3: Missing whitespace between numerals and the following word throughout this prose makes it hard to read. Examples: "contains7,974 inputs", "all512 4KiB RAM-page hashes ... reference;28 full 2MiB snapshots", "wins Arcade at8.80 seconds", "All57 resulting C files", "bounded at8,400 frames:7,974 original inputs plus426 neutral inputs", and "stopped the research tail after return10,704". Insert a space after each number (and after each comma-separated digit group) before the following word.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/TAS_ACCURACY.md, line 9:
<comment>Missing whitespace between numerals and the following word throughout this prose makes it hard to read. Examples: "contains7,974 inputs", "all512 4KiB RAM-page hashes ... reference;28 full 2MiB snapshots", "wins Arcade at8.80 seconds", "All57 resulting C files", "bounded at8,400 frames:7,974 original inputs plus426 neutral inputs", and "stopped the research tail after return10,704". Insert a space after each number (and after each comma-separated digit group) before the following word.</comment>
<file context>
@@ -0,0 +1,66 @@
+It builds on `f23c5ba1a220fe1ca8818cc48c026d6c2f7f2c64`, the base used for the
+measured campaign, with the final native integration's overlay ABI22.
+
+The unchanged Spikestuff movie contains7,974 inputs. The integrated native
+build wins Arcade at8.80 seconds. Every original-input return clock and all512
+4KiB RAM-page hashes match the independent Octoshock2.2.2 reference;28 full
</file context>
| #include <stdio.h> | ||
| extern void memory_init(const char *); | ||
| extern uint8_t *memory_get_ram_ptr(void); | ||
| static void to(uint64_t t){assert(t>=psx_get_cycle_count());psx_advance_cycles((uint32_t)(t-psx_get_cycle_count()));psx_devices_service_to_now();} |
There was a problem hiding this comment.
P3: to(t) truncates the cycle delta on the cast: psx_advance_cycles((uint32_t)(t-psx_get_cycle_count())). The assert only guards the lower bound (t>=count), not the upper one, so any stamp gap of more than 2^32 cycles (~126 s at 33.868 MHz) silently wraps and advances the wrong amount, corrupting the very timing this verification test is meant to check. Since this harness exists to catch timing deviations, widen the delta before advancing instead of truncating.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/tests/test_source_gpu_environment.c, line 13:
<comment>`to(t)` truncates the cycle delta on the cast: `psx_advance_cycles((uint32_t)(t-psx_get_cycle_count()))`. The assert only guards the lower bound (`t>=count`), not the upper one, so any stamp gap of more than 2^32 cycles (~126 s at 33.868 MHz) silently wraps and advances the wrong amount, corrupting the very timing this verification test is meant to check. Since this harness exists to catch timing deviations, widen the delta before advancing instead of truncating.</comment>
<file context>
@@ -0,0 +1,39 @@
+#include <stdio.h>
+extern void memory_init(const char *);
+extern uint8_t *memory_get_ram_ptr(void);
+static void to(uint64_t t){assert(t>=psx_get_cycle_count());psx_advance_cycles((uint32_t)(t-psx_get_cycle_count()));psx_devices_service_to_now();}
+int main(int argc,char **argv){
+ assert(argc==4);memory_init(argv[1]);timers_init();dma_init();interrupts_init();gr_init((uint16_t*)gpu_get_vram());gpu_init();source_gpu_runtime_init();
</file context>
Alexbeav
marked this pull request as ready for review
September 9, 2026 07:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make a clean clone of
Alexbeav/psxrecomp'stasreplaysbranch sufficient to build and verify the Tekken 3 TAS on Windows x64, using the player's own USA three-track disc and SCPH1001 BIOS. Merge after #28; both PRs targettasreplays.python tools/tasreplays/tekken3.py setup --disc <USA.cue> --bios <SCPH1001.BIN>checks exact asset hashes and disc topology, extracts the executable, downloads Spikestuff's unchanged movie 4164, builds and tests the tools, regenerates and verifies all 57 C files, and builds a native player with pinned SDL3/zlib sources.python tools/tasreplays/tekken3.py rundisplays the replay, exits at the observed victory, and verifies every original input plus 8,399 RAM-page and clock checkpoints.--headlesssupports automated verification.The recipe needs no private workspace paths, reference emulator or prebuilt title. It does not inject RAM or shift inputs. The committed reference contains fingerprints only. The external cold PRNG utility is a separate GPL-2.0-or-later program with source attribution and complete license; it is not linked into the native runtime. Movie, BIOS, disc, generated game C, binaries and replay outputs stay out of Git.
Validation at
5461aed53d4f07f672ffa276ef088880c34bbb2a:Both retail replays ran on Pegasus in independent checkouts. GitHub CI ran the asset-free checks on a separate Windows runner; it did not run copyrighted retail inputs.
Supported first-run environment: Windows x64, Python 3.11+, CMake 3.24+, Ninja and UCRT MinGW GCC (local validation: WinLibs 16.1.0). The run ends at 8,400 frames: 7,974 original inputs plus 426 declared neutral inputs. The later CDDA Play seek guard remains unresolved. Save states, hardware renderers, other titles, other disc layouts, and complete audio/pixel/hardware equivalence are outside this qualification. Historical oracle-dependent fixtures are not counted as portable CI passes. README documents prerequisites, exact inputs, commands and these limits.
Review scope: this branch remains
tasreplays; there is no planned merge intomain. The source-profile GPU DMA-request bits, zero-height C0 behavior, isolated-cache write routing and SPU decode IRQ comparison follow the pinned Octoshock 2.2.2 source. Hardware-oriented substitutions for those behaviors would change the compatibility target. Broader runtime/test-harness review findings remain follow-up work outside the qualified cold-boot recipe; passing CI is not a claim that every automated review comment is resolved.