jit: the rest of the walk mirror's unmodelled opcodes, and a CI gate for the roll-back-with-effects class - #1056
Merged
Conversation
…nd-stack mirror `classify_vstack_opcode` fell through to `_ => Unmodeled` for LOAD_BUILD_CLASS, MATCH_SEQUENCE, MATCH_MAPPING, MATCH_KEYS and MATCH_CLASS, which latches `vstack_valid = false` for the rest of the walk. All five push exactly one value through `emit_pushvalue_ref!` -> `setarrayitem_vable_r`, so they take `ResultToTos`. LOAD_BUILD_CLASS shares one codewriter arm with LOAD_LOCALS, whose `abort_permanent` arm is gated on `is_locals && !is_true_portal`, so it always lowers to the residual plus the push. MATCH_CLASS is net -2 and cannot use `MultiResultFromShadow`: that class clears `vstack_depth - 1 .. new_depth`, an empty range for a net-negative opcode, and both hole-fill helpers skip non-NONE slots, so the popped subject would survive in the result slot. A `PYRE_VSTACK_DIAG=1` sweep of the 361 synth fixtures records 34462 reconcile events, of which the Unmodeled ones are 5 MatchSequence in match_sequence_of_class_patterns and 2 LoadBuildClass in slots_class_var_conflict; both go to 0. The remaining opcodes that reach the fallthrough all lower to `emit_abort_permanent!`, so the walk ends inside their own jitcode region and never applies the class. match_sequence_of_class_patterns moves loops_aborted 5 -> 0, loops_compiled 1 -> 2, bridges_compiled 0 -> 2 and guard_failures 1 -> 401 (402 on wasm), identically on all three backends. Assisted-by: Claude
…sidual `fbw_diag` slot 1 (ROLLED_BACK_WITH_EFFECTS, trace.rs) counts walks that ended uncommitted with `effects > 0` — a residual had already written live heap or entered a Python frame, neither of which the store journal undoes, so the legacy replay the caller falls back to applies those effects twice. The counter was computed on every walk and read by no gate. Print it as `fbw_rolled_back_with_effects` from pyrex and from the wasm runner's MAJIT_STATS block, and add it to JITSTATS_BADNESS_FIELDS. The wasm runner already read the same slot, but printed it under a different key and inside the PYRE_WASM_JIT_STATS block, which check.py does not set; `_jit_stats_change` reads a field absent from a run as 0, so that key gated nothing. A baseline field that no run emits also reads as 0, so only the fixtures reporting a nonzero count needed recording: raise_reg_unbound_jitstress (1, end=ResidualCallArgUnbound), recursive_forced_frame_kept_stack (1, end=CompileTracePending) and set_hash_protocol (1, end=CompileTracePending, bridge) on all three backends, plus ca_bridge_multiframe_resume_double_call (1), global_store_plain_dict_globals (1) and pickle_terminal_raise_resume (5) on wasm only. Each count repeats identically across five runs. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (49)
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 68cbada). 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)None. 4. Structural adaptations
|
youknowone
added a commit
that referenced
this pull request
Aug 5, 2026
Both files were recorded against a base that predates #1050, #1056 and #1057. Re-measured after re-extracting LLBC, whose three crate fingerprints had all drifted: exception_inline_callee_tb_frames bridges_compiled 2 -> 3, guard_failures 403 -> 604 gc_bug_bridge_flavor_traceback_names guard_failures 1837 -> 1838 Both also gain the `fbw_rolled_back_with_effects` field the new base adds. The rest of the wasm suite is unchanged at 376 passing. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 5, 2026
Both files were recorded against a base that predates #1050, #1056 and #1057. Re-measured after re-extracting LLBC, whose three crate fingerprints had all drifted: exception_inline_callee_tb_frames bridges_compiled 2 -> 3, guard_failures 403 -> 604 gc_bug_bridge_flavor_traceback_names guard_failures 1837 -> 1838 Both also gain the `fbw_rolled_back_with_effects` field the new base adds. The rest of the wasm suite is unchanged at 376 passing. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 5, 2026
…d the trampoline scratch inside the JitFrame (#1058) * jit(wasm): count a heap load as loop-state advancement in the loop-closing bridge check `compile_bridge`'s livelock shield refuses a loop-closing bridge whose terminal JUMP carries no advancing value. Its advance predicate accepted only integer and float arithmetic, so a pointer chase did not qualify: `tb = tb.tb_next` lowers to GetfieldGcR, which is neither arithmetic nor a heap write, and the bridge that re-reads the link was declined even though each pass reads a different node and walks the loop toward its exit. Seven fixtures carry that traceback walk. Six of them lose their declines, and four land on dynasm exactly (guard_failures wasm -> dynasm): exception_inline_callee_tb_frames 975 -> 403 dynasm 403 exception_traceback_lineno_chain 802 -> 402 dynasm 402 exception_catching_frame_tb_node 601 -> 401 dynasm 401 exception_reentry_guard_finally_residual 2459 -> 2261 dynasm 2261 gc_bug_bridge_flavor_traceback_names 2036 -> 1837 dynasm 1655 exception_traceback_frame_lineno 814 -> 813 dynasm 811 `bridges_compiled` drops alongside on five of them, also onto dynasm's value: the extra bridges were the retraces that a declined guard forced. The shield itself stays. It refuses a bridge that resumes at the loop header with byte-identical state, whose guard then re-fails and spins the loop against the bridge; the resume-at-LABEL dispatch does not address that, and the two were introduced together. Only the predicate widens, so a JUMP built entirely of verbatim input reloads, fresh allocations and baked constants is still refused. check.py wasm 374/374. Assisted-by: Claude * jit(wasm): move the residual-call trampoline scratch out of the JitFrame, and drop the CALL_ASSEMBLER gate it required The trampoline's ABI was frame-relative on both sides of the host hop: the guest stored func_ptr/nargs/args off wasm local 0 and read the result back off local 0 before reloading it, and the host computed `call_area = frame_ptr + call_area_ofs` before re-entering the guest and wrote the result there afterwards. That is sound only for the host-entry frame, which is allocated old-gen and therefore non-moving. CALL_ASSEMBLER breaks it twice. `wasm_jit_ca_alloc_frame` allocates the callee frame in the nursery, so it can move across the hop; and it sizes it `ca_frame_bytes`, which excludes the trailing call area, so a trampoline call on such a frame would store past the object's end. The backend therefore refused to compile any CALL_ASSEMBLER-bearing bridge that shared a trace or token with a trampoline residual call. The scratch is now a module-static array whose address is baked into emitted code, the way the pending-exception cells already are. Frame geometry is untouched: the call area stays reserved in the frame, unused, so frame_bytes, ca_frame_bytes and the module cache do not move. A single shared area is sound because trampoline use is strictly LIFO — the host materialises every argument before invoking the callee and the guest loads its result immediately on return. The trampoline reads its scratch at `base + offset`, and the base-only import has the offset baked host-side, so every emitting module now takes the two-argument import. With nothing left to protect, the gate and its census go: the `has_trampoline_calls` checks on the pending self target, the registered target, the live loop and the redirect, the chained-bridge census, and `ca_reentry_safe`. The BRIDGE_DIAG slots stay; slot 15 stops firing. This was the last terminal bridge decline on wasm. On the only three fixtures that carried it, `decl_shortcircuit` 543/543/89 -> 0/0/0, `decl_callasm` and `decl_ca_trampoline` -> 0, and `entered == BRIDGE_OK` exactly (16, 16, 26): ca_bridge_multiframe_resume_double_call gf 3062 -> 2581 bridges 14 -> 16 recursion_memo_branch gf 3083 -> 2602 bridges 14 -> 16 foriter_call_resume_drops_iteration gf 5182 -> 5165 bridges 23 -> 26 dynasm reports 2592/16, 2613/16 and 5150/27. No other fixture moved. check.py wasm 374/374; cargo test -p majit-backend-wasm green. Assisted-by: Claude * bench: re-record two wasm jitstats baselines on the current base Both files were recorded against a base that predates #1050, #1056 and #1057. Re-measured after re-extracting LLBC, whose three crate fingerprints had all drifted: exception_inline_callee_tb_frames bridges_compiled 2 -> 3, guard_failures 403 -> 604 gc_bug_bridge_flavor_traceback_names guard_failures 1837 -> 1838 Both also gain the `fbw_rolled_back_with_effects` field the new base adds. The rest of the wasm suite is unchanged at 376 passing. Assisted-by: Claude * mapdict: say what the four `?` fields actually rely on in pyre `allow_unboxing`, `ever_mutated`, `attr` and `typ` are declared quasi-immutable upstream, and the comments asserted that flatly. pyre installs no quasi-immutable watcher for any of them — they are plain Cells, and mapdict.rs contains no quasi-immutable code — so the comments read as a property the code does not have. State the substitute instead: every read is paired with a GuardValue on the instance map, emitted by walker_guard_mapdict_instance_shape. The full rationale sits on `allow_unboxing`, the only one of the four that gates a fold decision; the other three refer to it. Same formulation already used for the `Function` `?` fields in descr.rs. Comments only. 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.
Follow-ups to #1051. Two changes: the rest of the walk mirror's unmodelled-opcode set,
and a CI gate for the whole double-execution bug class that #1051's three defects belonged
to.
1.
LOAD_BUILD_CLASSand theMATCH_*opcodes in the walk operand-stack mirror#1051's own follow-up list named
END_SEND,YIELD_VALUE,AEnterandAExit. Thatlist was wrong. All four lower to
emit_abort_permanent!(codewriter.rs:11641, :12920,:12096), so the walk ends inside the opcode's own jitcode region and never crosses the
boundary that applies a
VstackOpClass. Their classification is unreachable — and theearlier A/B that modelled the four await opcodes and moved no counter in either arm was
structurally guaranteed, not workload-specific.
Diffing the 227-variant
Instructionenum against the 97 armsclassify_vstack_opcodenames leaves 130 fallthroughs, of which only 21 are ever emitted and 16 of those are that
permanent-abort family. The real remaining set is five opcodes, all
ResultToTos:LOAD_BUILD_CLASSLOAD_LOCALS(codewriter.rs:12385); theabort_permanentarm there is gatedis_locals && !is_true_portal, so it always lowers to the residual plus the push. A plain omission.MATCH_SEQUENCE/MATCH_MAPPING/MATCH_KEYSGET_LEN/IMPORT_FROM, samepush_and_bump!loweringMATCH_CLASSemit_popvalue_ref!×3 thenpush_and_bump!All five push through
push_and_bump!→emit_pushvalue_ref!→setarrayitem_vable_r,which stamps
vstack_last_ref— exactly whatResultToTosconsumes.MultiResultFromShadowis wrong forMATCH_CLASS. Its pop point is hard-codedvstack_depth - 1, so for a net-negative opcode the clear looppop_point..new_depthisan empty range; nothing is NONEd, both hole-fill helpers skip non-NONE slots, and the
popped
subjectsurvives as a stale box in the result slot. That class is only sound forpop-1/push-N.
Measurement
PYRE_VSTACK_DIAG=1over the 361 synth fixtures, run sequentially: 34462 reconcileevents, of which
class=Unmodeledoccurs in exactly two fixtures —match_sequence_of_class_patterns(5× MatchSequence) andslots_class_var_conflict(2× LoadBuildClass). Nothing else, on any fixture. Both go to 0.
match_sequence_of_class_patternsalso moves, identically on all three backends:The mirror surviving
MATCH_SEQUENCEis what lets that loop compile at all; the guard andbridge rise is the documented consequence of a loop that now has compiled guards.
2. Gate the walks that roll back after an irreversible residual
fbw_diagslot 1 (ROLLED_BACK_WITH_EFFECTS, trace.rs) counts walks that endeduncommitted with
effects > 0— a residual had already written live heap or entered aPython frame, neither of which the store journal undoes, so the legacy replay the caller
falls back to applies those effects twice. All three defects in #1051 were found by
reading this counter's population by hand. The counter was computed on every walk and read
by no gate.
It is now printed as
fbw_rolled_back_with_effectsand is a member ofJITSTATS_BADNESS_FIELDS.The wasm runner already read the same slot — but printed it under a different key, inside
the
PYRE_WASM_JIT_STATSblock, which check.py does not set._jit_stats_changecomparesby field name and reads a field absent from a run as
0, the healthy value, so that keygated nothing. Moving it into the
MAJIT_STATSblock (which already refuses to report amissing export as 0) is what surfaced half the population below.
The population it names — 6 latent instances, recorded, unfixed
raise_reg_unbound_jitstressResidualCallArgUnboundrecursive_forced_frame_kept_stackCompileTracePendingset_hash_protocolCompileTracePending, bridgeca_bridge_multiframe_resume_double_callglobal_store_plain_dict_globalspickle_terminal_raise_resumeEach count repeats identically across five runs, which is what makes them gateable. A
baseline field that no run emits reads as 0, so only these needed recording; the remaining
fixtures took the field at 0 with no other counter moving (audited: the only numeric
movement in the 42 touched
.jitstatsfiles ismatch_sequence_of_class_patterns).Two things worth calling out:
CompileTracePendingis not an undermodelling abort. It is a routine control-flowoutcome, and it reaches the walk-end epilogue uncommitted with effects already applied.
This bug class is therefore not confined to the vstack mirror, which is where all three
of jit: three walk-image defects — locals-typed vable overlay, unpublished escape root stack, unmodeled LOAD_SPECIAL #1051's defects lived.
ca_bridge_multiframe_resume_double_callis named for the symptom.Verification
pyre/check.py: dynasm 379/379, cranelift 379/379, wasm 375/375, 3/3 backends, on thisbase.
test.test_asyncio, which #1017 recorded asTIMEOUT("stalls in about one arm in four"),now runs to completion: 6 arms with
--full, 5 PASS / 1 FAIL / 0 TIMEOUT. The stall isgone; the baseline entry is left alone in this PR because one arm still fails and recording
PASSwould make the gate flaky.Still open
!ctx.trace_ctx.is_bridge_traceat residual_call.rs:3081 still skips the vable-escapeblackhole latch entirely for a bridge walk. Measured population on the asyncio harness:
of 18 forced escapes, 11 are
bridge=true bh=true fs=0 subwalk=false— the largestgroup, larger than the 5 non-bridge shapes that do take the latch. The conjunct entered
as one of seven in a default-off rollout gate and is the last of that set never
individually argued; the adopt machinery contains no bridge term. The reason not to lift
it blind: a bridge shape that passes the pre-drive gates and then loses frame identity
hits
.expect(...)at trace.rs:2572/2616, a process abort rather than a free decline.set_hash_protocol(bridge, in the table above) is the deterministic test — its countershould go 1 → 0.
publish_root_stack: falseand gates on
writes_live_heap && odometer_unchanged, the two gates the comment directlyabove argues against for its sibling. An adversarial review could not establish that this
arm actually reaches a root-frame
getarrayitem_vable_r, so it needs a reachability tallybefore anything is changed.