jit: stop replaying an opcode's stack effect when the after-residual reconcile names the coordinate the mirror already holds - #1271
Conversation
`MAJIT_BH_ROOT_CHECK` reports a register bank resized while a root registration still names its buffer. `push_resume_ref_roots` and `push_bh_regs` root a bank by the raw `(pointer, length)` of its `Vec` buffer and document the same precondition — the bank is sized once and only indexed afterwards. `ref_bank_registration_len` answers whether that still holds for a given buffer, across both stacks. `MAJIT_BH_CALL_ARGS` reports a residual call's ref arguments by the register index each came out of, plus `num_regs_r` and the bank length, so an argument can be told apart as resume-seeded, written by an opcode of this run, or read out of the jitcode's constant pool. `MAJIT_BH_VABLE` reports a virtualizable array read by its index register rather than only the index value, plus the resolved array and its length. `handler_getarrayitem_vable_r` takes the index from `registers_i`, and an index register the resume section never named reads whatever `setposition` left there; zero is in bounds, so the existing bounds assert stays silent. All three follow the `MAJIT_GC_BH_PROBE` / `MAJIT_BH_NULL_ARG` shape: a `OnceLock`-cached env read and an `eprintln!`, off by default. Assisted-by: Claude
…reconcile names the coordinate the mirror already holds `capture_resumedata(after_residual_call=True)` advances the walk-level operand-stack mirror to the post-call boundary before the guard supplies the virtualizable snapshot. The advance ran unconditionally, including when the resume `py_pc` equals `vstack_cur_pypc` — the coordinate the mirror is already at. `step_vstack_mirror` returns on that same equality, so there is no boundary there and `reconcile_vstack_at_boundary` replays the current opcode's stack effect a second time. `VstackOpClass::Swap` and `Copy` are permutations: applying `SWAP i` twice is the identity. In `dataclasses._process_class` the comprehension `[f for f in fields.values() if f._field_type in (_FIELD, _FIELD_INITVAR)]` lowers to `LOAD_FAST_AND_CLEAR; SWAP 2; BUILD_LIST 0; SWAP 2; FOR_ITER`, and the `BUILD_LIST` residual's guard reconciles at the second `SWAP`'s own py_pc. The mirror then held operand slots 1 and 2 in their pre-swap order for the rest of the walk, and every later guard snapshot published the accumulator list and the iterator crossed in the virtualizable array. On guard failure `LIST_APPEND` read the iterator out of the accumulator slot and `w_list_append` segfaulted. Gate the advance on `py_pc != ctx.vstack_cur_pypc`, which is what "the same transition the following walk step would make" already meant. Assisted-by: Claude
This reverts commit fb8972dfb4cf3ea7412ffb3785fea270e75aea3e.
|
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 (1)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 0 remain after this review. WalkthroughThe residual-call resume path now guards vstack reconciliation with a Python PC comparison. Matching PCs no longer apply the same opcode stack permutation twice. ChangesResume reconciliation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The fix prevents duplicate JIT stack-effect replay that can corrupt resumed operand-stack ordering and crash affected executions, and the reported platform suites pass; however, required dynasm-enabled Rust checks were not run, so merge readiness remains incomplete until they pass or the omission is explicitly accepted. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 766471a). 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
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 766471a4d6
ℹ️ 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".
| // idempotent: a `SWAP` applied twice is the identity, so the two | ||
| // slots it exchanged keep their pre-swap order for the rest of the | ||
| // walk and every later guard snapshot publishes them crossed. | ||
| if after_residual_call && ctx.vstack_valid && py_pc != ctx.vstack_cur_pypc { |
There was a problem hiding this comment.
For a same-PC after-residual capture that still needs depth normalization or hole filling, this condition skips the entire reconciliation rather than only the duplicate opcode effect. reconcile_vstack_at_boundary already suppresses a repeated SWAP via repeat_boundary, while deliberately retaining box resizing, shadow hole filling, vstack_depth publication, and vstack_last_ref clearing; bypassing it can therefore leave stale mirror state that the next real boundary uses to build later guard snapshots. Keep the same-coordinate handling inside the reconciler and suppress only the effect that must not be replayed.
AGENTS.md reference: AGENTS.md:L309-L311
Useful? React with 👍 / 👎.
1 participant
test_dataclassesSIGSEGVs under the JIT on linux/arm64 (clean on darwin). It iswrong code, not a GC fault: at the crash the virtualizable array holds two
adjacent operand-stack slots transposed, both objects live and well-formed.
capture_resumedata(after_residual_call=True)advances the walk operand-stackmirror (
ctx.vstack_boxes) to the post-call boundary withreconcile_vstack_at_boundary. It ran even when the resumepy_pcequalledctx.vstack_cur_pypc— the coordinate the mirror already holds.step_vstack_mirrorreturns on exactly that equality, so there is no boundarythere, and reconciling replays the current opcode's stack effect a second
time.
VstackOpClass::Swap/Copyare permutations, so the second applicationis the identity: the two slots stay in pre-swap order for the rest of the walk.
dataclasses._process_class'slowers to
GET_ITER; LOAD_FAST_AND_CLEAR f; SWAP 2; BUILD_LIST 0; SWAP 2; FOR_ITER. TheBUILD_LISTresidual's guard reconciled at the secondSWAP'sown py_pc, so every later guard snapshot published the accumulator list and the
iterator crossed in the virtualizable array. On guard failure
write_from_resume_data_partialwrote them crossed intolocals_cells_stack_w, the blackhole'sLIST_APPENDread thedict_valueiteratorout of the accumulator slot, andw_list_append→switch_to_object_strategysegfaulted.PYRE_VSTACK_DIAG=1names it directly — the sameprev_pypcreconciled twice:The fix adds
py_pc != ctx.vstack_cur_pypcto that advance.Verification
test_dataclassesRan 276 tests … OK (skipped=2)); SIGSEGV 3/3 beforetest_dataclassescheck.pyMeasured at base
9dbc228b34e; the branch has since been rebased ontoe27b07851c2, whose only delta is unrelated.Two reds seen during verification are inherited, not this branch's:
wasm synth/short_circuit_value_kept_stack5.3–5.5x > gate 3.7x — the gate istuned to a ubuntu measurement (3.2x) and CI runs wasm only on ubuntu; an
origin/mainin-place control arm on darwin measured 5.7/6.1x for the samerow.
test.test_urllib2IMPORTERROR — reproduces withPYRE_NO_JIT=1,and the container clone is pinned to a base that predates its fix
(
bcbd4b26b32).No regression fixture: a hand-written comprehension with the same bytecode shape
does not reproduce on either platform, so coverage rests on
test_dataclassesin the CPython suite gate.
The first and last commits are an add/revert pair for the three env-gated
blackhole diagnostics used during the investigation; they cancel out.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PuYePQknDcMCUsy8omQ1fh
Summary by CodeRabbit