jit(fbw): flip and then retire the two blackhole-resume gates - #901
Merged
Conversation
`orig_vable_ptr_from_trace_ctx` prefers `ctx.virtualizable_heap_ptr()` over the ambient `self.vable_ptr`. The comment stated the consequence but named no upstream line; `send_loop_to_backend` reads the vable as `orig_inpargs[jitdriver_sd.index_of_virtualizable].getref_base()` (compile.py:510), a value bound to the trace being compiled. Comment only; the ordering itself landed in #876. Assisted-by: Claude
`multi_frame_blackhole_resume_enabled` was a hard `== Some("1")` opt-in. It
now reads like its single-frame twin: `OnceLock`, `None => true`, `=0`/`false`
opts back out.
The decline it documented was the walk-time frame identity answer, not outer
locals. The walker executes residuals concretely while an inline push does not
run the interpreter's call sequence, so `ec.topframeref` named the CALLER while
an inlined callee body ran; a `sys._getframe` that is itself the escaping
residual read that frame and the adopt committed it. `walker_ec_enter` /
`walker_ec_leave` publish the callee frame at the inlined-call push, which
closes it. A `sys._getframe` executed later, inside the blackhole, was already
correct, so the "outer-locals materialization" wording in the gate doc and in
`trace.rs` described a condition that was never the cause; both are rewritten.
Measured on `synth/getframe_while_escaping_read_frame_identity`, whose header
is updated from acceptance test to regression guard: `30000 30000 0 0` under
CPython, under the new default, and under `PYRE_FBW_MULTIFRAME=0`, with 10
`adopted multi-frame terminal` events and no `chain rooted at` decline — the
adopt commits rather than declining, so the flip is not inert.
`check.py`: 1 failed / 341 passed on dynasm and cranelift with the gate forced
on, and again on dynasm with it default-on; the one failure is
`synth/ast_compile_roundtrip`, a cpython/pypy reference mismatch present with
the gate off. Zero JIT panics in all three runs.
gate-triage.md records the flip, drops the default-OFF bucket to two, and notes
that `_BLACKHOLE_RESUME`'s retirement condition (the multi-frame twin flipping)
is now met.
Assisted-by: Claude
The gate was flipped default-ON in the previous commit, so the variable and its OFF path are removed: `multi_frame_blackhole_resume_enabled` and the `&& multi_frame_blackhole_resume_enabled()` term on the latch's `else if` are gone, leaving `inline_subwalk` plus a successful `build_multi_frame_miframe` as the condition. `PYRE_FBW_MULTIFRAME_DEPTH` is a separate depth bound and stays; `PYRE_FBW_BLACKHOLE_RESUME`, which still gates the enclosing `if`, is untouched. The removed doc's two live facts move to the latch: the blast radius (`if`/`else if` whose single-frame arm requires `framestack.is_empty() && !inline_subwalk`, so this arm is exactly `inline_subwalk` at a vable escape), and what closed the frame-identity answer that once justified the gate. `synth/getframe_inline_subwalk_multiframe`'s header said the chain-root identity gate declines its shape, because the chain roots at the intermediate residual frame, and that lifting the decline would raise `KeyError: 'base'`. Measured now: 5 builds, 5 `adopted multi-frame terminal`, zero `chain rooted at` declines, and the output still matches CPython. The decline it described wanted the `jit.virtual_ref` emit at the inline push, which `walker_ec_enter` / `walker_ec_leave` supplied; the header now states that and keeps the old readings as the failure mode it guards. Verified on this tree: `synth/getframe_while_escaping_read_frame_identity` `30000 30000 0 0` under CPython and pyre with 10 adopts and no declines, `synth/getframe_inline_subwalk_multiframe` `200150000 7` under both, the deleted variable ignored when still set, `cargo fmt --check` clean, and `check.py --backend dynasm` 1 failed / 342 passed with zero JIT panics — the failure being `synth/ast_compile_roundtrip`, a cpython/pypy reference mismatch. Assisted-by: Claude
`single_frame_blackhole_resume_enabled` and its leading conjunct on the blackhole latch are gone, leaving `writes_live_heap`, odometer-unchanged, non-bridge, a `blackhole_result`, and a resolvable snapshot sym as the condition. The `[s2-gate]` diagnostic drops its `flag=` field, which was that reader and is now constant. #754, the epic this gated, closed 2026-07-23, and the multi-frame twin was retired in the previous commit; gate-triage.md's §4 holds gates whose OFF path is still a needed safety net, so the row moves out of it into the retired-variable registry. The removed doc's description and safety argument move to the latch: both the latch and the adopt (`try_adopt_single_frame_blackhole` → `apply_single_frame_blackhole_crn`, which validates every mapped color and every live operand-stack slot before writing anything) decline to the pre-existing path on any unmet condition, so this only ever replaces a replay that would have produced the same state. Three stale passages left over from the multi-frame retirement are corrected in the same file: `_MULTIFRAME` was missing from the registry whose criterion is "no env read site" (added, count 11 -> 12, noting `_MULTIFRAME_DEPTH` is a separate live bound); the "flip is blocked" section still called `walker_ec_enter` / `walker_ec_leave` open work and still said the acceptance test fails if the gate is flipped first; and the blast-radius paragraph still described what happens "with the gate off". Historical measurements are kept and labelled as history. Verified on this tree with both arms exercised, since this gate wrapped both: `synth/getframe_while_escaping_read_frame_identity` `30000 30000 0 0` under CPython and pyre with 10 multi-frame adopts and no chain-root decline, `synth/getframe_inline_subwalk_multiframe` `200150000 7` with 5 builds and 5 adopts, `bench/getframe_escape_flush_writethrough_regression` `PASS total=199990000 marks=20000` with 5 single-frame adopts, the deleted variable ignored when still set, `cargo fmt --check` clean, and `check.py --backend dynasm` 1 failed / 342 passed with zero JIT panics — the failure being `synth/ast_compile_roundtrip`, a cpython/pypy reference mismatch. Assisted-by: Claude
WalkthroughThe PR removes retired environment-variable gates from blackhole resume paths, documents frame publication as closing the ChangesBlackhole resume adoption
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ResidualCall
participant Walker
participant BlackholeAdoption
participant RegressionFixture
ResidualCall->>Walker: publish callee frame during inline push
Walker->>BlackholeAdoption: provide corrected frame identity
BlackholeAdoption->>RegressionFixture: execute adopted blackhole path
RegressionFixture-->>BlackholeAdoption: validate sys._getframe results
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: 0f9c371b63
ℹ️ 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".
Comment on lines
2507
to
2508
| } else if ctx.fbw_mode.inline_subwalk | ||
| && multi_frame_blackhole_resume_enabled() | ||
| && let Some(framestack) = build_multi_frame_miframe( |
There was a problem hiding this comment.
When an inlined callee assigns a local and a virtualizable escape then resumes its multi-frame image, removing this gate makes the unsafe adopt unconditional. trace.rs:2573-2584 explicitly documents that only frame 0 has its locals published, so inner frames retain pre-walk heap contents and the blackhole can read null instead of the callee's updated register value; the recorded s21_sigsegv.py case consequently faults in object_getattr_miss. The execution-context enter/leave fix only repairs frame-chain identity and does not add the missing per-level slot-to-value writeback, so retain the decline until that materialization exists.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 0f9c371). 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/gate-triage.md`:
- Line 467: Synchronize the summary counts in pyre/gate-triage.md: update the
dead-gate count at lines 467-467 from 10 to 12, and update the default-OFF
experiment count at lines 519-529 from 3 to 2.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs`:
- Around line 2483-2507: Update the inline_subwalk branch around
try_adopt_multi_frame_blackhole so multi-frame adoption is not used while
inner-frame locals cannot be published: either add per-level publication before
driving all frames, or restore a pre-drive decline/fallback for unsupported
frame shapes. Ensure locals written by an inlined callee resume with their
current values rather than stale or NULL data, while preserving the existing
single-frame path.
🪄 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: 0aef5e99-0970-4971-973e-4d5c95484662
📒 Files selected for processing (7)
majit/majit-metainterp/src/pyjitpl.rspyre/bench/synth/getframe_inline_subwalk_multiframe.pypyre/bench/synth/getframe_while_escaping_read_frame_identity.pypyre/bench/synth/getframe_while_inlined_callee_subwalk.pypyre/gate-triage.mdpyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/trace.rs
| - `PYRE_STACKTOOBIG` — `pub static PyreStackTooBig` runtime symbol (`stack_check.rs`) | ||
|
|
||
| ## §3 — Dead (10): no env read site | ||
| ## §3 — Dead (12): no env read site |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the gate-triage summary counts.
The updated sections correctly report 12 dead gates and 2 remaining default-OFF experiments, but the summary still reports 10 and 3 respectively.
pyre/gate-triage.md#L467-L467: update the summary’s dead-gate count from 10 to 12.pyre/gate-triage.md#L519-L529: update the summary’s default-OFF experiment count from 3 to 2.
📍 Affects 1 file
pyre/gate-triage.md#L467-L467(this comment)pyre/gate-triage.md#L519-L529
🤖 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 467, Synchronize the summary counts in
pyre/gate-triage.md: update the dead-gate count at lines 467-467 from 10 to 12,
and update the default-OFF experiment count at lines 519-529 from 3 to 2.
Comment on lines
+2483
to
2507
| // An inlined sub-walk adopts the multi-frame blackhole image. | ||
| // The build side (`build_multi_frame_miframe`, the input-arg | ||
| // `_resref` seed, and the getfield-chain `recover_ref_value`) | ||
| // reconstructs the frame stack; the resume side | ||
| // (`drive_multi_frame_blackhole` → | ||
| // `convert_and_run_from_pyjitpl`) publishes each level as the | ||
| // chain reaches it. The blast radius is exactly | ||
| // `inline_subwalk` at a vable escape: this is an `if`/`else | ||
| // if`, and the single-frame arm requires | ||
| // `framestack.is_empty() && !inline_subwalk`. | ||
| // | ||
| // This path was once gated because the walker executes | ||
| // residuals concretely while an inline push does not run the | ||
| // interpreter's call sequence. `ec.topframeref` therefore | ||
| // named the CALLER while an inlined callee body ran, so a | ||
| // `sys._getframe` that was itself the escaping residual read | ||
| // the wrong frame at walk time. Adopting committed that answer | ||
| // where legacy escape/replay discarded it. A `sys._getframe` | ||
| // executed later, inside the blackhole, was always correct. | ||
| // `walker_ec_enter` / `walker_ec_leave` (the port of | ||
| // `executioncontext.py:85-107`) publish the callee frame at | ||
| // the inlined-call push, which closed the gap. | ||
| // `synth/getframe_while_escaping_read_frame_identity` is the | ||
| // regression guard. | ||
| } else if ctx.fbw_mode.inline_subwalk |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Do not make multi-frame adoption unconditional before publishing inner-frame locals.
Line 2507 now exposes a path where try_adopt_multi_frame_blackhole drives all frames, but the supplied adopter only captures/writes back root_addr; its own adjacent implementation notes that inner frame arrays retain pre-subwalk values. Locals assigned in an inlined callee can therefore resume as stale/NULL values and fault. Implement per-level local publication first, or retain a pre-drive decline/fallback for unsupported shapes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs` around lines 2483
- 2507, Update the inline_subwalk branch around try_adopt_multi_frame_blackhole
so multi-frame adoption is not used while inner-frame locals cannot be
published: either add per-level publication before driving all frames, or
restore a pre-drive decline/fallback for unsupported frame shapes. Ensure locals
written by an inlined callee resume with their current values rather than stale
or NULL data, while preserving the existing single-frame path.
Source: Coding guidelines
This was referenced Jul 30, 2026
youknowone
added a commit
that referenced
this pull request
Aug 11, 2026
… number A count brake over this file was not implementable because nothing separated a MEMBER of a section from a MENTION of a gate inside its prose. Adds a Conventions section recording: - a number in a heading is a membership count (seven of them, derivable from the section), a dated measurement (§6's 66, §1e's 339 files), or a soft `~` count; only the first is enforceable; - a member is a name in its entry's key position, and the position differs by section shape — first table cell, before the em dash in §2, after it in §5, inside §6b/§6c's terminal list block. §4's first row keys on two names, so the unit is the name, not the row. Measured 2026-08-11 by enumerating each section's members: the seven counts read 5, 4, 11, 12, 13, 4, 11 and all seven are correct. The five non-member names inside those sections are each disclaimed in the cell that contains them. Also spells §3's `PYRE_FBW_MULTIFRAME` and `PYRE_FBW_BLACKHOLE_RESUME` in full; both were written prefix-elided and neither full name occurred anywhere in the repository, which §6's "spell every name in full at least once" rule forbids. Names verified against 0135f56 (#901), #763 and #754. The live-documented name set is unchanged in both directions by this commit. Assisted-by: Claude
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.
Retires both
PYRE_FBW_*blackhole-resume switches. Four commits, each verifiedon its own tree.
What changed
majit: cite compile.py:510 for the per-trace virtualizable read—comment only.
orig_vable_ptr_from_trace_ctxprefers the per-tracectx.virtualizable_heap_ptr()over the ambientself.vable_ptr; the commentstated the consequence but named no upstream line.
send_loop_to_backendreadsthe vable as
orig_inpargs[jitdriver_sd.index_of_virtualizable].getref_base()(compile.py:510), a value bound to the trace being compiled. The ordering itself
landed in #876.
jit(fbw): flip PYRE_FBW_MULTIFRAME default-ON— the gate was a hard== Some("1")opt-in; it now reads like its single-frame twin.The decline it documented was the walk-time frame identity answer, not outer
locals. The walker executes residuals concretely while an inline push does not
run the interpreter's call sequence, so
ec.topframerefnamed the CALLER whilean inlined callee body ran; a
sys._getframethat is itself the escapingresidual read that frame and the adopt committed it.
walker_ec_enter/walker_ec_leavepublish the callee frame at the inlined-call push, which closesit. A
sys._getframeexecuted later, inside the blackhole, was already correct,so the "outer-locals materialization" wording in the gate doc and in
trace.rsdescribed a condition that was never the cause. Both are rewritten.
jit(fbw): retire the PYRE_FBW_MULTIFRAME reader— variable and OFF pathdeleted, leaving
inline_subwalkplus a successfulbuild_multi_frame_miframeas the condition.
PYRE_FBW_MULTIFRAME_DEPTHis a separate depth bound andstays.
synth/getframe_inline_subwalk_multiframe's header said the chain-root identitygate declines its shape and that lifting the decline would raise
KeyError: 'base'. Measured now: 5 builds, 5 adopts, zerochain rooted atdeclines, output unchanged. The decline it described wanted the
jit.virtual_refemit at the inline push, whichwalker_ec_enter/walker_ec_leavesupplied.jit(fbw): retire the PYRE_FBW_BLACKHOLE_RESUME reader— same treatment forthe single-frame twin. #754, the epic it gated, closed 2026-07-23, and
gate-triage.md§4 holds only gates whose OFF path is still a needed safety net,so the row moved into the retired-variable registry. The
[s2-gate]diagnosticdrops its
flag=field, which was that reader and is now constant.Each removal relocates the deleted doc's still-true prose to the latch rather
than dropping it: the blast radius, and the safety argument that both the latch
and the adopt decline to the pre-existing path on any unmet condition, so
neither path ever replaces more than a replay that would have produced the same
state.
Verification
Every gate removal was checked for inertness first — a green corpus proves
nothing if the latch stops firing. Both arms were exercised, since
_BLACKHOLE_RESUMEwrapped both:synth/getframe_while_escaping_read_frame_identity30000 30000 0 0synth/getframe_inline_subwalk_multiframe200150000 7bench/getframe_escape_flush_writethrough_regressionPASS total=199990000 marks=20000check.py --backend dynasm1 failed / 342 passed with zero JIT panics at eachstep; with the multi-frame gate forced on before the flip, dynasm and cranelift
were 1 failed / 341 passed each. The single failure throughout is
synth/ast_compile_roundtrip, a cpython/pypy reference mismatch that failsidentically without these commits.
cargo fmt --checkclean.Documentation
gate-triage.mdcarried several passages that the flip falsified: the"flip is blocked" section still called
walker_ec_enter/walker_ec_leaveopenwork, the acceptance test was still described as failing if the gate were
flipped first, the blast-radius paragraph described what happens "with the gate
off", and
_MULTIFRAMEwas missing from the registry whose criterion is "no envread site". All corrected; historical measurements kept and labelled as history.
Relation to #493
The blackhole-resume path resumes past the escaping opcode instead of falling
back to escape/replay, which is #493's target invariant. Making it
unconditional settles that protocol for one end-variant — a
VableEscapedresidual abort — in both its single- and multi-frame forms. It does not touch the
rest: measured today, only 2 of 48
DispatchErrorvariants reach a flush leg(43 and 2 at the epic's 2026-07-11 census, so 41 unconditional replays have
become 46), the store journal and sticky marker are intact, and the 10
deliberate-symbolic plus 2 value-unavailable residual decline classes are
unchanged.
— written by Claude
Summary by CodeRabbit
Bug Fixes
sys._getframe()during inlined and escaping calls.KeyErrorfailures in affected execution paths.Documentation