Repository navigation
jit: instruction-keyed, per-frame, and loop-close exception traceback recording - #756
Conversation
|
Warning Review limit reached
Next review available in: 57 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 Plus Run ID: 📒 Files selected for processing (13)
✨ 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: f55f8228b6
ℹ️ 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".
| ctx.trace_ctx.call_void_typed_with_effect( | ||
| hook, | ||
| &[exc, frame, jitcode, opcode], | ||
| &[Type::Ref, Type::Ref, Type::Int, Type::Int], | ||
| default_effect_info(), | ||
| ); |
There was a problem hiding this comment.
When a caught raise e records a runtime traceback callback, record_application_traceback mutates e.w_traceback by prepending a PyTraceback, but this call is emitted with default_effect_info(), whose write set is empty, so OptHeap can preserve cached fields across the call. If the trace has already cached e.__traceback__ (for example as None) and the handler reads it after the raise, the optimized trace can reuse the stale value instead of seeing the traceback just attached. Please give these hooks an EffectInfo that writes W_BaseException.w_traceback (or a conservative/random effect), and apply the same treatment to the inline hook below.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 630cfe3). 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)
4. Structural adaptations
|
Traceback recording during trace recording, blackhole, and the FBW walker previously suppressed record_application_traceback whenever the raised object was pointer-equal to last_caught_exception_value. That conflated an explicit re-raise of the caught object with a bare re-raise: compiled traces for `except E as e: raise e` attached no traceback at all, while a recording pass over a bare `raise` appended extra nodes at the re-raise and handler coordinates. Decide by instruction instead: RAISE_VARARGS with an operand always records; the bare-reraise path (RAISE_VARARGS 0 / RERAISE, RaiseWithExplicitTraceback upstream) never records; handler entry in the same frame records nothing; frame-boundary propagation records once per frame. jitcode_pc_is_bare_reraise maps a JitCode coordinate back to the Python instruction for the walker and blackhole paths, and a fresh locally-caught exception gets an optimizer-visible virtual PyTraceback so unobserved allocations still fold away. last_caught_exception_value is removed from MIFrame, BlackholeInterpreter, InlineFrame, and WalkSession; no traceback decision keys on object identity. Fixes, oracle pypy3, both backends: raise-e steady state depth 0 -> 2; bare-reraise recording extras (maxd 3 -> 1); mixed_ctx maxtbdepth 3 -> 1; mixed_call_reraise duplicate frame on re-recording (count 11 -> 0 at N=20000). One pre-existing seam remains in raise_e_tb: the except body's Counter update replays once near the compile boundary (4001 total vs pypy 4000, present before this change). Assisted-by: Claude
blackhole_resume_via_rd_numb attached the application traceback only at the bottommost _exit_frame_with_exception arm, so when an exception propagated through several reconstructed BlackholeInterpreter frames the intermediate frames were never recorded. A one-frame resume whose residual call raised (a compiled trace's first guard-failure replay calling into a callee) therefore produced a traceback missing the callee's frame. Record each exiting frame before advancing to nextblackholeinterp, matching _run_forever propagating one frame at a time and record_application_traceback attaching at every Python frame boundary; the bottommost frame is covered by the same call and still recorded once. Bare reraises stay instruction-keyed and add no node. Also apply jitcode_pc_is_bare_reraise in record_inline_traceback_for_recording so an inlined bare reraise does not add a duplicate node, matching the concrete blackhole callback. nested_cond and cond_d4 traceback shapes now match pypy on both backends; mixed_call_reraise stays count 0. Assisted-by: Claude
A handler loop carries the saved-previous-exception null sentinel as a real live operand across its loop-close merge point. flush_walk_end_ state_to_frame rejected any null Ref in the operand-stack region as an unpopulated mid-expression shadow, so run_perfn_walk declined the merge-point handoff and fell back to the legacy replay; the replay re-executed the trailing residual (e.g. a StoreSubscr) once, producing one extra side effect at the trace-compile boundary while traceback depths stayed correct. Add flush_walk_loop_end_state_to_frame for the raise_continue_running_ normally handoff: a null operand-stack slot is accepted only when its recorded box independently resolves to the same concrete null. The abort-point, item, stack-override, and full-stack callers keep the strict rejection, since their arbitrary resume coordinate can hold an unavailable mid-expression shadow slot. raise_e_tb and tb_walk_tail lose the extra tail observation (4001 -> 4000) on both backends; traceback battery, emh, and check.py stay green. Assisted-by: Claude
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 630cfe34ad
ℹ️ 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".
| exc, | ||
| concrete_exc, | ||
| op.pc, | ||
| false, |
There was a problem hiding this comment.
Separately from the previously posted effect-info issue, this false means the traceback hook is not executed at all during the authoritative recording walk for a same-frame caught raise e; only a runtime call for future compiled iterations is emitted. If the handler then observes e.__traceback__ via residual/interpreter getattr, or stores it somewhere, the concrete exception still has the pre-raise traceback (often None), so the recording iteration can record or leak a value the interpreter would not produce. Please also run the concrete traceback recording before entering the handler.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
#765) build_multi_frame_miframe copies last_caught_exception_value between the reconstructed InlineFrame/MIFrame chain, but #756 removed that field from MIFrame, BlackholeInterpreter, InlineFrame, and WalkSession — traceback recording is keyed on the raising instruction, not object identity. #763 was based before #756 and merged after it, so the references no longer resolve and pyre-jit-trace fails to compile (E0609 at residual_call.rs:240/241/273). Remove the vestigial per-frame propagation. The multi-frame build path is PYRE_FBW_MULTIFRAME gated (default off); no behavior change. check.py: dynasm 303/303, cranelift 303/303, wasm 300/300. Assisted-by: Claude
Follow-up to #743. Three commits closing the remaining exception-traceback correctness gaps surfaced by that PR's review and by fixture verification. Rebased onto current main (#745).
1.
jit: key traceback recording on the raising instructionrecord_application_traceback(recording path, blackhole, and the FBW walker) previously suppressed a traceback node whenever the raised object was pointer-equal tolast_caught_exception_value. That conflated an explicit re-raise of the caught object (except E as e: raise e) with a bare re-raise:raise eattached no traceback at all (steady-state depth 0), andraiseappended extra nodes at the re-raise and handler coordinates.Decide by instruction instead:
RAISE_VARARGSwith an operand always records; the bare-reraise path (RAISE_VARARGS 0/RERAISE,RaiseWithExplicitTracebackupstream) never records; handler entry in the same frame records nothing; frame-boundary propagation records once per frame.jitcode_pc_is_bare_reraisemaps a JitCode coordinate back to the Python instruction for the walker/blackhole paths, and a fresh locally-caught exception gets an optimizer-visible virtualPyTracebackso unobserved allocations still fold away.last_caught_exception_valueis removed fromMIFrame,BlackholeInterpreter,InlineFrame, andWalkSession; no traceback decision keys on object identity.Addresses the codex-review §2/§3 findings on #743 (identity-based suppression of
raise e).2.
jit: record every unhandled blackhole frame's tracebackblackhole_resume_via_rd_numbattached the application traceback only at the bottommost_exit_frame_with_exceptionarm. When an exception propagated through several reconstructedBlackholeInterpreterframes the intermediate frames were never recorded — a one-frame resume whose residual call raised (a compiled trace's first guard-failure replay calling a callee) produced a traceback missing the callee's frame.Record each exiting frame before advancing to
nextblackholeinterp, matching_run_foreverpropagating one frame at a time andrecord_application_tracebackattaching at every Python frame boundary; the bottommost frame is covered by the same call and still recorded once. Also appliesjitcode_pc_is_bare_reraiseinrecord_inline_traceback_for_recordingso an inlined bare reraise adds no duplicate node.3.
jit(fbw): accept a proven-null saved-exc sentinel at loop-close flushA handler loop carries the saved-previous-exception null sentinel as a real live operand across its loop-close merge point.
flush_walk_end_state_to_framerejected any nullRefin the operand-stack region as an unpopulated mid-expression shadow, sorun_perfn_walkdeclined the merge-point handoff and fell back to the legacy replay; the replay re-executed the trailing residual (e.g. aStoreSubscr) once, producing one extra side effect at the trace-compile boundary while traceback depths stayed correct.Add
flush_walk_loop_end_state_to_framefor theraise_continue_running_normallyhandoff: a null operand-stack slot is accepted only when its recorded box independently resolves to the same concrete null. The abort-point, item, stack-override, and full-stack callers keep the strict rejection, since their arbitrary resume coordinate can hold an unavailable mid-expression shadow slot.Verification (both backends, oracle pypy3)
Traceback battery — nested reraise
__context__(maxctx 0), new-object raise (1), blackhole inner/outer (0), multi-site (0), mixed bare/ctx (maxtbdepth 1), nested3/4/reraise (0), conditional-raise bridgenested_cond/cond_d4(0), mixed call-reraise (0),raise_e_tb({2: 4000}), an adversarial null-sentinel stress (accumulator + caught-identity checks exact) — all matching pypy.exception_metadata_hotbyte-identical to pypy3.cargo test(pyre-jit 325, pyre-jit-trace 294, gc 12, majit-metainterp dynasm 1403 / cranelift 1401): pass.pyre/check.pydynasm 300/300 and cranelift 300/300.🤖 Generated with Claude Code