imjasonh/nescript
1
0
Fork 0
mirror of https://github.com/imjasonh/nescript synced 2026-07-18 06:36:57 +00:00
Code Activity

ir: clear wide_hi between functions to fix 16-bit op aliasing

Browse source 
The IrLowerer's wide_hi map records "this u8 temp's high byte
lives at this other temp" pairs whenever a 16-bit value is
produced. Both lower_function and lower_handler reset next_temp
to 0 at the start of each function, but neither cleared wide_hi
— so stale (low_id -> high_id) entries from earlier functions
leaked into subsequent ones.

When a fresh function reused those temp IDs for unrelated u8
expressions, is_wide() returned spurious true and widen() handed
back stale (lo, hi) pairs whose hi happened to coincide with the
*next* temp ID fresh_temp() was about to allocate. The result
was 16-bit IR ops (CmpEq16 in particular) where the destination
temp aliased one of the source operand high bytes — for War this
made `match phase` arms past P_WIN_B impossible to enter and the
game would freeze with both face-up cards on the table forever.

Fix: clear wide_hi alongside the next_temp reset in both
lower_function and lower_handler. Adds a regression test
(ir::tests::wide_hi_does_not_leak_between_functions) that
constructs a function whose body has no u16 ops but follows a
function that does, and asserts no CmpEq16 op aliases its dest
with an operand high byte.

Also:
- Convert the war Playing state's phase machine from an
  if-chain to a `match`, which is what tripped this bug to the
  surface (it was lurking in earlier ROMs too but their layouts
  never produced the dest/source collision shape).
- Refactor begin_draw_a/b to set fly_card / fly_face_up via
  globals before calling arm_fly, since arm_fly only takes 4
  params (the v0.1 ABI limit, now diagnosed by E0506).
- Hoist the P_RESOLVE comparison result to the global pf_result
  to dodge the param-clobbering issue documented in
  examples/war/COMPILER_BUGS.md §2.
- Document the bug as item #6 in COMPILER_BUGS.md with a
  minimal repro and reproducer-test pointer.
- Refresh the war golden + audio hash to match the new ROM.

https://claude.ai/code/session_0143dTgh3UeRrtfHgQwzcv5z
This commit is contained in:
Claude 2026-04-15 15:57:26 +00:00
parent 155a0e7096
commit 4e8e349d7c
No known key found for this signature in database
8 changed files with 334 additions and 169 deletions
Download patch file Download diff file Expand all files Collapse all files

BIN
examples/war.nes
View file

Binary file not shown.

90
examples/war/COMPILER_BUGS.md
View file

@ -418,6 +418,96 @@ None — we just live with the JSR overhead and the bug-2 fallout.
--- ---
## 6. `wide_hi` IR-lowering map leaked between functions and corrupted 16-bit ops *(FIXED)*
### Symptom
A function whose body had no 16-bit values whatsoever would
nonetheless emit `CmpEq16` (and other `Op16` variants) where the
*destination* temp aliased one of the *source* temps. The
resulting comparison effectively became "is this byte equal to
some uninitialised stack memory?", which in War caused the
phase-machine `match phase { ... }` dispatcher to skip the
`P_WIN_B` arm forever once the game first reached it — the game
would freeze with both cards face-up and "PLAYER B WINS" never
firing.
### Reproduction (pre-fix)
A handful of `u16` `+= 1` operations early in a state handler
followed by a long `match` chain on a `u8` was enough to trip it.
The minimum repro is roughly:
```nescript
var clock: u16 = 0
var phase: u8 = 0
on frame {
clock += 1 // wide op leaves wide_hi entries
match phase { // u8 match — should be 8-bit
0 => { phase = 1 }
1 => { phase = 2 }
2 => { phase = 3 }
3 => { phase = 4 }
4 => { phase = 5 }
5 => { phase = 6 }
6 => { phase = 7 }
7 => { /* corrupt — never matched */ }
_ => {}
}
}
```
The IR for the `phase == 7` arm came out as
`CmpEq16 { dest: T147, a_lo: T145, a_hi: T148, b_lo: T146,
b_hi: T147 }` — note `dest == b_hi`. The codegen happily emits
the corresponding 16-bit asm, but reads garbage for the `b_hi`
operand because it points at the same scratch slot the result
will be written to.
### Root cause
`src/ir/lowering.rs::IrLowerer` carries a `wide_hi: HashMap<IrTemp, IrTemp>`
that records "this low temp's high byte lives at this other
temp" pairs whenever a 16-bit value is produced. `lower_function`
and `lower_handler` both reset `next_temp = 0` at the start of
each function — but they did *not* clear `wide_hi`. Stale entries
from earlier functions stuck around and matched against fresh
temp IDs in subsequent functions (which start counting from 0
again), causing `is_wide(t)` and `widen(t)` to return spurious
"wide" results for what should have been narrow `u8` values.
When that happens inside `lower_binop`'s `Eq` path, `widen(r)`
returns the stale `(r, hi_r)` pair where `hi_r` happens to be the
*next* temp ID `fresh_temp()` will hand out a moment later — so
the `dest` temp and `b_hi` end up identical.
### Fix
`src/ir/lowering.rs`: in both `lower_function` and `lower_handler`,
add `self.wide_hi.clear();` immediately after `self.next_temp = 0;`.
Done in this PR.
### Why this didn't show up sooner
Every prior example either declared no `u16` globals at all, or
declared one and used it sparingly enough that the temp IDs
the leaked entries claimed never collided with the rest of the
function. War is the first example that combines a `u16`
free-running counter with a deep state machine that does many
`u8` comparisons in the same `on frame` body, which is exactly
the shape the bug needs to manifest.
### Regression test
`src/ir/tests.rs::wide_hi_does_not_leak_between_functions` (added
in this PR) compiles a two-function program where function A
uses a `u16 += 1` (creating wide entries) and function B does
`u8 == const` comparisons in a match. Pre-fix, the IR would emit
`CmpEq16` with aliased dest/source; post-fix it emits the
expected 8-bit `CmpEq`.
---
## Verification path after fixes ## Verification path after fixes
Once any of the bugs above are fixed in the compiler, the Once any of the bugs above are fixed in the compiler, the

332
examples/war/play_state.ne
View file

@ -121,191 +121,187 @@ state Playing {
draw_table() draw_table()
// ── Phase dispatch ─────────────────────────────── // ── Phase dispatch ───────────────────────────────
// The phases share a simple "timer hits target, advance" // We use `match` instead of a flat if-chain so each
// shape. Each arm of the if-chain is self-contained and // frame only runs the FIRST matching arm. A naive
// ends either with a set_phase call or a fall-through // chain like `if phase == X { ... } if phase == Y {...}`
// that waits for more input / time. // would let one phase transition into another (via
// `set_phase`) and then run the next phase's body in
if phase == P_WAIT_A { // the same frame, which advances animation timers twice
// Human prompt: hint blink above the deck. // and made the card-fly overshoot its endpoint by
if a_is_cpu == 0 { // FLY_STEP every time.
if (phase_timer & 32) == 0 { match phase {
draw_word_press(8, 200) P_WAIT_A => {
} // Human prompt: hint blink above the deck.
if button.a or button.start { if a_is_cpu == 0 {
begin_draw_a() if (phase_timer & 32) == 0 {
} draw_word_press(8, 200)
} else { }
// CPU draws after a short delay. if button.a or button.start {
if phase_timer >= CPU_THINK_FRAMES { begin_draw_a()
begin_draw_a() }
} else {
// CPU draws after a short delay.
if phase_timer >= CPU_THINK_FRAMES {
begin_draw_a()
}
} }
} }
} P_FLY_A => {
step_fly_pos()
if phase == P_FLY_A { draw_flying_card(fly_x, fly_y)
step_fly_pos() if phase_timer >= FRAMES_FLY {
draw_flying_card(fly_x, fly_y) set_phase(P_WAIT_B)
if phase_timer >= FRAMES_FLY {
set_phase(P_WAIT_B)
}
}
if phase == P_WAIT_B {
// A's card is now parked in its play slot.
draw_card_face(PLAY_A_X, PLAY_Y, card_a)
if b_is_cpu == 0 {
if (phase_timer & 32) == 0 {
draw_word_press(208, 200)
}
if p2.button.a or p2.button.start or button.a or button.start {
begin_draw_b()
}
} else {
if phase_timer >= CPU_THINK_FRAMES {
begin_draw_b()
} }
} }
} P_WAIT_B => {
// A's card is now parked in its play slot.
if phase == P_FLY_B { draw_card_face(PLAY_A_X, PLAY_Y, card_a)
// A is in place; B is flying. if b_is_cpu == 0 {
draw_card_face(PLAY_A_X, PLAY_Y, card_a) if (phase_timer & 32) == 0 {
step_fly_pos() draw_word_press(208, 200)
draw_flying_card(fly_x, fly_y) }
if phase_timer >= FRAMES_FLY { if p2.button.a or p2.button.start or button.a or button.start {
set_phase(P_REVEAL) begin_draw_b()
}
} else {
if phase_timer >= CPU_THINK_FRAMES {
begin_draw_b()
}
}
} }
} P_FLY_B => {
// A is in place; B is flying.
if phase == P_REVEAL { draw_card_face(PLAY_A_X, PLAY_Y, card_a)
draw_card_face(PLAY_A_X, PLAY_Y, card_a) step_fly_pos()
draw_card_face(PLAY_B_X, PLAY_Y, card_b) draw_flying_card(fly_x, fly_y)
if phase_timer >= FRAMES_REVEAL { if phase_timer >= FRAMES_FLY {
set_phase(P_RESOLVE) set_phase(P_REVEAL)
}
} }
} P_REVEAL => {
draw_card_face(PLAY_A_X, PLAY_Y, card_a)
if phase == P_RESOLVE { draw_card_face(PLAY_B_X, PLAY_Y, card_b)
// Both cards go into the pot regardless of outcome. if phase_timer >= FRAMES_REVEAL {
push_back_pot(card_a) set_phase(P_RESOLVE)
push_back_pot(card_b) }
var pf_r: u8 = compare_cards(card_a, card_b)
if pf_r == 1 {
play CheerA
set_phase(P_WIN_A)
} }
if pf_r == 2 { P_RESOLVE => {
play CheerB // Both cards go into the pot regardless of outcome.
set_phase(P_WIN_B) push_back_pot(card_a)
push_back_pot(card_b)
pf_result = compare_cards(card_a, card_b)
if pf_result == 1 {
play CheerA
set_phase(P_WIN_A)
}
if pf_result == 2 {
play CheerB
set_phase(P_WIN_B)
}
if pf_result == 0 {
// It's a tie — but only enter the war flow if both
// sides actually have cards left to bury. If a
// player ran out of cards on this very tie, the
// OTHER player wins by default and takes the pot.
if deck_a_count == 0 {
pot_to_b()
winner = 1
transition Victory
}
if deck_b_count == 0 {
pot_to_a()
winner = 0
transition Victory
}
play WarFlash
set_phase(P_WAR_BANNER)
}
} }
if pf_r == 0 { P_WIN_A => {
// It's a tie — but only enter the war flow if both draw_card_face(PLAY_A_X, PLAY_Y, card_a)
// sides actually have cards left to bury. If a draw_card_face(PLAY_B_X, PLAY_Y, card_b)
// player ran out of cards on this very tie, the if phase_timer >= FRAMES_FLY {
// OTHER player wins by default and takes the pot. pot_to_a()
if deck_a_count == 0 { set_phase(P_CHECK)
}
}
P_WIN_B => {
draw_card_face(PLAY_A_X, PLAY_Y, card_a)
draw_card_face(PLAY_B_X, PLAY_Y, card_b)
if phase_timer >= FRAMES_FLY {
pot_to_b() pot_to_b()
set_phase(P_CHECK)
}
}
P_WAR_BANNER => {
draw_card_face(PLAY_A_X, PLAY_Y, card_a)
draw_card_face(PLAY_B_X, PLAY_Y, card_b)
// Flashing big "WAR" banner — only drawn on alternate
// 8-frame windows so the title strobes for emphasis.
if (phase_timer & 8) != 0 {
draw_big_war_banner(96, 80)
}
if phase_timer >= FRAMES_BANNER {
set_phase(P_WAR_BURY)
}
}
P_WAR_BURY => {
// Bury up to 3 face-down cards from each deck, then
// draw a new face-up pair. We don't animate each
// individual buried card; just play a noise thump
// per buried card and advance the counters.
if phase_timer == 1 {
if deck_a_count > 0 { bury_from_a() }
if deck_b_count > 0 { bury_from_b() }
play ThudDown
}
if phase_timer == 4 {
if deck_a_count > 0 { bury_from_a() }
if deck_b_count > 0 { bury_from_b() }
play ThudDown
}
if phase_timer == 7 {
if deck_a_count > 0 { bury_from_a() }
if deck_b_count > 0 { bury_from_b() }
play ThudDown
}
if phase_timer == 10 {
// Draw new face-ups for the comparison. If either
// side has run out of cards, the OTHER side wins
// and takes the entire pot — we transition straight
// to Victory.
if deck_a_count == 0 {
pot_to_b()
winner = 1
transition Victory
}
if deck_b_count == 0 {
pot_to_a()
winner = 0
transition Victory
}
card_a = draw_front_a()
card_b = draw_front_b()
}
if phase_timer >= FRAMES_BURY + 16 {
set_phase(P_REVEAL)
}
}
P_CHECK => {
if deck_a_count == 0 {
winner = 1 winner = 1
transition Victory transition Victory
} }
if deck_b_count == 0 { if deck_b_count == 0 {
pot_to_a()
winner = 0 winner = 0
transition Victory transition Victory
} }
play WarFlash // No winner yet — start the next round.
set_phase(P_WAR_BANNER) card_a = 0
card_b = 0
set_phase(P_WAIT_A)
} }
} _ => {}
if phase == P_WIN_A {
draw_card_face(PLAY_A_X, PLAY_Y, card_a)
draw_card_face(PLAY_B_X, PLAY_Y, card_b)
if phase_timer >= FRAMES_FLY {
pot_to_a()
set_phase(P_CHECK)
}
}
if phase == P_WIN_B {
draw_card_face(PLAY_A_X, PLAY_Y, card_a)
draw_card_face(PLAY_B_X, PLAY_Y, card_b)
if phase_timer >= FRAMES_FLY {
pot_to_b()
set_phase(P_CHECK)
}
}
if phase == P_WAR_BANNER {
draw_card_face(PLAY_A_X, PLAY_Y, card_a)
draw_card_face(PLAY_B_X, PLAY_Y, card_b)
// Flashing big "WAR" banner — only drawn on alternate
// 8-frame windows so the title strobes for emphasis.
if (phase_timer & 8) != 0 {
draw_big_war_banner(96, 80)
}
if phase_timer >= FRAMES_BANNER {
set_phase(P_WAR_BURY)
}
}
if phase == P_WAR_BURY {
// Bury up to 3 face-down cards from each deck, then
// draw a new face-up pair. We don't animate each
// individual buried card; just play a noise thump
// per buried card and advance the counters.
if phase_timer == 1 {
if deck_a_count > 0 { bury_from_a() }
if deck_b_count > 0 { bury_from_b() }
play ThudDown
}
if phase_timer == 4 {
if deck_a_count > 0 { bury_from_a() }
if deck_b_count > 0 { bury_from_b() }
play ThudDown
}
if phase_timer == 7 {
if deck_a_count > 0 { bury_from_a() }
if deck_b_count > 0 { bury_from_b() }
play ThudDown
}
if phase_timer == 10 {
// Draw new face-ups for the comparison. If either
// side has run out of cards, the OTHER side wins
// and takes the entire pot — we transition straight
// to Victory.
if deck_a_count == 0 {
pot_to_b()
winner = 1
transition Victory
}
if deck_b_count == 0 {
pot_to_a()
winner = 0
transition Victory
}
card_a = draw_front_a()
card_b = draw_front_b()
}
if phase_timer >= FRAMES_BURY + 16 {
set_phase(P_REVEAL)
}
}
if phase == P_CHECK {
if deck_a_count == 0 {
winner = 1
transition Victory
}
if deck_b_count == 0 {
winner = 0
transition Victory
}
// No winner yet — start the next round.
card_a = 0
card_b = 0
set_phase(P_WAIT_A)
} }
} }
} }

1
examples/war/state.ne
View file

@ -40,6 +40,7 @@ var a_is_cpu: u8 = 1 // bool-ish
var b_is_cpu: u8 = 1 var b_is_cpu: u8 = 1
var phase: u8 = 0 // one of the P_* constants var phase: u8 = 0 // one of the P_* constants
var phase_timer: u8 = 0 // counts up during each phase var phase_timer: u8 = 0 // counts up during each phase
var pf_result: u8 = 0 // P_RESOLVE comparison result (1=A, 2=B, 0=tie)
// ── Animation state ─────────────────────────────────────── // ── Animation state ───────────────────────────────────────
// Shared by every card-fly phase. We step (fly_x, fly_y) by a // Shared by every card-fly phase. We step (fly_x, fly_y) by a

17
src/ir/lowering.rs
View file

@ -395,6 +395,17 @@ impl LoweringContext {
fn lower_function(&mut self, fun: &FunDecl) { fn lower_function(&mut self, fun: &FunDecl) {
self.next_temp = 0; self.next_temp = 0;
// Clear the wide-temp tracking map. `wide_hi` records "this
// low temp has its high byte at this other temp" entries
// produced by `make_wide`; without clearing it, the entries
// from previous functions leak into the next function and
// get matched against fresh temp IDs (since next_temp resets
// to 0). That manifests as `is_wide(t)` spuriously returning
// true and, worse, `widen(t)` returning a stale `hi` temp ID
// that collides with a later `fresh_temp()` allocation —
// producing 16-bit IR ops where the destination temp is
// *also* one of the source temps. See COMPILER_BUGS.md §6.
self.wide_hi.clear();
self.current_blocks = Vec::new(); self.current_blocks = Vec::new();
self.current_locals = Vec::new(); self.current_locals = Vec::new();
@ -460,6 +471,12 @@ impl LoweringContext {
fn lower_handler(&mut self, name: &str, block: &Block, state: &StateDecl) { fn lower_handler(&mut self, name: &str, block: &Block, state: &StateDecl) {
self.next_temp = 0; self.next_temp = 0;
// Same per-function reset as `lower_function`. See the
// commentary there and COMPILER_BUGS.md §6 for why this is
// critical — without it, state-handler bodies pick up wide
// temp pairs left over from the previous function and emit
// catastrophically wrong 16-bit IR ops.
self.wide_hi.clear();
self.current_blocks = Vec::new(); self.current_blocks = Vec::new();
// Seed `current_locals` with the state's declared locals so any // Seed `current_locals` with the state's declared locals so any
// `VarDecl` inside the handler body — tracked by // `VarDecl` inside the handler body — tracked by

61
src/ir/tests.rs
View file

@ -740,3 +740,64 @@ fn lower_modulo_emits_mod_op_not_load_imm_zero() {
.collect::<Vec<_>>() .collect::<Vec<_>>()
); );
} }
#[test]
fn wide_hi_does_not_leak_between_functions() {
// Regression test for COMPILER_BUGS.md §6: the IR lowerer's
// `wide_hi` map used to persist across function boundaries
// even though `next_temp` resets to 0 per function. A
// function whose body had no u16 ops would inherit stale
// `(temp_id -> high_byte)` entries from earlier functions
// and emit `CmpEq16` (or other 16-bit ops) where the
// destination temp aliased one of the source temps.
//
// The shape that reproduces it: function A bumps a u16
// global (creating wide entries); function B does u8 ==
// const compares against a u8 global. Pre-fix, function B's
// last few comparisons would lower to `CmpEq16`. Post-fix,
// they all stay narrow.
let ir = lower_ok(
r#"
game "Test" { mapper: NROM }
var clock: u16 = 0
var phase: u8 = 0
var hits: u8 = 0
fun bump_a() { hits += 1 }
fun bump_b() { hits += 2 }
fun bump_c() { hits += 3 }
fun bump_d() { hits += 4 }
on frame {
clock += 1
if phase == 0 { bump_a() }
if phase == 1 { bump_b() }
if phase == 2 { bump_c() }
if phase == 3 { bump_d() }
wait_frame
}
start Main
"#,
);
let frame_fn = ir
.functions
.iter()
.find(|f| f.name.contains("frame"))
.expect("frame handler should exist");
let mut wide_eq_dest_aliases = 0;
for op in frame_fn.blocks.iter().flat_map(|b| &b.ops) {
if let IrOp::CmpEq16 {
dest, b_hi, a_hi, ..
} = op
{
// The dest of a 16-bit compare must never alias one
// of its operand high bytes — that's the symptom of
// bug #6 from war/COMPILER_BUGS.md.
if dest == b_hi || dest == a_hi {
wide_eq_dest_aliases += 1;
}
}
}
assert_eq!(
wide_eq_dest_aliases, 0,
"wide CmpEq16 destination aliased a source operand — wide_hi leaked between functions"
);
}

2
tests/emulator/goldens/war.audio.hash
View file

@ -1 +1 @@
37075c32 132084 4651593c 132084

BIN
tests/emulator/goldens/war.png
View file

Binary file not shown.
Side by side Swipe Overlay

Before

Width:  |  Height:  |  Size: 9.8 KiB

After

Width:  |  Height:  |  Size: 9.9 KiB

Before After
Before After