jit: reject side-effecting FOR_ITER replay exemptions - #1173
Merged
Conversation
Require the canonical object.__class__ descriptor on isinstance miss paths, and exact builtin scalar instances for str/GetIter replay safety. Add fixtures that reproduce the former N+1 side-effect replay against the PyPy oracle.\n\nAssisted-by: Codex
|
Warning Review limit reached
Next review available in: 25 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 (10)
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 003e14a). 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 13, 2026
… premise" This reverts commit 9d480a6. Gating the arm on `fbw_foriter_any_body_effect_signal()` moves two `#1173` fixtures the same way it moves `type_name_attr_fold`: `loops_aborted 1 -> 0`, `loops_compiled 2 -> 1` on dynasm and cranelift. bench/synth/foriter_isinstance_class_property_replay.py bench/synth/foriter_str_subclass_replay.py Both bodies commit an effect (`hits[0] += 1`) before their trailing opaque `id` call, so the arm steps aside and the sub-walk no longer aborts. Their stdout is unchanged; what changes is that neither fixture reaches a replay any more. Each was written to detect a replay-safety predicate invoking its subject twice (`N + 1` hits), so re-recording those baselines would leave two wrong-code detectors in the tree with nothing left to detect. The premise the reverted commit tests is a real question, and the arm it narrows has no PyPy counterpart — `do_residual_call` runs the residual at any framestack depth. Answering it needs a fixture that witnesses the double it claims, and fixtures for the replay path those two currently cover; that does not belong in this branch. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 13, 2026
… premise" This reverts commit 9d480a6. Gating the arm on `fbw_foriter_any_body_effect_signal()` moves two `#1173` fixtures the same way it moves `type_name_attr_fold`: `loops_aborted 1 -> 0`, `loops_compiled 2 -> 1` on dynasm and cranelift. bench/synth/foriter_isinstance_class_property_replay.py bench/synth/foriter_str_subclass_replay.py Both bodies commit an effect (`hits[0] += 1`) before their trailing opaque `id` call, so the arm steps aside and the sub-walk no longer aborts. Their stdout is unchanged; what changes is that neither fixture reaches a replay any more. Each was written to detect a replay-safety predicate invoking its subject twice (`N + 1` hits), so re-recording those baselines would leave two wrong-code detectors in the tree with nothing left to detect. The premise the reverted commit tests is a real question, and the arm it narrows has no PyPy counterpart — `do_residual_call` runs the residual at any framestack depth. Answering it needs a fixture that witnesses the double it claims, and fixtures for the replay path those two currently cover; that does not belong in this branch. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 14, 2026
… premise" This reverts commit 9d480a6. Gating the arm on `fbw_foriter_any_body_effect_signal()` moves two `#1173` fixtures the same way it moves `type_name_attr_fold`: `loops_aborted 1 -> 0`, `loops_compiled 2 -> 1` on dynasm and cranelift. bench/synth/foriter_isinstance_class_property_replay.py bench/synth/foriter_str_subclass_replay.py Both bodies commit an effect (`hits[0] += 1`) before their trailing opaque `id` call, so the arm steps aside and the sub-walk no longer aborts. Their stdout is unchanged; what changes is that neither fixture reaches a replay any more. Each was written to detect a replay-safety predicate invoking its subject twice (`N + 1` hits), so re-recording those baselines would leave two wrong-code detectors in the tree with nothing left to detect. The premise the reverted commit tests is a real question, and the arm it narrows has no PyPy counterpart — `do_residual_call` runs the residual at any framestack depth. Answering it needs a fixture that witnesses the double it claims, and fixtures for the replay path those two currently cover; that does not belong in this branch. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 14, 2026
* jit: test the DeferredCall nested-residual abort's write-free premise `fbw_abort_nested_unjournaled_residual`'s FOR_ITER-body arm aborted whenever a Python-level CALL residual inside a `CalleeReplaySafety::DeferredCall` sub-walk failed to inline, on the premise its own comment states: "every op the sub-walk has run so far is write-free, so the resume re-runs the body benignly". The static scan behind that premise clears direct heap writes, not the residual CALLs it leaves behind, and one of those can mutate live heap before the call that fails to inline is reached. `PYRE_FBW_DEBUG_ABORT=1` shows the arm firing after `[fbw-foriter] body effect committed since consume`, with `effects=1` on the abort: the rollback it falls back to cannot undo that write, and the replay re-runs the body over it. Gate the arm on `fbw_foriter_any_body_effect_signal()` — the predicate `fbw_foriter_inflight_take` already refuses delivery on — through `fbw_foriter_deferred_inline_with_free_abort`, and add a unit test that the arm owns the abort before a commit and steps aside after one. With the premise broken the general depth->=2 nested-residual rule applies instead and the residual executes, the way `do_residual_call` runs it at any framestack depth (`pyjitpl.py`). synth/type_name_attr_fold moves loops_aborted 1 -> 0 and loops_compiled 5 -> 4 on all three backends. The abort denied the FOR_ITER callee, which then compiled as its own trace; without the deny it inlines into the enclosing one. MAJIT_LOG: a separate 42-op trace and a 113-op trace become one 152-op trace, with abrt_bridge 1 -> 0, caro_funcentry 2 -> 1 and guard_failures unchanged. Assisted-by: Claude * test(pyre-jit-trace): gate the static-vable mirror's flush to the live frame `mirror_vable_static_to_boxes` pairs its `set_virtualizable_entry_at` with `synchronize_virtualizable()` (1c52b1fe54b, #1128), the `_opimpl_setfield_vable` shape at pyjitpl.py:1189-1194. Deleting that call leaves the whole tree green: - `cargo test --all --no-default-features --features dynasm`: 7599 passed, 0 failed, including `class_body_and_exec_loops_keep_the_vable_shadow_synchronized`, the test added with the flush. - all 397 `pyre/bench/synth/*.py` run under a debug `pyre-dynasm` without reaching `check_synchronized_virtualizable`'s assert. The search was live: 20 of the first 25 fixtures compile loops, and `class_body_exec_hot_loop.py` reports loops_compiled=4, bridges_compiled=1, guard_failures=403. The behavioural consequence is that assert, which is `debug_assertions`-only while check.py and the bench suite run release binaries, and no input on this tree reaches it through this helper. So gate the pairing itself: seed a frame-shaped buffer with `last_instr = 11`, mirror `37` into the shadow, and read the field back off the buffer. Asserts the frame holds 11 before the mirror, so the check cannot pass by the field already carrying the published value. Measured both ways on this tree: passes with the `synchronize_virtualizable()` call, fails `left: 11, right: 37` without it. `set_virtualizable_heap_ptr` is called explicitly — `init_virtualizable_boxes` seeds the boxes and the identity but not the synchronization target, and `synchronize_virtualizable` returns early without it. Assisted-by: Claude * test(cpython): stabilize resource-heavy suite gates * address PR review feedback * address follow-up PR review feedback * for_iter gate: admit LIST_APPEND bodies that contain a call `for_iter_body_is_jit_safe_at` scanned each FOR_ITER body for `Call`/`CallKw`/`CallFunctionEx`/`CallIntrinsic1` and admitted `LIST_APPEND` only when the scan found none. `SET_ADD` and `MAP_ADD` carried no such condition. Delete the scan and put `LIST_APPEND` on the body allow-list beside them. pyopcode.py:1492 spells LIST_APPEND as `space.call_method(v, 'append', w)`, :1515 SET_ADD as `space.call_method(w_set, 'add', w_value)` and :1525 MAP_ADD as `space.setitem(w_dict, w_key, w_value)`. `rg -l FOR_ITER rpython/jit/` matches no file, and interp_jit.py:117 reaches `can_enter_jit` without inspecting bytecode. The admission test is inverted and now covers `out.append(g(i))` alongside the comprehension spellings. Two region-gate tests switch their declining loop to `len(*item)` / `str(*y)`, whose `CALL_FUNCTION_EX` stays off the allow-list. Add `bench/synth/foriter_listappend_call_consuming_body.py`: a comprehension whose element call drains the list it iterates, so a replayed or dropped body execution fails an assert in either direction. Twenty jitstats baselines move. In each fixture `PYRE_FOR_ITER_GATE_DIAG` names exactly one code object that declines before the change and none after, and a pre-change binary reproduces every one of those baselines' gated counters. `loops_aborted`, `internal_compile_panics`, `fbw_rolled_back_with_ effects`, `fbw_store_journal_rollback_failed`, `field_pos_spec_misplaced`, `field_pos_attached_misplaced` and `descr_set_absent`/`ambiguous`/ `stale_absent` stay at 0 on dynasm and cranelift, and stdout is byte-identical on all three backends. `mapdict_frozen_unboxing_fold` holds loops_compiled=3 by trading a function-entry trace for a backedge loop (caro_funcentry 1 -> 0). wasm moves two further fixtures, each gaining one compiled loop and one tracing abort; on `global_store_plain_dict_globals` that abort is `abrt_force_qmut=1`, the count the native backends already report for it. The raised guard-failure counts are bounded rather than per-iteration: over a 20x sweep of `range_ctor_in_loop`'s N they move 810 -> 832, and over a 40x sweep of `list_append_virtual_payload`'s outer loop 759 -> 1604. Assisted-by: Claude * bench/synth: add the SET_ADD spelling of the consuming-call fixture `foriter_listappend_call_consuming_body.py` covers LIST_APPEND only. `for_iter_body_op_is_jit_safe` admits SET_ADD on the same footing, so add `foriter_setadd_call_consuming_body.py`: the same heapq comprehension with `{...}` in place of `[...]`, draining the heap it iterates so a replayed or dropped body execution trips `len(heap) == 0` in one direction or the other. The three backends record the same counters for it: loops_compiled=6, bridges_compiled=22, guard_failures=3980, loops_aborted=0. Measured ratios 9.3x / 12.6x / 14.7x against the fixture's max-pypy-ratio=20. Assisted-by: Claude * Revert "for_iter gate: admit LIST_APPEND bodies that contain a call" This reverts commit 486868c. Two parity fixtures fail with the widening in place and pass with `pyre/pyre-jit/src/eval.rs` restored to its pre-widening state, rebuilt and rerun on the same base: for_iter_exception_handler_comprehension.py assert run(items) == (120, 60) weakref_gc_lifeline.py assert callback_count == count Both bodies are the shape the deleted scan declined: a comprehension whose element expression calls a user Python function (`[str(make_payload(value)) for value in range(1)]`, `[Target() for _ in range(8)]`, `[Target() for _ in range(count)]`). The deleted comment named the mechanism: a per-element call that enters a user Python frame bumps the eval-loop entry odometer, and a mid-body abort then routes through `fbw_foriter_inflight_take`, which refuses delivery and drops the trace-attempt iteration's item. Both asserts count items, so a dropped iteration is what they read. The widening's evidence was a synth-corpus stdout and jitstats comparison; the parity corpus was not part of it. `bench/synth/foriter_setadd_call_consuming_body.py` stays. SET_ADD carries no CALL condition either before or after this revert, so the fixture still pins a live admission; its header no longer refers to the LIST_APPEND twin the revert removes. Assisted-by: Claude * Revert "jit: test the DeferredCall nested-residual abort's write-free premise" This reverts commit 9d480a6. Gating the arm on `fbw_foriter_any_body_effect_signal()` moves two `#1173` fixtures the same way it moves `type_name_attr_fold`: `loops_aborted 1 -> 0`, `loops_compiled 2 -> 1` on dynasm and cranelift. bench/synth/foriter_isinstance_class_property_replay.py bench/synth/foriter_str_subclass_replay.py Both bodies commit an effect (`hits[0] += 1`) before their trailing opaque `id` call, so the arm steps aside and the sub-walk no longer aborts. Their stdout is unchanged; what changes is that neither fixture reaches a replay any more. Each was written to detect a replay-safety predicate invoking its subject twice (`N + 1` hits), so re-recording those baselines would leave two wrong-code detectors in the tree with nothing left to detect. The premise the reverted commit tests is a real question, and the arm it narrows has no PyPy counterpart — `do_residual_call` runs the residual at any framestack depth. Answering it needs a fixture that witnesses the double it claims, and fixtures for the replay path those two currently cover; that does not belong in this branch. Assisted-by: Claude * cpython_tests: mark ResourceDenied skips with a marker only the driver writes `classify` read any `rc == 0` stdout line starting with `skipped: ` as SKIP. That prefix is not reserved: a module that prints it and then passes was recorded as SKIP. Print `pyre-cpython-suite: ResourceDenied: <exc>` from the two resource drivers and match that prefix instead, stripping it from the recorded detail. `test.test_urllibnet` under `_RESOURCE_MODULE_DRIVER` classifies as `SKIP Use of the 'network' resource not enabled`; `skipped: ...` printed by a passing module now classifies as PASS. Assisted-by: Claude * for_iter gate: correct the body CALL scan's stated mechanism `for_iter_body_is_jit_safe_at`'s comment gave a per-element call entering a user Python frame as the reason a `LIST_APPEND` body can lose its in-flight item, and called a call-free value-producing body "exact-resume safe". Measured on this tree, a call-free body reaches the same loss: for index in items: out.append([index for _ in range(1)]) over 60 items appends 59, at trials 17 and 18 of 400, on pyre-dynasm and pyre-cranelift alike; `PYRE_NO_JIT=1` appends 60. Binding the comprehension to a local first, spelling it `list(<genexp>)`, or writing the inner loop out as a nested `for` are each unaffected. The `SET_ADD` and `MAP_ADD` spellings, which the scan does not cover, lose an iteration the same way. State the refusal path (`fbw_foriter_inflight_take` destroying the stash and leaving the frame at the FOR_ITER header against an already-advanced iterator), record the scan as narrowing how often that is reached rather than as a boundary, and drop the exact-resume claim. Comment only. Assisted-by: Claude * for_iter gate: record the measured route of the dropped outer iteration The LIST_APPEND clause's comment attributed the inlined-comprehension drop to the R1 body-effect refusal in `fbw_foriter_inflight_take`. A census run on the shape reports `DELIVERED=2 REFUSED=0` at body_pc=35 with `store_journal_len=0 unjournaled=false`, so that refusal does not fire here. What the run shows instead: the code object's per-CodeObject JitCode carries a merge point per loop, with the outer loop's `for_iter_next` residual at JitCode pc 153 (walk entry for Python pc 5) and the inner one at 631 (entry for Python pc 34). Both captures record op_pc=153 with an outer item (`intval=1020`, `intval=1002`; the inner loop's item is always 0), and `inflight_foriter_body_pc` resolves 153 to Python pc 34, so the outer item carries the inner loop's body pc. The take then delivers it and `deliver_inflight_foriter_item`'s header check declines the push with `frame.next_instr()=5`, dropping the item. Comment only. Assisted-by: Claude * for_iter gate: keep the exit-state paragraph the rebase's revert overwrote Rebasing onto origin/main replayed "for_iter gate: admit LIST_APPEND bodies that contain a call" and its revert across a region main had rewritten in the meantime. Two texts did not survive that round trip: - The clause's leading paragraph. Main explains the scan by the compiled loop's exit state keeping the last per-element binding reachable, which is what `extra_tests/parity_tests/weakref_gc_lifeline.py` observes. The replayed side replaced it wholesale with the in-flight coordinate finding. Both stand; put main's back first and keep the measured finding as the paragraph after it. - The comment on `for_iter_call_bearing_list_append_comprehension_is_unsafe_for_entry_trace`. The revert restored the pre-widening wording, which attributes the decline to the R1 in-flight refusal; a census on the shape reports `DELIVERED=2 REFUSED=0`. Restore main's wording. Also drops "forcing the refusal" from the `list_write_barrier` exemption note for the same reason. Comment only; `pyre/pyre-jit/src/eval.rs` differs from origin/main in comments alone. Assisted-by: Claude * for_iter gate: scope the dropped-iteration note to the base that shows it The note was written against `c0508da5e32` and states the shape as a present-tense defect. On this base it does not reproduce: the comprehension probe prints 60 of 60 on both backends and the in-flight census records no take at all, across the whole probe family that previously diverged. The cause is `#1174`'s one-line change in fbw_state.rs — the walk raises `callee_inline_blackhole_required` where it raised `callee_inline_unsupported` — so the shape stops aborting and never reaches `fbw_foriter_inflight_take`. The mislabelling itself is untouched: `inflight_foriter_body_pc` (diag.rs) and `containing_py_pc_for_jitcode_pc` (py_coord.rs) are byte-identical across the range, and the dumped JitCode still carries a merge point per loop at 129 and 607. Say "before #1174" and record that the coordinate is unexercised rather than repaired. Comment only. Assisted-by: Claude * jit: select the in-flight FOR_ITER entry the parked frame can accept `fbw_foriter_inflight_take` popped the most-recent stash entry and cleared the rest. One frame can hold two entries at once with no nested frames: an outer `list` loop captures through the residual leg (residual_call.rs, `ForIterNext`) and a `range` body inside it through the specialised leg (specialize.rs, `w_range_iter_next`). `deliver_inflight_foriter_item` can only push at the header its frame is parked on, so a most-recent entry naming the inner body pc fails that header check while the clear has already discarded the outer entry the frame could have taken — neither is delivered. Select with `foriter_body_matches_frame` against the live frame and its resume pc, falling back to the most-recent entry when none matches, so the set of pushes the header check admits is unchanged. The R1 body-effect flag is now read off the selected entry rather than off the top. Also corrects the FOR_ITER-gate note: the resolution it blamed is right. `inflight_foriter_body_pc` answers Python pc 5 for the outer `for_iter_next` (JitCode pc 153) and 34 for the inner (631), measured on both. cargo test -p pyre-jit-trace foriter_delivery_tests: 4 passed. pyre/check.py --backend dynasm: ALL PASSED 426/426. Assisted-by: Claude * jit: treat a null concrete box as unresolved in the caller CALL stack image `concrete_ref_for_opref` answered `Some(null)` for a box whose recorded concrete is `Ref(0)`, so `collect_call_stack_overrides` published that null as a resolved operand-stack slot. Its sibling `concrete_ref_for_color` and the virtualizable-shadow fallback in the same function already answer unresolved there. The outer-call flush writes an override verbatim, so a `LOAD_ATTR name + NULL|self` pair the walk still held symbolically reached the resumed interpreter as a null callable and faulted in `classify_callable`. With the slot absent the flush declines and keeps the legacy replay. Assisted-by: Claude * jit: exempt the exception-carrier clear from the R1 body-effect discriminator `writes_gc_liveness_root_only` was defined at the gh#467 heap-write odometer and read only there, while `body_effect_candidate` counted `ClearInFlightException` through `writes_live_heap`. An in-flight FOR_ITER item consumed in a body containing `try/except` was refused delivery and the iteration dropped. The binding now sits with the discriminator and both sites read it. Add `parity_tests/exception_handler_method_load_resume.py`: on `e6d73aa8fb2` it SIGSEGVs on both backends, with only the preceding commit applied it returns 59 of 60 elements at trial 17. Assisted-by: Claude * jit: take the frame-matched in-flight entry only when the match is unique `foriter_body_matches_frame` compares the frame's `CodeObject` and body pc, not the frame, so two activations of one code answer it identically. Reaching past the top for one of them can deliver another activation's item. The selection now falls back to the legacy top entry whenever a second candidate matches, so it never trades a drop for a wrong value. The decision is `inflight_take_index`, driven directly by the unit test: the ambiguous case cannot be built through `fbw_foriter_inflight_capture`, which replaces an entry carrying the same `body`. Codex parity review §2, `fbw_state.rs:885 ↔ rpython/jit/metainterp/resume.py:1049`. 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.
Summary
isinstancemiss to resolve__class__to the canonicalobject.__class__descriptor before treating it as replay-safestr()and stringGetIterreplay exemptions__class__property andstrsubclass N+1 side-effect replaysPyPy grounding:
p_recursive_isinstance_type_wperformsgetattr(obj, "__class__")after a direct MRO miss, so inheritingobject.__getattribute__alone does not make the miss path pure. PyPy also dispatchesstrand iteration overrides on subclasses; only exact immutable builtins qualify for the no-user-code exemption. Both oracle fixtures execute their side effects exactly 5000 times with zero PyPy aborts. Before this patch pyre executed the__class__property and__str__override 5001 times.The fixtures now produce the correct result on dynasm, cranelift, and wasm. They retain one safe
LoopBearingCalleeInlineUnsupporteddecline; eliminating that abort tax and the mergedload_method_selfopcode gate requires the separate per-callee red-frame/resume-image restoration, not a wider replay exemption.Verification:
cargo check --features dynasmcargo test --features dynasmcargo fmt --checkstr_search_index_boundsregression fixturepolymorphic_binary_receiver: 5 loops, 3 bridges,loops_aborted=0Self-review
AI-assisted implementation and review; the commit includes
Assisted-by: Codex. The patch directly addresses the two correctness findings from the post-merge Codex/parity review of #1169. This implementation session also ran the PyPy oracle and local parity investigation; the repository automatic independent review is pending.Assisted-by