diff --git a/changelog.d/6932-gc-root-word-test-handle-hygiene.md b/changelog.d/6932-gc-root-word-test-handle-hygiene.md new file mode 100644 index 0000000000..36dbcd1c4c --- /dev/null +++ b/changelog.d/6932-gc-root-word-test-handle-hygiene.md @@ -0,0 +1,9 @@ +**Tests: handle hygiene in the #6910 root-word GC tests (follow-up to #6929).** + +A GC test that holds raw pointers across GC-capable calls is the same unrooted-handle bug class as #6655, inside tests whose job is validating GC correctness — an unsound GC test can pass for the wrong reason and mask the behavior it exists to pin. Audited all four tests landed in #6929. + +`mutable_root_mark_and_rewrite_accept_the_same_word_forms` held `mark_target` / `rewrite_target` as raw `usize` across two further arena allocations with no trigger suppression; evacuation moves arena objects, so either could have named from-space by the time the walks ran. Fixed with `GcTriggerThresholdTestGuard::suppress_automatic_triggers()` rather than a `RuntimeHandleScope` — deliberately, and the code says why: this test hand-builds a `ValidPointerSet` snapshot and hand-sets a forwarding address, neither of which survives a real collection however well the objects are rooted, so the hazard is the collection itself. `build_valid_pointer_set()` also moved after the last allocation (behavior-identical, but "snapshot the heap, then allocate into it" read wrong). + +`bare_address_in_shadow_slot_survives_a_real_collection` and `bare_address_in_global_root_survives_a_real_collection` were sound but brittle — `live` is `gc_malloc`-backed and the collector marks malloc objects in place without relocating (`CopyingPointerKind::Malloc => Some(addr)`). They now read the survivor back out of the root slot instead of reusing the pre-cycle raw pointer, which drops the dependence on that policy and additionally proves the slot itself was maintained across the collection. `decode_root_word_round_trips_each_representation` needed no change (pure bit math, no allocation). + +Detection power re-verified rather than assumed: with `mark_mutable_root_bits` temporarily reverted to the pre-#6910 NaN-box-only mark, the hardened tests still fail with the original signature. Stress matrix 9/9 green (`PERRY_GC_FORCE_EVACUATE=1`, `PERRY_GC_VERIFY_EVACUATION=1`, `PERRY_GEN_GC=0`, `PERRY_WRITE_BARRIERS=0` and combinations). Tests only — no runtime or codegen change. diff --git a/crates/perry-runtime/src/gc/tests/root_words.rs b/crates/perry-runtime/src/gc/tests/root_words.rs index 14ea602a68..f4fd449dc6 100644 --- a/crates/perry-runtime/src/gc/tests/root_words.rs +++ b/crates/perry-runtime/src/gc/tests/root_words.rs @@ -112,6 +112,19 @@ fn root_walk_rewrites(kind: ProbeSlot, bits: u64, valid_ptrs: &ValidPointerSet) fn mutable_root_mark_and_rewrite_accept_the_same_word_forms() { let _guard = GcTestIsolationGuard::new(); let _scan = ConservativeScanDisabledGuard::new(); + // This test hand-builds its own collector state: a `ValidPointerSet` + // snapshot and a hand-set forwarding address, then drives the mark and + // rewrite walks directly. A collection firing between any two lines below + // would invalidate all of that — and the three arena allocations are each + // a GC point, with the targets held as raw addresses across them. + // + // Note that rooting the targets would NOT be sufficient here, which is why + // this is trigger suppression rather than a `RuntimeHandleScope`: keeping + // the objects alive does not keep the pre-built `valid_ptrs` snapshot + // accurate, nor protect the forwarding bit this test sets by hand. The + // hazard is the collection itself, so the fix is to make the region + // collection-free. + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); clear_marks(); clear_mark_seeds(); @@ -119,8 +132,10 @@ fn mutable_root_mark_and_rewrite_accept_the_same_word_forms() { // while the rewrite probe needs a forwarded one. let mark_target = crate::arena::arena_alloc_gc(40, 8, GC_TYPE_OBJECT) as usize; let rewrite_target = crate::arena::arena_alloc_gc(40, 8, GC_TYPE_OBJECT) as usize; - let valid_ptrs = build_valid_pointer_set(); let moved = crate::arena::arena_alloc_gc_old(40, 8, GC_TYPE_OBJECT) as usize; + // Snapshot AFTER every allocation, so the set describes the heap the walks + // below actually run against. + let valid_ptrs = build_valid_pointer_set(); unsafe { set_forwarding_address( header_from_user_ptr(rewrite_target as *const u8), @@ -223,14 +238,24 @@ fn bare_address_in_shadow_slot_survives_a_real_collection() { 0, "the sweep must actually have run for this test to mean anything" ); + // Read the survivor back out of the ROOT rather than reusing the raw + // pointer captured before the cycle. `live` is `gc_malloc`-backed and the + // collector marks malloc objects in place (`CopyingPointerKind::Malloc` + // returns the address unchanged), so the two agree today — but depending + // on that makes the test quietly wrong the day malloc objects become + // relocatable, and reloading is the stronger assertion anyway: it also + // proves the slot itself was maintained across the collection. + let survivor = js_shadow_slot_get(0); + assert_ne!(survivor, 0, "the shadow slot must still hold the survivor"); + let survivor = survivor as *mut u8; assert!( - malloc_user_ptr_tracked(live), + malloc_user_ptr_tracked(survivor), "an object reachable only through a bare address in a shadow-stack \ slot must be marked, not swept (#6910)" ); unsafe { assert_eq!( - (*(live as *mut crate::closure::ClosureHeader)).type_tag, + (*(survivor as *mut crate::closure::ClosureHeader)).type_tag, crate::closure::CLOSURE_MAGIC, "surviving object must still be intact" ); @@ -270,14 +295,29 @@ fn bare_address_in_global_root_survives_a_real_collection() { 0, "the sweep must actually have run for this test to mean anything" ); + // Same discipline as the shadow-slot case: the root slot, not the raw + // pointer captured before the cycle, is the authority on where the object + // is now. + assert_ne!( + global_slot, 0, + "the global root must still hold the survivor" + ); assert!( - malloc_user_ptr_tracked(live), + malloc_user_ptr_tracked(global_slot as *mut u8), "a bare address in a registered global root must still be marked" ); assert_eq!( global_slot, live as u64, - "an unmoved target must leave the bare global slot untouched" + "a malloc-backed target is never relocated, so the bare global slot \ + must come back byte-identical" ); + unsafe { + assert_eq!( + (*(global_slot as *mut crate::closure::ClosureHeader)).type_tag, + crate::closure::CLOSURE_MAGIC, + "surviving object must still be intact" + ); + } } /// The decoder is the single place that knows the word forms; assert the two