fix(gc): layout_transfer clears a valid intact bit on shape-keyed objects (#6964)

changelog.d/6974-layout-transfer-shape-keyed-intact.md

@@ -0,0 +1 @@
+**GC / typed layouts:** keep `GC_OBJ_TYPED_LAYOUT_INTACT` across relocation for shape-keyed objects. `layout_transfer` resolved the moved object's canonical typed layout only through the per-object `TYPED_LAYOUTS` map, but #6893 moved the descriptor of every object carrying a `keys_array` — i.e. every class instance — into the shape-keyed `SHAPE_LAYOUTS` map and deletes the per-object entry, so the lookup missed and the relocated copy had a still-valid intact bit cleared, permanently deopting its typed guards. `INTACT` now survives when *either* map resolves a descriptor, mirroring #6963's split (per-object half ungated so a forged bit still cannot manufacture a descriptor; shared half gated on the source object's intact bit so a diverged object cannot re-adopt its shape's stale descriptor by moving). Latent until the evacuation paths become reachable (#6950), which is exactly why it lands first — otherwise it would surface as a diffuse performance regression arriving with a GC change. Regression coverage now uses a real class instance; every pre-existing `layout_transfer` test used `js_object_alloc` (class 0, no `keys_array`), which is why #6893 merged green (#6964).

crates/perry-runtime/src/gc/layout.rs

@@ -1137,6 +1137,12 @@ pub(crate) unsafe fn layout_transfer(old_user: *mut u8, new_user: *mut u8) {
     } else {
         crate::array::clear_array_numeric_layout_ptr(new_user as usize);
     }
+    // Read the source object's intact bit BEFORE the transfer clears it — it is
+    // the per-object half of the shape-keyed resolution below. `_reserved` is
+    // untouched by `set_forwarding_address` (which writes gc_flags and the first
+    // payload word), so it is still authoritative here even though the
+    // evacuation callers forward the original before calling us.
+    let old_intact = (*old_header)._reserved & GC_OBJ_TYPED_LAYOUT_INTACT != 0;
     let new_has_typed = TYPED_LAYOUTS.with(|m| {
         let mut typed = m.borrow_mut();
         typed.remove(&(new_user as usize));
@@ -1147,11 +1153,39 @@ pub(crate) unsafe fn layout_transfer(old_user: *mut u8, new_user: *mut u8) {
             false
         }
     });
+    // #6964: the canonical descriptor may live in EITHER map, exactly as the
+    // query helpers resolve it (#6957/#6963). The per-object `TYPED_LAYOUTS`
+    // entry is keyed by ADDRESS, so it has to be moved (above). The shape-keyed
+    // `SHAPE_LAYOUTS` entry (#6893) is keyed by the shared `keys_array`, which
+    // the relocated copy carries verbatim — it needs no move, but it only
+    // describes THIS object while the object is still INTACT.
+    //
+    // Probing only `TYPED_LAYOUTS` missed for every object #6893 actually moved
+    // (i.e. every class instance: it carries a keys_array and therefore has NO
+    // per-object entry), so `new_has_typed` was false and the relocated copy had
+    // a still-valid intact bit CLEARED — permanently deopting its typed guards.
+    // Latent until an evacuating minor became reachable (#6950); the fourth
+    // caller, array growth in `array/push_pop.rs`, is `GC_TYPE_ARRAY`, which is
+    // not `GcLayoutSlotKind::ObjectFields` and so never had a shape-keyed
+    // descriptor to lose.
+    //
+    // Read the shape through `new_user`: the evacuation callers install the
+    // forwarding pointer over the ORIGINAL's first payload word, which for an
+    // ObjectFields object overlaps the header fields this lookup reads.
+    //
+    // Mirrors #6963's split: the per-object half stays ungated (so a forged or
+    // stale intact bit cannot manufacture a descriptor), the shared half is
+    // gated on the source object's intact bit (so an object that diverged from
+    // its shape does not silently re-adopt the shape's stale descriptor by
+    // moving).
+    let new_has_shape_typed = !new_has_typed
+        && old_intact
+        && with_shape_shared_descriptor(new_user as usize, |_| ()).is_some();
     // Keep the intact bit in lock-step with the moved descriptor. Copying GC
     // normally propagates `_reserved` (so the bit already rode along), but
     // re-sync defensively for callers that allocate the destination fresh
     // (e.g. array growth) so a stale/missing bit can never desync from the map.
-    if new_has_typed {
+    if new_has_typed || new_has_shape_typed {
         header_set_typed_layout_intact(new_header);
     } else {
         header_clear_typed_layout_intact(new_header);

crates/perry-runtime/src/gc/tests/layout_trace.rs

@@ -556,6 +556,151 @@ fn test_typed_shape_descriptor_visible_for_shape_keyed_objects() {
     clear_mark_seeds();
 }
 
+/// #6964: `layout_transfer` resolved the moved object's typed descriptor only
+/// through the per-object `TYPED_LAYOUTS` map. #6893 moved the canonical
+/// descriptor of every object carrying a `keys_array` (i.e. every class
+/// instance) into the shape-keyed `SHAPE_LAYOUTS` map and DELETED the per-object
+/// entry, so that lookup missed and the relocated copy had a still-valid
+/// `GC_OBJ_TYPED_LAYOUT_INTACT` bit cleared.
+///
+/// Deliberately a *shape-keyed* object: every pre-existing `layout_transfer`
+/// test allocates with `js_object_alloc` (class 0, no keys_array), which keeps
+/// its per-object entry and therefore takes the surviving path. That gap is why
+/// #6893 merged green.
+#[test]
+fn test_shape_keyed_typed_layout_survives_layout_transfer() {
+    clear_marks();
+    clear_mark_seeds();
+
+    let packed = b"x\0y\0";
+    let keys = crate::object::js_build_class_keys_array(
+        0x6964_01,
+        2,
+        packed.as_ptr(),
+        packed.len() as u32,
+    );
+    let src = crate::object::js_object_alloc_class_inline_keys(0x6964_01, 0, 2, keys);
+    crate::object::js_object_set_unboxed_f64_field(src, 0, 1.5);
+    crate::object::js_object_set_field(src, 1, crate::value::JSValue::number(2.5));
+    let raw_mask = [0b01u64];
+    js_gc_init_typed_shape_layout(
+        src as u64,
+        2,
+        raw_mask.as_ptr(),
+        raw_mask.len() as u32,
+        std::ptr::null(),
+        0,
+    );
+    assert!(layout_typed_intact_for_user(src as usize));
+    assert!(layout_typed_raw_f64_slot_for_user(src as usize, 0));
+
+    // Model an evacuation copy the way every caller performs it: a destination
+    // of the same shape, payload copied verbatim, `_reserved` propagated, then
+    // `layout_transfer`.
+    let dst = crate::object::js_object_alloc_class_inline_keys(0x6964_01, 0, 2, keys);
+    unsafe {
+        let header_size = std::mem::size_of::<crate::object::ObjectHeader>();
+        std::ptr::copy_nonoverlapping(
+            src as *const u8,
+            dst as *mut u8,
+            header_size + 2 * std::mem::size_of::<crate::value::JSValue>(),
+        );
+        let src_header = header_from_user_ptr(src as *const u8);
+        let dst_header = header_from_user_ptr(dst as *const u8);
+        (*(dst_header as *mut GcHeader))._reserved = (*src_header)._reserved;
+        layout_transfer(src as *mut u8, dst as *mut u8);
+    }
+
+    assert!(
+        layout_typed_intact_for_user(dst as usize),
+        "a relocated shape-keyed object must keep GC_OBJ_TYPED_LAYOUT_INTACT — its \
+         SHAPE_LAYOUTS descriptor is keyed by the shared keys_array, which the copy carries"
+    );
+    assert!(
+        layout_typed_raw_f64_slot_for_user(dst as usize, 0),
+        "slot 0 is still raw-f64 after relocation"
+    );
+    assert!(!layout_typed_raw_f64_slot_for_user(dst as usize, 1));
+    assert!(
+        layout_slot_is_raw_f64_typed(dst as usize, 0),
+        "the store fast path must agree with the descriptor after relocation"
+    );
+
+    // The source is downgraded on transfer (it is dead / a forwarding stub), and
+    // that must NOT take the shared entry with it: an untouched sibling still
+    // reads the shape descriptor.
+    let sibling = crate::object::js_object_alloc_class_inline_keys(0x6964_01, 0, 2, keys);
+    crate::object::js_object_set_unboxed_f64_field(sibling, 0, 7.5);
+    crate::object::js_object_set_field(sibling, 1, crate::value::JSValue::number(8.5));
+    js_gc_init_typed_shape_layout(
+        sibling as u64,
+        2,
+        raw_mask.as_ptr(),
+        raw_mask.len() as u32,
+        std::ptr::null(),
+        0,
+    );
+    assert!(layout_typed_raw_f64_slot_for_user(sibling as usize, 0));
+
+    clear_marks();
+    clear_mark_seeds();
+}
+
+/// #6964, but driven through the real evacuation path (`gc/copying.rs`'s
+/// `layout_transfer` call site) instead of calling the helper directly.
+#[test]
+fn test_shape_keyed_typed_layout_survives_copying_minor() {
+    let _guard = CopyingNurseryTestGuard::new(1);
+    let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers();
+
+    let packed = b"x\0y\0";
+    let keys = crate::object::js_build_class_keys_array(
+        0x6964_02,
+        2,
+        packed.as_ptr(),
+        packed.len() as u32,
+    );
+    let obj = crate::object::js_object_alloc_class_inline_keys(0x6964_02, 0, 2, keys);
+    crate::object::js_object_set_unboxed_f64_field(obj, 0, 10.5);
+    crate::object::js_object_set_field(obj, 1, crate::value::JSValue::number(-3.25));
+    let raw_mask = [0b01u64];
+    js_gc_init_typed_shape_layout(
+        obj as u64,
+        2,
+        raw_mask.as_ptr(),
+        raw_mask.len() as u32,
+        std::ptr::null(),
+        0,
+    );
+    assert!(layout_typed_intact_for_user(obj as usize));
+    assert!(layout_typed_raw_f64_slot_for_user(obj as usize, 0));
+    js_shadow_slot_set(0, ptr_bits(obj as usize));
+
+    let trace = collect_minor_trace(GcTriggerKind::Direct);
+    assert_copied_minor_trace(&trace, true, CopiedMinorFallbackReason::None, false);
+
+    let after = (js_shadow_slot_get(0) & POINTER_MASK) as usize;
+    assert_ne!(
+        after, obj as usize,
+        "the copying minor must actually relocate the instance — an inert arm proves nothing"
+    );
+
+    let fields = unsafe {
+        (after as *const u8).add(std::mem::size_of::<crate::object::ObjectHeader>()) as *const u64
+    };
+    assert_eq!(f64::from_bits(unsafe { *fields.add(0) }), 10.5);
+
+    assert!(
+        layout_typed_intact_for_user(after),
+        "#6964: the relocated class instance must keep its shape-keyed typed layout"
+    );
+    assert!(
+        layout_typed_raw_f64_slot_for_user(after, 0),
+        "#6964: the shape descriptor still describes slot 0 as raw-f64 after relocation"
+    );
+    assert!(layout_slot_is_raw_f64_typed(after, 0));
+}
+
 #[test]
 fn test_typed_shape_raw_numeric_slots_accept_pointer_like_f64_bits() {
     clear_marks();