jit: size vable scalar stores by the target word - #858
Conversation
`read_descr_vable_field` synthesized every vable static field with `field_size: 8`, and the `PYFRAME_DESCR_GROUP` entries for `valuestackdepth` and `last_instr` declared the same literal. Both fields are machine words, not fixed 64-bit integers, and they are adjacent in `PyFrame`, so on a 32-bit target the size-dispatching `Backend::bh_setfield_gc_i` stored eight bytes over a four-byte field and the overrun cleared the `last_instr` behind it. Derive all three widths from `size_of::<usize>()` / `size_of::<isize>()`. The other `Type::Int` entries in the group table are `i64` and keep 8. `frame_lineno_mid_replay_regression.py` reported `plain` as `[0, 4]` on wasm against `[4]` on dynasm, cranelift, pypy3 and CPython; it now passes there, so the guard drops its `skip_backends`. Assisted-by: Claude
The blackhole drive reaches `last_instr` and `valuestackdepth` through `setfield_vable_i` against the frame register, which is the live frame. Both decline arms put the locals back and left those two where the drive had moved them, so the interpreter resumed at the coordinate the blackhole reached, reading the operand stack at a depth the restored locals do not match. No other mechanism covers them: the store journal carries heap effects, and `fbw_exit_last_instr_rollback` arms only on a return or void-return exit, which an unadoptable terminal is not. Adds `capture_frame_scalars` / `restore_frame_scalars` beside the locals pair and calls them from the post-drive decline in both `try_adopt_single_frame_blackhole` and `try_adopt_multi_frame_blackhole`. Assisted-by: Claude
`optimize_FINISH` queued the stashed `GUARD_NOT_FORCED_2` through `emit_extra`, so it went back through the passes after virtualize and was finalized before the FINISH's own emit force_box'd the FINISH args. A return box that was virtual is numbered as virtual there, where `postprocess_FINISH` numbers it materialized. Restores `have_postprocess_op` / `propagate_postprocess` on OptVirtualize and adds the Optimizer half they hand to, which never existed: the pass holds no Optimizer, and both `store_final_boxes_in_guard` and `collect_optimizer_knowledge_for_resume` are Optimizer-side. `drain_pending_finish_guard_postprocess` runs after each postprocess dispatch, finalizes the guard and inserts it at `new_operations.len() - 1`, then rebuilds `new_operations_index`, whose last-occurrence-wins mapping a mid-vector insert cannot maintain incrementally. Assisted-by: Claude
The codewriter paragraph still said a frame observed mid-replay reports the last coordinate published at a frame exit. The `-live-` marker hook publishes at every marker it passes, so what is left unpublished is the inlined non-portal callee level, whose `frame_var` aliases the outermost frame. The portal-return force names only the jitframe-chain lifetime as what blocks narrowing it back to the token protocol. Arming `jf_force_descr` for a standalone trailing `GUARD_NOT_FORCED_2` is a second, independent gap: no backend does it, so the armed-token test answers false for a portal exit even once the chain is retained. Assisted-by: Claude
`read_descr_vable_array` synthesized the array field pointer with the same literal 8 its scalar sibling used. Latent rather than live — the `_gc_r` accessors this descr reaches take `as_offset()` and store at pointer width without consulting the size — but the declared width should still be the target word. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (11)
✨ Finishing Touches🧪 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 694496b). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![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: 694496b369
ℹ️ 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".
| // Vable scalar word-size invariant — see fn doc-block. | ||
| field_size: 8, | ||
| field_size: std::mem::size_of::<usize>(), | ||
| field_type: majit_ir::value::Type::Ref, | ||
| field_flag: majit_ir::descr::ArrayFlag::Pointer, | ||
| is_field_signed: false, |
There was a problem hiding this comment.
On wasm32, when blackhole replay executes getfield_vable_i for a negative word-sized field—most notably PyFrame.last_instr == -1—this new 4-byte descriptor still sets is_field_signed: false, so Backend::bh_getfield_gc_i takes its (4, false) branch and returns 4294967295 instead of -1. The canonical PYFRAME_DESCR_GROUP explicitly marks last_instr signed (descr.rs:1375-1383), and RPython passes the original field descriptor through unchanged; derive the size, type, and signedness from vinfo.static_field_descr(field_index) rather than synthesizing only its offset.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
1 participant
Follow-up to #823, which merged earlier today. It closes the wasm gap that PR
had to skip around, and works through the review findings that were still open
when it landed.
The wasm store-width overrun
#823 registered
frame_lineno_mid_replay_regression.pywithskip_backends=("wasm",), because a plain hot loop read throughsys._getframe(1)reported offset0there against4on pypy3, CPython,dynasm and cranelift, from the first COMPILED call onward. The review asked for
the backend to be fixed rather than exempted. It is fixed here, and the skip is
gone.
It was not a writer clearing the field. It was a store whose width overran.
PyFrameisrepr(C)withvaluestackdepth: usizeimmediately followed bylast_instr: isize. On wasm32 those sit at offsets 20 and 24, spaced fourbytes.
read_descr_vable_fieldsynthesized every vable static field withfield_size: 8, and thePYFRAMEdescr group declared the same literal forboth fields. The size-dispatching
Backend::bh_setfield_gc_itherefore issuedan 8-byte store at offset 20, covering bytes 20..28 and depositing the zero high
half of a small non-negative depth onto
last_instr— exactly0, not the-1initialization sentinel, deterministically, on every compiled call.That is why the earlier instrumentation was consistent and still misleading: the
marker hook's own publish stores through
*mut isize, a correct four-bytestore, so it read back intact. The clobber arrived afterwards, from the
blackhole replaying a
setfield_vable_iat the neighbouring offset.The widths now come from
size_of::<usize>()/size_of::<isize>(). On a64-bit target that is literally the same number, so any behaviour change is
provably wasm-only.
The extent of the class
Exactly these two fields.
descr.rshas 78 hardcoded8s, but only 14 arenon-pointer, and every other one —
W_IntObject.intval,W_BoolObject.intval,W_IntRangeIterator.{current,remaining,step},W_SpecialisedTupleObject_ii.*,PyTraceback.{lasti,lineno}, thef64entries — is a genuine fixed 64-bitfield where 8 is right. Pointer fields are unaffected:
bh_setfield_gc_r/bh_getfield_gc_rtakeas_offset()and store at pointer width withoutconsulting the size, and the wasm codegen already word-corrects
is_pointer_field(). The array-field sibling inread_descr_vable_arrayisderived here too, for the declared width rather than for a live defect.
A refuted premise worth recording
The earlier hunt was pushed away from the blackhole by "
bh.cpuisNoneonwasm, so that handler would panic rather than store". That is false. The
majit-backend-dynasmcrate is target-gated out of wasm builds, but thedynasmfeature is not:pyre-jit-trace'sdefault = ["dynasm"]forwards itthrough
pyre-interpreter, sobuilder.cpuisSomeandBackendImplresolves to
WasmBackend, which overrides neitherbh_setfield_gc_inorforceand inherits the size-honoring default.Review findings from #823
optimize_FINISHfinalized the guard before the FINISH's own emit —section 1 of the CI parity review, and CodeRabbit Major.
emit_extrasent thestashed
GUARD_NOT_FORCED_2back through the passes after virtualize, so itwas numbered before
emit(op)force_box'd the FINISH args: a return box thatwas virtual is encoded virtual where
postprocess_FINISHencodes itmaterialized.
have_postprocess_op/propagate_postprocessare restored onOptVirtualize, and the Optimizer half they hand to — which never existed,which is why the original field had no reader — is built:
drain_pending_finish_guard_postprocessfinalizes withstore_final_boxes_in_guard+collect_optimizer_knowledge_for_resume,inserts at
new_operations.len() - 1, and rebuildsnew_operations_index,whose last-occurrence-wins mapping a mid-vector insert cannot maintain
incrementally. The blocker the in-code comment named ("
propagate_postprocessis a method on a pass, and the finalization needs the Optimizer") is answered
by the
pending_guard_class_postprocessprecedent in the same file.A declined adoption restored the locals and not the scalars — REAL, and
reachable by default (
PYRE_FBW_BLACKHOLE_RESUMEdefaults on). The declinearm's own comment already said the replay's vable stores "have to come off".
The reviewer's stated mechanism was wrong — no
EscapeFlushUndois or can bearmed on that arm, since the latch is gated on
committed_frame_escape_pc().is_none()— but the defect is not: the drivereaches
last_instrandvaluestackdepththroughsetfield_vable_iagainstthe frame register, which is the live frame, and no undo image covered them.
The interpreter resumed at the coordinate the blackhole reached while reading
the operand stack at a depth the restored locals do not match. Adds
capture_frame_scalars/restore_frame_scalarsbeside the locals pair, onboth the single- and multi-frame post-drive declines.
Two comments the recent work falsified. The codewriter paragraph still
claimed a frame observed mid-replay reports the last coordinate published at a
frame exit; the marker hook publishes at every marker it passes, so what is
actually left unpublished is the inlined non-portal callee level. And the
portal-return force named only the jitframe-chain lifetime as its blocker.
The portal-return force itself — section 2 of the parity review — is
DEFERRED, with both blockers now named in the comment: the jitframe is
libc-freed inside
execute_token, and no backend armsjf_force_descrfor astandalone trailing
GUARD_NOT_FORCED_2(upstream does, fromconsider_guard_not_forced_2), so the armed-token test would answer false fora portal exit even once the chain is retained. Filed with the ordered
convergence path. Note the coupling: that force suppresses the only
GUARD_NOT_FORCED_2producer, which is why the FINISH divergence above isdormant today and why it had to be fixed first.
The synthetic-pycode sentinel guard landed in jit: publish the frame's exit coordinate into last_instr #823 and is re-verified here
(
null/usize::MAXbeforeis_codebeforew_code_get_ptr, matching theprecedent it cites).
Verification
Rebased onto
origin/main; LLBC re-extracted; every number below is from that base.check.py --backend dynasmcheck.py --backend craneliftcheck.py --backend wasmcargo testpyre-interpreter / pyre-jit / pyre-jit-trace / majit-metainterpcargo fmt --checkThe wasm run now includes
frame_lineno_mid_replay, which was skipped there before.🤖 Generated with Claude Code