jit, gc: traced traceback nodes non-moving; virtualizable debugdata and token fixes; journal the walk-time traceback attaches - #1050
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 (38)
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 6ba64bd). 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
|
…ding-time constant `MIFrame::flush_to_frame` republished all six virtualizable statics as constants read out of the live frame. `DELETE_NAME` resolves the name through `debugdata.w_locals`, so the compiled loop deleted through the recording frame's mapping once `exec(code, globals)` reused the same code object with a fresh globals dict, raising NameError. Keep the box `read_boxes` seeded for that field; the remaining pointer statics stay promoted constants. Adds pyre/bench/synth/exec_fresh_globals_delete_name.py with its dynasm, cranelift and wasm jit-stats baselines. Assisted-by: Claude
`BhFieldSpec::from_field_descr` mapped every pointer field to `ArrayFlag::Unsigned`. `field_spec_from_bh` copies that flag into the rebuilt `SimpleFieldDescrSpec`, whose `is_pointer_field()` is `flag == ArrayFlag::Pointer` (descr.py:173), so a pointer field that traversed the round trip reported itself as non-pointer and `handle_write_barrier_setfield` declined its write barrier. The codewriter's other two producers, `value_type_to_field_flag` and `bh_field_flag_from_descr`, already map a pointer field to `ArrayFlag::Pointer`. Assisted-by: Claude
`compile_exit_frame_with_exception` (pyjitpl.py:3261) opens with `store_token_in_vable()`, the same as `compile_done_with_this_frame` (:3223). pyre's value and void arms settle the token via `fbw_force_virtualizable_before_return`; the top-level raise arm only published `last_instr`. Every residual call arms the token in `walker_vable_and_vrefs_before_residual_call`, so the exception exit left the frame naming a jitframe that dynasm frees in `execute_token`, and the next `is_force_token_armed` walked `jf_forward` off freed memory. pyre settles by storing back, which zeroes the slot, rather than by arming the token as upstream does: upstream's FORCE_TOKEN is the heap-allocated GC `JITFRAME`, pyre's is the machine frame pointer. Adds `top_level_raise_settles_the_vable_token`, which asserts the recorded tail op is a `SetfieldGc` of const-0 at the token offset. Assisted-by: Claude
…es them `record_top_level_application_traceback`, `record_inline_application_traceback` and `record_exc_edge_discarded_tracebacks` apply their node to the live exception while the walk records. Only the bridge handler entry wrapped that mutation in the fbw undo log, so a walk that did not commit left the other three sites' nodes attached and the interpreter's replay recorded the same frames again. Move the journaling into the three attach sites and drop the bridge arm's own wrapper, which is now redundant. A freshly raised exception hides this — the discarded walk's object is unreachable — so the observable case is a `raise` that re-uses one exception object: `tb_frame.f_locals` on the catching frame aborts the walk and the handler then reads a four-node chain where the interpreter builds two. Adds `pyre/bench/synth/exception_reused_object_tb_not_doubled.py` and a unit test for the multi-node rollback. Assisted-by: Claude
`walk_pyframe_roots_area` handed every `locals_cells_stack_w` slot to `walk_raw_exception_roots`, which calls `is_exception` and so dereferences the slot's leading word as an `ob_type`. A slot can hold a `JitVirtualRef`, whose leading word is the `JIT_VIRTUAL_REF_VTABLE` magic, and the dereference then faults. The same function already tested `ptr_is_virtual_ref` on the `f_backref` slot one block earlier; extract that arm as `forward_virtual_ref_forced` and apply it to the value-stack slots too, forwarding `forced` and skipping the raw walks. `walk_raw_immortal_roots` was already safe for this input: `walk_immortal_rec` guards its `ob_type` read behind `try_gc_owns_object`, and a vref is allocated with `alloc_oldgen_typed`. Reproduced on cranelift with a hot `raise`/`except` loop whose handler reads `tb_frame.f_locals` on the catching frame; the collection that faults is a minor one triggered by a nursery allocation from compiled code. dynasm and the interpreter are unaffected. Assisted-by: Claude
`exception_tb_f_locals_vref_root_walk.py` is the oracle for the virtual-ref root-walk fix: pre-fix it SIGSEGVs 3/3 on cranelift at `15000 head`, post-fix it matches dynasm, `PYRE_NO_JIT=1` and pypy. It stays out of the suite because the same file run with no arguments hits an unrelated open abort. `gc_varsize_item_const_shape_witness.py` is that abort: `GC BUG ... site=minor_varsize_item_target`, cranelift 3/3, dynasm and the interpreter clean. Its docstring records the decoded holder fields and the allocation-layout trigger. Assisted-by: Claude
`journaled_concrete_traceback_attach` reads the concrete exception's traceback head before the attach, so the walk now dereferences the pointer that `dispatch_via_miframe_mirrors_last_exc_value_back_into_sym` passed as a bare `0xDEAD_BEEF` sentinel. The sentinel survived only because the recording hook is not installed under test; the production path dereferences the same pointer. Build the operand from `w_exception_new` instead. Assisted-by: Claude
`trace_and_update_object` receives a `site` naming the root path it was reached from — `minor_remembered_set`, `minor_jitframe_root`, and the rest — but when it recursed into `copy_nursery_object` or `assert_traced_slot_initialized` it passed a hardcoded site naming only the SLOT KIND, so the invalid-type_id and nursery-poison panics could not say which root path produced the bad holder. Thread the caller's site down as `parent_site` and add it to both messages. The parameter is `&'static str` so no allocation happens on a traced slot. On the `minor_varsize_item_target` witness under `synth/_pending` this reports `parent_site=minor_remembered_set` 3/3, which rules out the jitframe shadow-stack root walk as that abort's path. Assisted-by: Claude
`w_pytraceback_new` allocates a traceback node through the stable old-gen allocator and states the obligation in place: raw `*mut PyTraceback` readers and the exception `w_traceback` chain hold bare pointers no root walker reaches. `PYTRACEBACK_DESCR_GROUP` never stamped `set_non_moving`, so a node built by compiled code was a nursery object a minor collection could relocate. Stamp the group's size descr, as the `W_ObjectObject` group and `mapdict_storage_gcarray_descr` already do for the same reason. `gc_varsize_item_const_shape_witness.py` and four stack-depth variants of it abort with `GC BUG ... site=minor_varsize_item_target` at 18 of 320 (fixture, nursery size) pairs over a 512K-32M `PYPY_GC_NURSERY` sweep before this change, and at none of them after; a finer 256K-step sweep to 51M over two of the fixtures is also clean. Assisted-by: Claude
`PYTRACEBACK_DESCR_GROUP`, the `W_ObjectObject` group and `mapdict_storage_gcarray_descr` each carry `set_non_moving(true)` to match an interpreter allocator that is non-moving because raw pointers reach the object without being rooted. Nothing checked the flags, and a missing one shows up only as a GC abort at particular nursery sizes — `check.py` pins `PYPY_GC_NURSERY` to 4MB, which is not one of them. Assert the three flags. Removing the traceback stamp fails the test. Assisted-by: Claude
…mp moved Fourteen `.jitstats` baselines, `guard_failures` only; `loops_compiled` and `bridges_compiled` are unchanged in every one. Assisted-by: Claude
… baseline The committed baseline named only the five badness fields. `_jit_stats_change` reads a field absent from either side as 0, so `loops_compiled`, `bridges_compiled` and `guard_failures` compared against 0 and the wasm run of this fixture now reports 1, 0 and 4 — the same three values its cranelift and dynasm baselines carry. Assisted-by: Claude
The file was parked because running it with no arguments hit `GC BUG ... site=minor_varsize_item_target` instead of its own subject. With traced traceback nodes allocated non-moving it is clean at all 64 sizes of a 512K-32M `PYPY_GC_NURSERY` sweep, and cranelift, dynasm and `PYRE_NO_JIT=1` print the same line, so it can gate the walker guard from the suite. wasm is exempted. It loses `e` from the catching frame's `f_locals` over part of the loop and prints a second tuple; that is compiled-code only (clean at N=4000, wrong from N=8000) and reproduces with the whole source tree reset to the merge base, so it is neither this fixture's subject nor this branch's. Same stale-`f_locals`-from-compiled-code class as getframe_caller_locals_nested_compiled_callee, which carries the same exemption. Assisted-by: Claude
Its cranelift guard_failures is not the same on every host. macos-latest and ubuntu-24.04 both report 5980; windows-latest reported 5979 against that recording. #1043 removed the closure_per_call overlay after two counters in that band disagreed with themselves across jobs, so a `.cranelift.win32.jitstats` overlay cannot hold this one either, and a missing baseline is a hard fail rather than an opt-out. The walker guard the file was promoted for therefore has no suite gate. The file stays as the reproduction, with the measurement and the reason in its header. Assisted-by: Claude
…es the frame
A `STORE_FAST` on a strictly-inlined callee's own fresh frame takes the fold in
`setarrayitem_vable_via_metainterp`: it updates the walker shadow, pokes the
recording-time frame through `store_live_frame_array_slot`, and records no
`SETARRAYITEM_GC`. `disarm_folded_inline_callee_after_escape` is the replay that
writes those slots back and disarms the fold, and it had one call site — the
branch where a residual call forced the virtualizable.
`emit_traceback_node` stores the callee frame box into `PyTraceback.frame`,
which is a second escape: the frame outlives the trace with its locals array
holding only the parameters `emit_new_pyframe_inline_with_params` seeded. Call
the replay from `traceback_node_site` before handing the box out, declining the
site on `Err` as both callers already do for an unresolved frame. It is inert at
the top level, where there is no inline callee.
`exception_inline_callee_tb_frame_locals` reads
`tb.tb_frame.f_locals` off the callee's node. dynasm and cranelift both answered
`('i',)` on 2758 of 4000 iterations before this and `('i', 'marker')` on all
4000 after, matching pypy3, CPython and PYRE_NO_JIT=1.
Assisted-by: Claude
Update note for PR #1050 (draft — post after the suites are green)Rebased onto New in this push
A Witness
Its cranelift The three CI failures on the previous push
— commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ba64bd02a
ℹ️ 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".
| && residual_call::disarm_folded_inline_callee_after_escape(ctx, opcode_position).is_err() | ||
| { | ||
| return None; |
There was a problem hiding this comment.
Propagate failed inline-frame flushes
When a folded inline callee escapes through a traceback and disarm_folded_inline_callee_after_escape hits a non-standard-virtualizable guard it cannot snapshot, that helper may already have recorded SetarrayitemGc/guard ops before returning Err. Converting the error to None here leaves those partial ops in the trace and falls back to the opaque traceback hook instead of aborting or rewinding, so the compiled trace can carry a guard with missing/incorrect resume data or only a partial locals flush. Please propagate the error or cut the trace back to the pre-disarm position before falling back.
Useful? React with 👍 / 👎.
Both files were recorded against a base that predates #1050, #1056 and #1057. Re-measured after re-extracting LLBC, whose three crate fingerprints had all drifted: exception_inline_callee_tb_frames bridges_compiled 2 -> 3, guard_failures 403 -> 604 gc_bug_bridge_flavor_traceback_names guard_failures 1837 -> 1838 Both also gain the `fbw_rolled_back_with_effects` field the new base adds. The rest of the wasm suite is unchanged at 376 passing. Assisted-by: Claude
Both files were recorded against a base that predates #1050, #1056 and #1057. Re-measured after re-extracting LLBC, whose three crate fingerprints had all drifted: exception_inline_callee_tb_frames bridges_compiled 2 -> 3, guard_failures 403 -> 604 gc_bug_bridge_flavor_traceback_names guard_failures 1837 -> 1838 Both also gain the `fbw_rolled_back_with_effects` field the new base adds. The rest of the wasm suite is unchanged at 376 passing. Assisted-by: Claude
…d the trampoline scratch inside the JitFrame (#1058) * jit(wasm): count a heap load as loop-state advancement in the loop-closing bridge check `compile_bridge`'s livelock shield refuses a loop-closing bridge whose terminal JUMP carries no advancing value. Its advance predicate accepted only integer and float arithmetic, so a pointer chase did not qualify: `tb = tb.tb_next` lowers to GetfieldGcR, which is neither arithmetic nor a heap write, and the bridge that re-reads the link was declined even though each pass reads a different node and walks the loop toward its exit. Seven fixtures carry that traceback walk. Six of them lose their declines, and four land on dynasm exactly (guard_failures wasm -> dynasm): exception_inline_callee_tb_frames 975 -> 403 dynasm 403 exception_traceback_lineno_chain 802 -> 402 dynasm 402 exception_catching_frame_tb_node 601 -> 401 dynasm 401 exception_reentry_guard_finally_residual 2459 -> 2261 dynasm 2261 gc_bug_bridge_flavor_traceback_names 2036 -> 1837 dynasm 1655 exception_traceback_frame_lineno 814 -> 813 dynasm 811 `bridges_compiled` drops alongside on five of them, also onto dynasm's value: the extra bridges were the retraces that a declined guard forced. The shield itself stays. It refuses a bridge that resumes at the loop header with byte-identical state, whose guard then re-fails and spins the loop against the bridge; the resume-at-LABEL dispatch does not address that, and the two were introduced together. Only the predicate widens, so a JUMP built entirely of verbatim input reloads, fresh allocations and baked constants is still refused. check.py wasm 374/374. Assisted-by: Claude * jit(wasm): move the residual-call trampoline scratch out of the JitFrame, and drop the CALL_ASSEMBLER gate it required The trampoline's ABI was frame-relative on both sides of the host hop: the guest stored func_ptr/nargs/args off wasm local 0 and read the result back off local 0 before reloading it, and the host computed `call_area = frame_ptr + call_area_ofs` before re-entering the guest and wrote the result there afterwards. That is sound only for the host-entry frame, which is allocated old-gen and therefore non-moving. CALL_ASSEMBLER breaks it twice. `wasm_jit_ca_alloc_frame` allocates the callee frame in the nursery, so it can move across the hop; and it sizes it `ca_frame_bytes`, which excludes the trailing call area, so a trampoline call on such a frame would store past the object's end. The backend therefore refused to compile any CALL_ASSEMBLER-bearing bridge that shared a trace or token with a trampoline residual call. The scratch is now a module-static array whose address is baked into emitted code, the way the pending-exception cells already are. Frame geometry is untouched: the call area stays reserved in the frame, unused, so frame_bytes, ca_frame_bytes and the module cache do not move. A single shared area is sound because trampoline use is strictly LIFO — the host materialises every argument before invoking the callee and the guest loads its result immediately on return. The trampoline reads its scratch at `base + offset`, and the base-only import has the offset baked host-side, so every emitting module now takes the two-argument import. With nothing left to protect, the gate and its census go: the `has_trampoline_calls` checks on the pending self target, the registered target, the live loop and the redirect, the chained-bridge census, and `ca_reentry_safe`. The BRIDGE_DIAG slots stay; slot 15 stops firing. This was the last terminal bridge decline on wasm. On the only three fixtures that carried it, `decl_shortcircuit` 543/543/89 -> 0/0/0, `decl_callasm` and `decl_ca_trampoline` -> 0, and `entered == BRIDGE_OK` exactly (16, 16, 26): ca_bridge_multiframe_resume_double_call gf 3062 -> 2581 bridges 14 -> 16 recursion_memo_branch gf 3083 -> 2602 bridges 14 -> 16 foriter_call_resume_drops_iteration gf 5182 -> 5165 bridges 23 -> 26 dynasm reports 2592/16, 2613/16 and 5150/27. No other fixture moved. check.py wasm 374/374; cargo test -p majit-backend-wasm green. Assisted-by: Claude * bench: re-record two wasm jitstats baselines on the current base Both files were recorded against a base that predates #1050, #1056 and #1057. Re-measured after re-extracting LLBC, whose three crate fingerprints had all drifted: exception_inline_callee_tb_frames bridges_compiled 2 -> 3, guard_failures 403 -> 604 gc_bug_bridge_flavor_traceback_names guard_failures 1837 -> 1838 Both also gain the `fbw_rolled_back_with_effects` field the new base adds. The rest of the wasm suite is unchanged at 376 passing. Assisted-by: Claude * mapdict: say what the four `?` fields actually rely on in pyre `allow_unboxing`, `ever_mutated`, `attr` and `typ` are declared quasi-immutable upstream, and the comments asserted that flatly. pyre installs no quasi-immutable watcher for any of them — they are plain Cells, and mapdict.rs contains no quasi-immutable code — so the comments read as a property the code does not have. State the substitute instead: every read is paired with a GuardValue on the instance map, emitted by walker_guard_mapdict_instance_shape. The full rationale sits on `allow_unboxing`, the only one of the four that gates a fold decision; the other three refer to it. Same formulation already used for the `Function` `?` fields in descr.rs. Comments only. Assisted-by: Claude
Thirteen commits: three wrong-code fixes, three crash fixes, one GC diagnostic,
and the fixtures and baselines they need.
Wrong code
debugdatacarried as a box, not a recording-time constant.MIFrame::flush_to_framerepublished all six virtualizable statics asconstants read out of the live frame.
DELETE_NAMEresolves the name throughdebugdata.w_locals, so a constant pins the recording frame's mapping and thecompiled loop then deletes through it once
exec(code, globals)reuses thesame code object with a fresh namespace, raising
NameError.virtualizable.py:86-93 read_boxescarries every static as a loop-carried boxfor exactly this reason; the remaining pointer statics keep their
promoted-constant representation because the bytecode path cannot rebind them
mid-loop.
The pointer flag survives the
BhFieldSpecround trip.from_field_descrmapped every pointer field toArrayFlag::Unsigned, andfield_spec_from_bhcopies that flag into the rebuiltSimpleFieldDescrSpec, whoseis_pointer_field()isflag == ArrayFlag::Pointer(descr.py:173). A pointer field that traversedthe round trip therefore reported itself as non-pointer and
handle_write_barrier_setfielddeclined its write barrier.Walk-time traceback attaches are journaled.
Three of the four attach sites mutated the live exception outside the fbw undo
log, so a walk that did not commit left its node attached and the
interpreter's replay recorded the same frames again. A freshly raised
exception hides this — the discarded walk's object is unreachable — so the
observable case is a
raisethat re-uses one exception object.Crashes
The virtualizable token is settled on the top-level raise exit.
compile_exit_frame_with_exception(pyjitpl.py:3261) opens withstore_token_in_vable(), the same ascompile_done_with_this_frame(
:3223). pyre's value and void arms settle the token; the raise arm onlypublished
last_instr, so the exception exit left the frame naming a jitframethat dynasm frees in
execute_token, and the nextis_force_token_armedwalked
jf_forwardoff freed memory.The root walk skips the PyObject-shaped raw walks for a virtual ref in a
frame slot.
walk_pyframe_roots_areahanded everylocals_cells_stack_wslot to
walk_raw_exception_roots, which dereferences the slot's leading wordas an
ob_type. A slot can hold aJitVirtualRef, whose leading word is theJIT_VIRTUAL_REF_VTABLEmagic. Thef_backrefarm one block earlier alreadytested for this; it is extracted and applied to the value-stack slots too.
Traced traceback nodes are allocated non-moving.
w_pytraceback_newallocates through the stable old-gen allocator and statesthe obligation in place: raw
*mut PyTracebackreaders and the exceptionw_tracebackchain hold bare pointers no root walker reaches.PYTRACEBACK_DESCR_GROUPnever stampedset_non_moving, so the same objecttype was movable or not depending on which allocator built it, and a node
built by compiled code was a nursery object a minor collection could relocate.
The
W_ObjectObjectgroup andmapdict_storage_gcarray_descralready carrythe stamp for the same reason; a new test asserts all three, since a missing
flag shows up only as a GC abort at particular nursery sizes and
check.pypins
PYPY_GC_NURSERYto 4MB, which is not one of them.Measured over a 512K–32M
PYPY_GC_NURSERYsweep: the witness and fourstack-depth variants abort at 18 of 320 (fixture, size) pairs before the
change and none after.
Diagnostic
trace_and_update_objectnow threads its caller'ssitedown tocopy_nursery_objectandassert_traced_slot_initializedasparent_site,so the invalid-type_id and nursery-poison panics name the root path that
reached the object rather than only the slot kind.
&'static str, so a tracedslot allocates nothing.
Fixtures
exception_reused_object_tb_not_doubled.pyandexec_fresh_globals_delete_name.pygate the first and third wrong-code fixes.exception_tb_f_locals_vref_root_walk.pymoves out of_pendingand into thesuite — the traceback stamp above unblocked it — and gates the virtual-ref
walker guard on dynasm and cranelift.
gc_varsize_item_const_shape_witness.pystays under_pending; see below.Known open, and NOT fixed here
Two divergences are documented in the tree rather than fixed, each with the
measurement that scopes it:
cranelift
GC BUG ... site=minor_varsize_item_target(
_pending/gc_varsize_item_const_shape_witness.py). The traceback stamp hidesit, but the stamp is not its root cause: with the stamp removed and the same
source, cranelift aborts at 3 of 64 nursery sizes while dynasm aborts at 0
of 64. Three hypotheses were tested and refuted this cycle — reloading every
ref root at every collecting call, treating every residual call as collecting,
and a probe on every cranelift Ref-store emitter (zero hits, gate proven
live).
wasm loses
efrom a catching frame'sf_locals. Found by the promotedfixture; compiled-code only (clean at N=4000, wrong from N=8000) and
reproduces with the whole source tree reset to the merge base, so it predates
this branch. Same class as the exemption
getframe_caller_locals_nested_compiled_calleealready carries, and exemptedthe same way.
Verification
pyre/check.py: dynasm and cranelift all-pass; wasm all-pass. Re-run afterthe fixture promotion changed the suite composition.
PYPY_GC_NURSERYsweep per arm, not asingle run — this abort is a ~5% duty-cycle resonance over nursery size, so
one clean run is not evidence.
adaptations, each already carrying its upstream citation in the code.
🤖 Generated with Claude Code