Two vable resume-frame defects: an unexecuted array read, and a promote stamped on the wrong op - #1140
Conversation
…ff the last guard op
`_opimpl_getarrayitem_vable`'s port recorded its nonstandard fallback ops
without executing them, so the element box carried no concrete and a
residual call taking it aborted the walk with unjournaled effects
(`fbw_rolled_back_with_effects`). `pyjitpl.py:1219` reaches the same
element through `opimpl_getfield_gc_r` + `opimpl_getarrayitem_gc_{i,r,f}`,
which execute the load and attach the value. Add `live_gc_ptr` plus
`stamp_vable_array_base` / `stamp_vable_array_item` and stamp the base and
the element at the seven recording sites.
Removing that abort let the affected traces reach `build_guard_metadata`,
which then panicked in `frame_value_count_at` on `jitcode_index=0 pc=0`.
`record_guard_with_snapshot` attaches a placeholder snapshot with
`jitcode_index: 0` to the `_nonstandard_virtualizable` promote, and
`walker_capture_inline_nonstandard_vable_guard` is what replaces it. Its
inline branch stamps the last *guard* op because `emit_force_virtualizable`
records GETFIELD_GC / PTR_NE / COND_CALL after the promote; its root-frame
branch stamped the last op, so the stamp landed on the COND_CALL and the
promote kept the placeholder. Add `GuardCaptureScope::stamp_last_guard_op`,
route both single-frame publishes through `publish_single_frame_snapshot`,
and set the flag in the root-frame branch.
Re-record the four improved jit-stats baselines:
dynasm/cranelift raise_reg_unbound_jitstress aborts 2->1, rolled_back 1->0, loops 8->9
wasm raise_reg_unbound_jitstress aborts 1->0, rolled_back 1->0, loops 6->7
wasm global_store_plain_dict_globals aborts 1->0, rolled_back 1->0, loops 5->6
wasm pickle_terminal_raise_resume aborts 14->9, rolled_back 5->0, loops 66->71
dynasm 397/397, cranelift 397/397, wasm 395 passed with the pre-existing
unary_negative jit-stats improvement still unrecorded (identical in the
clean-tree control).
Assisted-by: Claude
`record_guard_with_snapshot` mints a one-frame resume snapshot with no coordinate for the interpreter-side vable promotes, and the walker re-stamps it. The frame carried `jitcode_index: 0`, which is a live slot, so a missed re-stamp read as a legitimate frame: the decoder sized the frame from that entry's liveness and read that many tagged words the placeholder never wrote. Measured on `tests/abort_blackhole_virt_array.rs`, the native `frame_value_count` answered 2 for a frame carrying 0 boxes. Reserve `recorder::UNSTAMPED_JITCODE_INDEX` (`-2`) for it instead: `-1` is `create_empty_top_snapshot`'s index and `rd_numb` writes the field through `append_int`'s i16 check. Out of range, `frame_value_count` answers 0, and both pyre decoders — `frame_value_count_at` and `build_time_frame_value_count_at` — now panic naming the missed re-stamp rather than returning 0 or reporting `jitcode_index=0`. Also drops the doc paragraph arguing these guards are always removed before the backend; 2c9f8bbb85b was one that was not. Verified at base 2c9f8bbb85b: cargo test -p majit-metainterp green, dynasm 397/397, cranelift 397/397, wasm 395 passed + the pre-existing unrecorded `synth/unary_negative` jit-stats improvement. Assisted-by: Claude
WalkthroughThe change adds an explicit unstamped jitcode sentinel, recovers concrete virtualizable array values and pointers, targets snapshots at the correct guards, rejects unstamped resume frames, and updates benchmark statistics. ChangesVirtualizable resume stamping
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GuardCaptureScope
participant publish_single_frame_snapshot
participant SnapshotFrame
participant state_liveness_decoder
GuardCaptureScope->>publish_single_frame_snapshot: select the latest guard or recorded operation
publish_single_frame_snapshot->>SnapshotFrame: attach the resume snapshot
SnapshotFrame->>state_liveness_decoder: pass the jitcode coordinate
state_liveness_decoder-->>SnapshotFrame: reject an unstamped coordinate
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-metainterp/src/recorder.rs`:
- Around line 679-696: Update the final assertion in
unstamped_jitcode_index_is_reserved_and_encodable to validate that converting
idx to usize fails, using a target-width-independent check such as
usize::try_from(idx).is_err(). Remove the comparison against u32::MAX while
preserving the existing assertions and message intent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1531cecb-cabe-44bc-9146-eb6169e9c383
📒 Files selected for processing (11)
majit/majit-metainterp/src/history.rsmajit/majit-metainterp/src/recorder.rsmajit/majit-metainterp/src/trace_ctx.rspyre/bench/synth/global_store_plain_dict_globals.wasm.jitstatspyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstatspyre/bench/synth/raise_reg_unbound_jitstress.cranelift.jitstatspyre/bench/synth/raise_reg_unbound_jitstress.dynasm.jitstatspyre/bench/synth/raise_reg_unbound_jitstress.wasm.jitstatspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/state.rs
| /// The three constraints that pick [`UNSTAMPED_JITCODE_INDEX`], asserted | ||
| /// together so a future edit to the value fails here instead of silently | ||
| /// re-aliasing a coordinate the decoder accepts. | ||
| #[test] | ||
| fn unstamped_jitcode_index_is_reserved_and_encodable() { | ||
| let idx = UNSTAMPED_JITCODE_INDEX as i32; | ||
| // Written to `rd_numb` through `resumecode::Writer::append_int`, which | ||
| // asserts the value round-trips through `i16`. | ||
| assert_eq!(idx as i16 as i32, idx, "must survive the rd_numb i16 write"); | ||
| // `-1` is `create_empty_top_snapshot`'s own frame index. | ||
| assert_ne!(idx, -1, "collides with the empty-top-snapshot index"); | ||
| // Every `frame_value_count` decoder resolves the frame with | ||
| // `jitcodes.get(jitcode_index as usize)`; the reserved value must miss. | ||
| assert!( | ||
| (idx as usize) > u32::MAX as usize, | ||
| "must be out of range for any jitcode table" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
for bits in (32, 64):
idx = (-2) & ((1 << bits) - 1)
max_u32_as_usize = ((1 << 32) - 1) & ((1 << bits) - 1)
print(f"{bits}-bit: idx={idx}, u32::MAX as usize={max_u32_as_usize}, idx > max={idx > max_u32_as_usize}")
PY
rg -n -C 3 'unstamped_jitcode_index_is_reserved_and_encodable|u32::MAX as usize' majit/majit-metainterp/src/recorder.rsRepository: youknowone/pyre
Length of output: 1041
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- const definition ---'
rg -n -C 4 'UNSTAMPED_JITCODE_INDEX' majit/majit-metainterp/src/recorder.rs
echo '--- relevant test context ---'
sed -n '660,705p' majit/majit-metainterp/src/recorder.rs
echo '--- Rust cross target semantics (rustc unavailable) ---'
python3 - <<'PY'
from pathlib import Path
p = Path('majit/majit-metainterp/Cargo.toml')
print('Cargo.toml exists:', p.exists())
if p.exists():
print('\n'.join(f'{i}:{l}' for i,l in enumerate(p.read_text().splitlines(), 1)[:120]))
class R:
pass
for name, bits in [('wasm32', 32), ('x86_64', 64)]:
idx = (-2) & ((1 << 32) - 1)
idx_usize = idx & ((1 << bits) - 1)
print(f"{name}: idx as usize={idx_usize}, u32::MAX as usize={(1<<32)-1}, assertion_greater={idx_usize > ((1<<32)-1)}, usize_try_from={((-2) & ((1<bits)-1)) if idx_usize > (1<<bits)-1 else 'ok'}")
print(f"{name}: usize::from_u32/try_from equivalent behavior: value outside usize range=", idx_usize > ((1<<bits)-1))
PYRepository: youknowone/pyre
Length of output: 3512
Make the sentinel reservation test target-width independent.
On 32-bit targets, (-2i32 as u32) as usize is not greater than u32::MAX, so this test can fail on wasm32. Use a range check that does not depend on usize width, such as usize::try_from(idx).is_err().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-metainterp/src/recorder.rs` around lines 679 - 696, Update the
final assertion in unstamped_jitcode_index_is_reserved_and_encodable to validate
that converting idx to usize fails, using a target-width-independent check such
as usize::try_from(idx).is_err(). Remove the comparison against u32::MAX while
preserving the existing assertions and message intent.
![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: daca5a1d6f
ℹ️ 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".
| if let Some(Value::Int(item_index)) = self.concrete_of_opref(index) { | ||
| self.stamp_vable_array_item(op, array_opref, item_index, &descr, Type::Int); | ||
| } |
There was a problem hiding this comment.
When the native StandaloneFrameStack traces a nonstandard virtualizable array read, this newly executed load is discarded because the three vable_getarrayitem_*_descr helpers still return only an OpRef, while their _indexed callers return (op, None). Consequently, pyjitpl/dispatch.rs writes None into the destination concrete register, and the next read_*_reg panics with an uninitialized-concrete-register error despite the value now being present on the recorded op. Propagate the Option<Value> returned by stamp_vable_array_item through the int/ref/float nonstandard fallback paths, matching the standard fallback and upstream executed-Box behavior.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit daca5a1). 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
|
1 participant
Two stacked defects on the
_nonstandard_virtualizableresume path, plus the hardening that keeps the second one from recurring silently.1. The vable array read was recorded but never executed
_opimpl_getarrayitem_vable's port recorded the nonstandard fallback ops without performing the load, so the element box carried no concrete value. The trace then hit aResidualCallArgUnboundabort with unjournaled effects —fbw_rolled_back_with_effects, i.e. the interpreter replayed work the trace had already committed.Upstream (
pyjitpl.py:1219) executes the load viaopimpl_getarrayitem_gc_*.trace_ctx.rsnow stamps the concrete at seven sites: the threevable_getarrayitem_{int,ref,float}_vablefallback legs, theGetfieldGcRarray-base record in the three_indexedfunctions, andnonstandard_vable_array_base. A newlive_gc_ptrhelper rejects the unset marker, the all-ones tombstone, and NULL before any load.2. The promote's resume position was stamped on the wrong op
Fixing (1) stopped the aborts, which let traces reach
build_guard_metadata— where the_nonstandard_virtualizablepromoteGUARD_VALUEwas still holdingrecord_guard_with_snapshot's placeholder resume frame, andframe_value_count_atfailed loud on it.emit_force_virtualizablerecords GETFIELD_GC / PTR_NE / COND_CALL after the promote, so "last op" is not "last guard op".walker_capture_inline_nonstandard_vable_guard's inline branch already accounted for that; its root-frame branch did not, so the re-stamp landed on the COND_CALL and the promote kept the placeholder. AddsGuardCaptureScope::stamp_last_guard_opand routes both single-frame publishes through onepublish_single_frame_snapshot.3. The placeholder no longer aliases a live jitcode slot
The placeholder frame carried
jitcode_index: 0, which is a real slot — so a missed re-stamp reads as a legitimate frame: the decoder sizes the frame from that entry's liveness and reads that many tagged words the placeholder never wrote. Measured ontests/abort_blackhole_virt_array.rs, the nativeframe_value_countanswered 2 for a frame carrying 0 boxes.recorder::UNSTAMPED_JITCODE_INDEX(-2) is reserved for it instead. The value is forced from both ends:rd_numbwrites the field throughappend_int's i16 assert, so nothing above 32767 survives, and-1is alreadycreate_empty_top_snapshot's index.recorder::tests::unstamped_jitcode_index_is_reserved_and_encodablepins all three constraints. Out of range,frame_value_countanswers 0, and both pyre decoders now panic naming the missed re-stamp rather than reporting an opaquejitcode_index=0.The doc paragraph arguing these guards are always removed before the backend is deleted — (2) was one that was not.
Verification
cargo test -p majit-metainterpgreen; dynasm 397/397, cranelift 397/397, wasm 395 passed. Four.jitstatsbaselines re-recorded where check.py itself reported the change as improved:raise_reg_unbound_jitstress(dynasm/cranelift/wasm),global_store_plain_dict_globals(wasm),pickle_terminal_raise_resume(wasm) —fbw_rolled_back_with_effectsreaches 0 on all of them.Those runs were taken at the pre-rebase base; this branch has since been rebased onto
0768a7cd41fwith both commits carrying over byte-identical (git range-diffreports=for both).Known follow-up
The native majit tier (
#[majit_macros::jit_interp]example interpreters) has no re-stamp for the vable opcode arms at all, so its promotes are finalized with a coordinate-less frame. Not fixed here: the machinery to fix it isrecord_state_guard's snapshot half, which needs factoring out first. Filed separately.Summary by CodeRabbit
Bug Fixes
Performance