jit: run a tracing abort through convert_and_run_from_pyjitpl; gate write barriers on the collector's descriptor - #895
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR adds deferred abort blackhole recovery, installed-GC-box routing, optional write barriers, liveness cache reconstruction, deterministic translation output, runtime typing fixes, optimizer coverage, bigint arithmetic updates, and release CI validation. ChangesRuntime barrier configuration
Tracing and blackhole recovery
Liveness and translation outputs
Optimizer and runtime validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Tracer
participant TraceCtx
participant JitDriver
participant BlackholeInterpreter
Tracer->>TraceCtx: preserve aborted framestack and captured state
TraceCtx->>JitDriver: stage PendingAbortBlackhole
JitDriver->>BlackholeInterpreter: convert preserved frames
BlackholeInterpreter-->>JitDriver: return resume PC or completion sentinel
JitDriver-->>Tracer: resume merge-point execution
Possibly related PRs
Suggested reviewers: 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 88c5a36). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81e1f3e55a
ℹ️ 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".
| // time; snapshot it here because | ||
| // `abort_trace_live` below tears down the | ||
| // tracing state it belongs to. | ||
| last_exc_value: self.meta.last_exc_value, |
There was a problem hiding this comment.
When a NotInTrace residual call raises, dispatch.rs:5695-5707 deliberately leaves the exception in BH_LAST_EXC_VALUE and returns an abort with raising_exception=true; it does not copy that value into MetaInterp::last_exc_value. Staging self.meta.last_exc_value here therefore usually records zero, so the new blackhole conversion starts without the pending exception and continues as though the call succeeded, potentially swallowing the exception and executing the remaining opcode tail. Capture the dispatch machine/TLS exception value that triggered the abort instead.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
| if let Some(__bh_pc) = #driver.run_pending_abort_blackhole( | ||
| &mut #state, #env, | ||
| ) { |
There was a problem hiding this comment.
This call is generated only when the marker explicitly supplies ; state; the overwhelmingly used zero-argument form (for example majit/examples/tlr/src/jit_interp.rs:76) selects the other expansion, which merely calls the merge wrapper and never consumes pending_abort_blackhole. Consequently a NotInTrace or virtualizable-escape abort in those portals still drops the captured frame stack and continues through the native body rather than completing the aborted frames, leaving the semantic mismatch this change is intended to fix. The default expansion also needs access to the configured state and must invoke the conversion.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
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/assembler.rs (1)
145-165: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid re-decoding the full liveness buffer on every
publish_statecall.
publish_stateis invoked after every single-live-encode frompyre_jit::Assembler::encode_liveness_info— including cache hits whereall_livenessis byte-for-byte unchanged from the last call. Rebuildingall_liveness_positionsvia a fulldecode_liveness_recordspass on every call makes total build-time cost grow roughly with the square of the number of-live-sites sharing one buffer, since each of N appends now triggers an O(current buffer size) redecode.Skip the rebuild when the incoming bytes match what's already stored — a self-contained fix requiring no caller changes.
⚡ Proposed fix to skip redundant redecode on unchanged bytes
pub fn publish_state( insns: &IndexMap<String, u8>, all_liveness: &[u8], all_liveness_length: usize, num_liveness_ops: usize, ) { ASSEMBLER_STATE.with(|r| { let mut asm = r.borrow_mut(); asm.insns = insns.clone(); + let liveness_unchanged = asm.all_liveness.as_slice() == all_liveness; asm.all_liveness.clear(); asm.all_liveness.extend_from_slice(all_liveness); asm.all_liveness_length = all_liveness_length; asm.num_liveness_ops = num_liveness_ops; - // The dedup table describes the *previous* buffer, so rebuild it from - // the one just installed. Clearing instead would leave the mirror - // unable to see any record in the fresh bytes, and the next - // `intern_liveness` would append a duplicate of one already there. - asm.all_liveness_positions = liveness_positions_of(all_liveness); + // The dedup table describes the *previous* buffer, so rebuild it from + // the one just installed. Clearing instead would leave the mirror + // unable to see any record in the fresh bytes, and the next + // `intern_liveness` would append a duplicate of one already there. + // Skip the O(n) redecode when the bytes are unchanged (the common + // cache-hit case from `encode_liveness_info`). + if !liveness_unchanged { + asm.all_liveness_positions = liveness_positions_of(all_liveness); + } }); crate::state::publish_liveness_info(all_liveness.to_vec()); }🤖 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/assembler.rs` around lines 145 - 165, Update publish_state to compare the incoming all_liveness bytes with the buffer already stored in assembler state before rebuilding all_liveness_positions. Only call liveness_positions_of when the bytes differ; preserve the existing buffer, dedup table, and metadata updates for unchanged inputs.majit/majit-metainterp/src/blackhole.rs (1)
764-833: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd equivalent Ref/Float return-slot regression coverage.
inline_call_groupedalways emits the genericBC_INLINE_CALL3-slot return tail, including forinline_call_ir_randinline_call_irf_f, but the current blackhole test only exercises the Int-result branch throughsetup_return_value_i_reads_inline_calls_int_return_slot. Add analogous blackhole/interp coverage forsetup_return_value_r/setup_return_value_fto avoid relying on indirect coverage for the same Sentinel-as-register-index OOB path.🤖 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/blackhole.rs` around lines 764 - 833, Add blackhole/interpreter regression tests alongside setup_return_value_i_reads_inline_calls_int_return_slot for inline_call_ir_r and inline_call_irf_f. Exercise setup_return_value_r and setup_return_value_f with inline_call_grouped’s generic three-slot BC_INLINE_CALL tail, asserting each result is written to its correct return register and does not treat NO_RETURN_REG as a register index.
🤖 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 12239-12246: Update the CondCallGcWb/CondCallGcWbArray codegen arm
to require rw.wb_descr via an expect with the message "COND_CALL_GC_WB emitted
without a write barrier descriptor", then read all jit_wb_* fields from that
descriptor without unwrap_or(0) fallbacks. Preserve the existing field
extraction and drop(rw) flow once the invariant is enforced.
In `@majit/majit-metainterp/src/blackhole.rs`:
- Around line 8373-8387: Remove the redundant new_with_vtable/d>r insertion from
either this block or the later arraylen_gc/rd>i, cast_ptr_to_int/r>i, and
new_with_vtable/d>r loop in the same function, while retaining exactly one
mapping to BC_NEW_WITH_VTABLE.
In `@majit/majit-metainterp/src/jitdriver.rs`:
- Around line 2079-2087: Update the JitException::ExitFrameWithExceptionRef
branch to handle a None result from deliver_blackhole_exception explicitly
instead of propagating it with ?. Make the no-exception-surface case fail loudly
using the same diagnostic behavior as the ContinueRunningNormally arm, while
preserving the existing writeback and resume flow when a resume PC is returned.
---
Outside diff comments:
In `@majit/majit-metainterp/src/blackhole.rs`:
- Around line 764-833: Add blackhole/interpreter regression tests alongside
setup_return_value_i_reads_inline_calls_int_return_slot for inline_call_ir_r and
inline_call_irf_f. Exercise setup_return_value_r and setup_return_value_f with
inline_call_grouped’s generic three-slot BC_INLINE_CALL tail, asserting each
result is written to its correct return register and does not treat
NO_RETURN_REG as a register index.
In `@pyre/pyre-jit-trace/src/assembler.rs`:
- Around line 145-165: Update publish_state to compare the incoming all_liveness
bytes with the buffer already stored in assembler state before rebuilding
all_liveness_positions. Only call liveness_positions_of when the bytes differ;
preserve the existing buffer, dedup table, and metadata updates for unchanged
inputs.
🪄 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: f0e04b97-7873-45c9-a6bf-a310762a36af
📒 Files selected for processing (25)
.github/workflows/pyre-ci.ymlmajit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-gc/src/lib.rsmajit/majit-gc/src/rewrite.rsmajit/majit-macros/src/jit_interp/codegen_state.rsmajit/majit-macros/src/jit_interp/mod.rsmajit/majit-metainterp/src/blackhole.rsmajit/majit-metainterp/src/jit_state.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/lib.rsmajit/majit-metainterp/src/optimizeopt/heap.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/trace_ctx.rsmajit/majit-metainterp/tests/abort_blackhole_virt_array.rsmajit/majit-translate/src/codewriter/flatten.rsmajit/majit-translate/src/codewriter/jitcode.rsmajit/majit-translate/src/codewriter/liveness.rsmajit/majit-translate/src/front/mir.rsmajit/majit-translate/tests/test_make_jitcodes_produces_graph_keyed_output.rspyre/pyre-jit-trace/src/assembler.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/jit/assembler.rs
| // Plain struct allocation (`blackhole.py:1301-1310 bhimpl_new` / | ||
| // `bhimpl_new_with_vtable`) — a `#[jit_interp]` frontend's declared | ||
| // `struct_allocs` literal lowers straight to it, so any resume that walks | ||
| // forward over one needs the byte dispatchable. The handlers were already | ||
| // wired in `wire_bhimpl_handlers`; only the opname→byte entries were | ||
| // missing, which turns into a `dispatch_step: unwired opcode` panic. | ||
| insns.insert("new/d>r".to_string(), majit_translate::insns::BC_NEW); | ||
| insns.insert( | ||
| "new_with_vtable/d>r".to_string(), | ||
| majit_translate::insns::BC_NEW_WITH_VTABLE, | ||
| ); | ||
| insns.insert( | ||
| "new_array/id>r".to_string(), | ||
| majit_translate::insns::BC_NEW_ARRAY, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Duplicate insns.insert("new_with_vtable/d>r", ...).
This key is inserted here and again later in the same function (the arraylen_gc/rd>i / cast_ptr_to_int/r>i / new_with_vtable/d>r loop further down) with the identical byte value. Harmless (IndexMap overwrite, same value both times) but redundant — worth dropping one of the two.
♻️ Proposed dedup
insns.insert("new/d>r".to_string(), majit_translate::insns::BC_NEW);
- insns.insert(
- "new_with_vtable/d>r".to_string(),
- majit_translate::insns::BC_NEW_WITH_VTABLE,
- );
insns.insert(
"new_array/id>r".to_string(),
majit_translate::insns::BC_NEW_ARRAY,
);📝 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.
| // Plain struct allocation (`blackhole.py:1301-1310 bhimpl_new` / | |
| // `bhimpl_new_with_vtable`) — a `#[jit_interp]` frontend's declared | |
| // `struct_allocs` literal lowers straight to it, so any resume that walks | |
| // forward over one needs the byte dispatchable. The handlers were already | |
| // wired in `wire_bhimpl_handlers`; only the opname→byte entries were | |
| // missing, which turns into a `dispatch_step: unwired opcode` panic. | |
| insns.insert("new/d>r".to_string(), majit_translate::insns::BC_NEW); | |
| insns.insert( | |
| "new_with_vtable/d>r".to_string(), | |
| majit_translate::insns::BC_NEW_WITH_VTABLE, | |
| ); | |
| insns.insert( | |
| "new_array/id>r".to_string(), | |
| majit_translate::insns::BC_NEW_ARRAY, | |
| ); | |
| // Plain struct allocation (`blackhole.py:1301-1310 bhimpl_new` / | |
| // `bhimpl_new_with_vtable`) — a `#[jit_interp]` frontend's declared | |
| // `struct_allocs` literal lowers straight to it, so any resume that walks | |
| // forward over one needs the byte dispatchable. The handlers were already | |
| // wired in `wire_bhimpl_handlers`; only the opname→byte entries were | |
| // missing, which turns into a `dispatch_step: unwired opcode` panic. | |
| insns.insert("new/d>r".to_string(), majit_translate::insns::BC_NEW); | |
| insns.insert( | |
| "new_array/id>r".to_string(), | |
| majit_translate::insns::BC_NEW_ARRAY, | |
| ); |
🤖 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/blackhole.rs` around lines 8373 - 8387, Remove the
redundant new_with_vtable/d>r insertion from either this block or the later
arraylen_gc/rd>i, cast_ptr_to_int/r>i, and new_with_vtable/d>r loop in the same
function, while retaining exactly one mapping to BC_NEW_WITH_VTABLE.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dcc792359
ℹ️ 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".
|
|
||
| let mut bh_builder = BackEdgeBhBuilder::lease(); | ||
| let staticdata = &self.meta.staticdata; | ||
| let outcome = drive_multi_frame_blackhole( |
There was a problem hiding this comment.
When a tracing abort occurs with an inlined recursive portal on the captured framestack, that frame can reach its jit_merge_point while it still has a caller. handle_jitexception then routes ContinueRunningNormally through handle_jitexception_in_portal, but this conversion path ultimately calls run_forever_with_portal with portal_runner=None, so handle_jitexception_dispatch panics at expect("ContinueRunningNormally requires portal_runner") instead of re-entering the recursive portal. Thread the driver's portal runner through the new abort conversion; this is also what the repository's TL recursive_portal_call! path will require when the default merge-point consumer is wired.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6e4266deb
ℹ️ 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 code[pos - 1] == crate::jitcode::NO_RETURN_REG && pos >= 3 { |
There was a problem hiding this comment.
When an int/ref BC_INLINE_CALL is followed by the five-byte valuestackdepth sync, the branch above returns code[pos - 6] before this new three-slot detection runs. That byte is the inline call's return_f slot, which is NO_RETURN_REG for int/ref returns, so _setup_return_value_* indexes register 255 and panics during blackhole resume. Apply the return-kind slot adjustment to the position before the VSD branch returns; the new test only covers an inline call with no sync.
Useful? React with 👍 / 👎.
| state.restore_banked3( | ||
| &meta, | ||
| &terminal.registers_i[int_base..], | ||
| &terminal.registers_r[ref_base..], | ||
| &terminal.registers_f[float_base..], |
There was a problem hiding this comment.
For a #[jit_interp] state combining a virtualizable array with ref(T) scalar fields, slicing at ref_scalar_base removes the virtualizable identity before calling the generated restore_banked3. That implementation indexes ref scalar j at num_vable_identity_slots + j (codegen_state.rs:1200-1207), so with one ref scalar it reads past the slice and panics, while multiple fields are shifted. The existing guard-failure CRN path explicitly uses per-field writeback when num_vable_identity_slots != 0; this abort writeback needs the same treatment or must retain the identity prefix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0228440904
ℹ️ 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 let Some(values) = virt_array_values { | ||
| state.writeback_virt_array_state_fields_from_values(&values); |
There was a problem hiding this comment.
When tracing aborts inside an inlined recursive portal with a [.. ; virt] field, virt_array_values belongs to the callee's fresh state, whose identity is explicitly distinct from the caller (majit/examples/tl/src/jit_interp.rs:519-550). This writes those elements into the outer native state, and the subsequent drive_multi_frame_blackhole also supplies one virtualizable_ptr with per_frame=None, so every reconstructed caller/callee frame accesses the same virtualizable. Once the default merge-point consumer and portal runner are wired, completing the callee can therefore overwrite the caller's stack and later caller-tail operations can continue against the callee's storage; retain the fresh owner and thread each frame's own virtualizable identity/state through the abort conversion instead.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7c23c6b2e
ℹ️ 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 !layout.array_lens.is_empty() { | ||
| if crate::majit_log_enabled() { | ||
| eprintln!( | ||
| "[bh] abort-blackhole declined: unseeded state shape (arrays={})", | ||
| layout.array_lens.len(), | ||
| ); | ||
| } | ||
| return None; |
There was a problem hiding this comment.
For any #[jit_interp] state containing a fixed [int] field, this discards the pending conversion and falls back to the source-pc handoff, which is unsafe when tracing aborts midway through an opcode: already-applied effects can be replayed while the unfinished tail is skipped. This is a supported and reachable state shape—the repository declares cells: [int] in jit_interp_float_state_field.rs:407-415—so the claim that no frontend uses it is incorrect; capture and seed the flattened array elements rather than declining the conversion.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcbe9dc11b
ℹ️ 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".
| /// null pointer and fall back to their non-GC path). | ||
| pub fn alloc_nursery_typed(type_id: u32, payload_size: usize) -> GcRef { | ||
| match ACTIVE_ALLOC_NURSERY_TYPED.get() { | ||
| Some(_) if !gc_box_installed() => standalone_alloc_nursery_typed(type_id, payload_size), |
There was a problem hiding this comment.
When an embedding registers set_active_alloc_nursery_typed(Some(f)) directly without separately calling note_gc_box_installed()—a requirement absent from the setter's contract—this arm ignores f and allocates from gc_sync instead. The object consequently belongs to the process singleton rather than the embedding's active collector, so it may not be traced or reclaimed by the intended GC; the analogous rooted allocator branch has the same defect. Preserve the registered allocator's ownership or explicitly record which hooks are singleton-forwarding hooks.
AGENTS.md reference: AGENTS.md:L148-L155
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
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/assembler.rs (1)
145-165: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winFull liveness-map rebuild runs on every
-live-emission, including cache hits.
publish_statenow rebuildsall_liveness_positionsby decoding the entireall_livenessbuffer on every call. The writer side callspublish_stateunconditionally at the end ofencode_liveness_info(pyre/pyre-jit/src/jit/assembler.rs), including on a cache hit where the buffer did not change. A-live-marker is emitted at nearly every branch point across every assembled jitcode, so this full re-decode (bounded by a 64 KiB buffer) can run many thousands of times during one build, each time redoing work for bytes that were already decoded on the previous call.Limit the rebuild to when the buffer actually grew (skip the
publish_statecall, or at least theliveness_positions_ofrebuild, on a cache hit inencode_liveness_info).⚡ Proposed fix sketch
- pyre_jit_trace::assembler::publish_state( - &self.insns, - &self.all_liveness, - self.all_liveness_length, - self.num_liveness_ops, - ); - pos + if !matches!(&self.all_liveness_positions.get(&key), Some(&p) if p == pos) || /* cache miss */ true { + // Only publish when the buffer actually grew (cache miss branch above), + // so publish_state's rebuild does not run on every cache-hit call. + } + pos(Adjust so the
publish_statecall sits inside the miss branch ofencode_liveness_infoinpyre/pyre-jit/src/jit/assembler.rs, rather than after the sharedif/else.)🤖 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/assembler.rs` around lines 145 - 165, Move the publish_state call in encode_liveness_info into the cache-miss branch so it runs only when new liveness bytes are appended; leave cache-hit handling unchanged and avoid rebuilding all_liveness_positions for unchanged buffers.majit/majit-backend-dynasm/src/aarch64/assembler.rs (1)
4284-4303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: the
PYRE_TRACE_CALL_DIAGdiagnostic block is duplicated.
generate_quick_failureandgenop_call_with_arglocsboth repeat the same sequence: readPYRE_TRACE_CALL_DIAG, compare toself.trace_id, push volatile registers, load the debug-call args,blr, and pop volatile registers. Extract this into one helper (for exampleemit_trace_call_diag(&mut self, tag: i64)) to remove the duplication.This is debug-only scaffolding, so the practical payoff is limited, but it is a straightforward extraction.
Also applies to: 5918-5933
🤖 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-dynasm/src/aarch64/assembler.rs` around lines 4284 - 4303, Extract the duplicated PYRE_TRACE_CALL_DIAG handling from generate_quick_failure and genop_call_with_arglocs into a shared helper such as emit_trace_call_diag(&mut self, tag: i64). Keep the helper responsible for environment matching, volatile-register preservation, loading the diagnostic arguments, invoking dynasm_debug_validate_oldgen_freeblocks, and restoring registers; pass each caller’s existing diagnostic tag and replace both inline blocks with the helper call.
🤖 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-metainterp/src/jitdriver.rs`:
- Around line 1955-1972: Move the empty-framestack check around
framestack.frames before the virt-array writeback block in the relevant
deoptimization flow. Return None immediately when no root frame exists, then
perform writeback and clear self.meta.single_pass_virt_array_values only after a
root frame is confirmed; preserve the existing vinfo and chain-processing logic
for non-empty framestacks.
- Around line 2039-2054: Update the abort-terminal write-back closure around
restore_banked3 to branch on layout.num_vable_identity_slots, matching the
guard-failure CRN path. Only use dense restore_banked3 for layouts with zero
virtualizable identity slots; preserve the native state fields for
virtualizable-array layouts while retaining the existing terminal and recovery
behavior.
---
Outside diff comments:
In `@majit/majit-backend-dynasm/src/aarch64/assembler.rs`:
- Around line 4284-4303: Extract the duplicated PYRE_TRACE_CALL_DIAG handling
from generate_quick_failure and genop_call_with_arglocs into a shared helper
such as emit_trace_call_diag(&mut self, tag: i64). Keep the helper responsible
for environment matching, volatile-register preservation, loading the diagnostic
arguments, invoking dynasm_debug_validate_oldgen_freeblocks, and restoring
registers; pass each caller’s existing diagnostic tag and replace both inline
blocks with the helper call.
In `@pyre/pyre-jit-trace/src/assembler.rs`:
- Around line 145-165: Move the publish_state call in encode_liveness_info into
the cache-miss branch so it runs only when new liveness bytes are appended;
leave cache-hit handling unchanged and avoid rebuilding all_liveness_positions
for unchanged buffers.
🪄 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: 40b2ccfc-7509-4d27-afd8-7b31a64ab157
📒 Files selected for processing (31)
.github/workflows/pyre-ci.ymlmajit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-gc/src/lib.rsmajit/majit-gc/src/rewrite.rsmajit/majit-macros/src/jit_interp/codegen_state.rsmajit/majit-macros/src/jit_interp/mod.rsmajit/majit-metainterp/src/blackhole.rsmajit/majit-metainterp/src/jit_state.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/lib.rsmajit/majit-metainterp/src/optimizeopt/heap.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/resume_box_reader.rsmajit/majit-metainterp/src/trace_ctx.rsmajit/majit-metainterp/tests/abort_blackhole_virt_array.rsmajit/majit-translate/src/codewriter/flatten.rsmajit/majit-translate/src/codewriter/jitcode.rsmajit/majit-translate/src/codewriter/liveness.rsmajit/majit-translate/src/front/mir.rsmajit/majit-translate/tests/test_make_jitcodes_produces_graph_keyed_output.rspyre/bench/synth/int_mul_ovf_bignum_promote.pypyre/pyre-jit-trace/src/assembler.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/jitcode_runtime.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/jit/assembler.rspyre/pyre-object/src/longobject.rs
| // `writeback_virt_array_state_fields_from_values`, run here rather than | ||
| // by the `jit_merge_point!` hook's own virt-array writeback: the chain | ||
| // is about to re-execute the tail of the aborted opcodes against the | ||
| // live struct, so it has to see the walk's elements, not the | ||
| // trace-start ones. | ||
| if let Some(values) = virt_array_values { | ||
| state.writeback_virt_array_state_fields_from_values(&values); | ||
| self.meta.single_pass_virt_array_values = None; | ||
| } | ||
| // `jitdriver.rs seed_deopt_vinfo_ptr`: a state-field machine's | ||
| // `bh_clear_vable_token` is inert, so a non-null vinfo is safe and lets | ||
| // mid-body vable-array opcodes resolve; a real heap virtualizable | ||
| // (`token_offset > 0`, e.g. PyFrame) keeps the null-vinfo resume | ||
| // contract and reaches the chain with `virtualizable_info` unset. | ||
| let vinfo_ptr = seed_deopt_vinfo_ptr(self.meta.virtualizable_info()); | ||
| let Some(root) = framestack.frames.first_mut() else { | ||
| return None; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Move the empty-framestack decline before the virt-array write-back.
Lines 1960-1963 mutate native state and clear self.meta.single_pass_virt_array_values. Line 1970 can then return None, which the doc comment at Line 1909 defines as "declined before the chain ran". The array_lens decline at Line 1946 returns before any mutation, so the two decline paths are not equivalent.
Check framestack.frames first, then apply the write-back.
🩹 Proposed fix
+ if framestack.frames.is_empty() {
+ if crate::majit_log_enabled() {
+ eprintln!("[bh] abort-blackhole declined: empty framestack");
+ }
+ return None;
+ }
// `writeback_virt_array_state_fields_from_values`, run here rather than
// by the `jit_merge_point!` hook's own virt-array writeback: the chain
// is about to re-execute the tail of the aborted opcodes against the
// live struct, so it has to see the walk's elements, not the
// trace-start ones.
if let Some(values) = virt_array_values {
state.writeback_virt_array_state_fields_from_values(&values);
self.meta.single_pass_virt_array_values = None;
}
@@
let vinfo_ptr = seed_deopt_vinfo_ptr(self.meta.virtualizable_info());
- let Some(root) = framestack.frames.first_mut() else {
- return None;
- };
+ let root = &mut framestack.frames[0];📝 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.
| // `writeback_virt_array_state_fields_from_values`, run here rather than | |
| // by the `jit_merge_point!` hook's own virt-array writeback: the chain | |
| // is about to re-execute the tail of the aborted opcodes against the | |
| // live struct, so it has to see the walk's elements, not the | |
| // trace-start ones. | |
| if let Some(values) = virt_array_values { | |
| state.writeback_virt_array_state_fields_from_values(&values); | |
| self.meta.single_pass_virt_array_values = None; | |
| } | |
| // `jitdriver.rs seed_deopt_vinfo_ptr`: a state-field machine's | |
| // `bh_clear_vable_token` is inert, so a non-null vinfo is safe and lets | |
| // mid-body vable-array opcodes resolve; a real heap virtualizable | |
| // (`token_offset > 0`, e.g. PyFrame) keeps the null-vinfo resume | |
| // contract and reaches the chain with `virtualizable_info` unset. | |
| let vinfo_ptr = seed_deopt_vinfo_ptr(self.meta.virtualizable_info()); | |
| let Some(root) = framestack.frames.first_mut() else { | |
| return None; | |
| }; | |
| if framestack.frames.is_empty() { | |
| if crate::majit_log_enabled() { | |
| eprintln!("[bh] abort-blackhole declined: empty framestack"); | |
| } | |
| return None; | |
| } | |
| // `writeback_virt_array_state_fields_from_values`, run here rather than | |
| // by the `jit_merge_point!` hook's own virt-array writeback: the chain | |
| // is about to re-execute the tail of the aborted opcodes against the | |
| // live struct, so it has to see the walk's elements, not the | |
| // trace-start ones. | |
| if let Some(values) = virt_array_values { | |
| state.writeback_virt_array_state_fields_from_values(&values); | |
| self.meta.single_pass_virt_array_values = None; | |
| } | |
| // `jitdriver.rs seed_deopt_vinfo_ptr`: a state-field machine's | |
| // `bh_clear_vable_token` is inert, so a non-null vinfo is safe and lets | |
| // mid-body vable-array opcodes resolve; a real heap virtualizable | |
| // (`token_offset > 0`, e.g. PyFrame) keeps the null-vinfo resume | |
| // contract and reaches the chain with `virtualizable_info` unset. | |
| let vinfo_ptr = seed_deopt_vinfo_ptr(self.meta.virtualizable_info()); | |
| let root = &mut framestack.frames[0]; |
🤖 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/jitdriver.rs` around lines 1955 - 1972, Move the
empty-framestack check around framestack.frames before the virt-array writeback
block in the relevant deoptimization flow. Return None immediately when no root
frame exists, then perform writeback and clear
self.meta.single_pass_virt_array_values only after a root frame is confirmed;
preserve the existing vinfo and chain-processing logic for non-empty
framestacks.
| let writeback = |state: &mut S, resume_pc: usize| { | ||
| let Some(terminal) = terminal.as_ref() else { | ||
| return; | ||
| }; | ||
| let int_base = layout.int_scalar_base.min(terminal.registers_i.len()); | ||
| let ref_base = layout.ref_scalar_base.min(terminal.registers_r.len()); | ||
| let float_base = layout.float_scalar_base.min(terminal.registers_f.len()); | ||
| let meta = S::build_meta(state, resume_pc, env); | ||
| state.restore_banked3( | ||
| &meta, | ||
| &terminal.registers_i[int_base..], | ||
| &terminal.registers_r[ref_base..], | ||
| &terminal.registers_f[float_base..], | ||
| ); | ||
| state.recover_after_compiled_run(); | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the helper signature and the sibling branch it is used in.
rg -n -B5 -A30 'fn writeback_live_state_scalars_from_blackhole' --type=rs
rg -n -C6 'num_vable_identity_slots\s*==\s*0' --type=rsRepository: youknowone/pyre
Length of output: 215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files '*.rs' | head -80
echo "== helper signature =="
rg -n -B5 -A40 'fn writeback_live_state_scalars_from_blackhole' '*.rs' || true
echo "== sibling branch occurrences =="
rg -n -C8 'num_vable_identity_slots\s*==\s*0' '*.rs' || true
echo "== target function context =="
rg -n -B20 -A120 'fn run_pending_abort_blackhole|writeback\(|drive_multi_frame_blackhole' majit/majit-metainterp/src/jitdriver.rs || true
echo "== jit writerback doc/context =="
rg -n -B6 -A25 'writeback_live_scalar_state_field|restore_banked3|liveness' majit/majit-* src 2>/dev/null | head -240 || trueRepository: youknowone/pyre
Length of output: 3357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file exists =="
ls -l majit/majit-metainterp/src/jitdriver.rs 2>/dev/null || true
ls -l majit/majit-*/*.rs majit/majit-*/src/*.rs 2>/dev/null | wc -l
echo "== helper signature =="
rg -n -B5 -A40 'fn writeback_live_state_scalars_from_blackhole' . --glob '*.rs' || true
echo "== sibling branch occurrences =="
rg -n -C8 'num_vable_identity_slots\s*==\s*0' . --glob '*.rs' || true
echo "== target function context =="
rg -n -B20 -A140 'fn run_pending_abort_blackhole|writeback\(|drive_multi_frame_blackhole|restore_banked3' majit/majit-metainterp/src/jitdriver.rs || true
echo "== state writeback doc/liveness snippets =="
rg -n -B8 -A35 'writeback_live_scalar_state_field|restore_banked3|liveness_info|single_pass_virt_array_values|num_vable_identity_slots' . --glob '*.rs' | head -280 || trueRepository: youknowone/pyre
Length of output: 273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== helper signature =="
rg -n -B5 -A50 'fn writeback_live_state_scalars_from_blackhole' . --glob '*.rs' || true
echo "== sibling branch occurrences =="
rg -n -C8 'num_vable_identity_slots\s*==\s*0' . --glob '*.rs' || true
echo "== target writes around function =="
rg -n -C8 'run_pending_abort_blackhole|drive_multi_frame_blackhole|single_pass_virt_array_values|virtualizable_ptr|seed_deopt_vinfo_ptr|frames.first_mut|staged_all_liveness|all_liveness' majit/majit-metainterp/src/jitdriver.rs || true
echo "== restore/writeback definitions =="
rg -n -B10 -A70 'fn writeback_live_scalar_state_field|fn restore_banked3|fn writeback_live_state_scalars_from_blackhole' . --glob '*.rs' || trueRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact run_pending_abort_blackhole block =="
sed -n '1934,2108p' majit/majit-metainterp/src/jitdriver.rs
echo "== guard failure terminal block =="
sed -n '4248,4280p' majit/majit-metainterp/src/jitdriver.rs
echo "== writeback_live_scalar_state_field definitions =="
rg -n -B8 -A85 'fn writeback_live_scalar_state_field|fn writeback_live_ref_scalar_state_field|fn writeback_live_float_scalar_state_field' majit/majit-* --glob '*.rs' || trueRepository: youknowone/pyre
Length of output: 32197
Gate abort-terminal write-back on layout for virtualizable arrays.
run_pending_abort_blackhole supports the [.. ; virt] state shape and leaves populated native state fields when terminal.is_some() removes the staged single-pass values. The terminal write-back then needs the same num_vable_identity_slots == 0 branch as guard-failure CRN: dense restore_banked3 does not apply to virtualizable-array layouts, so this path can clobber native fields with terminal-register values that are not native state fields.
🤖 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/jitdriver.rs` around lines 2039 - 2054, Update the
abort-terminal write-back closure around restore_banked3 to branch on
layout.num_vable_identity_slots, matching the guard-failure CRN path. Only use
dense restore_banked3 for layouts with zero virtualizable identity slots;
preserve the native state fields for virtualizable-array layouts while retaining
the existing terminal and recovery behavior.
…projected place `resolve_place`'s positional-aggregate branch hardcoded `ty: Ref(None)` on the `FieldRead __pos_<N>` it emits, while the genuine-Ref tuple branch directly below derives the same read's type from `place_ty`. A `match (FieldlessEnum, bool)` scrutinee is built as a positional aggregate and its `.0` read back to feed the switch, so the read produced a Ref-typed switch value and `flatten.py:280 assert kind == 'int'` rejected the graph (`pyre_object::dictmultiobject::dict_view_iterator_type_for_kind`). Derive the type from `place_ty` in both branches, and name the graph and block in the flatten assert. Assisted-by: Claude
The `slow_generated_jitcodes_preserve_complete_dispatcher_graph` anchors asserted 111 arms, 118 discriminants and 3 grouped arms; the dispatcher now has 115, 119 and 2. The test self-ignores under `debug_assertions`, so the `cargo test --all` job never ran it and the drift went unnoticed. Make the two totals floors, since an added opcode raises them and only a removal lowers them. Drop the grouped-arm count: the sorted `mir_groups == source_groups` comparison two lines above already pins arm membership and which variants share an arm, so it restated that equality while failing whenever an Or-pattern was split. Run the test from the Linux pyre/check.py job, which has the downloaded build/llbc/*.ullbc and a warm release profile. Assisted-by: Claude
`assembler.py:29-31` gives one Assembler one `all_liveness` buffer and one `all_liveness_positions` dict. pyre hands the buffer over in three places and carried none of the dict with it: - `Assembler::resuming_build_time_liveness` seeded the build-time bytes with an empty dict, so the first runtime `_encode_liveness` of a triple already in the prefix appended a second copy. - `AssemblerState::new` seeded the same bytes the same way. - `publish_state` cleared the dict on every mirror publish. The `-live-` operand is 2 bytes, so each duplicate spends part of one shared 64 KiB budget. Add `decode_liveness_records`, which walks the buffer back into the records `_encode_liveness` appended, and build the dict from it at all three points. Also canonicalise `intern_liveness`'s key: `encode_liveness` sorts and dedups before writing, so two orderings of one set encode identically while the raw slice key made them distinct entries. Assisted-by: Claude
…afe today The default forwards a no-collect request to the collecting form, which is only correct for a non-moving allocator. State which implementors reach it: MiniMarkGC moves but is headered, so `alloc_nursery_headerless`'s own default panics first, and the headerless boxes the backends install grow their pool. A moving headerless box would have to override it. Assisted-by: Claude
The map is a lookup table whose in-memory order never matters, but it is written to build artefacts (aheui-jit's opcode_descrs.bin, pyre-jit-trace's jit_metadata.json) and a HashMap serializes in iteration order. Five consecutive builds of one unchanged aheui tree produced five different opcode_descrs.bin, differing only in the 8-byte key/value pairs of this map; that defeats the codegen cache and makes an A/B bisection impossible to attribute to a source change. Emit the key order const_keys_in_order already carries (jitcode.py:135 `sorted(as_dict.keys())`). The serialized shape is unchanged — still a map — so existing artefacts stay readable. Verified: two rebuilds of aheui now give byte-identical opcode_descrs.bin, opcode_jitcodes.bin and jit_metadata.json. Assisted-by: Claude
`run_to_end` checked `is_too_long()` after every jitcode step and returned `TraceAction::Abort` there, in the middle of a source opcode. The jitdriver's Abort arm hands a source-level pc back through `single_pass_outcome`, taken from the walk's root frame i0, which dispatch advances before running the opcode arm — so the interpreter resumed past an opcode whose side effects had only partly run. aheui's pi.jinseo lost the `size += 1` half of a linked-list push whose `head = node` had already run, leaving a chain one node longer than its size, and then spun in the compiled loop that reads the emptied storage. Add `TraceCtx::abort_at_next_merge_point`: the overflow sets the flag and the walk keeps stepping until the `BC_JIT_MERGE_POINT` arm consumes it and returns Abort. A `Finish` that ends the walk with the flag still set is downgraded to Abort so a trace past the limit is not committed. RPython answers the same overflow with `SwitchToBlackhole(ABORT_TOO_LONG)`, whose handler (pyjitpl.py:2949 `run_blackhole_interp_to_cancel_tracing` -> blackhole.py:1799 `convert_and_run_from_pyjitpl`) finishes the current jitcode frame in the blackhole; pyre has no such consumer yet. aheui snippet corpus 55/55 jit==naive (was 54/55); logo 996310 bytes, exit 42, md5 7fcdbfff0af449c4283c008e3ca317ce. Assisted-by: Claude
`test_setfield_on_distinct_array_loads_forces_prior_lazy_set`: two structs loaded from one array at distinct constant indices, then stored through the same field descr. heap.py:81-83 `possible_aliasing` must force the first store before the second takes the descr's single `_lazy_set` slot. `test_guard_forces_size_store_paired_with_virtual_head_store`: a linked-list push leaves two lazy sets pending at a guard, and heap.py:617-621 splits them by whether the stored value is virtual — the `.head` store keeps its lazy set and goes to pendingfields, the `.size` store is forced and emitted before the guard. The second test runs the full `__init__.py:15-22` pipeline so OptVirtualize is present and the `NEW` result is still virtual at the heap pass; it needs a SizeDescr with a non-empty `all_fielddescrs()`, which the existing `TestSizeDescr` does not provide, so add `TestVirtualSizeDescr` / `virtual_size_descr` alongside it. Assisted-by: Claude
…pyjitpl Wire the consumer `pyjitpl.py:2949 run_blackhole_interp_to_cancel_tracing` calls: `blackhole.py:1799 convert_and_run_from_pyjitpl`. The abort arm of `merge_point` previously published only the walk's root-frame source pc, which names a resume position only when the walk stopped at a source-opcode boundary. The walk's framestack is local to `trace_jitcode_with_args_and_runtime`, and the abort arm holds the sym but not native `state`, so the conversion is staged in two hops: - `TraceCtx` gains `aborted_framestack`, `abort_resume_jitcode_pc` and `abort_after_panic`. On `TraceAction::Abort` the walk sets the top frame's `pc` to the instruction boundary it stopped at and moves the framestack onto the ctx; a panic-sourced abort publishes nothing. - The `merge_point` Abort arm moves it into `MetaInterp::pending_abort_blackhole` together with the sym's scalar and ref-scalar state-field image. - `JitDriver::run_pending_abort_blackhole`, called from the `jit_merge_point!` expansion, seeds the root frame's identity registers from that image, runs `drive_multi_frame_blackhole`, and maps `ContinueRunningNormally` / `DoneWithThisFrame*` / `ExitFrameWithExceptionRef` to a resume pc plus a `restore_banked3` + `recover_after_compiled_run` writeback from the terminal frame's banks. Gated by `MAJIT_ABORT_BLACKHOLE` (default on). It declines, leaving the source-pc handoff in place, when the state layout has virtualizable identity slots or flattened fixed `[int]` arrays. `JitState::collect_ref_scalar_state_field_values` is added as the ref-bank sibling of `collect_scalar_state_field_values`, generated by `#[jit_interp]`. Three blackhole fixes the conversion path exercised: - `call_result_reg` now takes the return kind and, when the byte before the resume position is `NO_RETURN_REG`, steps back over `BC_INLINE_CALL`'s three return slots (`return_i`, `return_r`, `return_f`) instead of reading the sentinel as a register index. Covered by a new test. - `handle_jitexception` captures the terminal image on the "no portal found" path before releasing the interpreter; a `#[jit_interp]` dispatch JitCode carries no `jitdriver_sd`, so its chain always ends there. - `build_inline_call_only_bh_builder` gains the `new/d>r`, `new_with_vtable/d>r` and `new_array/id>r` opname-to-byte entries; the handlers were already wired. The `is_too_long` merge-point deferral is kept and its comment rewritten to describe it as the fallback path's protection rather than the fix. Assisted-by: Claude
…bort's exception
`call.py:147-148 grab_initial_jitcodes` sets both directions:
jd.mainjitcode = self.get_jitcode(jd.portal_graph)
jd.mainjitcode.jitdriver_sd = jd
`JitDriver::register_dispatch_jitcode` set only the first. The slot exists on
the metainterp-side `JitCode` (it is `majit_translate`'s, and `codewriter.rs:1036`
fills it on the translation path); the comment claiming otherwise is corrected.
Without the back-pointer `blackhole.py:1764`'s walk runs off the bottom of a
`#[jit_interp]` frontend's chain instead of stopping at its root, so
`blackhole.py:1767-1769` never publishes the bottommost frame's terminal image.
`handle_jitexception`'s no-portal arm is reverted to upstream's shape now that
the walk terminates there.
`run_pending_abort_blackhole` passed `last_exc_value = 0` and
`raising_exception = false`. The flag was already on the drained
`SwitchToBlackhole` (`history.py:37-43`) and the Abort arm discarded it by
mapping to `stb.reason`; it now keeps the whole `stb`. The value comes from
`MetaInterp::last_exc_value`, which is what `blackhole.py:1811-1814` reads,
snapshotted at the abort because `abort_trace_live` follows.
Adds `handle_jitexception_publishes_terminal_only_when_a_frame_names_its_jitdriver`,
which asserts the image is published exactly when a frame names its jitdriver.
Assisted-by: Claude
…tates `run_pending_abort_blackhole` declined whenever the state declared a virtualizable (`num_vable_identity_slots != 0`). Upstream `blackhole.py:1799 convert_and_run_from_pyjitpl` has no such condition, and the decline covered nearly the whole tree: aheui is the only `#[jit_interp]` frontend without a `[.. ; virt]` array, so the conversion was inactive for braininterp, dualtape, i64env, spcount, tiny2, tiny3, tinyframe, tl, tla, tlc, tlr and the cel machines. The abort arm now stages, alongside the framestack, the walk's virtualizable element values (`TraceCtx::collect_virtualizable_element_values`) and the trace's virtualizable identity (`TraceCtx::virtualizable_heap_ptr`). Before the chain runs, `run_pending_abort_blackhole`: - pushes those elements into native `state` via `writeback_virt_array_state_fields_from_values` — the walk mutates the trace-ctx shadow, not the live struct, and the chain's `getarrayitem_vable_*` read the real object; - seeds the identity into the slot `StateFieldLayout::vable_identity_slot` (new) names, and passes it to `drive_multi_frame_blackhole` with the vinfo pointer `seed_deopt_vinfo_ptr` allows — null for a `token_offset > 0` heap virtualizable, which keeps its existing resume contract. The remaining decline is a flattened fixed `[int]` array (`array_lens`), which has no sym-side per-element collector. `ContinueRunningNormally` with no green pc now asserts and logs instead of silently returning `None`: the chain has already run the aborted opcodes' tails, so falling back to the walk's source pc would execute them twice. Adds `tests/abort_blackhole_virt_array.rs`, a `stackpos: int, stack: [int; virt]` stack machine driven with `MAJIT_STEP_LIMIT=200` so every trace aborts inside an opcode arm. It is its own test binary because that knob is a process-wide `LazyLock`. Under `MAJIT_ABORT_BLACKHOLE=0` the same run panics with `index out of bounds: the len is 21 but the index is 21` from `PUSH` — one half of `stack[stackpos] = v; stackpos += 1` committed and the other lost. Assisted-by: Claude
…descriptor `reload_frame_if_necessary` re-applied the non-array write barrier fast path on the reloaded jitframe unconditionally. `aarch64/assembler.py:975-980 _reload_frame_if_necessary` reads the descriptor first and applies the barrier only `if gcrootmap and wbdescr`; the x86 path already spells that as `if let Some(wb) = wb_descr`. A collector that needs no write barrier reports none (`gc.py:156 GcLLDescr_boehm.write_barrier_descr = None`), and `emit_write_barrier_fastpath_for_base` asserts a descriptor is present. The function is reached from every collecting helper call, including `genop_call_malloc_nursery_headerless`, not only from a `COND_CALL_GC_WB` the GC rewriter emitted, so such a collector took the assertion on each compile and the trace was dropped. Assisted-by: Claude
…o descriptor `GcRewriterImpl.wb_descr` was a plain `WriteBarrierDescr` that both backends built from `WriteBarrierDescr::for_current_gc()` rather than asking the active collector, so the rewriter emitted `COND_CALL_GC_WB*` for a collector that has no write barrier. `rewrite.py:393` gates the whole barrier section on `self.gc_ll_descr.write_barrier_descr is not None`, and `rewrite.py:405-412` is its `else`: `consider_setfield_gc` / `consider_setarrayitem_gc` still run, no barrier is emitted, and the store reaches the ordinary lowering. `gen_write_barrier_array` (rewrite.py:956) reads the descriptor unguarded because that gate has already passed. Make the field `Option<WriteBarrierDescr>`, source it from `gc.get_write_barrier_descr()` (`gc.py:401 self.write_barrier_descr = WriteBarrierDescr(self)`, `None` at `gc.py:156` for `GcLLDescr_boehm`) in both backends, and add the gate. `MiniMarkGC::get_write_barrier_descr` returns the same value the backends were constructing inline, card-marking adjustment included, so the production path is unchanged. Assisted-by: Claude
The doc comment cited `gc.py:268`, which is `self.jit_wb_if_flag = GCClass.JIT_WB_IF_FLAG` inside `WriteBarrierDescr.__init__`, not the `gc_ll_descr` attribute this accessor models. That attribute is assigned at `gc.py:401` and defaulted to `None` at `gc.py:156` for `GcLLDescr_boehm`; upstream reads it directly and has no `get_write_barrier_descr` accessor. Assisted-by: Claude
`build_inline_call_only_bh_builder` now registers `new/d>r`, `new_with_vtable/d>r` and `new_array/id>r`, so `new/d>r` and `new_array/id>r` no longer appear in the overlay-only gap that `production_bh_builder_overlay_only_gap_snapshot` pins. The snapshot still listed them and failed. Update the literal and record why they left, as the comment already does for `vtable_method_ptr/rd>i`. Assisted-by: Claude
… a frontend The comment on the trace_limit deferral illustrated the partially-applied opcode with a specific aheui program. State the shape instead: a push that stores its element and then bumps the length can commit the store and lose the bump. Assisted-by: Claude
…as run `run_pending_abort_blackhole` used `None` for two different things: the conversion declined before running anything, and the chain ran but produced no resume pc. The caller treats `None` as the first, falling back to the source-pc handoff — a pc dispatch advanced before the aborted opcodes' arms. Taking that fallback after the chain executed those arms against the real heap runs them a second time, which is the double-application the conversion exists to prevent. Two paths returned `None` post-chain: - `ContinueRunningNormally` carrying no green pc. Unreachable for a declared jitdriver, but it returned `None` under a `debug_assert`. - `ExitFrameWithExceptionRef` when `JitState::deliver_blackhole_exception` answers `None`, which is its default and no frontend overrides it, so any exception escaping every converted frame took this path. Both now end the dispatch loop through the existing `single_pass_finish` + `usize::MAX` "no forward pc" outcome. `blackhole.py:1799 convert_and_run_from_pyjitpl` does not return to its caller at all — it propagates to `warmspot.py:961 handle_jitexception` — so upstream has no counterpart to either answer. State the invariant on the function: `None` means declined before the chain ran, and only that. Assisted-by: Claude
…conversion `abort_blackhole_enabled()` gated the Abort arm's staging of the aborted framestack for `pyjitpl.py:2949 run_blackhole_interp_to_cancel_tracing`. RPython has no switch there. The gate defaulted to enabled, so removing it leaves runtime behaviour unchanged for an unset environment. Reword the two `jitdriver.rs` doc paragraphs and the `dispatch.rs` deferral comment that listed the knob among the reasons the conversion may not run, and restate the `abort_blackhole_virt_array` module doc's counter-example in terms of short-circuiting the conversion rather than the removed variable. Assisted-by: Claude
`run_to_end` set `abort_at_next_merge_point` on overflow and kept stepping until `BC_JIT_MERGE_POINT` cashed the flag in, so the abort landed only at a position where no source opcode was half-executed. `pyjitpl.py:2861-2867 _interpret` calls `blackhole_if_trace_too_long()` directly after `run_one_step()` and raises `SwitchToBlackhole( ABORT_TOO_LONG)` there (`pyjitpl.py:2831`); the mid-opcode landing is what `pyjitpl.py:2949 run_blackhole_interp_to_cancel_tracing` handles. Move the check below the terminal-action test, matching the upstream order in which a step that raises its own exception never reaches `blackhole_if_trace_too_long` — so a closing step commits its trace even when that step pushed the recorded op count past `trace_limit`. This drops the `too_long` override that made a pending overflow outrank a `CloseLoop`/`Finish`, and the post-loop recheck the deferral needed. Remove `TraceCtx::abort_at_next_merge_point` and, with the merge-point arm that set it gone, `TraceCtx::abort_resume_jitcode_pc`. The aborting walk's top frame now always publishes `code_cursor` as its `frame.pc` for `blackhole.py:1804 copy_data_from_miframe`, which is where `pyjitpl.py:3863 self.pc = position` leaves `MIFrame.pc` after the handler decodes an instruction's operands. Assisted-by: Claude
`replay_pending_fields` and the generated `setup_bridge_sym` gated their tracing on `AHEUI_BRIDGE_DIAG`. Both print generic bridge-entry replay state (pendingfields, rd_virtuals, failarg counts) and sit next to `MAJIT_BRIDGE_DEBUG` in the same files. No `AHEUI_`-prefixed name remains under `majit/`. Assisted-by: Claude
`jit_bigint_add_int_int` / `sub_int_int` / `mul_int_int` computed the exact result in i128 and handed it to `rbigint.fromlong`, and `jit_bigint_add` / `jit_bigint_sub` carried an i128 fast path that did the same. `fromlong` goes through `args_from_long`, which builds a `Vec<Digit>` before `RBigInt::new` copies it into the GC digit block. Call `RBigInt::add_int_int_bigint_result` / `sub_int_int_bigint_result` / `mul_int_int_bigint_result` (rbigint.py:717/788/873) instead, and drop the i128 arms from `jit_bigint_add` / `jit_bigint_sub` so they reach `RBigInt::add` / `sub` (rbigint.py:681/755). The sign-zero early returns stay: they spell the "return the other operand itself" identity of rbigint.py:684-687 and :758-759 in the raw-pointer ABI. synth/int_mul_ovf_bignum_promote steady-state, startup-subtracted CPU time, min of 5, dynasm: 69.9ns -> 37.0ns per iteration at 8M iterations (pypy3 ratio 5.50x -> 3.05x). Assisted-by: Claude
The overflowing Add/Sub/Mul arm emitted `CallI(jit_bigint_fits_int, payload)` + `GuardFalse` before boxing the W_LongObject. The guard is answered by the `GuardOverflow` two ops earlier: the payload comes from `add/sub/mul_int_int_bigint_result`, which is the exact int-pair result, so a value that just overflowed a machine int cannot fit one back. `newlong_from_rbigint` (objspace.py:316-320) reaches the same conclusion at trace time through `rbigint.toint()`'s `numdigits() > MAX_DIGITS_THAT_CAN_FIT_IN_INT` test (rbigint.py:470); PyPy's own expected trace for this loop shape carries no such call (pypy/module/pypyjit/test_pypy_c/test_misc.py:52-72 test_factorial). `try_walker_specialize_binary_op_long_int_pow` already omits its result-fits guard on the same grounds. synth/int_mul_ovf_bignum_promote optimized trace: 52 -> 48 ops (steady loop 21 -> 19). Startup-subtracted CPU time, min of 9, dynasm, 8M/16M iterations: 32.7ns / 31.4ns per iteration (pypy3 ratio 2.88x / 2.75x). Assisted-by: Claude
Every allocation, write barrier and GC ownership query opened with a thread-local read plus a `RefCell` borrow of the backend's per-thread GC box. That box is installed only by `set_gc_allocator` -> `install_gc_box`, which is the test path; production calls `install_gc_standalone` and leaves the cell `None` on every thread, so the probe always missed and every caller paid a `_tlv_get_addr` for it. Add a per-backend process-global `GC_BOX_INSTALLED: AtomicBool`, set by `install_gc_box` (cranelift: `set_cranelift_active_gc(Some(..))`) and checked before each probe. It is never cleared: `clear_gc_allocator` can run on one thread while another still owns a box. RPython has no per-thread allocator — `gctransform/framework.py` weaves every `malloc` against the single translation-time `gcdata`. Gated: dynasm 12 sites, cranelift 6, wasm 4. synth/int_mul_ovf_bignum_promote steady state, startup-subtracted CPU time, min of 9, dynasm: 44.6ns -> 37.4ns per iteration at 8M, 41.4ns -> 35.4ns at 16M. Assisted-by: Claude
The three per-backend `GC_BOX_INSTALLED` statics tracked the same fact, so replace them with one in majit-gc behind `note_gc_box_installed` / `gc_box_installed`. Add `standalone_alloc_nursery_typed` and `standalone_alloc_nursery_collecting_typed_rooted`: the allocation each backend's hook performs once no per-thread box owns the request, spelled once in majit-gc. The dynasm trampolines fall through to them, and `alloc_nursery_typed` / `alloc_nursery_collecting_typed_rooted` call them directly when no box is installed instead of going out through the hook. Both are `#[inline]`; without it the entry points trade an indirect call for a cross-crate call. The hook is still read for its presence, so an uninstalled backend keeps returning null. Assisted-by: Claude
…rations At 20000 the promoting loop was ~0.8% of the run: the measurement was the 2.4M-iteration warm-up, and the whole exec came to 0.01s against a 0.01s pypy startup and a 0.08s dynasm one, so `max-pypy-ratio` compared two numbers that were mostly startup subtraction error. It read 9.3-9.8x on CI (both ubuntu and macos, and on main) against a gate of 8, and 2.75x locally. check.py's gate policy (check.py:1890-1897) covers this case: a gate is only tightened when the baseline is healthy, and where it is not the workload is sized up instead. pypy exec is now ~18x its startup. Measured through check.py: dynasm 2.3x, cranelift 2.8x, wasm 5.4x. The gate stays at 8. Assisted-by: Claude
`publish_state` rebuilt `all_liveness_positions` from the whole buffer on every call: `decode_liveness_records` over every record the buffer holds, three `Vec<u8>` allocated per record, then a rehash into a fresh IndexMap. It is called from `Assembler::intern_liveness` once per `-live-` marker interned, and the build-time drain's prefix is already ~21.5 KiB, so the cost is `interns * buffer_len`. `state::intern_liveness` is the only consumer of the mirror's dict, so derive it there on first use after each publish. `publish_state` and `AssemblerState::new` leave the field `None`; the dict a reader-side intern derives still describes the bytes it is asked about, so the duplicate append the rebuild was added to prevent stays prevented. The affected quantity is JIT compile time: on `bench/synth/inlined_helper_arith_hot`, N 40000 -> 4000000 moves pyre's user CPU 0.31s -> 0.36s against a 0.04s startup. Startup-subtracted ratios against pypy3, dynasm/cranelift/wasm on aarch64: that bench 8.0x -> 2.9x/3.7x/3.3x, `residual_raise_except_resume` -> 2.0x/2.5x/2.1x, `raise_catch` 1.7x -> 1.0x. check.py: dynasm 348/348, cranelift 348/348, wasm 344/344. Assisted-by: Claude
`visit_pool` ran once per exit layout of every trace of every compiled
loop and tested membership with `Vec::contains`, so one walk was
quadratic in the number of distinct const pools, and that count grows
with the compiled code. The walk itself runs on every collection.
Replace the `Vec<usize>` with an `IndexSet<usize>` and fold the
membership test into the insert.
Measured on 30 renamed copies of `bench/synth/inlined_helper_arith_hot`
at N=3000, dynasm/aarch64, user CPU:
loops 13 26 52 104 208 390
before 0.09 0.14 0.27 0.64 2.06 8.37s
after 0.09 0.15 0.26 0.53 1.24 3.18s
`sample` over the 390-loop run puts `rd_consts_root_walker_area` at 5339
of 7781 main-thread samples before and 104 after. check.py: dynasm
349/349, cranelift 349/349, wasm 345/345.
Assisted-by: Claude
There was a problem hiding this comment.
Actionable comments posted: 2
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/dispatch.rs (1)
7956-7993: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winApply abort-framestack recovery on both single-pass trace paths.
trace_jitcode_from_merge_pointalso builds aStandaloneFrameStackand returnsmachine.run_to_end(...)directly, so mid-opcode aborts through merge-point tracing bypass the new panic-flag check, final pc update, andaborted_framestackstash. Root the parkedref_valuesin the blackhole conversion path too; this stash keeps the active frame bank across a call boundary where unrooted raw GC pointers can become live to collection.🤖 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/dispatch.rs` around lines 7956 - 7993, Apply the same Abort recovery logic to trace_jitcode_from_merge_point before returning machine.run_to_end(...): update the final pc, honor and clear abort_after_panic, and stash the StandaloneFrameStack in aborted_framestack while preserving the fallback behavior. In the blackhole conversion path, root the parked ref_values across call boundaries using the existing rooting mechanism.
♻️ Duplicate comments (1)
majit/majit-backend-cranelift/src/compiler.rs (1)
12278-12284: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA missing write-barrier descriptor still degrades to a no-op barrier.
wb_mask_rawdefaults to0, soflag_ext & mask_valis always0andneeds_wbis always false. EveryCondCallGcWb/CondCallGcWbArraystore then skips the barrier slow path. The dynasm sibling fails loud with.expect("COND_CALL_GC_WB emitted without a write barrier descriptor"). Now thatgc_rewritersourceswb_descrfromgc.get_write_barrier_descr()(Line 8145), the rewriter only emits these ops when a descriptor exists, so reaching this arm without one is an invariant violation.🛡️ Proposed fix to fail loud instead of dropping the barrier
- let wb = rw.as_ref().and_then(|r| r.wb_descr.as_ref()); - let wb_byteofs = wb.map(|d| d.jit_wb_if_flag_byteofs as i32).unwrap_or(0); - let wb_mask_raw = wb.map(|d| d.jit_wb_if_flag_singlebyte).unwrap_or(0); - let wb_cards_set = wb.map(|d| d.jit_wb_cards_set).unwrap_or(0); - let wb_card_shift = wb.map(|d| d.jit_wb_card_page_shift).unwrap_or(0); - let wb_cards_singlebyte = - wb.map(|d| d.jit_wb_cards_set_singlebyte).unwrap_or(0); + let wb = rw + .as_ref() + .and_then(|r| r.wb_descr.as_ref()) + .expect("COND_CALL_GC_WB emitted without a write barrier descriptor"); + let wb_byteofs = wb.jit_wb_if_flag_byteofs as i32; + let wb_mask_raw = wb.jit_wb_if_flag_singlebyte; + let wb_cards_set = wb.jit_wb_cards_set; + let wb_card_shift = wb.jit_wb_card_page_shift; + let wb_cards_singlebyte = wb.jit_wb_cards_set_singlebyte;🤖 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 12278 - 12284, Update the write-barrier descriptor extraction in the compiler arm handling CondCallGcWb and CondCallGcWbArray so a missing wb_descr fails immediately with the same invariant-style expectation used by the dynasm sibling, rather than defaulting wb_mask_raw and related values to zero. Preserve the existing descriptor-based field extraction when wb_descr is present.
🤖 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 1709-1718: Handle the Err branch of try_borrow_mut in the via_box
logic so a reentrant borrow does not bypass the synchronized-GC fallback unless
the boxed allocator invariant guarantees that addr belongs to its address space.
Prefer routing the borrow failure through the raw mirror, consistent with
gc_owns_object_via_active_runtime and gc_is_nursery_object_via_active_runtime;
otherwise add a concise comment documenting the invariant.
In `@majit/majit-metainterp/src/optimizeopt/heap.rs`:
- Around line 4583-4604: Strengthen the SetfieldGc assertions in the affected
optimizer tests around result and the emitted guard: verify each expected
store’s descriptor, target object, and value rather than only counting
operations. In the size-store test, assert no emitted SetfieldGc targets d_head,
and verify the guard’s resume data retains d_head as a pending field while
preserving the existing ordering checks.
---
Outside diff comments:
In `@majit/majit-metainterp/src/pyjitpl/dispatch.rs`:
- Around line 7956-7993: Apply the same Abort recovery logic to
trace_jitcode_from_merge_point before returning machine.run_to_end(...): update
the final pc, honor and clear abort_after_panic, and stash the
StandaloneFrameStack in aborted_framestack while preserving the fallback
behavior. In the blackhole conversion path, root the parked ref_values across
call boundaries using the existing rooting mechanism.
---
Duplicate comments:
In `@majit/majit-backend-cranelift/src/compiler.rs`:
- Around line 12278-12284: Update the write-barrier descriptor extraction in the
compiler arm handling CondCallGcWb and CondCallGcWbArray so a missing wb_descr
fails immediately with the same invariant-style expectation used by the dynasm
sibling, rather than defaulting wb_mask_raw and related values to zero. Preserve
the existing descriptor-based field extraction when wb_descr is present.
🪄 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: f4a50d17-22a0-48a8-b231-2cdc99f2aeb3
📒 Files selected for processing (31)
.github/workflows/pyre-ci.ymlmajit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/runner.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-gc/src/lib.rsmajit/majit-gc/src/rewrite.rsmajit/majit-macros/src/jit_interp/codegen_state.rsmajit/majit-macros/src/jit_interp/mod.rsmajit/majit-metainterp/src/blackhole.rsmajit/majit-metainterp/src/jit_state.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/lib.rsmajit/majit-metainterp/src/optimizeopt/heap.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/resume_box_reader.rsmajit/majit-metainterp/src/trace_ctx.rsmajit/majit-metainterp/tests/abort_blackhole_virt_array.rsmajit/majit-translate/src/codewriter/flatten.rsmajit/majit-translate/src/codewriter/jitcode.rsmajit/majit-translate/src/codewriter/liveness.rsmajit/majit-translate/src/front/mir.rsmajit/majit-translate/tests/test_make_jitcodes_produces_graph_keyed_output.rspyre/bench/synth/int_mul_ovf_bignum_promote.pypyre/pyre-jit-trace/src/assembler.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/jitcode_runtime.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/jit/assembler.rspyre/pyre-object/src/longobject.rs
| let via_box = majit_gc::gc_box_installed().then(|| { | ||
| CRANELIFT_ACTIVE_GC.with(|cell| { | ||
| let mut guard = match cell.try_borrow_mut() { | ||
| Ok(guard) => guard, | ||
| Err(_) => return Some(addr), | ||
| }; | ||
| guard.as_deref_mut().map(|gc| gc.id_or_identityhash(addr)) | ||
| }) | ||
| }); | ||
| if let Some(r) = via_box { | ||
| if let Some(Some(r)) = via_box { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
A reentrant borrow now short-circuits the synchronized-GC fallback.
cell.try_borrow_mut() returns Some(addr) on failure. The outer if let Some(Some(r)) then returns addr and never reaches the gc_sync path at Line 1721. This is only correct when a reentrant borrow implies a boxed allocator owns the address space. Add a short comment that states this invariant, or route the borrow error through the raw mirror like gc_owns_object_via_active_runtime and gc_is_nursery_object_via_active_runtime do.
🤖 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 1709 - 1718,
Handle the Err branch of try_borrow_mut in the via_box logic so a reentrant
borrow does not bypass the synchronized-GC fallback unless the boxed allocator
invariant guarantees that addr belongs to its address space. Prefer routing the
borrow failure through the raw mirror, consistent with
gc_owns_object_via_active_runtime and gc_is_nursery_object_via_active_runtime;
otherwise add a concise comment documenting the invariant.
| let setfields: Vec<&Op> = result | ||
| .iter() | ||
| .filter(|o| o.opcode == OpCode::SetfieldGc) | ||
| .collect(); | ||
| assert_eq!( | ||
| setfields.len(), | ||
| 2, | ||
| "both stores must survive; emitted = {:?}", | ||
| result.iter().map(|o| o.opcode).collect::<Vec<_>>(), | ||
| ); | ||
| let guard_at = result | ||
| .iter() | ||
| .position(|o| o.opcode == OpCode::GuardTrue) | ||
| .expect("guard must be emitted"); | ||
| let last_setfield_at = result | ||
| .iter() | ||
| .rposition(|o| o.opcode == OpCode::SetfieldGc) | ||
| .unwrap(); | ||
| assert!( | ||
| last_setfield_at < guard_at, | ||
| "both stores must be emitted before the guard they precede", | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the identity and pending state of each store.
The first test only counts two SetfieldGc operations. It can pass if the optimizer duplicates one store and drops the other. Assert each store's descriptor, target object, and value.
The second test only checks the size-store order. It can pass if the virtual d_head store is also emitted. Assert that no emitted SetfieldGc uses d_head. Also assert that the guard resume data contains the d_head pending field.
These checks are required to prove the stated lazy-store and deoptimization contracts.
Also applies to: 4744-4758
🤖 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/optimizeopt/heap.rs` around lines 4583 - 4604,
Strengthen the SetfieldGc assertions in the affected optimizer tests around
result and the emitted guard: verify each expected store’s descriptor, target
object, and value rather than only counting operations. In the size-store test,
assert no emitted SetfieldGc targets d_head, and verify the guard’s resume data
retains d_head as a pending field while preserving the existing ordering checks.
Both `debug_assert!`s were bare, so a failure printed only the truncated expression (`assertion failed: (field_idx as usize) <`) with neither the slot that was written nor the field-list length it exceeded. That is the whole diagnosis when the failure reproduces only on a machine the developer cannot attach to. Carry `field_idx`, `all_fielddescrs().len()` and the descr index into the message on both the `Virtual` and `VirtualStruct` arms. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 141146307f
ℹ️ 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".
| // handler) leaves `MIFrame.pc`. | ||
| if !std::mem::replace(&mut ctx.abort_after_panic, false) { | ||
| if let Some(top) = standalone.frames.frames.last_mut() { | ||
| top.pc = top.code_cursor; |
There was a problem hiding this comment.
When the production recursive decision returns ResidualCall for an uncompiled green key, exec_recursive_call decodes the operands but returns Abort without executing or pushing the callee. Overwriting top.pc with the post-decode code_cursor therefore stages the conversion after BC_RECURSIVE_CALL_*; once this pending conversion is consumed, the blackhole skips the call entirely, leaving a typed result uninitialized and omitting the callee's side effects. Preserve the opcode-start position, or decline conversion for abort paths that did not complete the current instruction.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
`test_guard_forces_size_store_paired_with_virtual_head_store` builds the virtual with `virtual_size_descr(1, 4)` — four fields — and stored into it through `descr(10)`. `optimize_setfield_gc` indexes `PtrInfo::VirtualStruct.fields` by the field descr's slot, and `virtualize.rs` asserts the slot stays inside the struct's own `all_fielddescrs()`. A `TestDescr` reports no `index_in_parent`, so it reads back as the trait default 0 where the descr is used raw and as `offset() / field_size()` — its index — wherever it is reconciled. On the CI runners the store resolved to slot 10 of a 4-field struct and the assertion fired; the combination cannot arise in production, where the field descr comes from the struct type being stored into. Use `descr(0)`. `d_head` / `d_size` are stored into the non-virtual `p0` and keep their indices. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88c5a36d42
ℹ️ 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".
| // included), and `None` from one that needs no barrier | ||
| // (`gc.py:156 GcLLDescr_boehm`) keeps `rewrite.py:393` from | ||
| // emitting `COND_CALL_GC_WB*` the backend could not assemble. | ||
| wb_descr: gc.get_write_barrier_descr(), |
There was a problem hiding this comment.
When a custom generational GcAllocator overrides write_barrier but relies on the trait's existing default get_write_barrier_descr() (which returns None), this now passes None into GcRewriterImpl, so the new rewrite gates emit no COND_CALL_GC_WB*. Before this change both native backends constructed a generic descriptor for every allocator, so such implementations compile unchanged but silently lose old-to-young barriers and may reclaim reachable nursery objects; make the accessor required/default to the prior descriptor, or reserve None for collectors that explicitly opt out.
Useful? React with 👍 / 👎.
Two independent lines of work, both grounded in RPython/PyPy sources.
1. Run a tracing abort's framestack through
convert_and_run_from_pyjitplpyjitpl.py:2949 run_blackhole_interp_to_cancel_tracinghands the abortedmetainterp.framestacktoblackhole.py:1799 convert_and_run_from_pyjitpl,which converts it into a blackhole chain and runs it until the bottommost
frame's
jit_merge_pointraisesContinueRunningNormally. Half-executedopcodes therefore finish instead of being skipped. pyre had only a narrow
band-aid (
abort_at_next_merge_point); this wires up the orthodox consumer thein-code TODO named.
Because the walk's framestack is local to the dispatcher and the abort arm has
no native
state, it is staged in two hops:TraceCtx::aborted_framestack→MetaInterp::pending_abort_blackhole→JitDriver::run_pending_abort_blackhole,called from the
jit_merge_point!expansion — the first point holdingstate.Three coverage bugs surfaced on the way, each also latent on the guard-failure
resume path:
call_result_regassumed RPython's single-slot return tail, butinline_call_typedcloses everyBC_INLINE_CALLwith three slots, soblackhole.py:1655'scode[position-1]read theNO_RETURN_REGsentinel.BC_NEWhad a wired handler but no opname→byte entry.JitCodecarried nojitdriver_sd, soblackhole.py:1764's walk ran past the root and never captured the terminalimage. Fixed at the root per
call.py:147-148, which sets both directions.stb.raising_exceptionis now carried, with the value read fromMetaInterp::last_exc_valuewhereblackhole.py:1811-1814reads it.The conversion previously declined for any state with a virtualizable, which
turned out to disable it almost everywhere: aheui is the only frontend without
a
[.. ; virt]array. Upstream never declines, and that decline is now gone.The remaining
array_lensdecline (flattened fixed[int]) stays — no frontenddeclares one and there is no sym-side per-element collector.
MAJIT_LOG=1prints zero
abort-blackholelines). Forcing aborts withMAJIT_STEP_LIMIT=200on a virt-array frontend reproduces the torn-state defect
(
index out of bounds: the len is 21 but the index is 21,stackposunderflowed)with the conversion off, and passes with it on. Pinned as
majit-metainterp/tests/abort_blackhole_virt_array.rs, its own test binary so itcan set the env knob, and verified to fail under
MAJIT_ABORT_BLACKHOLE=0.2. Gate write barriers on the collector's descriptor
rewrite.py:393gates the entire write-barrier section onself.gc_ll_descr.write_barrier_descr is not None, andaarch64/assembler.py:975-980gates the jitframe barrier onif gcrootmap and wbdescr. pyre had neither gate on aarch64:GcRewriterImpl.wb_descrwas builtunconditionally from
WriteBarrierDescr::for_current_gc()instead of asking thecollector, and
reload_frame_if_necessaryre-applied the barrier withoutchecking. The x86 path already had the second gate.
This was latent until #796 replaced a silent
None => returninemit_write_barrier_fastpath_for_basewith an assertion — correct in itself, abarrier assembling to zero bytes is invisible until it corrupts memory. After
that, a collector reporting no descriptor (the
gc.py:156 GcLLDescr_boehmcase)took the assertion on every compile, so every trace was dropped and the frontend
fell back to the interpreter. Bisected:
314019bf78^0 panics vs314019bf788326.
Note the assertion text names
COND_CALL_GC_WB, but the reaching caller isreload_frame_if_necessary←genop_call_malloc_nursery_headerless— theallocator, with no barrier op in the trace at all.
MiniMarkGC::get_write_barrier_descrreturns exactly what the backends wereconstructing inline, card-marking adjustment included, so the production path is
unchanged.
Verification
pyre/check.py: dynasm 342/342, cranelift 342/342, wasm 339/339 — ALL PASSED7fcdbfff0af449c4283c008e3ca317ce, jit output byte-identical to--no-jitmajit-metainterp1427 passed,majit-gc215 passed + 2 new gate testscargo check --workspace --exclude pyreandcargo fmt --checkclean🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Tests