From 868382cd12d4afaa34b7f8e924f8c17460de800a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 07:45:12 +0000 Subject: [PATCH] ruff_r2il: fix the structurally-empty data_refs column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reachability_probe collected static data references with `addr.space == Const && in_image(addr.offset)`. That filter cannot match on this architecture, so `unreached+ref` printed 0.00% for every image and looked like a measurement. A full linear sweep of LOCODE counts 1418 load/store address operands — 860 Unique, 328 Register, 230 Const — and not one Const operand inside the image. The reason is in the lift, and it took a p-code dump to see. At $1d27, a 3-byte STA $ce00,y becomes: IntZExt { dst: Unique 7680, src: Register 1 } IntAdd { dst: Unique 7936, a: Const 52736, b: Unique 7680 } Store { addr: Unique 7936, val: ... } The data address is the constant operand of the IntAdd that defines the address temp, never the address operand itself. The near-identical shape at $1d2d is a stack push (IntSub of the stack pointer and Const 1, then Store through the temp) and must not count — it carries a constant too, so a pass that grabbed "the constant near the store" would emit $0001 as a data address. src/absref.rs resolves the address through the block: a Const operand directly, or the nearest preceding op that writes the address temp, accepted only when it is an IntAdd carrying a constant. Nearest-preceding is what makes it correct rather than approximate — 6502 blocks reuse temp offsets freely, so taking the first definition would read a stale base. Anything else resolves to nothing; an indirect Register operand needs a value analysis and is not guessed at. One level deep on purpose: a Copy chain would also be resolvable, and is absent because it was not observed on this corpus. Six hermetic tests, both real shapes among them, so neither half can pass by being trivially empty. Measured, replacing 0.00% everywhere: LOCODE refs 9 unreached+ref 0.08% HICODE refs 51 unreached+ref 0.20% The raw reference count is now its own column. One number could not distinguish "the resolver found nothing" from "it found plenty and they were all reached code", and the first of those is what the column silently was. Read together they say something the reached% column cannot: of 60 in-image referenced addresses, 58 are never reached as code — the "data is what is only ever loaded from" half of the hypothesis. mgra_emit consumes the same pass; its referenced-node count goes 4 -> 38 and 10 -> 126. The verdict, the ranges and the emitted graph's call side are all unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DCfrD5y19cvFc4AoyydXYv --- crates/ruff_r2il/examples/mgra_emit.rs | 31 +-- .../ruff_r2il/examples/reachability_probe.rs | 55 ++-- crates/ruff_r2il/src/absref.rs | 237 ++++++++++++++++++ crates/ruff_r2il/src/lib.rs | 1 + 4 files changed, 291 insertions(+), 33 deletions(-) create mode 100644 crates/ruff_r2il/src/absref.rs diff --git a/crates/ruff_r2il/examples/mgra_emit.rs b/crates/ruff_r2il/examples/mgra_emit.rs index 6f3ff71fb67d1..0e7873da7dc1f 100644 --- a/crates/ruff_r2il/examples/mgra_emit.rs +++ b/crates/ruff_r2il/examples/mgra_emit.rs @@ -45,6 +45,7 @@ use std::path::PathBuf; use r2il::{R2ILOp, SpaceId}; use r2sleigh_lift::{Disassembler, build_arch_spec, userop_map_for_arch}; +use ruff_r2il::absref::absolute_refs; use ruff_r2il::mgra::{EdgeKind, GraphBuilder, Palette, encode}; use sleigh_compiler::{SleighCompiler, SleighCompilerOptions}; @@ -145,6 +146,17 @@ fn walk_into( let size = (block.size as usize).max(1); instructions += 1; + // Absolute references, resolved through the block by `ruff_r2il::absref` + // rather than read off the address operand — on 6502 the address of an + // absolute-indexed access is the constant operand of the `IntAdd` that defines + // the address temp. `in_image` is deliberately NOT applied: an out-of-image + // absolute is zero page, the stack page, or a hardware register, and those are + // real references. It would also be meaningless against a temp, whose offset is + // a slot number rather than an address. + for a in absolute_refs(&block.ops) { + g.edge(origin, a, EdgeKind::AbsRef); + } + let mut stop = false; let mut redirect: Option = None; for op in &block.ops { @@ -178,25 +190,6 @@ fn walk_into( R2ILOp::Return { .. } | R2ILOp::BranchInd { .. } | R2ILOp::CallInd { .. } => { stop = true; } - // A constant address operand is a static reference. Measured on these - // images (1777 load/store address operands): 1257 land in `Unique`, - // 354 in `Register`, 166 in `Const` — and NOT ONE `Const` one is inside - // the image. 6502 absolute addressing lifts its address into a computed - // temp, so recovering an in-image data address needs constant - // propagation through the p-code temps, which this walk does not do and - // does not fake. What `Const` DOES catch is the out-of-image absolute: - // zero page, the stack page, a hardware register. Those are real - // references and they are what this arm emits. - // - // `in_image` is deliberately NOT applied. It would also be meaningless - // against a `Unique` operand, whose offset is a temp slot number rather - // than an address — 92 of them compare "inside the image" by pure - // coincidence of numbering. - R2ILOp::Load { addr: a, .. } | R2ILOp::Store { addr: a, .. } - if a.space == SpaceId::Const => - { - g.edge(origin, a.offset, EdgeKind::AbsRef); - } _ => {} } } diff --git a/crates/ruff_r2il/examples/reachability_probe.rs b/crates/ruff_r2il/examples/reachability_probe.rs index 0bfccff9aa8b2..a7e9241a8f7a1 100644 --- a/crates/ruff_r2il/examples/reachability_probe.rs +++ b/crates/ruff_r2il/examples/reachability_probe.rs @@ -8,6 +8,14 @@ //! > **Code is what a call graph REACHES. Data is what is only ever loaded FROM.** //! //! A byte's role is a property of its ADDRESS's position in the graph, not of the byte's value. +//! +//! **Correction, 2026-08-26: the `unreached+ref` column was structurally zero until now.** It +//! filtered `addr.space == Const && in_image(addr)`, which cannot match on this architecture — +//! see [`Walk::data_refs`]. With the reference resolved properly through the block +//! ([`ruff_r2il::absref`]) the column is a real measurement, and it says something: of 60 +//! in-image referenced addresses across both code images, **58 are never reached as code** +//! (LOCODE 7 of 9, HICODE 51 of 51). That is the "data is what is only ever loaded from" half +//! of the hypothesis, which the reached% column alone cannot show. //! So this probe replaces the linear sweep with **recursive descent** from the entry plus every //! discovered call target, and asks whether the reached set discriminates. //! @@ -65,6 +73,7 @@ use std::path::PathBuf; use r2il::{R2ILOp, SpaceId}; use r2sleigh_lift::{Disassembler, build_arch_spec, userop_map_for_arch}; +use ruff_r2il::absref::absolute_refs; use sleigh_compiler::{SleighCompiler, SleighCompilerOptions}; const LIFT_MIN_BYTES: usize = 16; @@ -138,9 +147,17 @@ const SUBJECTS: &[Subject] = &[ struct Walk { /// Bytes covered by an instruction recursive descent actually decoded. reached: BTreeSet, - /// In-image addresses that appear as the address operand of a Load or Store — "referenced AS - /// DATA". A byte both unreached and data-referenced is the strongest data evidence available - /// without running the program. + /// In-image addresses named by a resolved absolute reference — "referenced AS DATA". A byte + /// both unreached and data-referenced is the strongest data evidence available without + /// running the program. + /// + /// Resolved by [`ruff_r2il::absref`], not read off the address operand. The earlier + /// `addr.space == Const && in_image(addr)` filter made this set STRUCTURALLY EMPTY: a full + /// linear sweep of LOCODE finds 1418 load/store address operands — 860 `Unique`, 328 + /// `Register`, 230 `Const` — and not one `Const` operand inside the image, because 6502 + /// absolute-indexed addressing puts the base in the constant operand of the `IntAdd` that + /// defines the address temp. The `unreached+ref` column read 0.00% and looked like a + /// measurement. data_refs: BTreeSet, /// Call targets discovered during the walk (the 6502 stand-in for a symbol table). calls: BTreeSet, @@ -218,6 +235,17 @@ fn walk(disasm: &Disassembler, code: &[u8], load: u64, entries: &BTreeSet) w.reached.insert(addr + b); } + // Static data references, resolved through the block rather than read off the + // address operand. See `ruff_r2il::absref`: on 6502 the address of an + // absolute-indexed access is the constant operand of the `IntAdd` that defines + // the address temp, so the old `addr.space == Const` filter could never match + // and this set was structurally empty. + for a in absolute_refs(&block.ops) { + if in_image(a) { + w.data_refs.insert(a); + } + } + let mut stop = false; let mut redirect: Option = None; for op in &block.ops { @@ -248,13 +276,6 @@ fn walk(disasm: &Disassembler, code: &[u8], load: u64, entries: &BTreeSet) R2ILOp::Return { .. } | R2ILOp::BranchInd { .. } | R2ILOp::CallInd { .. } => { stop = true; } - // A constant address operand is a static data reference. A register operand - // is not resolvable here and is deliberately not guessed at. - R2ILOp::Load { addr: a, .. } | R2ILOp::Store { addr: a, .. } - if a.space == SpaceId::Const && in_image(a.offset) => - { - w.data_refs.insert(a.offset); - } _ => {} } } @@ -322,8 +343,8 @@ fn main() -> Result<(), Box> { println!("\n=== recursive descent from entry + discovered call targets ==="); println!( - "{:<12} {:>6} {:>8} {:>10} {:>12} {:>8} {:>10}", - "file", "truth", "bytes", "reached%", "unreached+ref", "seeds", "instrs" + "{:<12} {:>6} {:>8} {:>10} {:>7} {:>12} {:>8} {:>10}", + "file", "truth", "bytes", "reached%", "refs", "unreached+ref", "seeds", "instrs" ); for s in SUBJECTS { @@ -343,8 +364,13 @@ fn main() -> Result<(), Box> { let w = walk(&disasm, code, load, &seeds); let total = code.len() as f64; let reached_pct = 100.0 * w.reached.len() as f64 / total; - // Bytes never reached as code AND referenced by a constant-address load/store: the + // Bytes never reached as code AND named by a resolved absolute reference: the // strongest static evidence of a data role. + // + // Reported next to the RAW in-image reference count on purpose. One number cannot + // distinguish "the resolver found nothing" from "it found plenty and they were all + // reached code" — and the first of those is what this column silently was before + // `absref` landed. let unreached_ref = w .data_refs .iter() @@ -352,7 +378,7 @@ fn main() -> Result<(), Box> { .count(); let unreached_ref_pct = 100.0 * unreached_ref as f64 / total; println!( - "{:<12} {:>6} {:>8} {:>9.1}% {:>11.2}% {:>8} {:>10}", + "{:<12} {:>6} {:>8} {:>9.1}% {:>7} {:>11.2}% {:>8} {:>10}", s.file, if s.truth == Truth::Code { "CODE" @@ -361,6 +387,7 @@ fn main() -> Result<(), Box> { }, code.len(), reached_pct, + w.data_refs.len(), unreached_ref_pct, seeds.len(), w.instructions diff --git a/crates/ruff_r2il/src/absref.rs b/crates/ruff_r2il/src/absref.rs new file mode 100644 index 0000000000000..62d69d447a8b1 --- /dev/null +++ b/crates/ruff_r2il/src/absref.rs @@ -0,0 +1,237 @@ +//! Recovering the absolute address a `Load`/`Store` actually names. +//! +//! # The defect this exists to fix +//! +//! `reachability_probe` collected static data references with the filter +//! `addr.space == SpaceId::Const && in_image(addr.offset)`, and its `unreached+ref` column +//! was therefore **structurally zero** — not measured-zero. A full linear sweep of +//! `LOCODE.unprot.bin` counts 1418 `Load`/`Store` address operands: 860 in `Unique`, 328 in +//! `Register`, 230 in `Const`, and **not one `Const` operand inside the image**. A column +//! that cannot be non-zero is not a measurement, and the fix is not to widen the filter +//! until something matches. +//! +//! # What the lifted 6502 actually looks like +//! +//! Absolute-indexed addressing does not put the address in the address operand. Measured at +//! `$1d27` (a 3-byte `STA $ce00,y`): +//! +//! ```text +//! IntZExt { dst: Unique 7680, src: Register 1 } ; widen the index +//! IntAdd { dst: Unique 7936, a: Const 52736, b: Unique 7680 } ; base + index +//! Copy { dst: Unique 8064, src: Register 0 } +//! Store { addr: Unique 7936, val: Unique 8064 } ; <- address is a TEMP +//! ``` +//! +//! The data address is `Const 52736` = `$ce00`, the constant operand of the `IntAdd` that +//! defines the address temp. It is never the address operand itself. +//! +//! The near-identical shape at `$1d2d` is the one that must NOT count: +//! +//! ```text +//! IntSub { dst: Unique 19584, a: Register 34, b: Const 1 } ; stack pointer - 1 +//! Store { addr: Unique 19584, val: Const 7472 } ; a stack push +//! ``` +//! +//! Same `Store`-through-a-temp silhouette, and its constant (`1`) is an offset from a +//! register rather than a base address. Emitting `$0001` as a data reference here would be +//! worse than emitting nothing, because it would look like a measurement. +//! +//! # The rule, and its deliberate limit +//! +//! For each `Load`/`Store`, resolve its address operand: +//! +//! - `Const` — the address, directly. (Zero page and the hardware vectors arrive this way.) +//! - `Unique` — find the **nearest preceding op that writes that temp**, via +//! [`R2ILOp::output`], and accept only an `IntAdd` carrying a constant. Nearest-preceding +//! is what makes this correct rather than approximate: 6502 blocks reuse temp offsets +//! freely (offset 1920 is reassigned nine times in one BRK block), so a scan that took +//! the *first* definition would read a stale base. +//! - anything else — no address. A `Register` operand is indirect addressing and is not +//! resolvable without a value analysis; guessing is not on offer. +//! +//! This is one level deep on purpose. A `Copy` chain into the address temp would also be +//! resolvable and is cheap to add — it is absent because it was not observed on this +//! corpus, and untested code that looks like it works is worse than an honest gap. + +use std::collections::BTreeSet; + +use r2il::{R2ILOp, SpaceId, Varnode}; + +/// The constant operand of an `IntAdd`, if it has exactly one — or the sum if both are +/// constant. +fn add_base(a: &Varnode, b: &Varnode) -> Option { + match (a.space, b.space) { + // A fully constant address: rare, but it is still an address. + (SpaceId::Const, SpaceId::Const) => Some(a.offset.wrapping_add(b.offset)), + (SpaceId::Const, _) => Some(a.offset), + (_, SpaceId::Const) => Some(b.offset), + _ => None, + } +} + +/// Resolve one address operand against the ops that precede it in the same block. +/// +/// `at` is the index of the `Load`/`Store` itself; only ops before it are consulted, so a +/// later redefinition of the same temp cannot leak backwards. +#[must_use] +pub fn resolve_address(ops: &[R2ILOp], at: usize, addr: &Varnode) -> Option { + match addr.space { + SpaceId::Const => Some(addr.offset), + SpaceId::Unique => { + // Nearest preceding writer of this temp — see the module docs on reuse. + let defining = ops[..at.min(ops.len())].iter().rev().find(|op| { + op.output() + .is_some_and(|o| o.space == SpaceId::Unique && o.offset == addr.offset) + })?; + match defining { + R2ILOp::IntAdd { a, b, .. } => add_base(a, b), + // Every other definer — IntSub from a register (the stack), a shift, a + // load — leaves an address this pass cannot claim to know. + _ => None, + } + } + _ => None, + } +} + +/// Every distinct absolute address the block's loads and stores name. +/// +/// Callers decide what "in image" means; this returns addresses, not judgements. +#[must_use] +pub fn absolute_refs(ops: &[R2ILOp]) -> BTreeSet { + let mut out = BTreeSet::new(); + for (i, op) in ops.iter().enumerate() { + let addr = match op { + R2ILOp::Load { addr, .. } | R2ILOp::Store { addr, .. } => addr, + _ => continue, + }; + if let Some(a) = resolve_address(ops, i, addr) { + out.insert(a); + } + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + fn vn(space: SpaceId, offset: u64, size: u32) -> Varnode { + Varnode { + space, + offset, + size, + meta: None, + } + } + + /// The measured `STA $ce00,y` at `$1d27`, op for op. + fn indexed_absolute_store() -> Vec { + vec![ + R2ILOp::IntZExt { + dst: vn(SpaceId::Unique, 7680, 2), + src: vn(SpaceId::Register, 1, 1), + }, + R2ILOp::IntAdd { + dst: vn(SpaceId::Unique, 7936, 2), + a: vn(SpaceId::Const, 0xce00, 2), + b: vn(SpaceId::Unique, 7680, 2), + }, + R2ILOp::Copy { + dst: vn(SpaceId::Unique, 8064, 1), + src: vn(SpaceId::Register, 0, 1), + }, + R2ILOp::Store { + space: SpaceId::Ram, + addr: vn(SpaceId::Unique, 7936, 2), + val: vn(SpaceId::Unique, 8064, 1), + }, + ] + } + + /// The measured stack push at `$1d2d` — the same silhouette, and not a data reference. + fn stack_push() -> Vec { + vec![ + R2ILOp::IntSub { + dst: vn(SpaceId::Unique, 19584, 2), + a: vn(SpaceId::Register, 34, 2), + b: vn(SpaceId::Const, 1, 2), + }, + R2ILOp::Store { + space: SpaceId::Ram, + addr: vn(SpaceId::Unique, 19584, 2), + val: vn(SpaceId::Const, 7472, 2), + }, + ] + } + + #[test] + fn an_indexed_absolute_store_resolves_to_its_base() { + let refs = absolute_refs(&indexed_absolute_store()); + assert_eq!(refs, BTreeSet::from([0xce00])); + } + + #[test] + fn a_stack_push_resolves_to_nothing() { + // The silence half, and it is the one that matters: this block DOES carry a + // constant (1), and a pass that reached for "the constant near the store" would + // emit $0001 as a data address. Both halves run against real measured shapes, so + // neither can pass by being trivially empty. + assert!( + absolute_refs(&stack_push()).is_empty(), + "an offset from the stack pointer is not a data address" + ); + assert!(!absolute_refs(&indexed_absolute_store()).is_empty()); + } + + #[test] + fn a_direct_constant_address_resolves() { + // Zero page and the hardware vectors arrive this way; $fffe is the IRQ vector, + // observed verbatim in a real BRK block. + let ops = vec![R2ILOp::Load { + dst: vn(SpaceId::Unique, 16512, 2), + space: SpaceId::Ram, + addr: vn(SpaceId::Const, 0xfffe, 2), + }]; + assert_eq!(absolute_refs(&ops), BTreeSet::from([0xfffe])); + } + + #[test] + fn a_redefined_temp_reads_its_nearest_definition_not_a_stale_one() { + // 6502 blocks reuse temp offsets freely. If the scan took the FIRST definition + // instead of the nearest preceding one, this would resolve to $ce00 — a real + // address, from the wrong instruction. + let mut ops = indexed_absolute_store(); + ops.insert( + 3, + R2ILOp::IntAdd { + dst: vn(SpaceId::Unique, 7936, 2), + a: vn(SpaceId::Const, 0x1234, 2), + b: vn(SpaceId::Unique, 7680, 2), + }, + ); + assert_eq!(absolute_refs(&ops), BTreeSet::from([0x1234])); + } + + #[test] + fn an_undefined_temp_resolves_to_nothing() { + let ops = vec![R2ILOp::Store { + space: SpaceId::Ram, + addr: vn(SpaceId::Unique, 999, 2), + val: vn(SpaceId::Const, 0, 1), + }]; + assert!(absolute_refs(&ops).is_empty()); + } + + #[test] + fn an_indirect_register_address_is_not_guessed_at() { + // Indirect addressing needs a value analysis. Returning nothing is the honest + // answer; returning the register number would be nonsense that looks like data. + let ops = vec![R2ILOp::Load { + dst: vn(SpaceId::Register, 0, 1), + space: SpaceId::Ram, + addr: vn(SpaceId::Register, 34, 2), + }]; + assert!(absolute_refs(&ops).is_empty()); + } +} diff --git a/crates/ruff_r2il/src/lib.rs b/crates/ruff_r2il/src/lib.rs index 6b6491cc62d82..a8f5e928e38a0 100644 --- a/crates/ruff_r2il/src/lib.rs +++ b/crates/ruff_r2il/src/lib.rs @@ -63,6 +63,7 @@ //! reviewer who finds a signature here contradicting the actual upstream source should treat the //! source as authoritative and file a correction — never silently improvise a replacement shape. +pub mod absref; pub mod behavior; pub mod convention; pub mod facet;