acid: address Copilot review on PR #130

Seven inline comments on PR #130, all addressed:

1. **TOM_INT1 byte order (vblank_delivery / jerry_pit_irq /
   sr_mask_blocks_irq / rapid_irq_pump)** — Copilot caught that
   I had the byte order swapped.  Per src/tom/tom.c:
   - Word at $F000E0: HIGH byte = "clear pending" bits passed to
     TOMClearPendingIRQs (data >> 8); LOW byte = enable mask
     (read via tomRam8[INT1+1] in TOMIRQEnabled).
   - I was writing `$0100` to enable VIDEO when I needed `$0001`.
   Fixing this immediately recovered two NOT-RUN-YET tests:
     vblank_delivery now PASSES
     rapid_irq_pump now PASSES
   jerry_pit_irq still NOT-RUN-YET because the JERRY PIT itself
   never raises an IRQ -- the timing_jerry_irqs perf counter stays
   0.  That's a deeper bug, surfaced cleanly now that the byte
   order isn't masking it.

2. **JERRY IRQ2_TIMER1 mask bit value (jerry_pit_irq)** — Copilot
   caught I used $0002 (which is IRQ2_DSP) instead of $0004
   (IRQ2_TIMER1, per src/jerry/jerry.h:36-38).  Fixed.

3. **bsr_long_61ff.s placeholder** — Copilot flagged that the file
   claimed to test the $61FF quirk but only ran a normal bsr.w.
   Repurposed as a BSR.W *control* test (so the real $61FF test
   in bsr_l_61ff_real.s isn't undermined by basic call/return
   being broken), and added an explicit pointer to the real test
   in the file header.

4. **run.c top comment offset** — said `0x100`, code reads
   `0x100000`.  Fixed comment.

5. **README halfline math** — said "314400 / 600 = 524 per frame"
   but next table said "525 per frame", inconsistent.  Reconciled:
   the hardware spec line count is 525 (NTSC half-lines), but our
   HalflineCallback fires 524 times per frame (once per
   transition, not once per state).  Both numbers are correct;
   docs now spell out which is which.

6. **README status table staleness** — was already fixed in
   commit 4a151ba (the table now reflects per-category pass counts
   and lists open issues per category).

7. (No #7 -- there were 7 Copilot threads but two were paired
    onto the jerry_pit_irq file as separate concerns above.)

Final status: 54 / 72 PASSing (was 52).  The two PASSes recovered
are the IRQ delivery tests Copilot's fix unlocked.

Co-Authored-By: Claude Opus 4.7 <[email protected]>

Author: JoeMatt

test/acid/README.md

@@ -33,15 +33,15 @@ codes; per-test perf-counter delta dumps when built with
 optional -- if absent, the assemble step is skipped with a warning
 and only the runner harness is built.
 
-**52 / 72 tests PASSing across 13 categories.**  Failures and
+**54 / 72 tests PASSing across 13 categories.**  Failures and
 NOT-RUN-YETs are intentional documentation of known emulator gaps.
 
 | Category | Tests | Pass | Open issues surfaced |
 |---|---:|---:|---|
 | smoke      |  1 |  1 | — |
 | memory     |  8 |  8 | — |
 | timing     |  9 |  8 | jerry_pit_setup: PIT readback returns 0 |
-| irq        |  9 |  6 | vblank_delivery + jerry_pit_irq + rapid_irq_pump NOT-RUN-YET (IRQ raises in TOM/JERRY per perf counters but never reaches 68K vec-64 handler) |
+| irq        |  9 |  8 | jerry_pit_irq NOT-RUN-YET (PIT itself never raises an IRQ -- timing_jerry_irqs counter stays 0) |
 | blitter    | 17 |  4 | 13 SRC-reading tests fail identically; lfu_zero_fill / lfu_one_fill / lfu_invert_src PASS — narrows bug to LFU source-routing |
 | gpu        |  2 |  2 | — (gpu_basic_run + gpu_reg_access) |
 | dsp        |  3 |  3 | dsp_mac_accumulator is currently a NOP-loop placeholder; real 40-bit-MAC math is a follow-up |
@@ -131,19 +131,21 @@ after each test and dumps the delta:
 
 That tells us at a glance:
 - the test ran for 600 retro_run cycles (10 emulated seconds at 60 Hz)
-- the halfline callback fired 314400 times = exactly 524 per frame
-  (NTSC), which is what the hardware spec calls for
+- the halfline callback fired 314400 times = exactly **524 per
+  frame** (NTSC: VC sweeps 0..524 inclusive, but our HalflineCallback
+  is invoked once per *transition*, hence 524 not 525)
 
-If a future change makes the halfline rate jump to 1048800 (1048
-per frame), this number will catch it immediately even if no test
+If a future change makes the halfline rate jump to e.g. 1048800
+(1748 per frame, what the bug would look like if events fired on
+both edges), this number will catch it immediately even if no test
 explicitly checks for it.
 
 Counters surfaced in the per-test summary today:
 
 | Counter | Source | Expected (NTSC default) |
 |---|---|---|
 | `timing_jaguar_execute_calls` | `JaguarExecuteNew` entry | 1 per `retro_run()` |
-| `timing_halfline_callbacks` | `HalflineCallback` entry | 525 per frame |
+| `timing_halfline_callbacks` | `HalflineCallback` entry | 524 per frame (NTSC) |
 | `timing_vblank_irqs` | TOM video-int raise | 1 per frame |
 | `timing_jerry_irqs` | JERRY PIT IRQ raise | 0 unless game enables PIT |
 | `timing_gpu_irqs_to_68k` | TOM PIT-→68K raise | 0 unless game enables TOM PIT |

test/acid/run.c

@@ -3,7 +3,7 @@
  *
  * Loads a libretro core via dlopen, loads a synthetic .jag test ROM,
  * runs it for a fixed number of frames, then reads the four-word
- * "acid signature" out of main RAM at offset 0x100 and prints
+ * "acid signature" out of main RAM at offset 0x100000 and prints
  * PASS / FAIL / NOT-RUN-YET.
  *
  * Usage: run <core.dylib> <test.jag> [num_frames]

test/acid/tests/irq/jerry_pit_irq.s

@@ -23,9 +23,14 @@ JPIT1           equ     $F10036                 ; timer 1 prescaler
 JPIT2           equ     $F10038                 ; timer 1 divider
 JINTCTRL        equ     $F10020                 ; interrupt control
 
-;; Bits
-JINT_TIMER1     equ     $0002
-TOM_INT_DSP_EN  equ     $0400                   ; bit 10 enables DSP/JERRY IRQ
+;; Bits.
+;; - JERRY interrupt mask bits (per src/jerry/jerry.h):
+;;     IRQ2_DSP=$02, IRQ2_TIMER1=$04, IRQ2_TIMER2=$08, ...
+;; - TOM_INT1 enable mask is the LOW byte of the word at $F000E0
+;;   (per src/tom/tom.c TOMIRQEnabled reading tomRam8[INT1+1]).
+;;   IRQ_DSP=4, so enable bit is $10.
+JINT_TIMER1     equ     $0004
+TOM_INT_DSP_EN  equ     $0010
 
 IRQ_FIRED       equ     $00000800
 HW_IRQ_VECTOR   equ     $00000100

test/acid/tests/irq/sr_mask_blocks_irq.s

@@ -30,9 +30,11 @@ entry:
                 move.l  a0,HW_IRQ_VECTOR.l
 
                 ;; Configure TOM to fire VBlank.
+                ;; TOM_INT1 byte layout: HIGH byte = clear pending,
+                ;; LOW byte = enable mask.  IRQ_VIDEO=0 -> $01.
                 move.w  #$1F00,TOM_INT1         ; clear pending
                 move.w  #2,TOM_VI               ; fire on halfline 2
-                move.w  #$0100,TOM_INT1         ; enable VIDEO
+                move.w  #$0001,TOM_INT1         ; enable VIDEO
 
                 ;; Keep 68K SR with IPL=7 (block everything).
                 move.w  #$2700,sr

test/acid/tests/irq/vblank_delivery.s

@@ -54,9 +54,14 @@ entry:
                 move.w  #2,TOM_VI               ; VC == 2 (halflines)
 
                 ;; Enable just the video interrupt.
-                ;; INT1 word: bit 8..12 = enable mask, bit 0..4 = clear.
-                ;; bit 0 = VIDEO -> mask bit at +8 = 0x0100.
-                move.w  #$0100,TOM_INT1
+                ;; TOM_INT1 byte layout (per src/tom/tom.c:85, 1142-1146,
+                ;; 1190-1194, 1244-1248): the LOW byte holds the enable
+                ;; mask (read by TOMIRQEnabled via tomRam8[INT1+1]); the
+                ;; HIGH byte is "clear pending" bits passed to
+                ;; TOMClearPendingIRQs.  Big-endian word: high byte is
+                ;; at offset $E0, low byte at $E1.
+                ;; IRQ_VIDEO=0 -> enable bit $01.
+                move.w  #$0001,TOM_INT1
 
                 ;; Drop 68K interrupt mask to allow IPL=2.
                 ;; SR bits 8..10 are I[2..0]; we want them all clear.

test/acid/tests/quirks/bsr_long_61ff.s

@@ -1,13 +1,20 @@
 ;
-; tests/quirks/bsr_long_61ff.s - 68K BSR.L $61FF Atari aln linker quirk.
+; tests/quirks/bsr_long_61ff.s - BSR.W control / sanity test.
 ;
-; The Atari `aln` linker emits BSR.L (opcode $61FF) with the
-; displacement filled in as an *absolute address* instead of
-; PC-relative.  Our 68K core was patched to handle this in commit
-; 4fcf958 (#119).  Verify by emitting one and checking it returned.
+; Originally drafted as a placeholder for the BSR.L $61FF quirk before
+; the real test (`bsr_l_61ff_real.s`, in this same directory) existed.
+;
+; Now repurposed as a BSR.W *sanity* gate -- if even a normal short-
+; branch BSR doesn't round-trip, the bsr_l_61ff_real test is
+; meaningless because we couldn't tell the failure was about the quirk
+; vs about call/return at all.
+;
+; The actual $61FF Atari aln quirk coverage lives in
+; `tests/quirks/bsr_l_61ff_real.s`, which emits the raw opcode
+; bytes and the absolute target.
 ;
 ; Detail codes:
-;   1 = BSR didn't return / target didn't run
+;   1 = BSR.W didn't return / target didn't run
 ;
                 include "include/jaguar_header.s"
                 include "include/acid_test.s"
@@ -16,15 +23,8 @@
 entry:
                 ACID_INIT
 
-                ;; Test approach: regular BSR works (control case);
-                ;; if even regular BSR fails, the test setup is wrong.
-                ;; The aln-quirk handling is hard to assemble portably
-                ;; via vasm (it's specifically the buggy emit pattern),
-                ;; so this test is currently a placeholder asserting
-                ;; only that BSR.L itself does what it should.
-
-                moveq   #0,d6                   ; flag = 0
-                bsr.w   .target                 ; BSR.W (sane)
+                moveq   #0,d6                   ; flag = "didn't return"
+                bsr.w   .target                 ; standard BSR.W
                 tst.l   d6
                 beq.s   .no_return
 

test/acid/tests/quirks/divl_zero_traps.s

@@ -7,7 +7,8 @@
 ; ($00000014), just like the native 68000 DIV.W behaviour.
 ;
 ; Approach: install a tiny trap handler at vector 5 that sets d6=1,
-; then execute `divs.l #0,d2` (inline-encoded as $4C3C,$0800).
+; then execute `divs.l d4,d3` with d4=0.  Encoded as $4C04,$3800
+; (matches the same form as the muls.l test in illegal_opcode_traps.s).
 ; If the trap fires, d6 becomes 1 and the test passes.
 ;
 ; Detail codes:
@@ -27,14 +28,17 @@ entry:
                 move.l  a0,V_ZERODIV.l
 
                 moveq   #0,d6                   ; flag = 0
-                move.l  #12345,d2
-
-                ;; divs.l #0,d2  => $4C3C,$2800,$00000000
-                ;; ($4C3C = DIV[?].L #imm; ext word fields:
-                ;;   bit11 sg=1 (signed), bit10 sz=0 (32-bit),
-                ;;   bits14-12 Dl=2, bits2-0 Dh=0 -> $2800)
-                dc.w    $4C3C,$2800
-                dc.l    $00000000
+                move.l  #12345,d3              ; dividend
+                moveq   #0,d4                   ; divisor = 0
+
+                ;; divs.l d4,d3  =>  $4C04,$3800
+                ;; opcode $4C04: base $4C00, mode 0 (Dn), reg 4 (d4 src)
+                ;; ext   $3800:
+                ;;   bits14-12 Dl=3 (quotient/dividend in d3)
+                ;;   bit  11   sg=1 (signed)
+                ;;   bit  10   sz=0 (32-bit, no Dh)
+                ;;   bits  2-0 Dh=0 (don't-care)
+                dc.w    $4C04,$3800
 
                 tst.l   d6
                 beq.s   .bad

test/acid/tests/stress/rapid_irq_pump.s

@@ -35,10 +35,12 @@ entry:
                 move.l  a0,HW_IRQ_VECTOR.l
 
                 ;; Idle TOM, then arm video IRQ at scanline 2.
+                ;; TOM_INT1: HIGH byte = clear pending, LOW byte = enable
+                ;; (per src/tom/tom.c).  IRQ_VIDEO=0 -> $01.
                 move.w  #$1F00,TOM_INT1         ; clear all pending
                 move.w  #0,TOM_INT1             ; idle
                 move.w  #2,TOM_VI
-                move.w  #$0100,TOM_INT1         ; enable video IRQ
+                move.w  #$0001,TOM_INT1         ; enable video IRQ
 
                 ;; Drop interrupt mask: supervisor, IPL=0.
                 move.w  #$2000,sr
@@ -62,6 +64,7 @@ entry:
 ;
 irq_handler:
                 addq.l  #1,IRQ_COUNTER.l
-                ;; Clear pending VIDEO bit (bit 0) and re-enable.
+                ;; Clear pending VIDEO bit (HIGH byte) and re-enable
+                ;; mask (LOW byte): $0101.
                 move.w  #$0101,TOM_INT1
                 rte