From 6696d790bbceac52f44566f604fad088fa643377 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Apr 2026 17:46:06 +0000 Subject: [PATCH] codegen+ir: code-review followups (UTF-8 safety, leaf exhaustiveness, tests) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ups from a fresh review of the perf milestone: 1. **UTF-8 safety in `substitute_asm_vars` and `substitute_inline_const_params`.** Both walked the asm body byte-by-byte and emitted each non-substituted byte via `out.push(bytes[i] as char)` — a Latin-1 reinterpretation that mangles non-ASCII characters in inline-asm comments. The brace- level scan stays byte-based (braces can't appear inside a UTF-8 continuation), but the verbatim copy now uses `out.push_str(&body[i..i + ch_len])` with `ch_len` derived from the lead byte. Pre-existing latent bug in `substitute_asm_vars`, freshly introduced in `substitute_inline_const_params` — fixed in both, with a shared lead-byte length helper. 2. **`function_is_leaf` is now exhaustive on `IrOp`.** The match used to be selective: `Call`/`Mul`/`Div`/`Mod`/`Transition`/ `InlineAsm` were checked, everything else fell through with `_ => {}`. A new variant added later that secretly emitted a JSR (e.g. a future `Mul16` calling `__multiply16`) would have silently broken any leaf function that touched it. Listed every current variant explicitly so the compiler errors at the match arm if a new variant ships, and added a `function_is_leaf_detects_jsr_emitting_ops` test that walks the known JSR-emitting constructs (Call, *, /, %, asm with JSR token) and asserts each disqualifies leafness. 3. **Cleanups.** `gen_block` now binds the fused-cmp dest temp inside the original tuple instead of re-matching `block.ops.last().unwrap()` to retire it. New `inline_fun_with_asm_param_cascades_through_nested_inline` test exercises the eval_const → const_args_stack path that lets the inner of two nested inline funs see its outer's parameter as the constant the top-level call passed. Defensive comment on `body_has_inline_asm` explaining why it deliberately doesn't recurse (relies on `is_splicable_void_stmt`'s no-control-flow guarantee). ROMs and goldens unchanged — all the changes are non-observable through the existing example surface. Verified: cargo test/clippy/fmt clean on rustc 1.95.0; emulator harness 34/34; reproducibility diff clean; demo gifs byte-match fresh captures. https://claude.ai/code/session_01FRmSBruVWCufm3LsUVMs8v --- src/codegen/ir_codegen.rs | 210 ++++++++++++++++++++++++++++++++------ src/ir/lowering.rs | 39 ++++++- src/ir/tests.rs | 57 +++++++++++ 3 files changed, 275 insertions(+), 31 deletions(-) diff --git a/src/codegen/ir_codegen.rs b/src/codegen/ir_codegen.rs index 85fa954..61b9e45 100644 --- a/src/codegen/ir_codegen.rs +++ b/src/codegen/ir_codegen.rs @@ -995,7 +995,7 @@ impl<'a> IrCodeGen<'a> { IrOp::CmpGtEq(..) => CmpKind::GtEq, _ => unreachable!(), }; - Some((*a, *b, kind, true_lbl.clone(), false_lbl.clone())) + Some((*d, *a, *b, kind, true_lbl.clone(), false_lbl.clone())) } _ => None, }); @@ -1017,12 +1017,12 @@ impl<'a> IrCodeGen<'a> { self.retire_op_sources(op); } - if let Some((a, b, kind, true_lbl, false_lbl)) = fuse_cmp_branch { + if let Some((d, a, b, kind, true_lbl, false_lbl)) = fuse_cmp_branch { // Emit the fused compare + branch *first*. Retiring // a/b before the emit would free their slots while // the values are still live — `load_temp(a)` would - // then re-allocate `a` to whatever slot the free - // list pops next, which contains stale data. + // then re-allocate `a` to whatever stale slot the + // free list pops next. self.gen_cmp_branch(a, b, kind, &true_lbl, &false_lbl); // Now that the CMP has read both operands, drop their // use counts the same way `retire_op_sources` would @@ -1031,15 +1031,7 @@ impl<'a> IrCodeGen<'a> { // retire it too so its slot returns to the free list. self.dec_use(a); self.dec_use(b); - if let IrOp::CmpEq(d, ..) - | IrOp::CmpNe(d, ..) - | IrOp::CmpLt(d, ..) - | IrOp::CmpGt(d, ..) - | IrOp::CmpLtEq(d, ..) - | IrOp::CmpGtEq(d, ..) = block.ops.last().unwrap() - { - self.dec_use(*d); - } + self.dec_use(d); return; } @@ -2367,32 +2359,90 @@ fn scope_prefix_for_fn(name: &str) -> String { /// body. Leaf functions skip the parameter-spill prologue and read /// their args straight out of the transport slots `$04..$07`. /// -/// Conservatively false for any op that emits a JSR (Call, Mul, Div, -/// Mod, Transition) or for inline-asm bodies containing a `JSR` -/// token — the analyzer has no way to look inside hand-written asm, -/// so anything that mentions `JSR` is treated as a non-leaf. +/// The match below is exhaustive on `IrOp` so adding a new variant +/// that secretly emits a `JSR` (e.g. a future `Mul16` calling a +/// `__multiply16` runtime helper) becomes a compile error here +/// rather than a silent leaf-detection bug. The current JSR-emitting +/// ops are: `Call`, `Mul`, `Div`, `Mod`, `Transition`, plus +/// `InlineAsm` bodies that mention `JSR` as a token. fn function_is_leaf(func: &IrFunction) -> bool { for block in &func.blocks { for op in &block.ops { - match op { + // Returning `false` from any arm marks the function as + // non-leaf. Arms that fall through are explicitly + // listed so a new variant won't slip past. + #[allow(clippy::match_same_arms)] + let is_jsr_emitting = match op { IrOp::Call(..) | IrOp::Mul(..) | IrOp::Div(..) | IrOp::Mod(..) - | IrOp::Transition(..) => return false, + | IrOp::Transition(..) => true, IrOp::InlineAsm(body) => { // Strip the raw-asm magic prefix if present so - // we don't catch the marker characters. + // we don't false-match the marker characters. + // Tokenise on non-alphanumeric so `JSR` only + // matches as a whole word — `MJSR` or `JSRX` + // don't trip the check. let scan = body.strip_prefix(crate::ir::RAW_ASM_PREFIX).unwrap_or(body); let upper = scan.to_ascii_uppercase(); - let contains_jsr = upper + upper .split(|c: char| !c.is_ascii_alphanumeric()) - .any(|tok| tok == "JSR"); - if contains_jsr { - return false; - } + .any(|tok| tok == "JSR") } - _ => {} + // None of the following ops emit a JSR. Listed + // explicitly so the compiler errors on a new + // variant — see fn doc comment. + IrOp::LoadImm(..) + | IrOp::LoadVar(..) + | IrOp::StoreVar(..) + | IrOp::Add(..) + | IrOp::Sub(..) + | IrOp::And(..) + | IrOp::Or(..) + | IrOp::Xor(..) + | IrOp::ShiftLeft(..) + | IrOp::ShiftRight(..) + | IrOp::ShiftLeftVar(..) + | IrOp::ShiftRightVar(..) + | IrOp::Negate(..) + | IrOp::Complement(..) + | IrOp::CmpEq(..) + | IrOp::CmpNe(..) + | IrOp::CmpLt(..) + | IrOp::CmpGt(..) + | IrOp::CmpLtEq(..) + | IrOp::CmpGtEq(..) + | IrOp::ArrayLoad(..) + | IrOp::ArrayStore(..) + | IrOp::DrawSprite { .. } + | IrOp::ReadInput(..) + | IrOp::WaitFrame + | IrOp::CycleSprites + | IrOp::Scroll(..) + | IrOp::DebugLog(..) + | IrOp::DebugAssert(..) + | IrOp::Poke(..) + | IrOp::Peek(..) + | IrOp::LoadVarHi(..) + | IrOp::StoreVarHi(..) + | IrOp::Add16 { .. } + | IrOp::Sub16 { .. } + | IrOp::CmpEq16 { .. } + | IrOp::CmpNe16 { .. } + | IrOp::CmpLt16 { .. } + | IrOp::CmpGt16 { .. } + | IrOp::CmpLtEq16 { .. } + | IrOp::CmpGtEq16 { .. } + | IrOp::SetPalette(..) + | IrOp::LoadBackground(..) + | IrOp::PlaySfx(..) + | IrOp::StartMusic(..) + | IrOp::StopMusic + | IrOp::SourceLoc(..) => false, + }; + if is_jsr_emitting { + return false; } } } @@ -2413,7 +2463,9 @@ fn substitute_asm_vars Option>(body: &str, resolver: F) -> S let mut i = 0; while i < bytes.len() { if bytes[i] == b'{' { - // Find the closing `}`. + // Find the closing `}`. Brace bytes can't appear inside + // a UTF-8 continuation, so the byte-level search is safe + // even if the body contains non-ASCII comments. if let Some(end) = bytes[i + 1..].iter().position(|&b| b == b'}') { let name_start = i + 1; let name_end = i + 1 + end; @@ -2439,12 +2491,37 @@ fn substitute_asm_vars Option>(body: &str, resolver: F) -> S } } } - out.push(bytes[i] as char); - i += 1; + // Copy the next char through verbatim, preserving any + // multi-byte UTF-8 sequence (typically inside a comment). + // Casting `bytes[i] as char` would Latin-1-reinterpret each + // byte and corrupt non-ASCII characters; copy the whole + // char's slice in one go instead. + let ch_len = utf8_char_len(bytes[i]); + out.push_str(&body[i..i + ch_len]); + i += ch_len; } out } +/// Length in bytes of the UTF-8 character whose lead byte is +/// `lead`. UTF-8 lead bytes encode the length in the count of +/// leading 1-bits: `0xxx_xxxx` = 1, `110x_xxxx` = 2, `1110_xxxx` +/// = 3, `1111_0xxx` = 4. Continuation bytes (`10xx_xxxx`) shouldn't +/// appear at a char boundary; if one does we return 1 so iteration +/// still makes progress. +fn utf8_char_len(lead: u8) -> usize { + match lead { + 0x00..=0x7F => 1, + 0xC0..=0xDF => 2, + 0xE0..=0xEF => 3, + 0xF0..=0xFF => 4, + // Continuation byte at a char boundary — defensively + // advance one byte so we don't loop forever on malformed + // input. + _ => 1, + } +} + /// True if the given IR function contains at least one /// `DrawSprite` op. Used by the frame-handler OAM clear to skip /// the clear loop when a handler doesn't actually draw anything. @@ -4253,3 +4330,78 @@ fn gen_function_prologue_spills_params_to_local_ram() { effect" ); } + +#[test] +fn function_is_leaf_detects_jsr_emitting_ops() { + // `function_is_leaf` decides whether a fun's parameters can + // live in the `$04..$07` transport slots for the lifetime of + // its body — true only if nothing inside the body JSRs and + // re-clobbers them. Each construct below indirectly emits a + // JSR and therefore disqualifies leaf status: + // + // - `Statement::Call` → `IrOp::Call` → JSR + // - `*` (non-power-of-2) → `IrOp::Mul` → JSR __multiply + // - `/` (non-power-of-2) → `IrOp::Div` → JSR __divide + // - `%` (non-power-of-2) → `IrOp::Mod` → JSR __divide + // - `asm { ... JSR ... }` (the analyzer can't see inside + // hand-written asm, so any "JSR" token disqualifies) + // + // `Transition` also emits a JSR but lives in state handlers + // rather than user-callable funs; the existing `*_enter` + // dispatcher tests cover that path. The control case at the + // bottom — a function that only does loads/adds/stores — is + // the one shape that should be a leaf. + use crate::parser; + let cases: &[(&str, &str, bool)] = &[ + // (name, body source, expect_leaf) + ( + "call", + "fun helper(a: u8) -> u8 { return a } \ + fun f(x: u8) { var y: u8 = helper(x) }", + false, + ), + ("mul", "var c: u8 = 0 fun f(x: u8) { c = x * x }", false), + ("div", "var c: u8 = 0 fun f(x: u8) { c = x / x }", false), + ("mod", "var c: u8 = 0 fun f(x: u8) { c = x % x }", false), + ( + "asm-with-JSR", + "fun helper() {} fun f(x: u8) { asm { JSR helper } }", + false, + ), + ( + "asm-without-JSR", + "fun f(x: u8) { asm { LDA $00 STA $01 } }", + true, + ), + ("plain", "var c: u8 = 0 fun f(x: u8) { c = x + 1 }", true), + ]; + for (label, body, expect_leaf) in cases { + let src = format!( + r#" + game "T" {{ mapper: NROM }} + {body} + on frame {{ f(1) }} + start Main + "# + ); + let (prog, _) = parser::parse(&src); + let prog = prog.unwrap_or_else(|| panic!("[{label}] parse failed")); + let analysis = crate::analyzer::analyze(&prog); + assert!( + analysis.diagnostics.iter().all(|d| !d.is_error()), + "[{label}] unexpected analysis errors: {:?}", + analysis.diagnostics + ); + let ir = crate::ir::lower(&prog, &analysis); + let f = ir + .functions + .iter() + .find(|f| f.name == "f") + .unwrap_or_else(|| panic!("[{label}] no fn `f` in IR")); + assert_eq!( + function_is_leaf(f), + *expect_leaf, + "[{label}] leaf detection mismatch" + ); + } +} diff --git a/src/ir/lowering.rs b/src/ir/lowering.rs index cbaf159..48f8d46 100644 --- a/src/ir/lowering.rs +++ b/src/ir/lowering.rs @@ -1946,6 +1946,15 @@ pub fn can_inline_fun(return_type: Option<&NesType>, body: &Block) -> bool { /// `inline fun` splicer to decide whether all-constant arguments are /// required (asm `{param}` substitution can only synthesise a /// `#$` immediate at expansion time, not a runtime address). +/// +/// We deliberately don't recurse into nested statements: an inline +/// fun's body is gated by [`is_splicable_void_stmt`], which only +/// admits flat sequences of `Assign`/`Call`/`Draw`/`InlineAsm`/etc. +/// — never `If`/`While`/`Loop`/`For` — so any inline-asm statement +/// that's reachable shows up at the top level. Single-return- +/// expression bodies can't contain asm at all (asm is a statement, +/// not an expression). If `is_splicable_void_stmt` ever loosens to +/// admit nested control flow, this check needs to follow. fn body_has_inline_asm(body: &InlineBody) -> bool { match body { InlineBody::Expression(_) => false, @@ -1962,6 +1971,12 @@ fn stmt_contains_inline_asm(stmt: &Statement) -> bool { /// by `try_inline_call_stmt`. Names not in the map are left alone /// so the codegen's `substitute_asm_vars` can still resolve them /// (e.g. `{wk}` for a global array's address). +/// +/// Walks `body` as Unicode chars to preserve any non-ASCII content +/// (typically comments) verbatim. The `{` / `}` braces and the +/// identifier characters inside them are all ASCII, so the +/// byte-level brace search is safe — it can't mis-fire on a UTF-8 +/// continuation byte. fn substitute_inline_const_params(body: &str, consts: &HashMap) -> String { let mut out = String::with_capacity(body.len()); let bytes = body.as_bytes(); @@ -1988,12 +2003,32 @@ fn substitute_inline_const_params(body: &str, consts: &HashMap) -> S } } } - out.push(bytes[i] as char); - i += 1; + // Pass the next char through verbatim, copying all of its + // UTF-8 bytes in one go so multi-byte characters in + // comments survive intact. + let ch_len = utf8_char_len(bytes[i]); + out.push_str(&body[i..i + ch_len]); + i += ch_len; } out } +/// Length in bytes of the UTF-8 character whose lead byte is +/// `lead`. UTF-8 lead bytes encode the length in the count of +/// leading 1-bits: `0xxx_xxxx` = 1, `110x_xxxx` = 2, `1110_xxxx` +/// = 3, `1111_0xxx` = 4. Continuation bytes (`10xx_xxxx`) shouldn't +/// appear at a char boundary; if one does we return 1 so iteration +/// still makes progress on malformed input. +fn utf8_char_len(lead: u8) -> usize { + match lead { + 0x00..=0x7F => 1, + 0xC0..=0xDF => 2, + 0xE0..=0xEF => 3, + 0xF0..=0xFF => 4, + _ => 1, + } +} + fn type_size(t: &NesType) -> u16 { match t { NesType::U8 | NesType::I8 | NesType::Bool => 1, diff --git a/src/ir/tests.rs b/src/ir/tests.rs index baf9373..ddad7ff 100644 --- a/src/ir/tests.rs +++ b/src/ir/tests.rs @@ -1014,6 +1014,63 @@ fn inline_fun_with_asm_falls_back_for_runtime_arg() { ); } +#[test] +fn inline_fun_with_asm_param_cascades_through_nested_inline() { + // Outer inline fun calls inner inline fun (with asm) using + // its *own* parameter as the inner's argument. The outer's + // parameter is bound to a compile-time constant at the + // top-level call site, and that constness has to flow + // through `eval_const` so the inner — which has an asm body + // — sees its arg as constant too. + // + // Without the `inline_const_args_stack` lookup in + // `eval_const`, the inner would treat the outer's param as + // runtime and refuse to inline, falling back to a Call. + let ir = lower_ok( + r#" + game "Test" { mapper: NROM } + var sink: u8 = 0 + inline fun inner(value: u8) { + asm { + LDA {value} + STA {sink} + } + } + inline fun outer(p: u8) { inner(p) } + on frame { outer(123) } + start Main + "#, + ); + let frame_fn = ir + .functions + .iter() + .find(|f| f.name.contains("frame")) + .expect("frame handler should exist"); + let any_call = frame_fn + .blocks + .iter() + .flat_map(|b| &b.ops) + .any(|op| matches!(op, IrOp::Call(_, name, _) if name == "outer" || name == "inner")); + assert!( + !any_call, + "both inline funs should expand; no Call ops should remain" + ); + let asm_body = frame_fn + .blocks + .iter() + .flat_map(|b| &b.ops) + .find_map(|op| match op { + IrOp::InlineAsm(body) => Some(body.clone()), + _ => None, + }) + .expect("inlined asm block should be present"); + // 123 = $7B + assert!( + asm_body.contains("#$7B"), + "asm body should contain `#$7B` from the cascaded constant; got: {asm_body}" + ); +} + #[test] fn inline_fun_nested_inlines_substitute_correctly() { // Two inline functions where the outer calls the inner