jit: close the branchy-inlined-callee bridge gap (overlay NULL-skip + vable-array re-assertion) - #653
Conversation
|
Warning Review limit reached
Next review available in: 52 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 Run ID: 📒 Files selected for processing (1)
WalkthroughBridge setup now reasserts decoded virtualizable references into the live frame after resume-image correction and avoids concrete stamping for both ChangesBridge setup corrections
Estimated code review effort: 2 (Simple) | ~10 minutes 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 3ac9446). 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: 10b621bd29
ℹ️ 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 !matches!( | ||
| cv, | ||
| majit_ir::Value::Void | ||
| | majit_ir::Value::Ref(majit_ir::GcRef(0)) |
There was a problem hiding this comment.
When a bridge local is genuinely unbound (for example after DELETE_FAST or a branch that skips assignment), the live frame slot is also PY_NULL; this file documents that frame-slot NULL means an uninitialized local, and the LOAD_FAST walker raises UnboundLocalError only when ctx.box_value(value) is Ref(PY_NULL) (trace_opcode.rs:7397-7403). This blanket skip leaves a non-constant local box unstamped in that case, so the bridge records a normal GuardNonnull/symbolic path instead of reproducing the interpreter's unbound-local exception, causing a miscompiled or perpetually deopting bridge for deleted/unassigned locals. Please distinguish stale frame-array holes from real unbound locals before dropping the NULL stamp.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/state.rs (1)
8734-8740: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winSkip
GcRef(0)when seeding kept-stack colors from the vable image.The stack overlay mirrors the
overlay_locallogic by reading fromlive_local_values(thelocals_cells_stack_warray). Just like loop-carried locals, kept stack temps can leave unmaterialized holes (GcRef(0)) in the frame array post-guard. Stamping these holes will poison the real vable box with concrete NULL, triggering the exact sameMayForceNullRefArgUnsupportedresidual abort this PR intends to fix, just for stack slots instead of locals.Please apply the same
GcRef(0)check here as was added tooverlay_local.🐛 Proposed fix
if seed_bridge_locals { if let Some(&cv) = live_local_values.get(s) { - if !matches!(cv, majit_ir::Value::Void) { + if !matches!( + cv, + majit_ir::Value::Void | majit_ir::Value::Ref(majit_ir::GcRef(0)) + ) { ctx.try_set_opref_concrete(v, cv); } } }🤖 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/state.rs` around lines 8734 - 8740, Update the seed_bridge_locals handling around live_local_values to skip majit_ir::Value::GcRef(0), matching the existing overlay_local logic. Only call ctx.try_set_opref_concrete for non-Void values that are not the null GcRef sentinel, while preserving all other stack-color seeding behavior.
🤖 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.
Outside diff comments:
In `@pyre/pyre-jit-trace/src/state.rs`:
- Around line 8734-8740: Update the seed_bridge_locals handling around
live_local_values to skip majit_ir::Value::GcRef(0), matching the existing
overlay_local logic. Only call ctx.try_set_opref_concrete for non-Void values
that are not the null GcRef sentinel, while preserving all other stack-color
seeding behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac1af268-32c9-43fb-9d11-37068dae940b
📒 Files selected for processing (1)
pyre/pyre-jit-trace/src/state.rs
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/f6a87117dca574726afa4830039bd112be7d2243/pyre-jit-trace/src/state.rs#L8364
When an earlier bridge_decode_box in this vable-array loop materializes a virtual, it can allocate and trigger a minor GC before later array items are decoded; those later objects are already safe in the live frame and get forwarded there, but val is decoded from the unforwarded raw fail_values/resume copy. Passing that val back into store_live_frame_array_slot can overwrite the forwarded locals_cells_stack_w slot with the stale pre-GC address, which is the same dangling-pointer hazard the code below explicitly avoids by reading concrete array values from the rooted live frame instead of decoded vable_array_values after allocations.
ℹ️ 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".
overlay_local resolves a bridge local from the vable image and then stamps its concrete from the live frame slot, previously skipping only Void. A loop-carried local held in a register at an interior guard reads NULL (GcRef(0)) from locals_cells_stack_w because it was never written back; stamping that hole folds a later may-force residual's Ref arg to concrete NULL and aborts with MayForceNullRefArgUnsupported. Skip a GcRef(0) source too, matching the deferred-overlay seed loop, so the box stays symbolic and the residual reads the runtime value. Assisted-by: Claude
…sd correction At bridge setup the multi-frame vsd correction in decode_and_restore_guard_failure (clear_stack_above with the innermost callee's depth applied to the root frame) nulls the root frame's register-resident loop-carried locals that sync_virtualizable_after_guard_failure had just restored from the resume decode. setup_bridge_sym then reads those slots as NULL from locals_cells_stack_w, so a hot `s = f(s, x)` loop whose branchy inlined callee guard fails stamps s's box with concrete NULL, folds a later may-force residual's Ref arg to NULL, and aborts the bridge with MayForceNullRefArgUnsupported — deopting to the blackhole on every crossing. Re-assert the resume-decoded vable array items into the live frame's locals_cells_stack_w per item inside the vvals decode loop, immediately after each item is decoded and before the alloc-heavy frame-register consume loops. The array items are uniformly Ref so no boxing is introduced; writing each item right after its decode roots it in the traced frame before the next decode allocates. The vable statics keep the corrected last_instr/valuestackdepth. This restores the write_boxes-runs-at-setup ordering the earlier synchronize skips on the multi-frame path. probeE (s = add(s, x), branchy inlined callee): 9.5s -> 0.024s, bridge compiles (loops_aborted 299 -> 0, guard_failures 285887 -> 201); acceptance bench 874x -> 0.9x. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ac9446f29
ℹ️ 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".
| store_live_frame_array_slot( | ||
| sym.concrete_vable_ptr as usize, | ||
| idx - vable_array_start, | ||
| val, | ||
| ); |
There was a problem hiding this comment.
When a virtualizable_values array slot decodes as a virtual that also has deferred pending field writes, this overwrites the live frame slot with the object materialized by setup_bridge_sym. The guard-recovery path has already applied all pending fields before syncing the vable (pyre/pyre-jit/src/eval.rs:8231), but this bridge-side materializer does not replay generic rd_pendingfields (it only special-cases current-exception fields in seed_bridge_pending_current_exception), so the overwrite replaces the correct object with a stale copy missing those lazy field/array writes and the bridge can trace from corrupted object state.
Useful? React with 👍 / 👎.
1 participant
Closes the branchy-inlined-callee bridge gap (#343 depth-1 shape): a hot
s = f(s, x)loop whose data-branchy inlined callee guard-fails now compiles a bridge instead of deopting to the blackhole on every crossing. Two commits, both insetup_bridge_symon the pyre side, no eval.rs/jitdriver.rs changes.Commit 1 — overlay NULL-skip (
10b621bd29)overlay_localstamped a bridge local's concrete from the live frame slot, skipping onlyVoid. A loop-carried local held in a register at an interior guard reads NULL (GcRef(0)) fromlocals_cells_stack_w(never written back); stamping that hole folds a later may-force residual'sRefarg to concrete NULL →MayForceNullRefArgUnsupported. Skip aGcRef(0)source too, matching the deferred-overlay seed loop, so the box stays symbolic and the residual reads the runtime value.Commit 2 — re-assert the vable array image after the vsd correction (
4c8c0bf555)For the pure
s = add(s, x)shape,sync_virtualizable_after_guard_failurecorrectly restoressinto the heap frame at bridge entry — but the multi-frame vsd correction that runs next (clear_stack_abovewith the innermost callee's depth, in callee coordinates, applied to the root frame) then NULLs the root frame's register-resident loop-carried locals.setup_bridge_symreads NULL and the bridge aborts.Re-assert the resume-decoded vable array items into the live frame's
locals_cells_stack_w, per item inside the vvals decode loop, immediately after each decode (before the alloc-heavy frame-register consume loops). Items are uniformlyRefso no boxing; each write roots the value in the fully-walked frame array before the next decode allocates. The vable statics keep the correction'slast_instr/valuestackdepth. This restores thewrite_boxes-runs-at-setup ordering the earlier synchronize skips on the multi-frame path. The vsd correction itself (load-bearing for the interpreter/blackhole fallback) is untouched.Evidence
probeE(s = add(s, x), branchy inlined callee): 9.5s → 0.024s (~396x), value correct; bridge compiles (loops_aborted299 → 0,guard_failures285887 → 201).bench/bridge_branchy_callee_regression.py: 874x → 0.9x PASS.probeF(2-level nested callee): correct.MAJIT_GC_STRESS=1, collection per alloc):probeEand a loop-carried GC-mobile Ref-object probe both matchPYRE_NO_JIT; bridge still compiles. The write target is the GC-traced frame array (walked in full every collection), so no stale-address hazard — unlike the earlier off-heap-shadow approach that was reverted.Known follow-up (pre-existing, not introduced here)
A virtual-typed vable array item is materialized by two non-shared caches (the guard-recovery
syncpath vssetup_bridge_sym'sBridgeVirtualCache), so such a slot can hold a distinct physical object with identical field values. Commit 2 makes the frame agree with the trace (an improvement); unifying the two materializers is a separate ticket.— commented by Claude
Summary by CodeRabbit
Ref(GcRef(0))) concretes, preventing invalid null references from affecting subsequent operations and improving runtime stability.