jit: close the FBW inline sub-walk double-apply class (gh#495) + branch-resume local binding fix (gh#498)#513
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR adds regression benchmarks for closure, mutation, iterator, exception, allocation, and nested-callee behavior. It also updates full-body-walk dispatch, vstack reconstruction, inline abort recovery, stack flushing, exception cleanup, GC rooting, and trace commitment logic. ChangesFBW regression coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)pyre/pyre-jit-trace/src/jitcode_dispatch.rsast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 896a463). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![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: 844c82ee87
ℹ️ 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".
| // kill-switch OFF the stash is ignored and Terminate | ||
| // aborts; the direct record preserves the legacy | ||
| // decline shape. | ||
| fbw_finish_concrete_exc_set(exc, ctx.last_exc_value_concrete); |
There was a problem hiding this comment.
When an uncaught SubRaise bubbles out of an inlined callee, the caller only receives the concrete exception in the exc_concrete payload; the caller ctx.last_exc_value_concrete is not seeded unless the exception is caught in this frame. With PYRE_FBW_RAISE_NO_REPLAY on, using the caller context here leaves the exception stash empty or stale, so a valid uncaught callee exception maps to Terminate::NoFinishPayload/legacy replay instead of compiling and returning the exception no-replay path.
Useful? React with 👍 / 👎.
youknowone
force-pushed
the
single-walker
branch
from
844c82e to
199f49f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 199f49f36c
ℹ️ 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 stack_overrides = FBW_ABORT_OUTER_STACK_OVERRIDES.with(|c| { | ||
| let mut slots = c.borrow_mut(); | ||
| std::mem::take(&mut *slots) | ||
| }); |
There was a problem hiding this comment.
In the nested-inline abort path, this take removes call_stack_overrides from FBW_ABORT_OUTER_STACK_OVERRIDES, which is the only place the new root walker visits them, and then run_perfn_walk passes the returned Vec into flush_walk_end_state_to_frame_with_stack_overrides. During that flush, boxing any Int/Float local before the operand-stack slots can allocate and trigger GC; because the override refs are now only in an untracked Rust Vec, nursery objects in the CALL stack can move without the overrides being updated, so the flush may write stale PyObjectRef pointers into the live frame. Keep the overrides in a rooted cell until after the flush (or add a scoped root for the local vector).
Useful? React with 👍 / 👎.
…sub-walk Assisted-by: Claude
Generalize the pre-call residual decline from write-tagged residuals to every residual that is not provably side-effect-free. The existing helper limits the gate to inline sub-walks, while writes_live_heap remains available for the FOR_ITER body-effect candidate path. Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Forward-commit nested inline residual-decline aborts after the walk has already executed a non-pure residual. Stash the top-level caller CALL pc and concrete call stack at inline-capture time, then use that outer coordinate for the abort flush instead of mapping the callee error pc through the outer jitcode tables. Assisted-by: Claude
Move the walk exception clear to residual-call entry before any dispatcher fold, symbolic decline, or concrete helper execution. This matches execute_varargs semantics and prevents a handled exception from surviving into a later linear catch_exception/L. Remove the catch_exception/L clear-and-continue workaround so an active exception there is again a strict invariant error. The success-arm clear was also removed because residual-call entry now performs the same WalkContext reset before every residual-call path; the existing success arm did not reset BH_LAST_EXC_VALUE, so there was no BH state to mirror. Assisted-by: Claude
Add synth parity oracles for gh#495 covering inline subwalk mutation with abort/no-exception paths, property mutation, reflected-add consumption, user-iterator mutation, nested callee mutation, caught and uncaught mutating raises, and return-value side effects for float and constant-int callees. Assisted-by: Claude
Add gh#498 synthetic parity cases for a closure freevar read, a nonlocal mutation, and a list-cell mutation feeding the same branch-resume local binding shape. Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
The preferred source is get_list_of_active_boxes's registers_r[index] (pyjitpl.py:222); the pcdep ownership proof bounds it to where the pyre register read means the same thing. PYRE_FBW_STACK_LIVEREG=0 restores the shadow-first order. Assisted-by: Claude
A top-level FBW portal walk that concretely ran a FOR_ITER body's residual effects before aborting with BranchGuardKeptStackUnsupported had no forward-flush leg in run_perfn_walk: committed stayed false and legacy snapshot-replay re-ran the walked iteration, double-applying the executed __next__ effects. Extend the PYRE_FBW_BRANCH_FLUSH or-pattern to also match BranchGuardKeptStackUnsupported. That variant aborts after the consumed item's body ran, so the walk's symbolic shadow end state already counted the iteration; flush it with flush_walk_end_state_to_frame without re-delivering the in-flight item. BranchGuardUnrestorableKeptStackPermanent keeps its Shape-A in-flight item delivery. Assisted-by: Claude
Prefer a real color-indexed Ref-bank value when a force-alived portal red is present in liveness with no semantic frame slot. Fall back to the named red field, then CONST_NULL, so top-level finish snapshots do not panic on dead-or-unseeded red fields. Add a synth regression covering a three-term heap-loaded or chain with a freshly allocated loop argument. Re-applies the collect_outer_active_boxes portion of the pre-rebase change without its exception-stash match arm reflow (that stash was retired in favour of the upstream FinishConcrete::Raise disposition). Assisted-by: Claude
youknowone
force-pushed
the
single-walker
branch
from
199f49f to
1967c06
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1967c060ce
ℹ️ 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".
| // false, and rollback would miss that concrete mutation. The helper no-ops | ||
| // on an empty session framestack, so top-level depth-1 behavior is unchanged. | ||
| if !provably_side_effect_free { | ||
| fbw_abort_nested_unjournaled_residual(ctx, op_pc)?; |
There was a problem hiding this comment.
When this new inline-subwalk decline fires for a may-force residual, the code has already run tracing_before_residual_call above and set the live virtualizable's token to TOKEN_TRACING_RESCALL; returning here skips the matching tracing_after_residual_call cleanup. In an inlined callee that hits a non-pure CallMayForce* residual, the walk aborts with the frame token still armed, so the next residual/force path can trip the non-NONE token invariant or mis-handle the virtualizable. Move this abort check before the token pre-call setup, or explicitly clear the token on this error path.
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
🤖 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.rs`:
- Around line 6322-6329: Before the early return in the
!provably_side_effect_free branch, restore the saved virtualizable heap pointer
and run tracing_after_residual_call to clear TOKEN_TRACING_RESCALL when
vable_obj_ptr is set. Then invoke fbw_abort_nested_unjournaled_residual(ctx,
op_pc)?, preserving the existing abort behavior while ensuring live-frame state
is restored first.
🪄 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
Run ID: 11a7f1e6-0cd0-4038-8c2b-bd18508be395
📒 Files selected for processing (19)
pyre/bench/synth/closure_freevar_branch_list_cell.pypyre/bench/synth/closure_freevar_branch_nonlocal.pypyre/bench/synth/closure_freevar_branch_readonly.pypyre/bench/synth/const_int_return_side_effect.pypyre/bench/synth/float_return_side_effect.pypyre/bench/synth/foriter_exempt_nested_foriter.pypyre/bench/synth/foriter_exempt_shared_generator.pypyre/bench/synth/inline_subwalk_mutating_residual_abort.pypyre/bench/synth/inline_subwalk_mutating_residual_noexc.pypyre/bench/synth/inline_subwalk_property_mutates.pypyre/bench/synth/inline_subwalk_radd_consumed.pypyre/bench/synth/inline_subwalk_user_iterator.pypyre/bench/synth/mutate_then_raise_caught.pypyre/bench/synth/mutate_uncaught_raise_delivery.pypyre/bench/synth/nested_callee_chain_mutation_abort.pypyre/bench/synth/or_chain_fresh_alloc_arg.pypyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rs
| // Inside an inline sub-walk, decline before any residual that is not | ||
| // provably side-effect-free. Ref-result getters/dunders/user `__next__` | ||
| // can mutate live heap through user frames while `writes_live_heap` is | ||
| // false, and rollback would miss that concrete mutation. The helper no-ops | ||
| // on an empty session framestack, so top-level depth-1 behavior is unchanged. | ||
| if !provably_side_effect_free { | ||
| fbw_abort_nested_unjournaled_residual(ctx, op_pc)?; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pyre/pyre-jit-trace/src/jitcode_dispatch.rs"
echo "== Relevant line ranges =="
sed -n '6200,6425p' "$FILE" | cat -n
echo
echo "== Search for token/restore helpers =="
rg -n "tracing_before_residual_call|tracing_after_residual_call|saved_vable_heap_ptr|set_virtualizable_heap_ptr|fbw_abort_nested_unjournaled_residual|LoopBearingCalleeInlineUnsupported|VableEscapedDuringResidualCall|provably_side_effect_free" "$FILE"
echo
echo "== Nearby control-flow contexts =="
sed -n '6228,6410p' "$FILE" | cat -n
echo
echo "== Definitions/usages of residual-call helpers in repo =="
rg -n "fn .*tracing_before_residual_call|fn .*tracing_after_residual_call|struct .*Virtualizable|saved_vable_heap_ptr|fbw_abort_nested_unjournaled_residual" pyre/pyre-jit-trace/srcRepository: youknowone/pyre
Length of output: 38531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pyre/pyre-jit-trace/src/jitcode_dispatch.rs"
echo "== fbw_abort_nested_unjournaled_residual definition =="
sed -n '8912,8965p' "$FILE" | cat -n
echo
echo "== Other nested-abort call sites for comparison =="
sed -n '13295,13350p' "$FILE" | cat -n
echo
sed -n '16190,16240p' "$FILE" | cat -n
echo
sed -n '21380,21630p' "$FILE" | cat -n
echo
echo "== Search for virtualizable cleanup in abort paths =="
rg -n "set_virtualizable_heap_ptr\\(|tracing_after_residual_call\\(|TOKEN_TRACING_RESCALL|TOKEN_NONE|virtualizable_heap_ptr\\(" pyre/pyre-jit-trace/src/jitcode_dispatch.rs pyre/pyre-jit-trace/src/trace_opcode.rs pyre/pyre-jit-trace/src/*.rsRepository: youknowone/pyre
Length of output: 28666
Restore the virtualizable state before aborting nested inline residuals
When vable_obj_ptr is set, this early ? skips both set_virtualizable_heap_ptr(saved_vable_heap_ptr...) and tracing_after_residual_call, leaving TOKEN_TRACING_RESCALL stranded on the live frame. That can trip the next residual-call token assert. Clear the token and restore the saved heap pointer before aborting.
🤖 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.rs` around lines 6322 - 6329, Before
the early return in the !provably_side_effect_free branch, restore the saved
virtualizable heap pointer and run tracing_after_residual_call to clear
TOKEN_TRACING_RESCALL when vable_obj_ptr is set. Then invoke
fbw_abort_nested_unjournaled_residual(ctx, op_pc)?, preserving the existing
abort behavior while ensuring live-frame state is restored first.
Forward the nested-inline abort outer-frame stash (FBW_ABORT_OUTER_STACK_OVERRIDES) and the active walk session's InlineParentFrame.call_stack_overrides slots as GC roots through the captured FbwStoreJournalRootArea, matching the quiesced-area model. Both stashes hold nursery-resident PyObjectRefs across residual-call allocations and are populated by default-on paths (PYRE_FBW_NESTED_RESID_ABORT, PYRE_FBW_INLINE_MULTIFRAME). Assisted-by: Claude
youknowone
force-pushed
the
single-walker
branch
from
1967c06 to
c268598
Compare
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/jitcode_dispatch.rs`:
- Around line 8958-8964: The shared borrow created by
fbw_abort_outer_stack_overrides_with must not remain active during GC-capable
work. In pyre/pyre-jit-trace/src/jitcode_dispatch.rs:8958-8964, copy or
otherwise materialize the override values within a short borrow, release it,
then invoke the callback; apply the same borrow-lifetime separation at the
sibling site pyre/pyre-jit-trace/src/jitcode_dispatch.rs:9042-9047 and
pyre/pyre-jit-trace/src/trace.rs:2100-2110, ensuring
flush_walk_end_state_to_frame_with_stack_overrides runs only after the override
collection is no longer borrowed.
🪄 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
Run ID: 3df0d584-bbfc-4265-8333-50d20bfb8615
📒 Files selected for processing (2)
pyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/trace.rs
…eclines Abort override rooting: fbw_abort_outer_resume_take now returns only the resume pc. The outer-caller stack overrides stay in FBW_ABORT_OUTER_STACK_OVERRIDES (rooted by the #447 area walker) and are read in place via fbw_abort_outer_stack_overrides_with during the walk-end flush, then cleared with fbw_abort_outer_stack_overrides_clear. The flush boxes Int/Float locals, an allocation that can move the nursery-resident override refs; reading from the rooted cell lets a minor collection forward the very refs the flush writes. Residual vable token: move the tracing_before_residual_call arm from before the decline gates to after fbw_abort_nested_unjournaled_residual, so a residual declined by the inline-subwalk gate no longer strands TOKEN_TRACING_RESCALL on the active virtualizable. tracing_before now pairs with tracing_after only for residuals that proceed. Assisted-by: Claude
youknowone
force-pushed
the
single-walker
branch
from
c268598 to
f35ca5f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f35ca5f966
ℹ️ 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".
| opref: OpRef, | ||
| ) -> Option<pyre_object::PyObjectRef> { | ||
| match ctx.trace_ctx.concrete_of_opref(opref) { | ||
| Some(Value::Ref(r)) => Some(r.as_usize() as pyre_object::PyObjectRef), |
There was a problem hiding this comment.
When the caller's CALL stack contains a ref whose concrete value is only the GcRef::NO_CONCRETE sentinel (for example, a freshly allocated argument), this helper converts that sentinel to a PyObjectRef. The nested-inline abort flush then treats the override as complete and writes the sentinel into locals_cells_stack_w instead of declining, so the interpreter resumes with an invalid object pointer. Filter GcRef::NO_CONCRETE here (and in the color helper) the same way other concrete_of_opref callers do.
Useful? React with 👍 / 👎.
1 participant
Closes the gh#495 double-apply epic and gh#498, plus regression guards and gated Slice-A infra. 16 commits on
main.gh#495 — FBW inline sub-walk concrete-residual double-apply class
fbw_abort_nested_unjournaled_residual), so concrete heap effects outside the FBW journals can never be re-applied by entry replay.FBW_EXECUTED_NONPURE_RESIDUALlatch; const-int/float finish stashes; exception forward-delivery (exit_frame_with_exception, pyjitpl.py:2557 parity); abort-flush of executed-effect nested aborts with caller-CALL-pc + concrete stack overrides (rebuild_from_resumedata resume.py:1042 parity); GC-rooting for the stashes.pyre/bench/synth/inline_subwalk_*et al., each# gh#495 guard:).gh#498 — inlined-closure freevar + branch miscompile
get_list_of_active_boxesreadsregisters_r[index]— the live color). Fix: prefer the live walk register when the snapshot carries a guard pc.closure_freevar_branch_{readonly,nonlocal,list_cell}).Task #1 (codex-review §3) — ForIterNext exemption audit
PYRE_FBW_NESTED_RESID_ABORT=0on older bases. 2 synth guards (foriter_exempt_*) pin the masking machinery.Slice A0 (gated OFF)
PYRE_FBW_CALLEE_VSTACK(default OFF). A corpus-wide decline census found no reachable target for the flip on the current base, so it stays inert re-evaluation infra (177/177 in both modes).Verification
pyre/check.py --backend dynasm: ALL PASSED 177/177 at tip (also withPYRE_FBW_CALLEE_VSTACK=1); gh#495/JIT: inlined-closure freevar read + branch miscompile — freevar cell aliases the branch-arm return slot (wrong value / TypeError crash) #498 repro batteries oracle-equal, includingPYPY_GC_NURSERYstress.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
nonlocalbehavior, mutation/raise flows, and additional inline sub-walk edge cases.