diff --git a/examples/uxrom_banked.nes b/examples/uxrom_banked.nes index dbff6c6..7c9e513 100644 Binary files a/examples/uxrom_banked.nes and b/examples/uxrom_banked.nes differ diff --git a/examples/uxrom_user_banked.nes b/examples/uxrom_user_banked.nes index c4cf2da..94db0fa 100644 Binary files a/examples/uxrom_user_banked.nes and b/examples/uxrom_user_banked.nes differ diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index 4f3954c..27640fa 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -915,6 +915,34 @@ impl Analyzer { .map(|(n, l)| (n.clone(), l.size)) .collect(); let size = type_size_with(&var.var_type, &struct_sizes); + + // Warn on arrays whose byte size exceeds 256: the codegen + // lowers `arr[i]` to `LDA base,X` (or `ZeroPageX`), and the + // 6502's X register is 8 bits, so elements whose byte + // offset is >= 256 are unreachable. For a `u8` array the + // safe max count is 256; for a `u16` array it's 128 + // (since the codegen doesn't scale the index by element + // width — see the note in `emit_bounds_check`). This + // diagnostic replaces the previous silent-skip in the + // debug-mode bounds checker. + if let NesType::Array(_, _) = &var.var_type { + if size > 256 { + self.diagnostics.push( + Diagnostic::warning( + ErrorCode::W0108, + format!( + "array '{}' has byte size {size}, but the 6502's 8-bit X index can only reach the first 256 bytes — elements past that are unreachable", + var.name + ), + var.span, + ) + .with_help( + "shrink the array, split it across multiple smaller arrays, or use separate fields for each element".to_string(), + ), + ); + } + } + let Some(address) = self.allocate_ram(size, var.span) else { // Allocation failed (E0301 already emitted) — still add the // symbol so that later references don't cascade into E0502, diff --git a/src/analyzer/tests.rs b/src/analyzer/tests.rs index 52329c9..be41870 100644 --- a/src/analyzer/tests.rs +++ b/src/analyzer/tests.rs @@ -1661,3 +1661,77 @@ fn analyze_fast_var_underscore_exempt() { result.diagnostics ); } + +#[test] +fn analyze_oversized_array_warns_w0108() { + // A u8 array with 300 elements has byte size 300 > 256. The + // codegen lowers `arr[i]` to `LDA base,X` with X 8-bit, so + // elements 256..299 are unreachable. W0108 should fire. + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + var big: u8[300] + on frame { + big[0] = 0 + wait_frame + } + start Main + "#, + ); + assert!( + result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0108), + "oversized u8 array should emit W0108, got: {:?}", + result.diagnostics + ); +} + +#[test] +fn analyze_boundary_size_256_array_ok() { + // A u8[256] exactly fills the 8-bit X register — every element + // is reachable. No W0108. + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + var big: u8[256] + on frame { + big[0] = 0 + wait_frame + } + start Main + "#, + ); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0108), + "u8[256] should not emit W0108, got: {:?}", + result.diagnostics + ); +} + +#[test] +fn analyze_small_array_never_warns_w0108() { + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + var small: u8[16] + on frame { + small[0] = 0 + wait_frame + } + start Main + "#, + ); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0108), + "small array should not emit W0108, got: {:?}", + result.diagnostics + ); +} diff --git a/src/asm/mod.rs b/src/asm/mod.rs index 66c66c2..3a799a6 100644 --- a/src/asm/mod.rs +++ b/src/asm/mod.rs @@ -182,6 +182,24 @@ impl Assembler { self.output.push(0); // placeholder } } + AddressingMode::LabelAbsoluteX(name) => { + // `STA label,X` style indexed store with a label- + // resolved base address. Encodes like `absolute,X` + // but the 16-bit address is patched in by the + // fixup pass, same as plain `Label` fixups. + if let Some(op) = opcodes::encode(inst.opcode, &AddressingMode::AbsoluteX(0)) { + self.output.push(op); + self.fixups.push(Fixup { + offset: self.output.len(), + label: name.clone(), + kind: FixupKind::Absolute, + }); + self.output.push(0); // placeholder low byte + self.output.push(0); // placeholder high byte + } else { + panic!("opcode {:?} cannot target an absolute,X label", inst.opcode); + } + } AddressingMode::SymbolLo(name) => { if let Some(op) = opcodes::encode(inst.opcode, &AddressingMode::Immediate(0)) { self.output.push(op); diff --git a/src/asm/opcodes.rs b/src/asm/opcodes.rs index d06ac20..c361ef3 100644 --- a/src/asm/opcodes.rs +++ b/src/asm/opcodes.rs @@ -79,6 +79,13 @@ pub enum AddressingMode { // Pre-resolution symbolic forms Label(String), LabelRelative(String), + /// Absolute-X indexed form targeting a named label — resolves + /// to the 16-bit address of `label` at fix-up time, with the + /// instruction encoded as `absolute,X`. Used by `UxROM`'s + /// `__bank_select` to write into the bus-conflict table + /// (`STA __bank_select_table,X`) where the target address has + /// to be resolved by the linker. + LabelAbsoluteX(String), SymbolLo(String), SymbolHi(String), @@ -103,7 +110,11 @@ impl AddressingMode { | Self::IndirectY(_) | Self::Relative(_) => 1, Self::Absolute(_) | Self::AbsoluteX(_) | Self::AbsoluteY(_) | Self::Indirect(_) => 2, - Self::Label(_) | Self::LabelRelative(_) | Self::SymbolLo(_) | Self::SymbolHi(_) => 0, + Self::Label(_) + | Self::LabelRelative(_) + | Self::LabelAbsoluteX(_) + | Self::SymbolLo(_) + | Self::SymbolHi(_) => 0, // `Bytes` is the full emitted payload — the assembler // skips the usual opcode byte for `NOP+Bytes` and writes // the raw vector, so the whole thing is operand. @@ -125,7 +136,11 @@ impl AddressingMode { Self::Absolute(v) | Self::AbsoluteX(v) | Self::AbsoluteY(v) | Self::Indirect(v) => { v.to_le_bytes().to_vec() } - Self::Label(_) | Self::LabelRelative(_) | Self::SymbolLo(_) | Self::SymbolHi(_) => { + Self::Label(_) + | Self::LabelRelative(_) + | Self::LabelAbsoluteX(_) + | Self::SymbolLo(_) + | Self::SymbolHi(_) => { vec![] } Self::Bytes(v) => v.clone(), diff --git a/src/codegen/ir_codegen.rs b/src/codegen/ir_codegen.rs index aa176fe..79d1501 100644 --- a/src/codegen/ir_codegen.rs +++ b/src/codegen/ir_codegen.rs @@ -452,13 +452,17 @@ impl<'a> IrCodeGen<'a> { if size == 0 { return; } - // Anything >= 256 would overflow the u8 immediate; skip - // the check rather than emit a bogus compare. A proper - // 16-bit bounds check would need a two-byte compare - // against the high byte too. - let Ok(size_u8) = u8::try_from(size) else { + // Sizes > 256 skip the bounds check because the X register + // is 8 bits — any index the codegen can actually produce is + // already in-bounds for a byte count that large. The + // analyzer's W0108 warning fires at declaration time so the + // user knows those elements are unreachable. Size == 256 + // fits in a `CMP #$00` (trivially true), so we clamp to + // 255 — the tightest useful check — and treat 256+ the same. + let size_u8 = u8::try_from(size.min(255)).unwrap_or(255); + if size_u8 == 0 { return; - }; + } // Use a short BCC over an unconditional JMP instead of a // plain `BCS __debug_halt`. A single BCS can only span 127 // bytes, and `__debug_halt` is emitted at the very end of diff --git a/src/errors/diagnostic.rs b/src/errors/diagnostic.rs index 3dc73a8..8a840a5 100644 --- a/src/errors/diagnostic.rs +++ b/src/errors/diagnostic.rs @@ -42,6 +42,7 @@ pub enum ErrorCode { W0105, // palette sub-palette universal mismatch (mirror collision) W0106, // implicit drop of non-void function return value W0107, // `fast` variable rarely accessed (wastes zero-page slot) + W0108, // array elements past byte 255 unreachable via 8-bit X index } impl fmt::Display for ErrorCode { @@ -68,6 +69,7 @@ impl fmt::Display for ErrorCode { Self::W0105 => "W0105", Self::W0106 => "W0106", Self::W0107 => "W0107", + Self::W0108 => "W0108", }; write!(f, "{code}") } @@ -82,7 +84,8 @@ impl ErrorCode { | Self::W0104 | Self::W0105 | Self::W0106 - | Self::W0107 => Level::Warning, + | Self::W0107 + | Self::W0108 => Level::Warning, _ => Level::Error, } } diff --git a/src/runtime/mod.rs b/src/runtime/mod.rs index e753361..a40b380 100644 --- a/src/runtime/mod.rs +++ b/src/runtime/mod.rs @@ -1455,11 +1455,28 @@ pub fn gen_bank_select(mapper: Mapper) -> Vec { out.push(Instruction::implied(RTS)); } Mapper::UxROM => { - // UxROM: write bank number to any address in $8000-$FFFF. - // We use $FFF0 so the write lands in the fixed bank's - // tail area where the linker can back it with a matching - // bank-table byte to avoid bus conflicts. - out.push(Instruction::new(STA, AM::Absolute(0xFFF0))); + // UxROM: write the bank number to any address in + // $8000-$FFFF. On boards with bus conflicts the CPU's + // write and the ROM byte at that address are ANDed on + // the data bus, so we must write to an address whose + // ROM byte already equals the bank number. The linker + // splices a 256-byte table (`__bank_select_table`, + // bytes 0..255) into the fixed bank, and we index into + // it with X = bank number: `STA __bank_select_table, X` + // stores A (= bank number) at + // `__bank_select_table + X`, whose ROM byte is exactly + // X, so bus = A = X = ROM — no conflict. + // + // Previously this wrote to a fixed `$FFF0`, which + // happens to work on emulators that don't simulate bus + // conflicts (jsnes, Mesen permissive) but would glitch + // on real hardware because a single ROM byte can't + // match every possible bank number. + out.push(Instruction::implied(TAX)); + out.push(Instruction::new( + STA, + AM::LabelAbsoluteX("__bank_select_table".into()), + )); out.push(Instruction::implied(RTS)); } Mapper::MMC3 => { diff --git a/src/runtime/tests.rs b/src/runtime/tests.rs index 1ab227c..4301955 100644 --- a/src/runtime/tests.rs +++ b/src/runtime/tests.rs @@ -674,14 +674,32 @@ fn bank_select_mmc1_serializes_five_bits_to_e000() { } #[test] -fn bank_select_uxrom_writes_fff0() { - // UxROM bank-select writes A to $FFF0, which lives in the - // fixed bank's bus-conflict-safe table. +fn bank_select_uxrom_uses_bus_conflict_table() { + // UxROM bank-select has to write to a ROM address whose byte + // already equals the bank being selected. The routine does + // `TAX; STA __bank_select_table, X` so the store goes to + // `table + bank_num`, whose ROM byte is `bank_num` — making + // the bus write match the ROM byte regardless of which bank + // is being requested. We assert both pieces here so a + // regression back to the broken `STA $FFF0` form fails + // loudly. let sel = gen_bank_select(Mapper::UxROM); - let has_write = sel + let has_tax = sel.iter().any(|i| i.opcode == TAX && i.mode == AM::Implied); + assert!(has_tax, "UxROM bank-select must TAX before the store"); + let has_indexed_store = sel.iter().any(|i| { + i.opcode == STA && matches!(&i.mode, AM::LabelAbsoluteX(n) if n == "__bank_select_table") + }); + assert!( + has_indexed_store, + "UxROM bank-select must `STA __bank_select_table,X`" + ); + let writes_fff0 = sel .iter() .any(|i| i.opcode == STA && i.mode == AM::Absolute(0xFFF0)); - assert!(has_write, "UxROM bank-select must write to $FFF0"); + assert!( + !writes_fff0, + "UxROM bank-select must not fall back to the bus-conflict-unsafe STA $FFF0 form" + ); assert_eq!(sel.last().unwrap().opcode, RTS); } @@ -723,7 +741,15 @@ fn bank_select_stashes_bank_number_in_zp() { #[test] fn bank_select_assembles_for_every_mapper() { for m in [Mapper::NROM, Mapper::MMC1, Mapper::UxROM, Mapper::MMC3] { - let sel = gen_bank_select(m); + let mut sel = gen_bank_select(m); + // UxROM `gen_bank_select` references `__bank_select_table` + // via `AM::LabelAbsoluteX`; give the assembler something + // to resolve against in this standalone unit test. Real + // linking appends the table in the linker pass, so the + // label always resolves there. + if m == Mapper::UxROM { + sel.extend(super::gen_uxrom_bank_table()); + } let result = asm::assemble(&sel, 0xC000); assert!( !result.bytes.is_empty(), diff --git a/tests/integration_test.rs b/tests/integration_test.rs index f985be5..68549ab 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -2260,6 +2260,155 @@ start Main ); } +#[test] +fn source_map_survives_aggressive_peephole_folding() { + // Regression guard for the concern raised in code review: + // `__src_` markers are emitted as label pseudo-ops, and + // peephole uses labels as block boundaries. If peephole ever + // started pruning unreferenced labels the source map would + // silently lose entries. Compile a program that trips the + // peephole store-then-load and redundant-load folds on every + // single line, then assert every `__src_` label the codegen + // recorded is still in the linker's label table post-peephole. + let source = r#" + game "PeepholeFolds" { mapper: NROM } + var t0: u8 = 0 + var t1: u8 = 0 + var t2: u8 = 0 + var t3: u8 = 0 + var t4: u8 = 0 + on frame { + t0 = 1 + t1 = t0 + t2 = t1 + t3 = t2 + t4 = t3 + t0 = t4 + wait_frame + } + start Main + "#; + let (program, _) = nescript::parser::parse(source); + let program = program.unwrap(); + let analysis = analyzer::analyze(&program); + let mut ir_program = ir::lower(&program, &analysis); + optimizer::optimize(&mut ir_program); + let sprites = assets::resolve_sprites(&program, Path::new(".")).unwrap(); + let sfx = assets::resolve_sfx(&program).unwrap(); + let music = assets::resolve_music(&program).unwrap(); + let palettes = assets::resolve_palettes(&program, Path::new(".")).unwrap(); + let backgrounds = assets::resolve_backgrounds(&program, Path::new(".")).unwrap(); + + let mut codegen = IrCodeGen::new(&analysis.var_allocations, &ir_program) + .with_sprites(&sprites) + .with_audio(&sfx, &music) + .with_source_map(true); + let mut instructions = codegen.generate(&ir_program); + + // Snapshot the __src_ labels the codegen recorded BEFORE + // peephole runs. + let pre_peephole: std::collections::HashSet = codegen + .source_locs() + .iter() + .map(|(name, _)| name.clone()) + .collect(); + assert!( + pre_peephole.len() >= 6, + "codegen should have recorded at least one source loc per statement, got {} from {pre_peephole:?}", + pre_peephole.len() + ); + + // Run peephole. This is the pass that the reviewer worried + // might drop labels. + nescript::codegen::peephole::optimize(&mut instructions); + + // Link and inspect the resolved label table. + let linker = Linker::with_mapper(program.game.mirroring, program.game.mapper); + let switchable_banks: Vec = program + .banks + .iter() + .filter(|b| b.bank_type == BankType::Prg) + .map(|b| PrgBank::empty(&b.name)) + .collect(); + let link_result = linker.link_banked_with_ppu_detailed( + &instructions, + &sprites, + &sfx, + &music, + &palettes, + &backgrounds, + &switchable_banks, + ); + + // Every pre-peephole __src_ label must survive into the final + // linker label table. If peephole ever deletes a label this + // loop fails with the exact label that vanished. + for name in &pre_peephole { + assert!( + link_result.labels.contains_key(name), + "peephole dropped source marker {name}; this breaks source maps" + ); + } +} + +#[test] +fn debug_frame_overrun_counter_reads_back_from_user_code() { + // End-to-end contract test for the frame-overrun counter: + // when compiled with `--debug`, the NMI handler increments + // `$07FF` whenever the main loop didn't reach `wait_frame` + // in time, and user code is expected to read that counter + // with `peek(0x07FF)`. This test verifies three things that + // together make the feature usable: + // + // 1. The NMI handler's INC $07FF is still present. + // 2. A user `peek(0x07FF)` lowers to a matching LDA $07FF. + // 3. The analyzer's RAM allocator doesn't hand out $07FF + // to a user variable, so the peek reads the counter + // and not some unrelated byte. + let source = r#" + game "Overrun" { mapper: NROM } + var last_overruns: u8 = 0 + on frame { + last_overruns = peek(0x07FF) + wait_frame + } + start Main + "#; + let (rom, _mlb, _map) = compile_with_debug_artifacts(source, true); + let prg = &rom[16..16 + 16384]; + + // (1) NMI bumps the counter — look for `INC $07FF` + // (opcode EE, lo FF, hi 07). + let inc_07ff: [u8; 3] = [0xEE, 0xFF, 0x07]; + assert!( + prg.windows(inc_07ff.len()).any(|w| w == inc_07ff), + "debug NMI handler should INC $07FF" + ); + + // (2) User peek lowers to an `LDA $07FF` somewhere in the + // frame handler (opcode AD, lo FF, hi 07). + let lda_07ff: [u8; 3] = [0xAD, 0xFF, 0x07]; + assert!( + prg.windows(lda_07ff.len()).any(|w| w == lda_07ff), + "user `peek(0x07FF)` should lower to LDA $07FF" + ); + + // (3) No user variable should be allocated at $07FF — verify + // by re-parsing + re-analyzing and walking the allocations. + let (program, _) = nescript::parser::parse(source); + let program = program.unwrap(); + let analysis = analyzer::analyze(&program); + assert!( + analysis.var_allocations.iter().all(|a| { + // Last allocated byte is address + size - 1. + let last = a.address + a.size - 1; + last < 0x07FF + }), + "user variable must not land on the debug overrun counter at $07FF: {:?}", + analysis.var_allocations + ); +} + #[test] fn debug_build_emits_bounds_check_halt_routine() { // When compiled with `--debug`, a program that indexes an