Fix Windows dynasm test failures in debug and release - #23
Merged
Conversation
- cranelift test_raw_store_and_load_f_roundtrip: enlarge data buffer from 8 to 16 bytes so the 8-byte f64 write at offset 2 stays in bounds; the prior 2-byte overrun corrupted heap metadata and tripped STATUS_HEAP_CORRUPTION on Windows during process teardown. - gate four `#[should_panic]` tests on `#[cfg(debug_assertions)]` (set_forwarded_to_self_panics_in_debug, make_result_of_lastop_panics_on_recorded_resulttype_mismatch, do_recursive_call_panics_when_portal_runner_adr_is_zero, assert_no_exception_panics_when_value_set, builtin_func_for_spec_rejects_mismatched_extra_extrakey): their assertions are `debug_assert*!` and elided in release, so the expected panic never fires. - gate the `assert_no_indirect_call_targets` call in rpbc.rs tests on `#[cfg(debug_assertions)]` to match the function's own gating, fixing the release-mode build error.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThis PR contains two coordinated test improvements: a buffer allocation fix in the Cranelift backend test for f64 writes on Windows, and conditional compilation guards that restrict panic and invariant assertion tests to debug-only builds across the metainterp and translate modules. ChangesTest Robustness Updates
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
youknowone
added a commit
that referenced
this pull request
May 16, 2026
Clarify why these record-with-Void sites are not a propagation gap in current architecture: 1. Walker is invoked only via shadow_walker.rs allow-list, which today contains only Nop family (shadow_walker.rs:106-134). 2. Production residual_call routes through the trait leg (pyjitpl::do_residual_call_full at mod.rs:13189), which calls executor::execute_varargs and seeds the concrete via record(opcode, args, c_result, descr) at mod.rs:13225. 3. The walker arm is exercised by unit tests only; tests use synthetic ConstPtr/ConstInt that wouldn't survive a real call invocation anyway. Pattern divergence from E3.1/E3.2: those propagate via inline raw memory reads (read_field_concrete / read_arrayitem_concrete), which are side-effect-free derefs. residual_call requires invoking an arbitrary foreign function — a real side effect — which the walker cannot do without MetaInterp access in WalkContext. That plumbing lands at M4.Cutover (Task #23) when the walker takes over from the trait leg as production tracer.
youknowone
added a commit
that referenced
this pull request
Jul 19, 2026
…lk (#171) (#644) * interp_exceptions: pin the fresh managed exception across its args build `w_exception_new` / `w_exception_new_wtf8` / `PyError::to_exc_object` allocate a GC-managed `W_BaseException` and then call `w_list_new` to build `args` while the exception lives only in a Rust local. Should that allocation ever collect, the unrooted (non-moving oldgen) exception could be swept before `w_exception_set_args` writes through it. Pin the exception on the shadow stack (`push_roots` + `pin_root`) across the args/context build; the exception is non-moving so no reload is needed. Matches RPython rooting the OperationError local across allocation. Assisted-by: Claude * jit: root the raw-i64 exception carriers during GC root walk The residual-call/blackhole/portal exception path parks a raised exception in raw i64 cells the precise collector cannot see, so a safepoint major between the raise and the frame that catches it could sweep the oldgen traceback chain. Root all three carriers on every collection: - BH_LAST_EXC_VALUE (blackhole residual-call raise): walk_bh_last_exception in the interpreter global-prebuilt-root walk, marking the exception and its children. - JIT_EXC_VALUE (per-backend atomic slot): walk_jit_exc_value in pyre-jit, reading each backend slot through a new non-destructive jit_exc_value_peek(). - PENDING_CALL_ERROR: walk_pending_call_error forwards err.exc_object and the name/obj contexts. The carrier walkers resolve their thread-local on the collecting thread, which under the single-threaded execution model is the only mutator; per-mutator root-area migration is deferred with the free-threading EXC work. JIT_EXC_TYPE holds an immortal type pointer and is not rooted. Assisted-by: Claude * error: root the deferred name/obj context across the to_exc_object allocation `to_exc_object` allocated the fresh exception (`w_exception_new`) before rooting `self.w_name_context` / `self.w_obj_context`. Those references live only in the Rust `PyError`, which the precise collector does not scan, so a collection inside `w_exception_new` could sweep them before they are stamped onto the new exception at the `set_name` / `set_attr_obj` calls. Open the root frame and pin both contexts before the allocation. Assisted-by: Claude * jit: materialize err.to_exc_object() at residual/portal exception publish sites Several residual-call / portal / blackhole-resume error paths published `err.exc_object` directly and, guarded by a null check, published nothing when it was null — the case for a PyError built from a kind + message that was never materialized into an object. The trailing GuardNoException / resume then saw no pending exception. Publish `err.to_exc_object()` instead, which returns the cached object or materializes one, so a raise is always carried. Covers the BH_LAST_EXC_VALUE / store_jit_exception / ExitFrameWithExceptionRef publish sites in call_jit.rs and pyre-jit/eval.rs; the record_application_traceback reads keep the raw object they attach the traceback to. Assisted-by: Claude * reformat jit exception-carrier code with rustfmt 1.97.1 The #21/#23 commits carried formatting from a differently-versioned rustfmt; `cargo fmt --check` under the stable toolchain rejected the import list, the BH_LAST_EXC_VALUE publish line, and the jit_exc_value_peek_backend cfg attribute. Assisted-by: Claude * jit: root the stored PyError TLS carriers during GC root walk Three stored-PyError slots parked exceptions across boundaries where arbitrary code (and a collection) can run, while the exception's GC refs were reachable only through a raw TLS cell the precise collector never scans. A major collection between park and take then swept the exception, and `take_*` surfaced a dangling pointer (out-of-range kind, packed-heap- pointer subclassrange_min). The call-assembler stash `LAST_CA_EXCEPTION` is a use-after-free independent of any in-flight-clear. Forward each parked PyError's up-to-three GC refs, mirroring the existing `walk_pending_call_error`: - extract the shared three-field forwarder into `PyError::walk_gc_refs` and route `walk_pending_call_error` through it - `walk_pending_hash_error` (dict-probe __hash__/__eq__ stash), called from `walk_global_prebuilt_roots` before the is_minor gate - `walk_last_ca_exception` (call-assembler FFI stash) - `walk_walk_end_propagated_exception` (no-handler trace->portal stash) The last two register through `register_extra_root_walker` (3 -> 5 of 16). Marks are add-only, so the walkers cannot under-root; a still-lazy `exc_object` is skipped rather than materialised during a collection. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
Replace `<file>.rs:NNN` line citations in pyre-jit-trace comments with the symbol that lives at the cited location, after opening each target. Upstream `rpython/`, `pypy/` and `lib-python/` `.py:NNN` parity citations are left unchanged. Where the cited pyre-side location holds no nameable symbol, or the file/function no longer exists, only the `:NNN` is dropped and the filename kept. Strip internal session tracking labels (`B3`, `C3 S1`, `E1`, `G0`/`G1`/`G2`, `Epic G`, `gap 10 slice 2b`, `P2 drain`, `P3`, `Route C`, `Task 8`, `increment 2b`, `STEP 5`) from the comments that carried them; GitHub issue references (`#32`, `#73`, `#171`, `#203`, `#215`, `#62`/`#23`) are kept. Also make four cross-reference comments self-contained by stating the fact instead of pointing at another comment ("see the module preamble", "see above", "the `current`-frame pattern", "see `history.rs`"). Comment-only: no code, string literal or test data is modified. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
…d deny through the warm state (#1364) * jit: name the fixture the inline-chain depth cap was measured on `FBW_INLINE_CHAIN_DEPTH`'s doc cited `depthN_inline_chain`, which no longer names any file: #829 deleted `depth2_`/`depth3_`/`depth7_inline_chain_typeflip.py` and added `inline_chain_depth_typeflip.py` in the same commit. Cite the surviving file and record that the ~2.0-2.3x number was taken before the consolidation. Assisted-by: Claude * majit: publish the preview short-preamble export as one PreviewShortState Replace the four OptContext fields `exported_short_boxes`, `exported_short_inputargs`, `exported_short_inputarg_refs` and `exported_short_args_state` with a single `Option<PreviewShortState>` holding the three vectors plus an `Option` args_state. `preamble_end_args` stays a separate `Option<Vec<OpRef>>`. optimizer.rs binds `create_short_inputargs`, `create_short_inputarg_refs`, the args-state tuple and the filtered short boxes to locals and assigns the struct once, after the majit_log dump. `force_box_for_end_of_preamble` and its `preamble_end_args` write are unmoved. unroll.rs::export_state_with_bounds reads the args-state through `preview_short_state.and_then(|p| p.args_state)`, and takes the short inputargs / inputarg refs / short boxes from one match on the `Option` instead of an `is_empty()` test on a defaulted vector. The `debug_assert_eq!` cross-checking `exported_short_inputarg_refs` against `exported_short_inputargs` is dropped; the two are now built and published together. The length check against the export-site `label_args + virtuals` recompute is kept. unroll.rs tests gain `publish_preview_short_state` and `mint_short_inputargs` helpers; the four fixtures that wrote the ctx fields directly now construct the struct. Assisted-by: Claude * docs: correct the jit.virtual_ref emit note and two fbw fixture headers gate-triage.md claimed `opimpl_virtual_ref` / `_finish` have no caller outside a `#[test]`. They do: `walker_ec_enter` / `walker_ec_leave` call them on the live inline-push path in `pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`, which the same file already says 100 lines later. The parenthetical now states the narrower residue instead -- `vrefs_before_residual_call` / `vrefs_after_residual_call` iterate zero times over a level the walker inlines without seeding a frame -- and points at `jitcode_dispatch/mod.rs` item (a), where that residue is recorded. It also notes that `mod.rs`'s upstream citation for the residue is wrong: `perform_call` (pyjitpl.py:2445-2449) is `newframe` + `setup_call` and never touches `virtualref_boxes`; upstream's vref comes from `ExecutionContext.enter` (pypy/interpreter/executioncontext.py:88-89), traced through on an inlined call. getframe_root_loop_force_blackhole_crn.py said "this file adopts it five times". Its committed baselines record fbw_blackhole_adopted_single_frame=0, loops_aborted=0, loops_compiled=1 on all three backends; ca9edf7 (#1096) moved them from 5 / 5 / 0. Header now states the recorded numbers and points at the `_declined` sibling, which records 5 / 5 / 0. blackhole_inlined_callee_local_after_escape.py opened "Guard for what an adopted multi-frame blackhole chain owes its inner levels". fbw_blackhole_adopted_multi_frame is 0 in all three of its baselines and was 0 before #1096 as well; the five adopts it used to take were single-frame, and today it takes none (0 / 0 / 2). Header now says so, names the eight fixtures' worth of corpus that does pin the multi-frame arm (getframe_inline_subwalk_multiframe, getframe_while_inlined_callee_subwalk et al., all nonzero on three backends), and points at the `_declined` sibling. No baseline was re-recorded; no Rust and no executable Python changed. Assisted-by: Claude * docs: separate the MIFrame perform_call builds from the frame a vref is taken of The previous commit's gate-triage note called `pyjitpl.py:2445-2476` a wrong citation and said `mod.rs` held the last copy. Both overreach. The range spans `newframe` (:2455-2476), which does build a fresh frame per inlined call -- an `MIFrame`, the tracer's register frame. `pyre-jit-trace/src/helpers.rs` cites it for exactly that and is correct; stripping it there would remove a right citation. The same sentence also lives in `inline_call.rs`, so `mod.rs` was never the only copy. What is actually wrong at the two vref sites is conflating that `MIFrame` with the app-level frame `ExecutionContext.enter` takes `jit.virtual_ref` of. State that at both, and say in gate-triage which of the two claims the range supports. Also: the emit note said `virtualref_boxes` is populated for every seeded level, but `entered_ec` adds a non-null concrete frame and a non-null `execution_context` on top of seeding -- necessary, not sufficient. And the crn fixture header's inserted paragraph left "Its effects are idempotent" pointing at the `_declined` sibling instead of the drive. Assisted-by: Claude * majit: lower the int/float list capacity read as arraylen_gc `list.int_capacity` / `list.float_capacity` emitted `getfield_gc_r(<strategy>_items.block)` followed by a struct `getfield_gc_i(ItemsBlock.capacity)`. Upstream's capacity read is `len(l.items)` on `l.items: Ptr(GcArray(ITEM))` (rpython/rtyper/lltypesystem/rlist.py:251 in `_ll_list_resize_hint`, rlist.py:286 in `_ll_list_resize_ge`), which the rtyper lowers to `getarraysize` and `jtransform.py:808 rewrite_op_getarraysize` rewrites to `arraylen_gc`. `list.obj_capacity` already emitted `ArrayLen`. Both arms now emit `OpKind::ArrayLen` on the backing block, matching the object-strategy arm. The two unit tests are renamed and assert the new op shape. Assisted-by: Claude * majit: narrow the PreviewShortState alignment claim to the two vectors that share an index space The struct doc and the publication comment both said the three published vectors are "index-aligned by construction". Only two of them are: `short_inputargs` and `short_inputarg_refs` get one entry per `add_short_input_arg`. `short_boxes` is a different population -- the surviving produced short ops, after `short_boxes_exported`'s `filter_map` drops every one whose `canonical_result` is constant -- so its length is unrelated to the other two and `short_boxes[i]` pairs with nothing. What one publication site does buy is that a reader cannot see one vector from this evaluation of the preview beside another that was never written; both comments now say that instead. The unroll.rs test helper restates the refs/inputargs length invariant as a `debug_assert_eq!`. The production publisher gets it from `create_short_inputarg_refs`, which asserts internally; a fixture builds the two vectors by hand and had no check between a short refs vector and the failed `Weak` upgrade it causes past the peel boundary. Assisted-by: Claude * docs: correct eight comments that assert a capability the code already has Each of these states an absolute negative -- "never emits", "never calls it", "cannot be added and go unnamed" -- that its own call site contradicts. - `residual_call.rs`: `vrefs_after_residual_call` is called by the walker, under the `is_may_force` gate mirroring `pyjitpl.py:2007`. Its loops are empty because no `jit.virtual_ref` producers exist, which is a fact about the vref list, not about the call site. - `branch.rs` `decode_side_other_target`: the fused `goto_if_not_<cmp>` forms do reach the walk dispatch, minted by `majit-translate`'s jtransform as `ExitSwitch::Fused` for the LLBC-lowered graphs. What is true is narrower: the sole caller passes a `PyJitCode`, built per Python CodeObject by pyre's own codewriter, and `ExitSwitch::Tuple` -- the only path to a fused goto there -- has no producer outside `flatten.rs`'s own unit test. A fused form would be declined, not mis-decoded. - `inline_call.rs` / `fbw_state.rs`: `callee_body_contains_raise` caps a raising callee at TWO multiframe levels, not at the top inline level, and the cross-frame unwind bridge is built. The measurement beside `effective_multiframe_depth` is what bounds it: two levels green, a third taking `selfrec_tail_exception_unwind` from 937 to 7408 guard failures. - `state.rs` (two sites) / `helpers.rs`: `perform_call` (`pyjitpl.py:2445-2449`) is `newframe` + `setup_call` + `raise ChangeFrame`, and `newframe` (`:2455-2476`) builds an `MIFrame` and nothing else. Upstream has no recording-time app-level frame to hand out at that point; it gets one from tracing the interpreter's own frame construction, which pyre does not do. - `state.rs` / `trace.rs`: the pointer to `perform_call (trace_opcode.rs)` is dead -- no such function there. Callee sym state is set by `inline_call.rs`'s `setup_call` port. - `diag.rs` `SPEC_FOLD_ROWS`: the table cannot become a complete census by adding rows. It names a fold by its function, and two shapes have no name to give: a fold whose emit is inlined into a `match` arm has no function, and a registry-dispatched fold grows by one entry with no new call site. Also: `specialize.rs` drops line numbers from an in-repo file reference, and `tupleobject.rs` records that the `w_tuple_new` interception is the sole reason the `_ff` layout has no producer -- so restoring the upstream shape also makes the walker's `ff` specialisation arm live. Assisted-by: Claude * fbw: print all fourteen fbw_diag slots from both readers and namespace the escape/force keys The two readers of the same counter array printed disjoint index sets: the native reader (pyre/pyrex) printed {1, 6..13} and the wasm host (pyre-wasm-runner) printed {0..5, 11..13}, so slots 0 and 2..5 were bumped on the native backends and readable only through the wasm export, and slots 6..10 the other way round. One key per tally slot is now declared beside the counters as `pyre_jit_trace::trace::fbw_diag::LABELS` (length `RING_BASE`, so rustc rejects an unnamed slot), re-exported as `pyre_jit::FBW_DIAG_LABELS`, and joined against `get(i)` by both readers into a single `[jit-stats] fbw_diag` line carrying the same keys in the same order. The runner mirrors the array positionally, as it already does for `MC_DIAG_LABELS`, since it links no pyre crate. The MIDBODY_LATCH doc said the native corpus reaches neither leg "so these say whether the wasm target does"; it now says that a nonzero native value is itself the news, which is why both readers print it. On wasm the tally line moves out of the PYRE_WASM_JIT_STATS block — check.py never sets that variable — into the MAJIT_STATS block, the gate the native reader prints under. The `[fbw-census]` ring stays where it was. The four gated keys (fbw_rolled_back_with_effects, fbw_store_journal_rollback_failed, fbw_blackhole_adopted_single_frame, fbw_blackhole_adopted_multi_frame) keep their spelling and move from the counter line onto that fbw_diag line; the single `pyre_fbw_diag` lookup still feeds the missing-export refusal. The two `subset/total` fractions become named keys, the hazardous subset spelled `fbw_midbody_latch_new_unjournaled` and `fbw_escape_plain_fallback_unclean`. The five bare keys portal_only, published_callee_only, portal_and_published_callee, by_portal and by_callee_only are renamed fbw_escape_portal_only, fbw_escape_published_callee_only, fbw_escape_portal_and_published_callee, fbw_force_by_portal and fbw_force_by_callee_only: check.py's `_jit_stats_merged` folds every `[jit-stats]` line into one flat map, in which an un-namespaced key is a collision hazard. No committed .jitstats baseline carries any of the five under either spelling, so nothing is orphaned by the rename. They are left out of JITSTATS_SNAPSHOT_FIELDS, i.e. deliberately ungated, and check.py now records why: they are workload counts with no healthy value and no measured polarity (the reason `bridges_compiled` sits in neither regression list), and listing one would make every baseline that lacks it compare 0 -> N and fail until re-recorded. That re-record is a decision to take deliberately, with a polarity in hand. Checked with `cargo check -p pyrex`, `cargo check -p pyre-wasm-runner` and `cargo fmt --check`. No pyre binary was built or run, and no .jitstats snapshot was re-recorded. Assisted-by: Claude * fbw: make the decline census process-wide, as its own doc already claimed `FBW_DECLINE_CENSUS` was a `thread_local!` while the comment above it called it a "Per-process census". pyre installs `_thread` (`pyre-interpreter/src/importing.rs`), so Python threads are real OS threads and each traces on its own; the dump therefore reported only whichever thread happened to print it and silently dropped every decline the others took. Now a `static Mutex<BTreeMap>` behind a `census_map()` accessor that recovers from poisoning -- a map of counters has no invariant a panicking writer can leave broken, and a diagnostic that goes silent after an unrelated panic is worse than one that keeps counting. The lock costs nothing at this rate: the map is touched only on the cold decline path, never on the hot trace path. Pinned by `the_decline_census_counts_a_record_from_another_thread`, shown to fail on the `thread_local!` storage first: assertion `left == right` failed: a decline recorded off-thread never reached the census left: 0 right: 1 No gate exposure: `fbw_census` appears in no check.py field, no pyrex path and no committed baseline, so this changes a diagnostic only. Assisted-by: Claude * fbw: pin the wasm runner's fbw_diag label mirror against the slot constants `pyre-wasm-runner` links no pyre crate, so it restates `pyre_jit_trace::trace::fbw_diag::LABELS` as a positional array. rustc length-checks each side against its own constant (`RING_BASE` / `FBW_SLOTS`), but neither compiler sees the spellings, so a rename drifts silently and every tally from the divergence onward is printed under the wrong key -- and check.py folds every `[jit-stats]` line into one flat map, so a wrong name is compared against the wrong baseline rather than reported as missing. Four checks, following `majit-metainterp/tests/mc_diag_mirror.rs`: the parser is validated against the compiler-enforced count before being used to diagnose drift, the two declared counts must agree, and the two arrays must agree entry by entry. A positive control injects both drift shapes into the real runner source in memory -- a rename, caught by the entry comparison, and a dropped last slot, which leaves every surviving entry correctly named and so can only be caught by the length check. Perturbing the real text rather than a fixture is what makes the control cover the anchors. A fifth check closes what a two-array diff structurally cannot see: `LABELS` shifting against the slot CONSTANTS moves both arrays together, renaming every tally on both backends at once. Each label is bound to its own constant, and the bound slots are required to be exactly `0..RING_BASE` so a new slot cannot go unbound. The bindings are written out rather than derived from the constant names because two of them break the mechanical reading: `ESCAPE_FORCE_BY_PORTAL` is `fbw_force_by_portal`, not `fbw_escape_force_by_portal`. Assisted-by: Claude * docs: name the third fold shape SPEC_FOLD_ROWS structurally cannot hold The table names a fold by its function. Two shapes with no name to give were already recorded; a sweep of `vable_ops.rs` found a third. An ELISION fold recognises a shape and emits nothing, so a census keyed on "what IR did this fold emit instead" has nothing to key on. Three arms are this, all guarded by `fbw_strict_fold_frame_reg`: a store to the current inline level's own unseeded portal frame is a virtual-field write, folded away with no SETFIELD_GC recorded. Their recognisers (`fbw_strict_fold_frame_reg`, `folded_store_is_observable_local`) are predicates -- they cannot emit, because the eliding is the arm. This is distinct from the functionless-replace shape already listed: `bool_box_truth_lookup`'s arm has no function but does write a result. Assisted-by: Claude * majit-translate: re-anchor pyre-side comment refs to symbols Replace `file.rs:NNN` citations in codewriter/ and annotator/ comments with the symbol that owns the cited code, verified by opening each target. Upstream `.py` line citations are untouched. String-literal occurrences (assertion and panic messages) are untouched. Also drop the internal tracking labels `Z2.5 Path C`, `Phase I3` and `F2 followup`, the filename-less `(line ~3273)` pointer, and replace insns.rs's "documented at the const-table site above" with the fact that byte 18 now houses `BC_GOTO_IF_NOT`. Four citations are left as-is because their target no longer exists: `build_flow.rs:215` (file deleted with the syn-AST front-end) in call.rs and codewriter.rs, and `parse.rs:314-318` (parse.rs shrank from ~2000 to 98 lines) twice in call.rs. Assisted-by: Claude * pyre-jit: re-anchor comment references from line numbers to symbols Replace every `file.rs:NNN` citation in pyre/pyre-jit comments with the symbol that lives at the cited location, or with the bare filename where the surrounding text already names the symbol. Upstream `.py` citations (rpython/pypy/lib-python) are untouched. Also replace directional cross-references ("see below", "see comment above", "see the deferral below") with the named symbol they point at, drop the internal "Slice α-2" and "Phase L2" markers, and drop stale self-file line refs ("line 1891", "line 2120-2122", "at line 1495"). Comment-only; no code, string literal or test data changed. Assisted-by: Claude * optimizeopt: re-anchor pyre-side comment refs to symbols Replace `file.rs:NNN` / `symbol:NNN` citations in optimizeopt comments with the symbol that lives there. Upstream `.py:NNN` citations are unchanged. - `propagate_from_pass_range:3336-3339` and `Optimizer::emit_operation:3524-3528` / `:3527` drop their line ranges; both symbols had moved (4582 / 4816). - `dispatch_emit:2631/2766` in heap.rs and virtualize.rs names no existing symbol; replaced with `emit_residual_call` / `handle_side_effects`. Strip internal tracking labels from comments: `Cat-2.2`, `Path A`, `Post-S0`, `S11`, `S7`, `S8`, `E5b`. GitHub references (`#9`, `#115`, `#160`, `#175`) and the `PYRE_S9_PROBE` env-knob name are kept. Make cross-reference pointers self-contained: "see comment above", "see doc comment above", "see the closure above", "see field doc", "same evidence as the args loop above", "for the reason given in the field loop above", "same rationale as raw fields above", "see the Virtual arm above", "the arms below", "the guard below" now state the load-bearing fact or name the owning symbol. Repair two sentences left dangling by previously stripped refs (`optimizeopt/mod.rs` setinfo_from_preamble, `virtualstate.rs` visit count). Comment-only: no code, string literal, or test data changed. Assisted-by: Claude * pyre-jit-trace: re-anchor pyre-side comment refs to symbol names Replace `<file>.rs:NNN` line citations in pyre-jit-trace comments with the symbol that lives at the cited location, after opening each target. Upstream `rpython/`, `pypy/` and `lib-python/` `.py:NNN` parity citations are left unchanged. Where the cited pyre-side location holds no nameable symbol, or the file/function no longer exists, only the `:NNN` is dropped and the filename kept. Strip internal session tracking labels (`B3`, `C3 S1`, `E1`, `G0`/`G1`/`G2`, `Epic G`, `gap 10 slice 2b`, `P2 drain`, `P3`, `Route C`, `Task 8`, `increment 2b`, `STEP 5`) from the comments that carried them; GitHub issue references (`#32`, `#73`, `#171`, `#203`, `#215`, `#62`/`#23`) are kept. Also make four cross-reference comments self-contained by stating the fact instead of pointing at another comment ("see the module preamble", "see above", "the `current`-frame pattern", "see `history.rs`"). Comment-only: no code, string literal or test data is modified. Assisted-by: Claude * docs: correct three comments refuted by their own call sites Each claimed a capability was missing; each is contradicted by the code it sits next to. `descr.rs`'s tag block says the Field tag is load-bearing for a synthetic `FieldIndexDescr` that unpacks offset/size/type/signed out of the index bits. That descriptor and its helpers were deleted — `majit-ir`'s descr module records the removal — and `VirtualizableFieldState.fields` is keyed by `FieldDescr::index_in_parent()` now (`info.py:203-206`). Nothing decodes the tag; what it still buys is disjoint index ranges so two descr kinds cannot collide on one `HeapCache` key. The `ptr_eq/rr>i` opcode-table row says the `b1 is b2` fast path is omitted, "same rationale as int comparisons". Both handlers implement it: `binop_ref_to_int_record` answers an identical operand pair out of `fastpath_same_boxes` without recording, and so does `binop_int_record`. The `raise` arm says resume-data capture is omitted, pointing at the `goto_if_not/iL` arm, which carries no such comment. The guard this arm emits calls `walker_capture_snapshot_for_last_guard(ctx, op.pc)` twelve lines below, which is `generate_guard`'s `resumepc=orgpc`. Assisted-by: Claude * descr: mark PyCode.co_firstlineno immutable, per _immutable_fields_ `pycode.py:95-106` lists `co_firstlineno` in `_immutable_fields_`; the PyCode descr group marked every field mutable because its spec builder hard-coded the flag. Give the builder the flag as a parameter and set it from the upstream list: only `co_firstlineno` changes. `co_name` and `hidden_applevel` are absent from that list and stay mutable — `w_name` is realized lazily by `w_code_name_obj` and does go null -> non-null after construction — and `code_ptr` is the raw body pointer with no upstream slot. The slot really is write-once: `box_code_constant_with_firstlineno` writes it onto an object `box_code_constant` has just boxed out of a fresh `Box`, so no caching lets a reader see it first, and `code.replace` reads it and builds a new code object rather than writing this one. No behaviour change is expected or observed. The only trace-side reader of a field descr's `is_immutable` is the replay-cleanliness rule in `fbw_state.rs`, which fires on a `setfield_gc` into a freshly allocated object, and traced Python never constructs a PyCode. `check.py --no-build --backend dynasm`: 441/441, no jitstats delta. Assisted-by: Claude * majit-metainterp: replace pyre-side line refs in comments with symbol names Comment-only change across `majit/majit-metainterp/src/` (excluding `src/optimizeopt/`) and `majit/majit-metainterp/tests/`. - Rewrite `<file>.rs:NNN` citations of pyre's own Rust sources to name the symbol that lives there, or drop the line number when the surrounding prose already names it. Line refs inside string literals and inside ```text panic transcripts are left untouched. - Upstream `.py:NNN` citations (rpython/, pypy/, lib-python/, lib_pypy/) are unchanged. - Strip session-local tracking tags (Slice X-D/X-G/X3-E/QQ-n/P3/T-final, Sub-slice B/C.x, F.n-orthodox, M2 Step n, Box Identity Phase E Step n, #19 Step n, Step 2e.2b, P1.5) from comment prose. - Replace "see above"/"see the header"/"same rationale as" pointers with the fact plus the symbol that holds the rest. Assisted-by: Claude * majit-translate: replace pyre-side comment line refs with symbol names Strip `file.rs:NNN` / `file.rs:NNN-MMM` line numbers from comments in majit/majit-translate/src (excluding codewriter/ and annotator/) and majit/majit-translate/tests, keeping the symbol name the comment already cited or naming the enclosing item where the citation had none. Upstream `.py` citations are untouched. Also: - qualify ambiguous bare `model.rs` references to `flowspace/model.rs` or `annotator/model.rs` where the named symbol resolves there - drop self-referential filename parentheticals in rclass.rs, rpbc.rs, rtyper.rs, mir.rs, flowspace_adapter.rs, cutover.rs and rbuiltin.rs - point flowspace_adapter.rs's exc_from_raise cross-reference at the "TODO: `Constant` SSA carrier shape" section that exists in that module preamble - remove the "slice A" / "Slice C" tracking labels from flowspace_adapter.rs and llinterp.rs Comment-only: no code, string literal or test data changed. Assisted-by: Claude * pyre-interpreter: replace pyre-side line-number comment refs with symbol names Rewrite `<file>.rs:NNN` citations in comments under pyre/pyre-interpreter to name the symbol at the cited location instead of a line number, or drop the line number where the symbol was already named. Citations against the pinned rustpython-compiler-core and rustpython-sre_engine snapshots (`oparg.rs`, `bytecode/instruction.rs`, `string.rs`, `engine.rs`) are left as they are. Also replace directional cross-references ("see above", "see below", "the note above", "as noted above") with the fact or the owning symbol, and drop the "B1" tracking prefix from a jit_fnaddr comment. Comment-only; no code, string literal or test data changed. Assisted-by: Claude * Re-anchor pyre-side comment references to symbols Replace `<file>.rs:NNN` line citations in comments across majit-backend-{dynasm,cranelift,wasm}, majit-macros, majit-ir, majit-gc and pyre-object with the file name alone, or with the symbol that the cited line's enclosing item defines where the cited location still matches the comment's claim. Upstream `rpython/`, `pypy/`, `lib-python/` and `lib_pypy/` `.py:NNN` citations are unchanged, as is the `compiler.rs:12884` reference inside the `bridge_cache_addrs` expect string. Symbols named where verified: `bh_call_r` / `bh_call_f` / `bh_call_v` default trait impls, `gc_rewriter`, `do_compile`, `emit_guard_exit`, `cranelift_realloc_frame`, `dynasm_typeid_subclass_range`, `generate_state_fields_jit_state`, `generate_trace_fn`, `handle_new`, `gen_malloc_nursery`, `gen_write_barrier`, `handle_write_barrier_setfield`, `do_collect_nursery`, `rescan_major_nonstack_roots_and_drain`, `register_active_hooks`, `CompiledLoopToken`, `next_op_can_accept_cc`, `AbstractVirtualPtrInfo`. Replace three cross-reference pointers with the fact they pointed at: the `write_float_at_mem` "see read sibling above", the `reg_write_audit` "see the module doc", and the wasm `stamp_and_publish_label_targets` "the comment below". Drop the internal tracking ids `S-11`, `Phase E.3+`, `Slice 80-G.7` and `Pre-A.2.3` from the comments they appeared in. Assisted-by: Claude * fbw: record the measured mechanism behind the loop-bearing blackhole handoff decline The `walk_abort_adopted` deny-list arm for `LoopBearingCalleeInlineUnsupported` carried a comment saying its second blocker was open and that dropping the arm produces wrong code, without naming a cause. Measured it on both failing fixtures and replaced that paragraph with the mechanism. `bhimpl_jit_merge_point` treats a frame that has a `nextblackholeinterp` as the recursive portal level: it takes `bhimpl_recursive_call_*`, parks the result in `tmpreg_*` and raises `LeaveFrame`. A multi-frame image stacks the callee above its caller, and this decline reports that the callee bears a loop, so the callee reaches its own loop-header merge point before any `*_return` and the caller below it receives `tmpreg_*` as the callee's return value. Both fixtures leave at that opcode with `ret_type=Ref`: `inline_subwalk_user_iterator` on `[run@1054, step@260]`, and `list_append_write_barrier_gc` on `[big_live_len_regrow@1191, churn@162]`. Comment only; no behaviour change. Assisted-by: Claude * fbw: name the caller-image refusal, the vref bracket's size, and the two qmut decline causes Three diagnostics, all gated on fbw_debug_abort_enabled, no behaviour change. capture_inline_parent_blackhole answered None from its three liveness-pass early returns without printing anything, so the downstream "parent.blackhole None (capture missing)" could not say which bank, which color, or whether the walk's shadow was merely too short. Added report_caller_image_decline and wired it into the int, ref and float sites. Each site changed from `...get(color).copied()?` to an explicit `let Some(..) = got else`, which separates an out-of-range color from one whose shadow holds a different concrete kind; the `?` conflated them. The vref bracket's two halves iterate virtualref_boxes and nothing reported its length, so its size was only ever restated from which call sites populate it. Print the pair count above vrefs_before_residual_call. The qmut flush leg printed one decline message for both WalkEndResume variants that can be unprovable. Split it: RewindUnproven means no opcode-entry sample was taken, a still-unprovable Rewind means the opcode had already applied an effect. Assisted-by: Claude * fbw: correct two docs that call the vref bracket's loops empty Both said the bracket's loops are empty because no jit.virtual_ref producers exist. A producer does exist and runs: walker_ec_enter takes a vref of every seeded callee frame through TraceCtx::opimpl_virtual_ref, paired with opimpl_virtual_ref_finish when the frame leaves. Measured with the [vref-bracket] report over 431 synth + 93 parity fixtures: 5487 bracket entries, 686 of them (12.5%) with at least one pair, 66 of the 316 emitting fixtures reaching a nonzero count, maximum 7 pairs. Assisted-by: Claude * docs: name the produced-view source PreviewShortState::short_boxes `produced_short_boxes_from_exported_boxes`'s header still cited `ctx.exported_short_boxes`, a field removed when the preview export was collapsed into `PreviewShortState`. Point it at the surviving field. Assisted-by: Claude * descr: state PyCode field purity per field in its spec test `pycode_field_descrs_share_parent_and_preserve_specs` asserted `!descr.is_always_pure()` for every PyCode field. Marking `co_firstlineno` immutable made that field answer true — `is_always_pure()` returns the `immutable` flag — so the test has been failing since that change; its commit verified with `check.py` only, which does not run crate unit tests. Add the expected purity to the per-field tuple and compare it, so each field states its own answer and a move in either direction fails. Also re-point one comment in optimizer.rs at `exported_short_boxes`, the name the local kept after the preview-export collapse. `cargo test --release -p pyre-jit-trace -p majit-metainterp --features dynasm`: rc=0. Assisted-by: Claude * fbw: answer a caller image's unstamped ref color instead of refusing the image `capture_inline_parent_blackhole`'s liveness pass demanded a `ConcreteValue::Ref` for every ref color live at the resume pc and returned `None` for the whole image otherwise. The innermost-frame fill `build_single_frame_miframe` (residual_call.rs) runs the same pass and answers the two ways that demand fails: * a live color whose register holds no box is skipped — a `-live-` set is the union over the paths INTO its coordinate, so a color can be live there and undefined on the path walked, and `_copy_data_from_miframe` (`blackhole.py:1711-1730`) likewise leaves a `None` box unset; * a color whose shadow is `ConcreteValue::Null`, the walker's untracked sentinel that `write_ref_reg` stamps for every recorded-but-unobserved result, is recovered through `TraceCtx::recover_ref_value`. Port both. The image is still refused when neither applies. Measured with a report added at the refusal site, over 431 synth + 93 parity fixtures (dynasm, darwin): 21 refusals across 10 fixtures, all bank `r` with a `Null` shadow, partitioning as 11 no-box and 10 recoverable and 0 neither. After the change the corpus reports none. The report itself stays, extended with the box and its recoverability, and the header's decline count is corrected: it cited the downstream `[s2-build-decline]` symbol, which prints only when a multi-frame build was attempted and so undercounted tenfold. `check.py --no-build`: dynasm 441/441, cranelift 441/441, wasm 434/434, no jitstats delta. `cargo test --release -p pyre-jit-trace -p majit-metainterp --features dynasm`: rc=0. Assisted-by: Claude * fbw: route the hazardous-inline deny through disable_noninlinable_function `fbw_abort_nested_unjournaled_residual` names the callee an abort is attributable to and denies it, but the deny wrote only the walker-local `FBW_HAZARDOUS_INLINE_DENY` thread-local, so the callee's JitCell never carried `JC_DONT_TRACE_HERE` and no warm-state reader saw it. It now also calls `disable_noninlinable_function` on `make_green_key(callee_code, 0)`, the function-entry key `inline_call.rs` already uses for that callee — the same answer `pyjitpl.py:2818-2828` gives for the callee `find_biggest_function` names. The consuming half — `warmstate.py:485-496`, where a `JC_DONT_TRACE_HERE` cell that has never seen a procedure token retraces at once instead of waiting out the counter — is already carried by `WarmEnterState::maybe_compile_decision`. Measured (dynasm): the three fbw witness fixtures now mint the denied callee's cell, cells 3 -> 4 on each. `get_stats` counts the `BaseJitCellState` enum rather than the flag, so its `dont_trace_here` reads 1 only on `wasm_ca_trampoline_decline`; on the two `foriter_exempt_*` fixtures the new cell has already moved on to tracing (tracing 0 -> 1) and the state no longer names the deny its flag still records. `list_append_write_barrier_gc` gains a compiled loop (loops_compiled 12 -> 13), re-recorded on all three backends; the wasm baseline is from a measured wasm run. Assisted-by: Claude * fbw: name which hazard clause denied the nested-residual inline `fbw_inline_callee_hazardous` fires on three clauses and returned only the callee's code key, so the `PYRE_LB_SITE=1` `[lb-arm]` line could say `hazard=true` and nothing more. It now returns the clause name alongside the key and the report prints it: `hazard=repeat`, `hazard=for-iter`, `hazard=self-recursive`, or `hazard=false`. The clauses are not equally tight. `repeat` and `self-recursive` name the frame that is actually recursing; `for-iter` is `code_has_for_iter`, which fires on any code object whose bytecode contains a `FOR_ITER` anywhere, whether or not an iterator is in flight at the decline point. Census over 441 synth + 83 parity fixtures: for-iter 8 fires / 6 fixtures, self-recursive 2 / 2, repeat 2 / 2. Only two of the six `for-iter` fixtures are the witnesses that clause documents. Same denial set as before — the three clauses are checked in the same order and return the same key. check.py --no-build --backend dynasm: 441/441. cargo test --release -p pyre-jit-trace: rc=0. Assisted-by: Claude * warmstate: drop the DontTraceHere state and count the denial off its flag `JC_DONT_TRACE_HERE` had two representations: the flag, and a `BaseJitCellState` variant. Every real decision already read the flag — `can_inline_callable`, `counter_tick_checked`, `should_start_dont_trace_here_trace`, and `should_remove_jitcell` — while `is_compiled` and `is_tracing` read the token and `JC_TRACING`. The state variant reached only `get_stats`, and the two answers disagreed: `disable_noninlinable_function` set the state only when `JC_TRACING` was clear, so a cell denied on its way into a trace carried the flag but never took the state, and the census counted zero denials on every fixture that reaches the fbw hazard arm. The flag is now the only representation. `get_stats` counts it directly and independently of the lifecycle state, which is what makes a denied-then-tracing cell visible; `is_dont_trace_here` reads it; the two `state == DontTraceHere` tests in `counter_would_fire` and `counter_tick` were unreachable behind the flag test on the line above and are gone. warmstate.py has no such state either: `JC_DONT_TRACE_HERE` is orthogonal to the lifecycle — a denied cell still traces, compiles, and is invalidated, and `warmstate.py:485-496` retraces it once its procedure token dies. So the abort paths now leave `BaseJitCellState::NotHot` and set the flag alone, which also collapses `abort_tracing`'s three branches into the single condition `abort_tracing_for_key` already used. cargo test --release -p majit-metainterp --features dynasm: rc=0. Assisted-by: Claude * fbw: record that narrowing the for-iter hazard clause is wrong code `fbw_inline_callee_hazardous`'s `for-iter` clause is deliberately loose — it fires on any callee whose bytecode contains a `FOR_ITER`, in flight or not — and the census this branch added shows it carrying 8 of the 32 declines across 441 synth + 83 parity fixtures for 2 witnesses. Narrowing it to "a consume already ran in this frame" is measurable and wrong. `FBW_FORITER_INFLIGHT` answers that question without the per-frame Python pc `InlineFrame` lacks, since its `Jit` entries carry the `jitcode_index` each consume ran in, and it does cut the clause to 3 fires with both witnesses still declining. But `foriter_exempt_shared_generator` then produces wrong output on all three backends, `inline_subwalk_user_iterator` regresses (loops_aborted 1 -> 5, fbw_rolled_back_with_effects 0 -> 5, loops_compiled 3 -> 2) and `list_append_write_barrier_gc` loses its compiled loop again (13 -> 12). The witness still declined, just at pc 533 instead of 261: inlining the residual is what carries the walk to the consume, so a test conditioned on the consume having happened is always one step late. The clause has to stay forward-looking, and a real narrowing needs FOR_ITER reachability from the frame's current position — which is where the missing per-frame pc actually bites. Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #20
#[should_panic]tests on#[cfg(debug_assertions)](set_forwarded_to_self_panics_in_debug, make_result_of_lastop_panics_on_recorded_resulttype_mismatch, do_recursive_call_panics_when_portal_runner_adr_is_zero, assert_no_exception_panics_when_value_set, builtin_func_for_spec_rejects_mismatched_extra_extrakey): their assertions aredebug_assert*!and elided in release, so the expected panic never fires.assert_no_indirect_call_targetscall in rpbc.rs tests on#[cfg(debug_assertions)]to match the function's own gating, fixing the release-mode build error.Summary
Self-review
Prompt & Model
Model:
Prompt:
Answer
Summary by CodeRabbit
Tests
Bug Fixes