jit: record the catching frame's traceback node at bridge handler entries, and widen bridge ref-root/merge-point handling - #893
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (53)
WalkthroughThe PR changes JUMP type handling, bridge compilation outcomes, abort accounting, traceback journaling, diagnostics, and benchmark statistics. It also adds an exception traceback benchmark. ChangesJIT compilation and bridge control
Bridge-entry traceback journaling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 302618c). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
![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: 1b2dec2924
ℹ️ 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".
| if !try_multiframe | ||
| && inline_caller_frame_catch_marker_decline | ||
| == Some(InlineCallerFrameDecline::TryBlockCatchMarker) | ||
| { | ||
| return Ok(None); |
There was a problem hiding this comment.
When a strict-inlinable call is covered by a non-rejoining exception handler, this returns Ok(None) based on strict_seed before determining whether the callee can actually be seeded. For shapes rejected later by the seed block—for example, a strict callee with nonempty cellvars—the previous behavior fell back to the working single-frame inline, whereas this change now residualizes every call, causing a hot-loop performance regression. Only decline here after the seed preconditions have established that the strict callee will actually use the multiframe path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs`:
- Around line 4883-4893: Move FBW_TRACEBACK_STORE_JOURNAL out of thread-local
storage and into the active WalkSession as session-owned rollback state. Update
the journal access, commit, and discard paths to use that session field, and
include it in the existing session traversal/rooting so both PyObjectRef values
remain GC roots for the walk lifecycle. Remove the TLS declaration and avoid
introducing replacement thread-local state.
🪄 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: 8ae829c7-c384-4310-bb62-ba686d80d69a
📒 Files selected for processing (19)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-metainterp/src/optimizeopt/optimizer.rsmajit/majit-metainterp/src/optimizeopt/unroll.rsmajit/majit-metainterp/src/pyjitpl.rspyre/bench/fannkuch.cranelift.jitstatspyre/bench/fannkuch.dynasm.jitstatspyre/bench/fib_loop.cranelift.jitstatspyre/bench/fib_loop.dynasm.jitstatspyre/bench/nbody.cranelift.jitstatspyre/bench/synth/comprehension_object_append_hot.cranelift.jitstatspyre/bench/synth/comprehension_object_append_hot.dynasm.jitstatspyre/bench/synth/exception_bridge_traceback_head.pypyre/bench/synth/nested_list_comprehension_hot.cranelift.jitstatspyre/bench/synth/nested_list_comprehension_hot.dynasm.jitstatspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs
👮 Files not reviewed due to content moderation or server errors (2)
- majit/majit-backend-cranelift/src/compiler.rs
- majit/majit-metainterp/src/optimizeopt/optimizer.rs
| /// Undo entry for the concrete traceback-head store performed while a | ||
| /// bridge-entry exception arm is recorded: `(exception, attached_node)`. | ||
| /// The two arms are mutually exclusive and each is entered at most once | ||
| /// per walk session, so one slot covers the concrete attach. It shares | ||
| /// the store journal's lifecycle: a committing walk keeps the node and | ||
| /// clears the entry, while a discarded walk removes the node only when it | ||
| /// is still the exception's current traceback head. Both refs are GC | ||
| /// roots via [`fbw_store_journal_root_walker`]. | ||
| static FBW_TRACEBACK_STORE_JOURNAL: | ||
| std::cell::RefCell<Option<(pyre_object::PyObjectRef, pyre_object::PyObjectRef)>> = | ||
| const { std::cell::RefCell::new(None) }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Move the traceback journal out of TLS.
This is interpreter-owned, identity-sensitive semantic rollback state, not a thread-specific disposable cache. Store it on the active WalkSession (and root it through the existing session traversal) so lifecycle, rollback, and GC ownership follow the walk rather than a thread-local slot. The new TLS use also lacks the required upstream citation and written justification.
As per coding guidelines, “Do not use TLS for process-global, interpreter-owned, identity-sensitive, semantic, registry, cache, or GC-relevant runtime state.”
🤖 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/pyre-jit-trace/src/jitcode_dispatch/mod.rs` around lines 4883 - 4893,
Move FBW_TRACEBACK_STORE_JOURNAL out of thread-local storage and into the active
WalkSession as session-owned rollback state. Update the journal access, commit,
and discard paths to use that session field, and include it in the existing
session traversal/rooting so both PyObjectRef values remain GC roots for the
walk lifecycle. Remove the TLS declaration and avoid introducing replacement
thread-local state.
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1fd1c6564
ℹ️ 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".
| assert!( | ||
| entry.is_none(), | ||
| "bridge-entry traceback attach ran more than once in one walk session" | ||
| ); |
There was a problem hiding this comment.
When an exception-edge carrier enters a callee handler and that handler raises a second exception caught by the root frame, the callee entry records the first traceback undo here, then drive_bridge_carrier_walk continues the root with WalkJournals::Keep and the carrier-raise-seed arm records another. Because this journal is a single Option, the second valid attach hits this assertion; in release builds this is still an unconditional panic, and rollback could not restore both mutations anyway. Store all attaches for the carrier session and unwind them in reverse order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-metainterp/src/pyjitpl.rs (1)
5215-5238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the cross-loop-cut vable_ptr / bad-loop fix.
orig_vable_ptr_for_cutand its two call sites (compile_loop_body'scut_merge_point/orig_vable_ptr_loop, and the symmetriccompile_retracepath) correctly resolve the virtualizable pointer from the merge point override when present, falling back to the trace-ctx resolution otherwise — consistent with the documentedvable_ptr == 0sentinel onMergePoint. Given this, together with thehas_compiled_targetsfix in Line 5461, resolves a previously-mishandled cross-loop-cut scenario (trace closing into an already-compiled inner loop with a live virtualizable), it would be valuable to add a targeted unit test exercisingorig_vable_ptr_for_cutdirectly (similar in style to the existingtest_declared_identity_position_wins_over_an_earlier_alias/identity_live_positiontests) covering both themp.vable_ptr != 0override branch and the0-sentinel fallback branch, plus an end-to-end case where a cross-loop-cut trace targets an inner loop that already has compiled targets.Also applies to: 5524-5538, 6992-7003
🤖 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/pyjitpl.rs` around lines 5215 - 5238, Add regression tests for orig_vable_ptr_for_cut covering both a nonzero MergePoint.vable_ptr override and the zero-sentinel fallback to trace-context resolution, following the style of the existing identity-position tests. Also add an end-to-end test covering compile_loop_body and compile_retrace where a cross-loop-cut trace targets an inner loop with existing compiled targets and a live virtualizable.
🤖 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 4994-5023: Update the diagnostic guard around
log_internal_jump_type_mismatches so it runs in debug builds as well as when
MAJIT_LOG is set. Preserve the existing opt-in behavior for release builds, and
ensure debug execution asserts or otherwise fails when an internal JUMP/LABEL
type mismatch is detected.
---
Outside diff comments:
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 5215-5238: Add regression tests for orig_vable_ptr_for_cut
covering both a nonzero MergePoint.vable_ptr override and the zero-sentinel
fallback to trace-context resolution, following the style of the existing
identity-position tests. Also add an end-to-end test covering compile_loop_body
and compile_retrace where a cross-loop-cut trace targets an inner loop with
existing compiled targets and a live virtualizable.
🪄 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: cbf5c0f7-71ac-4d43-87a9-bef8a8322521
📒 Files selected for processing (20)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-metainterp/src/optimizeopt/optimizer.rsmajit/majit-metainterp/src/optimizeopt/unroll.rsmajit/majit-metainterp/src/pyjitpl.rspyre/bench/fannkuch.cranelift.jitstatspyre/bench/fannkuch.dynasm.jitstatspyre/bench/fannkuch.wasm.jitstatspyre/bench/fib_loop.cranelift.jitstatspyre/bench/fib_loop.dynasm.jitstatspyre/bench/nbody.cranelift.jitstatspyre/bench/synth/comprehension_object_append_hot.cranelift.jitstatspyre/bench/synth/comprehension_object_append_hot.dynasm.jitstatspyre/bench/synth/exception_bridge_traceback_head.pypyre/bench/synth/nested_list_comprehension_hot.cranelift.jitstatspyre/bench/synth/nested_list_comprehension_hot.dynasm.jitstatspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21423dfef9
ℹ️ 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".
| guard_failures=5394 | ||
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_aborted=4 |
There was a problem hiding this comment.
Changing this baseline from loops_aborted=0 to 4 teaches pyre/check.py that four failed loop compilations are acceptable, because _jit_stats_regression_floor only rejects values above the saved count; the matching dynasm and wasm edits disarm the same safeguard across every backend. The repository explicitly treats benchmark regressions as a reason not to commit, so investigate and fix the four aborts rather than recording them as the new floor.
AGENTS.md reference: AGENTS.md:L201-L201
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d33d2d7d00
ℹ️ 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".
| if current == node { | ||
| let previous = unsafe { pyre_interpreter::pytraceback::w_pytraceback_get_w_next(node) }; | ||
| unsafe { | ||
| pyre_object::interp_exceptions::w_exception_set_traceback(exception, previous); | ||
| } | ||
| } |
There was a problem hiding this comment.
When later concrete execution prepends another traceback node to the same exception before the walk aborts—for example, while executing a handler that re-raises or invokes another raising callee—current != node, so this rollback silently leaves the speculative bridge-entry node in the traceback chain. The replay path then records the catching frame again, exposing a duplicated frame even though the original walk was discarded; rollback needs to splice the journaled node out beneath newer heads rather than only handling the exact-head case.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
…ef-root scan `build_ref_root_slots` returned `BackendError::Unsupported` when the closing JUMP passed a Float at a Ref-typed inputarg position, failing the whole bridge compile. The scan already records that position in `non_ref_at_backedge`, which keeps it out of the emitted root list, so the GC never reads the non-pointer bits there; the extra bail rejected a case the surrounding code had already made safe. The Int crossing, which the bail let through, is the one that can still carry a live GC pointer forwarded through SameAsI. The dynasm backend has no analogous check: regalloc builds its gcmap from the runtime live type rather than the declared inputarg type. Assisted-by: Claude
… closed merge point The check read `ctx.green_key`, the trace's own key. pyjitpl.py:3185-3187 reads `original_boxes[:num_green_args]` — the greens of the merge point that was closed at, which for a cross-loop cut is the inner loop's key. Moved the check below the `cut_inner.unwrap_or(outer)` derivation so it uses the same key the loop is stored under two statements later. The abort went through `abort_trace(false)`, which tallies `AbortReason::Generic` (ABORT_BRIDGE). Replaced with the `abort_trace_live` + `aborted_tracing` pair so the profiler counts it under ABORT_BAD_LOOP, the reason pyjitpl.py:3189 raises. Assisted-by: Claude
fannkuch (both backends): loops_aborted 0 -> 4, bridges_compiled 26 -> 23, guard_failures 5628 -> 5394. nbody (cranelift): bridges_compiled 7 -> 6, guard_failures 1949 -> 1580, now equal to dynasm. fib_loop, comprehension_object_append_hot and nested_list_comprehension_hot move by one guard failure. That counter is build-sensitive: two dynasm binaries built from the same source read 190 and 191 on fib_loop, each stable across five runs. Assisted-by: Claude
…targ types alone build_ref_root_slots dropped a Ref-declared inputarg from the GC root list whenever the trace's closing JUMP passed a non-Ref value at the same position. The two lists are only the same list when the JUMP closes back onto the trace's own entry args: a bridge's JUMP targets another trace's label, and an unrolled loop's JUMP targets the LABEL in the middle of the trace, so the comparison lined up positions from two unrelated lists. Remove the JUMP-derived exclusion. regalloc.py:786 keys the gcmap on the live value's own type, and x86/regalloc.py:1303-1326 consider_jump moves each arg into the target's `_x86_arglocs[i]`, so the closing JUMP carries no information about this trace's own root map. In its place, log_internal_jump_type_mismatches reports the invariant that does hold — a JUMP and the LABEL it targets agree type-for-type — for the LABELs present in the same trace, under MAJIT_LOG. It reports and does not compensate. Across pyre/bench plus pyre/bench/synth (340 programs) the removed check fired 4 times, all four inside one nbody bridge; the reinstated invariant reports 0 mismatches over the same set. Assisted-by: Claude
…inputargs the JUMP writes into The terminal-op pass compared each JUMP arg's resolved type against `trace_inputargs[i]`. That names the positions the JUMP writes into only on the unroll paths, where unroll.py:454 builds `end_args` from `original_label_args`. A bridge's JUMP targets another trace's label while `trace_inputargs` holds the guard's failargs, so the check read unrelated positions; the bridge's type contract is virtual-state matching's (virtualstate.py:646-653 generate_guards). Gate the check on `!building_bridge` and let the plain force/materialize loop run otherwise. Log the surviving Ref-preservation branch under MAJIT_LOG. Over pyre/bench plus pyre/bench/synth (340 programs) it does not fire. unroll.rs: the jump_to_preamble comment described rejecting body JUMPs whose types disagree with the preamble inputargs "until force_box_for_end_of_preamble is implemented". That function is implemented (optimizer.rs force_box_for_end_of_preamble) and runs over the body JUMP's args earlier in the same function; no such rejection exists in the code. Restate what the block does. Assisted-by: Claude
…ler entry, and undo it when the walk is discarded
Seven walker paths enter an `except` handler. The five in-trace ones record a
traceback node for the catching frame; the two in `dispatch_via_miframe` — the
exception-edge arm and the carrier-raise-seed arm — called
`vstack_enter_exception_handler` with no record at all. On the bridge leg the
raising callee ran as real frames, so its nodes were attached by the interpreted
raise machinery, but the catching frame is the compiled one and its node exists
only if the trace records it. A handler reading `__traceback__` therefore saw a
chain whose head was the callee frame. `pyopcode.py` runs
`record_application_traceback` before `lookup_exceptiontable` routes to the
handler.
Add the same record triple to both arms.
The concrete leg of that record mutates the live exception, and a walk that is
later discarded leaves the mutation behind while the metainterp's own delivery
attaches the node again — `gc_bug_bridge_flavor_traceback_names` then printed
`('V', 'a_bridge_two_classes', 'a_bridge_two_classes', 'mid_two', 'leaf_two')`.
Journal the attach: the arm records `(exception, node)` in a slot that shares
the store journal's lifecycle, the rollback epilogues restore
`exception.w_traceback = node.tb_next` while that node is still the head, and
the commit path clears the slot without applying it. Both refs are GC roots
through the existing store-journal root walker.
Add `synth/exception_bridge_traceback_head.py`, which pins the head frame name
for two exception classes raised into one hot try. It reads the traceback
inline, so it does not depend on the callee-inline gates and catches this class
directly.
14 traceback fixtures, 3 runs each, both backends: all match `PYRE_JIT=off`.
check.py FAIL sets unchanged on both backends (dynasm 62, cranelift 63).
Assisted-by: Claude
… behind `bench: re-record the .jitstats baselines on both backends` recorded fannkuch's `loops_aborted` 0 -> 4 for dynasm and cranelift only. The wasm baseline kept 0, so `_jit_stats_regression_floor` — which runs on every invocation, not just under `--snapshot-diff` — reported the rise on the ubuntu wasm job while both native backends stayed green against their raised baselines. The rise itself is the one `jit: let the walker fall through to the merge-point scan when compile_trace declines` measured and states: the scan now reaches `compile_loop`, which gives up twice at its own `has_compiled_targets` (`pyjitpl.py:3185-3189`, where upstream raises as well) and the optimizer rejects two more as InvalidLoop, against `bridges_compiled` 26 -> 23 and `guard_failures` 5628 -> 5394. The removed skip had no counterpart in `reached_loop_header` (`pyjitpl.py:3018-3022`). Recording it on wasm too makes the three backends state the same thing. Nonzero `loops_aborted` baselines are already how this ratchet is expressed — the wasm baselines for `comprehension_object_append_hot`, `const_arg_call_resume` and `nested_list_comprehension_hot` carry 2, 1 and 2. `check.py --backend wasm`: 1 failed, 340 passed; the remaining failure is `synth/ast_compile_roundtrip`, which this host reports BASEFAIL (the cpython/pypy oracle itself) and which passes in CI. Assisted-by: Claude
The `jit_merge_point` arm returned `Continue` when the arriving green key already had compiled targets, `compile_trace` did not take, and the key was not the trace's root, skipping both the merge-point scan and the merge-point registration below it. `reached_loop_header` has no such skip. After its own `self.compile_trace(...)` returns without raising it runs the reverse scan over `current_merge_points` (pyjitpl.py:3018-3022) and closes at the first `same_greenkey` hit, where `compile_loop` gives the trace up at its own `has_compiled_targets` (pyjitpl.py:3186-3189); a first visit appends a merge point and keeps tracing (pyjitpl.py:3057-3059). The `!retrace_requested` carve-out was for the missing registration on the retrace path, so removing the skip subsumes it. Assisted-by: Claude
…an abort once `close_bridge` mapped every non-`Compiled` outcome to `Failed`, and both `Failed` arms in the driver ran `abort_trace(false)` plus a session teardown. `compile.compile_trace` returns None on InvalidLoop (compile.py:227) without an abort tally, and `raise_if_successful` (pyjitpl.py:3119) does not raise on None, so the trace is not given up. Add `BridgeCompileResult::Declined` for that outcome and let the driver fall through to compile_loop, as the `RetraceNeeded` arm already does. `Cancelled` with no trace ctx at all keeps mapping to `Failed`: the fall-through needs the ctx compile_loop reads. compile_loop's `has_compiled_targets` branch called `abort_trace_live` + `aborted_tracing(ABORT_BAD_LOOP)` itself, and the driver's `Aborted` arm then called `abort_trace`, which tallies again under `Generic`. Upstream names the reason at the `raise SwitchToBlackhole(reason)` and tallies once in the catch, so stage the reason in `pending_abort_reason` and let `abort_trace` consume it. fannkuch `loops_aborted` 4 -> 2 on dynasm, cranelift and wasm; the wasm baselines for `comprehension_object_append_hot` 2 -> 0, `const_arg_call_resume` 1 -> 0 and `nested_list_comprehension_hot` 2 -> 0. Assisted-by: Claude
Adds the descr_set_absent / descr_set_ambiguous / descr_set_stale_absent fields to every tracked baseline. Counter movement: fannkuch loops_aborted 4 -> 2 on all three backends, nbody bridges_compiled 6 -> 5 and guard_failures 1580 -> 1547 on the natives, comprehension_object_append_hot guard_failures 3612 -> 3611 on dynasm, and loops_aborted -> 0 for the wasm comprehension_object_append_hot (2), const_arg_call_resume (1) and nested_list_comprehension_hot (2). Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58fe4af2e0
ℹ️ 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".
| // the type contract belongs to virtual-state matching | ||
| // (virtualstate.py:646-653 `generate_guards`), and what applies here | ||
| // is the plain force_box loop. | ||
| let inputargs_are_the_jump_target = !self.building_bridge; |
There was a problem hiding this comment.
When a bridge closes to a compiled preamble whose label expects a Ref but the corresponding box has been forwarded to an Int or Float, this condition disables the only logic that retains the original Ref box. Virtual-state matching does not protect the direct single-target path or the fallback taken after the retrace limit, and force_box_for_end_of_preamble does not undo a non-virtual type-changing forwarding, so the external JUMP can deliver scalar bits to code that treats them as a GC pointer. Derive the expected types from the target token's label instead of disabling the check for every bridge.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
…h outcomes `loops_aborted` on wasm could be traced no further than "the walker's compile_trace did not take": every route out of `compile_trace_inner` returned the same `Cancelled`, and the wasm backend's index-8 decline covered both a stripped JUMP descr and an unpublished target label. MC_DIAG gains 27..33 — the walker fall-through, `compile_loop`'s has_compiled_targets giveup, and one slot per `compile_trace` cancel site (no front target token / origin loop gone / entry bridge failed / no entry-bridge data / not tracing). BRIDGE_DIAG gains 17..22 — the index-8 split (no descr vs unpublished) plus the publish-side counterpart (labels published off a peeled loop, off a non-peeled loop, left unpublished, and retracted by a dropped loop). `pyrex` had its own 18-entry copy of the MC_DIAG names against 27 counters, so the last nine printed nothing; it now reads `MC_DIAG_LABELS` from `majit_metainterp` directly. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 302618ca17
ℹ️ 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".
| entry.is_none(), | ||
| "bridge-entry traceback attach ran more than once in one walk session" | ||
| ); | ||
| *entry = Some((exception, node)); |
There was a problem hiding this comment.
When this bridge handler is walked inside an inlined call and a later unsupported opcode aborts that sub-walk, this journal entry is invisible to FBW_EXECUTED_EFFECT_COUNT. The zero-delta abort gate can therefore commit the journal and rewind to the caller's CALL; re-executing that call records the catching frame again, leaving a duplicate traceback node. Bump the executed-effect odometer when recording the concrete attach, as the other journaled heap mutations do, so a call containing this mutation cannot be committed and re-executed.
AGENTS.md reference: AGENTS.md:L16-L19
Useful? React with 👍 / 👎.
1 participant
Eight commits on top of
main, all in the bridge-compilation path. The last twoare the substantive ones; the first six were landed earlier on this branch and
are carried across the rebase.
jit(fbw): the catching frame's traceback node at a bridge handler entryWrong output in shipped binaries on both backends, not just a missed
optimization.
Seven walker paths enter an
excepthandler. The five in-trace ones record atraceback node for the catching frame; the two in
dispatch_via_miframe— theexception-edge arm and the carrier-raise-seed arm — called
vstack_enter_exception_handlerwith no record at all. On the bridge leg theraising callee runs as real frames, so its nodes are attached by the interpreted
raise machinery, but the catching frame is the compiled one and its node exists
only if the trace records it. A handler reading
__traceback__saw a chainwhose head was the callee frame.
pyopcode.pyrunsrecord_application_tracebackbeforelookup_exceptiontableroutes to thehandler.
Both arms now record the same triple. The concrete leg of that record mutates
the live exception, and a walk that is later discarded would leave the mutation
behind while the metainterp's own delivery attaches the node again — the
doubled head observed on
gc_bug_bridge_flavor_traceback_names. So the attachis journalled: the arm records
(exception, node)in a slot sharing the storejournal's lifecycle, the rollback epilogues restore
exception.w_traceback = node.tb_nextwhile that node is still the head, andthe commit path clears the slot without applying it. Both refs are GC roots
through the existing store-journal root walker.
synth/exception_bridge_traceback_head.pypins the head frame name for twoexception classes raised into one hot try. It reads the traceback inline, so it
does not depend on the callee-inline gates.
jit(fbw): hoist the inline caller-frame catch-marker declineThe decline was decided after the callee frame had been seeded and IR recorded.
Deciding it from the same caller payload before the
'seedblock removes thehalf-built state; the post-seed arm is now
unreachable!.exception_traceback_frame_lineno:loops_aborted 7 -> 2,loops_compiled 12 -> 13.Carried commits
jit(cranelift): drop the Float-at-Ref-inputarg bail from the bridge ref-rootscan, and build the ref-root list from the declared inputarg types alone.
jit(metainterp): keycompile_loop'shas_compiled_targetscheck to theclosed merge point; scope the end-of-
JUMPRef check to the traces whoseinputargs the
JUMPwrites into.jit: let the walker fall through to the merge-point scan whencompile_tracedeclines.bench: re-record the.jitstatsbaselines on both backends.Verification
Rebased onto
mainafter #886 and #876 landed, LLBC re-extracted, both backendsrebuilt.
PYRE_JIT=off: 0 diverged, no crashes. Includesexception_traceback_frame_lineno(the indicator jit: route the exception-edge bridge on the catch alone #886 names) andexc_caught_in_callee_return_loop(a shape jit: route the exception-edge bridge on the catch alone #886 newly routes through theexception-edge arm), plus
str_search_index_bounds.cargo testonpyre-jit-trace,pyre-jit,majit-metainterp: 0 failed.check.py --snapshot-diff: 73 failed / 270 passed on both backends, the samecount
mainproduces on this host. The localbench/synth/*.jitstatsareuntracked scratch from a pre-rebase run, so those diffs are not attributable
on their own. Measured directly instead, by reverting this branch's
.rsfiles to
mainand rebuilding: of the 11 benches reporting aloops_abortedregression, 10 read identically on
main, andexception_try_call_inlined_callee_raisegoes 4 -> 1 on this branch. Noloops_abortedregression originates here.const_arg_call_resumereadsguard_failures1805 against a recorded 1804, the build-sensitive +/-1 notedin the re-record commit. It is not a badness field and does not gate a plain
run.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Diagnostics