jit: barrier the blackhole's ref setfield stores - #857
Conversation
`bh_setfield_gc_r` and `bh_setinteriorfield_gc_r` wrote a GC pointer into a heap object with no write barrier. Upstream reaches both through `write_ref_at_mem` (`llmodel.py:495-497`), whose `raw_store` carries an implied barrier from the GC transform; pyre has no transform, so the old-to-young edge went unrecorded and a minor could sweep a young child reachable only through the written slot. The sibling `bh_setarrayitem_gc_r` already barriers in both backends, so these two were the outliers. Barrier the destination when the GC owns it, matching that sibling's shape. With all three exception-slot store paths now barriered, restate the audit note on the raw-exception walker: the walk still cannot be narrowed to the off-GC family, because pyre has no transform inserting these barriers and each one is a hand-written call a future store can omit — narrowing would turn such an omission from a leak into a use-after-free. Also correct two comments in the CALL_ASSEMBLER guard-failure helper that justified rooting the grabbed exception by relocation. Exceptions are allocated non-moving, so the root is about liveness across a major, not about a stale address. Workspace `cargo check` and `cargo fmt --check` clean; pyre-jit 335 tests pass. Exception-heavy script clean on the release dynasm binary by default, under `PYRE_GC_INTERP_COLLECT`, and under `MAJIT_GC_STRESS`. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 11 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 (2)
✨ 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 93974a8). 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
|
![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: 93974a8c62
ℹ️ 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 pyre_object::gc_hook::try_gc_owns_object(struct_ptr as *mut u8) { | ||
| pyre_object::gc_hook::try_gc_write_barrier(struct_ptr as *mut u8); |
There was a problem hiding this comment.
In free-threaded production, try_gc_owns_object enters the synchronized GC query path and can park behind another thread's stop-the-world collection. Because the young reference has already been written but the old destination has not yet been remembered, that collection can relocate or reclaim the referent without updating this slot, leaving a stale pointer before the barrier runs. GcAllocator::write_barrier explicitly requires the call before the store (majit/majit-gc/src/lib.rs:306-310); invoke the managed-safe barrier before publishing value rather than performing a separate ownership query afterward.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| if array != 0 && pyre_object::gc_hook::try_gc_owns_object(array as *mut u8) { | ||
| pyre_object::gc_hook::try_gc_write_barrier(array as *mut u8); |
There was a problem hiding this comment.
When another mutator starts a stop-the-world collection, the post-store try_gc_owns_object query can park this thread before the array has entered the remembered set. A young reference written by bh_setinteriorfield_gc_i can then move or die while the array slot remains unforwarded, so the subsequent barrier is too late. Match the required pre-store barrier ordering in majit/majit-gc/src/lib.rs:306-310, preferably by calling the barrier directly before delegating to the store and relying on its central unmanaged-object guard.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
1 participant
Follow-up to #854, which audited the three store paths into a
W_BaseExceptionreference slot and found one of them unbarriered. Thiscloses that gap.
The gap
bh_setfield_gc_randbh_setinteriorfield_gc_rwrote a GC pointer into aheap object with no write barrier. Upstream reaches both through
write_ref_at_mem(llmodel.py:495-497), whoseraw_storecarries an impliedbarrier from the GC transform — the comment there says so in as many words.
pyre has no transform, so the old-to-young edge went unrecorded and a minor
could sweep a young child reachable only through the written slot.
The sibling
bh_setarrayitem_gc_ralready barriers in both backends(
runner.rsviadynasm_write_barrier_if_managed,majit-backendviagc_write_barrier), so these two were the outliers rather than the rule. Thefix matches that sibling's shape: barrier the destination when the GC owns it.
I swept the rest of the blackhole allocator for the same class — the
_iand_fvariants store non-pointers and need none,bh_setarrayitem_gc_rdelegates to an already-barriered backend method, and
majit-backend'sbh_setinteriorfield_gc_ris an empty default stub. Thesetwo were the only real gaps.
Overlap with #796: that branch carries the same fix for the same two
methods, in the same shape, so whichever lands second should conflict
trivially rather than divergently.
Also here
With all three store paths barriered, the audit note on the raw-exception
walker is restated rather than dropped. The walk still cannot be narrowed to
the off-GC family: pyre has no transform inserting these barriers, so each one
is a hand-written call a future store can omit, and narrowing would turn such
an omission from a leak into a use-after-free.
Two comments in the CALL_ASSEMBLER guard-failure helper justified rooting the
grabbed exception by relocation ("would relocate the object", "possibly
relocated"). Exceptions are allocated non-moving, so the root is about liveness
across a major, not about a stale address.
Verification
cargo check,cargo fmt --checkclean; pyre-jit 335 tests passargs/__context__/__traceback__across collections: clean by default, under
PYRE_GC_INTERP_COLLECT, andunder
MAJIT_GC_STRESS327/328 with
nested_loopat 2.1x against its 2x gatenested_loopsits at its gate and the box was under load average 42-55 fromsibling worktrees, so I discriminated it against a control binary built from
this same tree with only the two barrier hunks removed, interleaving the two
binaries so common-mode load cancels:
A +1.5% median difference, with the control slower than the fix in 2 of 9
rounds, and both well under the 2x gate. The gate failure is the load, not
this change. (Same bench also failed the same way on this box before any of
this branch's commits, and #854's CI was green on all three platforms.)
🤖 Generated with Claude Code