Repository navigation
jit: resume overlong traces from post-step frames - #875
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTrace-too-long execution now captures complete blackhole state, preserves concrete values and GC roots across adoption, and uses a dedicated commit path. Vable concrete-value resolution, exception rooting, GC-hook test isolation, and effect-replay coverage are updated. ChangesTrace-too-long handling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Interpreter as trace_bytecode
participant Walker as walk()
participant Latch as latch_trace_too_long_blackhole
participant Adopter as try_adopt_blackhole
participant GC as walk_end_root_walker_area
Interpreter->>Walker: encounter TraceTooLong
Walker->>Latch: request safe snapshot
Latch-->>Walker: publish MIFrame blackhole
Walker->>Adopter: adopt with TraceTooLong commit leg
Adopter->>GC: forward walk-end roots
GC-->>Adopter: updated GC references
Adopter-->>Interpreter: commit terminal state without replay
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 |
![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: e140fc8238
ℹ️ 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".
| if fbw_executed_effect_count() != 0 { | ||
| majit_metainterp::mc_diag_bump(26); | ||
| } else { | ||
| let blackhole_latched = latch_trace_too_long_blackhole(ctx, pc); |
There was a problem hiding this comment.
When an over-limit step returns SubRaise or SubReturn, this unconditional latch runs before the outcome handling below updates the framestack. RPython's run_one_step() has already popped the returning/raising frame and delivered the result or moved to the exception handler before its length check; here the image instead retains the old callee at next_pc (and records raising_exception: false). If this path is hit after an effectful step, blackhole resume can therefore skip exception unwinding or lose a callee return rather than continue from the actual post-step frame state. Only latch after applying these frame-changing outcomes, or restrict this handoff to Continue.
AGENTS.md reference: AGENTS.md:L24-L30
Useful? React with 👍 / 👎.
| let Some(mut miframe) = build_single_frame_miframe(ctx, jitcode, resume_pc, None) else { | ||
| return false; |
There was a problem hiding this comment.
For an effectful trace whose limit is crossed after an ordinary arithmetic or store instruction, resume_pc need not be adjacent to a -live- marker, but build_single_frame_miframe immediately requires try_frame_liveness_reg_indices_by_bank_at_with_jitcode_pc to decode one. It therefore returns None, and walk deliberately keeps recording because effects prevent entry replay; a long straight-line tail with no later live marker can consequently run to completion and compile despite exceeding the trace limit. The new full-bank copy already exists specifically to support arbitrary forward control flow, so this path needs to construct and validate the complete bank without imposing the older live-marker-only precondition.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
![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 (2)
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs (1)
2225-2253: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
latch_trace_too_long_blackholecan snapshot stale exception state when the SAME step both raises and trips the trace-length limit.The
is_too_long()check (and itslatch_trace_too_long_blackholecall) runs BEFORE thematch outcome { DispatchOutcome::SubRaise { exc, exc_concrete } => { ctx.last_exc_value_concrete = exc_concrete; ... } }arm below (2267-2329) has a chance to updatectx.last_exc_value_concrete. If this step'soutcomeis itselfSubRaise(a residual call that raised) at exactly the point the op-count threshold is crossed,is_too_long()triggers an earlyreturn Err(DispatchError::TraceTooLong { .. }), so theSubRaisearm never runs —ctx.last_exc_value_concretestill holds the previous (stale) exception state, not the exception this step just raised.In
residual_call.rs'slatch_trace_too_long_blackhole, this stale value feedsLatchedSingleFrameBlackhole::last_exc_value, andraising_exceptionis hardcodedfalseregardless. The resulting adopted blackhole would resume as if nothing were raised, silently dropping/mishandling the just-raised exception instead of propagating it.Consider threading the current step's
outcome(or its exception payload) intolatch_trace_too_long_blackhole, or updatingctx.last_exc_value{,_concrete}before theis_too_long()check runs.🤖 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/jitcode_dispatch/mod.rs` around lines 2225 - 2253, Update the trace-too-long handling around latch_trace_too_long_blackhole so a same-step DispatchOutcome::SubRaise supplies its current exception payload before the blackhole snapshot is taken. Ensure the latched frame records the raised exception and propagates the raising state instead of using stale ctx.last_exc_value_concrete; preserve existing behavior for non-raising outcomes and fallback handling.pyre/pyre-jit-trace/src/jitcode_dispatch/vable_ops.rs (1)
572-606: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
vable_value_concretecan read the wrong register's concrete after the TOS-recovery override.At line 599,
valueis reassigned totos(fromctx.vstack_boxes) when the original register was unbound. But line 606 still callsvable_value_concrete(code, op, 2, ctx, value_bank, value)with the sameoperand_offset=2, which re-derives the concrete from the ORIGINAL bytecode-encoded register — a slot unrelated totos. If that register's concrete shadow happens to hold stale non-Nulldata (plausible given the same comment block's own point about reused colors across loop-header merges),from_registerwins and the wrong concrete gets recorded into the vable array, instead of falling through toconcrete_of_opref(tos)(the previous, correct behavior).🐛 Proposed fix: derive concrete from the possibly-overridden `value`
let mut value = match value_bank { 'i' => read_int_reg(code, op, 2, ctx)?, 'r' => read_ref_reg(code, op, 2, ctx)?, 'f' => read_float_reg(code, op, 2, ctx)?, _ => unreachable!("value_bank must be 'i', 'r' or 'f'"), }; + let mut concrete = vable_value_concrete(code, op, 2, ctx, value_bank, value); // A STORE_FAST (`index < nlocals`) pops the operand-stack TOS and writes ... if let Some(&tos) = ctx.vstack_boxes.last() { if !tos.is_none() { value = tos; + concrete = ctx.trace_ctx.concrete_of_opref(tos); } } ... let (fdescr, adescr) = vable_array_descrs_from_jitcode(code, op, 3, 5, ctx)?; - let concrete = vable_value_concrete(code, op, 2, ctx, value_bank, value).unwrap_or(Value::Void); + let concrete = concrete.unwrap_or(Value::Void);🤖 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/jitcode_dispatch/vable_ops.rs` around lines 572 - 606, Update the concrete-value derivation around vable_value_concrete so a TOS recovery from ctx.vstack_boxes does not re-read the original bytecode register at operand offset 2. Track whether value was overridden, and for the recovered OpRef derive its concrete directly from that OpRef (using the existing concrete_of_opref path), while preserving the current register-based derivation when no override occurred.
🤖 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/jitcode_dispatch/residual_call.rs`:
- Around line 147-204: Add GC rooting around the latched blackhole images
created in latch_trace_too_long_blackhole: root the relevant
ref_values/registers_r slices for the entire recording and consumption window,
and retain those roots until adoption completes. Update the adoption path to
consume images through take_*_blackhole() rather than directly reading TLS
state, and return the fallback failure result when either image is no longer
valid.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs`:
- Around line 2225-2253: Update the trace-too-long handling around
latch_trace_too_long_blackhole so a same-step DispatchOutcome::SubRaise supplies
its current exception payload before the blackhole snapshot is taken. Ensure the
latched frame records the raised exception and propagates the raising state
instead of using stale ctx.last_exc_value_concrete; preserve existing behavior
for non-raising outcomes and fallback handling.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/vable_ops.rs`:
- Around line 572-606: Update the concrete-value derivation around
vable_value_concrete so a TOS recovery from ctx.vstack_boxes does not re-read
the original bytecode register at operand offset 2. Track whether value was
overridden, and for the recovered OpRef derive its concrete directly from that
OpRef (using the existing concrete_of_opref path), while preserving the current
register-based derivation when no override occurred.
🪄 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: 17d2dc6a-9675-4db2-b264-9ea945f7602b
📒 Files selected for processing (6)
pyre/bench/synth/trace_too_long_effect_replay.pypyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/jitcode_dispatch/vable_ops.rspyre/pyre-jit-trace/src/trace.rs
| pub(crate) fn latch_trace_too_long_blackhole<Sym: WalkSym>( | ||
| ctx: &WalkContext<'_, '_, Sym>, | ||
| resume_pc: usize, | ||
| ) -> bool { | ||
| let last_exc_value = match ctx.last_exc_value_concrete { | ||
| ConcreteValue::Ref(value) => value as i64, | ||
| _ => 0, | ||
| }; | ||
|
|
||
| if ctx.session.borrow().framestack.is_empty() && !ctx.fbw_mode.inline_subwalk { | ||
| let jitcode = unsafe { | ||
| if ctx.fbw_mode.snapshot_sym.is_null() { | ||
| None | ||
| } else { | ||
| let jitcode = (&*ctx.fbw_mode.snapshot_sym).jitcode(); | ||
| (!jitcode.is_null()).then(|| (&(*jitcode).payload).jitcode.clone()) | ||
| } | ||
| }; | ||
| let Some(jitcode) = jitcode else { | ||
| return false; | ||
| }; | ||
| let Some(mut miframe) = build_single_frame_miframe(ctx, jitcode, resume_pc, None) else { | ||
| return false; | ||
| }; | ||
| fill_trace_too_long_register_banks(ctx, &mut miframe); | ||
| // Keep the per-frame red identity seeded by `frame_box`. The | ||
| // authoritative walk executed against `snapshot_for_tracing`, but the | ||
| // adopter validates this identity and publishes that snapshot's locals | ||
| // to the matching live frame before driving the blackhole. Replacing | ||
| // the register with the snapshot address would collapse those two | ||
| // identities and fail the live-root check. | ||
| FBW_SINGLE_FRAME_BLACKHOLE.with(|slot| { | ||
| *slot.borrow_mut() = Some(LatchedSingleFrameBlackhole { | ||
| miframe, | ||
| last_exc_value, | ||
| raising_exception: false, | ||
| }); | ||
| }); | ||
| true | ||
| } else if ctx.fbw_mode.inline_subwalk && multi_frame_blackhole_resume_enabled() { | ||
| let Some(mut framestack) = build_multi_frame_miframe(ctx, resume_pc, None) else { | ||
| return false; | ||
| }; | ||
| if let Some(innermost) = framestack.frames.last_mut() { | ||
| fill_trace_too_long_register_banks(ctx, innermost); | ||
| } | ||
| FBW_MULTI_FRAME_BLACKHOLE.with(|slot| { | ||
| *slot.borrow_mut() = Some(LatchedMultiFrameBlackhole { | ||
| framestack, | ||
| last_exc_value, | ||
| raising_exception: false, | ||
| }); | ||
| }); | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for GC root-scanning of the trace-too-long latch TLS cells.
rg -n 'FBW_SINGLE_FRAME_BLACKHOLE|FBW_MULTI_FRAME_BLACKHOLE' --type=rust -C3Repository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching residual_call.rs:"
fd -a 'residual_call\.rs$' . || true
echo
echo "Files matching blackhole symbols (case insensitive):"
rg -n -i 'blackhole|push_resume_ref_roots|shadow_stack' --type=rust -C2 . || true
echo
echo "Find module files in adjacent dirs:"
fd -a 'mod\.rs$|trace\.rs$' pyre/pyre-jit-trace/src/jitcode_dispatch 2>/dev/null || trueRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo root and relevant file existence:"
pwd
git ls-files | rg '(^|/)pyre/pyre-jit-trace/src/jitcode_dispatch/(residual_call\.rs|mod\.rs|trace\.rs)$|majit_gc.*\.rs$|shadow.*\.rs$|gc.*\.rs$' || true
echo
echo "Read candidate lines from residual_call.rs if present:"
if [ -f "pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs" ]; then
wc -l pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
sed -n '1,260p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
fiRepository: youknowone/pyre
Length of output: 13981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Blackhole TLS cell symbols in tracked Rust files:"
rg -n --glob '*.rs' 'FBW_SINGLE_FRAME_BLACKHOLE|FBW_MULTI_FRAME_BLACKHOLE|single_frame_blackhole_cell_ptr|multi_frame_blackhole_cell_ptr|take_single_frame_blackhole|take_multi_frame_blackhole|reset_single_frame_blackhole' .
echo
echo "Shadow-stack APIs and registrations around blackhole/image paths:"
rg -n --glob '*.rs' 'push_resume_ref_roots|refresh_from_gc|shadow_stack::|scan|root' majit/majit-gc/src/shadow_stack.rs pyre/pyre-jit-trace/src/jitcode_dispatch -C3 | rg -n 'blackhole|FBW|resume|scan|refresh|push_resume|shadow_stack' -C3 || true
echo
echo "GC root/walk API definitions/usages:"
rg -n --glob '*.rs' 'scan|root|gcreftracer|trace_roots|visit|GcRef|Gc' majit/majit-gc/src majit/majit/src pyre/pyre-jit-trace/src/jitcode_dispatch -C2 | head -n 200Repository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant residual_call.rs sections after definitions:"
sed -n '260,420p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
echo '--- take/latch callers ---'
rg -n --glob '*.rs' 'latch_trace_too_long_blackhole|try_adopt_single_frame_blackhole|try_adopt_multi_frame_blackhole|take_single_frame_blackhole|take_multi_frame_blackhole|single_frame_blackhole_cell_ptr|multi_frame_blackhole_cell_ptr|reset_single_frame_blackhole|FBW_SINGLE_FRAME_BLACKHOLE|FBW_MULTI_FRAME_BLACKHOLE' pyre/pyre-jit-trace -C2
echo
echo "Shadow stack implementation:"
wc -l majit/majit-gc/src/shadow_stack.rs
sed -n '1,260p' majit/majit-gc/src/shadow_stack.rsRepository: youknowone/pyre
Length of output: 29931
Add GC rooting for the latched trace-too-long blackhole images.
LatchedSingleFrameBlackhole/LatchedMultiFrameBlackhole copy Ref register addresses into mutable TLS slices and keep them visible until walk end. Because the walker can keep recording after the latch, these raw snapshots need explicit roots for the whole consumption/window before the collector moves objects. Push the relevant ref_values/registers_r slices as roots and use take_*_blackhole() for adoption, with a fallback that declines when the images are no longer valid.
🤖 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/jitcode_dispatch/residual_call.rs` around lines 147 -
204, Add GC rooting around the latched blackhole images created in
latch_trace_too_long_blackhole: root the relevant ref_values/registers_r slices
for the entire recording and consumption window, and retain those roots until
adoption completes. Update the adoption path to consume images through
take_*_blackhole() rather than directly reading TLS state, and return the
fallback failure result when either image is no longer valid.
Source: Coding guidelines
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 8f69cb9). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
e140fc8 to
bee049e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bee049ebf4
ℹ️ 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".
| // pre-transition callee image here. | ||
| let blackhole_latched = trace_too_long_blackhole_snapshot_safe(&outcome) | ||
| && latch_trace_too_long_blackhole(ctx, pc); | ||
| if blackhole_latched || fbw_executed_effect_count() == 0 { |
There was a problem hiding this comment.
When an over-limit walk has already executed an unjournaled effect, this returns TraceTooLong as soon as the image is latched, but try_adopt_blackhole later has additional failure paths—missing virtualizable entries, frame-identity mismatches, or a rejected apply_blackhole_crn. On any such failure the epilogue rolls back only journaled stores and the caller replays from trace entry, duplicating the already-applied effect; a failure after driving the blackhole can duplicate effects from both executions. Preflight every adoption requirement before returning the error, or make a failed adoption continue without entry replay.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| } else if ctx.fbw_mode.inline_subwalk && multi_frame_blackhole_resume_enabled() { | ||
| let Some(framestack) = | ||
| build_multi_frame_miframe(ctx, resume_pc, InnermostMiframeBuild::TraceTooLong) |
There was a problem hiding this comment.
When PYRE_FBW_MULTIFRAME=1 and the limit fires inside an inlined subwalk, this publishes a multi-frame image even though the adopter only materializes frame 0's locals; its own surrounding documentation notes that inner levels have no equivalent write-back. If the blackhole reads a local assigned in the callee before the abort, it therefore reads the callee frame's stale/null heap slot rather than the register image, producing an incorrect result while still committing the walk. Do not enable this handoff until locals are materialized independently for every encoded frame.
AGENTS.md reference: AGENTS.md:L24-L30
Useful? React with 👍 / 👎.
| &walk_result, | ||
| Err(crate::jitcode_dispatch::DispatchError::VableEscapedDuringResidualCall { .. }) | ||
| | Err(crate::jitcode_dispatch::DispatchError::TraceTooLong { .. }) |
There was a problem hiding this comment.
When an over-limit single-frame blackhole executes local assignments and then reaches ExitFrameWithExceptionRef, this new terminal path commits and preserves the exception, but try_adopt_single_frame_blackhole ran vable stores against the live frame without copying them into cf_addr. The caller subsequently restores the live frame from that stale tracing snapshot, so the traceback's tb_frame.f_locals loses assignments made between the length abort and the raise. Mirror the live blackhole frame into the committed snapshot before accepting terminal outcomes, as the multi-frame adopter already does.
AGENTS.md reference: AGENTS.md:L24-L30
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1f661ff4d
ℹ️ 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".
| let last_exc_value = match ctx.last_exc_value_concrete { | ||
| ConcreteValue::Ref(value) => value as i64, | ||
| _ => 0, |
There was a problem hiding this comment.
When the trace limit fires while last_exc_value is still live (for example, after last_exception but before last_exc_value/>r copies the caught exception into a register), this new latch stores the only concrete pointer outside the MIFrame Ref bank. take_single_frame_blackhole() then removes it from the TLS root area, while drive_single_frame_blackhole roots only miframe.ref_values/the blackhole register bank—not last_exc_value—unlike the multi-frame driver, which explicitly adds an exception_root. An allocation during the resumed blackhole can therefore move or reclaim the exception before it is read, leaving exception_last_value stale; include this pointer in the single-frame packed root set for the entire drive.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a69d05187
ℹ️ 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".
| (*(cf_addr as *mut pyre_interpreter::PyFrame)).restore_resume_state_from( | ||
| &*(committed_root_addr as *const pyre_interpreter::PyFrame), |
There was a problem hiding this comment.
When a single-frame adoption uses the normal distinct live frame and tracing snapshot, drive_single_frame_blackhole is passed cf_addr as its virtualizable_ptr, so local/scalar writes made while resuming land in the tracing snapshot. This copy immediately overwrites that post-blackhole state with the live frame's pre-drive state; on terminal return or exception there is no subsequent apply_blackhole_crn reconstruction, and the portal then copies the stale snapshot back to the live frame, losing assignments made after the trace-length abort (observable through an escaped frame or traceback). This overlaps the earlier terminal-locals report, but the fresh evidence is that the added attempted fix copies in the wrong direction relative to the driver's explicit cf_addr argument. Keep the snapshot as authoritative here, or run the blackhole against the live frame before folding live state back.
AGENTS.md reference: AGENTS.md:L32-L40
Useful? React with 👍 / 👎.
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/jitcode_dispatch/mod.rs (1)
2242-2264: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winOvershoot path now retries the full image build on every remaining step.
Once
is_too_long()is true it stays true, so an effectful walk that cannot latch re-runslatch_trace_too_long_blackholeper step —MIFrame::newplus a full three-bank fill pluscapture_frame_locals(aVecallocation) — for every opcode until the walk closes or aborts. Previously this was one comparison. Consider making the structurally-permanent declines sticky (nullsnapshot_sym, inline sub-walk / non-empty framestack, novirtualizable_info,frame_reg == u16::MAX) so only the genuinely step-dependent reasons are retried.🤖 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/jitcode_dispatch/mod.rs` around lines 2242 - 2264, Make the permanent failure cases in the overshoot handling sticky so effectful walks do not rebuild the frame image on every step: cache the decline when snapshot_sym is null, the walk is inline or has a non-empty framestack, virtualizable_info is absent, or frame_reg is u16::MAX. Update trace_too_long_blackhole_latched/latch_trace_too_long_blackhole and the is_too_long branch to reuse that cached result, while continuing to retry only genuinely step-dependent conditions.
🤖 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/jitcode_dispatch/residual_call.rs`:
- Around line 147-177: Update latch_trace_too_long_blackhole so it immediately
returns false unless ctx.is_authoritative_executor is true. Keep the existing
single-frame latch and publication logic unchanged for authoritative executors.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs`:
- Around line 2242-2264: Make the permanent failure cases in the overshoot
handling sticky so effectful walks do not rebuild the frame image on every step:
cache the decline when snapshot_sym is null, the walk is inline or has a
non-empty framestack, virtualizable_info is absent, or frame_reg is u16::MAX.
Update trace_too_long_blackhole_latched/latch_trace_too_long_blackhole and the
is_too_long branch to reuse that cached result, while continuing to retry only
genuinely step-dependent conditions.
🪄 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: 8bab593a-4bfa-48d7-8f62-5a10383e4ade
📒 Files selected for processing (7)
pyre/bench/synth/trace_too_long_effect_replay.pypyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/jitcode_dispatch/vable_ops.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80b32bbe1b
ℹ️ 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".
| if committed_root_addr != cf_addr { | ||
| unsafe { | ||
| (*(cf_addr as *mut pyre_interpreter::PyFrame)).restore_resume_state_from( | ||
| &*(committed_root_addr as *const pyre_interpreter::PyFrame), |
There was a problem hiding this comment.
When the live PyFrame is nursery-allocated and the resumed blackhole triggers a minor collection, its register-bank copy is forwarded in place, but committed_root_addr remains the raw address captured before the drive. The new live-to-snapshot copy fixes the earlier copy direction, but it then dereferences that stale address here, which can copy reclaimed data or crash; recover the frame from the forwarded terminal.registers_r[frame_reg] before folding it into cf_addr.
AGENTS.md reference: AGENTS.md:L32-L41
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f210447cb
ℹ️ 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".
| apply_blackhole_crn( | ||
| cf_addr, | ||
| jitcode_index, | ||
| terminal.position, |
There was a problem hiding this comment.
When an over-limit blackhole returns ContinueRunningNormally with a live Ref slot followed by an Int/Float slot, this materializes the result into cf_addr, whose snapshot_for_tracing locals array uses StdAlloc and is not GC-scanned. apply_blackhole_crn can store the Ref there and then allocate while boxing the later scalar; although terminal.registers_r is rooted, the already-copied snapshot slot is not forwarded because remember_frame_locals_array is a no-op for that array. Even without an intervening collection, the later restore_resume_state_from copies nursery refs into the old live array without a write barrier. The resumed frame can therefore receive a stale or subsequently reclaimed object; stage these values in rooted storage and arm the live array's barrier when publishing them.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
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-trace/src/trace.rs`:
- Around line 83-90: Replace TLS-based WALK_END_CRN_ROOTS and
FBW_SINGLE_FRAME_BLACKHOLE with an owned adoption handoff root carrier. In
pyre/pyre-jit-trace/src/trace.rs:83-90 create the carrier, finalize and drop it
after the frame barrier at 108-119, forward its roots through the owner’s GC
traversal at 304-316, and stage CRN boxes into it at 1994-2016; in
pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs:219-225 store
LatchedSingleFrameBlackhole in the owning adoption state. Update bridge and
portal adoption finalization to pass this carrier in
pyre/pyre-jit/src/call_jit.rs:3124-3128 and pyre/pyre-jit/src/eval.rs:7792-7797.
🪄 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: f0d59f87-db1a-4df4-9bb7-4ffcb86abcae
📒 Files selected for processing (5)
majit/majit-metainterp/src/jitdriver.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ccc560100
ℹ️ 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".
| static WALK_END_CRN_ROOTS: std::cell::RefCell<Vec<i64>> = | ||
| const { std::cell::RefCell::new(Vec::new()) }; |
There was a problem hiding this comment.
When one thread is between apply_blackhole_crn and finish_walk_end_frame_adoption and another mutator triggers a stop-the-world collection, this TLS vector is missed: walk_walk_end_propagated_exception is registered only as a generic extra-root walker, which resolves the collecting thread's TLS, while register_thread_root_areas does not capture WALK_END_CRN_ROOTS for foreign-mutator walking. The snapshot array is explicitly outside the GC, so references held only here can be moved or reclaimed before adoption; add this cell to the per-mutator extra-root areas, as is already done for the finish-concrete and store-journal TLS carriers.
AGENTS.md reference: AGENTS.md:L148-L155
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/432c75057dc8a7d1d435bc431d5a021bf0de8bf6/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs#L417-L418
When a minor collection moves a nursery object before the trace reaches its limit, concrete_registers_r still contains the old raw address—the concrete shadow is not visited by the active-trace root walker—while the corresponding OpRef payload is forwarded. Preferring the raw shadow here places stale pointers, including potentially the live PyFrame, into the latched MIFrame; the subsequent locals preflight or blackhole execution can then dereference reclaimed memory. Resolve the OpRef first and use the raw shadow only when no forwarded concrete payload exists.
AGENTS.md reference: AGENTS.md:L194-L196
https://github.com/youknowone/pyre/blob/432c75057dc8a7d1d435bc431d5a021bf0de8bf6/pyre-jit-trace/src/jitcode_dispatch/vable_ops.rs#L38-L42
If a Ref register survives an allocating residual that triggers a minor collection, its OpRef concrete is forwarded but its ConcreteValue::Ref shadow remains the pre-GC address (the later read path in this file already documents that raw concrete copies dangle). Returning from_register before consulting concrete_of_opref therefore makes setfield_vable/setarrayitem_vable synchronize a stale pointer into the frame and virtualizable shadow. Prefer the GC-forwarded OpRef value, falling back to the register shadow only for values that have no intrinsic OpRef payload.
AGENTS.md reference: AGENTS.md:L194-L196
https://github.com/youknowone/pyre/blob/432c75057dc8a7d1d435bc431d5a021bf0de8bf6/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs#L169-L170
When tracing runs on a nursery-allocated JIT frame and a collecting residual moves that frame, live_vable_frame_addr remains the raw address captured at trace entry; only the actual frame roots and OpRef payloads are forwarded. If the frame register is subsequently refreshed from its forwarded box, the latch's vable_frame != root_addr gate permanently rejects every over-limit image, so an effectful function-entry walk continues recording without a bound. Derive this identity from the rooted per-frame red box rather than the cached usize.
AGENTS.md reference: AGENTS.md:L32-L41
ℹ️ 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".
432c750 to
a5715c8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5715c8dc3
ℹ️ 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".
| roots: slots[stack_base..stack_depth] | ||
| .iter() | ||
| .map(|&value| value as i64) | ||
| .collect(), |
There was a problem hiding this comment.
When a collection occurs earlier in an effectful walk while a nursery object remains on the operand stack, these slots can still contain pre-forwarding addresses: pyframe.rs::snapshot_for_tracing explicitly keeps this StdAlloc snapshot off the CURRENT_FRAME chain with no GC root, while the parallel concrete register state is what gets forwarded. Copying the raw snapshot words into a root vector only after the trace limit is crossed is too late, and publish_captured_frame_stack then installs stale references into the live frame before blackhole execution, potentially causing wrong values or invalid-pointer access. Build this carrier from the GC-forwarded MIFrame/virtualizable shadow, or root the snapshot slots for the entire walk.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-metainterp/src/jitdriver.rs (1)
3725-3733: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated blackhole-chain seeding into a shared helper.
back_edge_internalandrun_back_edge_genericeach carry an identical (comment included)bh_jitdrivers_sdseeding block and an identicalstate_field_layout/vinfo-seeding loop overframe.nextblackholeinterp. Since both were touched in this PR, this is a good point to consolidate before the copies drift.
majit/majit-metainterp/src/jitdriver.rs#L3725-L3733:jitdrivers_sdseeding, identical to L6145-L6153.majit/majit-metainterp/src/jitdriver.rs#L3795-L3805: chain-widestate_field_layout/vinfo seeding loop, identical to L6206-L6217.majit/majit-metainterp/src/jitdriver.rs#L6145-L6153: duplicate of L3725-L3733.majit/majit-metainterp/src/jitdriver.rs#L6206-L6217: duplicate of L3795-L3805.♻️ Suggested extraction
+ fn seed_blackhole_chain( + &self, + bh: &mut crate::blackhole::BlackholeInterpreter, + sf_layout: crate::blackhole::StateFieldLayout, + vable_ptr: i64, + ) { + bh_builder.setup_jitdrivers_sd(bh_jitdrivers_sd(&self.meta_interp().staticdata)); + let seed_vinfo_ptr = seed_deopt_vinfo_ptr(self.meta.virtualizable_info()); + let mut current = Some(bh); + while let Some(frame) = current { + frame.state_field_layout = sf_layout.clone(); + if !seed_vinfo_ptr.is_null() { + frame.virtualizable_info = seed_vinfo_ptr; + frame.virtualizable_ptr = vable_ptr; + } + current = frame.nextblackholeinterp.as_deref_mut(); + } + }Also applies to: 3795-3805, 6145-6153, 6206-6217
🤖 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 `@majit/majit-metainterp/src/jitdriver.rs` around lines 3725 - 3733, Extract the duplicated blackhole-chain seeding logic from back_edge_internal and run_back_edge_generic into a shared helper, including both bh_jitdrivers_sd setup and the state_field_layout/vinfo loop over frame.nextblackholeinterp. Replace all four inline blocks with calls to that helper while preserving their existing initialization order and behavior.
🤖 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/jitcode_dispatch/mod.rs`:
- Around line 2237-2262: The trace-too-long fallback in the walk dispatch flow
can continue recording indefinitely for effectful inline sub-walks when no
blackhole image is latchable. Update the handling around
trace_too_long_abort_safe, ctx.fbw_mode.inline_subwalk, and the non-empty
framestack to enforce a hard termination bound or propagate
DispatchError::TraceTooLong after the fallback cannot safely replay; verify
MC_DIAG[26] monitoring only if it is part of that termination guarantee.
In `@pyre/pyre-jit-trace/src/state.rs`:
- Around line 4965-4994: Update publish_captured_frame_stack to clear the
inactive stack tail when the resumed captured depth is smaller than the
destination’s existing stack depth. After writing captured.roots, zero slots
from captured.scalars.valuestackdepth through the prior stack extent or
available capacity, while preserving the active [live_base, new_depth) values
and existing validation and write-barrier behavior.
- Around line 4885-4889: Update CapturedFrameStack and its capture/publication
flow to carry and validate a stable identity or generation token for the
originating live_frame, not just layout fields such as stack_base, scalars,
roots, depth, nlocals, or capacity. Ensure frame publication and call sites
reject or avoid applying captured state to a different live_frame instance with
matching layout.
---
Outside diff comments:
In `@majit/majit-metainterp/src/jitdriver.rs`:
- Around line 3725-3733: Extract the duplicated blackhole-chain seeding logic
from back_edge_internal and run_back_edge_generic into a shared helper,
including both bh_jitdrivers_sd setup and the state_field_layout/vinfo loop over
frame.nextblackholeinterp. Replace all four inline blocks with calls to that
helper while preserving their existing initialization order and behavior.
🪄 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: d6b93e5f-aa72-49c0-a3d0-27cfcdb86baf
📒 Files selected for processing (9)
majit/majit-metainterp/src/jitdriver.rspyre/bench/synth/trace_too_long_effect_replay.pypyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/jitcode_dispatch/vable_ops.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/eval.rs
| pub(crate) struct CapturedFrameStack { | ||
| stack_base: usize, | ||
| scalars: FrameScalars, | ||
| roots: Vec<i64>, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 '\b(can_publish_frame_stack|capture_frame_stack_for_publish|publish_captured_frame_stack)\s*\(' --glob '*.rs' .Repository: youknowone/pyre
Length of output: 2043
🌐 Web query:
github.com youknowone/pyre pyre-jit-trace/src/state.rs CapturedFrameStack publish_captured_frame_stack
💡 Result:
In the Pyre project (a Rust-based implementation of a Python interpreter with a meta-tracing JIT framework), the CapturedFrameStack and publish_captured_frame_stack are internal components located within pyre-jit-trace/src/state.rs. These elements are part of the JIT's state management and resume machinery [1][2][3]. The CapturedFrameStack structure is used to represent a stack of frames captured during the execution of a JIT-compiled trace, particularly during bridge resolution, guard failures, or when the JIT needs to reconstruct the interpreter state (e.g., for blackhole interpretation or resume operations) [3]. The publish_captured_frame_stack function is an internal utility within the state module that typically handles the finalization or synchronization of these captured frames back into the execution context. Its role involves ensuring that the state represented by the stack—which may have been partially reconstructed or modified during JIT execution—is correctly "published" or committed back into a form that the interpreter or further JIT bridge resolution can utilize [3]. The JIT framework, often referred to as MaJIT, focuses on providing RPython-style meta-tracing JIT infrastructure for Rust-based interpreters [4][5]. The files within pyre-jit-trace/src/ undergo frequent structural adjustments as the project works to achieve functional and architectural parity with PyPy's RPython JIT [2][3][5]. As a result, specific function signatures and internal structures like CapturedFrameStack are subject to change as the codebase matures [2][6][7]. The technical details regarding these specific functions are primarily managed within the internal Rust modules responsible for the "meta-interp" (meta-interpreter) logic, mirroring the mechanics of PyPy's resume and blackhole mechanisms [3][5].
Citations:
- 1: https://github.com/youknowone/pyre/pull/455
- 2: https://github.com/youknowone/pyre/pull/536
- 3: https://github.com/youknowone/pyre/pull/471
- 4: https://github.com/ever0de/pyre
- 5: youknowone@5209cac
- 6: https://github.com/youknowone/pyre/pull/390
- 7: youknowone@b7cdc87
🌐 Web query:
site:github.com/youknowone/pyre "CapturedFrameStack" "publish_captured_frame_stack"
💡 Result:
The term CapturedFrameStack and the function publish_captured_frame_stack appear to be internal components related to the JIT (Just-In-Time) compiler architecture of pyre, which is a Rust-based port of the PyPy interpreter [1]. Based on the technical context of the pyre codebase, these elements are part of the JIT's metainterpreter and resume mechanism, which handles the transition between compiled native machine code and the bytecode interpreter [1]. Specifically: 1. CapturedFrameStack is an internal data structure used to manage the state of the call stack when the JIT engine needs to "resume" execution back into the interpreter [2][3]. This is necessary during events such as trace aborts, guard failures, or when handling exceptions [2][3]. 2. The function publish_captured_frame_stack is an internal routine associated with these frame reconstruction processes [2]. It typically facilitates making the captured stack state available to the interpreter's eval loop so that execution can continue correctly from the point where the JIT trace was interrupted or exited [2][4]. Because pyre mirrors the architectural naming and structure of PyPy (a Python implementation written in RPython), these components serve functions analogous to PyPy's resume and frame-building logic [1]. These are not part of the public Python API and are internal to the JIT's implementation details [1]. Documentation for such components is generally found in the source code comments and commit messages of the pyre repository rather than in user-facing manuals [2][5][4].
Citations:
- 1: https://github.com/youknowone/pyre
- 2: https://github.com/youknowone/pyre/pull/538
- 3: Inline __init__ of a promoted class in FOR_ITER walks; remember TargetToken stores for the minor walk youknowone/pyre#301
- 4: youknowone@bf740e3
- 5: https://github.com/youknowone/pyre/pull/536
🌐 Web query:
site:github.com/youknowone/pyre "CapturedFrameStack" "stack_base" "roots"
💡 Result:
In the context of the Pyre JIT project, CapturedFrameStack, stack_base, and roots are concepts related to stack management, garbage collection (GC) tracing, and JIT compilation [1][2][3][4]. 1. CapturedFrameStack: This refers to structures used to manage frame state, particularly during bridge compilation or deoptimization, where the JIT must reconstruct or "capture" the interpreter state (like local variables and the operand stack) [1][2][4]. Pyre uses these to bridge the gap between JIT-compiled code and the interpreter, often ensuring that the state is properly rooted for the GC [2][4]. 2. stack_base: This field represents the base offset or pointer within a frame's allocated storage where its local variables and operand stack begin [1][4]. For example, JIT metadata typically uses stack_base alongside a maximum stack depth to calculate the size of a frame array (e.g., bridge frame arrays) [1]. 3. roots: In Pyre's GC subsystem, roots are references (such as pointers to GC-managed objects) that the collector must trace to ensure heap-allocated objects remain alive [3][5]. The system maintains root sets—such as the shadow stack or specialized resume-ref roots—which are walked during minor or major GC collections to seed the marking process [2][3]. These components work together to ensure memory safety during JIT execution: when a JIT frame is active or being resumed, the runtime must be able to identify all live object references on the stack (the "roots") to prevent the GC from prematurely reclaiming memory used by active frames [2][4][5].
Citations:
- 1: Cut LLBC store disk, prepass CPU about 9% and peak memory 0.45 GB; make the prepass output deterministic youknowone/pyre#300
- 2: Inline __init__ of a promoted class in FOR_ITER walks; remember TargetToken stores for the minor walk youknowone/pyre#301
- 3: youknowone@f34380b
- 4: youknowone@cf7dcf8
- 5: https://github.com/youknowone/pyre/pull/532
🌐 Web query:
site:github.com/evert/pyre CapturedFrameStack "live_frame" "can_publish_frame_stack"
💡 Result:
I couldn’t find a GitHub match for that exact string combination (CapturedFrameStack, live_frame, can_publish_frame_stack). The search results were empty for those exact terms. If you want, I can try a broader search around the evert/pyre repo or inspect a specific file/path.
🌐 Web query:
site:github.com evert pyre pyre-jit-trace state.rs CapturedFrameStack
💡 Result:
In the pyre project, specifically within the pyre-jit-trace component, CapturedFrameStack and state.rs are key elements of the JIT's metainterpreter state management, designed to mirror RPython's JIT architecture in Rust [1][2][3]. The file state.rs serves as a central repository for JIT state definitions, including helpers for PC-only translation, liveness tracking, semantic mapping, constant-slot handling, and inline frame reconstruction [2]. CapturedFrameStack is a structure used to manage the stack of captured frames during trace execution and resumption [1][3]. These components facilitate the following mechanisms: 1. Frame Capture and Resumption: When a JIT guard fails or a call boundary is crossed, the system must reconstruct the Python execution state. CapturedFrameStack helps maintain the necessary metadata (such as stack depth, PC, and frame values) to allow the JIT to resume execution from a "blackholed" state or a bridge [1][3]. 2. RPython Parity: The pyre project aims for structural parity with RPython’s metainterp [3]. Consequently, state.rs often contains logic that maps directly to RPython concepts like MIFrame or blackhole interpreter chains [3]. 3. Structural Adapting: Because Rust's memory model and type system differ from RPython's GC-based environment, state.rs manages the boxing of values and the storage of JitCode metadata to ensure that the JIT can correctly perform operations such as getfield or setfield on virtualizable objects [4][5][6]. In short, CapturedFrameStack and state.rs are part of the infrastructure that enables the pyre JIT to effectively pause, inspect, and resume Python code by maintaining a consistent snapshot of the interpreter's state during tracing and execution [2][3].
Citations:
- 1: https://github.com/youknowone/pyre/pull/455
- 2: https://github.com/youknowone/pyre/pull/536
- 3: https://github.com/youknowone/pyre/pull/471
- 4: https://github.com/youknowone/pyre/pull/390
- 5: youknowone@5209cac
- 6: https://github.com/youknowone/pyre/pull/465
🌐 Web query:
site:github.com/evert/pyre "live_frame" "CapturedFrameStack"
💡 Result:
I couldn’t find an exact GitHub match for site:github.com/evert/pyre "live_frame" "CapturedFrameStack". The closest result I found was pyfyre/pyfyre, which appears to be a different project. (github.com)
If you want, I can try a broader repo/code search for live_frame or CapturedFrameStack alone.
Citations:
🌐 Web query:
"CapturedFrameStack" "live_frame" "publish_captured_frame_stack" "can_publish_frame_stack"
💡 Result:
The terms you provided do not correspond to a single, well-known software API or library. Instead, they appear to be a mix of distinct concepts and potentially custom identifiers from specialized projects [1][2]. 1. CapturedFrameStack: This is not a standard, widely-used API. The term appears in specific, unrelated contexts: - A custom Python script named captureFrameStack.py associated with a research repository [1]. - It may be confused with StackFrames, a Ruby library for capturing call traces without object allocations [2]. 2. live_frame, publish_captured_frame_stack, and can_publish_frame_stack: These do not appear in major mainstream APIs (such as LiveKit or standard Linux CAN networking). - In the context of video streaming (e.g., LiveKit), developers typically use terms like VideoSource, capture_frame, and publish_track [3][4][5][6][7]. - In the context of Controller Area Network (CAN) bus development, the Linux kernel and associated libraries use terms like skb (socket buffer), can_frame, and can_put_echo_skb, but they do not use a publish_captured_frame_stack function [8][9][10]. If you are working with a specific proprietary codebase, these are likely internal helper functions or classes designed to manage a buffer or stack of frames for a specific capture process. If you can provide the name of the library or framework (e.g., a specific C++ project or a niche Python tool), I can provide more targeted technical guidance.
Citations:
- 1: https://github.com/ElsevierSoftwareX/SOFTX-D-23-00293/blob/main/captureFrameStack.py
- 2: https://github.com/Shopify/stack_frames/
- 3: https://github.com/livekit/python-sdks/blob/main/examples/local_video/publisher.py
- 4: https://gist.github.com/maisonsmd/d9af7345dc6406d25f3b174de7c046b3
- 5: https://github.com/livekit-examples/cpp-example-collection/blob/main/user_timestamped_video/producer/main.cpp
- 6: https://docs.livekit.io/python/livekit/rtc/video_stream.html
- 7: https://livekit.com/blog/frame-metadata
- 8: https://github.com/torvalds/linux/blob/master/drivers/net/can/dev/skb.c
- 9: https://android.googlesource.com/kernel/msm.git/+/9acc8f9b26a79451ec03d1ba0ba0bfece8cff8cc/net/can/bcm.c
- 10: https://python-can.readthedocs.io/en/v4.1.0/_modules/can/interfaces/socketcan/socketcan.html
Validate frame identity before publishing captured stacks.
CapturedFrameStack currently only stores layout-related fields (stack_base, scalars, roots). If publication checks only depth/nlocals/capacity, two different live_frame instances with the same layout can receive the same captured state. Store a stable capture-time identity/generation token in the carrier, or enforce it at every frame publish/call site.
🤖 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/state.rs` around lines 4885 - 4889, Update
CapturedFrameStack and its capture/publication flow to carry and validate a
stable identity or generation token for the originating live_frame, not just
layout fields such as stack_base, scalars, roots, depth, nlocals, or capacity.
Ensure frame publication and call sites reject or avoid applying captured state
to a different live_frame instance with matching layout.
| pub(crate) fn publish_captured_frame_stack( | ||
| live_frame: usize, | ||
| captured: &CapturedFrameStack, | ||
| ) -> bool { | ||
| let Some(live_base) = concrete_nlocals(live_frame) else { | ||
| return false; | ||
| }; | ||
| if live_base != captured.stack_base | ||
| || captured.scalars.valuestackdepth != live_base + captured.roots.len() | ||
| { | ||
| return false; | ||
| } | ||
| let live_arr = unsafe { | ||
| *((live_frame as *const u8).add(PYFRAME_LOCALS_CELLS_STACK_OFFSET) | ||
| as *const *mut pyre_object::FixedObjectArray) | ||
| }; | ||
| if live_arr.is_null() | ||
| || unsafe { &*live_arr }.as_slice().len() < captured.scalars.valuestackdepth | ||
| { | ||
| return false; | ||
| } | ||
| unsafe { | ||
| let slots = (*live_arr).as_mut_slice(); | ||
| for (offset, &value) in captured.roots.iter().enumerate() { | ||
| slots[live_base + offset] = value as usize as PyObjectRef; | ||
| } | ||
| } | ||
| restore_frame_scalars(live_frame, captured.scalars); | ||
| frame_array_write_barrier(live_frame as *mut u8, live_arr); | ||
| true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear the inactive stack tail when the resumed depth shrinks.
Only [live_base, new_depth) is overwritten. If the destination previously had a deeper stack, old references remain in [new_depth, capacity). The virtualizable path consumes the full locals_cells_stack_w image, so these stale values can remain GC roots and enter later virtualizable snapshots. (raw.githubusercontent.com)
Proposed fix
unsafe {
let slots = (*live_arr).as_mut_slice();
+ let new_depth = captured.scalars.valuestackdepth;
+ for slot in &mut slots[new_depth..] {
+ *slot = pyre_object::PY_NULL;
+ }
for (offset, &value) in captured.roots.iter().enumerate() {
slots[live_base + offset] = value as usize as PyObjectRef;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) fn publish_captured_frame_stack( | |
| live_frame: usize, | |
| captured: &CapturedFrameStack, | |
| ) -> bool { | |
| let Some(live_base) = concrete_nlocals(live_frame) else { | |
| return false; | |
| }; | |
| if live_base != captured.stack_base | |
| || captured.scalars.valuestackdepth != live_base + captured.roots.len() | |
| { | |
| return false; | |
| } | |
| let live_arr = unsafe { | |
| *((live_frame as *const u8).add(PYFRAME_LOCALS_CELLS_STACK_OFFSET) | |
| as *const *mut pyre_object::FixedObjectArray) | |
| }; | |
| if live_arr.is_null() | |
| || unsafe { &*live_arr }.as_slice().len() < captured.scalars.valuestackdepth | |
| { | |
| return false; | |
| } | |
| unsafe { | |
| let slots = (*live_arr).as_mut_slice(); | |
| for (offset, &value) in captured.roots.iter().enumerate() { | |
| slots[live_base + offset] = value as usize as PyObjectRef; | |
| } | |
| } | |
| restore_frame_scalars(live_frame, captured.scalars); | |
| frame_array_write_barrier(live_frame as *mut u8, live_arr); | |
| true | |
| pub(crate) fn publish_captured_frame_stack( | |
| live_frame: usize, | |
| captured: &CapturedFrameStack, | |
| ) -> bool { | |
| let Some(live_base) = concrete_nlocals(live_frame) else { | |
| return false; | |
| }; | |
| if live_base != captured.stack_base | |
| || captured.scalars.valuestackdepth != live_base + captured.roots.len() | |
| { | |
| return false; | |
| } | |
| let live_arr = unsafe { | |
| *((live_frame as *const u8).add(PYFRAME_LOCALS_CELLS_STACK_OFFSET) | |
| as *const *mut pyre_object::FixedObjectArray) | |
| }; | |
| if live_arr.is_null() | |
| || unsafe { &*live_arr }.as_slice().len() < captured.scalars.valuestackdepth | |
| { | |
| return false; | |
| } | |
| unsafe { | |
| let slots = (*live_arr).as_mut_slice(); | |
| let new_depth = captured.scalars.valuestackdepth; | |
| for slot in &mut slots[new_depth..] { | |
| *slot = pyre_object::PY_NULL; | |
| } | |
| for (offset, &value) in captured.roots.iter().enumerate() { | |
| slots[live_base + offset] = value as usize as PyObjectRef; | |
| } | |
| } | |
| restore_frame_scalars(live_frame, captured.scalars); | |
| frame_array_write_barrier(live_frame as *mut u8, live_arr); | |
| true | |
| } |
🤖 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/state.rs` around lines 4965 - 4994, Update
publish_captured_frame_stack to clear the inactive stack tail when the resumed
captured depth is smaller than the destination’s existing stack depth. After
writing captured.roots, zero slots from captured.scalars.valuestackdepth through
the prior stack extent or available capacity, while preserving the active
[live_base, new_depth) values and existing validation and write-barrier
behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 557edf134a
ℹ️ 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".
| matches!( | ||
| &walk_result, | ||
| Err(crate::jitcode_dispatch::DispatchError::TraceTooLong { .. }) | ||
| ) && try_adopt_blackhole(ctx, cf_addr, live_root_addr, WalkEndCommitLeg::TraceTooLong); |
There was a problem hiding this comment.
When a minor collection moves a nursery-allocated PyFrame earlier in an effectful over-limit walk, sym.live_vable_frame_addr() still contains the raw address saved before tracing; the latch deliberately recovers the forwarded address from the frame OpRef and stores it in the MIFrame, but this added adoption call passes the stale raw address. try_adopt_single_frame_blackhole then selects that nonzero stale address as root_addr, finds it differs from the forwarded MIFrame frame register, and panics because effectful TraceTooLong is considered preflighted. Unlike the earlier post-drive forwarding issue, this failure occurs before the blackhole starts; derive the live root from the forwarded frame register or refresh the stored address before adoption.
AGENTS.md reference: AGENTS.md:L32-L41
Useful? React with 👍 / 👎.
The hook cells are process-global and this crate's allocators read them with no gate, so a mock installed by one test answers every other libtest thread for as long as it stays installed. `mock_hook` returned `payload_size` cast to a pointer; `try_alloc_typed_items_block_nursery` running on a concurrent thread wrote the capacity header through it and the process took SIGSEGV at address 0x18. Allocate `payload_size` zeroed bytes instead, and return null when the request cannot be served — the failure `GcAllocHookFn` documents, and what `RBigInt::lshift(i64::MAX)` needs to get `RBigIntError::Memory` back. The mock's alignment matches `ItemsBlock` / `TypedItemsBlock`, the only payloads this crate hands to `std::alloc::dealloc`. Record the mock's arguments and the root-hook probes in thread-locals; a concurrent allocation calls the same mocks with its own type id and size. Assisted-by: Claude
557edf1 to
8f69cb9
Compare
Summary
The full-body walker carried a documented deviation from
SwitchToBlackhole(ABORT_TOO_LONG), and the cost of that deviation was that thetrace-length check could not fire on the walks that need it most.
RPython calls
blackhole_if_trace_too_long()immediately afterMIFrame.run_one_step(), andconvert_and_run_from_pyjitplcopies every liveMIFrame at its already-advanced
pc(pyjitpl.py:2863-2866,blackhole.py:1799-1821) so the interpreter continues forward from thetraced state. pyre instead raised a
DispatchError, which resumes byre-interpreting the iteration from the trace entry. Any irreversible effect the
walk had already executed would then be applied twice.
The previous guard avoided that corruption by declining to abort at all:
So the length bound only held for a walk that had executed no effect yet. An
effectful walk kept recording — the unbounded behaviour that counter was there
to make visible rather than to fix.
The forward handoff
latch_trace_too_long_blackholesnapshots the live meta-interpreter framestackwhile the
WalkContextstill owns it, atwalk()'s post-stepnext_pc— notopcode_position.blackhole_if_trace_too_longraises after the step, so theimage has to carry the advanced pc. The run-per-fn epilogue then adopts that
image and drives it forward, the way
run_blackhole_interp_to_cancel_tracingdoes.
Both shapes are covered: a single live frame goes through
build_single_frame_miframe, and an inline subwalk with paused callers goesthrough
build_multi_frame_miframewhen multi-frame blackhole resume is enabled.try_adopt_force_blackholebecomestry_adopt_blackholeand takes the commitleg as a parameter, so the vable-escape path and the new length path share the
adoption machinery without reporting the same leg.
Distinguishing the commit
WalkEndCommitLeg::TraceTooLong = 9is new. Both adoption sites previouslyhardcoded
VableEscape, so without it these commits would be indistinguishablein the walk-end tallies from "a virtualizable escaped during a residual call".
Concrete values for the image
vable_value_concreteresolves a register operand's concrete half from theparallel concrete bank first, falling back to the OpRef's intrinsic value.
RPython's MIFrame banks hold Box objects, so assigning a valuebox to
virtualizable_boxescarries its concrete value along; pyre splits that Boxbetween an OpRef bank and a concrete bank, and a residual result does not
necessarily have a value attached to its OpRef — only the authoritative walker's
concrete bank knows it. Reading the OpRef alone would publish a hole into the
forward image.
Fallbacks that stay
A complete image makes the abort safe regardless of effects. When one cannot be
built, the old behaviour is kept exactly:
irreversible effect twice.
The remaining overshoot is still tallied (
mc_diag_bump(26)), so an unsupportedframe shape cannot silently become unbounded recording again.
Coverage
pyre/bench/synth/trace_too_long_effect_replay.pygains Phase C: afunction-entry trace, whose root is pc 0 rather than a loop header. The root key
is therefore never revisited, so
ABORT_TOO_LONG's forward handoff is the onlything bounding the recording. It returns three independently observable mutation
classes (list element, dict value, list length) alongside the sum, so a handoff
that replayed from function entry would show up as a doubled count rather than
as a timing difference.
Self-review
This patch is AI-assisted.
This description was written by reading the diff, in a different session from
the one that wrote the code, and it has not been through a separate review pass.
Two things a reviewer should not take on trust from it:
caller in the vable-escape path; driving an inline-subwalk framestack forward
from a length abort is the new shape, and it is gated on
multi_frame_blackhole_resume_enabled(). Whether every paused caller's banksare complete at that point is the claim worth checking against
build_multi_frame_miframe's own preconditions.pyre/check.pywas not run against this branch by the session that openedthe PR. CI on this head is the first end-to-end signal.
latch_trace_too_long_blackholedocuments the invariant it depends on — returnfalsewithout publishing a partial image when any live value is unresolved —so the reviewable question is whether every early return in the builders it
calls actually upholds that. A partially filled image would still be adopted and
run forward.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests