diff --git a/changelog.d/6930-typed-shape-layout-ctor-exit.md b/changelog.d/6930-typed-shape-layout-ctor-exit.md new file mode 100644 index 0000000000..455b96b717 --- /dev/null +++ b/changelog.d/6930-typed-shape-layout-ctor-exit.md @@ -0,0 +1,9 @@ +**codegen: initialize the typed-shape layout on `lower_new_impl`'s standalone-constructor exit (#6921).** + +`lower_new_impl` had one exit that returned a freshly allocated class instance without emitting `js_gc_init_typed_shape_layout`; every other `new` exit emits it. Such an instance is left at `GC_LAYOUT_POINTER_FREE` with no `TypedLayoutDescriptor` — the one layout state where the per-store `layout_note_slot` call is load-bearing for GC correctness rather than a precision hint, since it is the only writer of the pointer-mask bit the collector reads. That blocked the full pointer-masked layout-note elision (the larger half of Phase 4b.1 in #6919, which had to narrow to the value-only predicate because of it). + +**The arm appears to be unreachable today, and no reproducer exists.** `call_local_constructor_symbol` returns `None` only when `ctx.methods` lacks `(class.name, "_constructor")`, but `lower_new_impl` resolves `class` exclusively from `ctx.classes` (`new.rs:237`), `ctx.classes` *is* the `class_table`, and `build_method_names` iterates `class_table.values()` inserting that key unconditionally for every entry — local and imported alike (`method_registry.rs:112-119`). An instrumented compiler reported zero hits across all 430 `test-files/test_gap_*.ts` (codegen-only) plus three hand-written self-referential construction shapes (self-construction in a method, a recursive own constructor, a field initializer constructing its own class) — all of which do enter the recursion-guarded branch and all of which take the `Some` arm that already emitted the init. + +The emitter lands anyway so the invariant "a user-class instance reaching a class-field store carries a typed descriptor, or is explicitly `GC_LAYOUT_UNKNOWN`" holds by construction at this exit rather than by an accident of the registry that a future change could silently revoke. Cost is zero — dead path today, and `emit_typed_shape_layout_init` is itself a no-op for a class with no `class_keys_globals` entry. + +Because the arm is unreachable, no behavioral test can fail before and pass after. What is testable is the premise the fix rests on — the instance reaching this exit has had no constructor run, so the layout init sees all-`undefined` fields. `gc::tests::layout_trace::typed_shape_layout_init_on_unconstructed_instance_is_conservative` pins that a fresh instance really is `GC_LAYOUT_POINTER_FREE` with no descriptor; that a raw-f64 mask over `undefined` fields is refused and downgraded to `GC_LAYOUT_UNKNOWN` (why emitting the init is safe); and that a pointer-only mask is installed and its slot traced with no `layout_note_slot` call (why emitting it is useful). Green across the GC stress matrix: `PERRY_GC_FORCE_EVACUATE=1`, `PERRY_GC_VERIFY_EVACUATION=1`, `PERRY_GEN_GC=0`, `PERRY_WRITE_BARRIERS=0` and combinations. diff --git a/crates/perry-codegen/src/lower_call/new.rs b/crates/perry-codegen/src/lower_call/new.rs index bc5f58dc18..0c7f321eea 100644 --- a/crates/perry-codegen/src/lower_call/new.rs +++ b/crates/perry-codegen/src/lower_call/new.rs @@ -902,6 +902,41 @@ fn lower_new_impl( ctx.block() .call(DOUBLE, "js_new_target_set", &[(DOUBLE, prev)]); } + // #6921: `call_local_constructor_symbol` returned `None` — this module + // has no `_constructor` entry, so no constructor ran and the + // instance leaves here exactly as `js_object_alloc_class_*` produced + // it. Every OTHER `new` exit initializes the typed-shape layout; this + // one used to return the instance at `GC_LAYOUT_POINTER_FREE` with no + // `TypedLayoutDescriptor`, the one state in which the per-store + // `layout_note_slot` call is load-bearing for GC correctness rather + // than a precision hint — so eliding that note (Phase 4b.1) could + // strand a live child on an object the collector scans zero slots of. + // + // Initialize it here too, so the invariant "a user-class instance + // reaching a class-field store carries a typed descriptor, or is + // explicitly `GC_LAYOUT_UNKNOWN`" is total. This is safe by + // construction rather than by reasoning about this path: the fields + // are still `TAG_UNDEFINED` (no ctor ran), and `init_typed_shape_layout` + // validates every live field word before promoting — a raw-f64 slot + // holding `undefined` fails `layout_raw_f64_bits` and the object lands + // in `GC_LAYOUT_UNKNOWN`, the conservative state, instead of a wrong + // mask. `emit_typed_shape_layout_init` is itself a no-op for a class + // with no `class_keys_globals` entry. + // + // Reachability, measured (not assumed): this arm is currently DEAD. + // `call_local_constructor_symbol` returns `None` only when + // `ctx.methods` lacks `(class.name, "_constructor")`, but + // `lower_new_impl` resolves `class` exclusively from `ctx.classes` + // (the `class_table`), and `build_method_names` iterates + // `class_table.values()` inserting that key unconditionally for every + // entry — local and imported alike. So no class reaching here can miss + // it. An instrumented compiler over the whole `test_gap_*` corpus plus + // hand-written recursive-construction shapes never hit this arm. + // The emitter stays anyway: the invariant must hold by construction at + // this exit, not by an accident of the registry that a future change + // to `build_method_names` (or a new `ctx.classes` population path) + // could silently revoke. + emit_typed_shape_layout_init(ctx, class_name, &obj_handle); return Ok(obj_box); } diff --git a/crates/perry-runtime/src/gc/tests/layout_trace.rs b/crates/perry-runtime/src/gc/tests/layout_trace.rs index 93ea7008fa..b2cce8a815 100644 --- a/crates/perry-runtime/src/gc/tests/layout_trace.rs +++ b/crates/perry-runtime/src/gc/tests/layout_trace.rs @@ -1672,3 +1672,173 @@ fn test_int32_store_without_typed_descriptor_is_left_verbatim() { "with no intact descriptor there is no raw-f64 contract to uphold — bits stay verbatim" ); } + +/// #6921 — the `lower_new_impl` standalone-constructor exit returns a freshly +/// allocated class instance on which NO constructor has run, so the +/// `js_gc_init_typed_shape_layout` that exit now emits sees an object whose +/// every field is still `undefined`. That is the premise the fix rests on, so +/// pin it here rather than reasoning about it. +/// +/// Three properties, in the order they matter: +/// +/// 1. A fresh instance really is left at `GC_LAYOUT_POINTER_FREE` with no +/// descriptor — the one state in which the per-store `layout_note_slot` +/// call is load-bearing for GC correctness rather than a precision hint. +/// That is why an exit which skips the layout init blocks the note elision. +/// 2. A raw-f64 mask CANNOT be honoured over `undefined` fields, and +/// `init_typed_shape_layout` must downgrade to `GC_LAYOUT_UNKNOWN` — the +/// conservative state — never install a mask that fails to describe the +/// live words. This is what makes emitting the init on that exit SAFE. +/// 3. A pointer-only mask IS installed over `undefined` fields, so a later +/// pointer store into that slot is traced even though `layout_note_slot` +/// never ran. This is what makes emitting it USEFUL. +#[test] +fn typed_shape_layout_init_on_unconstructed_instance_is_conservative() { + let _guard = GcTestIsolationGuard::new(); + // `GcTestIsolationGuard` takes the thread's mutable-root scanner registry, + // which includes the runtime-handle scanner. Put it back BEFORE opening the + // scope below, or the handles are decorative and every object here is an + // unrooted raw pointer held across a GC-capable allocation — the same bug + // class (#6655) this test exists to be free of. + register_runtime_handle_root_scanner_for_tests(); + let scope = RuntimeHandleScope::new(); + clear_marks(); + clear_mark_seeds(); + + // Every allocation below is a GC point, and evacuation MOVES arena objects, + // so nothing may be held as a raw pointer across one. Each instance is + // rooted in `scope` the moment it is created and re-read through + // `handle_user` after any later allocation. + fn handle_user(handle: &RuntimeHandle<'_>) -> usize { + (handle.get_nanbox_u64() & POINTER_MASK) as usize + } + let layout_state = |user: usize| unsafe { + (*header_from_user_ptr(user as *const u8))._reserved & GC_LAYOUT_STATE_MASK + }; + let alloc_instance = + || crate::object::js_object_alloc_class_inline_keys(0, 0, 2, std::ptr::null_mut()) as usize; + + // (1) The unconstructed instance, exactly as `js_object_alloc_class_*` + // hands it to the standalone-ctor exit. + let fresh = scope.root_nanbox_u64(ptr_bits(alloc_instance())); + let fresh_user = handle_user(&fresh); + assert_eq!( + layout_state(fresh_user), + GC_LAYOUT_POINTER_FREE, + "a fresh class instance is POINTER_FREE — the collector scans zero \ + slots on it until something publishes a pointer bit" + ); + assert!( + !layout_has_typed_descriptor(fresh_user), + "and carries no typed descriptor" + ); + + // (2) Raw-f64 mask over all-`undefined` fields must land in UNKNOWN. + let raw_only = scope.root_nanbox_u64(ptr_bits(alloc_instance())); + let raw_only_user = handle_user(&raw_only); + let raw_mask = [0b01u64]; + js_gc_init_typed_shape_layout( + raw_only_user as u64, + 2, + raw_mask.as_ptr(), + raw_mask.len() as u32, + std::ptr::null(), + 0, + ); + assert_eq!( + layout_state(raw_only_user), + GC_LAYOUT_UNKNOWN, + "`undefined` is not raw-f64 bits, so the descriptor must be refused \ + and the object downgraded to the conservative state — never left \ + POINTER_FREE, never given a mask that misdescribes it" + ); + assert!( + !layout_has_typed_descriptor(raw_only_user), + "a refused descriptor must not be installed" + ); + + // (3) Pointer-only mask over all-`undefined` fields IS installed, and the + // slot is traced on a later store without any `layout_note_slot` call. + let ptr_only = scope.root_nanbox_u64(ptr_bits(alloc_instance())); + let ptr_only_user = handle_user(&ptr_only); + let ptr_mask = [0b01u64]; + js_gc_init_typed_shape_layout( + ptr_only_user as u64, + 2, + std::ptr::null(), + 0, + ptr_mask.as_ptr(), + ptr_mask.len() as u32, + ); + assert_eq!( + layout_state(ptr_only_user), + GC_LAYOUT_SIDE_MASK, + "a pointer mask is compatible with `undefined` fields and is installed" + ); + assert_eq!( + test_layout_pointer_slot_count(ptr_only_user, 2), + Some(1), + "slot 0 is published as pointer-bearing" + ); + + // The child is reachable ONLY through that slot; write it with a raw store + // so no `layout_note_slot` runs, then prove tracing still finds it. + let child = scope + .root_nanbox_u64(string_bits( + crate::string::js_string_from_bytes(b"6921-child".as_ptr(), 10) as usize, + )); + // Allocating the child was a GC point: re-read the instance rather than + // reusing `ptr_only_user`, which may now name from-space. + let ptr_only_user = handle_user(&ptr_only); + let fields = unsafe { + (ptr_only_user as *mut u8).add(std::mem::size_of::()) + as *mut u64 + }; + unsafe { + std::ptr::write(fields, child.get_nanbox_u64()); + } + + // Force a real collection here, with the conservative native-stack scan + // pinned OFF. That makes the `RuntimeHandleScope` above LOAD-BEARING rather + // than decorative: the raw Rust locals are no longer a safety net, so the + // scope is the only thing keeping these objects alive. Drop the scanner + // registration at the top of this test and this collection reclaims them. + { + let _scan = ConservativeScanDisabledGuard::new(); + let _ = collect_minor_trace(GcTriggerKind::Direct); + } + // Re-read everything through the handles — a copied minor relocates + // nursery objects and rewrites the rooted slots. + let ptr_only_user = handle_user(&ptr_only); + assert_eq!( + test_layout_pointer_slot_count(ptr_only_user, 2), + Some(1), + "the typed descriptor must survive the collection (and any relocation) \ + — the note-elision premise depends on it staying intact, not just on \ + it being installed once" + ); + + // A collection may have left objects marked; start the hand-driven mark + // from a known-clean state so the assertions below mean what they say. + clear_marks(); + clear_mark_seeds(); + let valid_ptrs = build_valid_pointer_set(); + assert!( + try_mark_value(ptr_bits(handle_user(&ptr_only)), &valid_ptrs), + "test setup: the instance marks as a root" + ); + trace_marked_objects(&valid_ptrs); + unsafe { + let child_header = header_from_user_ptr(handle_user(&child) as *const u8); + assert_ne!( + (*child_header).gc_flags & GC_FLAG_MARKED, + 0, + "the typed descriptor alone must make the pointer slot traceable — \ + this is the liveness `layout_note_slot` would otherwise have had \ + to establish store-by-store" + ); + } + + clear_marks(); + clear_mark_seeds(); +} diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots.rs b/crates/perry-runtime/src/gc/tests/runtime_roots.rs index 1d0feea78a..6e21a175e6 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots.rs @@ -106,14 +106,8 @@ fn assert_moved_closure_ptr(bits: u64, original: usize) -> usize { rewritten } -fn register_runtime_handle_root_scanner_for_tests() { - gc_register_budgeted_mutable_root_scanner_with_source( - scan_runtime_handle_roots_mut, - scan_runtime_handle_roots_mut_step, - new_runtime_handle_root_scan_state, - MutableRootScannerSource::RuntimeHandles, - ); -} +// `register_runtime_handle_root_scanner_for_tests` moved to `super::support` +// so the layout/tracing tests can root handles the same way (#6930 review). #[test] fn test_scoped_root_scanner_registry_guard_restores_counts() { diff --git a/crates/perry-runtime/src/gc/tests/support.rs b/crates/perry-runtime/src/gc/tests/support.rs index 325fbfffb4..c0a4ef1028 100644 --- a/crates/perry-runtime/src/gc/tests/support.rs +++ b/crates/perry-runtime/src/gc/tests/support.rs @@ -229,6 +229,26 @@ impl Drop for ConservativeScanAutoGuard { } } +/// Put the runtime-handle mutable-root scanner back into this thread's +/// registry. +/// +/// REQUIRED before a `RuntimeHandleScope` roots anything inside +/// `ScopedRootScannerRegistryGuard` / `GcTestIsolationGuard` / +/// `CopyingNurseryTestGuard`: those guards `mem::take` the thread's +/// `MUTABLE_ROOT_SCANNERS` so a collection sees exactly the roots the test +/// installs, and the runtime-handle scanner goes with it. Without this call a +/// `RuntimeHandleScope` inside such a test is decorative — its handles are +/// neither marked nor rewritten, so a raw pointer held across a GC-capable +/// call is silently unrooted and the test can pass for the wrong reason. +pub(super) fn register_runtime_handle_root_scanner_for_tests() { + gc_register_budgeted_mutable_root_scanner_with_source( + scan_runtime_handle_roots_mut, + scan_runtime_handle_roots_mut_step, + new_runtime_handle_root_scan_state, + MutableRootScannerSource::RuntimeHandles, + ); +} + /// Pin this thread's conservative-scan mode to `Disabled` for the guard's /// lifetime, restoring the prior override on drop. ///