imjasonh/nescript
1
0
Fork 0
mirror of https://github.com/imjasonh/nescript synced 2026-07-09 01:16:12 +00:00
Code Activity

codegen: make var_addrs misses panic loudly and fix latent struct-field silent drop

Browse source 
Phase 1 of the post-PR-#31 audit. The PR #31 state-local bug had a
specific shape: analyzer allocated a slot, codegen looked it up by
VarId, silently emitted nothing on miss. Six sites in gen_op plus
the global-initializer loop and the parameter-shuffle prologue all
used the same `if let Some(&addr) = self.var_addrs.get(var) { ... }`
pattern with no else branch. Any future allocation-map desync would
slip through the same crack.

Replace every site with a new `IrCodeGen::var_addr(VarId) -> u16`
helper that panics with an explicit "compiler bug" message on miss.
An IR op referencing an unmapped VarId is not valid input — it means
the analyzer and lowerer disagreed on what to allocate, and we want
that crash to surface in CI rather than be absorbed by whatever
zero-filled RAM happened to sit at the read.

Running cargo test against the hardened lookup surfaced exactly the
bug shape the plan predicted: uninitialized struct globals (e.g.
`var p: Point` with no literal initializer) never had their flattened
field VarIds (`"p.x"`, `"p.y"`) registered in var_addrs. The IR
lowerer's `get_or_create_var("p.x")` minted a VarId, the analyzer's
`flatten_struct_fields` allocated an address for it, but IrCodeGen::new
only populated var_addrs from `ir.globals`, which doesn't contain
synthesized field entries for uninitialized structs. Every `p.x = N`
silently compiled to nothing.

Fix by exposing the IR lowerer's name→VarId map on IrProgram and
joining it with the analyzer's allocations in IrCodeGen::new. Every
allocated name that the lowerer knows about now gets a var_addrs
entry. Example ROMs are byte-identical (no example relied on the
dropped writes), but the bug was reachable — any user program with
a plain `var pos: Point` declaration and field writes would have hit
it silently.

Add `uninitialized_struct_field_store_emits_sta_to_allocated_address`
as a byte-level regression guard: compile `p.x = 123` and scan PRG
for `LDA #\$7B / STA <addr>`. Fails against the old silently-dropping
codegen.

https://claude.ai/code/session_01AoQ678uVeqpyayvWHpfDhC
This commit is contained in:
Claude 2026-04-17 23:49:29 +00:00
parent 431f144be7
commit f7012c6533
No known key found for this signature in database
5 changed files with 144 additions and 60 deletions
Download patch file Download diff file Expand all files Collapse all files

148
src/codegen/ir_codegen.rs
View file

@ -290,6 +290,19 @@ impl<'a> IrCodeGen<'a> {
var_sizes.insert(global.var_id, alloc.size);
}
}
// Pick up every *other* allocation the analyzer made that the
// IR lowerer synthesized a `VarId` for but didn't push as an
// `IrGlobal` — flattened struct fields (`"pos.x"`) are the
// biggest class, but this also catches any future synthesized
// name. Without this, `pos.x = 100` resolves its `VarId`
// against a `var_addrs` that never learned the address, and
// the codegen silently drops the write (the PR #31 shape).
for alloc in allocations {
if let Some(&var_id) = ir.var_map.get(&alloc.name) {
var_addrs.entry(var_id).or_insert(alloc.address);
var_sizes.entry(var_id).or_insert(alloc.size);
}
}
// Map every function-local — parameters AND body-declared
// vars — into the slot the analyzer already reserved for it.
// Parameters arrive via the zero-page transport slots
@ -510,6 +523,22 @@ impl<'a> IrCodeGen<'a> {
.push(Instruction::new(NOP, AM::Label(name.to_string())));
}
/// Resolve a variable's allocated address. Any IR op referencing a
/// `VarId` must have been allocated by the analyzer — a miss here
/// is a compiler bug, not valid input (this is the shape of the
/// state-local silent-drop from PR #31). Crash loudly rather than
/// emit zero bytes and let a golden capture the broken behaviour.
fn var_addr(&self, var: VarId) -> u16 {
*self.var_addrs.get(&var).unwrap_or_else(|| {
panic!(
"internal compiler error: IR op references {var:?} with no \
allocated address (did the analyzer forget to allocate it, \
or did the IR lowerer synthesize a VarId it didn't register \
as a global/local?)"
)
})
}
/// Return the zero-page address for an IR temp, allocating a new slot
/// if needed. Recycles slots from `free_slots` (temps whose use
/// count has hit zero) before growing the monotonic counter, so
@ -705,11 +734,17 @@ impl<'a> IrCodeGen<'a> {
// 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.
// them — and since struct-literal parents only exist to
// parent their expanded field globals, they land here with
// no init data and no allocated address (only the fields
// have addresses). Skip before calling `var_addr` so we
// don't mis-diagnose a legitimate container as a silent
// drop.
for global in &ir.globals {
let Some(&base) = self.var_addrs.get(&global.var_id) else {
if global.init_array.is_empty() && global.init_value.is_none() {
continue;
};
}
let base = self.var_addr(global.var_id);
if !global.init_array.is_empty() {
for (offset, &byte) in global.init_array.iter().enumerate() {
let addr = base.wrapping_add(offset as u16);
@ -976,13 +1011,12 @@ impl<'a> IrCodeGen<'a> {
.take(4)
.enumerate()
{
if let Some(&addr) = self.var_addrs.get(&local.var_id) {
self.emit(LDA, AM::ZeroPage(0x04 + i as u8));
if addr < 0x100 {
self.emit(STA, AM::ZeroPage(addr as u8));
} else {
self.emit(STA, AM::Absolute(addr));
}
let addr = self.var_addr(local.var_id);
self.emit(LDA, AM::ZeroPage(0x04 + i as u8));
if addr < 0x100 {
self.emit(STA, AM::ZeroPage(addr as u8));
} else {
self.emit(STA, AM::Absolute(addr));
}
}
}
@ -1203,23 +1237,21 @@ impl<'a> IrCodeGen<'a> {
self.store_temp(*dest);
}
IrOp::LoadVar(dest, var) => {
if let Some(&addr) = self.var_addrs.get(var) {
if addr < 0x100 {
self.emit(LDA, AM::ZeroPage(addr as u8));
} else {
self.emit(LDA, AM::Absolute(addr));
}
self.store_temp(*dest);
let addr = self.var_addr(*var);
if addr < 0x100 {
self.emit(LDA, AM::ZeroPage(addr as u8));
} else {
self.emit(LDA, AM::Absolute(addr));
}
self.store_temp(*dest);
}
IrOp::StoreVar(var, src) => {
if let Some(&addr) = self.var_addrs.get(var) {
self.load_temp(*src);
if addr < 0x100 {
self.emit(STA, AM::ZeroPage(addr as u8));
} else {
self.emit(STA, AM::Absolute(addr));
}
let addr = self.var_addr(*var);
self.load_temp(*src);
if addr < 0x100 {
self.emit(STA, AM::ZeroPage(addr as u8));
} else {
self.emit(STA, AM::Absolute(addr));
}
}
IrOp::Add(d, a, b) => {
@ -1340,29 +1372,27 @@ impl<'a> IrCodeGen<'a> {
IrOp::CmpLtEq(d, a, b) => self.gen_cmp(*d, *a, *b, CmpKind::LtEq),
IrOp::CmpGtEq(d, a, b) => self.gen_cmp(*d, *a, *b, CmpKind::GtEq),
IrOp::ArrayLoad(dest, var, idx) => {
if let Some(&base_addr) = self.var_addrs.get(var) {
self.load_temp(*idx);
self.emit_bounds_check(*var);
self.emit(TAX, AM::Implied);
if base_addr < 0x100 {
self.emit(LDA, AM::ZeroPageX(base_addr as u8));
} else {
self.emit(LDA, AM::AbsoluteX(base_addr));
}
self.store_temp(*dest);
let base_addr = self.var_addr(*var);
self.load_temp(*idx);
self.emit_bounds_check(*var);
self.emit(TAX, AM::Implied);
if base_addr < 0x100 {
self.emit(LDA, AM::ZeroPageX(base_addr as u8));
} else {
self.emit(LDA, AM::AbsoluteX(base_addr));
}
self.store_temp(*dest);
}
IrOp::ArrayStore(var, idx, val) => {
if let Some(&base_addr) = self.var_addrs.get(var) {
self.load_temp(*idx);
self.emit_bounds_check(*var);
self.emit(TAX, AM::Implied);
self.load_temp(*val);
if base_addr < 0x100 {
self.emit(STA, AM::ZeroPageX(base_addr as u8));
} else {
self.emit(STA, AM::AbsoluteX(base_addr));
}
let base_addr = self.var_addr(*var);
self.load_temp(*idx);
self.emit_bounds_check(*var);
self.emit(TAX, AM::Implied);
self.load_temp(*val);
if base_addr < 0x100 {
self.emit(STA, AM::ZeroPageX(base_addr as u8));
} else {
self.emit(STA, AM::AbsoluteX(base_addr));
}
}
IrOp::Call(dest, name, args) => {
@ -1691,25 +1721,23 @@ impl<'a> IrCodeGen<'a> {
IrOp::SetPalette(name) => self.gen_set_palette(name),
IrOp::LoadBackground(name) => self.gen_load_background(name),
IrOp::LoadVarHi(dest, var) => {
if let Some(&base) = self.var_addrs.get(var) {
let addr = base.wrapping_add(1);
if addr < 0x100 {
self.emit(LDA, AM::ZeroPage(addr as u8));
} else {
self.emit(LDA, AM::Absolute(addr));
}
self.store_temp(*dest);
let base = self.var_addr(*var);
let addr = base.wrapping_add(1);
if addr < 0x100 {
self.emit(LDA, AM::ZeroPage(addr as u8));
} else {
self.emit(LDA, AM::Absolute(addr));
}
self.store_temp(*dest);
}
IrOp::StoreVarHi(var, src) => {
if let Some(&base) = self.var_addrs.get(var) {
let addr = base.wrapping_add(1);
self.load_temp(*src);
if addr < 0x100 {
self.emit(STA, AM::ZeroPage(addr as u8));
} else {
self.emit(STA, AM::Absolute(addr));
}
let base = self.var_addr(*var);
let addr = base.wrapping_add(1);
self.load_temp(*src);
if addr < 0x100 {
self.emit(STA, AM::ZeroPage(addr as u8));
} else {
self.emit(STA, AM::Absolute(addr));
}
}
IrOp::Add16 {

1
src/ir/lowering.rs
View file

@ -534,6 +534,7 @@ impl LoweringContext {
rom_data: self.rom_data,
states: self.state_names,
start_state: self.start_state,
var_map: self.var_map,
}
}

9
src/ir/mod.rs
View file

@ -37,6 +37,15 @@ pub struct IrProgram {
pub states: Vec<String>,
/// Name of the initial state when the ROM boots.
pub start_state: String,
/// Name → `VarId` for every variable the lowerer synthesized,
/// including flattened struct fields (`"pos.x"`), function-scoped
/// locals, and state-local variables. The codegen joins this with
/// the analyzer's `var_allocations` list to populate `var_addrs`
/// for every allocated byte — without it, assignments to
/// synthesized field names (e.g. `pos.x = 100`) silently emit no
/// code because the field's `VarId` never lands in the address
/// map. This is the PR #31 state-local bug generalized.
pub var_map: std::collections::HashMap<String, VarId>,
}
/// A global variable in the IR.

3
src/optimizer/tests.rs
View file

@ -22,6 +22,7 @@ fn make_program(ops: Vec<IrOp>, terminator: IrTerminator) -> IrProgram {
rom_data: vec![],
states: vec![],
start_state: String::new(),
var_map: std::collections::HashMap::new(),
}
}
@ -420,6 +421,7 @@ fn inline_removes_trivial() {
rom_data: vec![],
states: vec![],
start_state: String::new(),
var_map: std::collections::HashMap::new(),
};
inline_small_functions(&mut prog);
@ -502,6 +504,7 @@ fn const_fold_preserves_loadimm_used_by_sibling_branch() {
rom_data: vec![],
states: vec![],
start_state: String::new(),
var_map: std::collections::HashMap::new(),
};
optimize(&mut prog);

43
tests/integration_test.rs
View file

@ -335,6 +335,49 @@ fn program_with_u16_struct_field() {
rom::validate_ines(&rom_data).expect("should be valid iNES");
}
#[test]
fn uninitialized_struct_field_store_emits_sta_to_allocated_address() {
// Regression guard for the silent-drop bug uncovered while
// hardening the `var_addrs` lookup in `IrCodeGen`. Before the
// fix, field `VarId`s synthesized by the IR lowerer (e.g.
// `"pos.x"`) were only registered in `var_addrs` when their
// parent struct global had a literal initializer. An
// uninitialized `var pos: Point` produced no field globals, so
// `pos.x = 100` emitted `IrOp::StoreVar(VarId(for pos.x), ...)`
// — and the codegen's `if let Some(&addr) = var_addrs.get(..)`
// guard skipped it, silently dropping the write with no
// diagnostic.
//
// We verify by compiling a program whose entire frame handler
// is a write with a distinctive immediate constant, then
// search the PRG for the corresponding `LDA #$7B ; STA zp/abs`
// pair. `123 = $7B` is chosen because it can't appear as an
// incidental constant in the runtime prelude.
let source = r#"
game "StructStore" { mapper: NROM }
struct Point { x: u8, y: u8 }
var p: Point
on frame {
p.x = 123
}
start Main
"#;
let rom_data = compile(source);
let prg = &rom_data[16..16 + 16384];
let mut found = false;
for i in 0..prg.len().saturating_sub(3) {
if prg[i] == 0xA9 && prg[i + 1] == 0x7B && (prg[i + 2] == 0x85 || prg[i + 2] == 0x8D) {
found = true;
break;
}
}
assert!(
found,
"expected an LDA #$7B / STA <addr> pair for `p.x = 123` — if this \
fails, struct-field writes are being silently dropped again"
);
}
#[test]
fn u16_struct_field_initializer_writes_both_bytes_to_rom() {
// Struct literal initializer with a u16 field > 255 — the