codewriter: materialize the caught exception on the value-stack slot at generic handler entry - #1054
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe JIT now stores propagated exceptions in durable frame slots and preserves operand-stack mirrors across single-frame blackhole adoption. Resume snapshots exclude local slots, and ChangesException and resume state
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ExceptionHandler
participant FrameValueStack
participant BlackholeLatch
participant ResumePath
ExceptionHandler->>FrameValueStack: store propagated exception
ExceptionHandler->>ResumePath: retain last_exc_value
BlackholeLatch->>BlackholeLatch: capture operand-stack mirror
BlackholeLatch->>ResumePath: publish mirrored operand stack
ResumePath->>ResumePath: reload operands or fall back when mirror is unrepresentable
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: 6779c7a4db
ℹ️ 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".
| || commit_leg == WalkEndCommitLeg::VableEscape | ||
| { | ||
| latched.mirror_stack.as_ref().and_then(|mirror| { | ||
| crate::state::capture_frame_stack_from_mirror( |
There was a problem hiding this comment.
When a vable escape reaches this branch with latched.mirror_stack == None (for example because ctx.vstack_valid is false or a live Ref slot has no concrete value), captured becomes None; try_adopt_single_frame_blackhole has already taken the latched image and returns false, so run_perfn_walk falls back to the escape-pc/legacy replay path. For escapes after an already-executed non-idempotent residual or heap store, that replays the traced region and applies the effect a second time instead of PyPy's forward blackhole resume, so the escape mirror needs to be made mandatory/capturable before latching or otherwise keep a forward-safe handoff rather than declining to replay.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
6779c7a to
029d91e
Compare
youknowone
changed the title
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a2cd5e6). 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
|
029d91e to
e92fddb
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![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-trace/src/trace.rs`:
- Around line 2467-2474: Update the escape-mirror handling around
capture_frame_stack_from_mirror so a None result records the same
effect/unjournaled reservation as the forcing residual before returning false.
This must force the escape-pc/legacy-replay path and prevent replay from
carrying a non-rerunnable residual’s side effect when the opcode window appears
clean.
🪄 Autofix
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: d9fb33ee-845a-4239-9526-79a90e3dd232
📒 Files selected for processing (4)
pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/jit/codewriter.rs
| // The vable-escape leg's loop-back reload reads the vable array, | ||
| // so when its operand mirror is unrepresentable the forward drive | ||
| // has no correct stack to reload and declining to the escape-pc / | ||
| // legacy-replay recovery is the only crash-free path (driving | ||
| // forward would reload a never-published NULL and SEGV). The | ||
| // replay's own non-idempotent-store-ahead hazard is pre-existing | ||
| // (see `residual_call.rs`); the deep fix is to make this mirror | ||
| // always capturable so the escape can resume forward. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Expect: an unrepresentable mirror cannot reach replay after an executed
# unjournaled effect.
rg -n -C 20 \
'VableEscapedDuringResidualCall|capture_frame_stack_from_mirror|escape_opcode_window_clean|fbw_executed_effect_count|fbw_has_unjournaled_effect|walk_end_resume_provable|commit_walk_end' \
pyre/pyre-jit-trace/src
rg -n -C 20 \
'mirror_stack|VableEscape|escape_opcode_window_clean' \
pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rsRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== trace outline around target =="
ast-grep outline pyre/pyre-jit-trace/src/trace.rs --match capture_frame_stack_from_mirror --view expanded || true
echo "== trace lines 2380-2495 =="
sed -n '2380,2495p' pyre/pyre-jit-trace/src/trace.rs | nl -ba -v2380
echo "== residual_call lines 1377-1425 and 1220-1280 =="
sed -n '1377,1425p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs | nl -ba -v1377
sed -n '1220,1280p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs | nl -ba -v1220
echo "== residual_call occurrences of capture_frame_stack_from_mirror and VableEscapedDuringResidualCall =="
rg -n -C 8 'capture_frame_stack_from_mirror|VableEscapedDuringResidualCall|fbw_has_unjournaled_effect|fbw_executed_effect_count|escape_opcode_window_clean|COMMITTED_FRAME_ESCAPE_PC|take_escape_flush_undo_pending' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
echo "== fbw_effect functions definitions =="
rg -n -C 6 'pub .*fbw_(executed_effect_count|has_unjournaled_effect|mark_unjournaled_effect|set_unjournaled_effect)' pyre/pyre-jit-trace/srcRepository: youknowone/pyre
Length of output: 302
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== trace lines 2380-2495 =="
sed -n '2380,2495p' pyre/pyre-jit-trace/src/trace.rs | awk '{printf "%6d %s\n", NR+2379, $0}'
echo "== residual_call lines 1377-1425 and 1220-1280 =="
sed -n '1377,1425p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs | awk '{printf "%6d %s\n", NR+1376, $0}'
sed -n '1220,1280p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs | awk '{printf "%6d %s\n", NR+1219, $0}'
echo "== residual_call occurrences of capture_frame_stack_from_mirror and VableEscapedDuringResidualCall =="
rg -n -C 8 'capture_frame_stack_from_mirror|VableEscapedDuringResidualCall|fbw_has_unjournaled_effect|fbw_executed_effect_count|escape_opcode_window_clean|COMMITTED_FRAME_ESCAPE_PC|take_escape_flush_undo_pending' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
echo "== fbw_effect functions definitions =="
rg -n -C 6 'pub .*fbw_(executed_effect_count|has_unjournaled_effect|mark_unjournaled_effect|set_unjournaled_effect)' pyre/pyre-jit-trace/srcRepository: youknowone/pyre
Length of output: 30035
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== walk end commit logic around replay/journal/unjournaled gate =="
rg -n -C 12 'walk_end_commit|commit_walk_end|fbw_has_unjournaled_effect|fbw_executed_effect_count|take_committed_frame_escape_pc|take_escape_flush_undo_pending|restore_escape_flush_undo|legacy|replay|escape_py_pc' pyre/pyre-jit-trace/src
echo "== definitions of effect-count/unjournaled helpers =="
rg -n -C 8 'fn fbw_(executed_effect_count|has_unjournaled_effect|mark_unjournaled_effect|abort_nested_unjournaled_residual|count_executed_residual)' pyre/pyre-jit-trace/src
echo "== residual execution effect/count gates =="
sed -n '3300,3420p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs | awk '{printf "%6d %s\n", NR+3299, $0}'
sed -n '5000,5070p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs | awk '{printf "%6d %s\n", NR+4999, $0}'Repository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== effect helper definitions =="
rg -n -C 10 'pub\n.*?fbw_executed_effect_count|pub\n.*?fbw_has_unjournaled_effect|pub\n.*?fbw_mark_unjournaled_effect|pub\n.*?fbw_abort_nested_unjournaled_residual|pub\n.*?fbw_count_executed_residual|pub\n.*?fbw_effect' pyre/pyre-jit-trace/src
echo "== residual_call exact line ranges around gate and commit =="
sed -n '3200,3235p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs | awk '{printf "%6d %s\n", NR+3199, $0}'
sed -n '3360,3372p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs | awk '{printf "%6d %s\n", NR+3359, $0}'
echo "== walk end commit snippets =="
rg -n -C 10 'take_committed_frame_escape_pc|Take|commit|no-replay|legacy|fbw_executed_effect_count|fbw_has_unjournaled_effect|restore_escape_flush_undo' pyre/pyre-jit-trace/src/trace.rsRepository: youknowone/pyre
Length of output: 379
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== effect helper definitions =="
rg -n -C 12 'fn fbw_executed_effect_count|fn fbw_has_unjournaled_effect|fn fbw_mark_unjournaled_effect|fn fbw_abort_nested_unjournaled_residual|fn fbw_count_executed_residual' pyre/pyre-jit-trace/src
echo "== residual_call exact line ranges around gate and commit =="
sed -n '3200,3235p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs | awk '{printf "%6d %s\n", NR+3199, $0}'
sed -n '3360,3372p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs | awk '{printf "%6d %s\n", NR+3359, $0}'
echo "== walk end commit snippets =="
rg -n -C 12 'take_committed_frame_escape_pc|commit|no-replay|legacy|fbw_executed_effect_count|fbw_has_unjournaled_effect|restore_escape_flush_undo' pyre/pyre-jit-trace/src/trace.rsRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== residual_call guard commit area =="
sed -n '3316,3375p' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs | awk '{printf "%6d %s\n", NR+3315, $0}'
echo "== captured_single_frame_blackhole definitions and callers =="
rg -n -C 8 'captured_single_frame_blackhole|take_single_frame_blackhole|ActiveFrameEscapeGuard::enter|COMMITTED_FRAME_ESCAPE_PC|take_committed_frame_escape_pc' pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs pyre/pyre-jit-trace/src/trace.rs
echo "== walk end adoption selection area =="
sed -n '1900,2080p' pyre/pyre-jit-trace/src/trace.rs | awk '{printf "%6d %s\n", NR+1899, $0}'Repository: youknowone/pyre
Length of output: 38663
Enforce the escape-pc path when the escape mirror is unrepresentable.
If capture_frame_stack_from_mirror returns None here, this returns false to the legacy replay path even though the forcing residual may already have run. Add the same effect/unjournaled reservation here, otherwise a non-re-runnable residual can have its side effect carried by replay while the opcode window is otherwise clean.
🤖 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 2467 - 2474, Update the
escape-mirror handling around capture_frame_stack_from_mirror so a None result
records the same effect/unjournaled reservation as the forcing residual before
returning false. This must force the escape-pc/legacy-replay path and prevent
replay from carrying a non-rerunnable residual’s side effect when the opcode
window appears clean.
…at generic handler entry The generic (non-explicit-raise) handler-landing arm gave the exception slot a `last_exc_value` producer for a distinct colour but left the operand in a register only. A blackhole/deopt handoff that resumes the interpreter at the handler reads the frame's value-stack array and finds an empty top slot, underflowing at PushExcInfo. Emit `setarrayitem_vable_r` after the `last_exc_value` read to write the exception to the same durable slot the explicit-raise arm's `getarrayitem_vable_r` reads; on the standard virtualizable it costs no compiled op and populates the operand image the blackhole hands back. Assisted-by: Claude
e92fddb to
a2cd5e6
Compare
1 participant
Single fix, rebased onto current
main.codewriter: materialize the caught exception on the value-stack slot at generic handler entry (task #52)
At a generic (non-explicit-raise) exception-handler entry the caught exception was read from the field but not materialized onto the value-stack slot the handler body expects, so a warmed-JIT
assertRaises→clear_frames/frame.clear()path could underflow and let aRuntimeErrorescape. Materialize the exception on the value-stack slot at generic handler entry (eval.rs handle_exceptionframe.push(exc)parity), satisfying the existing"exceptblock edge requires materialized exception pair"invariant.Scope note
This PR originally carried three asyncio-surfaced warmed-JIT fixes, but the other two — the locals-typed vable overlay and the unpublished single-frame vable-escape root stack — already landed on
mainvia #1051, so the branch was reduced to the one novel commit. It applies cleanly to currentmain(codewriter.rsis untouched since the base).Verification
Compiles on current
main; wascheck.py378/378 (dynasm/cranelift/wasm) when first committed. The escape it guards was a warmed-JIT Heisenbug that did not reproduce deterministically, so this is a land-safe fix for a latent gap; CI re-runs the full suite + parity review.— opened by Claude
Summary by CodeRabbit