Repository navigation
jit: propagate a handler-raised exception through the func-entry blackhole resume - #641
Conversation
…khole resume A named `except X as m:` handler that raises a second exception of a different class, in a function that also has a sibling `except` clause, miscompiled under a func-entry trace of the return path: the cold exception path deopts into the blackhole, which dropped the handler-raised exception at a Void leave-frame (BlackholeResult::Failed -> invalidate_loop), so the caller matched the first exception's class. jit output diverged from the interpreter by one iteration. - When a shared catch landing's FrameState widens, remap every incoming exception link's outgoing args so the exception register kinds and positions stay aligned. - Load the cleanup RERAISE's exception from its semantic frame slot when the handler entry carries a null symbolic placeholder, instead of a fresh placeholder. - Split the dynamic DELETE_FAST bound check's exception arm explicitly and keep its successful continuation off the generic per-op catch edge. - Drop the source-shape DELETE_FAST reraise-sibling abort_permanent decline; the cleanup path now compiles correctly, restoring JIT coverage for it. Regression bench: synth/handler_reraise_second_exc. Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR adds a benchmark for nested exception-handler re-raises and updates JIT frame-state merging, handler exception-value loading, cleanup control flow, and exception-edge handling, with regression tests for named-handler cleanup and explicit raise stack metadata. ChangesException-handler correctness
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 4e3174c). 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
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e3174c9d1
ℹ️ 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".
| joinpoints | ||
| .entry(py_pc + 1) | ||
| .or_insert_with(Vec::new) | ||
| .insert(0, bound_block.clone()); |
There was a problem hiding this comment.
Do not hide queued joinpoints after DELETE_FAST
When DELETE_FAST is immediately followed by a PC that is already a branch target, e.g. if c: del x; return x, that target block is already present in joinpoints[py_pc + 1] and queued for later walking. Inserting the DELETE_FAST success block at index 0 makes emit_mark_label_pc! pick this new block when the queued branch-target block is later popped (start_pc == py_pc skips the merge path), so the original branch target is never emitted and its predecessor still points at an empty/orphan block. Register the continuation without displacing existing pending joinpoints, or merge with the existing candidate.
Useful? React with 👍 / 👎.
Fixes a deterministic silent dynasm/cranelift JIT miscompile found by a fuzz
sweep (hunt-3), then confirmed as a new manifestation of the blackhole-failure
family (#15).
jit_out != nojit_outon the same binary; NOJIT and CPython agree.Symptom
A function with a hot
returnpath and a cold exception path where anexcept X as m:handler raises a second exception of a different class,in a function that also has a sibling
exceptclause, miscompiles: exactlyone iteration routes the propagated exception to the wrong
exceptclause inthe caller.
Root cause
fcompiles as a func-entry trace of its return path; the coldi % 9 == 0iteration guard-fails into the blackhole, which replays the exception path,
raises the ValueError, catches it, raises the handler's KeyError, then reaches a
Void leave-frame that drops the exception (
BlackholeResult::Failed→invalidate_loop), so the caller matches the first exception's class. Thedefect is in trace construction (the walker), not the backend exception cells:
FrameStatewidens, the earlier incomingexception links' outgoing args were not remapped, corrupting the exception
register kinds/positions. Remap every incoming exception link on widen.
RERAISEcould read a null symbolic placeholder instead of thelive exception; load it from the semantic frame slot when handler entry
carries a null placeholder.
carry_explicit_raise_value_on_catch_stackno longer swaps thelast_exception/last_exc_valuelink kinds.DELETE_FASTbound check's exception arm is split explicitly andits successful continuation is kept off the generic per-op catch edge.
This also subsumes the #633 cluster-B
DELETE_FASTreraise-sibling shape, soits source-shape
abort_permanentdecline is removed — that family nowJIT-compiles instead of falling back to the interpreter.
Verification
synth/handler_reraise_second_exc→(500, 500, 8000), jit == nojit,both backends.
named_reraise_sibling_hot→38172(jit == nojit ==CPython) with the decline gone; the hunt-2 B family sweeps (37 cases) pass via
the real fix; the E family sweeps (412 cases) are unaffected.
python ./pyre/check.py --backend dynasmand--backend cranelift→ALL PASSED(224/224 each).cargo test -p pyre-jit --features dynasmgreen, including two new tests(
named_handler_cleanup_with_sibling_clause_emits_delete_fast_path,explicit_raise_stack_value_preserves_exception_link_kinds).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
try/exceptflows, including re-raised exceptions and sibling handlers.Tests