Repository navigation
exceptions: GC-manage W_BaseException; FBW walker raise-across-call resume + setslice fold (#171) - #593
Conversation
WalkthroughThe changes add in-flight exception GC roots, update traceback handling for tracer-replaced exceptions, defer top-level JIT exception finishes, improve residual-call catch recovery, specialize eligible list slice stores, and use GC-aware exception allocation for regular and singleton instances. ChangesException lifetime and propagation
JIT exception dispatch
Virtual list slice specialization
Estimated code review effort: 4 (Complex) | ~60 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 36cc36b). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
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: 24e47a88ea
ℹ️ 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 Some(src_item) = (unsafe { pyre_object::w_list_getitem(value_obj, j) }) else { | ||
| unreachable!("setslice specialization: source index {j} has no element"); | ||
| }; | ||
| pyre_object::gc_roots::pin_root(src_item); | ||
| let Some(displaced) = (unsafe { pyre_object::w_list_getitem(list_obj, tgt_index) }) |
There was a problem hiding this comment.
When this specialized path runs with the moving/minor GC and w_list_getitem(value_obj, j) boxes an integer element, that allocation can relocate the target list. The code then immediately uses the stale list_obj for the displaced read, and later similarly uses pinned src_item/displaced without re-reading their forwarded shadow-stack slots after further boxing; a collection during slice assignment can therefore read or write through stale pointers. Re-read the forwarded operands/roots after each allocation before continuing.
Useful? React with 👍 / 👎.
`target[const_slice] = source` lowers to STORE_SUBSCR with a constant `slice` key (`store_subscr_fn`). When `source` is a virtualizable BUILD_LIST temp the residual force-materialises it every iteration. Intercept the StoreSubscr residual on the full-body-walk path for a same-length, step-1, non-negative constant-bounds, Integer-to-Integer slice with an exact-list target and source: guard both lists' class / exact w_class / Integer strategy and int_items length, then read the source elements through `getfield_gc(int_items)` + `getarrayitem_gc` and store them into the target's `int_items` block, eliding the `CALL_MAY_FORCE`. When the source is the freshly virtualized list the optimizer folds the reads against its `SetarrayitemGc` stores and the temporary DCEs. Falls through to the opaque residual (always byte-correct) for anything outside the gate: a non-constant / None / negative bound, a non-unit step, a length-changing slice, a non-Integer-storage target or source, or a list subclass. Gated on `PYRE_NEWLIST_VIRT` (default-on). Assisted-by: Claude
Generalize the guard-capture catch-resume from FOR_ITER_NEXT to any
residual CALL: rename GuardCaptureScope::foriter_next_catch_resume to
residual_call_catch_resume and set it whenever the residual CALL's pc is
covered by an exceptiontable entry (pycode::lookup_exceptiontable),
instead of only for is_for_iter_next.
A top-level uncaught raise is now recorded against
exit_frame_with_exception_descr rather than aborting the bridge:
fbw_terminate_with_raise stashes the exception box as an is_exception
FINISH payload behind FBW_FINISH_IS_EXCEPTION, and the Terminate arm in
full_body_walk_trace reads fbw_finish_is_exception() to build
TraceAction::Finish { exit_with_exception: true }.
Assisted-by: Claude
W_BaseException is allocated off-GC (w_exception_new_empty -> malloc_typed), so the collector never reaches it as a root, while its w_traceback chain is GC-managed oldgen. A non-moving major (enabled on wasm) could sweep that chain and leave exc.w_traceback dangling. Publish the exception being propagated in a thread-local IN_FLIGHT_EXCEPTION at record_application_traceback, and forward its w_traceback chain head from walk_global_prebuilt_roots (the registered extra-root walker) so the traceback stays reachable across a collection. Assisted-by: Claude
When bytecode_trace_after_exception replaces the propagating error (a tracer or signal handler raised during exception handling), record_application_traceback and exception_trace were passed the exception object captured before the trace hook ran, not the replacement. Re-derive the exception object from the possibly-replaced `err` after the trace block and use it for both, matching pyopcode.py:144-149 (`except OperationError as e: operr = e` then records and traces `operr`). Assisted-by: Claude
w_exception_new_empty allocates through try_gc_alloc_stable_raw (W_BASE_EXCEPTION_GC_TYPE_ID, non-moving oldgen) with note_alloc + write_barrier and a malloc_typed fallback, mirroring w_generator_new. The prebuilt singletons memory_error_singleton / standard_exc_instance route through a new w_exception_new_empty_immortal that keeps the malloc_typed path, since they are cached in GC-invisible OnceLocks and baked into JIT constant pools. walk_in_flight_exception forwards the exception object slot itself so the collector marks the GC-managed exception and recurses into its W_BASE_EXCEPTION_GC_PTR_OFFSETS children, and retains walk_raw_exception_roots for the off-GC (no-GC-installed) fallback. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 288bc59c05
ℹ️ 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 raw = crate::gc_hook::try_gc_alloc_stable_raw( | ||
| W_BASE_EXCEPTION_GC_TYPE_ID, | ||
| W_BASE_EXCEPTION_SIZE, | ||
| ); |
There was a problem hiding this comment.
When the GC hook is installed, this path makes every w_exception_new_empty() result an old-gen managed object. Callers such as w_exception_new() and PyError::to_exc_object() keep that pointer only in a Rust local and then call w_str_new()/w_list_new() to build args; a collection during those allocations cannot see the exception and can sweep it before w_exception_set_args(exc, ...) writes through it. Please pin/root the new exception, or build all allocating payloads before publishing it as a managed object.
Useful? React with 👍 / 👎.
| // catches it, it lives only in the in-flight Rust `PyError`, so a | ||
| // safepoint's non-moving major would otherwise sweep its old-gen | ||
| // traceback chain (`tstate->current_exception` parity). | ||
| crate::eval::set_in_flight_exception(w_exc_object); |
There was a problem hiding this comment.
This root is installed only when record_application_traceback runs, but handle_exception() materializes err.exc_object before calling bytecode_trace_after_exception, and a Python trace function or pending action can allocate and collect before execution reaches this line. With exceptions now old-gen managed, the original exception lives only in the PyError Rust local during that window and can be swept; root it before invoking the trace hook, and update the root if the hook replaces err.
Useful? React with 👍 / 👎.
youknowone
changed the title
… payload `inline_call_recursion_propagates_subraise_from_callee` and `raise_r_emits_guard_class_when_concrete_exc_pinned_in_shadow` predate the deferred-FINISH design (c691648): a top-level uncaught SubRaise now stashes the exception as an `is_exception` finish payload for the FBW Terminate arm's compile consumer instead of recording an inline FINISH in `walk()`. Assert the stashed payload (`fbw_finish_is_exception` + `fbw_finish_payload_take`) rather than an inline `Finish` op; `raise/r` still records its inline GuardClass. Assisted-by: Claude
…pply-loop The FBW setslice specialization applied the assignment as journaled setitems, pinning fresh boxes onto the shadow stack but reusing stale `list_obj` / `src_item` / `displaced` Rust locals after intervening `w_list_getitem` / `w_int_new` boxes. A moving minor collection rewrites the shadow-stack slot but not the Rust local, so those reads/writes could go through pre-move pointers. Reload every live ref after each boxing allocation before its next use — walker operands via `walker_concrete_ref_object`, pinned boxes via `shadow_stack_get` with the slot index captured just before the pin — matching the push_roots / pop_roots livevar reload discipline. Assisted-by: Claude
…exception `handle_exception` materialized the old-gen managed exception into the `PyError` local, then ran `bytecode_trace_after_exception` (arbitrary Python, allocating) before `record_application_traceback` published it as the in-flight GC root. A collection during the trace hook could sweep the still-unrooted exception. Publish `err.exc_object` via `set_in_flight_exception` before the trace hook; `record_application_traceback` re-publishes any tracer-replaced operr afterward. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/36cc36b6f824a69c03c2c8fe93c0ab73ed2f846d/pyre-jit-trace/src/trace.rs#L2971
When a multiframe/P2 outer continuation terminates via fbw_terminate_with_raise, this new finish_is_exception check is not used: drive_outer_continuation_and_map still consumes the same fbw_finish_payload_take() and always builds TraceAction::Finish { exit_with_exception: false } for non-void payloads. In that bridge/resume path, an uncaught raise across the resumed outer call is compiled as a normal return of the exception object instead of ExitFrameWithExceptionRef, so the caller observes a returned exception rather than a propagated raise.
https://github.com/youknowone/pyre/blob/36cc36b6f824a69c03c2c8fe93c0ab73ed2f846d/pyre-interpreter/src/eval.rs#L744-L745
The new TLS root is only ever assigned an exception object, so after a handled exception is pushed onto the handler stack / sys_exc_value, walk_in_flight_exception keeps the last propagated exception and its traceback graph rooted until another exception overwrites it. In long-running code that raises once and then continues, this permanently retains the traceback frames and their locals; clear this slot when propagation ends or when ownership moves to the normal exception roots.
ℹ️ 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".
![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-interpreter/src/eval.rs`:
- Around line 726-776: Replace the single overwriteable IN_FLIGHT_EXCEPTION Cell
with a scoped, nestable root stack or guard-based mechanism. Update
set_in_flight_exception and the corresponding propagation lifecycle so each
raised exception is pushed before handling and its prior root is restored or
removed when propagation ends, preserving outer PyError exceptions during nested
trace-hook handling. Ensure walk_in_flight_exception visits every active
in-flight exception and its traceback roots.
🪄 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: 9fe7ffd3-fcec-4d2d-9ec7-f5872440d04a
📒 Files selected for processing (5)
pyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/pytraceback.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-object/src/interp_exceptions.rs
| thread_local! { | ||
| /// The exception currently being raised / propagated up the Rust call | ||
| /// stack. Between the raising frame's `record_application_traceback` and | ||
| /// the frame that finally catches it, the exception is held only in the | ||
| /// Rust `PyError` value in flight — not on any frame's value stack or in | ||
| /// `ec.sys_exc_value` yet. `W_BaseException` is std::alloc-backed (outside | ||
| /// the managed nursery/old-gen), so the collector never reaches it as a | ||
| /// root and never traces its slots; a dispatch-loop safepoint running the | ||
| /// non-moving old-gen major would sweep its old-gen traceback chain from | ||
| /// under it. Mirrors `tstate->current_exception`: keep the in-flight | ||
| /// exception's traceback chain a GC root until the exception is caught. | ||
| static IN_FLIGHT_EXCEPTION: Cell<PyObjectRef> = const { Cell::new(pyre_object::PY_NULL) }; | ||
| } | ||
|
|
||
| /// Publish the exception now being raised / propagated so the GC root walker | ||
| /// keeps it (and its traceback chain) alive across a collection. Called from | ||
| /// `record_application_traceback`, the single chokepoint every raising frame | ||
| /// passes through. | ||
| pub fn set_in_flight_exception(exc: PyObjectRef) { | ||
| IN_FLIGHT_EXCEPTION.with(|c| c.set(exc)); | ||
| } | ||
|
|
||
| fn walk_in_flight_exception(visitor: &mut dyn FnMut(&mut majit_ir::GcRef)) { | ||
| IN_FLIGHT_EXCEPTION.with(|c| { | ||
| let exc = c.get(); | ||
| if exc.is_null() { | ||
| return; | ||
| } | ||
| // Forward the exception OBJECT slot itself. A GC-managed exception | ||
| // (oldgen-stable `w_exception_new_empty`) is marked here and the | ||
| // collector then recurses into its `W_BASE_EXCEPTION_GC_PTR_OFFSETS` | ||
| // slots via offset tracing, keeping the whole graph alive. The | ||
| // exception is oldgen-stable (non-moving), so the slot is not rewritten, | ||
| // but marking is what matters. Mirrors the value-stack walker, which | ||
| // forwards the on-stack exception slot then walks its raw children. | ||
| let slot = c.as_ptr(); | ||
| // SAFETY: `slot` points at the `Cell<PyObjectRef>`'s interior, which is | ||
| // valid for the duration of this closure. `PyObjectRef` and `GcRef` | ||
| // share layout (`*mut PyObject`). | ||
| unsafe { visitor(&mut *(slot as *mut majit_ir::GcRef)) }; | ||
| // Off-GC fallback: when the GC is not installed the exception is | ||
| // `malloc_typed`-immortal, so the visitor above is a no-op and the | ||
| // collector never traces its slots. Forward ALL its GC-managed children | ||
| // in place — the whole `W_BASE_EXCEPTION_GC_PTR_OFFSETS` set (args_w, | ||
| // w_cause, w_context, the w_traceback chain, w_dict, and the per-subclass | ||
| // slots). Read the object AFTER the visitor so a (hypothetically) moved | ||
| // exception is the live one. The slots are forwarded by address (not the | ||
| // barriered setter), so a moving child is relocated with no re-entry. | ||
| let exc = unsafe { *(slot as *const PyObjectRef) }; | ||
| unsafe { walk_raw_exception_roots(exc, visitor) }; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Use a nested, scoped root instead of one overwriteable Cell.
A trace hook can raise or catch another exception, replacing this slot while the outer exception still exists only in its Rust PyError. A collection during that nested handling can sweep the outer exception or traceback. Preserve nested roots and restore/remove them when each propagation ends.
🤖 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-interpreter/src/eval.rs` around lines 726 - 776, Replace the single
overwriteable IN_FLIGHT_EXCEPTION Cell with a scoped, nestable root stack or
guard-based mechanism. Update set_in_flight_exception and the corresponding
propagation lifecycle so each raised exception is pushed before handling and its
prior root is restored or removed when propagation ends, preserving outer
PyError exceptions during nested trace-hook handling. Ensure
walk_in_flight_exception visits every active in-flight exception and its
traceback roots.
Five commits stacked on
issue171, spanning full-body-walk (FBW) residualfolding and the exception GC lifecycle.
Commits
jit: fold same-length setslice on the FBW walker — intercept the
StoreSubscrresidual for a same-length, step-1, non-negative constant-boundsInteger→Integer slice with exact-list target/source, storing elements through
int_itemsand eliding theCALL_MAY_FORCE(falls through to the opaqueresidual for anything outside the gate). Gated
PYRE_NEWLIST_VIRT(default-on).jit: resume at caller's handler for raise-across-call on the FBW walker —
generalize the guard-capture catch-resume from
FOR_ITER_NEXTto any residualCALL whose pc is covered by an exceptiontable entry
(
residual_call_catch_resume), and record a top-level uncaught raise againstexit_frame_with_exception_descr(FINISH { exit_with_exception: true })instead of aborting the bridge.
interp: keep the in-flight exception's traceback chain a GC root —
publish the propagating exception in a thread-local
IN_FLIGHT_EXCEPTIONatrecord_application_tracebackand forward itsw_tracebackchain from theregistered extra-root walker, so a non-moving major cannot sweep the chain of
an off-GC exception.
interp: record/trace the tracer-replaced exception in handle_exception —
when
bytecode_trace_after_exceptionreplaces the propagating error, re-derivethe exception object from the possibly-replaced
errbefore recording thetraceback and tracing (
pyopcode.py:144-149).interp_exceptions: GC-manage W_BaseException via oldgen-stable alloc —
w_exception_new_emptyallocates throughtry_gc_alloc_stable_raw(
W_BASE_EXCEPTION_GC_TYPE_ID, non-moving oldgen) withnote_alloc+write_barrierand amalloc_typedfallback (mirroringw_generator_new);the prebuilt singletons stay immortal via
w_exception_new_empty_immortal.walk_in_flight_exceptionforwards the exception object slot so the collectorrecurses into
W_BASE_EXCEPTION_GC_PTR_OFFSETS, keepingwalk_raw_exception_rootsfor the off-GC fallback.Effect
Commit 5 makes
W_BaseExceptionsweepable, which fixes both the exception leak(off-GC
malloc_typedexceptions were never reclaimed) and agc.collect()crash at
majit-gc/src/trace.rs:934: a suspended generator's frame held aw_tracebacksubgraph through an off-GC exception the collector marked but neverrecursed into, so the subgraph was swept while reachable and
finalizer_childrenlater dereferenced a freed header. Native
test_exceptionswent from ~70–80%panics to 0/30.
Verification
pyre/check.py: dynasm 216/216, cranelift 216/216, wasm 215/215 (3/3 backend runs).test_exceptions: 0/30 panics (residual non-zero exit is a pre-existingast.parse-returns-code-object gap in traceback caret rendering, orthogonal to GC).pyre-objectexception unit tests 6/6 (singleton idempotency + GC tid).Follow-up
Raw-i64 JIT exception carriers (
BH_LAST_EXC_VALUE, backend_store_exceptioncells,
JIT_EXC_VALUE/TYPE),PENDING_CALL_ERROR, and a scopedIN_FLIGHTclear are not yet registered GC roots. They are unreachable today (pyre collects
only at safepoints, absent from the
to_exc_object→publish→guard windows, andIN_FLIGHTtransitively roots the last-raised exception), becoming relevant onlyunder free-threading. Tracked separately.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Performance