diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e77d5e..a360281 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,12 @@ is easiest to understand by reading the commit. diagnostic counters were moved off the hot path behind the new default-on `rexdiag` feature, so a last-drops build can drop them with `--no-default-features` (`f1d0fcb`). +- **Crash fix: `rex-jit` CIDMATCH probe on aux-plane draws.** A draw into an + overlay plane (OLAY/PUP/CID) with CID checking live re-derived the CID probe's + offset against `fb_rgb` while the pixel pointer was already `fb_aux`-based, + reading `fb_aux + (fb_aux - fb_rgb) + off` — a wild pointer that segfaulted the + REX3 thread under X11. `Dm1::use_aux()` now picks the base in both shader + emitters (`rules/rex3/cidmatch-aux-plane-base.md`). - The GFIFO push is retryable, and a shader rejected by a full compile queue can be requested again (`rules/testing/rex-jit-queue-retry.md`). - `CIDMATCH` is a mask of permitted CIDs, not an equality value diff --git a/rules/rex3/cidmatch-aux-plane-base.md b/rules/rex3/cidmatch-aux-plane-base.md new file mode 100644 index 0000000..6c05e63 --- /dev/null +++ b/rules/rex3/cidmatch-aux-plane-base.md @@ -0,0 +1,96 @@ +# CIDMATCH probe used the wrong framebuffer base on aux-plane draws — 2026-09-18 + +**Status: FIXED.** `Dm1::use_aux()` now picks the base for the CID probe in both shader +emitters; regression test `jit_cidmatch_aux_plane_matches_interp`, verified to fail +without the fix. + +A SIGSEGV on the `REX3-Processor` thread after ~5 hours of IRIX 6.5 with X11 running. +Worth keeping for two reasons: the fix is one line per site, and the *diagnosis* was done +entirely from a macOS crash report with no symbols — the faulting frame is JIT code, so +there is nothing to symbolicate. The method below generalises to any rex-jit or jitv2 +crash report. + +## The bug + +`emit_calculate_fb_address` picks its base by plane: + +```rust +let fb_ptr = if use_aux { pctx.fb_aux } else { pctx.fb_rgb }; // OLAY/PUP/CID → fb_aux +let px_ptr = b.ins().iadd(fb_ptr, byte_off64); +``` + +The CIDMATCH probe then re-derived the byte offset — unconditionally against `fb_rgb`: + +```rust +let byte_off64 = b.ins().isub(px_ptr, pctx.fb_rgb); // wrong base when px_ptr is aux +b.ins().iadd(pctx.fb_aux, byte_off64) +``` + +For an RGB/RGBA draw the two cancel and the probe is right, which is why every existing +CIDMATCH test passed: `check_cid_write_masks` sweeps all 16 masks, 4 CIDs and 3 adrmodes, +but only ever with `DM1_RGB24_SRC`. For an OLAY/PUP/CID draw `px_ptr` is *already* +`fb_aux + off`, so the probe read + + fb_aux + (fb_aux - fb_rgb) + off + +`fb_rgb` and `fb_aux` are two independent `Box<[u32]>` allocations, so `fb_aux - fb_rgb` +is whatever the allocator chose — 0x29C0_0000 (700 MB) in the crashed process. The +consequence therefore ranges from reading a stray mapped word to SIGSEGV, with nothing in +the shader to distinguish the cases. + +X11 draws menus, popups and the cursor into the overlay planes with CID checking live, so +the shape is common; it needs the *pair* (aux plane, `cidmatch != 0xF`) to fire, which is +why it survived this long. Both emitters carried the probe — `emit_shader` and the line +emitter — so both had to be fixed. + +## Reading the crash report + +The report gives `pc` in no mapped image and a thread name, and that is the whole of it: + +``` +Thread 33 Crashed:: REX3-Processor +0 ??? 0x117b141fc ??? +1 iris-gui ..._4iris4rex3..Rex3..Device5start + 1980 +KERN_INVALID_ADDRESS at 0x0000007d3188c028 esr 0x92000006 (translation fault) +x1 0x7cde000000 x2 0x7d07c00000 x3 0x29c8c028 x24 0x800 x26 0x8c028 x27 0x46 +``` + +The lever is `instructionByteStream` in the JSON tail: `beforePC` and `atPC`, 40 bytes +each, base64. Decode them (`base64 -d`, then read 4-byte little-endian words) and +disassemble by hand or with `llvm-objdump -d --triple=arm64`: + +``` +-12 8b3a4043 add x3, x2, w26, uxtw ; px_ptr = fb_ptr + byte_off + -8 f84003e1 ldur x1, [sp] ; reload the spilled fb_rgb + -4 cb010063 sub x3, x3, x1 ; "byte offset" = px_ptr - fb_rgb + +0 b8636843 ldr w3, [x2, x3] ; ← fault: base is fb_ptr, not fb_rgb + +4 12000463 and w3, w3, #3 ; cid = aux_raw & 3 + +8 5280009b mov w27, #4 ; cidmatch nibble, folded in as a constant ++12 1ac32763 lsrv w3, w27, w3 ; cidmatch >> cid ++16 360000a3 tbz w3, #0, ... ; bit clear → skip the pixel +``` + +`and #3` / `lsrv` / `tbz` is the CIDMATCH block and nothing else in the shader, so the +site is identified without symbols. The folded `mov w27, #4` even gives the register +state: `CIDMATCH=0b0100`, i.e. only CID 2 permitted. + +Then the arithmetic confirms it rather than merely suggesting it: + +- `x2 - x1 = 0x29C0_0000` — two allocation bases, 700 MB apart. +- `x26 = 0x8C028` → `/4 = 143370` → `y = 143370 / 2048 = 70`, `x = 10`. A pixel comfortably + inside a 1280x1024 screen: **the coordinates were never out of bounds**, which rules out + a clipping or DDA bug and points at the addressing itself. +- `x2 + (x2 - x1 + x26) = 0x7D31_88C0_28` = `far`, exactly. The faulting address is + reproduced from the registers, so the mechanism is not a guess. + +Two things made this fast, and both are worth repeating: **the shader emitters are +straight-line IR, so a short instruction window is enough to name the block**, and +**reproducing `far` arithmetically from the register file turns a hypothesis into a +proof.** If the recomputation had not matched, the story would have been wrong. + +## Guard against the general shape + +Any pointer formed in the shader has to pick its base with the *same* predicate +`emit_calculate_fb_address` used. `Dm1::use_aux()` now exists so the choice is made in one +place; the inlined `matches!(dm1.planes(), OLAY | PUP | CID)` copies (there were three) +are gone. A second base derived from a pointer is the smell — prefer carrying the offset. diff --git a/src/rex3_jit/compiler.rs b/src/rex3_jit/compiler.rs index 03a3ef9..59f0eb2 100644 --- a/src/rex3_jit/compiler.rs +++ b/src/rex3_jit/compiler.rs @@ -104,6 +104,16 @@ impl Dm1 { fn blendalpha(&self) -> bool { self.val & (1 << 27) != 0 } fn logicop(&self) -> u32 { (self.val >> 28) & 0xF } + /// OLAY/PUP/CID are packed into the auxiliary framebuffer; RGB/RGBA live in + /// the main one. Every framebuffer pointer the shader forms — the pixel + /// address, the scr2scr source, the CIDMATCH probe — has to pick its base + /// with this, and mixing two bases is what made a CID-plane draw under a + /// live CIDMATCH read `fb_aux + (fb_aux - fb_rgb) + off`. + fn use_aux(&self) -> bool { + matches!(self.planes(), + p if p == DRAWMODE1_PLANES_OLAY || p == DRAWMODE1_PLANES_PUP || p == DRAWMODE1_PLANES_CID) + } + /// Compute compile-time host pixel count (pixels per host word) from dm1 fields. /// Mirrors Rex3::host_setup() count logic. fn host_count(&self) -> u32 { @@ -496,8 +506,7 @@ fn emit_calculate_fb_address( let c4 = b.ins().iconst(types::I32, 4); let byte_off = b.ins().imul(addr_i32, c4); let byte_off64 = b.ins().uextend(types::I64, byte_off); - let use_aux = matches!(dm1.planes(), - p if p == DRAWMODE1_PLANES_OLAY || p == DRAWMODE1_PLANES_PUP || p == DRAWMODE1_PLANES_CID); + let use_aux = dm1.use_aux(); let fb_ptr = if use_aux { pctx.fb_aux } else { pctx.fb_rgb }; let px_ptr = b.ins().iadd(fb_ptr, byte_off64); let _ptr_type = ptr_type; // consumed by caller if needed @@ -523,8 +532,7 @@ fn emit_pixel_write( dm1: &Dm1, is_hostw: bool, ) { - let use_aux = matches!(dm1.planes(), - p if p == DRAWMODE1_PLANES_OLAY || p == DRAWMODE1_PLANES_PUP || p == DRAWMODE1_PLANES_CID); + let use_aux = dm1.use_aux(); let depth_mask: i64 = match dm1.drawdepth() { 0 => 0xF, 1 => 0xFF, 2 => 0xFFF, _ => 0xFFFFFF }; let dblsrc_shift: i64 = match dm1.drawdepth() { 0 => 4, 1 => 8, 2 => 12, _ => 0 }; let (aux_read_shift0, aux_read_shift1, aux_read_mask): (i64, i64, i64) = match dm1.planes() { @@ -640,9 +648,9 @@ fn emit_pixel_write( // // Reuse the destination load above when there was one. `fb_px_raw` is the // same address, loaded with no intervening store, so a second load fetches - // a value we already have. (Note this is only the *plane* pixel: the CID - // match test reads fb_aux at a different pointer and is a genuinely - // separate access, not part of this pair.) Cranelift's redundant-load + // a value we already have. (Note this is only the *plane* pixel: for an + // RGB-plane draw the CID match test reads fb_aux at a different pointer and + // is a genuinely separate access, not part of this pair.) Cranelift's redundant-load // elimination will not merge them itself — the loads use `memv`, the // possibly-aliased flag, so it cannot prove nothing wrote in between. let old_val = if needs_dst { @@ -1188,12 +1196,16 @@ fn emit_shader( // Only emitted when cidmatch != 0xF (0xF = disabled). // Does not apply to HOSTR (READ) since that reads from fb, not writes to it. if cidmatch != 0xF && !is_hostr { - let aux_ptr = { - // Re-derive the fb_aux byte offset from px_ptr (which is already byte-offset into fb_rgb). + let aux_ptr = if dm1.use_aux() { + // px_ptr is already an fb_aux pointer for this pixel. + px_ptr + } else { + // Re-derive the fb_aux byte offset from px_ptr (an fb_rgb pointer here). // fb_rgb and fb_aux share the same stride/layout (2048 u32 entries per row), - // so the byte offset into fb_aux is the same as into fb_rgb. - let fb_rgb_base = pctx.fb_rgb; - let byte_off64 = b.ins().isub(px_ptr, fb_rgb_base); + // so the byte offset into fb_aux is the same as into fb_rgb. Subtracting + // the wrong base is a wild pointer, not a wrong pixel — see + // rules/rex3/cidmatch-aux-plane-base.md. + let byte_off64 = b.ins().isub(px_ptr, pctx.fb_rgb); b.ins().iadd(pctx.fb_aux, byte_off64) }; let aux_raw = b.ins().load(types::I32, memv, aux_ptr, ir::immediates::Offset32::new(0)); @@ -1230,8 +1242,7 @@ fn emit_shader( // to decide whether we consumed a host pixel (false) or drew colorback (true). let mut draw_use_bg_bool: Value = b.ins().iconst(types::I8, 0); // default: drew from host/DDA let src_color = if is_scr2scr { - let use_aux = matches!(dm1.planes(), - p if p == DRAWMODE1_PLANES_OLAY || p == DRAWMODE1_PLANES_PUP || p == DRAWMODE1_PLANES_CID); + let use_aux = dm1.use_aux(); let (aux_read_shift0, aux_read_shift1, aux_read_mask): (i64, i64, i64) = match dm1.planes() { p if p == DRAWMODE1_PLANES_OLAY => (8, 16, 0xFF), p if p == DRAWMODE1_PLANES_CID => (0, 4, 0x3), @@ -1996,9 +2007,10 @@ fn emit_draw_iline( // CID mask check for lines (same logic as emit_shader). if cidmatch != 0xF { - let aux_ptr = { - let fb_rgb_base = pctx.fb_rgb; - let byte_off64 = b.ins().isub(px_ptr, fb_rgb_base); + let aux_ptr = if dm1.use_aux() { + px_ptr // already an fb_aux pointer for this pixel + } else { + let byte_off64 = b.ins().isub(px_ptr, pctx.fb_rgb); b.ins().iadd(pctx.fb_aux, byte_off64) }; let aux_raw = b.ins().load(types::I32, memv, aux_ptr, ir::immediates::Offset32::new(0)); diff --git a/src/rex3_tests.rs b/src/rex3_tests.rs index 25f984b..b3be4d5 100644 --- a/src/rex3_tests.rs +++ b/src/rex3_tests.rs @@ -4807,6 +4807,85 @@ mod jit_tests { dm0, dm1, ); } + + /// CIDMATCH on a draw whose target plane lives in fb_aux (OLAY/PUP/CID). + /// + /// The CID probe re-derives its fb_aux offset from `px_ptr`, which is only + /// an fb_rgb pointer when the target plane is RGB/RGBA. For an aux-plane + /// draw `px_ptr` is already fb_aux-based, and subtracting fb_rgb from it + /// produced `fb_aux + (fb_aux - fb_rgb) + off` — a pointer hundreds of MB + /// outside either framebuffer. It segfaulted the REX3 thread the moment + /// IRIX drew into an overlay plane with CID checking live (X11 menus and + /// the cursor do exactly that). `fb_rgb` and `fb_aux` are two independent + /// `Box<[u32]>` allocations, so what the wild read hits is a property of + /// the heap layout, not of the shader: in the emulator (bases 700 MB + /// apart) SIGSEGV; in this test process (bases adjacent) a stray but + /// mapped word, which shows up as the CID test answering differently from + /// the interpreter. Either outcome fails the test. + /// + /// Both shader emitters carry the probe, so the line adrmode is covered + /// alongside the block one. cid_write_masks_jit already covers the + /// RGB-plane half of the same test. + #[test] + fn jit_cidmatch_aux_plane_matches_interp() { + fn aux_pixel(rex: &Rex3, x: i32, y: i32) -> u32 { + unsafe { (*rex.fb_aux.get())[(y as u32 * 2048 + x as u32) as usize] } + } + + let rex_interp = make_rex3(); + let rex_jit = make_rex3_jit(); + let dst = 20 * 2048 + 10; + let mut drew_something = false; + + for planes in [DRAWMODE1_PLANES_OLAY, DRAWMODE1_PLANES_PUP, DRAWMODE1_PLANES_CID] { + let dm1 = planes | DRAWMODE1_COMPARE_DISABLE_SH | DRAWMODE1_LOGICOP_SRC_SH; + for dm0 in [DM0_DRAW_BLOCK, + DRAWMODE0_OPCODE_DRAW | DRAWMODE0_ADRMODE_I_LINE_SH | DM0_DOSETUP | DM0_STOPONXY] { + for mask in [0b0001u32, 0b0100, 0b1010] { + let cm = mask << CLIPMODE_CIDMATCH_SHIFT; + let jit = rex_jit.rex_jit.as_ref().unwrap(); + jit.request_compile(dm0, dm1, cm); + assert!(jit.wait_compiled(dm0, dm1, cm), + "JIT compile failed for dm0={dm0:#010x} dm1={dm1:#010x} cm={cm:#010x}"); + + for cid in 0..4_u32 { + let seed = 0x80000000 | (cid << 4) | cid; + let run = |rex: &Rex3| -> u32 { + rex3init(rex); + wait(rex); + unsafe { (*rex.fb_aux.get())[dst] = seed; } + reg(rex, REX3_DRAWMODE1, dm1); + reg(rex, REX3_COLORI, 0xFF); + reg(rex, REX3_CLIPMODE, cm); + reg(rex, REX3_XYSTARTI, xy(10, 20)); + reg(rex, REX3_XYENDI, xy(10, 20)); + reg_go(rex, REX3_DRAWMODE0, dm0); + aux_pixel(rex, 10, 20) + }; + + let before = rex_jit.jit_go_count.load(std::sync::atomic::Ordering::Relaxed); + let got_jit = run(rex_jit); + assert_eq!(rex_jit.jit_go_count.load(std::sync::atomic::Ordering::Relaxed) - before, 1, + "GO did not dispatch to the compiled shader: \ + dm0={dm0:#010x} dm1={dm1:#010x} cm={cm:#010x}"); + + let got_interp = run(rex_interp); + assert_eq!(got_jit, got_interp, + "JIT/interp mismatch: planes={planes} dm0={dm0:#010x} \ + CIDMATCH={mask:04b} cid={cid}"); + drew_something |= got_interp != seed; + } + } + } + } + + // A permitted CID has to actually write, or every comparison above is + // a comparison of two untouched seed values. + assert!(drew_something, "no aux-plane write landed — the comparison was vacuous"); + + rex_jit.stop(); + rex_interp.stop(); + } } // ---------------------------------------------------------------------------