1
0
Fork 0
mirror of https://github.com/imjasonh/nescript synced 2026-07-08 08:55:38 +00:00

Fix three compiler bugs exposed by array-using examples

Landing bug A from the previous writeup plus two adjacent bugs
that the fix exposed. All three miscompile anything that uses a
u8[N] global with a literal initializer.

1. Array-literal globals are now actually initialized.
   `lower_program` only expanded `Expr::StructLiteral` into per-
   field synthetic globals — `Expr::ArrayLiteral` hit
   `eval_const`, returned `None`, and the array boot-cleared to
   zero. `IrGlobal` now carries an `init_array: Vec<u8>`
   populated by lowering, and the IR codegen startup loop emits
   one `LDA #byte; STA base+i` pair per element.

2. Local variables no longer overlap array globals.
   `IrCodeGen::new` advanced `local_ram_next` past
   `max_global_base + 1` — for an array at `$0300-$0303` it
   placed the first handler-local at `$0301`, inside the array.
   The frame handler's stores through the local then corrupted
   the array mid-frame. The allocator now walks the analyzer's
   `VarAllocation` list and advances past `address + size` for
   every RAM global, not just the base.

3. Peephole `remove_redundant_loads` honors indexed LDAs.
   The pass tracked `LDA Immediate/ZeroPage/Absolute` but let
   `LDA AbsoluteX/AbsoluteY/ZeroPageX/IndirectX/IndirectY` fall
   through the match, leaving the A-equivalence tracker
   unchanged. A later `LDA #v` that happened to match a stale
   entry from BEFORE the indexed load would then be dropped as
   "already in A" — a silent miscompile that turned every
   `draw Sprite at: (arr[i], arr[j])` pattern into garbage
   (the second array index would be computed from `arr[i]`'s
   value, reading way out of bounds). Indexed LDAs now clear
   the tracker.

Regression tests:
- `src/codegen/peephole.rs`: a synthetic
  `LDA #0; TAX; LDA AbsX(arr1); STA temp; LDA #0; TAX;
   LDA AbsX(arr2); ...` sequence asserts both `LDA #0`s survive.
- `src/ir/tests.rs`: verifies `var xs: u8[4] = [1,2,3,4]`
  populates `IrGlobal::init_array` with `[1,2,3,4]`.
- `tests/integration_test.rs`: two IR-codegen tests — one checks
  the startup instructions contain `LDA #v; STA base+i` for
  every element, the other compiles a handler-local var
  alongside an array global and asserts no post-init stores
  land inside the array.

Smoke test impact (14/14 still passing, now more visible):
- arrays_and_functions:  56 -> 104 nonBlack, now animated
- loop_break_continue:   52 -> 208 (player + 3 hazards visible)
- structs_enums_for:     52 -> 104 (player + enemy visible)

Existing examples unchanged; no remaining work for bug B
(static OAM slot allocation in loops) — that's the next PR.

https://claude.ai/code/session_014Z5y3Q9krLcAxYpZQJhZ5V
This commit is contained in:
Claude 2026-04-12 19:32:22 +00:00
parent 1525922faa
commit f49dbce686
No known key found for this signature in database
9 changed files with 317 additions and 10 deletions

View file

@ -84,12 +84,22 @@ impl<'a> IrCodeGen<'a> {
// disjoint across functions so nested calls don't corrupt // disjoint across functions so nested calls don't corrupt
// each other. // each other.
let mut local_ram_next: u16 = 0x0300; let mut local_ram_next: u16 = 0x0300;
// Advance past any global addresses so we don't clobber them. // Advance past any RAM global so locals don't clobber them.
// This is conservative: scan existing var_addrs for the max. // Each global occupies `[address, address + size)` — for an
if let Some(max_global) = var_addrs.values().copied().max() { // array global at $0308 with size=4 that's $0308..$030C. We
if max_global >= local_ram_next { // must advance past the END, not the base, otherwise
local_ram_next = max_global + 1; // subsequent locals overlap with the tail of the array.
} // Globals are looked up by name against the analyzer's
// `allocations` (which has per-global sizes) rather than the
// `var_addrs` map, which only stores base addresses.
let max_ram_global_end = allocations
.iter()
.filter(|a| a.address >= 0x0100)
.map(|a| a.address + a.size.max(1))
.max()
.unwrap_or(0);
if max_ram_global_end > local_ram_next {
local_ram_next = max_ram_global_end;
} }
for func in &ir.functions { for func in &ir.functions {
for (i, local) in func.locals.iter().enumerate() { for (i, local) in func.locals.iter().enumerate() {
@ -188,16 +198,33 @@ impl<'a> IrCodeGen<'a> {
} }
// 1. Variable initializers // 1. Variable initializers
//
// Scalars write a single byte from `init_value`. Array
// literals write N bytes from `init_array` at successive
// offsets from the global's base address. Uninitialized
// globals (neither set) stay at the $00 the RAM-clear left
// them.
for global in &ir.globals { for global in &ir.globals {
if let Some(val) = global.init_value { let Some(&base) = self.var_addrs.get(&global.var_id) else {
if let Some(&addr) = self.var_addrs.get(&global.var_id) { continue;
self.emit(LDA, AM::Immediate(val as u8)); };
if !global.init_array.is_empty() {
for (offset, &byte) in global.init_array.iter().enumerate() {
let addr = base.wrapping_add(offset as u16);
self.emit(LDA, AM::Immediate(byte));
if addr < 0x100 { if addr < 0x100 {
self.emit(STA, AM::ZeroPage(addr as u8)); self.emit(STA, AM::ZeroPage(addr as u8));
} else { } else {
self.emit(STA, AM::Absolute(addr)); self.emit(STA, AM::Absolute(addr));
} }
} }
} else if let Some(val) = global.init_value {
self.emit(LDA, AM::Immediate(val as u8));
if base < 0x100 {
self.emit(STA, AM::ZeroPage(base as u8));
} else {
self.emit(STA, AM::Absolute(base));
}
} }
} }

View file

@ -392,6 +392,24 @@ fn remove_redundant_loads(instructions: &mut Vec<Instruction>) {
eq.clear(); eq.clear();
eq.push(AValue::Abs(*addr)); eq.push(AValue::Abs(*addr));
} }
// Indexed, indirect, and accumulator-mode LDAs clobber A
// but the value they load isn't trackable here (the
// effective address depends on a register or memory we
// don't track), so we can't add it to `eq`. We MUST still
// clear the tracker — otherwise a subsequent `LDA #v`
// might look redundant against a stale entry from before
// the indexed load, and get dropped. That's a miscompile,
// not an optimization.
(
Opcode::LDA,
AddressingMode::AbsoluteX(_)
| AddressingMode::AbsoluteY(_)
| AddressingMode::ZeroPageX(_)
| AddressingMode::IndirectX(_)
| AddressingMode::IndirectY(_),
) => {
eq.clear();
}
(Opcode::STA, AddressingMode::ZeroPage(addr)) => { (Opcode::STA, AddressingMode::ZeroPage(addr)) => {
// A unchanged; address now holds A's value. Add the // A unchanged; address now holds A's value. Add the
// address to the equivalence class. // address to the equivalence class.
@ -941,4 +959,76 @@ mod tests {
optimize(&mut insts); optimize(&mut insts);
assert_eq!(insts.len(), before); assert_eq!(insts.len(), before);
} }
#[test]
fn indexed_load_invalidates_redundant_load_tracker() {
// Regression test for a miscompile that affected every
// `draw Sprite at: (arr[i], arr[j])` pattern in the IR
// codegen. The original `remove_redundant_loads` only
// tracked `LDA Immediate/ZeroPage/Absolute`; indexed-mode
// loads like `LDA AbsoluteX(...)` fell through the match
// and left the A-equivalence tracker unchanged. A later
// `LDA #imm` that happened to match a stale entry from
// BEFORE the indexed load was then silently dropped as
// "already in A" — even though A really held the element
// the AbsoluteX just loaded.
//
// The buggy pattern: load 0 into A to index array1, load
// arr1[0], STASH it in a temp (so remove_dead_loads keeps
// the AbsoluteX), load 0 again to index array2, read
// arr2[0]. With the buggy pass, the second `LDA #0` was
// dropped as redundant because the tracker still said
// A = Imm(0) from before the AbsoluteX. Then TAX would
// push the arr1[0] value into X and the second array
// load would use an out-of-bounds index.
let stash = 0x90; // a temp slot addr >= 0x80
let mut insts = vec![
Instruction::new(LDA, AM::Immediate(0)),
Instruction::new(TAX, AM::Implied),
Instruction::new(LDA, AM::AbsoluteX(0x0300)),
Instruction::new(STA, AM::ZeroPage(stash)),
Instruction::new(LDA, AM::Immediate(0)),
Instruction::new(TAX, AM::Implied),
Instruction::new(LDA, AM::AbsoluteX(0x0308)),
Instruction::new(STA, AM::Absolute(0x0200)),
Instruction::new(LDA, AM::ZeroPage(stash)),
Instruction::new(STA, AM::Absolute(0x0203)),
Instruction::new(RTS, AM::Implied),
];
optimize(&mut insts);
// The exact shape after optimization isn't the point —
// what matters is that there are still two `TAX`
// instructions each preceded by a fresh `LDA #0` so
// both AbsoluteX loads target index 0. If the optimizer
// dropped the second `LDA #0`, the second `TAX` would
// copy whatever arr1[0] was into X and the second
// AbsoluteX load would read arr2[arr1[0]] — wildly out
// of bounds.
let mut saw_lda_zero_before_tax = 0;
let mut saw_other_lda_before_tax = false;
let mut last_lda: Option<AM> = None;
for inst in &insts {
match inst.opcode {
LDA => {
last_lda = Some(inst.mode.clone());
}
TAX => {
match &last_lda {
Some(AM::Immediate(0)) => saw_lda_zero_before_tax += 1,
_ => saw_other_lda_before_tax = true,
}
last_lda = None;
}
_ => {}
}
}
assert!(
!saw_other_lda_before_tax,
"a TAX is preceded by a non-Imm(0) LDA — optimizer kept a stale index: {insts:?}"
);
assert_eq!(
saw_lda_zero_before_tax, 2,
"both TAXes must be preceded by a fresh LDA #0: {insts:?}"
);
}
} }

View file

@ -193,15 +193,26 @@ impl LoweringContext {
// Struct-literal initializers are expanded into per-field // Struct-literal initializers are expanded into per-field
// globals so each field gets its own `init_value`; the parent // globals so each field gets its own `init_value`; the parent
// struct itself is still registered (size=0) so any later IR // struct itself is still registered (size=0) so any later IR
// op referencing it by name still resolves. // op referencing it by name still resolves. Array-literal
// initializers are lowered into `init_array` on the parent
// global — the IR codegen's startup loop emits one LDA/STA
// per byte into the global's base address.
for var in &program.globals { for var in &program.globals {
let var_id = self.get_or_create_var(&var.name); let var_id = self.get_or_create_var(&var.name);
let init = var.init.as_ref().and_then(|e| self.eval_const(e)); let init = var.init.as_ref().and_then(|e| self.eval_const(e));
let init_array = match &var.init {
Some(Expr::ArrayLiteral(elems, _)) => elems
.iter()
.filter_map(|e| self.eval_const(e).map(|v| v as u8))
.collect(),
_ => Vec::new(),
};
self.globals.push(IrGlobal { self.globals.push(IrGlobal {
var_id, var_id,
name: var.name.clone(), name: var.name.clone(),
size: type_size(&var.var_type), size: type_size(&var.var_type),
init_value: init, init_value: init,
init_array,
}); });
if let Some(Expr::StructLiteral(_, fields, _)) = &var.init { if let Some(Expr::StructLiteral(_, fields, _)) = &var.init {
for (fname, fexpr) in fields { for (fname, fexpr) in fields {
@ -213,6 +224,7 @@ impl LoweringContext {
name: full, name: full,
size: 1, size: 1,
init_value: fval, init_value: fval,
init_array: Vec::new(),
}); });
} }
} }

View file

@ -45,7 +45,18 @@ pub struct IrGlobal {
pub var_id: VarId, pub var_id: VarId,
pub name: String, pub name: String,
pub size: u16, pub size: u16,
/// Scalar initial value for single-byte globals. `None` means the
/// RAM-clear at reset leaves this global at 0.
pub init_value: Option<u16>, pub init_value: Option<u16>,
/// Per-byte initial contents for array-literal globals
/// (e.g. `var xs: u8[4] = [1,2,3,4]`). Empty for scalars or
/// uninitialized arrays. Each entry is the initial byte at offset
/// `i` from the global's base address; trailing bytes not covered
/// by the literal stay zero-filled by the hardware init's RAM
/// clear. Mutually exclusive with a meaningful `init_value` in
/// practice: `lower_program` takes one path for scalars and
/// another for array literals.
pub init_array: Vec<u8>,
} }
/// A block of constant data to be placed in ROM. /// A block of constant data to be placed in ROM.

View file

@ -312,3 +312,32 @@ fn lower_wait_frame() {
.any(|op| matches!(op, IrOp::WaitFrame)); .any(|op| matches!(op, IrOp::WaitFrame));
assert!(has_wait, "should emit WaitFrame op"); assert!(has_wait, "should emit WaitFrame op");
} }
#[test]
fn array_literal_global_init_is_captured() {
// Regression test: `var xs: u8[4] = [1, 2, 3, 4]` used to lose
// its initializer because `eval_const` returns None for
// `Expr::ArrayLiteral` and `init_value` ended up `None`. The
// fix captures the per-element values in a new `init_array`
// field so the IR codegen can emit one `LDA #imm; STA base+i`
// per byte at startup.
let ir = lower_ok(
r#"
game "Arr" { mapper: NROM }
var xs: u8[4] = [1, 2, 3, 4]
on frame { wait_frame }
start Main
"#,
);
let xs = ir
.globals
.iter()
.find(|g| g.name == "xs")
.expect("`xs` global should exist");
assert_eq!(
xs.init_array,
vec![1, 2, 3, 4],
"array literal initializer should populate init_array: {:?}",
xs.init_array
);
}

Binary file not shown.

Before

Width:  |  Height:  |  Size: 828 B

After

Width:  |  Height:  |  Size: 882 B

Before After
Before After

Binary file not shown.

Before

Width:  |  Height:  |  Size: 816 B

After

Width:  |  Height:  |  Size: 963 B

Before After
Before After

Binary file not shown.

Before

Width:  |  Height:  |  Size: 816 B

After

Width:  |  Height:  |  Size: 854 B

Before After
Before After

View file

@ -854,3 +854,141 @@ fn ir_codegen_multi_oam() {
let rom_data = compile_with_ir_codegen(source); let rom_data = compile_with_ir_codegen(source);
rom::validate_ines(&rom_data).expect("should be valid iNES"); rom::validate_ines(&rom_data).expect("should be valid iNES");
} }
#[test]
fn ir_codegen_array_literal_globals_emit_per_byte_init() {
// Regression test: `var xs: u8[4] = [10, 20, 30, 40]` used to
// compile to a zero-initialized array because `eval_const`
// returned `None` for `Expr::ArrayLiteral` and no startup
// stores were emitted. The fix captures the literal values
// in `IrGlobal::init_array` and has the IR codegen emit one
// `LDA #imm; STA base+i` per byte during startup.
use nescript::asm::{AddressingMode, Opcode};
use nescript::codegen::IrCodeGen;
let source = r#"
game "ArrLit" { mapper: NROM }
var xs: u8[4] = [10, 20, 30, 40]
on frame { wait_frame }
start Main
"#;
let (prog, diags) = nescript::parser::parse(source);
assert!(diags.is_empty(), "parse errors: {diags:?}");
let prog = prog.unwrap();
let analysis = analyzer::analyze(&prog);
let mut ir_program = ir::lower(&prog, &analysis);
optimizer::optimize(&mut ir_program);
let xs_addr = analysis
.var_allocations
.iter()
.find(|a| a.name == "xs")
.expect("xs should be allocated")
.address;
let codegen = IrCodeGen::new(&analysis.var_allocations, &ir_program);
let instructions = codegen.generate(&ir_program);
// For each element, look for `LDA #val` followed shortly by
// `STA absolute(xs_addr + i)`. We don't require them to be
// adjacent because the peephole passes can reshuffle, but a
// store of the correct value to the correct address must
// exist.
for (i, &expected) in [10u8, 20, 30, 40].iter().enumerate() {
let target = xs_addr + i as u16;
let has_store = instructions.windows(2).any(|w| {
matches!(w[0].mode, AddressingMode::Immediate(v) if v == expected)
&& w[0].opcode == Opcode::LDA
&& w[1].opcode == Opcode::STA
&& matches!(w[1].mode, AddressingMode::Absolute(a) if a == target)
});
assert!(
has_store,
"expected `LDA #{expected}; STA ${target:04X}` for xs[{i}] but did not find it"
);
}
}
#[test]
fn ir_codegen_locals_do_not_overlap_array_globals() {
// Regression test for the local-allocator off-by-array-size
// bug. `IrCodeGen::new` used to start handler-local vars at
// `max_global_base + 1`, which for an array global at
// `$0300-$0303` put the first local at `$0301` — inside the
// array. Any store through that local then corrupted the
// array mid-frame. The fix advances past the global's END,
// not its base.
//
// We verify by asking the IR codegen what addresses it
// assigned. Since `var_addrs` is private, we check indirectly
// via emitted instructions: any `STA $030N` for N > 3 that
// isn't part of the startup init must be writing to a local
// whose address is outside the array. If the bug regressed,
// we'd see `STA $0302` or similar in the frame handler's
// computation code.
use nescript::asm::{AddressingMode, Opcode};
use nescript::codegen::IrCodeGen;
let source = r#"
game "LocalVsArr" { mapper: NROM }
var xs: u8[4] = [11, 22, 33, 44]
on frame {
var tmp: u8 = 0
tmp = xs[0]
tmp += 1
wait_frame
}
start Main
"#;
let (prog, diags) = nescript::parser::parse(source);
assert!(diags.is_empty(), "parse errors: {diags:?}");
let prog = prog.unwrap();
let analysis = analyzer::analyze(&prog);
let mut ir_program = ir::lower(&prog, &analysis);
optimizer::optimize(&mut ir_program);
let xs_alloc = analysis
.var_allocations
.iter()
.find(|a| a.name == "xs")
.expect("xs should be allocated");
let xs_base = xs_alloc.address;
let xs_end = xs_base + xs_alloc.size; // one past last element
let codegen = IrCodeGen::new(&analysis.var_allocations, &ir_program);
let instructions = codegen.generate(&ir_program);
// Collect the (ordered) list of `STA absolute` targets and
// immediate values preceding each store. The first four
// stores into `[xs_base, xs_end)` should be the `LDA #imm;
// STA addr` init pairs — those are fine. Any STA into the
// array AFTER the init sequence would indicate a local var
// was allocated inside the array.
let mut init_stores_seen = 0usize;
for w in instructions.windows(2) {
if w[1].opcode != Opcode::STA {
continue;
}
let AddressingMode::Absolute(addr) = w[1].mode else {
continue;
};
if addr < xs_base || addr >= xs_end {
continue;
}
if w[0].opcode == Opcode::LDA
&& matches!(w[0].mode, AddressingMode::Immediate(_))
&& init_stores_seen < 4
{
init_stores_seen += 1;
continue;
}
panic!(
"store into xs array (${addr:04X}) after init sequence — \
local probably overlapping with array global"
);
}
assert_eq!(
init_stores_seen, 4,
"expected 4 init stores for xs[0..4], found {init_stores_seen}"
);
}