jit(fbw): latch a multi-frame blackhole image on ABORT_TOO_LONG - #910
Merged
Conversation
WalkthroughThe change adds inline live-frame synchronization, validates and publishes multi-frame ChangesInline multi-frame blackhole handling
GC descriptor index validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TraceLoop
participant latch_trace_too_long_blackhole
participant multi_frame_blackhole_preflight
participant try_adopt_multi_frame_blackhole
participant publish_captured_frame_stack
TraceLoop->>latch_trace_too_long_blackhole: encounter ABORT_TOO_LONG in inline_subwalk
latch_trace_too_long_blackhole->>multi_frame_blackhole_preflight: validate multi-frame image
multi_frame_blackhole_preflight-->>latch_trace_too_long_blackhole: accept or decline
latch_trace_too_long_blackhole-->>try_adopt_multi_frame_blackhole: provide latched image
try_adopt_multi_frame_blackhole->>publish_captured_frame_stack: publish captured root stack
publish_captured_frame_stack-->>try_adopt_multi_frame_blackhole: success or decline
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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 |
![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: 1c446f5a3a
ℹ️ 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".
Comment on lines
+2643
to
+2645
| for (&(frame_index, reg_index), &forwarded) in image_ref_locations.iter().zip(&image_ref_roots) | ||
| { | ||
| latched.framestack.frames[frame_index].ref_values[reg_index] = Some(forwarded); |
There was a problem hiding this comment.
When write_back_outer_locals boxes an Int/Float local, it can collect and forward a nursery-allocated root PyFrame. This loop updates only the MIFrame ref banks; the previously captured root_addr and per_frame still contain the old addresses and are subsequently passed to publish_captured_frame_stack and drive_multi_frame_blackhole. In that case the new trace-too-long path dereferences an evacuated frame, risking a crash or corrupt resume state; rebuild those raw pointers from the forwarded banks before publishing or driving.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 4679e1e). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit-trace/src/trace.rs (1)
2395-2513: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing fail-fast invariant for a preflighted trace-too-long multi-frame image, unlike the single-frame path.
try_adopt_single_frame_blackholeasserts!trace_too_longon every decline path, because a preflightedABORT_TOO_LONGimage (is_authoritative_executoralready executed real residual-call side effects) must never fall back to legacy replay — that would double those effects, exactly the hazard this PR's comments describe at length.try_adopt_multi_frame_blackholehas no equivalent invariant anywhere in its decline paths (virtualizable_info missing, jitcode index out of range, unstamped/null frame register, no concrete nlocals, identity-collapse guard, root-identity gate).multi_frame_blackhole_preflightinresidual_call.rswas written precisely to guarantee these gates all pass before latching, but nothing here converts a preflight/adopt mismatch into a loud failure — it just declines via a debug-gatedmfdbg!and silently falls back to replay.🛡️ Proposed fix: assert non-decline for a preflighted trace-too-long image
+ let trace_too_long = commit_leg == WalkEndCommitLeg::TraceTooLong + && crate::jitcode_dispatch::fbw_executed_effect_count() != 0; let Some(mut latched) = crate::jitcode_dispatch::take_multi_frame_blackhole() else { return false; }; let depth = latched.framestack.len(); if ctx.virtualizable_info().is_none() { mfdbg!("no virtualizable_info"); + assert!(!trace_too_long, "preflighted trace-too-long virtualizable info disappeared"); return false; }(repeat for the remaining decline sites in this range, mirroring
try_adopt_single_frame_blackhole's pattern)🤖 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-trace/src/trace.rs` around lines 2395 - 2513, Add the same fail-fast invariant used by try_adopt_single_frame_blackhole to every decline path in try_adopt_multi_frame_blackhole: when the latched image is trace-too-long and was preflighted, assert that adoption cannot decline before returning false. Cover virtualizable_info, frame recovery, concrete nlocals, identity-collapse, and root-identity gates, while preserving normal fallback behavior for non-preflighted images.
🤖 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-trace/src/trace.rs`:
- Around line 2643-2665: After copying forwarded roots into
latched.framestack.frames in the multi-frame path, re-derive each frame’s
current address from its updated ref_values before calling
publish_captured_frame_stack or releasing roots. Update root_addr and the
per-frame addresses using the same committed_root_addr approach as the
single-frame path, ensuring restore and publication use relocated memory.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 2395-2513: Add the same fail-fast invariant used by
try_adopt_single_frame_blackhole to every decline path in
try_adopt_multi_frame_blackhole: when the latched image is trace-too-long and
was preflighted, assert that adoption cannot decline before returning false.
Cover virtualizable_info, frame recovery, concrete nlocals, identity-collapse,
and root-identity gates, while preserving normal fallback behavior for
non-preflighted images.
🪄 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: b7a4572d-6891-4bb9-bb76-987860e0c71b
📒 Files selected for processing (7)
majit/majit-ir/src/descr.rspyre/bench/synth/getframe_inline_subwalk_multiframe.pypyre/bench/synth/trace_too_long_inline_multiframe.pypyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/vable_ops.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rs
`latch_trace_too_long_blackhole` previously returned false for an inlined sub-walk, so an ABORT_TOO_LONG inside an inlined callee fell back to entry replay. It now builds the multi-frame framestack and latches it behind `multi_frame_blackhole_preflight`, a read-only re-check of every adopter gate that can later reject the image -- the abort runs after the opcode's effects, so publishing is only safe when the handoff cannot decline afterwards. `LatchedMultiFrameBlackhole` gains `publish_root_stack`. ABORT_TOO_LONG stops at an arbitrary post-step coordinate, so frame 0's active operand stack has to cross from the detached tracing snapshot to the live red frame before the blackhole runs; the vable-force path keeps its existing handoff and passes false. `try_adopt_multi_frame_blackhole` roots every MIFrame ref bank and the pending exception across root-locals boxing and copies the forwarding updates back before the banks are read, and it pins the canonical `PyFrame` virtualizable info for the whole drive instead of whichever translator-state vinfo happens to remain installed on `TraceCtx`. Standard-vable writes inside an inlined MIFrame are mirrored onto that frame's own concrete red `PyFrame` (`current_inline_concrete_frame`, `store_live_frame_static_int`), so an inner level no longer depends on a per-level slot side-table to be resumable. Adds `synth/trace_too_long_inline_multiframe`, which drives ABORT_TOO_LONG inside an inlined callee that mutates three containers and, in a second part, unwinds instead of returning. Assisted-by: Claude
#874 and #907 each added `fn interp_return_log_enabled` to `pyre-interpreter/src/eval.rs`. Each PR built against its own merge base, so neither run saw the other's copy; landing both left the name defined twice and `origin/main` fails to compile with E0428. The two bodies are identical (`OnceLock` over `PYRE_INTERP_RETURN_LOG`, both `#[cfg(not(feature = "sandbox"))]`), so this removes the second and keeps the first, whose doc names the caller on the return path. Assisted-by: Claude
youknowone
force-pushed
the
trace-too-long-multiframe
branch
from
July 30, 2026 17:21
9de1bf3 to
4679e1e
Compare
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
jit(fbw): latch a multi-frame blackhole image on ABORT_TOO_LONGlatch_trace_too_long_blackholepreviously returned false for an inlinedsub-walk, so an
ABORT_TOO_LONGinside an inlined callee fell back to entryreplay. It now builds the multi-frame framestack and latches it behind
multi_frame_blackhole_preflight, a read-only re-check of every adopter gatethat can later reject the image — the abort runs after the opcode's effects, so
publishing is only safe when the handoff cannot decline afterwards.
LatchedMultiFrameBlackholegainspublish_root_stack.ABORT_TOO_LONGstopsat an arbitrary post-step coordinate, so frame 0's active operand stack has to
cross from the detached tracing snapshot to the live red frame before the
blackhole runs; the vable-force path keeps its existing handoff and passes
false.
try_adopt_multi_frame_blackholeroots every MIFrame ref bank and the pendingexception across root-locals boxing and copies the forwarding updates back
before the banks are read, and it pins the canonical
PyFramevirtualizableinfo for the whole drive instead of whichever translator-state vinfo happens to
remain installed on
TraceCtx.Standard-vable writes inside an inlined MIFrame are mirrored onto that frame's
own concrete red
PyFrame(current_inline_concrete_frame,store_live_frame_static_int), so an inner level no longer depends on aper-level slot side-table to be resumable.
Adds
synth/trace_too_long_inline_multiframe, which drivesABORT_TOO_LONGinside an inlined callee that mutates three containers and, in a second part,
unwinds instead of returning.
Rebased onto
mainafter #874 landed; it applies with no conflict and nothinghere touches the files #874 rewrote in a conflicting way.
interp: drop the duplicate interp_return_log_enabled definitionUnrelated to the work above —
origin/maincurrently does not compile:#874 and #907 each added
fn interp_return_log_enabledtopyre-interpreter/src/eval.rs. Each PR built against its own merge base, soneither run saw the other's copy; landing both left the name defined twice. The
two bodies are identical (
OnceLockoverPYRE_INTERP_RETURN_LOG, both#[cfg(not(feature = "sandbox"))]), so this removes the second and keeps thefirst, whose doc names the caller on the return path.
Happy to split this into its own PR if you would rather land it independently —
it is here only because it blocks this branch from building at all.
🤖 Generated with Claude Code