jit: three walk-image defects — locals-typed vable overlay, unpublished escape root stack, unmodeled LOAD_SPECIAL - #1051
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe changes update operand-stack mirror handling for residual-call aborts and escapes. Snapshot reconciliation now excludes local slots and handles ChangesBlackhole stack reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Trace
participant RootWalk
participant ResidualCall
participant SingleFrameBlackhole
Trace->>RootWalk: stop on WalkAbort or VableEscape
RootWalk->>ResidualCall: provide resolved operand-stack mirror
ResidualCall->>SingleFrameBlackhole: store mirror_stack
Trace->>SingleFrameBlackhole: publish root operand stack
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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit af8f4d8). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
…nd-stack slots The GUARD_NOT_FORCED vable-snapshot overlay projected every live Ref register named by the per-PC pcdep color->slot map into virtualizable_boxes[nvs + slot], slots below nlocals included. That map labels a slot per program point instead of binding a register to it, and the register allocator reuses a local's color for unrelated SSA temps, so a local slot could be snapshotted holding an unrelated box. In asyncio _run_once the recorded resume section named local 0 as a ConstPtr to the frame's own code object; forcing the virtualizable wrote that code object into locals_cells_stack_w[0], and the debug arm's finally raised AttributeError: 'code' object has no attribute '_current_handle'. Skip slots below nlocals. The residual call's result lands on the operand stack, and a local's slot is maintained in lockstep by the shadow. Assisted-by: Claude
…adopt `try_adopt_single_frame_blackhole` published the frame's root operand stack only for the `TraceTooLong` and `WalkAbort` legs, on the stated premise that the vable-escape leg "resumes immediately after one forcing residual and keeps its narrower resume-marker image". The escape latch matched that premise with `mirror_stack: None`. A blackhole position ring in jitcode `_run_once`, seeded by the escape leg at resume pc 4180, records the resume running to 4668, branching through 5802 and 5091 to 5157, and re-entering the loop header at 3319, where the `getarrayitem_vable_r` at 3322 reloads an operand from the virtualizable's `locals_cells_stack_w`. With no stack published that slot is NULL, and the `residual_call_r_r` at 3330 calls `jit_next(NULL)`. Publish the root stack for this leg as well. Like `WalkAbort` it stops INSIDE an opcode, where a root walk never wrote the snapshot array, so the source is the walker's OpRef mirror; the escape latch now captures that mirror through `capture_vstack_mirror_image`, the same helper `latch_abort_blackhole`'s single-frame arm uses under the identical `framestack.is_empty() && !inline_subwalk` precondition. Measured with an asyncio Runner.close/shutdown_asyncgens harness over a 9-module `test.test_asyncio` warmup, A/B'd inside one binary through an env hatch: 3/6 runs SIGSEGV without the publish, 0/6 with it. `check.py --backend dynasm` 377/377. Assisted-by: Claude
`classify_vstack_opcode` had no arm for `LOAD_SPECIAL`, so every `with` / `async with` body reached the `Unmodeled` fallthrough and latched `vstack_valid = false` for the rest of the walk. A later branch guard with a kept operand stack deeper than one slot then aborts (`BranchGuardKeptStackUnsupported`), and that abort is outside `DispatchError::leaves_complete_image`, so no blackhole adoption leg covers it: the walk ends uncommitted and the interpreter replays a region whose residual calls already ran concretely. The opcode pops the context-manager object at `prev_depth - 1` and pushes the special method and the call self/NULL slot upward from that position. Both pushed values go through `emit_pushvalue_ref!`, so the virtualizable shadow is authoritative for each: that is `MultiResultFromShadow`, the same shape `UNPACK_SEQUENCE` uses. `AEnter` / `AExit` keep the `Unmodeled` fallthrough. Measured on an asyncio harness (400 rounds, one binary, env-gated A/B): mirror invalidations 5 -> 0, `BranchGuardKeptStackUnsupported` declines 5 -> 0, uncommitted walks with a non-zero effect count 11 -> 6. Assisted-by: Claude
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![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: af8f4d852b
ℹ️ 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".
| commit_leg == WalkEndCommitLeg::TraceTooLong || commit_leg == WalkEndCommitLeg::WalkAbort; | ||
| let publishes_root_stack = commit_leg == WalkEndCommitLeg::TraceTooLong | ||
| || commit_leg == WalkEndCommitLeg::WalkAbort | ||
| || commit_leg == WalkEndCommitLeg::VableEscape; |
There was a problem hiding this comment.
For WalkEndCommitLeg::VableEscape, this new requirement makes adoption fail whenever the latch's mirror_stack is None (for example after an unmodeled stack opcode has already set ctx.vstack_valid = false, or a live stack slot cannot be resolved). That latch is built after the forcing residual has already executed, and in the live-heap/reentrant cases the escape PC has been canceled, so returning false here drops into the legacy replay path and re-runs the residual side effects. Please preflight a complete mirror before choosing this leg, or keep recording/blackhole-forward instead of declining after the residual has run.
Useful? React with 👍 / 👎.
1 participant
Three independent defects on the walk / blackhole-adopt path, each of which lets a
frame resume from an image that does not describe the state the residual left behind.
All three were found with the same probe method — run the asyncio suite under
PYRE_FBW_CENSUS=1and treatcommitted=false AND effects>0as the hazard predicate —and each is verified by a deterministic counter, not by the failure rate.
1.
resume_snapshot.rs— the after-residual vable overlay wrote LOCAL slotsThe overlay that re-projects live
Refregisters after a residual call used theper-PC
pcdepmap to decide which slot a register belongs to.pcdepis keyed byprogram point, not by binding: two registers live at the same PC are indistinguishable
to it. When a local slot and an operand-stack slot were both live across the residual,
the overlay could publish the wrong register into the local slot — a
selfslot holdinga code object, among other faces.
Restrict the overlay to operand-stack slots, where the
pcdeplabel is the identity.Locals keep the ordinary snapshot value.
2.
residual_call.rs/trace.rs— the vable-escape single-frame adopt published no root operand stacktry_adopt_single_frame_blackholelatched aMIFramewithout publishing the rootoperand stack, so a resumed
getarrayitem_vable_ragainst that frame read NULL.The tell is that the
[fbw-escape]diagnostic line is not the image — there are twolatch producers and only one of them published the stack. The
VableEscapeleg nowpublishes the root stack like the other producer. 6/6 → 0/6 on the repro.
3.
vstack_mirror.rs—LOAD_SPECIALwas unmodeled, so everywithbody killed the walk mirrorThe root cause behind the residual asyncio instability. Every link verified in source
and measured:
That double application accounts for the whole observed symptom set:
InvalidStateError("FINISHED: …"),RuntimeError: cannot enter context … already entered,AttributeError: 'NoneType' object has no attribute '_source_traceback', andTypeError: 'builtin_function_or_method' object is not an iterator.Instruction::LoadSpecial { method }restricted toSpecialMethod::Enter | Exitnowclassifies as
VstackOpClass::MultiResultFromShadow.Why that class and not
ResultToTos.LOAD_SPECIALpops the manager atprev_depth - 1and pushes two values.ResultToTosonly writes[new_depth-1],leaving the popped object as a stale non-NONE box where the bound method belongs — and
both hole-fill helpers skip non-NONE slots (
if *slot != OpRef::NONE { continue; }inreseed_vstack_from_shadowandreseed_vstack_from_callee_shadow). A stale box isstrictly worse than an invalid mirror.
MultiResultFromShadowNONEs[pop_point .. new_depth)and lets the shadow source each pushed slot; unsourceable slotsstay NONE and the kept-stack check declines — never a corrupt box. Premise verified: both
pushed values go through
emit_pushvalue_ref!→setarrayitem_vable_r(codewriter.rs 12068 / 12083 / 7716).
AEnter/AExitare deliberately leftUnmodeled(permanent-abort lowering).Measurement
400-round asyncio harness, one-binary A/B behind a temporary env gate:
op=LoadSpecial arg=OpArg(1))BranchGuardKeptStackUnsupporteddeclinescommitted=false AND effects>0walksBefore the fix the three counters are one-to-one — every mirror kill produced exactly
one decline and exactly one uncommitted-with-effects walk, and all five kills were the
identical
__exit__site.End-to-end, as a supporting (two-binary, underpowered) number: the same harness at
600 rounds × 10 tries went from 9/10 tries completing — 1 class-B failure, 2 other
failures, 1 hard non-zero exit — to 10/10 clean, zero failures of any class.
Refuted along the way (recorded so they are not re-derived)
call_jit.rs:3709— the comment there describes areal but unfiring path.
PYRE_WALKABORT_OFF=1— no change; that leg only coversleaves_complete_image()errors, and this abort is not one.
_run_oncedrain shape — 0 hits. The abort needsthe real warmed workload.
SEND/GET_AWAITABLE/GET_AITER/GET_ANEXT) —A/B'd,
decline_why5 in both arms. Reverted rather than landed unverified.Known follow-ups (not in this PR)
VableEscapedDuringResidualCall committed=false. Per-walkpairing shows two causes: the latch is skipped entirely for a bridge trace
(
!ctx.trace_ctx.is_bridge_trace, residual_call.rs:3081 — listed as a precondition in acomment that explicitly refutes the other gates), and the inline-sub-walk arm declines
on
odo_unchanged=false(residual_call.rs:3142-3143), which is exactly the case wherereplay is unsound.
END_SEND,YIELD_VALUE,AEnter,AExitremainUnmodeledin the mirror.state from which no image can be built. Upstream cannot —
_copy_data_from_miframecopies the banks unconditionally and has no failing path (
blackhole.py:1711-1730,cited at mod.rs:2338-2341).
test.test_asyncioas TIMEOUT; worth revisiting now.Summary by CodeRabbit