From 73d8d68654aa07bb202bb7fef79972e810b29311 Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 3 Aug 2026 00:58:21 -0400 Subject: [PATCH] fix(gc-tests): suppress automatic GC triggers around raw-pointer forwarding setup prototype_addr_cache.rs and side_table_scanners.rs hold raw GC pointers (nursery/old-gen addresses in native locals, no root registered) across further arena_alloc_gc / arena_alloc_gc_old calls while hand-staging forwarding chains. If one of those allocations lands on the block-full slow path, arena_cell_alloc's gc_check_trigger() can run a real collection and move/free the held object out from under the test. Apply the sibling pattern already used in callback_scanners.rs, hook_dispatch_handles.rs, and transient_handles.rs: GcTriggerThresholdTestGuard::suppress_automatic_triggers() at every exposed call site. Sites that are not exposed carry a comment explaining why, rather than a blanket guard. --- .../7283-gc-test-trigger-suppression.md | 13 ++++++++ .../runtime_roots/prototype_addr_cache.rs | 32 ++++++++++++++++++ .../runtime_roots/side_table_scanners.rs | 33 +++++++++++++++++++ 3 files changed, 78 insertions(+) create mode 100644 changelog.d/7283-gc-test-trigger-suppression.md diff --git a/changelog.d/7283-gc-test-trigger-suppression.md b/changelog.d/7283-gc-test-trigger-suppression.md new file mode 100644 index 0000000000..612579c565 --- /dev/null +++ b/changelog.d/7283-gc-test-trigger-suppression.md @@ -0,0 +1,13 @@ +Several `gc::tests::runtime_roots` helpers and tests in `prototype_addr_cache.rs` +and `side_table_scanners.rs` held a raw GC pointer across `arena_alloc_gc` / +`arena_alloc_gc_old` without suppressing automatic GC triggers. Those +allocations can land on the block-full slow path, which calls +`gc_check_trigger()` — an uncontrolled collection there could relocate or free +the held pointer's object before the test finished with it, since the +pointer was live only as a native local with no root registered. + +Both files now use the same `GcTriggerThresholdTestGuard::suppress_automatic_triggers()` +pattern already used by `callback_scanners.rs`, `hook_dispatch_handles.rs`, and +`transient_handles.rs`, applied at every call site where a raw pointer/address +is live across a further allocation. Sites that are not exposed carry a brief +comment explaining why. 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 237b133688..907775b20c 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 @@ -57,12 +57,27 @@ impl Drop for ProtoAddrCacheGuard { } /// Allocate a nursery object to stand in for the intrinsic. +/// +/// Not exposed on its own: nothing is live in this function before the +/// allocation, so there is no raw pointer for an in-flight trigger to move +/// out from under. Callers that keep the returned pointer live across a +/// FURTHER allocation (`forwarded_pair`, the multi-hop test, the collector +/// rewrite test) carry their own guard. fn nursery_stand_in() -> *mut u8 { crate::arena::arena_alloc_gc(64, 8, GC_TYPE_ARRAY) } /// Evacuate `from`: allocate an old-gen destination and forward `from` → `to`. +/// +/// `from` is a raw pointer to a movable nursery object that stays live across +/// the allocation below. If that allocation lands on the block-full slow +/// path, `arena_cell_alloc` calls `gc_check_trigger()`, and — absent +/// suppression — an automatic collection could relocate/free `from` before +/// the forwarding address is installed, corrupting the synthetic setup this +/// test file relies on (#6981's tests never intend to exercise a *real* +/// concurrent collection here, only the hand-driven forwarding chain). fn evacuate(from: *mut u8) -> usize { + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); let to = crate::arena::arena_alloc_gc_old(64, 8, GC_TYPE_ARRAY); unsafe { set_forwarding_address(header_from_user_ptr(from) as *mut GcHeader, to); @@ -71,7 +86,13 @@ fn evacuate(from: *mut u8) -> usize { } /// Allocate `from` and `to`, forward `from` → `to`, and return the pair. +/// +/// `from` is also live in THIS frame across the call to `evacuate` (which +/// performs the allocation). Guarded here too, in addition to `evacuate`'s +/// own guard, so the suppression holds for as long as `from` is live in any +/// frame on this call chain, independent of `evacuate`'s internals. fn forwarded_pair() -> (usize, usize) { + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); let from = nursery_stand_in(); let to = evacuate(from); (from as usize, to) @@ -104,6 +125,9 @@ fn read_addr(which: &str) -> usize { #[test] fn prototype_addr_reads_through_a_forwarding_stub() { let _guard = ProtoAddrCacheGuard::new(); + // Not exposed at this level: `from`/`to` come back as plain `usize`s from + // `forwarded_pair`, which carries its own trigger guard, and nothing else + // in this loop body allocates. for which in ["array", "object"] { let (from, to) = forwarded_pair(); @@ -132,6 +156,10 @@ fn prototype_addr_reads_through_a_forwarding_stub() { #[test] fn prototype_addr_reads_through_a_multi_hop_forwarding_chain() { let _guard = ProtoAddrCacheGuard::new(); + // `first` is live across `second`'s allocation, and both `first` and + // `second` are live across `final_user`'s allocation — any of the three + // could reach the block-full slow path's `gc_check_trigger()`. + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); let first = crate::arena::arena_alloc_gc(64, 8, GC_TYPE_ARRAY); let second = crate::arena::arena_alloc_gc(64, 8, GC_TYPE_ARRAY); @@ -156,6 +184,10 @@ fn prototype_addr_reads_through_a_multi_hop_forwarding_chain() { #[test] fn prototype_addr_cache_is_rewritten_by_the_collector() { let _guard = ProtoAddrCacheGuard::new(); + // `array_from` is live across the second `nursery_stand_in` call below + // (its own allocation, unguarded on its own), and both from-pointers stay + // live across the `evacuate` calls that follow. + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); // The from-space objects must exist before the valid-pointer set is built — // that set is what tells the rewrite visitor an address is a real heap diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/side_table_scanners.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/side_table_scanners.rs index 90f3dc91cd..7bb22898eb 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots/side_table_scanners.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots/side_table_scanners.rs @@ -10,6 +10,11 @@ fn test_implicit_this_root_scanner_marks_and_rewrites() { // when the cell is its only root, and (b) REWRITE the cell to the moved // copy so the body's next `this`-derived dispatch derefs live memory // instead of the stale slot (the reported SIGSEGV in js_native_call_method). + // `nursery_user` is live across the `arena_alloc_gc_old` call below (used + // afterwards to build `nursery_hdr`); the block-full slow path in that + // allocation can reach `gc_check_trigger()`, so suppress automatic + // triggers for the setup below. + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); clear_marks(); clear_mark_seeds(); let prev_this = crate::object::js_implicit_this_get(); @@ -59,6 +64,12 @@ fn test_implicit_this_root_scanner_marks_and_rewrites() { #[test] fn test_class_side_table_scanner_marks_values_but_not_function_keys() { let _guard = GcTestIsolationGuard::new(); + // `dynamic_value`/`prototype_value`/`cached_value`/`prototype_object` are + // all live across the two `arena_alloc_gc` calls below (`parent_closure`, + // `function_key`), and `parent_closure` is live across `function_key`'s — + // any of those allocations can reach the block-full slow path's + // `gc_check_trigger()`. + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); clear_marks(); clear_mark_seeds(); crate::object::test_clear_class_side_table_roots(); @@ -115,6 +126,10 @@ fn test_class_side_table_scanner_marks_values_but_not_function_keys() { #[test] fn test_registered_class_side_table_scanner_rewrites_values_and_function_keys() { let _guard = GcTestIsolationGuard::new(); + // `value_user` is live across `key_user`'s `arena_alloc_gc` call, and both + // `value_user`/`key_user` (plus `value_old` once allocated) are live + // across the remaining `arena_alloc_gc`/`arena_alloc_gc_old` calls below. + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); crate::object::test_clear_class_side_table_roots(); gc_register_mutable_root_scanner(crate::object::scan_class_side_table_roots_mut); @@ -191,6 +206,10 @@ fn test_registered_class_side_table_scanner_rewrites_values_and_function_keys() #[test] fn test_symbol_side_table_scanner_marks_keys_and_values_without_marking_owner() { + // Not exposed: every allocation here goes through opaque helpers + // (`js_object_alloc`, `alloc_nursery_test_symbol`, `young_leaf`) with no + // direct `arena_alloc_gc`/`arena_alloc_gc_old` call written in this file, + // so there is no in-file call site to guard. let _guard = GcTestIsolationGuard::new(); clear_marks(); clear_mark_seeds(); @@ -226,6 +245,10 @@ fn test_symbol_side_table_scanner_marks_keys_and_values_without_marking_owner() #[test] fn test_symbol_side_table_registered_scanner_rewrites_roots_and_metadata() { let _guard = GcTestIsolationGuard::new(); + // `owner`/`sym_key`/`value`/`static_sym_key`/`static_value` (and the + // `_old` addresses as they're allocated) are all live across the three + // `arena_alloc_gc_old` calls below. + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); crate::symbol::test_clear_symbol_side_table_roots(); gc_register_mutable_root_scanner(crate::symbol::scan_symbol_side_table_roots_mut); @@ -307,6 +330,8 @@ fn test_symbol_side_table_registered_scanner_rewrites_roots_and_metadata() { #[test] fn test_runtime_root_visitor_rewrites_raw_pointer_slots() { + // `nursery_user` is live across the `arena_alloc_gc_old` call below. + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); let nursery_user = crate::arena::arena_alloc_gc(64, 8, GC_TYPE_OBJECT); let valid_ptrs = build_valid_pointer_set(); let old_user = crate::arena::arena_alloc_gc_old(64, 8, GC_TYPE_OBJECT); @@ -353,6 +378,10 @@ fn test_class_inheritance_side_table_roots_mark_and_rewrite() { const PROTO_CID: u32 = 0xDEAD_0001; const CLOSURE_CID: u32 = 0xDEAD_0002; + // `proto_user`/`decl_proto_user`/`closure_user` (and the `_old` addresses + // as they're allocated) are all live across the later `arena_alloc_gc`/ + // `arena_alloc_gc_old` calls below. + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); clear_marks(); clear_mark_seeds(); @@ -432,6 +461,8 @@ fn test_class_inheritance_side_table_roots_mark_and_rewrite() { #[test] fn test_runtime_root_visitor_rewrites_cell_and_atomic_slots() { + // `nursery_user` is live across the `arena_alloc_gc_old` call below. + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); let nursery_user = crate::arena::arena_alloc_gc(64, 8, GC_TYPE_OBJECT); let valid_ptrs = build_valid_pointer_set(); let old_user = crate::arena::arena_alloc_gc_old(64, 8, GC_TYPE_OBJECT); @@ -485,6 +516,8 @@ fn test_runtime_root_visitor_rewrites_cell_and_atomic_slots() { #[test] fn test_runtime_root_visitor_rewrites_metadata_without_marking() { + // `nursery_user` is live across the `arena_alloc_gc_old` call below. + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); let nursery_user = crate::arena::arena_alloc_gc(64, 8, GC_TYPE_OBJECT); let valid_ptrs = build_valid_pointer_set(); let old_user = crate::arena::arena_alloc_gc_old(64, 8, GC_TYPE_OBJECT);