Repository navigation
ruff_r2il: fix the structurally-empty data_refs column - #110
Conversation
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DCfrD5y19cvFc4AoyydXYv
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a1bb3a3a-00c6-4080-a100-ff78986b0a8c) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 868382cd12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (SpaceId::Const, _) => Some(a.offset), | ||
| (_, SpaceId::Const) => Some(b.offset), |
There was a problem hiding this comment.
When an address temporary is defined by IntAdd(Register, Const)—for example, a 6502 stack pull that increments the stack pointer—these arms return the constant as an absolute base. This emits a false $0001-style AbsRef in mgra_emit, and a sufficiently large register offset can also pollute reachability_probe's in-image measurement; the existing stack test only covers the IntSub direction. Restrict the accepted nonconstant operand to the indexed-address shape and add a focused register-plus-constant falsifier.
AGENTS.md reference: AGENTS.md:L169-L170
Useful? React with 👍 / 👎.
| 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) | ||
| })?; |
There was a problem hiding this comment.
When a block reuses a Unique offset at a different width, this search can select that unrelated writer instead of the definition of addr, either returning its base or prematurely returning None. The crate explicitly documents varnode identity as (space, offset, size) in facet.rs, so the predicate must compare size as well as space and offset before treating an output as the address definition.
Useful? React with 👍 / 👎.
| // A fully constant address: rare, but it is still an address. | ||
| (SpaceId::Const, SpaceId::Const) => Some(a.offset.wrapping_add(b.offset)), |
There was a problem hiding this comment.
For the explicitly supported fully constant IntAdd case, arithmetic must wrap at the destination varnode's width, not at 64 bits. For example, a 2-byte p-code addition of 0xffff + 1 names address 0x0000, while this returns 0x10000, causing the reference to be assigned to the wrong node or filtered out as out-of-image. Pass the result width into this calculation and mask or truncate accordingly.
Useful? React with 👍 / 👎.
reachability_probecollected static data references withaddr.space == Const && in_image(addr.offset). That filter cannot match on this architecture, sounreached+refprinted0.00%for every image and looked like a measurement.A full linear sweep of
LOCODE.unprot.bincounts 1418 load/store address operands:UniqueRegisterConstWhy — it took a p-code dump to see
At
$1d27, a 3-byteSTA $ce00,ylifts to:The data address is
Const 52736=$ce00— the constant operand of theIntAddthat defines the address temp, never the address operand itself.The near-identical shape at
$1d2dis the one that must not count:Same silhouette, and it carries a constant too — so a pass that grabbed "the constant near the store" would emit
$0001as a data address. Worse than emitting nothing, because it would look like a measurement.The fix
src/absref.rsresolves the address through the block:Constoperand → the address directly (zero page, hardware vectors).Unique→ the nearest preceding op that writes that temp (viaR2ILOp::output()), accepted only when it's anIntAddcarrying a constant.Registeroperand needs a value analysis and is not guessed at.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 taking the first definition would read a stale base. Pinned by its own falsifier.
One level deep on purpose. A
Copychain into the address temp would also be resolvable and is cheap to add; it's absent because it wasn't observed on this corpus, and untested code that looks like it works is worse than an honest gap.Six hermetic tests, both real measured shapes among them, so neither half can pass by being trivially empty.
Measured
LOCODE.unprot.binHICODE.unprot.binThe 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
reached%alone cannot: of 60 in-image referenced addresses, 58 are never reached as code (LOCODE 7 of 9, HICODE 51 of 51). That's the "data is what is only ever loaded from" half of the hypothesis, which had no evidence behind it until now.mgra_emitconsumes the same pass — referenced nodes go 4 → 38 and 10 → 126. The verdict, the CODE/DATA ranges, and the emitted graph's call side are all unchanged.Gates
cargo clippy --features lift --all-targets -- -D warningsclean ·cargo fmt --checkclean ·cargo test -p ruff_r2il --lib52/52 ·prekon every changed file · both probes re-run end to end.Generated by Claude Code