jit: drop the WITH_EXCEPT_START gate + with-frame exception-correctness fixes (#389) - #532
Conversation
|
Warning Review limit reached
Next review available in: 34 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 (4)
✨ 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 de09d65). 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)None. 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: bf740e31d8
ℹ️ 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".
| push_fresh_ref(&mut current_state, &mut graph); | ||
| current_depth += 1; | ||
| emit_vsd!(current_depth, py_pc); |
There was a problem hiding this comment.
With this gate removed, frames containing WITH_EXCEPT_START can now be traced, but the only lowering for that opcode is still this shadow-stack update: repo-wide search shows no residual/helper call for with_except_start. When a hot path actually takes an exceptional with exit, such as a context manager whose __exit__ swallows the exception or mutates state, the trace can pass through this opcode without invoking __exit__, so the compiled path observes the wrong boolean and misses required side effects. Please keep the structural decline until this arm emits the real helper semantics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15ea5baec6
ℹ️ 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 entry_carrier_call_py_pc == Some(resume_py_pc) { | ||
| crate::jitcode_dispatch::fbw_abort_outer_stack_overrides_clear(); |
There was a problem hiding this comment.
When the Entry carrier's flush_walk_end_state_at_outer_call declines, this comparison still treats the same resume PC as already handled because entry_carrier_call_py_pc was set before the commit result was known. For a LoopBearingCalleeInlineUnsupported path that also latched FBW_ABORT_OUTER_RESUME_PY_PC, this clears the rooted stack overrides and skips the flush_walk_end_state_to_frame_with_stack_overrides fallback, so an executed non-pure residual falls back to legacy replay instead of being resumed forward. Gate this skip on the Entry flush actually setting WALK_END_FLUSH_COMMITTED.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0e14b8462
ℹ️ 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".
| len, | ||
| bank: "i", | ||
| })?; | ||
| *slot = val; |
There was a problem hiding this comment.
When insert_renamings emits an integer cycle, this pop only restores registers_i[dst] and leaves concrete_registers_i[dst] holding whatever value the destination had before the swap. On paths where the concrete Int shadow is populated and a later goto_if_not/iL, switch/id, or int_return/i reads it, the walker can fold or return using stale concrete data even though the symbolic OpRef was moved correctly. Mirror the ref tmpreg path by carrying a tmpreg_i_concrete value from int_push/i and using write_int_reg on pop.
Useful? React with 👍 / 👎.
The per-mutator root walker fbw_store_journal_root_walker_area read the sys_exc journal from the current thread's TLS instead of the captured area, unlike every other journal it walks. Under a stop-the-world collection walking a paused mutator's area, this forwarded the collector thread's journal rather than the paused thread's, leaving a displaced sys_exc_value unrooted. Add a sys_exc field to FbwStoreJournalRootArea and walk it through area. Assisted-by: Claude
…gate) unsupported_jit_shape declined any frame containing WITH_EXCEPT_START as StructuralRegion, and the codewriter emitted emit_abort_permanent! for the op, keeping with-block loops and their callees out of the JIT. With the bridge exception-disposition fix in place, WITH_EXCEPT_START lowers as a plain residual (push the exit-function result, +1 stack effect) and the frame is admitted. The StructuralRegion variant stays for the census/JitSuppressionGuard machinery but is no longer produced. with_loop.py now compiles (loops_compiled=2) with the correct result. Assisted-by: Claude
…s the same outer resume The gh#467/#495 inline-abort flush ran two arms on a single LoopBearingCalleeInlineUnsupported abort: the Entry-carrier arm (depth-validated flush_walk_end_state_at_outer_call) and the executed-nonpure arm (flush_walk_end_state_to_frame_with_stack_overrides, reading FBW_ABORT_OUTER_RESUME_PY_PC). When both described the same outer resume pc, the second arm committed a forward-flush that the first had already handled (by commit or by depth-mismatch decline), overwriting a caller operand-stack slot — a bound method stored in a local read back as the receiver, so a `local(args)` call dispatched through the receiver's __call__. Track the Entry carrier's call_py_pc; when the executed-nonpure arm's resume pc matches it, clear the override stash and skip the second flush. The arm still runs for outer-resume stashes with no matching Entry carrier. Assisted-by: Claude
…f tagged-int support A kept-stack branch-guard bridge resumed through `bridge_registers_r` skipped seeding a reserved red color unless `tagged_int::CAN_BE_TAGGED` was set. With that flag false, a real operand living in a non-frame reserved red color (the free-register allocator reuses the portal EC color for a live operand at PCs with no live EC read) was left holding the stale `ConstPtr(ec)` seed. The residual CALL then read that stale pointer as its callable, deterministically routing a wrong value into `re/_parser` on the `re.match` path (surfacing as a SIGSEGV dereferencing a bogus ob_type, or a wrong-type value reaching _compile/_parse). The seeding is independent of tagged-int support: whenever the bridge names a genuine operand (an opref other than the pre-seeded ec_box/ frame_box) in a reserved red color, seed the real operand. Only the frame color stays protected, since overwriting its standard virtualizable identity forces the nonstandard vable-finish leg. Assisted-by: Claude
The FBW walker declined `ref_push`/`ref_pop`/`int_push`/`int_pop`/
`float_push`/`float_pop` at the catch-all `UnsupportedOpname`. These are
the blackhole `tmpreg_{r,i,f}` scratch ops (blackhole.py:661-679) that
`insert_renamings` (flatten.py:154) emits to break a cyclic parallel move
(a register swap `r_a <-> r_b` lowers to `push r_b; copy r_b<-r_a;
pop r_a`).
Add `tmpreg_r`/`tmpreg_i`/`tmpreg_f` (plus the Ref-bank concrete shadow)
to `WalkSession` and handle the six ops, mirroring `ref_copy/r>r`: push
reads a source register into the tmpreg, pop writes it back into a dst
register in lock-step with its concrete shadow. No IR op recorded (pure
SSA-level scratch move).
Assisted-by: Claude
`try_walker_specialize_load_attr` elided `guard_value(self.map, C_map)` whenever `box_value(map_op) == Some(Int(map))`. Every traced getfield op carries its live concrete value (`opimpl_getfield_gc_i` → `set_opref_concrete`), so that equality holds on the trace's very first map read and the guard was dropped entirely. `guard_class` pins only `ob_type == INSTANCE_TYPE`, shared by all boxed instances, not the map layout, so a trace compiled for one map re-entered by a same-class instance with a different map read `storage[storageindex]` at the wrong slot. Emit the map guard unless `map_op` is already a compile-time constant, then `replace_box(map_op, map_const)` so a later fold on the same receiver in-trace elides — matching the trait fold path (`implement_guard_value`). Fixes test_shlex `read_token` (IndexError on empty deque / SIGSEGV), where shlex instances across the suite carry multiple maps. Assisted-by: Claude
1 participant
Removes the
WITH_EXCEPT_STARTJIT gate so hot loops inside awith-block are admitted for tracing, and lands the exception-correctness and resume fixes that the gate previously masked.Commits (rebased onto current
main)0c527d51591jit: read the FBW sys_exc journal through the captured root area — the walker readFBW_SYS_EXC_JOURNALvia TLS on the store-journal path; on abort rollback this could diverge from the captured root area. Now threads the journal cell through the captured root area so the walker and the rollback see the same store.5e03d05dfc7jit: preserve exception disposition on an uncaught exception-guard bridge (#389c root Optimize info transfer in replace_op for AbstractInfo handling #6) — a loop-freewith-callee trace reaching the no-replay portal exit swallowed an exception-guard bridge's raise, so e.g.re.compile('\\')intermittently did not raise. The bridge resolver now returns the exception continuation for an uncaught exception-guard bridge instead of the captured concrete return.4f84553d51bjit: admit with-block frames for tracing (drop the WITH_EXCEPT_START gate) — deletes theemit_abort_permanent!at theWithExceptStartcodewriter arm and theStructuralRegiondecline inunsupported_jit_shape.with-frames now trace as plain residual regions.948c40dca36jit: skip the executed-nonpure abort-flush when the entry carrier owns the same outer resume (test_fnmatch) — a double abort-flush at the same outer resume pc corrupted a bound-method local in a nestedFOR_ITER. The executed-nonpure arm now declines when the Entry-carrier arm already owns that resume.15ea5baec63jit: seed a bridge-named operand in a reserved red color regardless of tagged-int support (test_strftime) — the bridge red-color seeding was gated onCAN_BE_TAGGED, leaving a staleConstPtr(ec)callable on no-token and a SIGSEGV. The seeding now runs unconditionally.Status
Every baseline-gated regression the gate drop exposed is fixed and green:
test_range(baseline-gated) —948c40dca3615ea5baec63pyframe.rs:2098(baseline-gated) — no longer reproduces after the rebase onto currentmain; the super-instruction resume defect it hit is subsumed by the upstream resume-frame rework (jit: #369 pc-word flip — resume frame collapses to RPython 2-word shape #536 / majit/wasm: GC-lifetime fixes + dict/set O(N²) and module-scope JIT resume defects #543). The dedicated resume commit that previously fixed it was dropped as redundant — against the new resume-frame shape it miscompiled thegh#498branch-resume guard (synth/closure_freevar_branch_list_cell).re.PatternError/OverflowError) escapes awith assertRaises(...)block instead of being suppressed (non-gated;test_rebaseline is already FAIL, no CI impact) — #389c root Align exception table to upstream #4-alt, tracked as a follow-up. RCA is still open: the escape route traces asCompareOp -> PopJumpIfFalse -> RaiseVarargs(not the with-cleanup opcodes); theWITH_EXCEPT_STARTresult-slot hypothesis was investigated and refuted.check.py --backend dynasm: 181/181 (rebased ontomainat928139e6d95).All exposed miscompiles reproduce only with the JIT on (
PYRE_NO_JIT=1is clean), confirming they are trace-path bugs, not interpreter regressions.🤖 Generated with Claude Code