Repository navigation
jit: reset the EXTENDED_ARG accumulator at trace-walk block entry - #647
Conversation
WalkthroughThis PR adds regression benchmarks for JIT behavior and updates Pyre’s JIT pipeline to handle bytecode control flow, exception propagation, blackhole resumes, residual-call dispatch, and single-byte JitCode encoding limits. ChangesJIT correctness updates
Possibly related issues
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 afb9e14). 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)None. 4. Structural adaptations
|
![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/bench/synth/inlined_callee_extended_arg_handler.py`:
- Line 16: Rename the unused loop variable in the `for` loop from `j` to `_` to
clearly indicate that the iteration value is intentionally ignored.
🪄 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
Run ID: d9e2f411-908f-48ce-a9b1-5c728e7678a2
📒 Files selected for processing (2)
pyre/bench/synth/inlined_callee_extended_arg_handler.pypyre/pyre-jit/src/jit/codewriter.rs
| try: | ||
| raise IndexError | ||
| except KeyError: | ||
| for j in range(2): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Rename unused loop variable.
The loop control variable j is not used within the loop body. Consider renaming it to _ to clarify intent.
♻️ Proposed fix
- for j in range(2):
+ for _ in range(2):📝 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.
| for j in range(2): | |
| for _ in range(2): |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 16-16: Loop control variable j not used within loop body
Rename unused j to _j
(B007)
🤖 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/inlined_callee_extended_arg_handler.py` at line 16, Rename
the unused loop variable in the `for` loop from `j` to `_` to clearly indicate
that the iteration value is intentionally ignored.
Source: Linters/SAST tools
![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: 8e52890601
ℹ️ 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 !(callbacks.ensure_majit_jitcode)(raw_code, code) { | ||
| return std::ptr::null(); |
There was a problem hiding this comment.
When the new checked assembly path declines a callee JitCode (for example a hot caller reaches a Python callee whose register+constant pool exceeds the 256-slot encoding limit), this raw null escapes to existing callers that do not handle it. In particular, trace_opcode.rs assigns sym.jitcode = jitcode_for(w_code) and immediately calls setup_kind_register_banks, whose only protection is a debug-only assertion before unsafe { &*self.jitcode }, so release builds can crash/UB instead of aborting the trace and falling back to the interpreter. Please thread an Option/trace-abort through the direct jitcode_for callers before returning null here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec3a112568
ℹ️ 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".
| // exception ref is the returned jitframe's slot 0, as staged by | ||
| // `run_compiled_code`'s propagate-exception handling. | ||
| let attached_ptrs = attachments.descr_ptrs(); | ||
| if attached_ptrs.is_exit_frame_with_exception_descr(fail_descr_ptr as usize) { |
There was a problem hiding this comment.
When a Cranelift CALL_ASSEMBLER callee exits through the propagate-exception path (for example CheckMemoryError stores attached_descr_ptrs().propagate_exception_descr into JF_DESCR), fail_descr_ptr is another attached singleton Arc data pointer, not a FailDescrCell. This new pre-recovery classification only catches ExitFrameWithExceptionDescrRef, so the subsequent recover_fail_descr_cell(fail_descr_ptr) still runs on the propagate descriptor pointer and can crash/UB instead of raising the pending exception; please route attached_ptrs.propagate_exception_descr here as the direct run_compiled_code path does before attempting cell recovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d6915dff7
ℹ️ 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".
| return false; | ||
| } | ||
| } | ||
| true |
There was a problem hiding this comment.
When a loop's only semantic backedge is in an exception handler, such as while ...: try: ...; break except E: ...; continue, the handler continue also has target_pc < handler_pc <= source_pc and there may be no earlier backward jump to the same target before the handler. This then returns true, so both the JumpBackward emission path and find_loop_header_pcs suppress the loop header; eval_loop_jit's new semantic_loop_headers gate will never call can_enter_jit for that hot loop, leaving it permanently interpreted even though it is a real loop backedge rather than layout-only cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/dcce183fa8cb39eabd8b76c000aac1b7a821ae8c/pyre/pyre-jit/src/jit/assembler.rs#L239
When an overflow is introduced while encoding a ListOfKind operand, this checked path still panics before it can return None: write_insn can call expect_list_regs_or_pool, which adds ConstInt/ConstRef entries to the builder and immediately does u8::try_from(reg_idx).expect(...). For residual-call or marker lists whose constant pool is already near the 256-slot limit, that bypasses the new decline plumbing and aborts the process instead of falling back to interpretation; the list encoder needs to latch/bubble overflow the same way as the new push_reg_u8/try_finish path.
ℹ️ 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".
💡 Codex Reviewhttps://github.com/youknowone/pyre/blob/e7cbdb173298f844e8f903fc018978ccd09670a1/pyre/pyre-jit/src/jit/assembler.rs#L239 For graphs whose overflow comes from constants/registers embedded in a https://github.com/youknowone/pyre/blob/e7cbdb173298f844e8f903fc018978ccd09670a1/pyre/pyre-jit/src/jit/codewriter.rs#L14171 This filter also drops real loop backedges when the only backward jump to the loop header is emitted from an out-of-line exception handler, such as a loop whose ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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 trace codewriter dequeues blocks in a non-sequential order but carried a single running OpArgState across the whole walk. A block ending on an EXTENDED_ARG code unit left stale high bits that folded into the next dequeued block's first instruction argument — decoding e.g. COMPARE_OP 148 as (1 << 8) | 148 = 404 — which Arg<ComparisonOperator>::try_from rejects, aborting JIT codegen with InvalidBytecode. Reset the accumulator at each block entry; within a block the walk is sequential and targets land on instruction starts, so in-block EXTENDED_ARG prefixes stay intact. Add a synth regression bench (inlined callee whose exception handler needs EXTENDED_ARG). Assisted-by: Claude
…trace walker
Structural pattern matching is not in the RPython subset, so the trace walker
aborts any trace that reaches a match opcode. MATCH_SEQUENCE / GET_LEN already
modelled their +1 stack effect before aborting, but MATCH_CLASS, MATCH_KEYS and
MATCH_MAPPING had no arm and fell through to the catch-all, which aborts WITHOUT
adjusting the modelled operand-stack depth. An unmodelled MATCH_CLASS (net -2:
pops subject/type/names, pushes the attrs tuple or None) left the depth two too
high, so the enclosing block's return link carried stale pattern temporaries and
tripped make_return's "expects 1 or 2 args" assert in the flatten pass — a JIT
codegen crash on e.g. `case [Node('lit', a), Node('lit', b)]:`.
Add arms modelling each opcode's stack effect (MATCH_CLASS -2, MATCH_KEYS +1,
MATCH_MAPPING +1) before the abort, matching the existing MATCH_SEQUENCE arm.
Add a synth regression bench (sequence-of-class-patterns match).
Assisted-by: Claude
A JitCode encodes each register/const-pool operand as a single byte, so the per-kind register+const-pool count must stay <= 256. Traces reaching 256 slots of a kind previously tripped the touch_*_reg_or_pool_slot / finish asserts and panicked the process. Route the overflow into a trace-compilation decline instead: the assembler latches an encoding_overflow flag and try_finish / try_assemble return None before any operand is patched; that None propagates up through finish_with_positions_from, transform_graph_to_jitcode, register_portal_jitdriver and jitcode_for so the caller drops the trace and the interpreter keeps running, as it does for an over-long trace. The unchecked finish/assemble wrappers retain the invariant as debug_assert for trusted callers. Adds a synth bench that forces >256 int slots in one hot function. Assisted-by: Claude
walker_record_guard_exception discarded the GuardException result and left last_exc_value as the recording-time const_ref. A residual- or method-raised exception caught in the same compiled loop then read that frozen constant instead of the live raised instance, so e.args, e.value and every other attribute read back the value seen when the trace was recorded rather than the current one. A same-frame raise was unaffected because its raised value stays live. Bind last_exc_value to the GuardException result box, supplying the recording-time shadow via set_opref_concrete, and fall back to const_ref only when class_of_last_exc_is_const. Thread class_of_last_exc_is_const through the walker and subwalk mode, set it on catch routing and inline-call catches, and reset it for fresh residual exceptions. Adds a synth bench covering a callee-raised ValueError and an exhausted-generator StopIteration. Assisted-by: Claude
…ames An exception raised while evaluating the argument of a self-recursive CALL_ASSEMBLER tail call and unwinding through several suspended copies of the same JIT frame reached the caller's handler as a raw uninitialized object (`TypeError: unsupported operand ... 'int' and 'object'`) instead of the real exception. The blackhole resume loop ran the caller's next opcode with `got_exception` already set, consuming the deliberately-garbage recursive-call return slot as a successful value. - blackhole_resume_via_rd_numb: skip bh.run() and the abort rollback while got_exception is set, so the pending exception propagates to the suspended caller before its post-call opcode runs. - Bind each blackhole frame's virtualizable_ptr from its own frame red in a multi-frame snapshot, where the single vable section names only the outer portal frame. - Give the portal frame/ec reds dedicated Ref colors above the allocator range so an always-live red is never coalesced with a local/stack value at an interior resume point. - cranelift call_assembler_guard_failure: classify the ExitFrameWithException FINISH descriptor and raise its exception from the returned jitframe slot. Adds a synth bench exercising self-recursive tail-call exception unwind. Assisted-by: Claude
A `break` inside an `except` handler is laid out after the protected body in
3.14, so it is encoded as a JUMP_BACKWARD even though its landing is past the
source loop. The trace walker treated that layout-only backward edge as a loop
header and emitted a loop-crossing whose resume snapshot never captured the
handler-bound local that is live after the loop; resuming the side-exit then
peeked an empty value stack ("stack underflow during interpreter peek").
Add backward_jump_is_handler_only_target: a backward jump whose source sits in
an exception handler and whose target precedes that handler, with no mainline
backedge to the same target, is a handler return, not a loop backedge. Exclude
such targets from find_loop_header_pcs, skip emit_loop_header for them in both
JumpBackward arms, and, in eval_loop_jit, skip JIT entry when a CloseLoop's
loop_header_pc is not a semantic loop header. Adds a synth bench exercising
break-from-except with a post-loop read of the handler-bound local.
Assisted-by: Claude
…LD_SLICE emit_frontend_buildslice_shadow_graph synthesized the implicit step of a two-argument BUILD_SLICE with Constant::none(), which lowers to the null Ref sentinel rather than the None object. A `del L[-n:]` then built a slice whose step was a null reference, corrupting slice normalization and crashing on guard-failure blackhole resume (SIGSEGV for `del L[-2:]`, wrong result for `del L[-1:]`). Pass w_None as a Ref constant instead. Adds a synth bench covering negative-bound open slice deletes in a hot loop. Assisted-by: Claude
…OAD_ATTR residual live on blackhole walker_int_specialization_operands now requires both operands to be exact builtin instances. A numeric subclass keeps the builtin int layout while its Python-visible class lives in w_class, so the raw int path ran int.__add__ on a subclass instead of its own __add__; such operands route through the residual BINARY_OP instead. classify_vstack_opcode gains TO_BOOL so the vstack effect of a conditional guarding the polymorphic site is tracked. filter_liveness_in_place seeds the in-place LOAD_ATTR residual receiver/result into the blackhole Ref liveness so they stay live across a guard-failure deopt, which otherwise read a stale slot and faulted. The physical GuardClass in walker_unbox_int_typed already side-exits on a later subclass arrival and hoists out of monomorphic int loops, so no extra per-site w_class guard is emitted. polymorphic_binary_receiver.py exercises int/user-class and int/int-subclass alternation at a shared BINARY_OP site plus the reflected-dispatch LOAD_ATTR blackhole path; jit == nojit on both backends. Assisted-by: Claude
build_residual_call_r_r_insn_from_operands classified bh_call_fn_N with the helper wrapper's empty effect set, so the tracing heapcache and OptHeap kept cached heap values across the residual call. The callable is resolved at runtime and can mutate any escaped heap location, so a callee that rewrites an IntMutableCell.intvalue in place (a module-global reassign that leaves the module-dict version unchanged) went unobserved and the caller loop read the stale global. CallFn now uses EffectInfo::MOST_GENERAL (RANDOM_EFFECTS), which forgets cached fields across the call; other call flavors are unchanged. callee_store_global_read_after_call.py reads a module global in a hot loop that a called helper toggles in place; jit == nojit. Assisted-by: Claude
UNPACK_SEQUENCE and UNPACK_EX inside a try region lowered to a raising length validator followed by the item-reader residuals, with catch_exception emitted only after the readers. When a specialization guard failed and the blackhole re-executed the validator on a mismatching runtime arity, the raised ValueError landed on an item reader instead of the byte-adjacent catch marker and escaped the enclosing handler. The validator now closes its own can-raise block with catch_exception immediately after its trailing marker, and the item reads start a fresh continuation block. The mixed int/ref residual-call GuardNoException snapshot is captured with residual_call_catch_resume so its resume lands at the call's own catch coordinate rather than a generic fallthrough that may sit outside the covered exception-table range. unpack_wrong_arity_caught.py exercises too-many / not-enough tuple and list unpacks caught by ValueError in a hot loop; jit == nojit. Assisted-by: Claude
The full-body walk traverses flattened JitCode whose layout can switch to a different source block without the Python PCs forming a CFG edge. The previous SWAP/COPY-only detection missed conditional-expression arms: replaying the prior opcode across such a switch reused a vstack_last_ref belonging to the other block, overwriting the loop-carried FOR_ITER slot with the selected local, and a later branch guard serialized that foreign box into the virtualizable resume image, losing the range iterator (TypeError: not an iterator). Detect the layout switch by comparing the transition against the semantic fallthrough and branch-target PCs; enter the shadow-reseed region on any non-successor transition, subsuming the former SWAP/COPY case. Two synth benches guard it: a self-referential conditional, and an object-select receiver whose two backing locals swap mid-loop. Assisted-by: Claude
A guard-failure bridge that re-enters an inlined helper containing exact- integer residual arithmetic built a second virtual frame whose operand stack was not a bridge red input, so an overflow path compiled NULL vable stack slots into the bridge. Separately, the abort-point flush cleared the outer-call stack overrides even when the flush had not committed, and collect_call_stack_overrides dropped a slot's NULL null-or-self sentinel, so the reconstructed resume frame diverged from the interpreter. - In a bridge trace, keep an all-builtin-integer callee whose body has a BinaryOp residual as a single residual call instead of re-inlining it; the primary loop still inlines, so hot-path codegen is unchanged. - Preserve the CALL stack override for a NULL sentinel slot and source it after the live register/vstack, using the virtualizable shadow only as a fallback. - Only clear the outer-call stack overrides once flush_walk_end_state has actually committed them (track committed_entry_carrier_call_py_pc), and take the validated override as authoritative in the flush. Bench pyre/bench/synth/inline_bignum_bridge_twoclamp.py. Assisted-by: Claude
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 (2)
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs (2)
815-824: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing
FBW_CELL_STORE_JOURNALcheck in R1 never-double guards.Both resume-delivery selectors omit the
FBW_CELL_STORE_JOURNALcheck when evaluating if an irreversible body effect has been committed since the consume. This creates a risk of double-applying cell mutations upon re-running the body, as the journal condition is incomplete.
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs#L815-L824: add|| FBW_CELL_STORE_JOURNAL.with(|j| j.borrow().len()) != 0to the refusal condition.pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs#L850-L855: add&& FBW_CELL_STORE_JOURNAL.with(|j| j.borrow().len()) == 0to the acceptance condition.🤖 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/fbw_state.rs` around lines 815 - 824, Both R1 never-double resume-delivery selectors must account for FBW_CELL_STORE_JOURNAL: in the refusal condition at pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs:815-824, add a non-empty cell-store journal check; in the acceptance condition at pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs:850-855, require that journal to be empty. Update the relevant selector logic without changing the other guard conditions.
931-939: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude
cell_store_lenin the refusal diagnostic output.The diagnostic output lists the evaluated criteria but misses
cell_store_len, which is part of the refusal condition. Including it ensures the log fully reflects the constraints.♻️ Proposed refactor
- eprintln!( - "[fbw-foriter] deliver REFUSED (body effect committed since consume) body_pc={} \ - body_effect={body_effect} store_journal_len={store_len} \ - append_journal_len={append_len} unjournaled={unjournaled} \ - — keeping legacy drop-on-abort to avoid a double-apply (R1)", - body_pc - ); + eprintln!( + "[fbw-foriter] deliver REFUSED (body effect committed since consume) body_pc={} \ + body_effect={body_effect} store_journal_len={store_len} \ + append_journal_len={append_len} cell_store_len={cell_store_len} \ + unjournaled={unjournaled} — keeping legacy drop-on-abort to avoid a double-apply (R1)", + body_pc + );🤖 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/fbw_state.rs` around lines 931 - 939, Update the refusal diagnostic in the fbw-foriter delivery path to include the cell_store_len value alongside the existing body_pc, body_effect, store_journal_len, append_journal_len, and unjournaled fields. Use the existing cell_store_len symbol from the refusal condition and preserve the current message and behavior.
🤖 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/src/jit/codewriter.rs`:
- Around line 9787-9823: The catch-splitting sequence is duplicated between the
UNPACK_SEQUENCE site at pyre/pyre-jit/src/jit/codewriter.rs lines 9787-9823 and
the UnpackEx site at lines 11180-11208. Extract the shared temporary-FlowValue
and catch_label block-splitting logic into a helper, then replace both inline
implementations with calls to that helper while preserving exception-edge
handling, state linking, block ordering, and temporary stack cleanup.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs`:
- Around line 815-824: Both R1 never-double resume-delivery selectors must
account for FBW_CELL_STORE_JOURNAL: in the refusal condition at
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs:815-824, add a non-empty
cell-store journal check; in the acceptance condition at
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs:850-855, require that
journal to be empty. Update the relevant selector logic without changing the
other guard conditions.
- Around line 931-939: Update the refusal diagnostic in the fbw-foriter delivery
path to include the cell_store_len value alongside the existing body_pc,
body_effect, store_journal_len, append_journal_len, and unjournaled fields. Use
the existing cell_store_len symbol from the refusal condition and preserve the
current message and behavior.
🪄 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
Run ID: c31ccd9f-161f-478a-9c30-597e06fba442
📒 Files selected for processing (31)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-metainterp/src/jitcode/assembler.rspyre/bench/synth/break_except_live_local.pypyre/bench/synth/callee_store_global_read_after_call.pypyre/bench/synth/delete_negative_open_slice_hot.pypyre/bench/synth/for_iter_conditional_store_bridge.pypyre/bench/synth/for_iter_select_receiver_swap.pypyre/bench/synth/inline_bignum_bridge_twoclamp.pypyre/bench/synth/inlined_callee_extended_arg_handler.pypyre/bench/synth/jit_callee_raised_exc_value.pypyre/bench/synth/jit_reg_const_pool_256_slot_decline.pypyre/bench/synth/match_sequence_of_class_patterns.pypyre/bench/synth/polymorphic_binary_receiver.pypyre/bench/synth/selfrec_tail_exception_unwind.pypyre/bench/synth/unpack_wrong_arity_caught.pypyre/pyre-jit-trace/src/callbacks.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/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/jitcode_dispatch/vstack_mirror.rspyre/pyre-jit-trace/src/liveness.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/assembler.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-jit/src/jit/flatten.rspyre/pyre-jit/src/jit/ssa_emitter.rs
| // This opcode lowers to more than one residual call, | ||
| // but only the validator can raise for a validated | ||
| // built-in sequence. Its trailing `-live-` must be | ||
| // immediately followed by `catch_exception`: a | ||
| // specialization guard can resume at this call and | ||
| // discover a different runtime arity. Split the | ||
| // successful continuation before emitting the item | ||
| // reads so blackhole exception dispatch sees the | ||
| // validator's own exception edge. | ||
| if let Some(catch_label) = catch_for_pc[py_pc] { | ||
| emit_catch_exception!(catch_label); | ||
| exception_edge_handled = true; | ||
|
|
||
| // Carry the normalized tuple across the graph | ||
| // edge as a temporary FrameState value. It is | ||
| // not part of the Python operand stack, so remove | ||
| // it again as soon as the continuation starts. | ||
| current_state.stack.push(tuple_value.clone()); | ||
| let mut next_state = current_state.clone(); | ||
| next_state.next_offset = py_pc; | ||
| next_state.blocklist = frame_blocks_for_offset(code, py_pc); | ||
| let next_block = SpamBlockRef::new( | ||
| graph.new_block(Vec::new()), | ||
| Some(next_state.clone()), | ||
| ); | ||
| all_walker_blocks.push(next_block.clone()); | ||
| next_block.block().borrow_mut().inputargs = | ||
| next_state.getvariables(); | ||
| append_exit( | ||
| ¤t_block.block(), | ||
| output_link(¤t_state, &next_state, next_block.block()), | ||
| ); | ||
| restore_canraise_exit_order(¤t_block.block()); | ||
| current_block = next_block; | ||
| current_state = next_state; | ||
| current_state.stack.pop(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate catch-splitting logic between UNPACK_SEQUENCE and UnpackEx. Both sites implement the same "split the validator's successful continuation onto a fresh block while threading one temporary tuple value across the edge" pattern almost line-for-line, differing only in the opcode-specific residual call feeding tuple_value.
pyre/pyre-jit/src/jit/codewriter.rs#L9787-L9823: extract this block-splitting logic (push temp value → clone state → new block → wire exit link → restore order → pop temp) into a shared helper parameterized by the temporaryFlowValueandcatch_label.pyre/pyre-jit/src/jit/codewriter.rs#L11180-L11208: call the same shared helper instead of re-implementing the identical sequence forUnpackEx.
📍 Affects 1 file
pyre/pyre-jit/src/jit/codewriter.rs#L9787-L9823(this comment)pyre/pyre-jit/src/jit/codewriter.rs#L11180-L11208
🤖 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/src/jit/codewriter.rs` around lines 9787 - 9823, The
catch-splitting sequence is duplicated between the UNPACK_SEQUENCE site at
pyre/pyre-jit/src/jit/codewriter.rs lines 9787-9823 and the UnpackEx site at
lines 11180-11208. Extract the shared temporary-FlowValue and catch_label
block-splitting logic into a helper, then replace both inline implementations
with calls to that helper while preserving exception-edge handling, state
linking, block ordering, and temporary stack cleanup.
Summary
Fixes a silent JIT-codegen crash: the trace codewriter aborted compilation with
Result::unwrap() on Err(InvalidBytecode)whenever an inlined callee'sexception handler contained a jump large enough to need
EXTENDED_ARG.transform_graph_to_jitcodedequeues basic blocks in a non-sequential order butcarried a single running
OpArgStateacross the whole walk.OpArgState::getfolds an
EXTENDED_ARG's high bits and resets only after a non-ExtendedArgop — correct for a sequential stream, but a block that ends on an
EXTENDED_ARGcode unit leaves the accumulator non-zero, and the next dequeuedblock's first instruction then folds those stale high bits into its own
argument. Observed:
EXTENDED_ARG(1)at one pc, then aCOMPARE_OP 148at thenext-visited pc decoded as
(1 << 8) | 148 = 404, whichArg<ComparisonOperator>::try_fromrejects.The fix resets the accumulator at each block entry. Within a block the walk is
sequential (
start_pc..num_instrs) and every branch/handler target lands on aninstruction start (its
EXTENDED_ARGprefix included), so in-block prefixes stayintact while the cross-block leak is cut. The adjacent
block_switch_pendingreset already guards the same "previous block's state bleeds into this one" bug
class.
Found by a randomized compositional differential fuzzer (
jit != nojiton thesame binary). Affects both backends (the bug is in the shared codewriter).
Test
jit == nojit, both dynasm and cranelift.0 divergent.
pyre/check.py: 226/226 on both backends (includes the new synth regressionbench
inlined_callee_extended_arg_handler.py).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests