diff --git a/changelog.d/7994-per-realm-prototype-addr.md b/changelog.d/7994-per-realm-prototype-addr.md new file mode 100644 index 0000000000..4baeda5cd8 --- /dev/null +++ b/changelog.d/7994-per-realm-prototype-addr.md @@ -0,0 +1,62 @@ +### Fixed + +- **`Array.prototype` / `Object.prototype` address caches are per-realm now, not process-global (#7988).** + `crates/perry-runtime/src/array/prototype_addr.rs` memoized both intrinsics in + two process-global `AtomicUsize` cells holding **raw addresses of objects in a + thread-local arena**, while the realm they name is per-thread: + `js_get_global_this` bootstraps `THREAD_GLOBAL_THIS` once per *thread*, but + `resolve_prototype_addr` missed only once per *process*. The first thread to + touch either intrinsic decided the value for every other `perry/thread` agent, + with three consequences — all closed structurally by moving the storage into + one `crate::perry_thread_local!` declaration: + + 1. **Wrong identity.** `object_prototype_addr_matches(addr)` on agent B + compared B's objects against **A's** `Object.prototype`, so B's own + intrinsic was never recognised. Observable: `Object.prototype[7] = v` on a + worker never flipped `OBJECT_PROTO_HAS_INDEX` (the write hook's "is this + the prototype?" test compared against a foreign address), so the hole/OOB + read fallback stayed switched off and `[1,2,3][7]` read `undefined` on + every thread but the first. Same for `Array.prototype[8] = v`. + 2. **Unattributed dereference.** `heal_prototype_addr` read the cached + address's `GcHeader` from *any* thread with no ownership check, and + `note_array_index_write` calls it on every indexed array write. A's + collector can sweep or move that object, and A's arena blocks are + `dealloc`'d at thread exit; a stray `GC_FLAG_FORWARDED` byte there sent + `resolve_forwarding` one word further into memory the reader did not own. + 3. **Cross-thread root rewrite.** `scan_prototype_addr_cache_roots_mut` is + registered per thread and *writes* the cell with its **own** to-space + address, so agent B's collector could overwrite a cell naming A's heap. + + #7974 fixed the *test* that exposed this (#7975's `dead_owner_side_tables` + ordering flake) by driving the #6981 algebra on a cell the test owns, and + explicitly left the shipped cells alone; the product bug survived. Same family + as #7954 (a process-global promotion veto) and #7981 (a class-parent edge read + from a shape stamp). + + **The recorded obstacle was stale.** #7988/#7955 record the objection as "the + accessor is on `note_array_index_write`, the not-forwarded case must stay + call-free, and Darwin has no local-exec TLS". True of `std::thread_local!`; + not true of `crate::perry_thread_local!` (#7469, `tls_hot.rs`), which puts the + value's address in this thread's `HotTls` cache — an `mrs` plus two loads that + LLVM CSEs across the enclosing function rather than an out-of-line + `_tlv_get_addr` call. Both intrinsics now share ONE declaration (a + `[Cell; 2]` indexed positionally against `PROTOTYPE_ADDR_BUILTINS`), so + a function that consults both — `array_oob_prototype_get` does — pays one + resolution instead of two, and the `globalThis` walk moved behind a `#[cold]` + boundary so the hot arm is a slot load, a compare and a branch. The root + scanner iterates that array itself, so the #6981 invariant ("every cell an + accessor reads is a cell the collector rewrites") stays true by construction. + + Coverage: `a_second_agents_prototype_addresses_are_its_own` (in + `gc::tests::runtime_roots::prototype_addr_cache`, so `cargo-test`-visible) — + two live agents, bootstraps serialized and both threads held live across the + comparison by a barrier, must memoize **different** addresses; each address is + asserted to be a real one (non-zero, not the `usize::MAX` sentinel) *before* + the distinctness check, so a green verdict cannot be earned by two agents that + resolved nothing. `the_shipped_cells_are_the_ones_the_scanner_visits` gained + the assertion that every accessor's row lies inside the array the collector + rewrites. `test-files/test_issue_7988_thread_realm_prototype.ts` is the + behavioural multi-agent probe (main thread warms both intrinsics first, which + is what makes it discriminating rather than lucky). The gap suite and the + compile corpus are **vacuous gates** for this change — nothing in either uses + `perry/thread`. diff --git a/crates/perry-runtime/src/array/mod.rs b/crates/perry-runtime/src/array/mod.rs index 7d0063093e..eba92d949a 100644 --- a/crates/perry-runtime/src/array/mod.rs +++ b/crates/perry-runtime/src/array/mod.rs @@ -145,7 +145,7 @@ pub(crate) use self::prototype_addr::{ }; #[cfg(test)] pub(crate) use self::prototype_addr::{ - test_memoized_prototype_addr, test_prototype_addr_cache_wiring, + test_memoized_prototype_addr, test_prototype_addr_cache_wiring, test_prototype_addr_cell_count, test_rewrite_prototype_addr_slot, }; pub(crate) use self::sort::object_prototype_has_index_prop; diff --git a/crates/perry-runtime/src/array/prototype_addr.rs b/crates/perry-runtime/src/array/prototype_addr.rs index 4ab3c2d079..18401932d1 100644 --- a/crates/perry-runtime/src/array/prototype_addr.rs +++ b/crates/perry-runtime/src/array/prototype_addr.rs @@ -1,6 +1,7 @@ -//! The memoized `Array.prototype` / `Object.prototype` addresses (#6981). +//! The memoized `Array.prototype` / `Object.prototype` addresses (#6981), +//! **one pair per thread** (#7988). //! -//! Two `AtomicUsize` cells and the algebra over them: lazy resolution from +//! Two `usize` cells and the algebra over them: lazy resolution from //! `globalThis`, healing through the GC forwarding chain, and the registered //! root scanner that lets a relocating cycle rewrite them. //! @@ -9,106 +10,150 @@ //! invariant is easiest to keep true when the cells, the accessors and the //! scanner are the only things in the file (and it kept `indexing.rs` under //! `scripts/check_file_size.sh`'s 2000-line cap). +//! +//! # Why the cells are PER-THREAD (#7988) +//! +//! Both cells hold a **raw address of an object in a thread-local arena**, and +//! the realm they name is per-thread: `js_get_global_this` bootstraps +//! `THREAD_GLOBAL_THIS` once *per thread*, so every `perry/thread` agent has +//! its own `Array.prototype` and its own `Object.prototype` in its own heap. +//! When the cells were process-global `AtomicUsize` statics they missed only +//! once *per process*, so the first thread to touch either intrinsic decided +//! the value for every other agent, with three consequences: +//! +//! 1. **Wrong identity.** [`object_prototype_addr_matches`] on agent B +//! compared B's objects against *A's* `Object.prototype`, so B's own +//! intrinsic was never recognised. `array_oob_prototype_get`'s hole/OOB +//! fallbacks and the typed-feedback guards that consult it silently took +//! the other branch on every thread but the first — B's +//! `Object.prototype[7] = v` never even flipped `OBJECT_PROTO_HAS_INDEX`, +//! because the write hook's "is this the prototype?" test compared against +//! a foreign address. +//! 2. **Unattributed dereference.** [`heal_prototype_addr`] reads the cached +//! address's `GcHeader` from *any* thread, and `note_array_index_write` +//! calls it on every indexed array write. A's collector can sweep or move +//! that object, and A's arena blocks are `dealloc`'d at thread exit, so +//! the read was on memory the reading thread had no claim to. +//! 3. **Cross-thread root rewrite.** [`scan_prototype_addr_cache_roots_mut`] +//! is registered per thread and *writes* the cell with **its own** +//! to-space address, so agent B's collector could overwrite a cell naming +//! A's heap with a B-heap address. +//! +//! All three are structural once the storage is per-thread: a cell only ever +//! holds an address this thread allocated, healed by this thread's forwarding +//! chain and rewritten by this thread's collector. +//! +//! # Why a thread-local is affordable here +//! +//! [`crate::perry_thread_local`] is not `std::thread_local!`: the address of +//! the value lands in this thread's [`crate::tls_hot::HotTls`] cache, which on +//! Apple aarch64 is reached with an `mrs` plus two loads that LLVM CSEs across +//! the enclosing function — not the out-of-line `_tlv_get_addr` call that made +//! "Darwin has no local-exec TLS" the recorded objection to this fix (#7955). +//! Both intrinsics share ONE declaration (an array indexed by +//! [`ARRAY_PROTO_CACHE`] / [`OBJECT_PROTO_CACHE`]) so a function that consults +//! both — `array_oob_prototype_get` does — pays one resolution, not two. -use std::sync::atomic::{AtomicUsize, Ordering}; - -/// Lazily-memoized address of the `Array.prototype` array. An out-of-bounds -/// element read on an ordinary array must fall through to -/// `Array.prototype[index]` (ECMA-262 OrdinaryGet → prototype chain), but in -/// real code nobody adds numeric indices to `Array.prototype`, so the hot OOB -/// path stays a single relaxed atomic load until the (rare) write flips -/// `ARRAY_PROTO_HAS_INDEX`. `usize::MAX` marks the address as not-yet-computed. -/// -/// ***THIS IS A RAW ADDRESS OF A MOVABLE OBJECT*** (#6981). `Array.prototype` -/// relocates two different ways, and BOTH leave this cache pointing at a -/// `GC_FLAG_FORWARDED` stub while every reader resolves its own receiver -/// through `clean_arr_ptr` (which follows forwarding): -/// -/// 1. `js_array_grow` — an indexed write past the dense capacity -/// (`Array.prototype[300] = v`) reallocates and forwards the old head; -/// 2. the copying young-gen minor — it evacuates the prototype and forwards. -/// -/// A stale cache is not merely a wrong value: `array_oob_prototype_get`'s -/// self-recursion guard is `proto != receiver`, and after a move those are two -/// different addresses **for the same object**, so the guard stops firing and -/// `js_array_get_f64` ⇄ `array_oob_prototype_get` recurse until the stack guard -/// page (SIGSEGV, "excessive recursion"). Hence the two defences below: -/// [`memoized_prototype_addr`] resolves the forwarding chain and self-heals, -/// and [`scan_prototype_addr_cache_roots_mut`] lets the collector rewrite the -/// slot so the address stays live even once the from-space stub is recycled. -static ARRAY_PROTO_ADDR: AtomicUsize = AtomicUsize::new(usize::MAX); - -/// Same idea for `Object.prototype`: a numeric index installed there -/// (`Object.prototype[2] = 2`, or a defineProperty accessor) shows through -/// array HOLES and OOB reads (chain: arr → Array.prototype → -/// Object.prototype; test262 concat/S15.4.4.4_A3_T3). Consulted by the -/// typed-feedback guards and the hole/OOB read fallbacks. -static OBJECT_PROTO_ADDR: AtomicUsize = AtomicUsize::new(usize::MAX); - -/// One memoized intrinsic-prototype address: the cell, and the `globalThis` -/// builtin whose `.prototype` fills it. -/// -/// The pairing is a TABLE rather than two hand-written accessors so that the -/// two facts a reader has to trust — "the collector rewrites every cell some -/// accessor reads" and "each accessor resolves the builtin its cell is named -/// for" — are established by construction instead of by a test that has to -/// mutate a process-global to observe them (#7955). See -/// [`PROTOTYPE_ADDR_CACHES`]. -struct PrototypeAddrCache { - cell: &'static AtomicUsize, - /// `globalThis` key whose `.prototype` this cell memoizes. - builtin: &'static [u8], -} +use std::cell::Cell; -/// Every memoized prototype address in the runtime, in the order the GC root -/// scanner visits them. +/// How many intrinsic prototype addresses one realm memoizes. /// -/// [`scan_prototype_addr_cache_roots_mut`] iterates this table and -/// [`array_prototype_addr`] / [`object_prototype_addr`] index it, so a cell -/// that an accessor reads but the collector never rewrites — the #6981 defect -/// — is not representable. Adding a third memoized intrinsic address means -/// adding a row here, and it is covered by both halves automatically. -static PROTOTYPE_ADDR_CACHES: [PrototypeAddrCache; 2] = [ - PrototypeAddrCache { - cell: &ARRAY_PROTO_ADDR, - builtin: b"Array", - }, - PrototypeAddrCache { - cell: &OBJECT_PROTO_ADDR, - builtin: b"Object", - }, -]; +/// This is the length of BOTH the per-thread cell array and the builtin-name +/// table below, and the root scanner iterates the cell array itself, so +/// "a cell an accessor reads that the collector never rewrites" — the #6981 +/// defect — is not representable. Adding a third memoized intrinsic address +/// means bumping this and adding a row to [`PROTOTYPE_ADDR_BUILTINS`]; it is +/// then covered by both halves automatically. +const PROTOTYPE_ADDR_CACHE_COUNT: usize = 2; +/// Row index of the `Array.prototype` cell. const ARRAY_PROTO_CACHE: usize = 0; +/// Row index of the `Object.prototype` cell. const OBJECT_PROTO_CACHE: usize = 1; -/// GC root scanner for the memoized prototype addresses (#6981). +crate::perry_thread_local! { + /// **THIS THREAD's** lazily-memoized intrinsic prototype addresses, indexed + /// by [`ARRAY_PROTO_CACHE`] / [`OBJECT_PROTO_CACHE`]. `usize::MAX` marks a + /// row as not-yet-computed. + /// + /// Row 0 is `Array.prototype`. An out-of-bounds element read on an ordinary + /// array must fall through to `Array.prototype[index]` (ECMA-262 + /// OrdinaryGet → prototype chain), but in real code nobody adds numeric + /// indices to `Array.prototype`, so the hot OOB path stays one load until + /// the (rare) write flips `ARRAY_PROTO_HAS_INDEX`. + /// + /// Row 1 is `Object.prototype`: a numeric index installed there + /// (`Object.prototype[2] = 2`, or a defineProperty accessor) shows through + /// array HOLES and OOB reads (chain: arr → Array.prototype → + /// Object.prototype; test262 concat/S15.4.4.4_A3_T3). Consulted by the + /// typed-feedback guards and the hole/OOB read fallbacks. + /// + /// ***THESE ARE RAW ADDRESSES OF MOVABLE OBJECTS*** (#6981). + /// `Array.prototype` relocates two different ways, and BOTH leave the cache + /// pointing at a `GC_FLAG_FORWARDED` stub while every reader resolves its + /// own receiver through `clean_arr_ptr` (which follows forwarding): + /// + /// 1. `js_array_grow` — an indexed write past the dense capacity + /// (`Array.prototype[300] = v`) reallocates and forwards the old head; + /// 2. the copying young-gen minor — it evacuates the prototype and + /// forwards. + /// + /// A stale cache is not merely a wrong value: `array_oob_prototype_get`'s + /// self-recursion guard is `proto != receiver`, and after a move those are + /// two different addresses **for the same object**, so the guard stops + /// firing and `js_array_get_f64` ⇄ `array_oob_prototype_get` recurse until + /// the stack guard page (SIGSEGV, "excessive recursion"). Hence the two + /// defences below: [`memoized_prototype_addr`] resolves the forwarding + /// chain and self-heals, and [`scan_prototype_addr_cache_roots_mut`] lets + /// the collector rewrite the slot so the address stays live even once the + /// from-space stub is recycled. + static PROTOTYPE_ADDRS: [Cell; PROTOTYPE_ADDR_CACHE_COUNT] = + const { [const { Cell::new(usize::MAX) }; PROTOTYPE_ADDR_CACHE_COUNT] }; +} + +/// The `globalThis` builtin whose `.prototype` fills each row of +/// [`PROTOTYPE_ADDRS`], in the same order. /// -/// `ARRAY_PROTO_ADDR` / `OBJECT_PROTO_ADDR` hold raw addresses of movable -/// objects, so a relocating cycle must REWRITE them exactly like the other -/// address-holding side tables (`CLASS_PROTOTYPE_OBJECTS`, -/// `TYPED_ARRAY_VIEW_META`, …). Forwarding-chain healing alone is not -/// sufficient: once the from-space stub is swept and its block recycled the -/// `GC_FLAG_FORWARDED` bit is gone, and the cache would then name an unrelated -/// live object. Both intrinsics are reachable from `globalThis`, so the marking -/// half of this visit is redundant; the rewriting half is the point. +/// The pairing is POSITIONAL rather than two hand-written accessors so that the +/// second fact a reader has to trust — "each accessor resolves the builtin its +/// cell is named for" — is established by construction instead of by a test +/// that has to mutate a process-global to observe it (#7955). +static PROTOTYPE_ADDR_BUILTINS: [&[u8]; PROTOTYPE_ADDR_CACHE_COUNT] = [b"Array", b"Object"]; + +/// GC root scanner for this thread's memoized prototype addresses (#6981). +/// +/// The cells hold raw addresses of movable objects, so a relocating cycle must +/// REWRITE them exactly like the other address-holding side tables +/// (`CLASS_PROTOTYPE_OBJECTS`, `TYPED_ARRAY_VIEW_META`, …). Forwarding-chain +/// healing alone is not sufficient: once the from-space stub is swept and its +/// block recycled the `GC_FLAG_FORWARDED` bit is gone, and the cache would then +/// name an unrelated live object. Both intrinsics are reachable from +/// `globalThis`, so the marking half of this visit is redundant; the rewriting +/// half is the point. +/// +/// #7988: the scanner runs on the collecting thread and now visits **that +/// thread's** cells. Before the storage was per-thread it wrote the collecting +/// thread's to-space address into a cell that could be naming another agent's +/// heap. pub fn scan_prototype_addr_cache_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { - for entry in &PROTOTYPE_ADDR_CACHES { - rewrite_prototype_addr_slot(entry.cell, visitor); - } + PROTOTYPE_ADDRS.with(|cells| { + for cell in cells { + rewrite_prototype_addr_slot(cell, visitor); + } + }); } /// The per-cell half of [`scan_prototype_addr_cache_roots_mut`]. /// /// Split out so the #6981 rewrite algebra can be exercised on a cell the test -/// owns privately: driving it through the shipped `static`s made the assertion -/// depend on no other libtest thread touching the realm's real intrinsics -/// meanwhile, which is the #7955 flake. +/// owns privately: driving it through the realm's real intrinsics made the +/// assertion depend on no other libtest thread touching them meanwhile, which +/// is the #7955 flake. fn rewrite_prototype_addr_slot( - cache: &AtomicUsize, + cache: &Cell, visitor: &mut crate::gc::RuntimeRootVisitor<'_>, ) { - let cached = cache.load(Ordering::Relaxed); + let cached = cache.get(); if cached == usize::MAX || cached == 0 { return; } @@ -120,7 +165,7 @@ fn rewrite_prototype_addr_slot( // relocated the object, and the value written is the visitor's own // to-space address — barriering it would push an edge into the // remembered set that this very cycle is rebuilding. - cache.store(addr, Ordering::Relaxed); + cache.set(addr); } } @@ -128,14 +173,14 @@ fn rewrite_prototype_addr_slot( /// chain. `None` means "not resolved yet" — the caller must run the /// `globalThis` bootstrap. #[inline] -fn memoized_prototype_addr(cache: &AtomicUsize) -> Option { - let cached = cache.load(Ordering::Relaxed); +fn memoized_prototype_addr(cache: &Cell) -> Option { + let cached = cache.get(); (cached != usize::MAX).then(|| heal_prototype_addr(cache, cached)) } /// Re-read a memoized prototype address through the GC forwarding chain and /// write the healed address back, so every caller compares (and dereferences) -/// the object's CURRENT location. See the [`ARRAY_PROTO_ADDR`] doc for why an +/// the object's CURRENT location. See the [`PROTOTYPE_ADDRS`] doc for why an /// unresolved cache is a hang, not just a wrong answer (#6981). /// /// `note_array_index_write` calls this on every indexed array write until the @@ -145,8 +190,14 @@ fn memoized_prototype_addr(cache: &AtomicUsize) -> Option { /// address. It also classifies the address band before dereferencing, so the /// not-yet-resolved sentinel (`usize::MAX`) and any non-heap value fall /// straight through. +/// +/// #7988: the address is now always one THIS thread allocated, so the header +/// read has an owner. It used to be able to land on another agent's arena +/// block — possibly already swept, moved, or `dealloc`'d at that thread's exit +/// — and a stray `GC_FLAG_FORWARDED` byte there sent `resolve_forwarding` one +/// word further into it. #[inline] -fn heal_prototype_addr(cache: &AtomicUsize, cached: usize) -> usize { +fn heal_prototype_addr(cache: &Cell, cached: usize) -> usize { let forwarded = unsafe { crate::value::addr_class::try_read_gc_header(cached) .is_some_and(|header| header.gc_flags & crate::gc::GC_FLAG_FORWARDED != 0) @@ -156,22 +207,33 @@ fn heal_prototype_addr(cache: &AtomicUsize, cached: usize) -> usize { } let resolved = crate::value::resolve_forwarding(cached); if resolved != cached { - cache.store(resolved, Ordering::Relaxed); + cache.set(resolved); } resolved } -/// Resolve one row of [`PROTOTYPE_ADDR_CACHES`]: the memoized address if it is -/// already known (healed through any forwarding chain), otherwise the +/// Resolve one row of [`PROTOTYPE_ADDRS`]: the memoized address if this thread +/// already knows it (healed through any forwarding chain), otherwise the /// `globalThis` bootstrap, memoized. -fn resolve_prototype_addr(entry: &PrototypeAddrCache) -> usize { - if let Some(addr) = memoized_prototype_addr(entry.cell) { +#[inline] +fn resolve_prototype_addr(slot: usize) -> usize { + if let Some(addr) = PROTOTYPE_ADDRS.with(|cells| memoized_prototype_addr(&cells[slot])) { return addr; } - let ctor = crate::object::js_get_global_this_builtin_value( - entry.builtin.as_ptr(), - entry.builtin.len(), - ); + bootstrap_prototype_addr(slot) +} + +/// The cold half of [`resolve_prototype_addr`]: derive the intrinsic's address +/// from THIS thread's `globalThis` and memoize it. +/// +/// Out of line so the hot arm — `note_array_index_write` on every indexed array +/// write — is a slot load, a compare and a branch, with the whole `globalThis` +/// walk off the fast path. +#[cold] +#[inline(never)] +fn bootstrap_prototype_addr(slot: usize) -> usize { + let builtin = PROTOTYPE_ADDR_BUILTINS[slot]; + let ctor = crate::object::js_get_global_this_builtin_value(builtin.as_ptr(), builtin.len()); let ctor_value = crate::value::JSValue::from_bits(ctor.to_bits()); let addr = if ctor_value.is_pointer() { let ctor_ptr = ctor_value.as_pointer::() as usize; @@ -190,53 +252,63 @@ fn resolve_prototype_addr(entry: &PrototypeAddrCache) -> usize { // call into here via `note_array_proto_iterator_write`). Re-derive until it // resolves. if addr != 0 { - entry.cell.store(addr, Ordering::Relaxed); + PROTOTYPE_ADDRS.with(|cells| cells[slot].set(addr)); } addr } pub(crate) fn array_prototype_addr() -> usize { - resolve_prototype_addr(&PROTOTYPE_ADDR_CACHES[ARRAY_PROTO_CACHE]) + resolve_prototype_addr(ARRAY_PROTO_CACHE) } pub(crate) fn object_prototype_addr() -> usize { - resolve_prototype_addr(&PROTOTYPE_ADDR_CACHES[OBJECT_PROTO_CACHE]) + resolve_prototype_addr(OBJECT_PROTO_CACHE) } -/// `true` when `addr` is the canonical `Object.prototype` (cheap: cached -/// atomic + compare; lazily computes the address on first use). +/// `true` when `addr` is **this realm's** canonical `Object.prototype` (cheap: +/// one slot load + compare; lazily computes the address on first use). pub(crate) fn object_prototype_addr_matches(addr: usize) -> bool { addr != 0 && addr == object_prototype_addr() } -/// Test-only handle on the shipped table, for the read-only wiring assertion -/// in `gc::tests::runtime_roots::prototype_addr_cache`. The mutating #6981 -/// cases run on cells they own; nothing hands out a writable reference to the -/// realm's real intrinsic cells any more (#7955). +/// Test-only handle on the shipped wiring, for the read-only assertion in +/// `gc::tests::runtime_roots::prototype_addr_cache`. Returns each accessor's +/// row index paired with the `globalThis` builtin that row bootstraps from. +/// The mutating #6981 cases run on cells they own; nothing hands out a writable +/// reference to the realm's real intrinsic cells (#7955). #[cfg(test)] -pub(crate) fn test_prototype_addr_cache_wiring() -> [(&'static AtomicUsize, &'static [u8]); 2] { +pub(crate) fn test_prototype_addr_cache_wiring() -> [(usize, &'static [u8]); 2] { [ ( - PROTOTYPE_ADDR_CACHES[ARRAY_PROTO_CACHE].cell, - PROTOTYPE_ADDR_CACHES[ARRAY_PROTO_CACHE].builtin, + ARRAY_PROTO_CACHE, + PROTOTYPE_ADDR_BUILTINS[ARRAY_PROTO_CACHE], ), ( - PROTOTYPE_ADDR_CACHES[OBJECT_PROTO_CACHE].cell, - PROTOTYPE_ADDR_CACHES[OBJECT_PROTO_CACHE].builtin, + OBJECT_PROTO_CACHE, + PROTOTYPE_ADDR_BUILTINS[OBJECT_PROTO_CACHE], ), ] } +/// Number of cells this thread owns — i.e. how many +/// [`scan_prototype_addr_cache_roots_mut`] visits. Read by the wiring test so +/// "the scanner visits every cell an accessor can index" is asserted rather +/// than assumed. +#[cfg(test)] +pub(crate) fn test_prototype_addr_cell_count() -> usize { + PROTOTYPE_ADDRS.with(|cells| cells.len()) +} + /// The two halves of the #6981 defences, exported so the tests can drive them -/// on a private cell instead of the process-global one (#7955). +/// on a private cell instead of this thread's real ones (#7955). #[cfg(test)] -pub(crate) fn test_memoized_prototype_addr(cache: &AtomicUsize) -> Option { +pub(crate) fn test_memoized_prototype_addr(cache: &Cell) -> Option { memoized_prototype_addr(cache) } #[cfg(test)] pub(crate) fn test_rewrite_prototype_addr_slot( - cache: &AtomicUsize, + cache: &Cell, visitor: &mut crate::gc::RuntimeRootVisitor<'_>, ) { rewrite_prototype_addr_slot(cache, visitor) diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/prototype_addr_cache.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/prototype_addr_cache.rs index 1f9d2abadc..99c32ac78c 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots/prototype_addr_cache.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots/prototype_addr_cache.rs @@ -1,8 +1,10 @@ //! #6981 — the memoized `Array.prototype` / `Object.prototype` addresses are //! raw pointers to MOVABLE objects, so they must survive relocation. //! -//! `array::prototype_addr` memoizes both intrinsic addresses in process-global -//! `AtomicUsize` cells. `Array.prototype` relocates two ways, and both leave a +//! `array::prototype_addr` memoizes both intrinsic addresses in a PER-THREAD +//! pair of cells (#7988 — the realm they name is per-thread, so a process-wide +//! cell handed every `perry/thread` agent the first thread's addresses). +//! `Array.prototype` relocates two ways, and both leave a //! `GC_FLAG_FORWARDED` stub at the memoized address: //! //! * `js_array_grow` — `Array.prototype[300] = v` reallocates the dense @@ -40,24 +42,33 @@ //! better: restoring the value read at test entry stamps a stale address over //! whatever another thread resolved meanwhile. //! -//! Both defences are algebra over an `&AtomicUsize`, so each case now owns its -//! cell and the shipped `static`s are never written from a test. What that +//! Both defences are algebra over a `&Cell`, so each case now owns its +//! cell and the realm's real cells are never written from a test. What that //! decomposition would otherwise lose — "the collector rewrites every cell an //! accessor reads" — is not recovered by a test at all but by CONSTRUCTION: -//! `PROTOTYPE_ADDR_CACHES` is one table, the scanner iterates it and the -//! accessors index it. `the_shipped_cells_are_the_ones_the_scanner_visits` -//! pins the table itself, read-only, so it cannot be raced either. +//! `PROTOTYPE_ADDRS` is one fixed-length per-thread array, the scanner iterates +//! that array and the accessors index it positionally against +//! `PROTOTYPE_ADDR_BUILTINS`. `the_shipped_cells_are_the_ones_the_scanner_visits` +//! pins the wiring itself, read-only, so it cannot be raced either. +//! +//! # The per-thread half (#7988) +//! +//! `a_second_agents_prototype_addresses_are_its_own` is the isolation gate: two +//! live threads, each with its own realm and its own arena, must memoize +//! DIFFERENT addresses. It is deliberately not satisfiable by a run in which +//! nothing happened — it asserts both threads resolved a real (non-zero, +//! non-sentinel) address first, so "distinct" cannot be earned by two failures. use super::*; -use std::sync::atomic::{AtomicUsize, Ordering}; +use std::cell::Cell; /// A memoized-prototype-address cell owned by ONE test. /// /// `memoized_prototype_addr` / `rewrite_prototype_addr_slot` take the cell as /// an argument, so the #6981 algebra is exercised exactly as shipped without /// any test writing to the realm's real intrinsic cells. -fn private_cache_cell() -> AtomicUsize { - AtomicUsize::new(usize::MAX) +fn private_cache_cell() -> Cell { + Cell::new(usize::MAX) } /// Allocate a nursery object to stand in for the intrinsic. @@ -118,7 +129,7 @@ fn prototype_addr_reads_through_a_forwarding_stub() { for which in ["array", "object"] { let cell = private_cache_cell(); let (from, to) = forwarded_pair(); - cell.store(from, Ordering::Relaxed); + cell.set(from); assert_eq!( crate::array::test_memoized_prototype_addr(&cell), @@ -130,7 +141,7 @@ fn prototype_addr_reads_through_a_forwarding_stub() { fallback and hangs the mutator (#6981)" ); assert_eq!( - cell.load(Ordering::Relaxed), + cell.get(), to, "the read must write the healed address back so the hot path stays \ a single relaxed load" @@ -145,7 +156,7 @@ fn prototype_addr_reads_through_a_forwarding_stub() { fn an_unresolved_prototype_cell_reports_no_address() { let cell = private_cache_cell(); assert_eq!(crate::array::test_memoized_prototype_addr(&cell), None); - assert_eq!(cell.load(Ordering::Relaxed), usize::MAX); + assert_eq!(cell.get(), usize::MAX); } /// Multi-hop chains (grow, then grow again, then evacuate) must resolve all the @@ -166,7 +177,7 @@ fn prototype_addr_reads_through_a_multi_hop_forwarding_chain() { } let cell = private_cache_cell(); - cell.store(first as usize, Ordering::Relaxed); + cell.set(first as usize); assert_eq!( crate::array::test_memoized_prototype_addr(&cell), Some(final_user as usize), @@ -195,8 +206,8 @@ fn prototype_addr_cache_is_rewritten_by_the_collector() { let object_to = evacuate(object_from); let array_cell = private_cache_cell(); let object_cell = private_cache_cell(); - array_cell.store(array_from as usize, Ordering::Relaxed); - object_cell.store(object_from as usize, Ordering::Relaxed); + array_cell.set(array_from as usize); + object_cell.set(object_from as usize); for cell in [&array_cell, &object_cell] { crate::array::test_rewrite_prototype_addr_slot( @@ -206,14 +217,14 @@ fn prototype_addr_cache_is_rewritten_by_the_collector() { } assert_eq!( - array_cell.load(Ordering::Relaxed), + array_cell.get(), array_to, "a memoized prototype cell must be rewritten by the relocating cycle — \ it is a raw address of a movable object, exactly like the other \ registered side tables (#6981)" ); assert_eq!( - object_cell.load(Ordering::Relaxed), + object_cell.get(), object_to, "the rewrite is per-cell, so both rows of PROTOTYPE_ADDR_CACHES get it \ (#6981)" @@ -252,19 +263,20 @@ fn prototype_addr_cache_scanner_leaves_the_unset_sentinel_alone() { &mut RuntimeRootVisitor::for_rewrite(&valid_ptrs), ); - assert_eq!(cell.load(Ordering::Relaxed), usize::MAX); + assert_eq!(cell.get(), usize::MAX); } /// The WIRING, and deliberately read-only so it cannot be raced (#7955). /// /// The cases above prove the algebra on cells they own; on its own that would -/// leave nothing asserting that the shipped `static`s are the cells in play — -/// the "gate runs but its subject never did" shape. `PROTOTYPE_ADDR_CACHES` is -/// the single table the scanner iterates and the accessors index, so this -/// pins the table: two DISTINCT cells (a copy-pasted row would give +/// leave nothing asserting that the realm's real cells are the cells in play — +/// the "gate runs but its subject never did" shape. `PROTOTYPE_ADDRS` is the +/// single per-thread array the scanner iterates and the accessors index, so +/// this pins the wiring: two DISTINCT rows (a copy-pasted index would give /// `Array.prototype`'s address to `object_prototype_addr()` and leave one cell -/// unrewritten), each paired with the `globalThis` builtin whose `.prototype` -/// its accessor resolves. Nothing here writes. +/// unrewritten), each paired positionally with the `globalThis` builtin whose +/// `.prototype` its accessor resolves, and the scanner covering EVERY row an +/// accessor can index. Nothing here writes. #[test] fn the_shipped_cells_are_the_ones_the_scanner_visits() { let wiring = crate::array::test_prototype_addr_cache_wiring(); @@ -278,10 +290,106 @@ fn the_shipped_cells_are_the_ones_the_scanner_visits() { "row 1 is what object_prototype_addr() indexes; it must bootstrap from \ globalThis.Object" ); - assert!( - !std::ptr::eq(wiring[0].0, wiring[1].0), + assert_ne!( + wiring[0].0, wiring[1].0, "the two intrinsics must memoize into DIFFERENT cells — sharing one \ cell makes the second accessor return the first intrinsic's address \ and leaves the collector with nothing to rewrite for it (#6981)" ); + // The scanner iterates the per-thread cell array itself, so covering every + // accessor's row reduces to the array being at least as long as the highest + // index an accessor uses. + let cells = crate::array::test_prototype_addr_cell_count(); + for (row, builtin) in wiring { + assert!( + row < cells, + "accessor row {row} ({}) is outside the {cells}-cell array the \ + collector rewrites — that cell would never be visited (#6981)", + String::from_utf8_lossy(builtin) + ); + } +} + +/// #7988 — ISOLATION. Two live `perry/thread`-style agents must memoize their +/// OWN realm's intrinsics. +/// +/// Each thread bootstraps its own `globalThis` into its own arena, so its +/// `Array.prototype` / `Object.prototype` are different objects at different +/// addresses. When the cells were process-global the second thread never even +/// missed: it read the first thread's addresses, compared its own objects +/// against a foreign heap (so `object_prototype_addr_matches` never matched), +/// dereferenced that foreign address's `GcHeader` on every indexed array +/// write, and let its own collector rewrite the cell with a to-space address +/// from the wrong heap. +/// +/// LIVENESS. "The two addresses differ" is satisfiable by two threads that both +/// resolved NOTHING, so each thread's address is asserted to be a real one +/// (non-zero and not the `usize::MAX` not-yet-resolved sentinel) before the +/// distinctness check runs — the two paths to a green verdict are separated. +/// The bootstraps are serialized (`GLOBAL_THIS_PTR`, the process-global root +/// slot, is written by each one) but both threads are held live across the +/// comparison by a barrier, so the two arenas provably coexist and the +/// addresses cannot have merely been recycled. +#[test] +fn a_second_agents_prototype_addresses_are_its_own() { + use std::sync::{Arc, Barrier, Mutex}; + + let bootstrap_gate = Arc::new(Mutex::new(())); + let both_alive = Arc::new(Barrier::new(2)); + + let agent = |gate: Arc>, barrier: Arc| { + move || -> (usize, usize) { + let addrs = { + let _serialized = gate.lock().expect("bootstrap gate"); + ( + crate::array::array_prototype_addr(), + crate::array::object_prototype_addr(), + ) + }; + // Hold this thread — and therefore its arena — alive until the other + // agent has resolved too. Without this the second thread could be + // handed a recycled address and "distinct" would prove nothing. + barrier.wait(); + addrs + } + }; + + let spawn_agent = |gate: Arc>, barrier: Arc| { + std::thread::Builder::new() + .stack_size(16 << 20) + .spawn(agent(gate, barrier)) + .expect("spawn realm agent") + }; + + let a = spawn_agent(Arc::clone(&bootstrap_gate), Arc::clone(&both_alive)); + let b = spawn_agent(bootstrap_gate, both_alive); + let (a_array, a_object) = a.join().expect("agent A panicked"); + let (b_array, b_object) = b.join().expect("agent B panicked"); + + for (label, addr) in [ + ("A Array.prototype", a_array), + ("A Object.prototype", a_object), + ("B Array.prototype", b_array), + ("B Object.prototype", b_object), + ] { + assert!( + addr != 0 && addr != usize::MAX, + "{label} did not resolve ({addr:#x}) — the distinctness assertion \ + below would then be satisfied by two agents that resolved nothing" + ); + } + + assert_ne!( + a_array, b_array, + "two live agents must memoize their OWN Array.prototype: a shared cell \ + makes the second agent compare its arrays against the first agent's \ + heap and read that heap's GcHeader on every indexed write (#7988)" + ); + assert_ne!( + a_object, b_object, + "two live agents must memoize their OWN Object.prototype: a shared cell \ + is why `Object.prototype[i] = v` on a worker never flipped \ + OBJECT_PROTO_HAS_INDEX — the write hook compared against the main \ + thread's address (#7988)" + ); } diff --git a/gc-handoff/REALM-NOTES.md b/gc-handoff/REALM-NOTES.md new file mode 100644 index 0000000000..073fbef24b --- /dev/null +++ b/gc-handoff/REALM-NOTES.md @@ -0,0 +1,251 @@ +# REALM-NOTES — per-realm state under `perry/thread` (#7988, #6763) + +Working notes for the "what is process-global that should be per-realm?" thread. +Written incrementally; the PR is #7994. + +## The shape + +`perry/thread` gives each OS thread its **own realm** (`THREAD_GLOBAL_THIS`, +`crates/perry-runtime/src/object/global_this/fetch_globals.rs:13`) and its **own +arena** (`ARENA` / `OLD_ARENA` / `LONGLIVED_ARENA`, all `thread_local!`). A +process-global `static` that holds either + +* a **raw heap address** — it names an object in one thread's arena, which the + owning thread's collector may sweep or move and whose blocks are `dealloc`'d + at that thread's exit; or +* **per-realm identity** — "which object *is* this realm's `Object.prototype` / + `C.prototype` / `localStorage`", + +is a bug with three faces: + +1. **Wrong identity** — agent B compares its own objects against A's. +2. **Unattributed dereference** — B reads a `GcHeader` in A's arena. +3. **Cross-thread root rewrite** — B's collector stores its own to-space address + into a cell that names A's heap. This is the dangerous one. + +A **sticky "somebody somewhere patched X" flag is NOT this bug** — it is a safe +over-approximation (every thread takes the slow path; nobody gets a wrong +answer). `ARRAY_PROTO_HAS_INDEX` / `OBJECT_PROTO_HAS_INDEX` / +`PERRY_ARRAY_INDEX_FAST_PATH_INVALIDATED` are deliberately left process-global +for exactly that reason. + +Three members of the family were found in one day, all as side effects of +chasing test flakes: #7954 (a promotion veto read from a process-global), #7981 +(a class-parent edge taken from a shape stamp), #7988 (this one). + +## #7988 — what was fixed + +`crates/perry-runtime/src/array/prototype_addr.rs`: `ARRAY_PROTO_ADDR` and +`OBJECT_PROTO_ADDR` were `static AtomicUsize`, holding raw addresses of +`Array.prototype` / `Object.prototype`. `js_get_global_this` bootstraps once per +*thread*; `resolve_prototype_addr` missed once per *process*. First thread wins, +for the life of the process. + +Now: one `crate::perry_thread_local!` holding `[Cell; 2]`, indexed +positionally against `PROTOTYPE_ADDR_BUILTINS`. The root scanner iterates that +array, so the #6981 invariant ("every cell an accessor reads is a cell the +collector rewrites") survives by construction. + +### The recorded obstacle was stale + +#7988 and #7955 both record the objection as *"the accessor is on +`note_array_index_write`, the not-forwarded case must stay call-free, and Darwin +has no local-exec TLS"*. That is true of `std::thread_local!` — on Darwin every +access is an out-of-line `_tlv_get_addr` call in `libdyld`. + +It is **not** true of `crate::perry_thread_local!` (#7469, +`crates/perry-runtime/src/tls_hot.rs`). That macro publishes the value's address +into this thread's `HotTls` cache, which on Apple aarch64 is reached through the +pthread TSD array directly from `TPIDRRO_EL0`: an `mrs` plus two loads that LLVM +CSEs across the whole enclosing function. `tls_hot.rs`'s own module docs say the +declaration form is the **default** and there is nothing to wire. + +**Anyone hitting "we can't make this per-thread, Darwin TLS is a call" in this +runtime should check `tls_hot.rs` first — that objection has been obsolete since +#7469.** It is currently repeated verbatim in at least two issues. + +### Measured cost of the per-thread read + +One compiler, two `libperry_runtime.a` + `libperry_stdlib.a` pairs, swapped with +`PERRY_RUNTIME_DIR`, `PERRY_NO_AUTO_OPTIMIZE=1` so the pinned archive is what +gets linked. Nine interleaved rounds on the quiet M1 mini +(`perry@perry-macos.local`, load ~1.4 throughout), with a third binary that is a +**byte-identical copy of the fixed arm** as the A/A noise floor. Medians of +rounds 2-9 (round 1 discarded as warm-up): + +| phase | what it exercises | baseline | fixed | A/A control | +|---|---|---|---|---| +| `fieldSets` (4M × set+get, runtime key) | `object_prototype_addr_matches` on the by-name set fast path | **398 ms** | **403 ms** | 403 ms | +| `fills` (20k × `new Array(512).fill(r)`) | `note_array_index_write` → `array_prototype_addr()` | 38 ms | 38 ms | 38 ms | +| `misses` (4M × absent-key read) | `field_get_set/accessors.rs`'s `object_prototype_addr()` | 3-4 ms | 3-4 ms | 3-4 ms | + +Checksums identical across arms. The A/A control lands on the same median as +the fixed arm, so the noise floor is under 1 ms on a ~400 ms measurement +(< 0.25 %) and the +5 ms is real, not drift. + +**+1.3 % on a microbenchmark that does nothing but property set/get** — about +0.6 ns, ~2 cycles, per accessor call. That is the cost of one extra load and a +bound check on top of a `hot()` resolution LLVM CSEs across the function. The +array-fill phase did not move measurably; the `misses` phase is too small to +resolve and is reported as such rather than as a win. + +### Sabotage record + +The isolation gate was verified able to fail, on a **rebuilt** binary rather +than an edited tree. A temporary process-global mirror was added to +`resolve_prototype_addr` (first thread to resolve decides for the process), +`cargo test --release -p perry-runtime prototype_addr_cache` rebuilt, and the +result was exactly one failure: + +``` +a_second_agents_prototype_addresses_are_its_own ... FAILED + assertion `left != right` failed: two live agents must memoize their OWN Array.prototype ... + left: 3109324032744 right: 3109324032744 +test result: FAILED. 7 passed; 1 failed +``` + +The other seven cases — the #6981 forwarding/rewrite algebra on privately-owned +cells — are unaffected by the sabotage, which is the intended decomposition. The +sabotage was then reverted **and rebuilt**: 8 passed, 0 failed. + +## The multi-agent probe + +`test-files/test_issue_7988_thread_realm_prototype.ts`. The **main thread warms +both intrinsics before any agent starts** — without that the worker might be the +thread that fills the shared cell and the probe passes on the broken tree. Then +an agent pollutes its own realm (`Object.prototype[7]`, `Array.prototype[8]`) +and reads `[1,2,3][7]` / `[1,2,3][8]` through the chain. + +Neither the gap suite nor the compile corpus exercises `perry/thread` at all, so +**both are vacuous gates for this class of change**. Do not cite corpus-green as +evidence for a `perry/thread` fix. + +The first version of this probe was itself vacuous, and it took an A/B against a +real pre-fix `.a` to notice: its warm-up (`main[1] = 9`, `main[7]`) resolves +NEITHER address, so the spawned agent was simply the first thread to fill the +shared cell, and the probe printed the expected string 5/5 on the broken +runtime. Two lessons worth carrying: + +* **A `perry/thread` probe must state which main-thread operation resolves the + state under test, and print it.** Guessing wrong is silent. +* `note_array_index_write` is NOT reached by an ordinary `arr[i] = v`; the + reachable-from-JS callers are the bulk fill/extend helpers and an indexed + write *to the prototype object itself*. `array_oob_prototype_get`'s call to + `array_prototype_addr()` sits **behind** `ARRAY_PROTO_HAS_INDEX`, so an + out-of-bounds read on a clean realm resolves nothing. + +### Perry-only tests need a stored expected output, not a tolerated failure + +`run_parity_tests.sh` compares against Node, and `perry/thread` has no Node +equivalent, so a `perry/thread` test scores `parity_fail` forever. Four already +do on `main` — `parity_known_failures.py` reports +`test_issue_{4449_thread_promise_void, 7302_thread_throws, +7769_thread_class_dispatch, 7981_thread_shape_stamp_parent}` as unlisted +failures on macOS, and none is in `test-parity/known_failures.json`. + +The harness already has the mechanism: `test-parity/expected/.txt` is +compared against Perry's output *and exit code* instead of against Node +(`run_parity_tests.sh:1438`; the `threaded-fd-semantics-*` files use it). This +PR's probe ships one, so it PASSES the parity suite rather than being tolerated +by it. **The four siblings should get the same treatment** — as written they are +four tests whose next regression is already absorbed. + +## Inventory: other process-globals that should be per-realm + +Ranked. Every entry is a `static` (not a `thread_local!`) holding an address or +a per-realm identity. Not yet fixed — filed for follow-up. + +| # | Declaration | File | Hazard | Symptom for agent B | +|---|---|---|---|---| +| 1 | `CLASS_PROTOTYPE_OBJECTS`, `CLASS_DECL_PROTOTYPE_OBJECTS`, `CLASS_PARENT_CLOSURES` | `object/class_registry/state.rs:316,325,381` | all three | `class_id` is a compile-time constant shared by every thread; the **value** is `C.prototype`'s address in one thread's arena. `prototype_objects.rs:34` is the first-thread-wins gate. `new C()` on B embeds A's `C.prototype` into a live object B just built, and `class_gc_roots.rs:44` rewrites the shared map from every thread's collector. Worse than #7988: a foreign pointer *stored into the heap graph*, not merely compared. | +| 2 | `GLOBAL_THIS_PTR` / `GLOBAL_THIS_READY` | `object/mod.rs:252` | 1, 3 | The realm root itself. `THREAD_GLOBAL_THIS` already shields the common read path (and its doc comment states the hazard), but the process-global slot is still written by every thread's first `js_get_global_this` and is a registered scanner target (`object/mod.rs:1151`). | +| 3 | `ITERATOR_PROTOTYPE_PTR` + 5 siblings | `object/iterator_prototypes.rs:41` | 1, 2 | `ensure_iterator_prototypes()` is a once-per-**process** gate; `attach_iterator_prototype` then chains every new iterator on every thread to whichever thread built the tower. Hit by any `for..of` / spread. | +| 4 | generator / async-generator intrinsic tower (6 cells) | `object/mod.rs:280-285` | 1, 2 | same once-per-process gate in `global_this/generator.rs:716`. #7251 fixed the *test* isolation via `per_test_global!`, which expands to a plain `static` in production. | +| 5 | `TYPED_ARRAY_INTRINSIC_PTR` / `..._PROTO_PTR` | `object/mod.rs:256` | 1, 2 | `global_this/typed_array.rs:418`; its own comment asserts "single-threaded under the singleton CAS", which stops being true the moment a second thread runs `populate_global_this_builtins`. | +| 6 | `HTTP_METHODS_CACHE`, `FS_CONSTANTS_CACHE`, 5 `OS_CONSTANTS_*` | `object/mod.rs:245-251` | 2 | built with `*_longlived` allocators, and `LONGLIVED_ARENA` is thread-local (`arena/block.rs:992`). B's first `require('http').METHODS` hands back A's array; guaranteed dangling once A exits. | +| 7 | `LOCAL_STORAGE_PTR` / `SESSION_STORAGE_PTR` | `object/mod.rs:287` | 1 | not even `per_test_global!`. `web_storage.rs:375` brand-checks by pointer equality, and `web_storage.rs:220` overwrites both on every thread's bootstrap — so A's own valid `localStorage` call starts failing its brand check after B starts. | +| 8 | `FUNCTION_CLASS_IDS` | `object/class_registry/state.rs:309` | 1 | keyed by NaN-boxed closure **heap pointer** bits. Narrower (needs an address collision across two arenas) but the same shape. | +| 9 | `SYMBOL_PROPERTIES` / `SYMBOL_PROPERTY_ATTRS` / `CLASS_STATIC_SYMBOLS` | `symbol.rs:499,506,819` | leak only | already documented in-file at `symbol.rs:508`: cross-thread owners are only reclaimed by the owning thread. Bounded leak, not corruption. | + +Not fully triaged, same neighbourhood as #1 and worth the same treatment: +`CLASS_VTABLE_REGISTRY`, `CLASS_STATIC_METHODS`, `CLASS_STATIC_ACCESSORS`, +`CLASS_SYMBOL_METHODS`, `CLASS_SYMBOL_ACCESSORS`, `CLASS_DYNAMIC_PARENT_VALUE` +(a `u64` that looks like it could be a NaN-boxed JSValue, i.e. a heap pointer), +`CLASS_OBJECT_VALUES` — all `object/class_registry/state.rs:241-407`. + +### Checked and ruled out (this half matters as much) + +* `SYMBOL_REGISTRY` / `WELL_KNOWN_SYMBOLS` / `REGISTERED_SYMBOL_DESCRIPTIONS` + (`symbol.rs:112,270,132`) — `Symbol.for()` identity is spec-mandated to be + shared, and the descriptions are deliberately Rust-owned off-arena + (`symbol.rs:119-141` reasons the arena hazard through explicitly). Correct as + designed. +* `CLASS_REGISTRY` / `PARENT_DENSE` (`object/class_meta_registry.rs:11,47`) — + both key and value are codegen-assigned `u32` class ids. A static program + fact, identical in every realm. +* `TYPED_ARRAY_VIEW_META` (`typedarray_view.rs:156`), `ITER_RESULT_KEYS` + (`iter_result.rs:79`) — already `thread_local!`. `ITER_RESULT_KEYS`'s doc + comment is the positive control: *"each `perry/thread` worker has its own + arena, so a pointer interned from worker A's arena is a cross-arena read from + worker B."* +* `DECLARED_FIELD_NAME_HASHES` / `PROTO_DESCRIPTOR_KEY_HASHES` + (`object/descriptor_state.rs:215,222`) — FNV hashes driving a one-way + "disable this fast path" latch. Conservative over-approximation. +* `ELEMENT_SHAPE_EPOCH` / `CLASS_SHAPE_GENERATION` + (`array/element_shape.rs:167,170`) — the shape table itself is thread-local; + the global counter is a "maybe stale" signal, so a false positive costs a + re-proof. +* `GC_UNSAFE_ZONES` and the `OnceLock` policy caches across `gc/*` — env-var and + OS-config derived, identical for every thread by construction. + +## #6763 (`node:worker_threads`, 167 failing node-suite tests) — scoping + +**Almost none of #6763 is downstream of #7988.** Sampling the failure list, the +failures are overwhelmingly *missing surface*, not realm aliasing: + +* `BroadcastChannel` — missing `ERR_MISSING_ARGS` throws, missing + `ERR_INVALID_THIS` brand checks, missing `DataCloneError`, `once` listener + semantics, `MessageEvent` metadata, structured-clone of typed arrays. +* `MessagePort` / `MessageChannel` — missing `ERR_CLOSED_MESSAGE_PORT`, + close-callback ordering, transfer-list validation. +* `environment-data` / `direct-message` — `getEnvironmentData` / + `setEnvironmentData` and `postMessageToThread` are largely absent. +* `main-thread/prototype-surface` — `Worker.prototype.postMessage` is not on the + prototype at all. + +That is a **structured-clone + error-taxonomy + Web-ish-API project**, not a GC +or realm-isolation project. `node:worker_threads` is also a *different* +threading surface from `perry/thread`: it needs per-worker realms with an +explicit message port, whereas `perry/thread` is closure-shipping with +deep-copy. The realm-isolation work in this note is a **precondition** for +`node:worker_threads` behaving sanely once the surface exists — a `Worker` that +runs on an OS thread inherits every entry in the inventory table above — but it +closes approximately zero of the 167. + +## Two gates found RED on `main` while doing this (neither caused here) + +* `check_thread_locals.py` (the `lint` job, a REQUIRED context) — #7987 + (`23a8aad31`) added `BLOCK_PERSIST_FORCE_MARKS` to + `crates/perry-runtime/src/gc/trace.rs` without re-recording the file's count, + so the ratchet reads "2 recorded, 3 found". Re-recorded in this PR as its own + commit, because nothing can go green until it is. +* `check_test_registration.py` — three DARK TESTS from #7962 and #7978 exist on + disk but are in no registry, so `gc_repsel_matrix.sh` (gc-stress, + gc-moving-witnesses, gc-ptr-shape-off-witness) never runs them: + `test_gap_gc_container_value_rooting`, + `test_gap_gc_define_properties_key_rooting`, + `test_gap_gc_define_property_descriptor_rooting`. Deliberately NOT fixed here + — registering them would newly run three GC-rooting witnesses under every + matrix arm, which is their authors' call, not this PR's. Filed for whoever + owns #7949/#7978. + +`Tests` has been failing on `main` for at least five consecutive days +(2026-08-08 .. 2026-08-12), which is how both of these survived. + +## #6763 scoping (continued) + +Recommendation: keep #6763 as an umbrella and split it by subsystem +(`BroadcastChannel`, `MessagePort`/`MessageChannel` + transfer lists, +`environment-data`, `Worker` surface/events, structured clone), per the repo's +"granular per-gap issues, not umbrellas" convention. It is not a task to +half-start alongside a runtime isolation fix. diff --git a/scripts/thread_local_cold_allowlist.json b/scripts/thread_local_cold_allowlist.json index d2a86daa2f..24d26b9021 100644 --- a/scripts/thread_local_cold_allowlist.json +++ b/scripts/thread_local_cold_allowlist.json @@ -1,6 +1,6 @@ { "_comment": "Files still declaring raw `thread_local!`. Every entry is a declaration that pays `_tlv_get_addr` on Darwin; the count is a ratchet, so adding one to an already-listed file fails too. New code should use `crate::perry_thread_local!` \u2014 see crates/perry-runtime/src/tls_hot.rs. Regenerate with scripts/check_thread_locals.py --update.", - "_hot_declarations": 163, + "_hot_declarations": 174, "files": { "crates/perry-runtime/src/agent.rs": 1, "crates/perry-runtime/src/arena/block.rs": 2, @@ -49,7 +49,7 @@ "crates/perry-runtime/src/gc/shape_install.rs": 1, "crates/perry-runtime/src/gc/telemetry.rs": 2, "crates/perry-runtime/src/gc/tenuring.rs": 1, - "crates/perry-runtime/src/gc/trace.rs": 2, + "crates/perry-runtime/src/gc/trace.rs": 3, "crates/perry-runtime/src/intl/number_format.rs": 1, "crates/perry-runtime/src/iter_result.rs": 1, "crates/perry-runtime/src/json/mod.rs": 1, diff --git a/test-files/test_issue_7988_thread_realm_prototype.ts b/test-files/test_issue_7988_thread_realm_prototype.ts new file mode 100644 index 0000000000..1c8258b2f7 --- /dev/null +++ b/test-files/test_issue_7988_thread_realm_prototype.ts @@ -0,0 +1,92 @@ +// #7988: `Array.prototype` / `Object.prototype` are memoized as RAW ADDRESSES, +// and the realm they name is per-thread — `js_get_global_this` bootstraps one +// `globalThis` per thread, so every `perry/thread` agent has its own +// `Array.prototype` and its own `Object.prototype` in its own arena. +// +// The memo cells used to be process-global `AtomicUsize` statics, so they +// missed only once per PROCESS: the first thread to touch either intrinsic +// decided the address for every other agent. This probe pins BOTH directions of +// the resulting realm confusion: +// +// * LEAK — an agent reading `[1,2,3][4]` walked the MAIN thread's +// `Array.prototype`, so a main-realm prototype index showed up inside the +// agent's realm (and dereferenced a `GcHeader` in an arena the agent does +// not own, which is why the pre-fix run also SIGSEGVs intermittently). +// * BLINDNESS — the agent's own `Array.prototype[8] = v` was invisible to the +// agent's own reads, because the read resolved the main thread's prototype. +// +// LIVENESS: the main thread pollutes its OWN realm first, which is what forces +// both addresses to be resolved and memoized before any agent starts. Without +// that the agent is simply the first thread to fill the shared cell and the +// broken tree answers correctly — a vacuous probe. `main:` below prints the +// main-realm values, so a run in which the warm-up did nothing is visible +// rather than silently green. +// +// perry-only (`perry/thread` has no Node equivalent), so this is an +// `test_issue_*` behavioural test, not a byte-for-byte gap test. +import { parallelMap, spawn } from "perry/thread"; + +const MAIN_ARRAY_INDEX = 4; +const MAIN_OBJECT_INDEX = 5; +const AGENT_ARRAY_INDEX = 8; +const AGENT_OBJECT_INDEX = 9; + +// 1. The main thread pollutes ITS OWN realm. Both writes go through the +// `note_*_index_write` hooks, whose `obj == _prototype_addr()` +// test resolves and memoizes the address — so from here on the process-wide +// cells hold MAIN's addresses, which is the state an agent used to inherit. +const mainArrProto = Array.prototype as unknown as Record; +const mainObjProto = Object.prototype as Record; +mainArrProto[MAIN_ARRAY_INDEX] = "mainArr"; +mainObjProto[MAIN_OBJECT_INDEX] = "mainObj"; +const mainProbe: number[] = [1, 2, 3]; +console.log( + "main:", + String(mainProbe[MAIN_ARRAY_INDEX]), + String(mainProbe[MAIN_OBJECT_INDEX]), +); + +// Runs on an agent thread, in the agent's OWN realm: +// leakArr/leakObj — the main realm's prototype indices must NOT be visible. +// ownArr/ownObj — the agent's own prototype indices MUST be visible. +function agentProbe(tag: number): string { + const before: number[] = [1, 2, 3]; + const leakArr = String(before[MAIN_ARRAY_INDEX]); + const leakObj = String(before[MAIN_OBJECT_INDEX]); + + const arrProto = Array.prototype as unknown as Record; + const objProto = Object.prototype as Record; + arrProto[AGENT_ARRAY_INDEX] = "arr" + tag; + objProto[AGENT_OBJECT_INDEX] = "obj" + tag; + + const after: number[] = [1, 2, 3]; + const ownArr = String(after[AGENT_ARRAY_INDEX]); + const ownObj = String(after[AGENT_OBJECT_INDEX]); + return leakArr + "/" + leakObj + "/" + ownArr + "/" + ownObj; +} + +// 2. A single background OS thread — deterministic, one agent, one realm. +const EXPECTED_SPAWN = "undefined/undefined/arr1/obj1"; +const spawned = await spawn((): string => agentProbe(1)); +console.log("spawn agent:", spawned, "match:", spawned === EXPECTED_SPAWN); + +// 3. Many agents at once, all given the same input, so every worker realm must +// reach the same answer. +const EXPECTED_MAPPED = "undefined/undefined/arr2/obj2"; +const mapped = parallelMap([2, 2, 2, 2], (n: number): string => agentProbe(n)); +let allMatch = true; +for (let i = 0; i < mapped.length; i++) { + if (mapped[i] !== EXPECTED_MAPPED) allMatch = false; +} +console.log("parallelMap count:", mapped.length, "allMatch:", allMatch); + +// 4. Isolation runs the other way too: the agents' pollution is in the agents' +// realms, so the main realm must be exactly as it was. +const mainAfter: number[] = [1, 2, 3]; +console.log( + "main after:", + String(mainAfter[MAIN_ARRAY_INDEX]), + String(mainAfter[MAIN_OBJECT_INDEX]), + String(mainAfter[AGENT_ARRAY_INDEX]), + String(mainAfter[AGENT_OBJECT_INDEX]), +); diff --git a/test-parity/expected/test_issue_7988_thread_realm_prototype.txt b/test-parity/expected/test_issue_7988_thread_realm_prototype.txt new file mode 100644 index 0000000000..7c296ea419 --- /dev/null +++ b/test-parity/expected/test_issue_7988_thread_realm_prototype.txt @@ -0,0 +1,4 @@ +main: mainArr mainObj +spawn agent: undefined/undefined/arr1/obj1 match: true +parallelMap count: 4 allMatch: true +main after: mainArr mainObj undefined undefined