Skip to content

jit: deliver the exception a jd1 trace finishes with; clear the drain's loop-exit StopIteration - #799

Merged
youknowone merged 8 commits into
mainfrom
gc-decouple
Jul 26, 2026
Merged

youknowone merged 8 commits into
mainfrom
gc-decouple

Conversation

@youknowone

@youknowone youknowone commented Jul 25, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #751: applies its review findings. Two of them were live
default-settings wrong answers.

Fixed: an exception raised while the jd1 drain is being traced was dropped

TraceAction::Finish carried only the symbolic finish_args, so
eval.rs drive_unpack_iterable_trace's Finish arm compiled the FINISH and
returned — nothing raised. count(*it) where __next__ raises on the
trace-recording crossing returned a truncated list:

cpython: lost 0    dynasm: lost 1    PYRE_NO_JD1=1: lost 0

pyjitpl.py:2530-2562 finishframe_exception snapshots
excvalue = self.last_exc_value before calling
compile_exit_frame_with_exception(self.last_exc_box) and then raises
jitexc.ExitFrameWithExceptionRef(excvalue), which warmspot.py:998-1005
re-raises out of ll_portal_runner. The compile half consumes only the
symbolic args; the raise half needs the value. So the value now travels on
the action as TraceAction::Finish { exc_value }, and jd1 parks it before
the compile — the compile allocates, and the pending-exception slot is the
only root for the raw ref.

Fixed: the drain's own loop-exit StopIteration leaked to the interpreter

unwind_to_exception_handler's drained-framestack terminal never called
clear_exception(), so BH_LAST_EXC_VALUE stayed set after the walk and the
interpreter re-surfaced it at its next residual-call boundary. Every other
frame exit (BC_*_RETURN) clears it; that asymmetry was the bug. Repro shape
(needs a cold JIT and the store off the hot path):

e = ValueError("x"); it = [1, 2, 3]
for i in range(40000):
    if i % 997 == 996:
        e.args = it          # bogus StopIteration traceback here

Both bugs are now covered by synth/unpack_drain_star_raise, verified to fail
on a build with the fixes reverted.

Fixed: merge_point's finish arm dropped that same exception

Follow-up to the two above, found in review of this PR. Closing the
BH_LAST_EXC_VALUE channel at the producer left the other consumer without
one. JitDriver::merge_point is reached only from the
#[jit_interp]-generated tracer (majit-macros/src/jit_interp/mod.rs:1346),
and a macro-front-end client has no second carrier for an escaping exception,
so its Finish arm silently dropped the value and the portal finished as if the
frame had returned normally.

Latent today — every non-zero writer of BH_LAST_EXC_VALUE outside tests is in
pyre/, and pyre does not use #[jit_interp] — but it is a wrong-answer shape
this PR introduced, so it is closed here rather than left standing.
BH_LAST_EXC_VALUE is now set from exc_value before
compile_finish_from_active_session, for the same rooting reason as the jd1
side. The comment that previously justified discarding the value claimed this
arm was reached by the full-body walk (which raises via
WALK_END_PROPAGATED_EXCEPTION); that thread-local lives in pyre-jit-trace
and pyre does not route through merge_point, so the comment was wrong.

Covered by finish_with_exception_republishes_the_escaping_exception, verified
to fail (left: 0) with the republish disabled.

Rest of the review

  • root scope — push_roots() around the jd1 live-enter block.
  • per-element pins — drain_collect_items pins each element it reads back
    instead of relying on the container pin, and its doc comment is corrected.
  • cranelift one-shot — next_frame_value_count_fn is .take()n once per
    do_compile, next to its two siblings, instead of leaking into the next
    compile.
  • SETARRAYITEM_GC mark_escaped — heapcache_invalidate_caches_varargs
    before record_op_with_descr, matching pyjitpl.py:2742 →
    _record_helper (:2694) → heapcache.py:217-241.
  • per-mutator GC root area — the JIT pending-exception cell is walked from
    PyFrameRootArea like BH_LAST_EXC_VALUE, so it is a root for every mutator
    rather than only the collecting thread (rthread.py:429-437 _trace_tlref).
  • MAJIT_BRIDGE_ONLY — temporary diagnostic removed.

Refuted by measurement

One finding claimed install_build_time_jitcode_at became vestigial once
every jd1 coordinate consumer moved to the build-time store, so the slot-0
install could be deleted. Measured false: without it
synth/slots_class_var_conflict SIGSEGVs in
pyre_jit::call_jit::blackhole_resume_via_rd_numb on dynasm and cranelift,
and passes under PYRE_NO_JD1=1 / PYRE_JD1_NO_ENTER=1. The jd1 live-enter
blackhole resume still resolves sd.jitcodes[0], so without the install it
decodes whichever user jitcode owns slot 0. Kept, with the measurement recorded
in its doc comment. The real fix stays the build-time↔runtime index-space
unification (codewriter.py:74-88, warmspot.py:281-282).

Not in this PR

A #[dont_look_inside] callee returning Vec<T> is erased to a one-word
"ref" by dont_look_inside_return_token
(majit/majit-translate/src/front/mir.rs), so drain_collect_items,
compute_mro and memoryview_gather_bytes would residualize with a broken
sret ABI. Measured latent — no runtime path dispatches those residuals today.
The orthodox fix is to make unpackiterable/fixedview return the W_List
ref, which touches ~50 call sites; filed separately rather than ballooning
this diff.

Gate

python3 pyre/check.py --backend dynasm,cranelift,wasm →
dynasm 2 failed / 319 passed, cranelift 2 failed / 319 passed,
wasm 1 failed / 317 passed.

Every remaining failure was reproduced with a binary built from unmodified
main, i.e. they are red on main already and are not caused by this branch:

  • synth/getframe_force_cancel_journal wrong output (19995 vs 20000), all
    three backends.
  • synth/const_arg_call_resume cranelift 33.8–34.4x over the 30x gate.
  • cargo test: majit-translate/test_rbigint_mir and
    pyre-jit-trace --lib::{sub_jitcode_body_by_index_builds_w_list_append, list_append_jitcode_resolves_charon_body} — the stale feature-flavoured LLBC
    extract artifact.

synth/unpack_drain_star_raise passes on all three backends.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed exception propagation during compiled iterator draining, including preservation of non-StopIteration errors.
    • Improved reliability when exceptions escape compiled execution paths.
    • Prevented stale or missing exception values during trace completion.
    • Improved garbage-collection safety for list elements, array updates, and pending exceptions.
    • Corrected bridge recording behavior after guard failures.
  • Tests
    • Added regression coverage for exception preservation and expanded iterator-draining scenarios.
  • Documentation
    • Added diagnostic context for a rare compiled-code resume failure.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026 •

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The PR extends JIT finish actions with exception values, roots pending exceptions during GC, preserves non-StopIteration errors through jd1 drain execution, stabilizes drained list elements, and adjusts compilation and bridge-recording paths.

Changes

JIT exception flow

Layer / File(s) Summary
Finish payload and trace propagation
majit/majit-metainterp/src/lib.rs, majit/majit-metainterp/src/pyjitpl/dispatch.rs, pyre/pyre-jit-trace/...
TraceAction::Finish now carries exc_value; exception unwinding and trace termination populate it, while consumers and tests handle the expanded payload.
GC rooting and drained item stability
pyre/pyre-interpreter/src/eval.rs, pyre/pyre-interpreter/src/stack_check.rs, pyre/pyre-interpreter/src/baseobjspace.rs
Pending JIT exceptions use shared raw-cell root walking, and drained list elements are pinned before collection.
jd1 drain exception preservation
pyre/pyre-jit/src/eval.rs, pyre/bench/synth/unpack_drain_star_raise.py
Drain exception exits preserve non-StopIteration errors, scope pinned roots, and add early-raise and argument-drain benchmark cases.
Compilation and bridge wiring
majit/majit-backend-cranelift/src/compiler.rs, majit/majit-metainterp/src/jitdriver.rs, pyre/pyre-jit-trace/src/state.rs
One-shot compilation captures the frame-value callback, bridge recording drops the diagnostic allowlist, and build-time jitcode installation behavior is documented.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Interpreter
  participant TraceActionFinish
  participant JITTrace
  participant Jd1Eval
  participant CallerLoop
  Interpreter->>TraceActionFinish: emit finish arguments and exc_value
  TraceActionFinish->>JITTrace: compile finish or propagated raise
  JITTrace->>Jd1Eval: return exception exit payload
  Jd1Eval->>CallerLoop: preserve and raise non-StopIteration error
Loading

Possibly related PRs

Suggested reviewers: lifthrasiir

Poem

I’m a rabbit with traces tucked tight,
Carrying exceptions through the night.
Roots stay pinned, drains raise true,
Bridges record what they should do.
Hop, hop—the finish fields grew bright!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main PR change: preserving and delivering jd1 trace exceptions while clearing the drain's loop-exit StopIteration.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch gc-decouple

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 25, 2026 •

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit cbea7fe).
Updated: 2026-07-26T03:56:39.872Z

Files in the reviewed diff
majit/majit-backend-cranelift/src/compiler.rs
majit/majit-metainterp/src/jitdriver.rs
majit/majit-metainterp/src/lib.rs
majit/majit-metainterp/src/pyjitpl/dispatch.rs
pyre/bench/synth/unpack_drain_star_raise.py
pyre/pyre-interpreter/src/baseobjspace.rs
pyre/pyre-interpreter/src/eval.rs
pyre/pyre-interpreter/src/stack_check.rs
pyre/pyre-jit-trace/src/state.rs
pyre/pyre-jit-trace/src/trace.rs
pyre/pyre-jit/src/eval.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

None.

3. Pre-existing mismatches (already present before this patch)

  • pyre/pyre-jit-trace/src/state.rs:648-654 ↔ rpython/jit/metainterp/warmspot.py:281-282: the existing install_build_time_jitcode_at overwrites a runtime jitcodes[index] slot because build-time and runtime index spaces both start at zero. Upstream installs one generated jitcodes table; it does not mix colliding runtime slots. This collision predates the patch; the patch only documents its observed failure mode.

4. Structural adaptations

  • majit/majit-metainterp/src/lib.rs:376-387 ↔ rpython/jit/metainterp/pyjitpl.py:2530-2562: Rust carries the escaping exception as TraceAction::Finish.exc_value; PyPy keeps a GCREF local and raises ExitFrameWithExceptionRef. This is a necessary Rust return-value/control-flow adaptation, and the new producer/consumer path preserves the upstream snapshot-before-compile behavior.
  • pyre/pyre-interpreter/src/eval.rs:46-55 ↔ rpython/rlib/rthread.py:429-437: the new per-mutator root-area pointer for TL_JIT_PENDING_EXCEPTION adapts PyPy’s custom tracing of every thread-local block to Rust TLS plus free-threaded GC.
  • pyre/pyre-interpreter/src/baseobjspace.rs:10621-10633 ↔ pypy/interpreter/baseobjspace.py:1013-1024: PyPy returns its interpreter-level items list directly; Rust must convert it to Vec<PyObjectRef>, requiring explicit temporary roots across allocation-capable element boxing.
  • pyre/pyre-jit/src/eval.rs:5105-5218 ↔ pypy/interpreter/baseobjspace.py:1013-1024: jd1 is entered from a Rust merge-point hook returning (), unlike PyPy’s portal runner that propagates ExitFrameWithExceptionRef; parking and re-delivering non-StopIteration errors is the required control-flow adaptation.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 519c1a98b5

ℹ️ 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".

Comment thread majit/majit-metainterp/src/jitdriver.rs Outdated
Comment on lines +2799 to +2802
// The full-body walk delivers the `raise` half of
// `pyjitpl.py:2558-2562` itself (`WALK_END_PROPAGATED_EXCEPTION`),
// so this consumer only runs the compile half.
exc_value: _,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Propagate exceptions from direct dispatch finishes

Do not discard exc_value here: #[jit_interp]-generated tracers call trace_jitcode_with_args_and_runtime directly, so when an uncaught exception drains that dispatch's frame stack, dispatch.rs now clears BH_LAST_EXC_VALUE and returns the only remaining concrete exception in this field. Unlike pyre's full-body walker, this direct path does not populate WALK_END_PROPAGATED_EXCEPTION; compiling the FINISH and dropping exc_value therefore makes the portal finish without delivering the exception. Restrict the special handling to full-body actions or explicitly propagate the value for direct dispatches.

AGENTS.md reference: AGENTS.md:L14-L18

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ef462485fe

ℹ️ 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".

Comment on lines +2845 to +2846
if exit_with_exception && exc_value != 0 {
crate::blackhole::BH_LAST_EXC_VALUE.with(|c| c.set(exc_value));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Exit the generated loop after an exception finish

For a #[jit_interp] single-pass trace that ends in an uncaught exception, unwind_to_exception_handler drains the root frame without calling capture_single_pass_finish, so ctx.walk_final_pc remains None; consequently the Finish arm does not set single_pass_outcome, and the generated macro only checks take_single_pass_finish() inside the if let Some(...) outcome branch (majit-macros/src/jit_interp/mod.rs:2170). Republishing the exception here therefore does not exit or write back the native dispatch loop: it continues from pre-trace state and may repeat side effects or replace the original exception before a residual boundary consumes the TLS value. Fresh evidence in this revision is that the new regression test also leaves walk_final_pc unset but asserts only the TLS cell, not that the generated loop exits; ensure exception finishes publish a terminal outcome (or make the macro consume the finish flag independently).

AGENTS.md reference: AGENTS.md:L14-L18

Useful? React with 👍 / 👎.

`bridge_only_allows` was added as a bisection aid and marked "remove
before commit". Drop the function and the two `should_bridge`
conjunctions that consulted it.

Assisted-by: Claude
`do_compile` read the field by copy while `next_trace_id` and
`next_header_pc` next to it are `.take()`n. `compile_tmp_callback` sets
none of the three, so it inherited the previous compile's
driver-specific `-live-` decoder instead of falling back to
`get_frame_value_count_fn`.

Assisted-by: Claude
Removing the install makes the jd1 live-enter blackhole resume decode
`sd.jitcodes[0]` as whichever user jitcode owns that slot:
`synth/slots_class_var_conflict` SIGSEGVs in
`blackhole_resume_via_rd_numb` on dynasm and cranelift, and passes again
under `PYRE_NO_JD1=1` / `PYRE_JD1_NO_ENTER=1`.

Assisted-by: Claude
`w_int_new` / `w_float_new` on the Integer/Float strategies park the
mutator at a `gc_op`, so a foreign stop-the-world collection can mark and
sweep while the readback is in progress, and the boxes gathered so far
are reachable from no root. Pin each one and read it back through
`shadow_stack_get`, as `alloc_list_items_block_gc` does.

The doc comment claimed the pin on `items` guards against relocation.
The `W_ListObject` header is old-gen and non-moving; the pin is a
liveness bracket.

Assisted-by: Claude
`TL_JIT_PENDING_EXCEPTION` was reachable only through
`register_extra_root_walker`, which resolves TLS on whichever thread
drives the collection, so a stopped mutator's parked exception was
invisible. Add the cell to `PyFrameRootArea` next to the other
thread-local exception carriers, which `walk_all_extra_areas` visits for
every registered mutator. `rthread.py:429-437 _trace_tlref` traces a
GC-pointer thread local by enumerating every thread's block.

`walk_bh_last_exception_area` becomes `walk_raw_exception_cell_area`,
shared by both raw-`i64` carriers; that also gives the pending-exception
walker the `walk_raw_exception_roots` tail it lacked.

Assisted-by: Claude
`execute_setarrayitem_gc` (pyjitpl.py:2742) records through
`execute_and_record` -> `_record_helper`, which runs
`heapcache.invalidate_caches` -> `mark_escaped` -> `_escape_from_write`
before the op is appended. Without it a ref written into an
already-escaped array keeps `HF_IS_UNESCAPED`, so `invalidate_unescaped`
preserves its cached fields across a residual call that can reach and
mutate it through the array. `clear_caches_not_necessary`
(heapcache.py:314) lists SETARRAYITEM_GC, so only the mark-escaped half
runs.

Assisted-by: Claude
`TraceAction::Finish` carried only the symbolic finish args, so the jd1
consumer compiled the FINISH and returned without raising: `count(*it)`
on an iterator that raised while the drain was being traced returned a
truncated list and the exception was gone.

- `TraceAction::Finish` gains `exc_value`, the snapshot
  `pyjitpl.py:2531 excvalue = self.last_exc_value` takes before
  `compile_exit_frame_with_exception`.
- The drained-framestack exit clears the exception channel after handing
  the value over. Leaving `BH_LAST_EXC_VALUE` set made the interpreter
  re-surface the trace's exception at its next residual-call boundary;
  `exc.args = <list>` in a hot loop raised the drain's own loop-exit
  StopIteration.
- jd1 parks the value before compiling, because the compile allocates
  and the pending-exception slot is the only place the raw value is
  rooted.
- The three jd1 drain exits share one `drain_error_from_exc_ref`
  decoder. The compiled-drain finish exit reads its exception from
  result slot 0 (`compile.py:658-662`) instead of ignoring it, and the
  post-drain slot clears promote a non-StopIteration remainder instead
  of discarding it.
- The jd1 live-enter pins get a `push_roots` scope.

`synth/unpack_drain_star_raise` gains a raise on the trace-recording
crossing and an `exc.args = <list>` loop; each fails without the
corresponding change.

Assisted-by: Claude
`unwind_to_exception_handler` clears `BH_LAST_EXC_VALUE` when it builds the
exception-carrying FINISH and hands the value over on `TraceAction::Finish`.
`JitDriver::merge_point` is reached only from the `#[jit_interp]`-generated
tracer (majit-macros/src/jit_interp/mod.rs:1346), which has no second carrier
for an escaping exception, so its Finish arm dropped the value.

Set `BH_LAST_EXC_VALUE` from `exc_value` before
`compile_finish_from_active_session`. The removed comment claimed this arm was
reached by the full-body walk, which delivers its own raise via
`WALK_END_PROPAGATED_EXCEPTION`; that thread-local lives in pyre-jit-trace and
pyre does not route through `merge_point`.

Assisted-by: Claude

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cbea7feeda

ℹ️ 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".

// process-global callback rather than inherit the previous driver's
// build-time store, which decodes the same `-live-` marker to a
// different — and silently plausible — count.
let frame_value_count_fn = self.next_frame_value_count_fn.take();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Consume the frame-count override before validation

If either validate_call_assembler_rewrite_prereqs or validate_oprefs_for_compile rejects a trace, this .take() is never reached, so the per-driver override remains installed. A later compile_tmp_callback intentionally sets no override and will therefore decode its guard numbering with the failed trace's frame-count function rather than the process-global callback, producing incorrect but plausible exit layouts. Move consumption to the start of do_compile or clear it on every early return.

Useful? React with 👍 / 👎.

Comment thread pyre/pyre-jit/src/eval.rs
Comment on lines +5205 to +5208
if let Err(err) = pyre_interpreter::stack_check::drain_jit_pending_exception()
&& err.kind != pyre_interpreter::PyErrorKind::StopIteration
{
pending_err.get_or_insert(err);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Raise parked errors before builtin iterator fast paths

When a jd1 compiled entry leaves a non-StopIteration error here (notably the documented prologue-overflow RecursionError), it is re-parked for the next call boundary, but _unpackiterable_unknown_length next invokes baseobjspace::next, whose list/tuple/reverse-iterator fast paths return directly without draining this slot. For those iterator types the unpack can therefore finish normally and the error surfaces only at an unrelated later call; deliver the error synchronously from the merge hook or drain it before all next fast paths.

AGENTS.md reference: AGENTS.md:L14-L18

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@majit/majit-backend-cranelift/src/compiler.rs`:
- Around line 8416-8421: Move the one-shot override consumption for
next_frame_value_count_fn, next_trace_id, and next_header_pc to the beginning of
do_compile, before validate_call_assembler_rewrite_prereqs and
validate_oprefs_for_compile can return early. Preserve each override’s existing
fallback behavior while ensuring rejected compilations cannot leave these
callbacks installed for the next compile.

In `@majit/majit-metainterp/src/lib.rs`:
- Around line 371-381: Update the `exc_value` field documentation to note that
it may also be `0` when `exit_with_exception` is true for the full-body-walk
uncaught-raise path, which propagates the exception through
`WALK_END_PROPAGATED_EXCEPTION` instead. Preserve the existing explanation of
the normal exception-value flow.

In `@pyre/bench/synth/unpack_drain_star_raise.py`:
- Line 7: Update the test cases around EARLY_AT and the jd1 trace-threshold
setup so EARLY_AT is derived from the shared threshold constant or explicitly
asserted to remain beyond it. Preserve the distinct behavior of case 4 and
ensure threshold changes cannot silently make it duplicate case 3.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5b08e7c0-d8cb-48ba-88a3-37f3b1574f05

📥 Commits

Reviewing files that changed from the base of the PR and between 519c1a9 and cbea7fe.

📒 Files selected for processing (11)
  • majit/majit-backend-cranelift/src/compiler.rs
  • majit/majit-metainterp/src/jitdriver.rs
  • majit/majit-metainterp/src/lib.rs
  • majit/majit-metainterp/src/pyjitpl/dispatch.rs
  • pyre/bench/synth/unpack_drain_star_raise.py
  • pyre/pyre-interpreter/src/baseobjspace.rs
  • pyre/pyre-interpreter/src/eval.rs
  • pyre/pyre-interpreter/src/stack_check.rs
  • pyre/pyre-jit-trace/src/state.rs
  • pyre/pyre-jit-trace/src/trace.rs
  • pyre/pyre-jit/src/eval.rs

Comment on lines +8416 to +8421
// One-shot like `next_trace_id` / `next_header_pc`: a compile entry that
// installs no override (`compile_tmp_callback`) must fall back to the
// process-global callback rather than inherit the previous driver's
// build-time store, which decodes the same `-live-` marker to a
// different — and silently plausible — count.
let frame_value_count_fn = self.next_frame_value_count_fn.take();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The one-shot take() is bypassed by the two earlier ? returns.

validate_call_assembler_rewrite_prereqs (Line 8384) and validate_oprefs_for_compile (Line 8409) return before this point, so a rejected compile leaves next_frame_value_count_fn (and next_trace_id / next_header_pc) installed for whatever compiles next — precisely the stale-inheritance the comment argues against. Taking all three overrides at the top of do_compile, before the validators, closes that window.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@majit/majit-backend-cranelift/src/compiler.rs` around lines 8416 - 8421, Move
the one-shot override consumption for next_frame_value_count_fn, next_trace_id,
and next_header_pc to the beginning of do_compile, before
validate_call_assembler_rewrite_prereqs and validate_oprefs_for_compile can
return early. Preserve each override’s existing fallback behavior while ensuring
rejected compilations cannot leave these callbacks installed for the next
compile.

Comment on lines +371 to +381
/// The concrete exception object this finish escapes with, as a raw
/// GC-ref word (`0` when `exit_with_exception` is false).
///
/// `pyjitpl.py:2530-2562 finishframe_exception` snapshots
/// `excvalue = self.last_exc_value` *before* calling
/// `compile_exit_frame_with_exception(self.last_exc_box)` and then
/// raises `jitexc.ExitFrameWithExceptionRef(excvalue)`, which
/// `warmspot.py:998-1005` re-raises out of `ll_portal_runner`. The
/// compile half consumes only the symbolic `finish_args`; the raise
/// half needs the value, so it travels alongside them.
exc_value: i64,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Doc comment doesn't cover the known exit_with_exception=true, exc_value=0 case.

The doc states exc_value is 0 when exit_with_exception is false, implying it's populated otherwise. But pyre-jit-trace/src/trace.rs's full-body-walk Terminate handling intentionally sets exc_value: 0 on an uncaught-raise arm where exit_with_exception: true, relying on a separate WALK_END_PROPAGATED_EXCEPTION channel instead. Worth documenting this exception on the field so future producers/consumers don't assume exc_value is authoritative whenever exit_with_exception is true.

📝 Suggested doc tweak
         /// The concrete exception object this finish escapes with, as a raw
-        /// GC-ref word (`0` when `exit_with_exception` is false).
+        /// GC-ref word (`0` when `exit_with_exception` is false, or when the
+        /// producer routes the exception through a different channel — e.g.
+        /// the full-body-walk tracer's uncaught-raise arm, which delivers
+        /// via `WALK_END_PROPAGATED_EXCEPTION` instead).
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// The concrete exception object this finish escapes with, as a raw
/// GC-ref word (`0` when `exit_with_exception` is false).
///
/// `pyjitpl.py:2530-2562 finishframe_exception` snapshots
/// `excvalue = self.last_exc_value` *before* calling
/// `compile_exit_frame_with_exception(self.last_exc_box)` and then
/// raises `jitexc.ExitFrameWithExceptionRef(excvalue)`, which
/// `warmspot.py:998-1005` re-raises out of `ll_portal_runner`. The
/// compile half consumes only the symbolic `finish_args`; the raise
/// half needs the value, so it travels alongside them.
exc_value: i64,
/// The concrete exception object this finish escapes with, as a raw
/// GC-ref word (`0` when `exit_with_exception` is false, or when the
/// producer routes the exception through a different channel — e.g.
/// the full-body-walk tracer's uncaught-raise arm, which delivers
/// via `WALK_END_PROPAGATED_EXCEPTION` instead).
///
/// `pyjitpl.py:2530-2562 finishframe_exception` snapshots
/// `excvalue = self.last_exc_value` *before* calling
/// `compile_exit_frame_with_exception(self.last_exc_box)` and then
/// raises `jitexc.ExitFrameWithExceptionRef(excvalue)`, which
/// `warmspot.py:998-1005` re-raises out of `ll_portal_runner`. The
/// compile half consumes only the symbolic `finish_args`; the raise
/// half needs the value, so it travels alongside them.
exc_value: i64,
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@majit/majit-metainterp/src/lib.rs` around lines 371 - 381, Update the
`exc_value` field documentation to note that it may also be `0` when
`exit_with_exception` is true for the full-body-walk uncaught-raise path, which
propagates the exception through `WALK_END_PROPAGATED_EXCEPTION` instead.
Preserve the existing explanation of the normal exception-value flow.

# is the shape that exercises the second jit driver end to end.
#
# Three cases, each driven past the jd1 trace threshold:
# Five cases, each driven past the jd1 trace threshold:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

EARLY_AT = 99 silently couples this case to the jd1 trace threshold.

If the threshold moves, case 4 degenerates into a duplicate of case 3 with no signal. Deriving it from a shared constant (or asserting the relationship) keeps the case meaningful.

Also applies to: 17-35

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pyre/bench/synth/unpack_drain_star_raise.py` at line 7, Update the test cases
around EARLY_AT and the jd1 trace-threshold setup so EARLY_AT is derived from
the shared threshold constant or explicitly asserted to remain beyond it.
Preserve the distinct behavior of case 4 and ensure threshold changes cannot
silently make it duplicate case 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant