Repository navigation
jit: branch-arm hazard scoping to invalid mirror + inlined-callee abort_permanent decline (gh#467) - #471
Conversation
|
Warning Review limit reached
Next review available in: 31 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 (8)
WalkthroughChangesAArch64 Call ABI
JIT Forward-Flush Recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant InlineDispatch
participant AbortUnwind
participant WalkTrace
participant OuterFrame
InlineDispatch->>InlineDispatch: record executed-effect state
InlineDispatch->>AbortUnwind: latch CALL pc and operand stack
AbortUnwind->>WalkTrace: root saved PyObjectRef values
WalkTrace->>OuterFrame: flush_walk_end_state_at_outer_call
OuterFrame-->>WalkTrace: restore frame or reject flush
WalkTrace->>OuterFrame: re-execute CALL or use legacy replay
Possibly related issues
Possibly related PRs
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 |
![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: 9da2dea0e9
ℹ️ 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 cand.is_null() || (*cand).ob_type as *const () as usize != function_type_addr { | ||
| return false; | ||
| } | ||
| let callee_w_code = pyre_interpreter::function_get_code(cand); |
There was a problem hiding this comment.
When a hot loop references an imported/module builtin created with make_builtin_function* (for example from sys import getdefaultencoding), the object has FUNCTION_TYPE but its code is a BuiltinCode, not a PyCode. This gate admits it, then function_get_code feeds that BuiltinCode to sub_jitcode_body_for_code/jitcode_for, which treats the pointer as a PyCode via w_code_get_ptr and can panic or dereference bogus CodeObject memory during the upfront scan. Please also reject is_builtin_code(callee_w_code as PyObjectRef) before trying to build/scan a sub-jitcode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 0b607d1. make_builtin_function* wraps a BuiltinCode in a FUNCTION_TYPE object via function_new_with_fixed_code, so an imported builtin in module globals passed the gate and its BuiltinCode reached sub_jitcode_body_for_code/w_code_get_ptr as if it were a PyCode. The scan now rejects is_builtin_code right after the null/visited check. Regression repros (builtin import called in a hot loop with a heap store, alone and combined with a hidden callee) run without crash and match the interpreter; check.py 161/161 on both backends.
— commented by Claude
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 4a776ce). 1. 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
|
9da2dea to
0b607d1
Compare
…t scan make_builtin_function* wraps a BuiltinCode in a FUNCTION_TYPE object (function_new_with_fixed_code), so a builtin imported into module globals (from sys import getsizeof) passed the exact-FUNCTION_TYPE gate in loop_inlines_abort_permanent_callee and its BuiltinCode reached sub_jitcode_body_for_code / w_code_get_ptr, which cast it as a PyCode CodeObject and dereferenced garbage during the up-front scan. Reject is_builtin_code before scanning; a builtin carries no traceable body and never inlines. Found by Codex review on PR #471. Repros: builtin import called in a hot loop with a heap store, alone and combined with a hidden callee — no crash, output identical to the interpreter. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b607d13b0
ℹ️ 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!(e, DispatchError::AbortPermanentMarkerReached { .. }) | ||
| && is_top_inline | ||
| && fbw_concrete_heap_write_count() == concrete_writes_before | ||
| { |
There was a problem hiding this comment.
When the caller has already hit a residual that try_execute_residual_call_via_executor declined, fbw_mark_unjournaled_effect() means that side effect has not run and depends on the legacy replay from loop entry. This gate only checks that the odometer did not move during the callee, so a pre-CALL unjournaled effect is included in concrete_writes_before and still allows the CALL-forward flush; run_perfn_walk then resumes the frame at the CALL and skips the earlier unexecuted residual entirely. Require that no unjournaled effect was present at the CALL before latching this forward resume.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9609b6e (slice 2 of the CALL-forward flush, originally pushed as 9d08d3c before the rebase). Exactly as suggested: try_walker_inline_user_call now snapshots the unjournaled-effect flag at sub-walk entry (unjournaled_before_subwalk), and the latch requires the flag to have been clear at the CALL — a pre-CALL declined residual therefore forbids the forward flush and the walk stays on legacy replay from loop entry. The odometer was also split so it counts only executed effects (FBW_EXECUTED_EFFECT_COUNT); a mark raised inside the discarded zero-executed-effect callee no longer poisons the latch, while an outside mark still does. Covered by the m3_hidden_* battery; check.py 161/161 on both backends.
— commented by Claude
…t scan make_builtin_function* wraps a BuiltinCode in a FUNCTION_TYPE object (function_new_with_fixed_code), so a builtin imported into module globals (from sys import getsizeof) passed the exact-FUNCTION_TYPE gate in loop_inlines_abort_permanent_callee and its BuiltinCode reached sub_jitcode_body_for_code / w_code_get_ptr, which cast it as a PyCode CodeObject and dereferenced garbage during the up-front scan. Reject is_builtin_code before scanning; a builtin carries no traceable body and never inlines. Found by Codex review on PR #471. Repros: builtin import called in a hot loop with a heap store, alone and combined with a hidden callee — no crash, output identical to the interpreter. Assisted-by: Claude
0b607d1 to
9d08d3c
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/9d08d3c54cdf12590395b3b7291b5c7d464589f3/pyre-jit-trace/src/trace.rs#L1840-L1843
This upfront decline treats every plain FUNCTION_TYPE value as a possible inlined callee, but the actual inline path rejects several shapes before walking the body (for example closures and zero-arg functions in try_walker_inline_user_call). When such a non-inlineable helper is merely referenced/called from the loop and its body contains an abort_permanent, this scan still disables the whole full-body walk even though the callee would have stayed a residual interpreter call and could not trigger the inlined-callee replay bug. Please mirror the inline eligibility gates here (at least reject closure/zero-param bodies via resolve_inlinable_callee) before scanning for abort_permanent.
ℹ️ 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".
9d08d3c to
9609b6e
Compare
…t scan make_builtin_function* wraps a BuiltinCode in a FUNCTION_TYPE object (function_new_with_fixed_code), so a builtin imported into module globals (from sys import getsizeof) passed the exact-FUNCTION_TYPE gate in loop_inlines_abort_permanent_callee and its BuiltinCode reached sub_jitcode_body_for_code / w_code_get_ptr, which cast it as a PyCode CodeObject and dereferenced garbage during the up-front scan. Reject is_builtin_code before scanning; a builtin carries no traceable body and never inlines. Found by Codex review on PR #471. Repros: builtin import called in a hot loop with a heap store, alone and combined with a hidden callee — no crash, output identical to the interpreter. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9609b6e1d2
ℹ️ 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 let Some(body) = crate::state::sub_jitcode_body_for_code(callee_w_code) { | ||
| for op in crate::jitcode_runtime::decoded_ops(body.code) { | ||
| if op.opname == "abort_permanent" { | ||
| return true; |
There was a problem hiding this comment.
At this point the pre-scan treats any FUNCTION_TYPE whose body contains abort_permanent as an inline-abort hazard, but the actual try_walker_inline_user_call path only sub-walks exact positional, closure-free callees and returns Ok(None) for shapes such as closures (and other non-inlineable arities). In a hot loop that references or calls a closure helper with an unsupported opcode, this new scan will decline full-body walking even though that callee would have gone through the residual interpreter and could not trigger the inline-callee abort replay bug; please apply the same inline-eligibility gates before returning true here.
Useful? React with 👍 / 👎.
|
Re: the P2 "Skip callees that the walker cannot inline" review — confirmed and fixed in f971f09. One honest consequence: the precision fix removes an accidental mask in one adversarial scratchpad repro (a zero-parameter factory referenced by the loop used to decline the whole walk), re-exposing the documented executed-effect callee-abort residual that is deferred to the #126/#215 forward-resume convergence (tracked in gh#467). — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f971f09740
ℹ️ 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 !provably_side_effect_free | ||
| && (writes_live_heap | ||
| || heap_write_odometer_before | ||
| .is_some_and(|before| pyre_interpreter::call::frame_entry_count() != before)) |
There was a problem hiding this comment.
When a top-level inlined callee advances an already-live iterator via FOR_ITER and then hits a supported abort/nested LoopBearingCalleeInlineUnsupported before any other counted effect, this gate does not bump the executed-effect odometer because ForIterNext is included in provably_side_effect_free above. The new CALL-forward latch only checks this odometer, so it can commit the outer frame, clear the in-flight FOR_ITER item, and re-execute the CALL, advancing the same iterator a second time; please either count ForIterNext consumes for this odometer or block the forward flush while an in-flight item exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bd8976abb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let Some(cand) = | ||
| pyre_object::dictmultiobject::w_dict_getitem_str(globals, name.as_str()) | ||
| else { |
There was a problem hiding this comment.
When a referenced call target is resolved through the frame's builtins fallback rather than the module globals (for example builtins.h = helper with no global h, or a transitive LOAD_GLOBAL miss inside a callee), this scan skips it because it only probes w_globals. The interpreter/JIT LOAD_GLOBAL path falls back to the frame's picked w_builtin on a globals miss, so such a helper can still be inlined; if it executes any counted effect before hitting abort_permanent, the CALL-forward flush is refused and the legacy replay can double-apply the caller's pre-CALL heap write. Please mirror the builtins lookup here before deciding no aborting callee is reachable.
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-jit-trace/src/state.rs`:
- Around line 3906-3915: Guard the raw code pointer before liveness analysis: in
the code block producing raw_code, check whether the pointer is null and
decline/return through the existing failure path before calling
crate::liveness::liveness_for. Match the null-check pattern used by the other
w_code_get_ptr call sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8c41730-d87f-4cd1-8f83-b414511ffbf2
📒 Files selected for processing (4)
majit/majit-backend-dynasm/src/aarch64/assembler.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8a116ef77
ℹ️ 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 nparams > body.num_regs_r || !visited.insert(callee_w_code) { | ||
| return false; |
There was a problem hiding this comment.
When two distinct function objects share the same CodeObject but have different __globals__ (for example via types.FunctionType(code, globals_a) and types.FunctionType(code, globals_b)), this code-only visited entry causes the second function's globals never to be scanned transitively. If the first globals bind h to a clean helper and the loop actually calls the second function whose LOAD_GLOBAL h reaches an abort_permanent helper, the upfront decline is missed and the original inline-callee abort/double-apply path remains reachable; the scan should remember at least (callee_w_code, callee_globals) for transitive global resolution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e02d5f6d6
ℹ️ 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".
| | pyre_interpreter::Instruction::BinaryOp { .. } | ||
| | pyre_interpreter::Instruction::CompareOp { .. } |
There was a problem hiding this comment.
In the mid-body abort path, try_commit_midbody_abort runs execute_frame on the rebuilt callee before deciding whether the outer frame can be flushed; if this post-marker suffix contains BINARY_OP/COMPARE_OP on user objects, the interpreter can enter __add__/__eq__, mutate heap, and then raise. Because an Err here falls back to the legacy replay, that mutation remains and the loop/call is executed again, reintroducing the double-apply that gh#467 is trying to avoid. Only opcodes that cannot dispatch to user code or commit observable effects should pass this check.
Useful? React with 👍 / 👎.
…t scan make_builtin_function* wraps a BuiltinCode in a FUNCTION_TYPE object (function_new_with_fixed_code), so a builtin imported into module globals (from sys import getsizeof) passed the exact-FUNCTION_TYPE gate in loop_inlines_abort_permanent_callee and its BuiltinCode reached sub_jitcode_body_for_code / w_code_get_ptr, which cast it as a PyCode CodeObject and dereferenced garbage during the up-front scan. Reject is_builtin_code before scanning; a builtin carries no traceable body and never inlines. Found by Codex review on PR #471. Repros: builtin import called in a hot loop with a heap store, alone and combined with a hidden callee — no crash, output identical to the interpreter. Assisted-by: Claude
0e02d5f to
855414f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 855414fe56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let Ok(mut retval) = frame.execute_frame(None, None) else { | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
When this mid-body path is selected, the inlined callee has already moved the executed-effect counter before the abort marker; if an allowed post-marker opcode can raise without mutating (for example LOAD_FAST_CHECK on an unbound local), execute_frame returns Err here. Returning false leaves the walk uncommitted, so the epilogue falls back to the legacy replay from loop entry and re-executes the already-committed prefix effect that this path was meant to avoid. Please reject suffixes that can raise until forward exception delivery exists, or commit the outer frame/exception instead of replaying after the rebuilt callee has run.
Useful? React with 👍 / 👎.
…t scan make_builtin_function* wraps a BuiltinCode in a FUNCTION_TYPE object (function_new_with_fixed_code), so a builtin imported into module globals (from sys import getsizeof) passed the exact-FUNCTION_TYPE gate in loop_inlines_abort_permanent_callee and its BuiltinCode reached sub_jitcode_body_for_code / w_code_get_ptr, which cast it as a PyCode CodeObject and dereferenced garbage during the up-front scan. Reject is_builtin_code before scanning; a builtin carries no traceable body and never inlines. Found by Codex review on PR #471. Repros: builtin import called in a hot loop with a heap store, alone and combined with a hidden callee — no crash, output identical to the interpreter. Assisted-by: Claude
855414f to
331a77c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 331a77cc06
ℹ️ 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 let Some(err) = propagated_exception { | ||
| return Some(LoopResult::Done(Err(err))); |
There was a problem hiding this comment.
When the new mid-body abort path sets WALK_END_PROPAGATED_EXCEPTION at a merge-point trace site, this early return bypasses the post-trace cleanup just below that clears meta.tracing_call_depth and the starting trace bookkeeping. If the exception is caught and execution continues, eval_loop_jit keeps using the stale depth in its merge-point gating, so subsequent execution can skip or mis-route trace feeding until some unrelated path clears it. Please run the existing cleanup before returning the propagated exception.
Useful? React with 👍 / 👎.
TO_BOOL never reaches classify_vstack_opcode: the codewriter lowers
Instruction::ToBool to zero JitCode instructions (codewriter.rs
`Instruction::ToBool => {}`), so the py-pc boundary mapping never
yields its pc. Remove TO_BOOL from the ResultToTos doc list and note
in the Unmodeled arm why it cannot appear there.
Assisted-by: Claude
Hazard (1) of the branch-guard resume decline (reads_null_ref) ran the branch_arm_reads_unrestorable_ref register scan regardless of walk-mirror validity, while Hazards (2)/(3) were already scoped to !vstack_valid. A valid #73 walk mirror sources kept operand-stack slots from vstack_boxes and kept locals from the vable shadow per-slot on resume, so the register scan only proves a hazard for the invalid mirror. Gate Hazard (1) identically. capture_resumedata (pyjitpl.py:2610-2624) appends virtualizable_boxes and the framestack to every guard's resume data, so the not-taken arm's vable and kept-slot reads are always restorable upstream. exec_defined_global_read and loops_comprehension gain one compiled loop each with output identical to the interpreter; check.py 160/160 on both dynasm and cranelift; pyre-jit-trace and pyre-jit test suites pass. Assisted-by: Claude
…nent loop_body_has_abort_permanent scans only the top-level per-CodeObject jitcode, so an abort_permanent inside an inlined callee slipped past it: a non-journaled concrete heap store in the loop body executed during the walk, the callee sub-walk aborted, and the epilogue replayed from loop entry, re-executing the store (d[0]=d[0]+1 over-counted 300001 vs 300000). Add loop_inlines_abort_permanent_callee as a sibling up-front decline: resolve candidate callees concretely before the walk (module globals via co_names, root-frame fastlocals and closure cells), gate to plain user functions, scan their jitcode bodies transitively for abort_permanent, and decline the walk so the loop re-interprets. Over-declining stopgap until the forward-resume convergence (#126/#215); callees reached via attributes, containers, call results, or deeper frames remain unresolvable up front and are documented as the residual. Corpus over-decline census: 0 of 201 programs newly declined. Repros (global helper, extend variant, arg-passed helper) flip to interpreter-identical output; a clean-helper control still compiles. check.py 160/160 dynasm; pyre-jit-trace tests pass. Assisted-by: Claude
… abort (gh#467) When an abort_permanent marker fires inside a top-level inline sub-walk whose callee committed no heap effect, flush the outer frame at the CALL that entered the callee and resume the interpreter re-executing that CALL, instead of the legacy replay from loop entry that re-applied a non-journaled pre-CALL store (the gh#467 double-apply). try_walker_inline_user_call latches (outer CALL py_pc, concrete [callable, null_or_self, args...]) only when a new concrete-heap-write counter is unmoved across the sub-walk; the carrier is GC-rooted through the store-journal root walker. flush_walk_end_state_at_outer_call flushes locals from the vable shadow, rebuilds the operand stack from the concrete call operands, cross-checks the height against the forward analysis depth_at_py_pc, and declines all-or-nothing on any mismatch (the caller then keeps the legacy replay). Journals commit on this path. Frame-level analogue of run_blackhole_interp_to_cancel_tracing (pyjitpl.py:2949) without the inner-frame rebuild (#126/#215). The up-front callee scan stays: it covers named effectful callees the forward-flush gate declines; hidden effect-free callees (factory-made, container-held, call-returned) are covered by this path. Hidden EFFECTFUL callees remain on the legacy path, unchanged. Repros: hidden dict/return/attr callee shapes flip 300001 to 300000; named-callee and journaled-store shapes unchanged; effectful-callee gate declines; clean-helper control still compiles. check.py 161/161 dynasm and 161/161 cranelift; pyre-jit-trace and pyre-jit tests pass; GC stress on the hidden repros passes. Assisted-by: Claude
…t scan make_builtin_function* wraps a BuiltinCode in a FUNCTION_TYPE object (function_new_with_fixed_code), so a builtin imported into module globals (from sys import getsizeof) passed the exact-FUNCTION_TYPE gate in loop_inlines_abort_permanent_callee and its BuiltinCode reached sub_jitcode_body_for_code / w_code_get_ptr, which cast it as a PyCode CodeObject and dereferenced garbage during the up-front scan. Reject is_builtin_code before scanning; a builtin carries no traceable body and never inlines. Found by Codex review on PR #471. Repros: builtin import called in a hot loop with a heap store, alone and combined with a hidden callee — no crash, output identical to the interpreter. Assisted-by: Claude
…llee aborts (gh#467) The forward-flush latch armed only on AbortPermanentMarkerReached, but the hidden effectful-callee repro never reaches that marker: its store residual is declined by the concrete executor and the sub-walk exits earlier with LoopBearingCalleeInlineUnsupported. A declined residual did not execute, so the whole callee attempt is discardable and re-executing the CALL in the interpreter runs the callee exactly once — observationally the same as never having inlined the call (tracing aborts and switch_to_blackhole re-runs it). Rename FBW_CONCRETE_HEAP_WRITE_COUNT to FBW_EXECUTED_EFFECT_COUNT and make it count only effects actually applied (successful effectful executor calls, user-frame entry, journal pushes); fbw_mark_unjournaled_effect no longer bumps it. Snapshot the unjournaled flag at sub-walk entry so a mark raised inside the discarded callee cannot poison the latched flush while a pre-existing outside mark still forbids it. Arm the latch and the trace.rs consumer for both abort classes; ResidualCallArgUnbound stays ineligible. A non-latched nested-decline abort keeps legacy replay. m3_hidden_gstore flips to interpreter-identical (300000, 300000); a new adversary whose hidden callee executes a store before aborting stays gated on the legacy path with its callee store applied exactly once. check.py 161/161 dynasm and 161/161 cranelift; pyre-jit-trace and pyre-jit tests pass; GC-stress runs match the interpreter. Assisted-by: Claude
…rker (gh#467) The FBW walker executes residuals concretely; when a top-level inlined callee runs a concrete effect and then reaches an abort_permanent marker (e.g. MATCH_SEQUENCE) inside its sub-walk, the marker pc is a callee coordinate with no outer meaning, so every landed flush declined and the walk replayed from loop entry, re-applying the outer non-journaled store (m3_hidden_executed_then_abort: (300001,300000) vs (300000,300000)). Add a depth-1 inner-frame carrier: capture the callee w_code, exact callee pc (via the prereq-1 exact first-jit anchor first_jit_pc_by_py_pc[py] == abort_jit_pc), live stack refs from the callee register bank, and live locals from the callee-local shadow into a GC-rooted MidBodyPayload before the sub-walk guards drop; consume it in one trace.rs leg that rebuilds the callee FrameBox, runs it forward in the plain interpreter from the marker pc, delivers the return into the outer post-CALL slot, and resumes the outer frame past the CALL. This is the depth-1 specialization of run_blackhole_interp_to_cancel_tracing / _copy_data_from_miframe / _setup_return_value_r. All-or-nothing gates decline to legacy (never worse) on: nested inline (depth>1), generator/coroutine, raw cellvars/freevars, missing exact first-jit anchor, unsourceable live stack/local slot, non-statement-call outer shape, lastblock/capacity mismatch, and — validated BEFORE running the callee — a post-marker body that is not effect-free. The effect-free whitelist is fail-closed: only opcodes proven free of heap/global/cell mutation and user calls (load/store-fast, match/unpack, binary/compare, jumps, return) may reach execute_frame, so an Err (implicit raise) cannot have committed an effect the subsequent legacy fallback would double. Landed zero-executed-effect Entry (Rung-B) semantics are unchanged; LocalsRoot is exposed as FrameLocalsRoot for the trace-side rebuild. Deferred (decline-to-legacy = status quo): forward exception delivery for a raising callee (including a post-marker user-dunder that mutates then raises), subexpression-position calls, and depth>1 (#126/#215). check.py 161/161 dynasm and cranelift; 39/39 scratchpad battery matches the interpreter; GC stress clean. Assisted-by: Claude
…bling the structural fold (gh#467) When the mid-body abort carrier rebuilds an inlined callee and runs it forward, an exception raised by that callee is now delivered to the outer frame at the CALL instead of declining to legacy replay (which double-applies the outer non-journaled store). Parity: _resume_mainloop / _exit_frame_with_exception (blackhole.py:1612-1682); sys.exc_info save/restore stays an interpreter-level concern (PUSH_EXC_INFO/POP_EXCEPT). - try_commit_midbody_abort: on execute_frame Err at a portal trace site, restore the outer pre-CALL sys_exc_value, position the outer frame at the CALL pc with the empty statement-position operand stack, and run handle_exception. A matching handler resumes at the handler pc through the existing WALK_END_FLUSH_COMMITTED seam; no handler stores the exception in the new WALK_END_PROPAGATED_EXCEPTION channel, which both portal tracing entry sites consume as LoopResult::Done(Err). The store journal commits either way (the outer store is real, not doubled). - Remove the effect-free / empty-exception-table pre-execution gate for portal traces, where Err now has a forward channel. Bridge trace sites have no PyError propagation channel yet and retain the former gate (fail-closed). - E-G2: accept only an exactly reconstructible depth-0 handler stack; any handler preserving an operand below the call stays on legacy replay. - Extend the carrier to LoopBearingCalleeInlineUnsupported (MidBodyAbortKind::Structural) when the failing opcode applied no observable effect (FBW_STRUCTURAL_ABORT_OPCODE_EFFECTS zero-delta) and inverts to an exact first-jit anchor; the callee resumes from the enclosing opcode boundary. All existing generator/cell/freevar/liveness/GC-root gates are kept. - Factor write_back_outer_locals out of flush_walk_end_state_after_outer_call and reuse it on the exception-delivery path. refute_double_store flips to interpreter parity (300000, 300000, 300000). The outer-CALL try-region shape declines the enclosing CALL before the carrier forms and remains a documented residual. Assisted-by: Claude
…er inline decline (gh#467) try_walker_inline_user_call declined a multiframe callee whose CALL sits inside a try block before entering the callee sub-walk: compute_inline_caller_frame returns no paused parent frame when the call carries an after-residual catch marker, and the resulting LoopBearingCalleeInlineUnsupported error carried no abort carrier, so the walk fell back to legacy replay from the loop entry and re-applied non-journaled stores that preceded the CALL. Distinguish that decline reason (InlineCallerFrameDecline::TryBlockCatchMarker) from the other unavailable-frame reasons, hoist the existing Entry-latch inputs above the paused-parent-frame gate, and latch the CALL-forward Entry carrier (fbw_set_abort_call_resume) before returning the unchanged error when every existing Entry predicate holds. The callee was never entered at this decline, so re-running the whole CALL forward is observationally identical to never inlining it (pyjitpl.py:2949). Other decline reasons keep the bare error and legacy replay. Assisted-by: Claude
…llees (gh#467) The always-portal flip (9e1dc2e) gives an inlined callee body the portal frame-vable prologue, which broke the executed-effect MidBody carrier in two places; the walk declined to legacy replay and re-applied non-journaled outer stores. First, serialize_op's first-wins rule hands the marker's python-pc anchor slot to the adjacent same-pc setfield_vable_i(last_instr) write, so the exact_first_jit_anchor equality never holds in portal shape. Add portal_marker_first_jit_anchor: a Marker abort also anchors when every op between the first jit pc and the marker is that same-pc portal last_instr bookkeeping write. Structural aborts keep the exact anchor. Second, the live operand-stack recovery reads pcdep-colored concrete registers, but a portal-shaped callee keeps those values in the vable shadow. Tag each concrete callee-shadow entry with its writer frame register and let callee_vable_ref_at accept either the strict-fold witness or an innermost-scope entry whose recorded register matches the callee metadata's portal_frame_reg. Recovery still declines on Int/Float/null/missing and foreign-frame entries. m3_hidden_executed_then_abort now matches the interpreter; the PYRE_ALWAYS_PORTAL=0 path keeps its pre-flip commit unchanged. check.py 161/161 dynasm and cranelift, three runs each; cargo tests green on both backends; GC stress green. Assisted-by: Claude
331a77c to
286c1ee
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 286c1ee408
ℹ️ 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".
| | pyre_interpreter::Instruction::JumpBackward { .. } | ||
| | pyre_interpreter::Instruction::JumpBackwardNoInterrupt { .. } |
There was a problem hiding this comment.
When the bridge mid-body replay path (allow_propagate_out = false) resumes a callee at an abort marker inside a loop, allowing JumpBackward/JumpBackwardNoInterrupt here means execute_frame can leave the vetted post-marker suffix and run earlier bytecode that was not checked by midbody_post_marker_is_effect_free (for example a prior heap mutation in the loop body). If that replay later raises and returns false, the caller falls back to legacy replay from the loop entry, so the jumped-to mutation is left committed and then executed again; reject backward targets before start_pc or include their target range in the safety check.
Useful? React with 👍 / 👎.
What
Three JIT tracing fixes, census-driven:
Scope the branch-arm unrestorable-Ref hazard to the invalid walk mirror
(
jitcode_dispatch.rs). Hazard (1) of the branch-guard resume decline(
reads_null_ref) ran thebranch_arm_reads_unrestorable_refregister scanregardless of walk-mirror validity, while Hazards (2)/(3) were already scoped
to
!vstack_valid. A valid Eliminatepc_mapand per-PC labels by switching the tracer to interpret JitCode bytecode #73 walk mirror sources kept operand-stack slotsfrom
vstack_boxesand kept locals from the vable shadow per-slot on resume,so the register scan only proves a hazard for the invalid mirror.
capture_resumedata(pyjitpl.py:2610-2624) appendsvirtualizable_boxesandthe framestack to every guard's resume data, so these reads are always
restorable upstream. Soundness was force-verified: with the decline force-
disabled, the two affected guards re-executed their not-taken arms across
594+404 real guard failures with output byte-identical to the interpreter.
exec_defined_global_readandloops_comprehensioneach gain one compiledloop.
Decline the walk when an inlined callee body carries
abort_permanent(
trace.rs, gh#467 stopgap).loop_body_has_abort_permanentscans only thetop-level jitcode, so an
abort_permanentinside an inlined callee slippedpast it: a non-journaled concrete heap store executed during the walk, the
callee sub-walk aborted, and the epilogue replayed from loop entry —
re-executing the store (
d[0] = d[0] + 1over-counted 300001 vs 300000,deterministic). The new
loop_inlines_abort_permanent_calleeresolves calleecandidates concretely before the walk (module globals via
co_names,root-frame fastlocals, closure cells), scans their jitcode bodies
transitively, and declines up front so the loop re-interprets (correct, at
interpreter speed). This is a documented over-declining adaptation: upstream
has no up-front scan — an unsupported op raises
SwitchToBlackholeandrun_blackhole_interp_to_cancel_tracing(pyjitpl.py:2949) continues forwardin the blackhole interpreter, so nothing ever replays. Callees unresolvable
before the walk (attributes, container elements, call results) remain the
residual tracked in gh#467; the convergence is the fib_recursive: recursive CALL_ASSEMBLER lands (now faster than CPython); residual ~4-5x-vs-pypy floor needs flat/virtualized jitframe #126/fib_recursive major performance blockers #215 forward-resume
work.
Correct stale TO_BOOL references in the vstack classifier docs
(
jitcode_dispatch.rs, doc-only). TO_BOOL emits no JitCode(
Instruction::ToBool => {}in the codewriter) and never reachesclassify_vstack_opcode.Validation
check.pydynasm 160/160 + cranelift 160/160.cargo test -p pyre-jit-traceand-p pyre-jit(dynasm): all green.extendvariant, arg-passed helper) flip tointerpreter-identical output; a clean-helper control loop still JIT-compiles
(~7.6×).
declined.
Codex parity review
Sections 1/2: the two findings are the stopgap's own over-declining/static
nature — accepted as a deliberate, documented adaptation (the in-code comment
cites
run_blackhole_interp_to_cancel_tracingas the orthodox mechanism andgh#467/#126/#215 as the convergence). Sections 3/4: pre-existing #125-family
and #215 P2 items, unchanged here; this diff narrows the
jitcode_dispatch.rskept-stack decline mismatch rather than widening it.Closes nothing; tracks gh#467.
🤖 Generated with Claude Code
Summary by CodeRabbit