Skip to content

Commit 1feeabd

Browse files
committed
jit: re-mint the tracing sentinel when the GC is rebuilt
`set_tracing_rescall_dummy_gc_type_id` is called from `build_gc` (`pyre-jit/src/eval.rs:1338,3639`), which `reset_gc_fresh_for_test` runs again per GC-stress worker. The sentinel was a `OnceLock`, so a second heap kept the address minted in the first — `is_managed_heap_object` no longer recognises it once the heap it belongs to has been replaced, putting the traced `virtual_token` / `vable_token` slots back on an address the collector does not own. The assertion added with the previous commit turned that into a panic on the second registration instead. Hold the address in an `AtomicUsize` the setter clears, so the next request mints in the heap that is now current, and publish it with a compare-exchange so racing minters agree on one address. The GC-stress harness produces this ordering the moment one of its programs reaches a traced residual call; none does today, so the new test mints the sentinel between two resets directly. It panics at the old assertion without this change. Reported by the Codex review bot on #1084. Assisted-by: Claude
1 parent db368ff commit 1feeabd

2 files changed

Lines changed: 73 additions & 15 deletions

File tree

majit/majit-metainterp/src/virtualref.rs

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
//!
1414
//! Mirrors `rpython/jit/metainterp/virtualref.py`.
1515
16-
use std::sync::atomic::{AtomicU32, Ordering};
16+
use std::sync::atomic::{AtomicU32, AtomicUsize, Ordering};
1717

1818
/// The value [`VREF_GC_TYPE_ID`] holds before `set_vref_gc_type_id` runs. Zero
1919
/// is a legitimate id, so the sentinel has to be a value the registry never
@@ -195,10 +195,10 @@ pub const TOKEN_NONE: *mut u8 = std::ptr::null_mut();
195195
/// the translated identity word is pointer-sized too.
196196
pub const JITFRAME_DUMMY_VTABLE: usize = 0x4A46_444D; // "JFDM"
197197

198-
/// Lazy initialisation of the `_dummy` address. `OnceLock<usize>`
199-
/// (instead of `OnceLock<*mut u8>`) so the cell is `Sync` —
200-
/// raw-pointer types are not.
201-
static TRACING_RESCALL_DUMMY_PTR: std::sync::OnceLock<usize> = std::sync::OnceLock::new();
198+
/// Lazy initialisation of the `_dummy` address, as a `usize` because raw
199+
/// pointers are not `Sync`. Zero means "not minted yet"; a null sentinel would
200+
/// collide with `TOKEN_NONE`, so it is not a value this can ever hold.
201+
static TRACING_RESCALL_DUMMY_PTR: AtomicUsize = AtomicUsize::new(0);
202202

203203
const TRACING_RESCALL_DUMMY_GC_TYPE_ID_UNSET: u32 = u32::MAX;
204204
static TRACING_RESCALL_DUMMY_GC_TYPE_ID: AtomicU32 =
@@ -207,15 +207,15 @@ static TRACING_RESCALL_DUMMY_GC_TYPE_ID: AtomicU32 =
207207
/// Publish the registered leaf type used by the prebuilt
208208
/// `virtualizable.py:326-330 JITFRAME_DUMMY` object.
209209
///
210-
/// Must run before the sentinel is first requested — the address is minted
211-
/// once and never re-minted, so a late registration would leave the process
212-
/// with the unmanaged fallback for good.
210+
/// Registration comes from `build_gc`, so a second call means a second heap.
211+
/// A sentinel minted in the previous one is no longer part of the live heap —
212+
/// `is_managed_heap_object` would stop recognising it and the traced
213+
/// `virtual_token` / `vable_token` slots would be back to holding an address
214+
/// the collector does not own. Drop it so the next request mints in the heap
215+
/// that is now current.
213216
pub fn set_tracing_rescall_dummy_gc_type_id(type_id: u32) {
214-
assert!(
215-
TRACING_RESCALL_DUMMY_PTR.get().is_none(),
216-
"JITFRAME_DUMMY type registered after the tracing sentinel was minted",
217-
);
218217
TRACING_RESCALL_DUMMY_GC_TYPE_ID.store(type_id, Ordering::Relaxed);
218+
TRACING_RESCALL_DUMMY_PTR.store(0, Ordering::Relaxed);
219219
}
220220

221221
/// `virtualizable.py:327 _dummy = lltype.malloc(_DUMMY)` — allocate
@@ -254,11 +254,27 @@ fn allocate_tracing_rescall_dummy() -> *mut u8 {
254254
/// ```python
255255
/// TOKEN_TRACING_RESCALL = lltype.cast_opaque_ptr(llmemory.GCREF, _dummy)
256256
/// ```
257-
/// The returned address is a registered, rooted GC leaf and remains stable for
258-
/// the process lifetime.
257+
/// The returned address is a registered, rooted GC leaf, and stays the same for
258+
/// as long as the heap it was minted in is the current one.
259259
#[inline]
260260
pub fn token_tracing_rescall() -> *mut u8 {
261-
*TRACING_RESCALL_DUMMY_PTR.get_or_init(|| allocate_tracing_rescall_dummy() as usize) as *mut u8
261+
let minted = TRACING_RESCALL_DUMMY_PTR.load(Ordering::Relaxed);
262+
if minted != 0 {
263+
return minted as *mut u8;
264+
}
265+
let fresh = allocate_tracing_rescall_dummy() as usize;
266+
// Racing minters both produce a valid sentinel, but the token protocol
267+
// compares tokens by address, so exactly one may be published. The loser's
268+
// object is immortal either way — upstream's `_dummy` is prebuilt.
269+
match TRACING_RESCALL_DUMMY_PTR.compare_exchange(
270+
0,
271+
fresh,
272+
Ordering::Relaxed,
273+
Ordering::Relaxed,
274+
) {
275+
Ok(_) => fresh as *mut u8,
276+
Err(published) => published as *mut u8,
277+
}
262278
}
263279

264280
/// Virtual reference state for a single reference.

pyre/pyre-jit/tests/gc_stress.rs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1640,3 +1640,45 @@ while i < 40:
16401640
"module dict move_to_end reentrant scan callback GC rooting program failed",
16411641
);
16421642
}
1643+
1644+
/// `virtualizable.py:326-330` makes `TOKEN_TRACING_RESCALL` the address of a
1645+
/// prebuilt GC object, so the sentinel has to belong to the heap that is
1646+
/// current when a traced slot holds it. `reset_gc_fresh_for_test` builds a
1647+
/// second heap and leaks the first, which leaves any sentinel minted in the
1648+
/// first outside the live heap — `is_managed_heap_object` stops recognising it,
1649+
/// and the traced `virtual_token` / `vable_token` slots would be back to
1650+
/// holding an address the collector does not own.
1651+
///
1652+
/// Mint the sentinel between two resets and require a fresh address, which is
1653+
/// what tells the two heaps apart. This is the ordering the GC-stress harness
1654+
/// itself produces the moment any of its programs reaches a traced residual
1655+
/// call; none does today, so nothing else in this binary covers it.
1656+
#[test]
1657+
fn tracing_sentinel_is_reminted_for_a_rebuilt_heap() {
1658+
let _serial = GC_STRESS_SERIAL.lock().unwrap_or_else(|e| e.into_inner());
1659+
let handle = std::thread::Builder::new()
1660+
.stack_size(256 * 1024 * 1024)
1661+
.spawn(|| {
1662+
init_jit_hooks();
1663+
reset_gc_fresh_for_test();
1664+
let first = majit_metainterp::virtualref::token_tracing_rescall();
1665+
assert!(!first.is_null(), "sentinel must never be TOKEN_NONE");
1666+
1667+
reset_gc_fresh_for_test();
1668+
let second = majit_metainterp::virtualref::token_tracing_rescall();
1669+
assert!(!second.is_null(), "sentinel must never be TOKEN_NONE");
1670+
assert_ne!(
1671+
first, second,
1672+
"the sentinel stayed in the heap that was replaced",
1673+
);
1674+
1675+
// Stable within one heap: the token protocol compares by address.
1676+
assert_eq!(
1677+
second,
1678+
majit_metainterp::virtualref::token_tracing_rescall(),
1679+
"the sentinel moved without the heap being rebuilt",
1680+
);
1681+
})
1682+
.expect("spawn worker thread");
1683+
handle.join().expect("worker thread panicked");
1684+
}

0 commit comments

Comments
 (0)