diff --git a/changelog.d/7434-read-pic-gc-epoch.md b/changelog.d/7434-read-pic-gc-epoch.md new file mode 100644 index 0000000000..1d86594f48 --- /dev/null +++ b/changelog.d/7434-read-pic-gc-epoch.md @@ -0,0 +1,12 @@ +Fixed the #6080(a) read-PIC ABA residual: a property-get site primed with a raw +keys-array POINTER token (class instances and other unstamped receivers) could +silently return the wrong slot after GC moved or freed the cached array and its +address was recycled by a different-shape keys array — the `@perry_ic_N` cache +globals are invisible to every GC scanner, so nothing invalidated them. The +runtime now exports `PERRY_IC_EPOCH` (bumped in `GcStats::record_collection`, +the single per-collection funnel, plus at budgeted-sweep entry where sweep +slices interleave with the mutator), the miss handler stamps it into `cache[2]` +at prime time, and the emitted hit predicate refuses a pointer-token hit whose +primed epoch is stale. Shape-ID tokens (#6804) skip the check — ids are never +reused — so stamped plain objects never re-prime after a collection; only +pointer-token sites pay one extra miss per GC cycle. diff --git a/crates/perry-codegen/src/expr/property_get/generic_dispatch.rs b/crates/perry-codegen/src/expr/property_get/generic_dispatch.rs index bd0060d721..b68a226bfd 100644 --- a/crates/perry-codegen/src/expr/property_get/generic_dispatch.rs +++ b/crates/perry-codegen/src/expr/property_get/generic_dispatch.rs @@ -177,12 +177,14 @@ pub(crate) fn lower_generic_property_get( ], ); - // Issue #51: monomorphic inline cache. Per-site 16-byte global - // holds [cached_keys_array_ptr, cached_slot_index]. The fast path - // compares obj->keys_array (offset 16) to cache[0]; on match, - // loads the field directly at obj+24+slot*8 — no function call, - // no hash, no linear scan. On miss, calls the slow helper which - // does the full lookup and primes the cache for next time. + // Issue #51: monomorphic inline cache. Per-site `[8 x i64]` global + // holds [shape_token, cached_slot_index, primed_epoch, ...unused]. + // The fast path compares the receiver's discriminated shape token + // (#6804: ShapeId stamp or raw keys_array pointer) to cache[0]; on + // match — pointer tokens additionally epoch-gated, #6080a — loads + // the field directly at obj+24+slot*8: no function call, no hash, + // no linear scan. On miss, calls the slow helper which does the + // full lookup and primes the cache for next time. let site_id = ctx.ic_site_counter; ctx.ic_site_counter += 1; let cache_name = format!("perry_ic_{}", site_id); @@ -369,6 +371,26 @@ pub(crate) fn lower_generic_property_get( let hit_token = ctx.block().and(I1, &is_object, &token_eq); let hit = ctx.block().and(I1, &hit_token, &token_nonnull); + // #6080a: pointer tokens are only trustworthy within the GC epoch they + // were primed in. The `@perry_ic_N` global is invisible to every GC + // scanner, so after a collection frees or evacuates a shape-shared keys + // array, its recycled address can be adopted by a different-shape keys + // array — `token_eq` then falsely matches and the hit path loads the + // wrong slot, silently. `js_object_get_field_ic_miss` snapshots + // `PERRY_IC_EPOCH` into `cache[2]` at prime time and every completed + // collection bumps the global, so requiring `cache[2] == PERRY_IC_EPOCH` + // forces the first read after any collection back through the miss + // handler (which re-primes against live arrays). Shape-ID tokens + // (`is_stamp`, #6804) bypass the check — ids are never reused, so they + // cannot alias across collections. Cost on the hot stamped path: two + // loads + icmp + or, folded into the existing `hit` cond_br. + let cache_epoch_ptr = ctx.block().gep(I64, &cache_ref, &[(I64, "2")]); + let cache_epoch = ctx.block().load(I64, &cache_epoch_ptr); + let live_epoch = ctx.block().load(I64, "@PERRY_IC_EPOCH"); + let epoch_eq = ctx.block().icmp_eq(I64, &cache_epoch, &live_epoch); + let epoch_ok = ctx.block().or(I1, &is_stamp, &epoch_eq); + let hit = ctx.block().and(I1, &hit, &epoch_ok); + let hit_idx = ctx.new_block("pic.hit"); let miss_idx = ctx.new_block("pic.miss"); let merge_idx = ctx.new_block("pic.merge"); diff --git a/crates/perry-codegen/src/expr/property_get/tests.rs b/crates/perry-codegen/src/expr/property_get/tests.rs index 6b9a55c7a9..b2fab0a00a 100644 --- a/crates/perry-codegen/src/expr/property_get/tests.rs +++ b/crates/perry-codegen/src/expr/property_get/tests.rs @@ -122,6 +122,35 @@ fn no_call_location_without_debug_symbols() { ); } +/// #6080a: the inline PIC hit predicate must gate raw keys-POINTER tokens on +/// the GC epoch — `cache[2] == @PERRY_IC_EPOCH` — because the `@perry_ic_N` +/// globals are invisible to every GC scanner, so a primed keys-array address +/// that GC frees/moves can be recycled under a different shape and falsely +/// pointer-match. This asserts the emitted IR still carries the guard: the +/// per-site epoch-slot load (gep index 2) and the live-epoch load from the +/// runtime-exported global. Deleting either from `lower_generic_property_get` +/// turns this red. +#[test] +fn generic_property_get_hit_path_is_epoch_gated() { + let ir = emit(false, None); + assert!( + ir.contains("@perry_ic_"), + "test premise: the generic read reaches the inline monomorphic PIC:\n{ir}" + ); + assert!( + ir.contains("load i64, ptr @PERRY_IC_EPOCH"), + "hit path must load the live read-PIC epoch (@PERRY_IC_EPOCH):\n{ir}" + ); + // The per-site primed-epoch slot: a gep to index 2 of some @perry_ic_N + // global (the site number depends on how many IC sites precede this one). + assert!( + ir.lines().any(|l| { + l.contains("getelementptr i64, ptr @perry_ic_") && l.trim_end().ends_with(", i64 2") + }), + "hit path must load the per-site primed-epoch slot (cache[2]):\n{ir}" + ); +} + #[test] fn fs_parent_promises_property_installs_before_resolution() { let mut module = Module::new("fs_parent_promises_property.ts"); diff --git a/crates/perry-codegen/src/runtime_decls/objects.rs b/crates/perry-codegen/src/runtime_decls/objects.rs index 70cd18c90d..36a0fa1546 100644 --- a/crates/perry-codegen/src/runtime_decls/objects.rs +++ b/crates/perry-codegen/src/runtime_decls/objects.rs @@ -47,6 +47,17 @@ pub fn declare_phase_b_objects(module: &mut LlModule) { // inline `header + 16 + idx*elem_size` load matches the runtime `data_ptr`). module.add_external_global("PERRY_TA_KIND_CACHE", "[64 x i64]"); module.add_external_global("PERRY_TA_VIEW_GUARD", I64); + // #6080a: process-global read-PIC epoch (perry-runtime + // `object::field_get_set::ic_miss::PERRY_IC_EPOCH`, starts at 1, bumped on + // every completed GC collection and at budgeted-sweep entry). The inline + // monomorphic property-get hit path compares its per-site `cache[2]` + // snapshot against this before trusting a raw keys-array POINTER token — + // the `@perry_ic_N` globals are invisible to every GC scanner, so a + // primed address that GC has since freed/moved would otherwise + // pointer-match a recycled keys array of a different shape and load the + // wrong slot. Shape-ID tokens (#6804, bit 62) skip the check: ids are + // never reused. + module.add_external_global("PERRY_IC_EPOCH", I64); module.declare_function("js_object_alloc", I64, &[I32, I32]); // #3149: `Object(value)` plain-call coercion. Takes & returns a NaN-boxed // JSValue (DOUBLE): nullish/primitive -> fresh {}, object passes through. diff --git a/crates/perry-runtime/src/gc/cycle.rs b/crates/perry-runtime/src/gc/cycle.rs index c7cbcf1c22..890b3633cd 100644 --- a/crates/perry-runtime/src/gc/cycle.rs +++ b/crates/perry-runtime/src/gc/cycle.rs @@ -1648,6 +1648,15 @@ impl GcCycleState { fn step_sweep(&mut self, budget: GcWorkBudget) { let phase_start = trace_phase_start(&self.trace); if self.sweep_state.is_none() { + // #6080a: invalidate pointer-token read-PIC primes BEFORE the + // first address can be freed. A budgeted cycle's sweep slices + // interleave with the mutator, so waiting for the end-of-cycle + // `record_collection` bump would leave a window where a primed + // `@perry_ic_N` cache pointer-matches a keys array whose address + // an earlier slice already recycled. Primes taken after this + // bump reference marked (live-this-cycle) arrays, which later + // slices of this same sweep never free. + crate::object::pic_epoch_bump(); let full_trace = self.minor.is_none(); // Close the finalize->sweep gap: the barrier stayed enabled across // the mutator windows since AtomicFinalize ended. Trace whatever diff --git a/crates/perry-runtime/src/gc/telemetry.rs b/crates/perry-runtime/src/gc/telemetry.rs index a2f346e5be..446415b2f8 100644 --- a/crates/perry-runtime/src/gc/telemetry.rs +++ b/crates/perry-runtime/src/gc/telemetry.rs @@ -22,7 +22,16 @@ impl GcStats { /// Single funnel for per-collection accounting: last/max pause and the /// recent-pause ring advance together with the counters, so no future /// collection path can update one without the others. + /// + /// #6080a: the read-PIC epoch bump rides the same funnel — every + /// completed collection may have freed or moved a keys array whose raw + /// address is primed in a `@perry_ic_N` cache no GC scanner can see, so + /// pointer-token primes must stop hitting from here on. (Budgeted cycles + /// bump a second time at sweep ENTRY — see `step_sweep` — because their + /// sweep slices interleave with the mutator before this funnel runs. + /// Double-bumping is harmless: it only costs one extra re-prime.) pub(super) fn record_collection(&mut self, freed_bytes: u64, elapsed_us: u64) { + crate::object::pic_epoch_bump(); self.collection_count += 1; self.total_freed_bytes = self.total_freed_bytes.saturating_add(freed_bytes); self.last_pause_us = elapsed_us; diff --git a/crates/perry-runtime/src/gc/tests/telemetry_verifier.rs b/crates/perry-runtime/src/gc/tests/telemetry_verifier.rs index 4f2f0aa79e..7fdf77ec4d 100644 --- a/crates/perry-runtime/src/gc/tests/telemetry_verifier.rs +++ b/crates/perry-runtime/src/gc/tests/telemetry_verifier.rs @@ -385,3 +385,25 @@ fn test_pause_ring_records_max_and_window() { assert!(recent_max >= GC_RECENT_PAUSE_WINDOW as u64); assert!(recent_avg > 0 && recent_avg <= recent_max); } + +/// #6080a: `record_collection` is the single per-collection funnel, and the +/// read-PIC epoch bump rides it — a completed collection may have recycled a +/// keys-array address some `@perry_ic_N` cache still holds as a raw pointer +/// token, so every collection must strand those primes. Asserting >= (not ==) +/// keeps the test robust against concurrent collections on other test threads +/// (the epoch is process-global by design). +#[test] +fn test_record_collection_bumps_read_pic_epoch() { + use std::sync::atomic::Ordering; + let before = crate::object::PERRY_IC_EPOCH.load(Ordering::Relaxed); + assert!(before >= 1, "epoch starts at 1 so zeroinitializer never hits"); + GC_STATS.with(|stats| { + stats.borrow_mut().record_collection(0, 1); + }); + let after = crate::object::PERRY_IC_EPOCH.load(Ordering::Relaxed); + assert!( + after > before, + "every completed collection must advance PERRY_IC_EPOCH \ + (before={before}, after={after})" + ); +} diff --git a/crates/perry-runtime/src/node_submodules/tests.rs b/crates/perry-runtime/src/node_submodules/tests.rs index 6a8483d9db..d37afe396a 100644 --- a/crates/perry-runtime/src/node_submodules/tests.rs +++ b/crates/perry-runtime/src/node_submodules/tests.rs @@ -468,7 +468,7 @@ fn test_default_and_named_exports_share_the_self_alias() { let property = crate::object::js_object_get_field_by_name_f64(closure as *const ObjectHeader, key); assert_eq!(property.to_bits(), default.to_bits()); - let mut cache = [0, 0]; + let mut cache = [0, 0, 0]; let property = crate::object::js_object_get_field_ic_miss(closure as *const ObjectHeader, key, &mut cache); assert_eq!(property.to_bits(), default.to_bits()); diff --git a/crates/perry-runtime/src/object/field_get_set.rs b/crates/perry-runtime/src/object/field_get_set.rs index 06551e2ea9..1f7841d355 100644 --- a/crates/perry-runtime/src/object/field_get_set.rs +++ b/crates/perry-runtime/src/object/field_get_set.rs @@ -199,8 +199,9 @@ pub(crate) use ic_miss::{ pub use ic_miss::{ js_object_get_field_by_name_f64, js_object_get_field_by_property_id_f64, js_object_get_field_ic_miss, js_object_set_field_by_property_id, js_private_brand_check, - js_private_guard, + js_private_guard, PERRY_IC_EPOCH, }; +pub(crate) use ic_miss::pic_epoch_bump; #[cfg(test)] mod buffer_ic_miss_tests { @@ -231,7 +232,7 @@ mod buffer_ic_miss_tests { unsafe { for len in [16usize, 24, 32] { let buf = secret_buffer(len); - let mut cache = [0i64; 2]; + let mut cache = [0i64; 3]; let ty = js_object_get_field_ic_miss( buf as *const ObjectHeader, diff --git a/crates/perry-runtime/src/object/field_get_set/ic_miss.rs b/crates/perry-runtime/src/object/field_get_set/ic_miss.rs index 30b2675d60..ce33632b4d 100644 --- a/crates/perry-runtime/src/object/field_get_set/ic_miss.rs +++ b/crates/perry-runtime/src/object/field_get_set/ic_miss.rs @@ -188,8 +188,16 @@ pub(crate) fn is_timer_handle_method_key(key: &[u8]) -> bool { /// #6759 C3c: is `keys` safe to prime into a per-site PIC cache whose hit /// path does an UNVALIDATED compare-and-load? True only for /// `GC_FLAG_SHAPE_SHARED` arrays — those are shape-cache-resident -/// (process-rooted, so the address can never be freed and recycled under a -/// different shape). Conservative `false` for anything else. +/// (process-rooted, so they stay LIVE for as long as a cache references +/// them). Conservative `false` for anything else. +/// +/// Rooted is not address-STABLE, though: the copying minor moves +/// shape-shared arrays like anything else (`move_young` merely preserves +/// the flag), rewriting every rooted reference — but not the `@perry_ic_N` +/// globals, which no GC scanner knows about. The vacated from-space address +/// is then recycled, and a different keys array landing there makes a +/// primed site falsely HIT with the old slot mapping (#6080a). That residual +/// is closed by [`PERRY_IC_EPOCH`] below, not by this predicate. pub(crate) unsafe fn keys_cacheable_for_pic(keys: *const crate::array::ArrayHeader) -> bool { let Some(gc) = crate::value::addr_class::try_read_gc_header(keys as usize) else { return false; @@ -197,6 +205,32 @@ pub(crate) unsafe fn keys_cacheable_for_pic(keys: *const crate::array::ArrayHead gc.obj_type == crate::gc::GC_TYPE_ARRAY && gc.gc_flags & crate::gc::GC_FLAG_SHAPE_SHARED != 0 } +/// #6080(a): process-global read-PIC epoch, exported to the emitted IR as +/// `@PERRY_IC_EPOCH` (same pattern as `PERRY_TA_VIEW_GUARD`). A keys-POINTER +/// token primed into a `@perry_ic_N` cache is only trustworthy for as long +/// as no address has been freed or moved since priming: the cache global is +/// invisible to every GC scanner, so a recycled keys-array address would +/// pointer-match a different shape and the inline hit path would load the +/// wrong slot — silently. +/// +/// The miss handler snapshots this epoch into `cache[2]` at prime time; the +/// emitted hit predicate requires `cache[2] == PERRY_IC_EPOCH` before +/// trusting a pointer token (shape-ID tokens skip the check — ids are never +/// reused, so they cannot alias). Every completed collection bumps the epoch +/// (`GcStats::record_collection`, the single per-collection funnel), and +/// budgeted cycles additionally bump at sweep ENTRY, because their sweep +/// slices interleave with the mutator — an address freed by an early slice +/// must not be trusted while the cycle is still running. +/// +/// Starts at 1 so a `zeroinitializer` cache (epoch 0) can never match. +#[no_mangle] +pub static PERRY_IC_EPOCH: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1); + +/// Invalidate every pointer-token read-PIC prime (see [`PERRY_IC_EPOCH`]). +pub(crate) fn pic_epoch_bump() { + PERRY_IC_EPOCH.fetch_add(1, std::sync::atomic::Ordering::Relaxed); +} + /// Monomorphic inline cache miss handler (issue #51). /// /// Called when the codegen-emitted shape check (`obj->keys_array == cache[0]`) @@ -204,7 +238,10 @@ pub(crate) unsafe fn keys_cacheable_for_pic(keys: *const crate::array::ArrayHead /// then populates the per-site cache so subsequent calls with the same shape /// hit the inline fast path (no function call, direct field load). /// -/// `cache` layout: `[keys_array_ptr: i64, field_slot_index: i64]` +/// `cache` layout: `[shape_token: i64, field_slot_index: i64, primed_epoch: i64]` +/// (`shape_token` is a shape-ID token or a raw keys-array pointer — see #6804; +/// `primed_epoch` is the [`PERRY_IC_EPOCH`] snapshot taken at prime time, +/// #6080a). The emitted global is `[8 x i64]`; slots 3..8 are unused here. /// /// Only caches when: /// - obj is a valid ObjectHeader (not null, not handle, not string/array/etc.) @@ -217,7 +254,7 @@ pub(crate) unsafe fn keys_cacheable_for_pic(keys: *const crate::array::ArrayHead pub extern "C" fn js_object_get_field_ic_miss( obj: *const ObjectHeader, key: *const crate::StringHeader, - cache: *mut [i64; 2], + cache: *mut [i64; 3], ) -> f64 { // SSO receiver — never cacheable. Route through the SSO-aware // `js_object_get_field_by_name` which handles `.length` inline @@ -463,13 +500,21 @@ pub extern "C" fn js_object_get_field_ic_miss( // is unvalidated and a recycled owned-array address // would read the wrong slot. let stamp = (*obj).parent_class_id; + // #6080a: stamp the current GC epoch alongside either + // token kind. The emitted hit predicate only consults it + // for pointer tokens, but priming it unconditionally + // keeps `cache[2]` coherent when a site re-primes from + // one token kind to the other. + let epoch = PERRY_IC_EPOCH.load(std::sync::atomic::Ordering::Relaxed) as i64; if (*obj).class_id == 0 && crate::object::shapes::is_shape_id(stamp) { (*cache)[0] = (stamp as u64 | crate::object::shapes::PIC_ID_TOKEN_BIT) as i64; (*cache)[1] = i as i64; + (*cache)[2] = epoch; } else if keys_cacheable_for_pic(keys) { (*cache)[0] = keys as i64; (*cache)[1] = i as i64; + (*cache)[2] = epoch; } let field_ptr = (obj as *const u8) .add(std::mem::size_of::() + i * 8) @@ -508,7 +553,7 @@ pub extern "C" fn js_object_get_field_ic( obj_bits: i64, key: *const crate::StringHeader, site_id: u64, - cache: *mut [i64; 2], + cache: *mut [i64; 3], ) -> f64 { // POINTER_MASK: lower 48 bits — strips the NaN-box tag to a raw heap pointer. const POINTER_MASK: u64 = 0x0000_FFFF_FFFF_FFFF; @@ -800,4 +845,59 @@ mod c3c_pic_tests { ); } } + + /// #6080a: a pointer-token prime snapshots the live PIC epoch into + /// `cache[2]`, and a subsequent epoch bump strands that snapshot — the + /// exact inputs of the emitted `cache[2] == @PERRY_IC_EPOCH` guard, so + /// this proves the guard CAN fail (a primed entry goes stale), not just + /// that priming writes something. + #[test] + fn pointer_token_prime_stamps_epoch_and_goes_stale_on_bump() { + let _lock = crate::gc::global_side_table_test_lock(); + unsafe { + use std::sync::atomic::Ordering; + // A CLASS instance (class_id != 0) is the population that still + // primes raw keys pointers — plain objects take the #6804 + // shape-ID token, which the epoch guard deliberately skips. + let obj = crate::object::js_object_alloc(0x6080, 8); + let key = crate::string::js_string_from_bytes(b"pic6080_x".as_ptr(), 9); + crate::object::js_object_set_field_by_name(obj, key, 7.0); + let keys = (*obj).keys_array; + assert!(!keys.is_null(), "test premise: field append built keys"); + let gc = (keys as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *mut crate::gc::GcHeader; + (*gc).gc_flags |= crate::gc::GC_FLAG_SHAPE_SHARED; + + let mut cache = [0i64; 3]; + let v = super::js_object_get_field_ic_miss(obj, key, &mut cache); + assert_eq!(v, 7.0); + assert_eq!( + cache[0], keys as i64, + "class instance must prime the raw keys pointer token" + ); + let primed_epoch = cache[2]; + assert_eq!( + primed_epoch, + super::PERRY_IC_EPOCH.load(Ordering::Relaxed) as i64, + "prime must snapshot the LIVE epoch" + ); + assert!(primed_epoch >= 1, "epoch starts at 1, never 0"); + + super::pic_epoch_bump(); + assert_ne!( + cache[2], + super::PERRY_IC_EPOCH.load(Ordering::Relaxed) as i64, + "a bump must strand every pointer-token prime (the emitted \ + hit predicate then misses and re-primes)" + ); + + // Re-priming heals: the miss handler stamps the NEW epoch. + let v = super::js_object_get_field_ic_miss(obj, key, &mut cache); + assert_eq!(v, 7.0); + assert_eq!( + cache[2], + super::PERRY_IC_EPOCH.load(Ordering::Relaxed) as i64, + "re-prime must heal the epoch snapshot" + ); + } + } } diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index aed7c4038a..ff4f4a3e3b 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -46,6 +46,7 @@ mod descriptors; mod disposable_proto_thunks; pub(crate) mod exotic_expando; mod field_get_set; +pub(crate) use field_get_set::pic_epoch_bump; pub(crate) use field_get_set::scan_accessor_receiver_override_root_mut; mod field_set_by_name; mod global_fetch; diff --git a/crates/perry-runtime/src/value/dynamic_object.rs b/crates/perry-runtime/src/value/dynamic_object.rs index 7ab6fd2887..a8f0ba0b9c 100644 --- a/crates/perry-runtime/src/value/dynamic_object.rs +++ b/crates/perry-runtime/src/value/dynamic_object.rs @@ -745,7 +745,7 @@ mod length_handle_band_tests { }; let console_ptr = crate::value::js_nanbox_get_pointer(console_ctor) as usize; let length_key = crate::string::js_string_from_bytes(b"length".as_ptr(), 6); - let mut cache = [0_i64; 2]; + let mut cache = [0_i64; 3]; assert_eq!(js_value_length_f64(console_ctor), 1.0); assert_eq!(