Repository navigation
jit: #73 jitcode-blackhole — backxlat consumer-family twin cutover + midbody py forward-carry (P1) - #745
Conversation
WalkthroughJIT trace resume paths now prefer forward Python-PC twins, carry callee Python PCs through midbody abort payloads, and provide environment-gated audits for coordinate consistency and last-instruction inversion. ChangesPython-PC Resume Mapping
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 1bee8cf). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
972f566 to
b9071d5
Compare
resolve_parent_resume_py_pc's Backxlat arm inverted the parent jitcode_pc via backxlat_py_pc. Read the forward_py_pc twin (a codewriter-built, trivia- normalized twin of backxlat_py_pc's result, proven equal corpus-wide in Slice 3') instead; the inversion survives only for the empty-twin class. Byte-identical; dynasm/cranelift 293/293. Assisted-by: Claude
Gated telemetry at the three blackhole exception-delivery sites comparing the production inversion python_pc_for_jitcode_pc_public(last_opcode_position) against the forward candidate frame.last_instr + 1. Off by default (zero production effect). Measured over the synth corpus: ~99.97% divergence (75689/75712 hits), deltas up to ~100 bytecodes — the vable last_instr is stale by many opcodes at the raising op, so the inversion is load-bearing for traceback fidelity and cannot be replaced without reconciling the last_instr convention. Assisted-by: Claude
…_or_backxlat (audit phase) Introduce state::forward_py_pc_or_backxlat(jitcode_index, pc_word) beside backxlat_py_pc. It returns the backxlat inversion and, under the PYRE_M73_BACKXLAT_TWIN_AUDIT gate, asserts the codewriter-built forward_py_pc twin equals that inversion for every non-negative resume word. Negative sentinel words (NO_JITCODE_PC -1 / tagged branch-orgpc <= -2) and the empty-twin class (skeleton/fixture jitcodes) never consult the twin, so the negative word is delegated straight to backxlat_py_pc. Convert the 12 backxlat_py_pc consumer call sites (state.rs x2, trace.rs x9, jitcode_dispatch/mod.rs x1) to the helper. The helper still returns backxlat, so behaviour is unchanged and byte-identical; only the gated audit branch is added. backxlat_py_pc now has two remaining callers: this helper and the diag.rs Backxlat fallback. check.py dynasm 293/293 + cranelift 293/293 byte-identical (audit off). PYRE_M73_BACKXLAT_TWIN_AUDIT sweep over 280 synth tests x both backends: 0 divergence; the walker entry_py_pc arm alone exercises the audit on nearly every JIT trace entry. Assisted-by: Claude
…r phase) Flip forward_py_pc_or_backxlat to return the codewriter-built forward_py_pc twin for non-negative resume words, keeping backxlat_py_pc only for the empty-twin class (skeleton/fixture jitcodes) and negative sentinel words. The PYRE_M73_BACKXLAT_TWIN_AUDIT gate now asserts the twin equals backxlat before returning it. The twin is a by-construction mirror of backxlat's block-head/floor resolution plus its skip_python_trivia_forward normalization; the preceding audit phase certified it equal across 280 synth tests x both backends with zero divergence. check.py dynasm 293/293 + cranelift 293/293 byte-identical. Assisted-by: Claude
…P1) Add MidBodyPayload.callee_py_pc and stamp it at capture (inline_call.rs) from the callee_py_pc already computed for the abort anchor validation, so the midbody flush reads the resume py as a scalar (trace.rs resolve_midbody_flush_words) instead of re-inverting abort_jitcode_pc through the block_head_py_by_jit_pc + py_floor_by_jit_pc tables. This removes one runtime reader of those tables on the midbody callee-rebuild resume path. Byte-parity rests on encode/resume input identity: both sides compute python_pc_for_jitcode_pc over the same append-only-immutable jitcode metadata (round-tripped via callee_jitcode_index) at the same pc (abort_jitcode_pc == abort_pc); the function reads only metadata, never code_ptr. The PYRE_M73_MIDBODY_CARRY_AUDIT gate asserts carry == inversion at the flush and stays as a permanent tripwire. The midbody callee-rebuild commit path is not exercised by the synth corpus (0/280 both backends), so the audit is a by-construction certificate here rather than an exercised one. check.py dynasm 294/294 + cranelift 294/294 byte-identical. Assisted-by: Claude
b9071d5 to
1bee8cf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-jit/src/call_jit.rs`:
- Around line 650-663: Track first-hit heartbeats independently for each audit
site instead of using the shared HITS counter alone. Update the instrumentation
around the site identifier and the caught/exit_guard_exc/exit_got_exc paths to
maintain separate atomic counters and emit each site’s heartbeat on its own
first execution, while preserving HITS for aggregate counting if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a4bf8142-0b9e-43bd-b1bc-42521b90ff02
📒 Files selected for processing (6)
pyre/pyre-jit-trace/src/jitcode_dispatch/diag.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rs
| use std::sync::atomic::{AtomicU64, Ordering}; | ||
| static HITS: AtomicU64 = AtomicU64::new(0); | ||
| static DIVERGE: AtomicU64 = AtomicU64::new(0); | ||
| static NONE: AtomicU64 = AtomicU64::new(0); | ||
| let inv = jitcode_index.and_then(|index| { | ||
| pyre_jit_trace::state::python_pc_for_jitcode_pc_public(index, opcode_position) | ||
| }); | ||
| let fwd = unsafe { (*frame_ptr).last_instr as i64 } + 1; | ||
| let hits = HITS.fetch_add(1, Ordering::Relaxed) + 1; | ||
| if hits == 1 { | ||
| // Per-process heartbeat: confirms the site was exercised even when no | ||
| // divergence prints (each check.py test is a fresh process). | ||
| eprintln!("[M73-LASTINSTR] first-hit site={site}"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Track first hits per audit site.
HITS is shared by "caught", "exit_guard_exc", and "exit_got_exc", so only whichever site runs first emits a heartbeat. Later sites can be exercised without any confirmation, weakening the audit’s gated-path verification. Keep the aggregate counter if useful, but add per-site first-hit counters.
As per coding guidelines, gated trace/debug paths must be verified as actually reached.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-jit/src/call_jit.rs` around lines 650 - 663, Track first-hit
heartbeats independently for each audit site instead of using the shared HITS
counter alone. Update the instrumentation around the site identifier and the
caught/exit_guard_exc/exit_got_exc paths to maintain separate atomic counters
and emit each site’s heartbeat on its own first execution, while preserving HITS
for aggregate counting if needed.
Source: Coding guidelines
Continues the jitcode-blackhole epic (#73) on top of #727 (Slice 3'), retiring
runtime consumers of the
py_pc ↔ jitcode_pctranslation tables by reading thecodewriter-built forward py_pc twin instead of inverting
jitcode_pc → py_pc.Commits
d977ab8— cut theBackxlatparent-resume coordinate to the forwardpy_pc twin.
resolve_parent_resume_py_pc'sBackxlatarm reads thecodewriter-built
forward_py_pc_for_jitcode_pctwin, keeping the inversiononly for the empty-twin (skeleton / fixture) class.
cabe2c8—PYRE_M73_LASTINSTR_AUDITblackhole-traceback divergencecensus. A gated census over the blackhole traceback-delivery sites,
comparing the
jitcode_pc → py_pcinversion againstframe.last_instr + 1.Measured large, varied divergence (not ±1) across the synth corpus, proving
last_instris many opcodes stale at the raising op — the inversion isload-bearing for
tb_lasti/tb_lineno, so a naive forward-carry here is aNO-GO. Census retained as a tripwire.
2003cdf+e032ddd— route thebackxlat_py_pcresume-word consumerfamily through
state::forward_py_pc_or_backxlat(two phases: audit, thencutover). One helper with a negative-guard (
NO_JITCODE_PC/ taggedbranch-orgpc words delegate to
backxlat_py_pc), aforward_py_pctwin readfor non-negative words, and an empty-twin fallback. All
backxlat_py_pcresume-word call sites converted; the inversion is now reached only via the
helper and the diagnostic fallback. A 3-lens adversarial pass proved
twin == backxlatby construction (marker tier iterates the sameblock_head_py_by_jit_pc+skip_python_trivia_forward; pred tier mirrorsfloor resolution).
PYRE_M73_BACKXLAT_TWIN_AUDITsweep over 280 synthtests × both backends: 0 divergence.
972f566— Eliminatepc_mapand per-PC labels by switching the tracer to interpret JitCode bytecode #73 P1: forward-carry the midbody callee resume py onMidBodyPayload. AddsMidBodyPayload.callee_py_pc, stamped at capturefrom the value already computed for the abort anchor, so the midbody flush
reads a scalar instead of re-inverting
abort_jitcode_pcthroughblock_head_py_by_jit_pc+py_floor_by_jit_pc. Removes one runtime readerof both tables. Byte-parity rests on encode/resume input identity;
PYRE_M73_MIDBODY_CARRY_AUDITis a permanent tripwire (the midbodycallee-rebuild commit path is not exercised by the synth corpus, so the
certificate here is by-construction).
Scoping note (recorded for follow-up)
A map + decompose + adversary scoping of the walker-as-tracer epic found the
byte-preserving forward-carry lever is exhausted after P1: the walk itself is
py-free (
op.pcis a jitcode offset;InlineParentFramecarries jitcode coords,no py scalar), so a forward-carry reduces table readers only where the encode
already converts py for another reason — P1 is the unique such site. The
remaining warm consumers (Entry / outer abort carriers) are pure relocations,
and the true remaining progress is the structural reentry-in-jitcode-coords
change (byte-changing, design-bearing). No
pc_map/ table deletion lands here.Verification
python3 pyre/check.py --backend dynasm,cranelift= 294/294 each, byte-identical.PYRE_M73_BACKXLAT_TWIN_AUDITsweep: 280 synth × 2 backends, 0 divergence.— authored by Claude
Summary by CodeRabbit
Bug Fixes
Diagnostics