exc: mark the traceback's frame escaped on app-level reads - #804
Conversation
Upstream issues `tb.frame.mark_as_escaped()` from two getters, `descr_gettraceback` and `OperationError.get_traceback`, while direct `_application_traceback` reads do not. pyre had it from neither. Add `pytraceback::mark_traceback_escaped` -- the frame type is not visible from pyre-object, so it cannot sit beside the slot reader -- and call it from the seven sites that mirror one of the two getters: the `__traceback__` getattr arm, the attribute fold, `record_application_traceback`, the tracer hand-off in `exception_trace`, `sys.exc_info`, and both `write_unraisable` reads. The printing, chain-trimming and metadata-compare readers mirror direct slot reads and are left alone. The fold lowers the read to a raw slot load, so the specializer pairs it with a `CallN` to an `extern "C"` entry carrying `cannot_raise_effect_info`, and applies the mark concretely on the walk. This restores the upstream contract; it does not change any observed value. The `f_lineno` divergence it was written for survives, and a control proves why: `tb.tb_frame`'s getter has always marked the frame, and marking from inside the handler -- at either site -- has no effect, while marking before the `try` does. Also correct the `PyTraceback.frame` note that forbids dereferencing the pointer: `pytraceback_object_custom_trace` forwards that edge and frames are GC-owned, so `tb_frame` is live for the traceback's lifetime. check.py dynasm 322/322 + cranelift 322/322; lib tests 400/326/294. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 22 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 (7)
✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecde196c84
ℹ️ 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".
| // `tb = operror.get_traceback()` — the read that grows the chain | ||
| // marks the previous head's frame, matching `get_traceback`. | ||
| let prev_tb = pyre_object::interp_exceptions::w_exception_get_traceback(w_exc_object); | ||
| mark_traceback_escaped(prev_tb); |
There was a problem hiding this comment.
Preserve the escape mark in the IR traceback prepend
When the full-body walker takes record_prepend_application_traceback for a nonconstant exception with a materialized frame, pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs:805-809 bypasses this function and lowers get_traceback() to only a GETFIELD_GC_R; it emits neither the concrete escape mark nor the residual marking call added for the attribute fold. Consequently, optimized JIT exception paths can still grow a traceback chain without marking the previous head's frame escaped, retaining the interpreter/JIT divergence this change is intended to remove. Pair that inline slot read with the same mark used by the opaque path.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit ecde196). 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)None. 4. Structural adaptations
|
Upstream issues
tb.frame.mark_as_escaped()from two getters —interp_exceptions.py descr_gettracebackanderror.py OperationError.get_traceback— while direct_application_tracebackreads (printing, chain trimming,got_any_traceback) do not. pyre had it from neither. This is what the Codex parity review on #768 flagged in both §2 and §3.What this does
pytraceback::mark_traceback_escaped— the frame type is not visible frompyre-object, so it cannot sit beside the slot reader — called from the seven sites that mirror one of the two getters:__traceback__getattr arm (descr_gettraceback)__traceback__attribute foldrecord_application_traceback(tb = operror.get_traceback())eval.rsexception_trace (executioncontext.pytracer hand-off)sys.exc_info(vm.py exc_info_with_tb→get_w_traceback)error.rswrite_unraisable, twice (the value, and the exception the hook itself raised)Left alone, because they mirror direct slot reads:
write_traceback_chain_from_exc, the importlib bootstrap-frame trimming inimporting.rs, and the exception-group metadata compare inbuiltins.rs.The fold lowers the read to a raw
GetfieldGc, so the specializer pairs it with aCallNto anextern "C"entry carryingcannot_raise_effect_info(the callee cannot raise, allocates nothing, and writes only the frame's status byte, which no field descriptor exposes to the trace), and applies the mark concretely on the walk.Also corrects the
PyTraceback.framenote that forbade dereferencing the pointer:pytraceback_object_custom_traceforwards that edge and frames are GC-owned since the FrameArena deletion, sotb_frameis live for the traceback's lifetime.Scope — this restores a contract, it does not fix a bug
It changes no observed value, and measures free against a baseline binary (
exception_metadata_hot0.66s both,exception_traceback_loop_forms0.62s both).It was written for a real divergence — a same-frame raise+catch in a compiled
whileloop, traceback carried out,f.f_linenoreading thedefline on pyre JIT against thereturnline on pypy3, CPython 3.14 andPYRE_NO_JIT=1— but that divergence has a different root cause, and four hypotheses were built and refuted on the way:tb.tb_frame's getter has always marked the frame, and firing it from inside the handler is equally ineffective.force_frame(frame)inleave'sescaped() || got_exceptionbranch — no change.ExecutionContext::leavenever runs for JIT-entered frames at all: the warm entry (try_function_entry_jit/handle_jitexception) sits outsideexecute_frame's enter/leave bracket, where upstream reaches the portal fromdispatch, i.e. inside it. Instrumented: 0 leave events for the test function, 2 for<module>.force_frameat the traceback capture point inrecord_application_traceback— no change; that runs during unwinding.fbw, EC enter/leave at the inlined-call push) — tested against that branch's build, does not fix it either.The boundary turns out to be the raise, not the handler:
So
last_instris maintained correctly once the frame is forced before the raise; the compiled trace's exception path never writes the vablelast_instrback, leaving the-1thathelpers.rsbuilds inline callee frames with (offset2linenoreturnsfirst_line_numberforstopat < 0). That is a JIT exception-unwind defect, one family with two crashes found alongside it and both confirmed pre-existing by reproducing them on sibling worktree binaries built from unrelated HEADs:sys._getframe()exposure inside a compiledexcepthandler SIGSEGVs (either read route —e.__traceback__orsys.exc_info()[2]); one exposure is not enough, an exception is requiredsys._getframe()placed after a traceback read in the same handler raisesTypeError: call failedThat work belongs with the in-flight frame-lifecycle changes in #794 and #796 rather than against them, so it is not in this PR. The discriminator bench is deliberately not added to
pyre/bench/synth/— it would turn check.py red while the defect stands.Verification
check.py dynasm 322/322 + cranelift 322/322 (sequential, on the rebased base); lib tests 400/326/294, 0 failed;
cargo fmt --checkclean; exception corpora diff clean against pypy3 on both backends.