jit: keep the adopted terminal's result when a carrier-drain walk aborts - #1130
Conversation
`drive_bridge_carrier_walk`'s abort tail called `fbw_finish_payload_reset` after `try_adopt_blackhole`, which had just stored the adopted blackhole terminal's `DoneWithThisFrame*` value there. The reset is older than the adopt, which was inserted above it, so the drain cleared the frame's return value on every adopted abort. The drop is silent. The adopt also commits the walk-end state, so the caller finds no concrete to hand back (`fbw_finish_concrete_take`) and does not replay through the blackhole either; the frame's return value reaches Python as None. Clear the stash before the adopt instead. A stale sub-walk value is still discarded, and whatever the adopt installs now survives to the caller. The non-adopted leg keeps its post-discard reset, and a CRN adoption stores nothing, so neither leg can carry a stale value forward. `test.test_pickletools` `OptimizedPickleTests.test_ints` failed on this: `pickletools.optimize` returned None and `pickle.loads` raised `EOFError: Ran out of input`. The CPython suite gate goes from PASS 106/FAIL 2 to PASS 107/FAIL 1, the remaining failure being the pre-existing `test_re`. The parity test reproduces the defect in 0.2s. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 33 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 (2)
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 6147b27). 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
|
drive_bridge_carrier_walk's abort tail calledfbw_finish_payload_reset()after
try_adopt_blackhole, wiping theDoneWithThisFrame*value the adopt hadjust stored. The guard's caller (
fbw_finish_concrete_takeincall_jit.rs)then found no concrete, and because the adopt had committed the walk-end state
it did not replay through the blackhole either, so the frame's return value
reached Python as
None.pickletools.optimize()has a singlereturn out.getvalue(); under the JIT itreturned
Noneandpickle.loadsraisedEOFError: Ran out of input(
test.test_pickletoolsOptimizedPickleTests.test_ints).Clear the stash before the adopt instead. A stale sub-walk stash is still
discarded, and whatever the adopt installs survives. A declined adopt inherits
the cleared slot, so
if !adopted { reset }alone would not be equivalent.discard_bridge_carrier_walk, the only call between the adopt and the caller'stake, sets no payload.
run_perfn_walk's epilogue already guards this exact hazard(
blackhole_terminal_no_replay, "otherwise aDoneWithThisFrameresult isdiscarded"), but
trace_bytecodereturns the carrier drain's actionimmediately, so that epilogue never runs on this path. A census of the four
try_adopt_blackholecall sites found this to be the only unguarded one.Verification
test.test_pickletools108 to run, 22 skipped)The remaining gate failure is
test_re, the unrelated pre-existing defectowned by #1122.
pyre/extra_tests/parity_tests/pickletools_optimize_bridge_abort.pyis theregression guard (0.2 s). A fixed input does not reproduce the defect — the
varying protocol/width matrix and a second round are both required.
🤖 Generated with Claude Code