Repository navigation
Exception-path JIT: fold exception construction/attrs/str, lower DELETE_FAST/DEREF - #562
Conversation
|
Warning Review limit reached
Next review available in: 13 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 (17)
✨ 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: 5aaf76f9a3
ℹ️ 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".
| // disposition `Raise` so the `Terminate` consumer builds a | ||
| // `TraceAction::Finish { exit_with_exception: true }`. | ||
| FBW_FINISH_PAYLOAD.with(|c| c.set(Some((exc, Type::Ref)))); | ||
| fbw_finish_raise_set(exc_concrete); |
There was a problem hiding this comment.
When the exception's concrete shadow is unavailable (for example an unseeded ref register on a bridge/resume path, where read_ref_reg_concrete returns ConcreteValue::Null), this now records Some(FinishConcrete::Raise(Null)). The no-replay epilogue only checks that the concrete stash is Some, so it can keep this value and the later eval.rs/call_jit.rs consumers hit unreachable!("FinishConcrete::Raise must hold a concrete Ref") instead of deopting or replaying. Keep the symbolic exception FINISH payload separate from the concrete no-replay result, or only stash a concrete raise when it is a non-null Ref.
Useful? React with 👍 / 👎.
| if matches!( | ||
| crate::jitcode_dispatch::fbw_finish_concrete_peek(), | ||
| Some(crate::jitcode_dispatch::FinishConcrete::Raise(_)) | ||
| ) { |
There was a problem hiding this comment.
When an uncaught raise follows an unjournaled effect, run_perfn_walk clears FBW_FINISH_CONCRETE before this Terminate mapping but leaves FBW_FINISH_PAYLOAD intact for replay. Because this new block uses the concrete no-replay stash as the only Raise discriminator, that case falls through to the normal-return payload handling below and compiles FINISH(exc, done_with_this_frame_descr_ref) instead of exit_frame_with_exception_descr_ref, making the replayed raising trace return the exception object. The raise/return disposition needs to be stored independently of the concrete no-replay value.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a601a48). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
|
Rebased onto Both findings traced to one root: the disposition was conflated with the no-replay value
The fix went further than "a separate disposition channel"The first attempt added a sibling So the disposition is carried in the payload: Auditing that fix surfaced a third defect in the same commit: in
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ddf972953
ℹ️ 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 result_value = emit_graph_op_with_result( | ||
| &mut graph, | ||
| ¤t_block.block(), | ||
| "store_deref_value", | ||
| vec![cell_value.into(), super::flow::Constant::none().into()], |
There was a problem hiding this comment.
When DELETE_DEREF is inside a try and the cell is empty, the preceding load_deref_value residual raises the NameError, but this added store_deref_value residual is also lowered as a Plain/can-raise residual. The flattening path anchors a block's catch_exception to the last can-raise residual, so the handler is placed after this store call instead of next to the load_deref_value guard; an unbound-cell del x in an enclosing except NameError path will therefore escape instead of being caught. Split before the store or make the store helper cannot-raise so the catch stays attached to the unbound check.
Useful? React with 👍 / 👎.
youknowone
changed the title
|
Force-pushed a reduced stack, rebased onto Why the wasm job was red. It is not a bug in that commit. The commit's only sin is that it made this
So wasm is the parity deviation, and it is latent for any trace that carries a A secondary, backend-symmetric hole was also confirmed and is in the same issue:
Gates on the pushed stack: dynasm 192/192, cranelift 192/192, wasm 190 passed / — commented by Claude |
|
The wasm GC parity gap described above is now tracked as #581 (three layered defects, all cited to source). The withheld exit-frame FINISH commits are preserved locally at tag — commented by Claude |
x = 1
del x
del x # must raise UnboundLocalErrorIn a hot loop (200000 iterations), Codex's mechanism reads correct: commit The PR body's claim that both lowerings are "byte-identical to the interpreter path by construction" is therefore wrong for — commented by Claude |
youknowone
changed the title
⛔ Do not merge — this PR introduces a JIT miscompile (retracting my previous green claim)My earlier comment said this stack was verified green. That was wrong, and I am retracting it. The failing The A/B, both sides rebuilt with the canonical command, binaries hash differently
Deterministic local repro — no CI, no timing, 3/3 reproductions: JIT off passes, JIT on fails ( Why I initially misread it as a pre-existing flakeTwo plausible signals lined up and both were misleading:
Bisecting the two commits now. I will report which one is responsible and either fix it or drop it, as with the — commented by Claude |
✅ Retracting the do-not-merge hold — my accusation was wrong, these commits are innocentMy previous comment claimed these two commits introduce a What was wrong with my experimentI compared this branch against So I was measuring "my base without #545" vs "newer main with #545" and attributing the whole delta to my two commits. The tell was there and I explained it away: the two "main" builds produced different binary hashes ( The control I never ran, run now — my base ALONE
Removing my commits does not change the outcome. The defect is pre-existing on the base and already fixed on main, almost certainly by #545 — Independent confirmation from a parallel investigation: for the only loop whose trace differs, the per-CodeObject jitcode dump is byte-identical between the two binaries (same md5, 596/596 lines), and the NextRebasing this branch onto current Apologies for the noise — the earlier hold was posted on a confounded measurement. — commented by Claude |
9aa5209 to
18eae96
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18eae963f0
ℹ️ 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 last_exception = link.last_exception; | ||
| link.last_exception = link.last_exc_value; | ||
| link.last_exc_value = last_exception; |
There was a problem hiding this comment.
When the dynamic-unbound DELETE_FAST arm is caught, carry_explicit_raise_value_on_catch_stack only replaces the handler stack-slot argument with the constructed exception; the original (last_exception, last_exc_value) pair remains later in link.args and target.inputargs (the target input list is not deduplicated). Swapping these metadata fields makes generate_last_exc treat the Ref exception-value variable as the Int exception-type destination and vice versa, so caught unbound del x paths can assemble with kind-mismatched last_exception/last_exc_value moves or seed the handler with the wrong values.
Useful? React with 👍 / 👎.
|
CI failure diagnosis for the latest run (29510274730):
Note this race predates the branch —
— commented by Claude |
DELETE_DEREF still emitted `abort_permanent`, so a hot loop reaching it fell back to the interpreter. This blocks `except E as e:` handlers whose exception variable is captured by a nested function: the capture makes `e` a cell, so the implicit handler cleanup emits DELETE_DEREF (not DELETE_FAST). Lower it traceably by composing existing graph ops, mirroring the LoadDeref and StoreDeref arms: `load_deref_value` dereferences the cell and raises the unbound error when the contents are empty (pyopcode.py:597), then `store_deref_value(cell, NULL)` clears the cell contents while the slot keeps holding the same cell. No new residual; byte-identical to the interpreter. DELETE_GLOBAL / DELETE_NAME / SETUP_ANNOTATIONS still abort. Add exception_as_cell_cleanup guard bench (no bench previously exercised DELETE_DEREF). check.py dynasm 187/187, cranelift 187/187; pyre-jit lib 306/0. Assisted-by: Claude
A `Copy` handler landing reached by an explicit raise seeded its top-of-stack exception with a `last_exc_value` read. A residual call between the raise landing and that read clears the metainterp exception transient (`execute_varargs` clears before a call that may itself raise), so the read found no value and the walk aborted with LastExcValueWithoutActiveException, leaving the loop interpreter-only. Reload the propagated exception from the frame's durable top stack slot via `getarrayitem_vable_r` for those landings — catch dispatch has already stored it there. Reraise-entered landings keep the `last_exc_value` read, which still carries the live payload supplied by the exception edge. The per-thread current-exception slot is not used as the source: at this landing it can still name the older exception whose handler raised this one, which would mislink `__context__`. Also apply the `__context__` write to the concrete exception built in `try_walker_trace_raise_builtin`, mirroring the recorded SETFIELD, since the full-body walk is also the authoritative execution of the tracing iteration. exception_bare_reraise_restore and exception_context_chain_inhandler now compile (loops_compiled 0 -> 1) with output byte-identical to the interpreter oracle. check.py dynasm 192/192, cranelift 192/192; pyre-jit 311, pyre-jit-trace 263, majit-metainterp 1399 lib tests pass. Assisted-by: Claude
Match pyopcode.py:998 by checking the raw local before clearing it. Construct and explicitly raise UnboundLocalError only on the null arm, while the bound arm performs one PY_NULL write. The three-N delete repro reports loops_compiled=1 at 1000, 20000, and 200000 iterations. Both full backend gates pass 192/192. Assisted-by: Claude
Decode the guard-owned sys_exc_value pending setfield during bridge setup and carry its symbolic fieldbox plus concrete shadow through current-exception reads, including nullary bare-reraise and multi-frame carrier walks. exception_bare_reraise_nested_outer improved from loops_compiled=1 bridges_compiled=1 loops_aborted=166 guard_failures=33401 to loops_compiled=1 bridges_compiled=2 loops_aborted=0 guard_failures=401. Output matches the no-JIT oracle; measured wall time was 0.06s versus pypy3 0.08s. Assisted-by: Claude
Store union-find weights and root info in hash maps so union removes absorbed roots in expected O(1) time, matching upstream dictionary deletion semantics. Preserve upstream infos() ordering by filtering the insertion-only link_to_parent keys to surviving roots. On seqiter_tuple_error_parity.py, release dynasm wall time fell from 0.62s to 0.26s. JIT stats stayed at loops_compiled=2, bridges_compiled=1, loops_aborted=0, and guard_failures=202. Assisted-by: Claude
Mirror typed exception-slot loads and stores in the full-body walker with class, Python class, version-tag, and non-null guards. Reuse the cached per-kind field descriptors, preserve args list/tuple conversion, and apply authoritative live writes with GC barriers. exception_subclass_attrs: CallMayForce 16 -> 10 per traced iteration (80 -> 50 scaled); wall 5.90s -> 0.95s. exception_oserror_fields: wall 1.39s -> 0.41s; loops_compiled=1 and loops_aborted=0. Assisted-by: Claude
Accept cacheable heap exception subclasses when their __new__ and __init__ descriptors match the canonical class for the constructed kind. Guard the exact class and its version tag, verify the physical exception pytype, and reuse typed integer-list virtualization for SystemExit(i) arguments. Measured exception_subclass_attrs residuals over the five-iteration trace body: 50 -> 40. Wall time: ~0.92s -> 0.78s. Assisted-by: Claude
Mirror OSError slot filling and errno-based class retagging in the walker-native exception constructor fold. Guard the value-dependent branches and preserve materialized args-list class identity. Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
descriptor.py:139-143 catches only AttributeError when reading obj.__class__; any other exception from a property or __getattribute__ override re-raises. The if-let Ok arm swallowed every error into the final TypeError. Assisted-by: Claude
W_AbstractRangeIterator (functional.py:823) exposes no __getitem__; iter(range(n))[i] must raise TypeError. The getitem_range_iter arm made range iterators indexable and sliceable. Assisted-by: Claude
Eight threads call init_typeobjects concurrently and the main thread probes object.__class__ afterwards. Guards the PATCH_TYPEOBJECTS Once barrier: without it a second caller returning from the initialized OnceLock re-entered the patch passes while the first was still storing into the shared type dicts (flaky IndexMap index-out-of-bounds panics across cargo test threads). Assisted-by: Claude
Class creation unconditionally deleted __class__/__classdict__ from the namespace (and delitem'd them on custom __prepare__ mappings), destroying attributes the class body stored explicitly, e.g. a __class__ property (gh#605). The deletions guarded against a fast2locals cellvar mirror from the retired class-namespace-scratch architecture; since the body writes through setdictscope and the __class__/__classdict__ cells stay empty during the body, those keys never reach the namespace by scaffolding — only by explicit assignment, which type.__new__ must keep. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a601a48393
ℹ️ 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".
| unsafe { | ||
| match slot { |
There was a problem hiding this comment.
When this direct STORE_ATTR fold runs inside an inlined callee, the raw exception-slot write mutates the live exception object but bypasses the generic residual path that records executed effects and rollback state. If a later supported abort in that same sub-walk takes the gh#467 call-forward path, fbw_executed_effect_count() still appears unchanged, so the abandoned callee is re-executed by the interpreter and a mutation like e.args = e.args + (x,) is applied on top of the already-mutated object. Either journal/rollback the displaced exception slot and bump the executed-effect odometer, or keep this on the residual path so abort handling can see the committed heap write.
Useful? React with 👍 / 👎.
youknowone
changed the title
Removes a JIT trace abort on the exception hot path, and fixes an exception-reload
defect that kept two exception loops interpreter-only.
1.
DELETE_DEREFno longerabort_permanentWhen a nested function closes over an
except E as e:name, the local becomes acell and the handler's implicit
del ecompiles toDELETE_DEREF. That opcodereached
abort_permanent, andloop_body_has_abort_permanentis a static scan —one occurrence declines the whole loop.
It is now lowered by composing existing graph ops — no new residual:
load_deref_value(raise-if-unbound) +store_deref_value(cell, NULL)(clears thecell contents, keeps the cell in the slot) +
store_local_value. The liveinterpreter impl is
delete_deref. Added a guard benchexception_as_cell_cleanup.py, which compiles (loops_compiled=1,bridges_compiled=2) and matches thePYRE_NO_JIT=1oracle.2. Cleanup-landing exception reloaded from the frame's stack slot
A
Copyhandler landing reached by an explicit raise seeded its top-of-stackexception with a
last_exc_valueread. A residual call between the raise landingand that read clears the metainterp exception transient (
execute_varargsclearsbefore a call that may itself raise), so the read found no value and the walk
aborted with
LastExcValueWithoutActiveException.Those landings now reload the propagated exception from the frame's durable top
stack slot via
getarrayitem_vable_r— catch dispatch has already stored it there.Reraise-entered landings keep the
last_exc_valueread, which still carries thelive payload from the exception edge. The per-thread current-exception slot is
deliberately not the source: at this landing it can still name the older
exception whose handler raised this one, which would mislink
__context__.Results
exception_bare_reraise_restoreandexception_context_chain_inhandlernowcompile (
loops_compiled0 → 1), output byte-identical to the oracle.swap_except_return_resume2.79 → 0.06s,finally_bare_raise0.86 → 0.05s,exception_oserror_fields1.12 → 0.51s.Withheld from this PR
DELETE_FAST→ #586. The lowering in the earlier revision of this PRmiscompiled: it dropped the arm's
ptr_iszeroguard and emitted the slot clearunconditionally after the can-raise
load_fast_check, breaking thepyopcode.py:998shape where the raise prevents the clear.del x; del xin ahot loop then swallowed exactly one
UnboundLocalError(oracle 200000, JIT 199999— loss is exactly 1 at every N, so it is a trace/compile transition artifact).
origin/mainis correct here because it declines the loop. Caught by the Codexparity review; full repro and characterisation in #586.
Exit-frame raise FINISH → #581. It made
synth/exception_inlined_callee_caughtcompile a bridge for the first time, whichexposed a pre-existing GC hole and crashed wasm. Root cause is not in that commit:
the wasm backend has no
gcreftracer/LoadFromGcTablepath and bakesOpRef::ConstPtr(GcRef)into emitted code as a raw address immediate, where the GCcan neither find nor forward it — dynasm and cranelift both run
remove_constptrs_in→GcTable::from_gcrefs(RPython'sGCREFTRACER).Gates
check.pydynasm and cranelift; lib testspyre-jit,pyre-jit-trace,majit-metainterp. wasm's one failing fixture,synth/residual_raise_except_resume, fails identically onorigin/main(0.21s vs pypy 0.01s = 21.4x): #576 added
# pyre-check: max-pypy-ratio=10to itand the wasm backend does not meet it, while unannotated fixtures in the same sweep
run at 13–33x and pass ungated. This branch adds no new wasm failure.
🤖 Generated with Claude Code