fix(codegen): initialize typed-shape layout on the standalone-ctor exit (#6921)

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, "<Class>_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.

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 `<Class>_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, "<Class>_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);
     }
 

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::<crate::object::ObjectHeader>())
+            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();
+}

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() {

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.
 ///