jit: make JIT-created interpreter frames GC-owned (retire FrameArena and the portal frame rebuild) - #794
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
WalkthroughThe PR replaces arena-based JIT callee-frame allocation and rooting with live-frame tracking, routes forced execution through existing ChangesJIT frame and inline execution changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JITCall
participant InlineCall
participant FrameGuards
participant ResidualExecution
participant PyFrame
JITCall->>PyFrame: allocate and publish callee frame
InlineCall->>FrameGuards: enter concrete callee frame
FrameGuards->>ResidualExecution: execute residual with rewired frame chain
ResidualExecution-->>FrameGuards: restore frame state
JITCall->>PyFrame: run forced callee through portal
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 8ebf464). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 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: 54c7374ba6
ℹ️ 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".
| if !frame.is_null() { | ||
| INLINE_CONCRETE_FRAME.with(|slot| slot.set(frame)); | ||
| } |
There was a problem hiding this comment.
When a nested strict callee is not seeded (for example, once inline_depth reaches fbw_max_multiframe_depth() or a seed precondition bails out), concrete_callee_frame remains null, but enter(null) leaves INLINE_CONCRETE_FRAME pointing to the outer callee. Every residual in the nested sub-walk then publishes that outer frame through ResidualFrameChainGuard, so frame-sensitive operations such as sys._getframe() or traceback creation report the parent's pycode/globals/locals as the current callee. The null case must not inherit another frame; ideally this path should materialize and select the nested callee's own concrete frame.
AGENTS.md reference: AGENTS.md:L24-L41
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: 3
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-trace/src/jitcode_dispatch/inline_call.rs (1)
2211-2220: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCallee frames are now GC-owned, but several raw
*mut PyFramecopies assume they never relocate. Replacing pinned arena slots withFrameBox::newallocations makes reachability the only guarantee the collector provides; address stability is assumed but nowhere asserted, while compiled code (register / jitframe slot),callee_concrete_r, and the new sub-walk pointer all hold copies the collector cannot fix up.
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L2211-L2220:concrete_callee_frameis retained pastdrop(frame)and dereferenced/mutated byResidualFrameChainGuardfor the whole sub-walk; if the frame moves, the stamped frontend op is updated but this local is not. Re-read the pointer from the rooted channel (the op's concrete) at guard-entry time, or document/enforce the pinning.pyre/pyre-jit/src/call_jit.rs#L382-L389: the walker's comment asserts "non-moving stable allocation, so the visitor never rewrites the slot" — turn that into adebug_assertthat the visited slot is unchanged, so a future collector change fails loudly instead of silently stranding compiled code's copy.🤖 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/inline_call.rs` around lines 2211 - 2220, The callee-frame raw pointer must not outlive an unverified GC allocation. In pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs:2211-2220, update the ResidualFrameChainGuard sub-walk setup to re-read concrete_callee_frame from the rooted frontend op at guard entry, or explicitly enforce pinning before retaining it past drop(frame); preserve the existing reachability root. In pyre/pyre-jit/src/call_jit.rs:382-389, add a debug assertion that the visited frame slot remains unchanged after GC visitation, matching the stable-allocation assumption.
🤖 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/inline_call.rs`:
- Around line 2067-2071: Make the nested strict-inline fallback explicit for
concrete_callee_frame when the seed block cannot materialize a frame, including
raw.is_null(), nonzero ncells, PopJumpIfNone, unresolved frame registers, or
null snapshot_sym. Update the InlineConcreteFrameGuard::enter flow or its
surrounding handling so a null value does not implicitly retain the outer callee
frame; publish an intentional no-frame state, and document the resulting
shallow-resolution behavior.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs`:
- Around line 443-456: Update the escape-query logic around ACTIVE_FRAME_ESCAPE
and PUBLISHED_INLINE_FRAME so the initial published-frame check only computes
whether the frame matches. Move or remove the f_back mark_as_escaped
propagation, ensuring it occurs only when the active escape branch is actually
taken or solely through ResidualFrameChainGuard::drop, with no side effects when
the query returns false.
In `@pyre/pyre-jit/src/call_jit.rs`:
- Around line 663-665: Update run_frame_through_portal to validate frame_ptr
before dereferencing it, matching the sentinel and null checks used by
jit_drop_callee_frame. Return the existing clean deoptimization/failure result
for invalid values, while preserving the current portal_runner path for valid
PyFrame pointers so both jit_force_callee_frame and ll_portal_runner_shim are
protected.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 2211-2220: The callee-frame raw pointer must not outlive an
unverified GC allocation. In
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs:2211-2220, update the
ResidualFrameChainGuard sub-walk setup to re-read concrete_callee_frame from the
rooted frontend op at guard entry, or explicitly enforce pinning before
retaining it past drop(frame); preserve the existing reachability root. In
pyre/pyre-jit/src/call_jit.rs:382-389, add a debug assertion that the visited
frame slot remains unchanged after GC visitation, matching the stable-allocation
assumption.
🪄 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: 79a74c70-dbef-4d35-b18a-dcde7cf68923
📒 Files selected for processing (9)
majit/majit-backend-cranelift/src/compiler.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rs
| let escaped_published = PUBLISHED_INLINE_FRAME.with(|slot| { | ||
| let published = slot.get(); | ||
| let matched = !published.is_null() && std::ptr::eq(published, frame); | ||
| if matched { | ||
| let f_back = unsafe { (*published).get_f_back() }; | ||
| if !f_back.is_null() { | ||
| unsafe { (*f_back).mark_as_escaped() }; | ||
| } | ||
| } | ||
| matched | ||
| }); | ||
| ACTIVE_FRAME_ESCAPE.with(|slot| { | ||
| if let Some((expected, py_pc)) = slot.get() | ||
| && expected == frame as usize | ||
| && (expected == frame as usize || escaped_published) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Move the mark_as_escaped side effect out of the predicate.
escaped_published is evaluated before ACTIVE_FRAME_ESCAPE is even consulted, so the caller-escape propagation fires on any query for the published frame — including when there is no active escape guard and the function returns false having done nothing else. It also duplicates the identical propagation in ResidualFrameChainGuard::drop (Lines 391-396). Computing only matched here and doing the marking inside the taken branch (or relying solely on the guard's drop) keeps the query side-effect-free and the propagation in one place.
🤖 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 443 -
456, Update the escape-query logic around ACTIVE_FRAME_ESCAPE and
PUBLISHED_INLINE_FRAME so the initial published-frame check only computes
whether the frame matches. Move or remove the f_back mark_as_escaped
propagation, ensuring it occurs only when the active escape branch is actually
taken or solely through ResidualFrameChainGuard::drop, with no side effects when
the query returns false.
| fn run_frame_through_portal(frame_ptr: i64) -> i64 { | ||
| let frame = unsafe { &mut *(frame_ptr as *mut PyFrame) }; | ||
| let result = crate::eval::portal_runner(frame); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
run_frame_through_portal dereferences frame_ptr unchecked while jit_drop_callee_frame guards the same value.
jit_drop_callee_frame (Line 3882) treats frame_ptr & 1 != 0 as a non-frame sentinel and bails. Both helpers read the callee-frame slot the rewritten CALL_ASSEMBLER passes as arg 0, so the force path can observe the same sentinel — and a NULL slot after a failed callee-frame creation — yet here it is cast straight to &mut PyFrame and handed to portal_runner. That is a segfault inside the portal rather than a clean deopt. Both callers (jit_force_callee_frame, ll_portal_runner_shim) inherit the gap, so guard it at this root.
🛡️ Proposed guard mirroring the drop path
fn run_frame_through_portal(frame_ptr: i64) -> i64 {
+ // Mirror `jit_drop_callee_frame`: a tagged/absent callee-frame slot is
+ // not a `PyFrame` and must not be resumed.
+ if frame_ptr == 0 || frame_ptr & 1 != 0 {
+ return pyre_object::PY_NULL as i64;
+ }
let frame = unsafe { &mut *(frame_ptr as *mut PyFrame) };
let result = crate::eval::portal_runner(frame);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn run_frame_through_portal(frame_ptr: i64) -> i64 { | |
| let frame = unsafe { &mut *(frame_ptr as *mut PyFrame) }; | |
| let result = crate::eval::portal_runner(frame); | |
| fn run_frame_through_portal(frame_ptr: i64) -> i64 { | |
| // Mirror `jit_drop_callee_frame`: a tagged/absent callee-frame slot is | |
| // not a `PyFrame` and must not be resumed. | |
| if frame_ptr == 0 || frame_ptr & 1 != 0 { | |
| return pyre_object::PY_NULL as i64; | |
| } | |
| let frame = unsafe { &mut *(frame_ptr as *mut PyFrame) }; | |
| let result = crate::eval::portal_runner(frame); |
🤖 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/call_jit.rs` around lines 663 - 665, Update
run_frame_through_portal to validate frame_ptr before dereferencing it, matching
the sentinel and null checks used by jit_drop_callee_frame. Return the existing
clean deoptimization/failure result for invalid values, while preserving the
current portal_runner path for valid PyFrame pointers so both
jit_force_callee_frame and ll_portal_runner_shim are protected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02443b6a09
ℹ️ 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".
| // Calls return in LIFO order, so the match is normally the last | ||
| // entry. | ||
| if let Some(pos) = frames.iter().rposition(|p| *p == ptr) { | ||
| frames.swap_remove(pos); |
There was a problem hiding this comment.
When a long-running JIT callee has already survived a minor collection, that collection resets its old-generation locals array's remembered-set state; a later jit_frame_set_slot_{ref,int,float} write can then install a young object without a write barrier. Removing the frame from LIVE_CALLEE_FRAMES here means that, if the frame escaped through a traceback or frame reference before returning, the next minor collection reaches the old frame but does not rescan its locals array, leaving f_locals with an evacuated pointer and risking corruption or a crash. Re-remember the frame/locals array before unrooting it, or barrier each slot write.
AGENTS.md reference: AGENTS.md:L24-L30
Useful? React with 👍 / 👎.
…l calls An inline sub-walk executes the callee's residual calls concretely, but nothing published the callee frame on the interpreter chain, so a residual that reads the chain (sys._getframe, a traceback) resolved one level too shallow. Bracket each such residual with executioncontext.py:85 enter / :91-109 leave. The leave half propagates the escape: a frame handed to application code keeps a reference to its caller, so the caller is forced and marked escaped. Without that propagation the escape is recorded only on a frame the walk owns privately, and the walk stops reporting it as an escape of the traced virtualizable. flush_active_frame_escape therefore also matches the frame a live ResidualFrameChainGuard has published, keying the flush on the traced virtualizable whose resume pc the residual already latched. For that redirected match the escape is reported only once the flush commits a resume pc; forcing without one raises an escape the walk can answer only by replaying from entry, double-applying the residual's body effects. The multi-frame CRN arm resets valuestackdepth to stack_base and declines when the resume pc's cached stack depth is non-zero, so a resumed frame does not leak a stack slot per iteration. Assisted-by: Claude
…ena slots Delete FrameArena and its slot-recycling path. The four callee-frame creation helpers now allocate through FrameBox::new with an OldGenGc locals array — the same non-moving PYFRAME_GC_TYPE_ID allocation the interpreter uses for every other frame (baseobjspace.py:799-801 createframe, pyframe.py:52 class PyFrame(W_Root)) — and jit_drop_callee_frame only drops the JIT's root on the frame instead of returning the block to a free list or freeing it. A frame the program retains past the call (a traceback node, an f_back chain) previously kept pointing at a slot that the next call at the same depth overwrote in place, including drop_in_place of the old locals array. The extra root walker now also marks the frame object itself: while compiled code runs, the live-frame list is the frame's only root. Removed with the arena: the ARENA_BUF_BASE / ARENA_TOP / ARENA_INITIALIZED statics (no readers), GcFrameSlot, GcPyFrame, heap_alloc_frame / heap_free_frame and reset_reused_call_frame. arena_jitframe_descrs / arena_global_info describe the JitFrame layout, not the frame arena, and are renamed to jitframe_layout_descrs / jitframe_layout_info. Assisted-by: Claude
`jit_force_callee_frame` read pycode/w_globals/execution_context off the incoming pointer by raw offset and built a fresh `PyFrame` with an empty argument slice, then ran that. The rebuilt frame was a Rust stack local, so `portal_runner` published a C-stack address as `CURRENT_FRAME` and `ec.topframeref`; every parameter slot was `PY_NULL` and `last_instr` was reset to -1. The incoming pointer is the callee `PyFrame` that `emit_new_pyframe_inline_with_params` builds (`NewWithVtable` + `SetfieldGc`), which the GC rewriter stores into the callee jitframe's first slot. `ll_portal_runner` forwards its reds unchanged (warmspot.py:953-954), so run that frame instead of rebuilding one. `ll_portal_runner_shim` already did; both now call `run_frame_through_portal`. Assisted-by: Claude
`PyFrame::new_minimal` and `PyFrame::new_with_namespace` return a `PyFrame` by value and have no callers. `alloc_fixed_array_from_vec` was used only by `new_minimal`. Assisted-by: Claude
`assemble_bridge_inline_pending` built a `StdAlloc` `PyFrame` by value, seeded its locals and operand stack from the recipe and set its `last_instr`. No code reads the field: the callee's concrete state is carried by `sym.concrete_locals` / `sym.concrete_stack` and by the frame vable that `setup_reconstructed_callee_frame` binds to `sym.frame`. Assisted-by: Claude
`instance_lock_for(obj).lock()` blocked while the thread was still counted
as a running mutator. The stripe owner can allocate under the stripe and
request a collection, which then waits for the blocked thread while that
thread waits for the stripe. Route every acquire through `lock_stripe`,
which try-locks first and leaves the census around a contended wait, as
`w_list_lock` and `w_dict_lock` already do.
`instance_node_dict_{length,clear,keys,values,items}` read and rebuild the
same map and storage as the guarded get/set/delete entry points but took no
stripe; guard them too. The lock is reentrant and every one of these
functions is already `dont_look_inside`.
Also drops a stale rationale above `SYS_MODULES`: there is no GIL to
serialize semantic access.
Assisted-by: Claude
`time.sleep` held the guard across the whole EINTR retry loop, so `checksignals_now` ran a Python signal handler while the mutator was outside the RUNNING census. Any allocation or second blocking call in that handler then trips a running-mutator assertion. `nanosleep` is declared `releasegil=True` (interp_time.py:504-506), so only the syscall runs outside; `checksignals()` runs with the GIL re-acquired (interp_time.py:707). Scope the guard to each blocking call to match. Assisted-by: Claude
`_ThreadHandle.join(float('inf'))` reached `Duration::from_secs_f64`, which
aborts the process on a non-finite or out-of-range value. `parse_acquire_args`
(os_lock.py:33-39) raises `OverflowError("timeout value is too large")`; the
handle's own timeout path now does the same, for the int arm too.
`stack_size()` read a non-integer argument's payload word as an integer.
`@unwrap_spec(size=int)` (os_thread.py:216) unwraps through `space.int_w`,
which rejects it.
`_local.__new__` allocated the instance and registered it in the execution
context before rejecting construction arguments; os_local.py:81 rejects them
before `allocate_instance`, so a refused construction never reaches
`_register_in_ec`. The check also applied only to the exact type, while
os_local.py:75-85 rejects arguments for every subtype that inherits
`object.__init__`.
`clone_for_thread` carried `coroutine_origin_tracking_depth` into the new
thread; a fresh ExecutionContext starts at 0 (executioncontext.py:53).
Assisted-by: Claude
`w_list_clear` drops the object items, replaces every typed backing array and resets the strategy, but took no stripe, so it raced with the guarded append/getitem/setitem/len entry points. Assisted-by: Claude
`run_module` and `run_source` called `finalize_runtime` only on the success path and, through `finalize_system_exit`, on SystemExit. Any other uncaught exception exited with status 1 without running threading shutdown, atexit handlers or module teardown. targetpypystandalone.py:88 runs `space.finish()` from a `finally`. The exception is printed before finalizing: `finalize_runtime` collects, and `PyError`'s raw object fields are not GC-visible. Assisted-by: Claude
…rame at unroot InlineConcreteFrameGuard::enter kept the enclosing callee's frame when the nested sub-walk had none of its own — a seed block that bailed. The inner callee's residuals then published that outer frame, resolving the chain one level too shallow, which is the error the guard exists to stop. Set the slot unconditionally; a null slot makes ResidualFrameChainGuard::enter publish nothing. unroot_callee_frame re-arms the remembered set on the frame and its old-gen locals array. The live-frame list is what makes a minor collection scan those slots; once the frame leaves it, a young ref stored since the last minor is reachable only through the array's own items, which a minor does not walk. A frame the program retained now outlives the call, so the window is real. run_frame_through_portal asserts its argument is an untagged frame pointer: jit_drop_callee_frame tolerates a tagged word on the same slot because it only unroots, but the portal has nothing to run for one, and its NULL result spells "exception stored" rather than a deopt. Also reformat the join timeout overflow arms. Assisted-by: Claude
builtin_subclass_dunder_obj resolves a user override for a builtin leaf, but it ran its probe for every instance, exact ones included: two full uncached MRO walks (lookup_where_pair walks the same name twice) plus a descriptor call through the generic call protocol, on every str/repr/format of an int, float, bool, str, list or bytearray. Only a subclass can redirect the dunder — it keeps the builtin ob_type and retags w_class — which is what is_exact_builtin_instance tests, so an exact instance returns None and the caller runs the leaf formatter it would have reproduced. `long` is excluded: its __str__/__repr__ also enforce sys.set_int_max_str_digits (longobject.py descr_repr), and that check sits in the descriptor rather than in the conversion, so an exact long keeps going through it. A machine int cannot reach any settable limit — 19 digits against a floor of 640. Measured on synth/comprehension_object_append_hot, per shape, 500 x 1000: f-string 0.674s -> 0.320s, dict 0.254s -> 0.215s. The bench's pypy ratio drops from 27.8/32.9/27.5/51.7x to 22.6/25.0/22.1/25.7x across dynasm and cranelift x default and PYRE_FBW_MULTIFRAME=1; its gate goes 40 -> 32. Assisted-by: Claude
|
Applied the review findings on
Frames relocating under the collector (coderabbit, outside-diff): Verification on the current base: — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/8ebf4642363626be2ffdf0e425f2a1b5c3e51c34/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L2506-L2508
When a seeded inline callee modifies a local before a frame-sensitive residual such as sys._getframe().f_locals, this guard publishes concrete_callee_frame, but that object was populated only with entry arguments at lines 2207-2215; subsequent setarrayitem_vable writes update the symbolic/concrete shadow in vable_ops.rs:591-600, not this frame object. The residual therefore observes stale entry-time locals, and its traceback can likewise use stale frame state. Synchronize the callee shadow into this object before publishing it, or make the object the callee's actual red frame throughout the walk.
AGENTS.md reference: AGENTS.md:L24-L30
https://github.com/youknowone/pyre/blob/8ebf4642363626be2ffdf0e425f2a1b5c3e51c34/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L2506-L2508
When a nested strict callee exceeds fbw_max_multiframe_depth() or its seed block bails, concrete_callee_frame remains null and this call deliberately installs that null; ResidualFrameChainGuard::enter then returns at residual_call.rs:356-358, leaving the caller as the visible top frame. Consequently sys._getframe(), traceback creation, and other frame-chain reads still report the wrong frame for that callee. The fresh evidence beyond the earlier comment is the new unconditional null installation plus the explicit no-publication branch; allocate and thread the nested callee's own frame instead.
AGENTS.md reference: AGENTS.md:L32-L41
ℹ️ 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".
1 participant
Makes every JIT-created interpreter frame an ordinary GC object.
FrameArenahad no upstream analogue:space.createframeallocates a freshPyFrame(W_Root)per call (baseobjspace.py:799-801,pyframe.py:52) and lifetime is the GC's —ec.leave(executioncontext.py:91-107) frees nothing.Commits
jit: enter/leave the inlined callee frame around its concrete residual calls— names the frame a sub-walk executes concretely so each residual it runs canenter/leaveit on the interpreter frame chain.jit: allocate JIT callee frames as GC objects instead of recycling arena slots— deletesFrameArena.jit: run the CALL_ASSEMBLER force path on the callee frame it is handed— deletes the portal's frame rebuild.interp: remove the unused raw-value PyFrame constructorsjit: drop the unread concrete_frame from PendingInlineFrameThe recycle bug (commit 2)
scratchpad/s4_arena_tb_recycle.py— a self-recursivefib(n, k)whose base case raisesZeroDivisionError, an outer function that walkse.__traceback__and retains everytb_frame, then morefibchurn, then re-readsf_localson the held frames:held 38 mutated 0held 38 mutated 15— silent data corruptionPYRE_NO_JIT=1held 38 mutated 0gc_alloc_nursery_shim, abortjit_drop_callee_frameunconditionally returned the slot;putdisarmed it (removing it from the root walk) and the next call at that depth overwrote it in place, includingdrop_in_placeof the old locals array — which is what made cranelift crash inPyFrame::fast2locals.escaped()cannot protect it even in principle: the bit is set lazily when Python readstb.tb_frame, long after the drop.The four create sites now call one
alloc_callee_frame=FrameBox::new(PyFrame::new_for_call_with_closure_and_globals_obj(..., OldGenGc)), andjit_drop_callee_frameonly unroots. Between create and drop the frame's only root isLIVE_CALLEE_FRAMES, so that walker must mark the frame object itself, not just walk its slots — slot-walking alone (what the arena needed) would let a major collection sweep the block compiled code is running on.pytraceback_object_custom_tracealready forwardstb.frameguarded bytry_gc_owns_object, so making the frame GC-owned lights that edge up on its own; noPYTRACEBACK_GC_PTR_OFFSETSchange was needed.★The reason my first probes found nothing: the arena is only entered when
recursive_force_cache_safeaccepts the callee, and it rejects anyLoadGlobal/LoadNamethat does not name the function itself.sys._getframe()needsLOAD_GLOBAL sys, so such a callee never uses the arena — it escapes its frame via a traceback, notsys._getframe.The portal rebuild (commit 3)
jit_force_callee_framereadpycode/w_globals/execution_contextoff the incoming pointer by raw offset and built a freshPyFramewith an empty argument slice. Two defects on one line: the rebuilt frame was a Rust stack local, soportal_runner→install_current_framepublished a C-stack address asCURRENT_FRAMEandec.topframeref; and every parameter slot wasPY_NULLwithlast_instrreset to -1. The repo already recorded the second failure once — the cranelift comment aboutw_list_append's sibling path notes "leftlocals_w[0]uninitialized … the stalenvalue drove the self-recursive CA into an unbounded loop" — and fixed it by deleting one call site, leaving the body.The premise justifying the rebuild — "the callee frame may be a nursery-allocated JitFrame-like block" — is false.
emit_new_pyframe_inline_with_paramsemitsNewWithVtable+SetfieldGcwith the realPyFramelayout, which is why the raw offsets worked at all; a dynasm regression test is named "force_fn must NOT be called with jitframe pointer … expected a PyFrame pointer".#777independently reached the same conclusion and stampedassembler_call_helper resumes the callee frame the rewritten CALL_ASSEMBLER passed as arg 0.Upstream never rebuilds:
ll_portal_runner(*args)forwards greens+reds verbatim (warmspot.py:941-959), theframered is a live heapPyFrame(W_Root)(interp_jit.py:66-92), and forcing a virtualizable writes back into the existing object (resume.py:1399-1408,virtualizable.py:126-137).rg "flavor\s*=\s*'stack'"overrpython/+pypy/returns 0 hits — RPython has no stack-allocated frame flavor.ll_portal_runner_shimalready ran the frame directly; both now sharerun_frame_through_portal.No reachable repro for this one. An
lldbbreakpoint on the mangled symbol (it resolves, so 0 is meaningful) counted 0 hits across 8 hand-written probes × 2 backends, all 19bench/*.py+ 31bench/synth/*.py, and 112 corpus scripts on cranelift ([ca-shim] entering= 0 in every one). On dynasm the leg is unreachable by construction — the GC rewriterexpect()s the CA token to be registered, and the samecompile_loopbody that registers it stampsset_ll_function_addra few dozen lines later with no early return between. So this commit restores upstream structure on a path that would be wrong whenever it does fire, rather than fixing an observed crash.Cleanups (commits 4-5)
An
ast-grep -p 'PyFrame { $$$ }'census found exactly fivePyFramestruct literals repo-wide, and 20 of 22new_for_call*callers already wrap inFrameBox::new. The two that did not were commit 3's rebuild andPendingInlineFrame.concrete_frame— aStdAllocby-value frame whose locals seeding andlast_instrstamp were dead weight (no code reads the field; the callee's concrete state is carried bysym.concrete_locals/sym.concrete_stackand the frame vable bound tosym.frame).PyFrame::new_minimal,new_with_namespaceandalloc_fixed_array_from_vechad no callers at all.Verification
s4_arena_tb_recycle:held 38 mutated 0on both backends (a control binary built without commit 2 still printsmutated 15).sys._getframe/f_backdiscriminators: 9/9 × 2 backends × {default,PYRE_FBW_MULTIFRAME=1}.RecursionError-inside-CALL_ASSEMBLER probe: correct on every combination.check.py: 313/313 × 4 — dynasm and cranelift × default andPYRE_FBW_MULTIFRAME=1, run serially.cargo test -p pyre-jit-trace -p pyre-jit --features dynasm: 644 passed.fibRSS 60 MB with the JIT vs 593 MB underPYRE_NO_JIT=1.fib_recursiveand +15% onint_loop, a pure loop that allocates no callee frame at all; that impossible second number is what exposed it as load drift.)Re-verified end to end on the current base (
cb4f6bd409, after #768 / #784 / #788 / #795):check.py: 322/322 x 4 — dynasm and cranelift x default andPYRE_FBW_MULTIFRAME=1, run serially.s4_arena_tb_recycle(held 38 mutated 0): 9/9 x both backends.cargo test -p pyre-jit-trace -p pyre-jit --features dynasm: 645 passed.Review follow-ups
InlineConcreteFrameGuard::enterinherited the enclosing callee's frame on a null seed — the slot is now set unconditionally. A nested sub-walk whose seed block bailed has no frame of its own, and publishing the outer callee's frame for the inner callee's residuals resolves one level too shallow, which is the error the guard exists to stop; a null slot makesResidualFrameChainGuard::enterpublish nothing.unroot_callee_framedid not re-arm the remembered set — it does now, on the frame and its old-gen locals array. The live-frame list is what makes a minor collection scan those slots; once the frame leaves it, a young ref stored since the last minor (argument boxing, the CALL_ASSEMBLER writeback injit_frame_set_slot_*) is reachable only through the array's own items, which a minor does not walk. Before this PR the frame was recycled at that point, so the window is new.FrameBox::newarms the same barrier at creation and the first minor consumes it.run_frame_through_portaldereferenced its argument unchecked — now adebug_assert!. The suggested "return NULL for a tagged/absent slot" guard was not taken: the portal's result is a Ref whose NULL spelling means exception stored, so returning NULL for a non-frame manufactures an exception-less error instead of a clean deopt.jit_drop_callee_frametolerates a tagged word only because it does nothing but unroot.mark_as_escapedinside theescaped_publishedpredicate — left as is.escaped()is set lazily, when Python readstb.tb_frame; relying onResidualFrameChainGuard::dropalone would drop the mark on a frame whose escape has not been observed yet.Unrelated rider
display: skip the subclass-dunder probe for exact builtin instances—builtin_subclass_dunder_objran two uncached MRO walks plus a descriptor call on everystr/repr/formatof an exactint/float/bool/str/list/bytearray, although only a subclass can redirect the dunder.longis excluded because its__str__/__repr__also enforcesys.set_int_max_str_digits(longobject.py descr_repr) and that check sits in the descriptor rather than in the conversion —synth/int_max_str_digitscaught the first, broader version of this change on all four arms.synth/comprehension_object_append_hotgoes 27.8/32.9/27.5/51.7x to 22.6/25.0/22.1/25.7x; its gate goes 40 to 32.Summary by CodeRabbit
Bug Fixes
Documentation