jit(fbw): point the multi-frame blackhole adopt at the live frame and name its flip blocker; root the grabbed guard exception - #830
Merged
Merged
Conversation
`try_adopt_multi_frame_blackhole` declines when the recovered blackhole chain's root does not equal `cf_addr`. Its comment attributed that to a chain rooted at an intermediate frame, and named the `jit.virtual_ref` emit at the inline push as the prerequisite for closing it. Measured with the in-tree `[s2-gate]` / `[s2-build-decline]` diagnostics under `PYRE_FBW_MULTIFRAME=1 PYRE_FBW_DEBUG_ABORT=1`, on a `while` loop calling an inlined callee that calls a zero-argument `sys._getframe`: the latch site is reached, `build_multi_frame_miframe` returns an image at depth 2, and all five decline events print `per_frame[0]` equal to the live frame address and different from `cf_addr`. `per_frame[0]` is read from the trace's frame register, which `seed_virtualizable_boxes` bakes against `live_vable_frame_addr` (`state.rs`); that field is set from a real `&PyFrame` at both production entries (`call_jit.rs`, `eval.rs`) and falls back to the snapshot address only on the unit-test path. `cf_addr` is the `snapshot_for_tracing` copy. The two are different representations of the same frame, so for this shape the comparison declines unconditionally and no `VIRTUAL_REF` emit is involved. Rewrites the comment to state that, and records the measurement in gate-triage §1d in place of the claim that building such a benchmark was the prerequisite for the materialization and per-frame-vable items: the build side already succeeds, so those items sit below this comparison. Comment and documentation only; no behavior change. Assisted-by: Claude
…rame `try_adopt_multi_frame_blackhole` compared the recovered chain's root against `cf_addr`, the `snapshot_for_tracing` copy, while `per_frame[0]` is recovered from the trace's frame register, whose root vable identity `seed_virtualizable_boxes` bakes against `live_vable_frame_addr`. Both production entries set that field from a real `&PyFrame`, so the two operands were never equal and the adopt declined every chain, including ones rooted at the walked frame. Thread the live root address in and use it for the two uses that ask an identity question: the comparison, and the root `f_backref` operand whose `ptr::eq` skip has to fire for `frames[0]`. The other uses keep the snapshot. `apply_blackhole_crn` writes the image the portal epilogue propagates, and `concrete_nlocals` / the `execution_context` read are address-agnostic. The vable root and `stack_base` passed to `drive_multi_frame_blackhole` are dead on this path, because `convert_and_run_from_pyjitpl` overrides both per level from `per_frame`; noted rather than removed. That override is also why the adopt now folds the live frame into the snapshot before the CRN write: frame 0's level runs against `per_frame[0]`, so its `setfield_vable` stores land on the live frame, while the epilogue copies the snapshot's whole locals array back over it (`restore_resume_state_from`) and would revert any of those stores the CRN write does not cover. Adds `synth/getframe_while_inlined_callee_subwalk`, the first fixture that reaches the vable-escape latch inside an inline sub-walk. The corpus had no coverage: every other `getframe_*` fixture drives with `for`, and with a FOR_ITER item in flight `fbw_abort_nested_unjournaled_residual` declines the callee's nested residual before `execute_residual_call` runs. All of this sits behind `PYRE_FBW_MULTIFRAME`, default off. With the gate on, the new fixture reports 5 `BUILT multi-frame depth=2` and 5 `adopted multi-frame terminal` with zero declines, against 0 adopts before, and prints the same result as CPython and PyPy; the synthetic corpus under the gate is 308 passed with no correctness failure. Default gates: `pyre-jit-trace` and `majit-metainterp` unit tests pass; `check.py --backend dynasm` 322 passed with `nested_loop` discriminated as machine load (isolated 1.7x / 1.8x / 1.9x against its 2x gate). Assisted-by: Claude
…ild decline The multi-frame blackhole path had one fixture. Adds three, each verified to reach the vable-escape latch inside an inline sub-walk on a clean build: - `getframe_while_captured_frame_outlives_call` keeps the callee's frame past the loop and reads its `f_back`, which is the only way to observe a chain root linked to the `snapshot_for_tracing` copy rather than the live frame: the snapshot is freed at walk end. - `getframe_while_caller_locals_across_subwalk` carries two caller locals across every iteration. Frame 0's blackhole level writes the live frame while the resume-state write targets the snapshot, so a slot the resume-state write does not cover would revert and change the printed totals. - `getframe_while_subwalk_decline_shapes` pins the two shapes the build refuses, so a decline cannot silently become a wrong answer. Coverage across the corpus goes from 0 multi-frame adopts to 20, plus 10 pinned declines. Both refused shapes -- an exception handler around the inlined call, and two nested inlined levels -- share one cause, instrumented rather than inferred: a ref color that is live at the caller's post-call coordinate holds `ConcreteValue::Null`. Neither is the not-yet-produced result slot. Neither involves the bridge parent-frame constructors, since every latch event reports `not_bridge=true` and the latch requires `!is_bridge_trace`. `Null` is the untracked sentinel and is deliberately distinct from `Ref(PY_NULL)`, an uninitialised local, so accepting it would fabricate a parent frame; the decline is correct and closing these shapes is the outer-locals materialization already named in the ledger. The gate-triage text that attributed this to the bridge constructors was wrong and is corrected. `check.py` on the full corpus is 326/326 on dynasm and cranelift, with `PYRE_FBW_MULTIFRAME` both off and on. Assisted-by: Claude
With the multi-frame adopt working, an inlined callee that reads its CALLER's
frame through `sys._getframe(1)` is neither declined nor correct. The chain
runs innermost-first and each level runs against its own live frame, but an
outer level's recovered locals are still only in its blackhole registers at
that moment; nothing writes them back before the inner level re-executes.
Forcing the gate on:
f = _gf(1); return x + f.f_locals["bias"] -> KeyError: 'bias'
f = _gf(1); return x + (1 if f.f_code.co_name == ...) -> 29995, not 30000
The count is exact -- one lost iteration per multi-frame adopt, 5 adopts and 5
missing. Both are correct with the gate off, which is the default.
Adds `synth/getframe_while_outer_frame_read_from_subwalk` as the flip's
acceptance test: it passes today and fails loudly if the gate is flipped before
the outer-frame materialization lands. The gate helper's own comment named this
condition correctly; the ledger now carries the measurement instead of the
earlier reading that put the materialization downstream of the adopt.
Everything else previously thought to block the flip has been measured and does
not. The full corpus is 326/326 on dynasm and cranelift with the gate both off
and on. The blast radius is exactly `inline_subwalk = true` at a vable escape:
the latch is an `if`/`else if` whose single-frame arm requires
`!inline_subwalk`, so with the gate off that condition latches nothing and falls
to legacy escape/replay.
Bench and documentation only; no behavior change. `check.py` is 327/327 on
cranelift, and 326 passed on dynasm with `raise_catch` at 1.7x against its 1.5x
gate, which isolated three times gives 1.6x / 1.4x / 1.1x -- the recorded
boundary behavior of that bench, and untouchable by a commit that adds a
fixture and markdown.
Assisted-by: Claude
…p blocker
Three things, all found by measuring shapes the corpus does not contain.
1. The live->snapshot fold ran only in the ContinueRunningNormally arm, but
every adopted arm sets WALK_END_FLUSH_COMMITTED, and the portal epilogue
then copies the snapshot's whole locals array onto the live root
(`restore_resume_state_from`). A `DoneWithThisFrame*` or
`ExitFrameWithExceptionRef` terminal therefore had frame 0's blackhole
stores reverted, and for the exception terminal the traceback keeps the root
frame reachable, so the stale copy is observable through `tb_frame.f_locals`
long after the walk. Hoisted above the match.
2. Adds a frame-identity collapse guard: every recovered level must be a
distinct frame and only `frames[0]` may be the walked frame. A level whose
frame register resolved to the root would make the relink write an
`f_backref` cycle and run an inner level against the root's virtualizable.
No producer is known; the failure would be silent, so decline instead of
relying on the absence.
3. Replaces the acceptance fixture added in the previous commit, whose header
named the wrong mechanism. The blocker is not outer-locals staleness. The
walker executes residuals concretely while an inline push never runs the
interpreter's call sequence, so `ec.topframeref` still names the CALLER
while an inlined callee body runs, and a `sys._getframe` that is itself the
escaping residual reads the wrong frame at walk time. Adopting commits that
answer where legacy escape/replay discards it:
_gf().f_code.co_name -> "main", not "leaf"
_gf(1).f_code.co_name -> "<module>", not "main"
_gf(1).f_locals["k"] -> KeyError
One wrong iteration per adopt, 5 adopts and 5 wrong in each part of
`synth/getframe_while_escaping_read_frame_identity`. A `sys._getframe`
executed after the escape, inside the blackhole, is correct, and an
in-blackhole read of a caller local mutated earlier in the same iteration
matches CPython and PyPy. So the original decline comment was right that an
inline-push `enter` is the prerequisite and wrong only about which check it
gated; that sentence is restored alongside the corrected one.
Also recorded: with a side-effecting inlined callee in a `while` loop that
returns from inside the loop, the default path runs the side effect ~5.2k times
too often -- the known trace-abort double-run class -- while the adopt produces
the exact count.
`PYRE_FBW_MULTIFRAME` stays default-off. Unit tests pass; `check.py` is 327/327
on dynasm and on cranelift.
Assisted-by: Claude
…doff `grab_exc_value` (llmodel.py:240) reads `jf_guard_exc` off the deadframe and drops the jitframe, which held the collector's only reference to that exception (`jitframe_trace`). The handoff then decodes resume data and rebuilds virtuals through the blackhole allocator, so the value spans an allocating window as a bare `i64` that the precise collector cannot see. RPython's `grab_exc_value` result is a shadowstack-rooted local across the same span. Add `GuardExcRoot`, which parks the value in a `GUARD_EXC_VALUE` thread-local, and a frontend root walker that marks the carrier and forwards its young child slots — the same shape as `walk_jit_exc_value` / `walk_bh_last_exc_value`. Park at `handle_fail`, `blackhole_resume_via_rd_numb` (which also covers the CALL_ASSEMBLER caller) and `back_edge_internal`. A `handle_fail` census over the 339-file `bench/` + `bench/synth/` corpus counts 732,660 guard failures, of which 34,790 carry a live exception into the window — 170 of those on the bridge route, which is the `bridge_guard_exc` read itself. gate-triage §1e records the census and retires `bridge_guard_exc` from the `PYRE_CARRIER_EXC_RESUME` pre-flip list: the same pointer drives the default blackhole resume, so the gate never bounded the exposure, and the seed site is reachable rather than inert. The walker is not load-bearing on any measured workload. Under `MAJIT_GC_STRESS` on a `gc_stress` build, the five benches that produce live guard exceptions pass identically with it registered and suppressed — on the residual-raise path the same exception is still parked in `BH_LAST_EXC_VALUE`, which is already rooted. This is a parity fix at a reachable site, not a demonstrated bug fix. Assisted-by: Claude
…ce correctly The row called the gap "the unconditional `execute_ll_raised` exception assign", which is not what diverges. `seed_bridge_standing_exception_from_current` is ungated and already mirrors upstream's branch — it assigns `last_exc_value` / `last_exc_box` when it finds an exception and clears all four exception slots when it does not. What differs is the source: `_prepare_exception_resumption` takes the exception the failing guard carried (`cpu.grab_exc_value`), while pyre takes `sym.current_exc_value` falling back to `get_current_exception()` — the execution context's current exception, i.e. the `sys.exc_info()` mirror. The gate is a back-channel into that function: it writes `guard_exc` into `current_exc_value` so the ungated code picks it up, and its `is_null` conjunct avoids clobbering a live `sys.exc_info` value, which suppresses the injection exactly when the EC already holds an exception. Measured with the gate forced on: dynasm 334/334, byte-identical to the default, and the seven benches that produce a live guard exception individually identical, despite the seed site being entered 170 times. Record that, plus two further deltas in the same function to settle before any flip: it early-returns when `last_exc_box` is already set, and it sets `class_of_last_exc_is_const = true` where the `_prepare_exception_resumption` path reaches `execute_ll_raised` with the default `constant=False`. Assisted-by: Claude
The corpus grew from 334 to 336 files when the branch was rebased, so the two recorded gate measurements no longer name the corpus they were taken against. Re-measured on the rebased tree: dynasm, default 335/336 dynasm, PYRE_FBW_MULTIFRAME=1 336/336 dynasm, PYRE_CARRIER_EXC_RESUME=1 336/336 cranelift, default 336/336 The one default-run difference is `nested_loop` at 2.0x against its 2x perf gate, measured while eight to eleven cargo builds from sibling worktrees were running. Interleaved against pypy with the same startup subtraction check.py uses, seven rounds give 1.54x / 1.62x / 1.64x / 1.65x / 1.66x / 1.68x / 1.69x, and the bench passes in the two gated runs above. Assisted-by: Claude
WalkthroughThe change roots failed-guard exceptions through blackhole and bridge handoffs, updates multiframe adoption to use live root frame state, and adds synthetic frame-behavior benchmarks plus gate-triage measurements. ChangesBlackhole handoff correctness
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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: 08b29da1be
ℹ️ 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".
| impl GuardExcRoot { | ||
| pub fn park(exc: i64) -> Self { | ||
| Self { | ||
| prev: GUARD_EXC_VALUE.with(|cell| cell.replace(exc)), |
There was a problem hiding this comment.
When a bridge handoff deopts while an outer handoff is still active, replace removes the outer exception from the only registered root for the entire nested handoff. Restoring prev on drop is too late if the nested resume decode allocates and triggers a collection, because the outer exception remains only in raw i64 state and can become dangling. Store all active handoff exceptions in a rooted stack rather than a single replaceable cell.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| pyre_interpreter::eval::register_pyframe_root_walker(); | ||
| majit_gc::shadow_stack::register_extra_root_walker(walk_jit_exc_value); | ||
| majit_gc::shadow_stack::register_extra_root_walker(walk_bh_last_exc_value); | ||
| majit_gc::shadow_stack::register_extra_root_walker(walk_guard_exc_value); |
There was a problem hiding this comment.
In a multithreaded run, register_extra_root_walker invokes this callback on the collecting thread, so it reads that thread's GUARD_EXC_VALUE rather than the cell belonging to a stopped mutator currently handling a guard failure. If another thread starts a collection during that handoff, the parked exception is omitted from the root set and may be reclaimed; the existing pending-exception and BH_LAST_EXC_VALUE carriers solve this by also exposing their TLS cells through a per-mutator root area, which this new carrier needs as well.
AGENTS.md reference: AGENTS.md:L148-L162
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: 5
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/src/eval.rs (1)
3319-3357: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMove the guard-exception root out of TLS.
GUARD_EXC_VALUEis interpreter-owned, identity-sensitive, GC-relevant semantic state, yetGuardExcRoot::parkstores it in TLS. The cited upstream model is a shadowstack-rooted local, not TLS; retain the exception on the established interpreter/GC-root owner and have the walker traverse that owner instead.
pyre/pyre-jit/src/eval.rs#L3319-L3357: replace the TLS root walker with traversal of the durable owner.pyre/pyre-jit/src/call_jit.rs#L1905-L1909: pass or scope that owner across blackhole resume rather than parking into TLS.majit/majit-metainterp/src/trace_ctx.rs#L541-L544: update the ownership contract after moving the carrier.As per coding guidelines, “Do not use TLS for process-global, interpreter-owned, identity-sensitive, semantic, registry, cache, or GC-relevant runtime state.”
🤖 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/src/eval.rs` around lines 3319 - 3357, Move GUARD_EXC_VALUE ownership out of TLS and into the established interpreter/GC-root owner, preserving the guard exception across blackhole resume without changing its identity semantics. In pyre/pyre-jit/src/eval.rs lines 3319-3357, replace walk_guard_exc_value’s TLS traversal with traversal of that durable owner; in pyre/pyre-jit/src/call_jit.rs lines 1905-1909, pass or scope the owner through blackhole resume instead of parking it in TLS; and in majit/majit-metainterp/src/trace_ctx.rs lines 541-544, update the ownership contract to reflect the moved carrier.Source: Coding guidelines
🤖 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 `@majit/majit-metainterp/src/blackhole.rs`:
- Around line 310-324: Replace the thread-local `GUARD_EXC_VALUE` storage with
the interpreter-owned root registry/stack or another established shared-root
mechanism used by the GC root walker. Update the associated `grab_exc_value`
handoff and root-walking access so the parked exception remains visible across
the blackhole transition without relying on TLS.
- Around line 326-347: Update GuardExcRoot::park and its Drop implementation to
use a GC-visible root stack or registry instead of storing the previous
exception as an unregistered i64. Ensure every active nested exception root is
walked and updated during collection, including nested park(0), and restore the
GC-updated prior value when each GuardExcRoot is dropped.
In `@majit/majit-metainterp/src/jitdriver.rs`:
- Around line 3480-3484: Update run_back_edge_generic to install a
GuardExcRoot::park for result_exc immediately after extracting it, and keep the
RAII guard alive through the allocation-capable bridge and blackhole resume
paths, including both prepare_resume_from_failure call sites. Preserve the
existing back_edge_internal protection and do not drop or move the guard before
handoff completes.
In `@pyre/bench/synth/getframe_while_captured_frame_outlives_call.py`:
- Around line 16-22: Replace the module-level scalar `kept` assignment in `leaf`
with a mutable holder such as `kept_box`, updating its element with `_gf()` and
removing the `global kept` declaration. Update the post-`main()` read to use the
holder so the captured frame remains alive without rebinding a module global.
In `@pyre/gate-triage.md`:
- Line 471: Update the PYRE_FBW_BLACKHOLE_RESUME retirement condition in
pyre/gate-triage.md to reflect that the multi-frame root-mismatch decline is
resolved. Keep the gate active until the remaining escaping sys._getframe
identity wrong-answer is fixed and its acceptance fixture passes.
---
Outside diff comments:
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 3319-3357: Move GUARD_EXC_VALUE ownership out of TLS and into the
established interpreter/GC-root owner, preserving the guard exception across
blackhole resume without changing its identity semantics. In
pyre/pyre-jit/src/eval.rs lines 3319-3357, replace walk_guard_exc_value’s TLS
traversal with traversal of that durable owner; in pyre/pyre-jit/src/call_jit.rs
lines 1905-1909, pass or scope the owner through blackhole resume instead of
parking it in TLS; and in majit/majit-metainterp/src/trace_ctx.rs lines 541-544,
update the ownership contract to reflect the moved carrier.
🪄 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: d71da678-632e-4df5-9907-337975a32425
📒 Files selected for processing (12)
majit/majit-metainterp/src/blackhole.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/trace_ctx.rspyre/bench/synth/getframe_while_caller_locals_across_subwalk.pypyre/bench/synth/getframe_while_captured_frame_outlives_call.pypyre/bench/synth/getframe_while_escaping_read_frame_identity.pypyre/bench/synth/getframe_while_inlined_callee_subwalk.pypyre/bench/synth/getframe_while_subwalk_decline_shapes.pypyre/gate-triage.mdpyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rs
Comment on lines
+310
to
+324
|
|
||
| /// llmodel.py:240 `grab_exc_value(deadframe)`: the exception a failing | ||
| /// guard carried, parked for the bridge / blackhole handoff. | ||
| /// | ||
| /// Grabbing the value reads `jf_guard_exc` off the deadframe and drops the | ||
| /// jitframe, which was the collector's only handle on the exception | ||
| /// (`jitframe_trace`). The handoff then reconstructs the resume state | ||
| /// through the blackhole allocator before anything re-roots the value, so | ||
| /// in that window the exception — and the young `args` / `__traceback__` | ||
| /// reachable only through it — live behind a bare `i64`. RPython's | ||
| /// `grab_exc_value` result is a shadowstack-rooted local across the same | ||
| /// span; pyre has no GC transform, so the frontend registers a root walker | ||
| /// over this cell instead. | ||
| pub static GUARD_EXC_VALUE: std::cell::Cell<i64> = const { std::cell::Cell::new(0) }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not use TLS for GC-relevant exception state.
GUARD_EXC_VALUE is directly consumed by the GC root walker, so this is GC-relevant interpreter state. Move it to the interpreter-owned root registry/stack or another established shared-root mechanism; otherwise root visibility depends on thread-local storage and can miss the parked exception.
As per coding guidelines, TLS is prohibited for GC-relevant runtime state.
🤖 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/blackhole.rs` around lines 310 - 324, Replace the
thread-local `GUARD_EXC_VALUE` storage with the interpreter-owned root
registry/stack or another established shared-root mechanism used by the GC root
walker. Update the associated `grab_exc_value` handoff and root-walking access
so the parked exception remains visible across the blackhole transition without
relying on TLS.
Source: Coding guidelines
Comment on lines
+326
to
+347
| /// Park a grabbed guard exception in [`GUARD_EXC_VALUE`] for the duration of | ||
| /// one handoff. | ||
| /// | ||
| /// Restores the previous value on drop rather than clearing, so a nested | ||
| /// handoff (a bridge trace that itself deopts) unwinds to the exception its | ||
| /// caller is still carrying. | ||
| pub struct GuardExcRoot { | ||
| prev: i64, | ||
| } | ||
|
|
||
| impl GuardExcRoot { | ||
| pub fn park(exc: i64) -> Self { | ||
| Self { | ||
| prev: GUARD_EXC_VALUE.with(|cell| cell.replace(exc)), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Drop for GuardExcRoot { | ||
| fn drop(&mut self) { | ||
| GUARD_EXC_VALUE.with(|cell| cell.set(self.prev)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Nested GuardExcRoots can restore a collected pointer.
park replaces the only walked TLS slot and stores prev as an unregistered i64. During a nested handoff, the outer exception is therefore invisible to the GC; if allocation collects it, Drop restores a stale pointer. A nested park(0) has the same failure mode. Use a root stack/registry that walks and updates every active exception root.
🤖 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/blackhole.rs` around lines 326 - 347, Update
GuardExcRoot::park and its Drop implementation to use a GC-visible root stack or
registry instead of storing the previous exception as an unregistered i64.
Ensure every active nested exception root is walked and updated during
collection, including nested park(0), and restore the GC-updated prior value
when each GuardExcRoot is dropped.
Comment on lines
+3480
to
+3484
| // The deadframe root died with the grab and the reconstruction | ||
| // below allocates through the blackhole allocator, so hold the | ||
| // exception where the frontend's root walker can reach it until | ||
| // `prepare_resume_from_failure` hands it to the blackhole. | ||
| let _guard_exc_root = crate::blackhole::GuardExcRoot::park(guard_exc); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Root the exception in every guard-failure handoff path.
This protects back_edge_internal, but run_back_edge_generic still drops its result after extracting result_exc and then performs allocation-capable resume work at Lines 5925 and 6013 without a GuardExcRoot. A GC during that work can reclaim or move the exception before it reaches prepare_resume_from_failure. Install the same RAII root immediately after extracting result_exc, keeping it alive through both bridge and blackhole paths.
🤖 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 3480 - 3484, Update
run_back_edge_generic to install a GuardExcRoot::park for result_exc immediately
after extracting it, and keep the RAII guard alive through the
allocation-capable bridge and blackhole resume paths, including both
prepare_resume_from_failure call sites. Preserve the existing back_edge_internal
protection and do not drop or move the guard before handoff completes.
Comment on lines
+16
to
+22
| kept = None | ||
|
|
||
|
|
||
| def leaf(x): | ||
| global kept | ||
| kept = _gf() | ||
| return x + 1 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Avoid rebinding the module global from leaf.
global kept triggers Ruff PLW0603. Preserve module-lifetime storage with a mutable holder (for example, kept_box[0] = _gf()) and read that holder after main() so the captured frame remains alive without the warning.
Also applies to: 34-35
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 20-20: Using the global statement to update kept is discouraged
(PLW0603)
🤖 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/bench/synth/getframe_while_captured_frame_outlives_call.py` around lines
16 - 22, Replace the module-level scalar `kept` assignment in `leaf` with a
mutable holder such as `kept_box`, updating its element with `_gf()` and
removing the `global kept` declaration. Update the post-`main()` read to use the
holder so the captured frame remains alive without rebinding a module global.
Source: Linters/SAST tools
| | var | subsystem | retire when | | ||
| |---|---|---| | ||
| | PYRE_FBW_BLACKHOLE_RESUME | single-frame resume-past-escape (#754) | flipped default-ON 2026-07-25; retirement was conditioned on the multi-frame twin (`_MULTIFRAME`) landing, but §1 now measures that twin as having zero corpus coverage, so the condition is unevaluable — keep the gate and re-open the question only once a benchmark reaches `inline_subwalk=true` at a vable escape | | ||
| | PYRE_FBW_BLACKHOLE_RESUME | single-frame resume-past-escape (#754) | flipped default-ON 2026-07-25; retirement was conditioned on the multi-frame twin (`_MULTIFRAME`) landing, but §1 now measures that twin as having zero corpus coverage, so the condition is unevaluable — keep the gate and re-open the question once the multi-frame adopt's root-mismatch decline (§1d) is resolved | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale retirement trigger.
The root-mismatch decline is documented as resolved on July 26, 2026. Keep this gate until the remaining escaping sys._getframe identity wrong-answer is fixed and its acceptance fixture passes.
🤖 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/gate-triage.md` at line 471, Update the PYRE_FBW_BLACKHOLE_RESUME
retirement condition in pyre/gate-triage.md to reflect that the multi-frame
root-mismatch decline is resolved. Keep the gate active until the remaining
escaping sys._getframe identity wrong-answer is fixed and its acceptance fixture
passes.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 08b29da). 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
|
This was referenced Jul 27, 2026
This was referenced Aug 5, 2026
Merged
This was referenced Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent clusters, both landing on
try_adopt_multi_frame_blackholeandthe guard-exception handoff. Every executable change is either behind
PYRE_FBW_MULTIFRAME(default off) or a GC root registration.1.
PYRE_FBW_MULTIFRAMEmulti-frame blackhole adopt (5 commits)The adopt declined every chain, including well-formed ones.
per_frame[0]is recovered from the trace's frame register, whose root vable identity
seed_virtualizable_boxesbakes againstlive_vable_frame_addr, while thecomparison operand was
cf_addr— thesnapshot_for_tracingcopy. Bothproduction entries set the live field from a real
&PyFrame, so the twooperands were never equal. The prior comment attributed the decline to a chain
rooted at an intermediate frame and named the
jit.virtual_refemit as theprerequisite; measurement refuted that.
live_root_addrin and uses it for the two uses that ask an identityquestion — the gate and the root
f_backrefoperand whoseptr::eqskip hasto fire for
frames[0]. The other uses keep the snapshot.blackhole level runs against
per_frame[0]becauseconvert_and_run_from_pyjitploverrides each level'svirtualizable_ptr, soits
setfield_vablestores land on the live frame while the portal epiloguecopies the snapshot's whole locals array back over it. Hoisted above the
match: every adopted arm sets
WALK_END_FLUSH_COMMITTED, and for theexception terminal the traceback keeps the root frame reachable, so a stale
copy is observable through
tb_frame.f_localslong after the walk.and only
frames[0]may be the walked frame. No producer is known; thefailure mode is silent, so it declines rather than relying on the absence.
to 20, plus 10 pinned declines.
The flip is still blocked, and the blocker is now named. The walker executes
residuals concretely while an inline push never runs the interpreter's call
sequence, so
ec.topframerefstill names the CALLER while an inlined calleebody runs. A
sys._getframethat is itself the escaping residual therefore readthe wrong frame at walk time, and adopting commits that answer where legacy
escape/replay discards it:
One wrong iteration per adopt, 5 adopts and 5 wrong in each part of
synth/getframe_while_escaping_read_frame_identity, which is the acceptancetest: it passes today with the gate off and fails loudly if the gate is flipped
first. Closing it needs the inlined-call push to publish the callee frame on the
execution context — the open
walker_ec_enter/walker_ec_leavework (#796).2. Root the grabbed guard exception across the handoff (2 commits)
grab_exc_value(llmodel.py:240) readsjf_guard_excoff the deadframe anddrops the jitframe, which held the collector's only reference to that exception
(
jitframe_trace). The handoff then decodes resume data and rebuilds virtualsthrough the blackhole allocator, so the value spans an allocating window as a
bare
i64that the precise collector cannot see. RPython keeps the same valuein a shadowstack-rooted local.
Adds
GuardExcRoot+ aGUARD_EXC_VALUEwalker that marks the carrier andforwards its young child slots, the same shape as
walk_jit_exc_value/walk_bh_last_exc_value. Parked athandle_fail,blackhole_resume_via_rd_numbandback_edge_internal.This is a parity fix, not a demonstrated bug fix, and the PR says so. Under
MAJIT_GC_STRESSon agc_stressbuild the five benches that produce liveguard exceptions pass identically with the walker registered and suppressed —
on the residual-raise path the same exception is still parked in
BH_LAST_EXC_VALUE, which is already rooted.A
handle_failcensus over the 339-file corpus counts 732,660 guard failures,34,790 carrying a live exception into the window, 170 of them on the bridge
route. That retires
bridge_guard_excfrom thePYRE_CARRIER_EXC_RESUMEpre-flip list: the same pointer drives the default blackhole resume, so the gate
never bounded the exposure.
The second commit corrects the ledger's description of that gate.
seed_bridge_standing_exception_from_currentis ungated and already mirrorsupstream's assign-or-clear branch; what diverges is the source —
_prepare_exception_resumptiontakescpu.grab_exc_value(deadframe), pyretakes
sym.current_exc_valuefalling back toget_current_exception(), thesys.exc_info()mirror. The gate is a back-channel into that function, and itsis_nullconjunct exists to avoid clobbering a livesys.exc_infovalue — sothe "add the missing unconditional assign" reading would have corrupted
PUSH_EXC_INFO / POP_EXCEPT rather than restored parity.
Rebase note
Rebased onto
origin/main948340d. Three conflicts, all insidetry_adopt_multi_frame_blackhole, where #798 and this branch rewrote the samecomment from the same original. Resolved as blends: #798's
mfdbg!decline-naming is kept in full and extended to the two new declines, the
identity-gate prose takes this branch's corrected attribution, and the decline
message no longer prints the
virtual_refcause the same commit refutes. #798'sremoval of the
resume_py_pc == 0check survives untouched.Gates
Measured on the rebased tree, after re-extracting LLBC.
check.py --backend dynasmcheck.py --backend dynasm,PYRE_FBW_MULTIFRAME=1check.py --backend dynasm,PYRE_CARRIER_EXC_RESUME=1check.py --backend craneliftcargo test -p majit-metainterp -p pyre-jit-trace -p pyre-jitThe one difference is
nested_loopat 2.0x against its 2x perf gate, measuredwhile eight to eleven cargo builds from sibling worktrees were running.
Interleaved against pypy with check.py's own startup subtraction, seven rounds
give 1.54x / 1.62x / 1.64x / 1.65x / 1.66x / 1.68x / 1.69x, and the bench passes
in both gated runs above.
— authored by Claude
Summary by CodeRabbit
Bug Fixes
Tests
Documentation