diff --git a/changelog.d/7942-weak-read-barrier.md b/changelog.d/7942-weak-read-barrier.md new file mode 100644 index 0000000000..de4b1ad7f7 --- /dev/null +++ b/changelog.d/7942-weak-read-barrier.md @@ -0,0 +1,74 @@ +Closed a **use-after-sweep** in the budgeted (incremental) collector: a weak-to-strong +transition through a *read* that no barrier observed. + +A budgeted cycle performs its one-time `FinalRootRemark` and then keeps opening mutator +windows while `AtomicFinalize` is still sliced — the full path's `RememberedSetRebuild`, +and (since #7892) the weak-holder loop itself. The soundness argument recorded for those +windows is "the incremental mark barrier shades every store, and mid-cycle allocations +are born black". Both mechanisms only observe values the mutator **writes** or +**creates**. + +`WeakRef.deref()` and `WeakMap.get()` do neither. They take a **white** object — white +*by construction*, because weak edges are deliberately excluded from the strong trace — +and hand it to compiled code as a strong local. The remark has already run, so no later +root scan can discover the new reference: the next weak slice sees the target unmarked, +tombstones it, and the sweep reclaims memory the mutator is still holding. It surfaces +cycles later as `TypeError: value is not a function`. + +The window is reachable on default settings. `gc_incremental_enabled()` has been ON since +#6180, so ordinary allocation pressure runs budgeted cycles, and every `WeakMap`/`WeakSet` +**entry** is its own registered holder — any collection with more entries than the step +budget (2048 on a host safepoint, 256 plus debt scaling on an allocator assist) parks +mid-registry with the rest of the registry pending. + +**Fix — a weak-read barrier** (`crates/perry-runtime/src/weakref/read_barrier.rs`). Every +weak read shades the value words it hands out, using the same shade-and-seed the store +barrier uses (`gc::gc_weak_read_shade` → `incremental_mark_barrier_value`): the target in +`WeakRef.deref()`, and the matched key plus returned value in `WeakMap.get()` / +`WeakMap.has()` (`WeakSet.has()` delegates). Three properties make that sufficient rather +than a patch: + +* the pending weak decision is a mark-set predicate (`weak_target_should_clear`), so a + shaded target is kept — which is also what the spec's `AddToKeptObjects` requires of + `WeakRef.deref`; +* the shade pushes a mark seed, and both pre-sweep drains already exist (the minor arm of + `RememberedSetRebuild`, the full arm of `DisableBarrier`), so the target's children are + traced — a marked-but-untraced object would have been the same bug one level down; +* it closes the full path's **pre-existing** window as well, where the sliced + remembered-set rebuild sits between the remark and the weak decisions. Un-slicing the + weak loop would not have. + +Outside a cycle the barrier is inert, and that is asserted in its own test: a stray mark +laid down with no cycle in flight reads as "already live" to the next cycle's trace. + +**Two comments were wrong and are corrected.** The `FinalRootRemark` enum doc still said +"from this subphase to the Sweep transition the minor path runs ATOMICALLY (no mutator +windows)" — untrue since #7892 put `WeakProcessing` in the sliced set; the sibling comment +470 lines away was updated and this one was not. The sliced-set comment argued weak +slicing was safe because "a target the mutator can still name was marked at remark or was +born black" — neither covers a target the mutator names *for the first time* through a +weak read. + +**Coverage** (`crates/perry-runtime/src/gc/tests/weak_read_barrier.rs`) is a deterministic +state-machine test, not a "does not throw" test. Four cases: the full budgeted ordering, +the budgeted-minor ordering (no remembered-set rebuild between remark and weak +decisions), the `WeakMap` shape, and the barrier's OFF state. Each run asserts its own +premises: + +* **the window opened** — 8 holders against a one-unit budget, `weak_processing` consumed + exactly one holder and is still the parked subphase; +* **the remark actually ran** — a *remark witness*: a white, unreferenced object installed + into a shadow slot only at the `AtomicFinalize` boundary, i.e. after `RootScan` and + `MarkPropagation`. `js_shadow_slot_set` performs no barrier, so nothing but + `FinalRootRemark` can have marked it by the time weak processing parks; +* **the read shaded something white** — a shade counter, so a run where the target + happened to be marked already cannot pass as evidence; +* **the target survived** — its `OVERFLOW_FIELDS` side-table entry is intact (a swept + owner has it cleared by the dead-payload sweep arm, the same shape the production bug + presented as) *and* `deref()` still returns the same bits. + +Sabotage-verified in two arms with the fix committed first. With the shade removed +entirely the subject-live assertions fire, proving the counter is wired to the mark and +not to the call; with the barrier counting but not marking, both survival assertions fire +with the use-after-sweep message — which is the arm that proves the tests catch the bug +rather than the instrument. diff --git a/crates/perry-runtime/src/gc/barrier/mod.rs b/crates/perry-runtime/src/gc/barrier/mod.rs index a31291f491..f902ffb56e 100644 --- a/crates/perry-runtime/src/gc/barrier/mod.rs +++ b/crates/perry-runtime/src/gc/barrier/mod.rs @@ -981,6 +981,23 @@ pub(super) fn incremental_mark_barrier_value(value_bits: u64) -> bool { incremental_mark_barrier_value_with_valid_ptrs(value_bits, valid_ptrs) } +/// Weak-to-strong READ barrier (#7900): shade a value word that a weak slot is +/// about to hand the mutator as a strong reference. +/// +/// This is the same shade-and-seed the store barrier performs, exposed to +/// `crate::weakref` because the *read* side of a weak edge is the one +/// white-to-strong transition neither the store barrier nor allocate-black +/// birth accounting can observe — and budgeted cycles keep opening mutator +/// windows AFTER `FinalRootRemark`, i.e. after the last root observation that +/// could otherwise have discovered the new reference. See +/// `crate::weakref::read_barrier` for the full argument. +/// +/// Returns `true` when a previously-white object was marked. +#[inline] +pub(crate) fn gc_weak_read_shade(value_bits: u64) -> bool { + incremental_mark_barrier_value(value_bits) +} + #[allow(dead_code)] pub(super) fn drain_incremental_mark_barrier_seeds(valid_ptrs: &ValidPointerSet) { loop { diff --git a/crates/perry-runtime/src/gc/cycle.rs b/crates/perry-runtime/src/gc/cycle.rs index 38fba3906c..db00e78474 100644 --- a/crates/perry-runtime/src/gc/cycle.rs +++ b/crates/perry-runtime/src/gc/cycle.rs @@ -857,11 +857,11 @@ enum AtomicFinalizeSubphase { /// one-shot scan and would be swept live. Re-scan all roots with the /// marks nearly final, then drain the resulting seeds, so WeakProcessing /// and Sweep read a complete mark set. Bounded by root-set size (shadow - /// stack + globals + registered scanners), not heap size. From this - /// subphase to the Sweep transition the minor path runs ATOMICALLY (no - /// mutator windows); the full path's sliced RememberedSetRebuild is the - /// one post-remark window, and it is store-covered by the still-active - /// mark barrier. + /// stack + globals + registered scanners), not heap size. This is the LAST + /// root observation of the cycle: everything after it (the full path's + /// sliced RememberedSetRebuild, and WeakProcessing on both paths) still + /// opens mutator windows, so every white-to-strong transition there must be + /// barrier-covered — stores, black births, and weak reads (#7900). FinalRootRemark, RememberedSetRebuild, DisableBarrier, @@ -1325,10 +1325,10 @@ impl GcCycleState { .expect("atomic finalize state exists") .subphase; // SLICED subphases honor the caller's budget and may return to the - // mutator. WeakProcessing is safe to slice while the barrier and - // allocate-black births stay active: a target the mutator can - // still name was marked at remark or was born black, and holders - // born after the registry snapshot wait for the next cycle. + // mutator. Post-remark windows are sound only because EVERY way the + // mutator can acquire a heap reference is shaded: stores by the + // incremental mark barrier, births by allocate-black, and weak + // READS by `weakref::read_barrier` (#7900 — that arm was missing). let sliced = matches!( subphase, AtomicFinalizeSubphase::BarrierSeedDrain diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 5895154be6..c542b10571 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -46,3 +46,4 @@ mod teardown; mod telemetry_verifier; mod temp_roots; mod triggers; +mod weak_read_barrier; diff --git a/crates/perry-runtime/src/gc/tests/weak_read_barrier.rs b/crates/perry-runtime/src/gc/tests/weak_read_barrier.rs new file mode 100644 index 0000000000..b82fb1eb2c --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/weak_read_barrier.rs @@ -0,0 +1,396 @@ +//! #7900: the post-remark weak-to-strong READ race. +//! +//! A budgeted cycle performs its one-time `FinalRootRemark` and then keeps +//! opening mutator windows while weak processing (and, on the full path, the +//! sliced remembered-set rebuild) is still incomplete. `WeakRef.deref()` and +//! `WeakMap.get()` turn an unmarked weak target into a STRONG compiled-code +//! local through a **read** — a transition neither the incremental store +//! barrier nor allocate-black birth accounting observes, and one that no later +//! root scan can discover because the remark already ran. A subsequent weak +//! slice then tombstones the target and the sweep reclaims it while generated +//! code still holds the pointer. +//! +//! The fix is a weak-READ barrier (`crate::weakref::read_barrier`): every weak +//! read shades the value words it hands the mutator while an incremental mark +//! cycle is in flight, so the pending weak decision sees a marked target and +//! the pre-sweep drain traces its children. +//! +//! These tests are state-machine tests, not "does not throw" tests: each one +//! asserts the window it depends on actually opened (weak processing parked +//! mid-registry), that the read actually shaded something (a live subject — +//! see CLAUDE.md's "a gate must assert its subject was live"), and that the +//! target survives BOTH as a heap object and as the WeakRef's own answer. + +use super::super::*; +use super::support::*; + +fn trace_snapshot(kind: GcTriggerKind) -> GcTriggerSnapshot { + GcTriggerSnapshot { + kind, + steps_before: Some(GcStepSnapshot::current()), + } +} + +fn run_cycle_in_single_unit_steps(state: &mut GcCycleState) { + for _ in 0..200_000 { + if state.phase() == GcCyclePhase::Complete { + return; + } + state.step(GcWorkBudget::bounded(1)); + } + panic!("GC cycle did not complete within step limit"); +} + +fn run_cycle_until_phase(state: &mut GcCycleState, target: GcCyclePhase) { + for _ in 0..200_000 { + if state.phase() == target { + return; + } + state.step(GcWorkBudget::bounded(1)); + } + panic!("GC cycle did not reach {target:?} within step limit"); +} + +/// A budgeted minor-fallback cycle: `low_pause_non_moving`, exactly as +/// `gc_start_budgeted_minor_fallback_cycle_with_snapshot` builds it (evacuation +/// refused, so `atomic_finalize_minor_prelude`'s non-moving assert holds). +fn start_budgeted_minor_fallback_state(trigger: GcTriggerSnapshot) -> GcCycleState { + let prev_in_alloc = GC_FLAGS.with(|f| { + let prev = f.get(); + f.set(prev | GC_FLAG_IN_ALLOC); + prev & GC_FLAG_IN_ALLOC + }); + let trace = GcCycleTrace::new(GcCollectionKind::Minor, trigger); + let start = std::time::Instant::now(); + crate::arena::old_pages_begin_gc_cycle(); + clear_mark_seeds(); + GcCycleState::new_minor_fallback( + trigger, + trace, + start, + GcProgressKind::NormalIncremental, + prev_in_alloc, + gc_last_pause_us(), + crate::process::get_rss_bytes(), + /* evacuation_policy_allowed = */ false, + /* force_evacuation = */ false, + "low_pause_non_moving", + OldPageDefragSelection::default(), + crate::arena::OldArenaSourceBlockSelection::default(), + ) +} + +/// Push the freshly-allocated holders/targets out of the block-persistence +/// window (the 5 most recent general blocks are conservatively force-marked), +/// so a weak-only target really is white when finalization decides. +fn age_out_of_block_persist_window() { + let aged_from = crate::arena::general_block_count(); + while crate::arena::general_block_count().saturating_sub(aged_from) < 7 { + for _ in 0..64 { + let _ = crate::arena::arena_alloc_gc(4096, 8, GC_TYPE_STRING); + } + } +} + +/// Allocate `count` WeakRefs over weak-only targets, rooting each WeakRef in +/// shadow slot `i`. Returns nothing: the refs are read back out of the shadow +/// stack so the test never holds a raw address across a collection. +fn seed_weak_refs(count: u32) { + for slot in 0..count { + let target = crate::object::js_object_alloc(0, 0); + let weak_ref = crate::weakref::js_weakref_new(f64::from_bits(ptr_bits(target as usize))); + js_shadow_slot_set(slot, ptr_bits(weak_ref as usize)); + } +} + +/// A white object that no root reaches. Installed into a shadow slot only once +/// the one-shot root scan is over, so the ONLY thing that can mark it is +/// `FinalRootRemark` — which makes it a witness that the remark really ran, +/// rather than an assumption that `progress_kind.is_budgeted()` implies it. +fn alloc_remark_witness() -> usize { + let (witness, _) = unsafe { alloc_nursery_test_object(1) }; + unsafe { + let header = header_from_user_ptr(witness as *const u8); + (*header).gc_flags &= !GC_FLAG_MARKED; + } + witness as usize +} + +/// Drive `state` to the first mutator window that is parked INSIDE weak +/// processing, i.e. after `FinalRootRemark` and with holders still pending. +/// +/// `witness` is installed into `witness_slot` at the AtomicFinalize boundary — +/// after RootScan and MarkPropagation are complete — and must be MARKED by the +/// time weak processing parks. `js_shadow_slot_set` performs no barrier, so a +/// marked witness proves a root scan ran after the store: the remark. +fn park_inside_weak_processing( + state: &mut GcCycleState, + holders: u32, + witness: usize, + witness_slot: u32, +) { + run_cycle_until_phase(state, GcCyclePhase::AtomicFinalize); + js_shadow_slot_set(witness_slot, ptr_bits(witness)); + let mut steps = 0usize; + while state.atomic_finalize_subphase_for_tests() != Some("weak_processing") { + state.step(GcWorkBudget::bounded(1)); + steps += 1; + assert!(steps < 200_000, "weak processing was never reached"); + } + let witness_flags = unsafe { (*header_from_user_ptr(witness as *const u8)).gc_flags }; + assert_ne!( + witness_flags & GC_FLAG_MARKED, + 0, + "SUBJECT-LIVE CHECK: a root installed after the one-shot root scan is \ + still white at weak processing, so FinalRootRemark did NOT run and this \ + test is not exercising the post-remark window" + ); + assert_eq!( + crate::weakref::test_support::full_weak_processing_work_units(), + 1, + "the step that enters weak processing must consume exactly one holder" + ); + assert_eq!( + state.atomic_finalize_subphase_for_tests(), + Some("weak_processing"), + "the cycle must be PARKED mid-registry: {holders} holders, budget 1" + ); + assert!( + incremental_mark_barrier_active(), + "the mark barrier must still be armed in a post-remark mutator window" + ); +} + +/// The mutator's weak read. Returns `(shadow slot, target bits, target addr)` +/// for the first holder whose target is still pending (an already-processed +/// holder answers `undefined`, since every target here is weak-only). +fn mutator_weak_read(holders: u32) -> (u32, u64, usize) { + for slot in 0..holders { + let weak_ref = f64::from_bits(js_shadow_slot_get(slot)); + let bits = crate::weakref::js_weakref_deref(weak_ref).to_bits(); + if bits != crate::value::TAG_UNDEFINED { + let addr = (bits & POINTER_MASK) as usize; + return (slot, bits, addr); + } + } + panic!("no holder was still pending in the window — the race was not set up"); +} + +/// #7900, full budgeted cycle (BarrierSeedDrain → FinalRootRemark → +/// RememberedSetRebuild → WeakProcessing): a target handed to the mutator by +/// `WeakRef.deref()` in a post-remark window must not be tombstoned or swept +/// by the slices that follow. +#[test] +fn weak_read_after_final_remark_survives_full_budgeted_cycle() { + const HOLDERS: u32 = 8; + let _guard = CopyingNurseryTestGuard::new(HOLDERS + 1); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + crate::weakref::test_support::clear_weak_holders(); + crate::weakref::test_support::reset_weak_read_barrier_shades(); + + seed_weak_refs(HOLDERS); + let witness = alloc_remark_witness(); + age_out_of_block_persist_window(); + + let mut state = GcCycleState::new_full(trace_snapshot(GcTriggerKind::ArenaBytes)); + state.set_progress_kind(GcProgressKind::NormalIncremental); + park_inside_weak_processing(&mut state, HOLDERS, witness, HOLDERS); + + // ---- mutator window ---- + let (slot, bits, addr) = mutator_weak_read(HOLDERS); + // Observability: a swept owner has its OVERFLOW_FIELDS entry cleared by the + // dead-payload sweep arm, exactly how the production shape of this bug + // (lost side-table fields) presented. + crate::object::test_seed_overflow_fields_root(addr, 42f64.to_bits()); + assert!( + crate::weakref::test_support::weak_read_barrier_shades() >= 1, + "SUBJECT-LIVE CHECK: the weak read must have shaded an unmarked target. \ + Zero shades means the target was already marked and this run proves nothing" + ); + // ---- collector resumes ---- + + run_cycle_in_single_unit_steps(&mut state); + let _ = state.take_outcome().expect("cycle should complete"); + + assert!( + crate::object::debug_overflow_entry_len(addr).is_some(), + "#7900: a target handed to the mutator by WeakRef.deref() after the final \ + root remark was SWEPT while the mutator still held it" + ); + let weak_ref = f64::from_bits(js_shadow_slot_get(slot)); + assert_eq!( + crate::weakref::js_weakref_deref(weak_ref).to_bits(), + bits, + "#7900: a target the mutator strongly acquired through deref() was \ + tombstoned by a later weak slice" + ); + crate::object::test_clear_overflow_fields_root(); +} + +/// #7900, budgeted MINOR ordering (BarrierSeedDrain → FinalRootRemark → +/// WeakProcessing → MinorPrelude → RememberedSetRebuild). The full path's +/// sliced remembered-set rebuild sits between the remark and the weak +/// decisions; the minor path has no such phase, so it pins the other ordering. +#[test] +fn weak_read_after_final_remark_survives_budgeted_minor_cycle() { + const HOLDERS: u32 = 8; + let _guard = CopyingNurseryTestGuard::new(HOLDERS + 1); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + crate::weakref::test_support::clear_weak_holders(); + crate::weakref::test_support::reset_weak_read_barrier_shades(); + + seed_weak_refs(HOLDERS); + let witness = alloc_remark_witness(); + age_out_of_block_persist_window(); + + let mut state = start_budgeted_minor_fallback_state(trace_snapshot(GcTriggerKind::ArenaBytes)); + park_inside_weak_processing(&mut state, HOLDERS, witness, HOLDERS); + + let (slot, bits, addr) = mutator_weak_read(HOLDERS); + crate::object::test_seed_overflow_fields_root(addr, 42f64.to_bits()); + assert!( + crate::weakref::test_support::weak_read_barrier_shades() >= 1, + "SUBJECT-LIVE CHECK: the weak read must have shaded an unmarked target" + ); + + run_cycle_in_single_unit_steps(&mut state); + let _ = state.take_outcome().expect("cycle should complete"); + + assert!( + crate::object::debug_overflow_entry_len(addr).is_some(), + "#7900 (minor ordering): a deref()'d target was swept while the mutator held it" + ); + let weak_ref = f64::from_bits(js_shadow_slot_get(slot)); + assert_eq!( + crate::weakref::js_weakref_deref(weak_ref).to_bits(), + bits, + "#7900 (minor ordering): a deref()'d target was tombstoned by a later weak slice" + ); + crate::object::test_clear_overflow_fields_root(); +} + +/// The same race reaching a `WeakMap`: the key the mutator presents to +/// `get()` can only be white if it was itself acquired weakly in this window, +/// which is exactly the shape here — `deref()` recovers a key, `get()` then +/// reads its entry. Both reads must shade, or the pending weak slice tombstones +/// the entry and the sweep takes the key the mutator is holding. +/// +/// 8 entries + 8 key WeakRefs = 16 holders and a budget of 1, so at most one +/// holder is decided when the window opens and at least seven key/entry pairs +/// are intact. The test scans for one rather than assuming a registry iteration +/// order, so it can never silently skip itself. +#[test] +fn weak_map_read_after_final_remark_survives_budgeted_cycle() { + const ENTRIES: u32 = 8; + const MAP_SLOT: u32 = ENTRIES; + const WITNESS_SLOT: u32 = ENTRIES + 1; + let _guard = CopyingNurseryTestGuard::new(ENTRIES + 2); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + crate::weakref::test_support::clear_weak_holders(); + crate::weakref::test_support::reset_weak_read_barrier_shades(); + + let map = crate::weakref::js_weakmap_new(); + js_shadow_slot_set(MAP_SLOT, ptr_bits(map as usize)); + let map_bits = ptr_bits(map as usize); + for slot in 0..ENTRIES { + let key = crate::object::js_object_alloc(0, 0); + let value = crate::object::js_object_alloc(0, 0); + crate::weakref::js_weakmap_set( + f64::from_bits(map_bits), + f64::from_bits(ptr_bits(key as usize)), + f64::from_bits(ptr_bits(value as usize)), + ); + // One WeakRef per key, so the mutator can recover a key without ever + // holding it strongly (a strongly-held key would be marked at the + // remark and its entry could not be tombstoned). + let key_ref = crate::weakref::js_weakref_new(f64::from_bits(ptr_bits(key as usize))); + js_shadow_slot_set(slot, ptr_bits(key_ref as usize)); + } + let witness = alloc_remark_witness(); + age_out_of_block_persist_window(); + + let mut state = GcCycleState::new_full(trace_snapshot(GcTriggerKind::ArenaBytes)); + state.set_progress_kind(GcProgressKind::NormalIncremental); + park_inside_weak_processing(&mut state, ENTRIES, witness, WITNESS_SLOT); + + // ---- mutator window: weak read #1 recovers a key, #2 reads its entry ---- + let map_value = f64::from_bits(js_shadow_slot_get(MAP_SLOT)); + let mut acquired = None; + for slot in 0..ENTRIES { + let key_bits = + crate::weakref::js_weakref_deref(f64::from_bits(js_shadow_slot_get(slot))).to_bits(); + if key_bits == crate::value::TAG_UNDEFINED { + continue; + } + let value_bits = + crate::weakref::js_weakmap_get(map_value, f64::from_bits(key_bits)).to_bits(); + if value_bits != crate::value::TAG_UNDEFINED { + acquired = Some((key_bits, value_bits)); + break; + } + } + let (key_bits, value_bits) = acquired.expect( + "with 16 holders and a one-unit budget at least seven key/entry pairs \ + must still be pending — the race was not set up", + ); + let key_addr = (key_bits & POINTER_MASK) as usize; + crate::object::test_seed_overflow_fields_root(key_addr, 42f64.to_bits()); + assert!( + crate::weakref::test_support::weak_read_barrier_shades() >= 1, + "SUBJECT-LIVE CHECK: the weak reads must have shaded at least one white value" + ); + + run_cycle_in_single_unit_steps(&mut state); + let _ = state.take_outcome().expect("cycle should complete"); + + assert!( + crate::object::debug_overflow_entry_len(key_addr).is_some(), + "#7900: a WeakMap key the mutator strongly acquired in a post-remark \ + window was swept while it still held it" + ); + assert_eq!( + crate::weakref::js_weakmap_get( + f64::from_bits(js_shadow_slot_get(MAP_SLOT)), + f64::from_bits(key_bits), + ) + .to_bits(), + value_bits, + "#7900: a WeakMap entry the mutator read in the window was tombstoned" + ); + crate::object::test_clear_overflow_fields_root(); +} + +/// Contract test for the acceptance criterion "no mutator window exists after +/// the last root observation unless weak reads participate in marking". +/// +/// The barrier's kill state is the absence of a cycle: with no incremental mark +/// in progress a weak read must shade NOTHING (it would otherwise leave stray +/// marks that the next cycle reads as live). This pins both directions. +#[test] +fn weak_read_barrier_is_inert_outside_a_cycle() { + let _guard = CopyingNurseryTestGuard::new(1); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + crate::weakref::test_support::clear_weak_holders(); + crate::weakref::test_support::reset_weak_read_barrier_shades(); + + let target = crate::object::js_object_alloc(0, 0); + let weak_ref = crate::weakref::js_weakref_new(f64::from_bits(ptr_bits(target as usize))); + js_shadow_slot_set(0, ptr_bits(weak_ref as usize)); + js_shadow_slot_set(1, ptr_bits(target as usize)); + + assert!(!incremental_mark_barrier_active()); + let bits = crate::weakref::js_weakref_deref(f64::from_bits(js_shadow_slot_get(0))).to_bits(); + assert_ne!(bits, crate::value::TAG_UNDEFINED); + assert_eq!( + crate::weakref::test_support::weak_read_barrier_shades(), + 0, + "a weak read outside a mark cycle must not mark anything: a stray mark \ + reads as live to the next cycle" + ); + let header = unsafe { header_from_user_ptr(((bits & POINTER_MASK) as usize) as *const u8) }; + assert_eq!( + unsafe { (*header).gc_flags } & GC_FLAG_MARKED, + 0, + "weak read outside a cycle must leave the target unmarked" + ); +} diff --git a/crates/perry-runtime/src/weakref.rs b/crates/perry-runtime/src/weakref.rs index bc2f5bed8d..5562a700b3 100644 --- a/crates/perry-runtime/src/weakref.rs +++ b/crates/perry-runtime/src/weakref.rs @@ -21,6 +21,8 @@ use crate::value::{ }; use std::cell::RefCell; +/// #7900: weak-to-strong READ barrier. See the module for the full argument. +mod read_barrier; pub(crate) mod sliced; #[cfg(test)] pub(crate) mod test_support; @@ -426,7 +428,9 @@ pub extern "C" fn js_weakref_deref(weakref: f64) -> f64 { if val.is_undefined() { f64::from_bits(TAG_UNDEFINED) } else { - f64::from_bits(val.bits()) + // #7900: a white target becomes a STRONG mutator local here, through a + // read the store barrier cannot see. Shade it before handing it over. + read_barrier::weak_read_barrier_f64(val.bits()) } } @@ -1673,7 +1677,11 @@ pub extern "C" fn js_weakmap_get(map: f64, key: f64) -> f64 { continue; // tombstoned (key collected) } if stored_key == key.to_bits() { - return f64::from_bits(object_field_bits(entry, WEAK_ENTRY_VALUE_FIELD)); + // #7900: shade the key (so a pending weak slice cannot tombstone + // this entry mid-turn) and the value handed to the mutator. + read_barrier::weak_read_barrier(stored_key); + let value = object_field_bits(entry, WEAK_ENTRY_VALUE_FIELD); + return read_barrier::weak_read_barrier_f64(value); } } } @@ -1702,6 +1710,7 @@ pub extern "C" fn js_weakmap_has(map: f64, key: f64) -> f64 { continue; // tombstoned (key collected) } if stored_key == key.to_bits() { + read_barrier::weak_read_barrier(stored_key); // #7900: keep has/get agreeing return f64::from_bits(TAG_TRUE); } } diff --git a/crates/perry-runtime/src/weakref/read_barrier.rs b/crates/perry-runtime/src/weakref/read_barrier.rs new file mode 100644 index 0000000000..813372232a --- /dev/null +++ b/crates/perry-runtime/src/weakref/read_barrier.rs @@ -0,0 +1,71 @@ +//! Weak-to-strong READ barrier (#7900). +//! +//! # The hole this closes +//! +//! A budgeted cycle runs its one-time `FinalRootRemark` and then keeps opening +//! mutator windows while `AtomicFinalize` is still sliced — the full path's +//! `RememberedSetRebuild`, and (since #7892) the weak-holder loop itself. The +//! collector's soundness argument for those windows is "the incremental mark +//! barrier shades every store, and mid-cycle allocations are born black". Both +//! mechanisms only observe values the mutator **writes** or **creates**. +//! +//! `WeakRef.deref()` and `WeakMap.get()` do neither. They take a white object — +//! white *by construction*, because weak edges are deliberately excluded from +//! the strong trace — and hand it to compiled code as a strong local. That is a +//! white-to-strong transition through a pure READ. The remark has already run, +//! so no later root scan can discover the new reference; the next weak slice +//! sees the target unmarked, tombstones it, and the sweep reclaims memory the +//! mutator is still holding. +//! +//! # The barrier +//! +//! Every weak read shades the value words it hands out, exactly as a store +//! barrier would have shaded them on the way into the heap. Consequences: +//! +//! * the pending weak decision sees `GC_FLAG_MARKED` and keeps the slot +//! (`weak_target_should_clear` is a mark-set predicate), so the target is not +//! tombstoned mid-turn — which is also what the spec's `AddToKeptObjects` +//! requires of `WeakRef.deref`; +//! * the shade pushes a mark seed, and the pre-sweep drains (the minor arm of +//! `RememberedSetRebuild`, the full arm of `DisableBarrier`, and `step_sweep`'s +//! gap drain) trace the target's children, so marking it does not leave a +//! marked-but-untraced object with white children; +//! * outside a cycle it is inert. That matters: a stray mark laid down with no +//! cycle in flight reads as "already live" to the NEXT cycle's trace. The +//! inertness is pinned by `weak_read_barrier_is_inert_outside_a_cycle`. +//! +//! Cost is one relaxed load of the process-wide barrier-active count on a path +//! that is already a linear scan (`WeakMap`) or a by-name field read +//! (`WeakRef`). It is not a hot path. +//! +//! # Scope +//! +//! Only the budgeted (incremental) collector has post-remark mutator windows. +//! The copied minor processes its weak registry inside one uninterruptible +//! step, and synchronous cycles pass an unbounded budget, so neither can +//! interleave a read — but the barrier is unconditional rather than +//! phase-gated, because "which subphase is parked" is not observable from a +//! runtime helper and a phase-gated barrier is one reordering away from being +//! wrong again. + +/// Shade one value word handed from a weak slot to the mutator. +/// +/// Returns `true` when this call actually marked a previously-white object, +/// which is what the tests assert to prove the barrier's subject was live. +#[inline] +pub(super) fn weak_read_barrier(value_bits: u64) -> bool { + let shaded = crate::gc::gc_weak_read_shade(value_bits); + #[cfg(test)] + if shaded { + super::test_support::note_weak_read_barrier_shade(); + } + shaded +} + +/// Convenience wrapper for the `f64`-typed FFI returns: shade, then pass the +/// value straight through. +#[inline] +pub(super) fn weak_read_barrier_f64(value_bits: u64) -> f64 { + weak_read_barrier(value_bits); + f64::from_bits(value_bits) +} diff --git a/crates/perry-runtime/src/weakref/test_support.rs b/crates/perry-runtime/src/weakref/test_support.rs index 9bfc37102c..5960464c27 100644 --- a/crates/perry-runtime/src/weakref/test_support.rs +++ b/crates/perry-runtime/src/weakref/test_support.rs @@ -3,6 +3,23 @@ use super::WEAK_HOLDERS; thread_local! { static FULL_WEAK_PROCESSING_WORK_UNITS: std::cell::Cell = const { std::cell::Cell::new(0) }; + /// #7900: how many white objects the weak-READ barrier actually shaded. + /// Tests assert this is non-zero so a green run cannot mean "the read + /// happened to return an already-marked target" (CLAUDE.md: a gate must + /// assert its subject was live). + static WEAK_READ_BARRIER_SHADES: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +pub(crate) fn weak_read_barrier_shades() -> usize { + WEAK_READ_BARRIER_SHADES.with(std::cell::Cell::get) +} + +pub(crate) fn reset_weak_read_barrier_shades() { + WEAK_READ_BARRIER_SHADES.with(|shades| shades.set(0)); +} + +pub(crate) fn note_weak_read_barrier_shade() { + WEAK_READ_BARRIER_SHADES.with(|shades| shades.set(shades.get().saturating_add(1))); } pub(crate) fn full_weak_processing_work_units() -> usize {