fix(gc): #6893 left the typed-layout query helpers blind to shape-keyed objects (#6957)

changelog.d/6963-shape-keyed-typed-layout-queries.md

@@ -0,0 +1 @@
+**GC / typed layouts:** restore the typed-shape descriptor lookups for shape-keyed objects. #6893 moved the canonical `TypedLayoutDescriptor` of every class instance into the shape-keyed `SHAPE_LAYOUTS` map and deleted the per-object entry, but four query helpers kept probing `TYPED_LAYOUTS` alone — so every class-field typed guard deopted, `PERRY_VERIFY_TYPED_INTACT=1` aborted on any class instance, and one FFI INT32 store permanently evicted an object's typed descriptor. Fixes the 33 red `perry-runtime` tests that had `cargo-test` blocking every PR (#6957).

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

@@ -351,29 +351,86 @@ unsafe fn object_keys_array_ptr(user_ptr: usize) -> usize {
     (*(user_ptr as *const crate::object::ObjectHeader)).keys_array as usize
 }
 
-/// The shared canonical descriptor for `user_ptr`'s shape, if shape-keying is
-/// on, the object carries a keys_array, and the shape is unambiguous (`Some`).
+/// Borrow the shared canonical descriptor for `user_ptr`'s shape, if
+/// shape-keying is on, the object carries a keys_array, and the shape is
+/// unambiguous (`Some`). Runs `f` against the descriptor in place — the GC
+/// trace path and the store fast path both consult it per object/per store, and
+/// a `Heap` mask would allocate a `Vec` on every clone.
 #[inline]
-unsafe fn shape_shared_descriptor(user_ptr: usize) -> Option<TypedLayoutDescriptor> {
+unsafe fn with_shape_shared_descriptor<R>(
+    user_ptr: usize,
+    f: impl Fn(&TypedLayoutDescriptor) -> R,
+) -> Option<R> {
     if !shape_layout_keyed_enabled() {
         return None;
     }
     let keys = object_keys_array_ptr(user_ptr);
     if keys == 0 {
         return None;
     }
-    let desc = SHAPE_LAYOUTS.with(|m| m.borrow().get(&keys).and_then(|e| e.clone()))?;
     // Defense-in-depth: the descriptor's `slot_count` is pinned to the owning
     // object's `field_count` at install (`init_typed_shape_layout` rejects a
     // mismatch). A differing current field_count means this object's shape is
     // not the one the descriptor describes — e.g. a keys_array address reused by
     // a shape with a different field count (moving-GC relocation before the new
     // address is re-installed). Fall back (per-object → conservative).
     let field_count = (*(user_ptr as *const crate::object::ObjectHeader)).field_count as usize;
-    if desc.slot_count != field_count {
+    SHAPE_LAYOUTS.with(|m| {
+        let map = m.borrow();
+        let desc = map.get(&keys)?.as_ref()?;
+        if desc.slot_count != field_count {
+            return None;
+        }
+        Some(f(desc))
+    })
+}
+
+/// Cloning form of [`with_shape_shared_descriptor`], for the callers that need
+/// to keep the descriptor past the `SHAPE_LAYOUTS` borrow.
+#[inline]
+unsafe fn shape_shared_descriptor(user_ptr: usize) -> Option<TypedLayoutDescriptor> {
+    with_shape_shared_descriptor(user_ptr, |desc| desc.clone())
+}
+
+/// Answer a *query* about `user_ptr`'s current canonical typed layout, whichever
+/// map holds it: the per-object `TYPED_LAYOUTS` entry (objects that diverged
+/// from their shape, or carry no keys_array), else — and only while the object
+/// is still `GC_OBJ_TYPED_LAYOUT_INTACT` — the shape-shared `SHAPE_LAYOUTS`
+/// entry.
+///
+/// #6957: #6893 moved the descriptor of every *shape-keyed* object (i.e. every
+/// class instance — it carries a shared `keys_array`) out of `TYPED_LAYOUTS` and
+/// **deleted the per-object entry**. It taught `layout_note_slot`,
+/// `layout_visit_pointer_slots` and `heap_payload_slot_selection`'s mask lookup
+/// about the new home but not the query helpers below, so every one of them
+/// started reporting "no typed descriptor" for real class instances — silently
+/// deopting every typed guard that consults them. The existing layout tests all
+/// allocate with `js_object_alloc` (class 0, no keys_array), which still takes
+/// the per-object path, so nothing caught it.
+///
+/// The INTACT gate on the shared half is load-bearing.
+/// `layout_set_typed_unknown` downgrades exactly ONE object (a store that
+/// contradicts the descriptor) by clearing its intact bit and dropping its
+/// per-object entry; it cannot drop the `SHAPE_LAYOUTS` entry, which still
+/// correctly describes every sibling that has *not* diverged. Reading the shared
+/// descriptor without the bit would therefore keep reporting the pre-downgrade
+/// layout for the very object that just invalidated it.
+///
+/// The per-object half stays ungated, so this remains an independent check on a
+/// forged/stale intact header bit (see
+/// [`layout_typed_accepts_finite_number_slot_for_user`]).
+#[inline]
+fn with_typed_descriptor_for_query<R>(
+    user_ptr: usize,
+    f: impl Fn(&TypedLayoutDescriptor) -> R,
+) -> Option<R> {
+    if let Some(result) = TYPED_LAYOUTS.with(|m| m.borrow().get(&user_ptr).map(&f)) {
+        return Some(result);
+    }
+    if !layout_typed_intact_for_user(user_ptr) {
         return None;
     }
-    Some(desc)
+    unsafe { with_shape_shared_descriptor(user_ptr, f) }
 }
 
 /// Trace-path helper: pointer mask for a SIDE_MASK object with no per-object
@@ -739,11 +796,22 @@ pub(crate) fn layout_slot_is_raw_f64_typed(parent_user: usize, slot_index: usize
         if (*header)._reserved & GC_OBJ_TYPED_LAYOUT_INTACT == 0 {
             return false;
         }
-        TYPED_LAYOUTS.with(|m| {
-            m.borrow().get(&parent_user).is_some_and(|typed| {
-                slot_index < typed.slot_count && typed.raw_f64_mask.contains_slot(slot_index)
+        // #6893/#6957: per-object descriptor (diverged objects, and objects with
+        // no keys_array) OR the shared shape descriptor — exactly as
+        // `layout_note_slot` resolves it, which is the agreement this helper
+        // documents.
+        TYPED_LAYOUTS
+            .with(|m| {
+                m.borrow().get(&parent_user).map(|typed| {
+                    slot_index < typed.slot_count && typed.raw_f64_mask.contains_slot(slot_index)
+                })
+            })
+            .or_else(|| {
+                with_shape_shared_descriptor(parent_user, |typed| {
+                    slot_index < typed.slot_count && typed.raw_f64_mask.contains_slot(slot_index)
+                })
             })
-        })
+            .unwrap_or(false)
     }
 }
 
@@ -1155,14 +1223,10 @@ pub(crate) fn layout_typed_intact_for_user(user_ptr: usize) -> bool {
 }
 
 pub(crate) fn layout_typed_raw_f64_slot_for_user(user_ptr: usize, slot_index: usize) -> bool {
-    TYPED_LAYOUTS.with(|m| {
-        m.borrow()
-            .get(&user_ptr)
-            .map(|layout| {
-                slot_index < layout.slot_count && layout.raw_f64_mask.contains_slot(slot_index)
-            })
-            .unwrap_or(false)
+    with_typed_descriptor_for_query(user_ptr, |layout| {
+        slot_index < layout.slot_count && layout.raw_f64_mask.contains_slot(slot_index)
     })
+    .unwrap_or(false)
 }
 
 /// Validate that an intact typed descriptor contains `slot_index`.
@@ -1177,23 +1241,16 @@ pub(crate) fn layout_typed_accepts_finite_number_slot_for_user(
     user_ptr: usize,
     slot_index: usize,
 ) -> bool {
-    TYPED_LAYOUTS.with(|m| {
-        m.borrow()
-            .get(&user_ptr)
-            .is_some_and(|layout| slot_index < layout.slot_count)
-    })
+    with_typed_descriptor_for_query(user_ptr, |layout| slot_index < layout.slot_count)
+        .unwrap_or(false)
 }
 
 fn layout_typed_raw_f64_slot_count_for_user(user_ptr: usize, slot_count: usize) -> usize {
-    TYPED_LAYOUTS.with(|m| {
-        m.borrow()
-            .get(&user_ptr)
-            .map(|layout| {
-                let bounded_count = slot_count.min(layout.slot_count);
-                layout.raw_f64_mask.count_slots(bounded_count)
-            })
-            .unwrap_or(0)
+    with_typed_descriptor_for_query(user_ptr, |layout| {
+        let bounded_count = slot_count.min(layout.slot_count);
+        layout.raw_f64_mask.count_slots(bounded_count)
     })
+    .unwrap_or(0)
 }
 
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]

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

@@ -464,6 +464,98 @@ fn test_typed_shape_descriptor_tracks_raw_numeric_slots() {
     clear_mark_seeds();
 }
 
+/// #6957 regression guard: the typed descriptor of a **shape-keyed** object must
+/// be visible to the layout query helpers.
+///
+/// #6893 keys the canonical descriptor by the shared `keys_array` (`SHAPE_LAYOUTS`)
+/// and deletes the per-object `TYPED_LAYOUTS` entry — so every class instance
+/// (the only objects that carry a keys_array) moved to the shared map. Every
+/// other test in this file allocates with `js_object_alloc` (class 0, no
+/// keys_array), which still takes the per-object path; that is precisely why the
+/// query helpers could go blind on real class instances with the whole layout
+/// suite green.
+#[test]
+fn test_typed_shape_descriptor_visible_for_shape_keyed_objects() {
+    clear_marks();
+    clear_mark_seeds();
+
+    let packed = b"x\0y\0";
+    let keys = crate::object::js_build_class_keys_array(
+        0x6957_01,
+        2,
+        packed.as_ptr(),
+        packed.len() as u32,
+    );
+    let first = crate::object::js_object_alloc_class_inline_keys(0x6957_01, 0, 2, keys);
+    let second = crate::object::js_object_alloc_class_inline_keys(0x6957_01, 0, 2, keys);
+    unsafe {
+        assert_eq!(
+            (*first).keys_array,
+            (*second).keys_array,
+            "same-shape objects must share one canonical keys array"
+        );
+    }
+
+    let raw_mask = [0b01u64];
+    for object in [first, second] {
+        crate::object::js_object_set_unboxed_f64_field(object, 0, 1.5);
+        crate::object::js_object_set_field(object, 1, crate::value::JSValue::number(2.5));
+        js_gc_init_typed_shape_layout(
+            object as u64,
+            2,
+            raw_mask.as_ptr(),
+            raw_mask.len() as u32,
+            std::ptr::null(),
+            0,
+        );
+    }
+
+    for object in [first, second] {
+        let user = object as usize;
+        assert!(
+            layout_typed_intact_for_user(user),
+            "the shared shape install must set the intact bit"
+        );
+        assert!(
+            layout_typed_raw_f64_slot_for_user(user, 0),
+            "slot 0 is raw-f64 in the shape descriptor"
+        );
+        assert!(!layout_typed_raw_f64_slot_for_user(user, 1));
+        assert!(
+            layout_slot_is_raw_f64_typed(user, 0),
+            "the store fast path must agree with layout_note_slot's own resolution"
+        );
+        assert!(
+            layout_typed_accepts_finite_number_slot_for_user(user, 1),
+            "an ordinary JSValue slot of an intact descriptor accepts finite numbers"
+        );
+    }
+
+    // A contradicting store downgrades ONLY the object that made it. The shared
+    // entry cannot be removed (it still describes every sibling), so the intact
+    // bit is what separates the two — assert both halves.
+    let payload = crate::string::js_string_from_bytes(b"boxed".as_ptr(), 5);
+    crate::object::js_object_set_field(first, 0, crate::value::JSValue::string_ptr(payload));
+
+    assert!(
+        !layout_typed_raw_f64_slot_for_user(first as usize, 0),
+        "a boxed store into a raw-f64 slot must evict this object's descriptor"
+    );
+    assert!(!layout_slot_is_raw_f64_typed(first as usize, 0));
+    assert!(
+        !layout_typed_accepts_finite_number_slot_for_user(first as usize, 0),
+        "a downgraded object must not keep reading its shape's stale descriptor"
+    );
+    assert!(
+        layout_typed_raw_f64_slot_for_user(second as usize, 0),
+        "the sibling never diverged and must keep the shared shape descriptor"
+    );
+    assert!(layout_slot_is_raw_f64_typed(second as usize, 0));
+
+    clear_marks();
+    clear_mark_seeds();
+}
+
 #[test]
 fn test_typed_shape_raw_numeric_slots_accept_pointer_like_f64_bits() {
     clear_marks();

crates/perry-runtime/src/typed_feedback.rs

@@ -51,7 +51,29 @@ pub(crate) fn typed_feedback_active() -> bool {
 }
 
 #[cfg(test)]
-pub(crate) static TYPED_FEEDBACK_TEST_LOCK: LazyLock<Mutex<()>> = LazyLock::new(|| Mutex::new(()));
+static TYPED_FEEDBACK_TEST_LOCK: LazyLock<Mutex<()>> = LazyLock::new(|| Mutex::new(()));
+
+/// Serializes the typed-feedback unit tests, which all drive the one
+/// process-global site registry.
+///
+/// Recovers from poisoning **on purpose**. The mutex guards no invariant of its
+/// own: every test re-initializes the shared state with
+/// `reset_typed_feedback_for_tests()` as its next statement, so a lock left
+/// poisoned by an earlier test's assertion failure is still perfectly usable.
+///
+/// #6957: with a plain `.unwrap()`, the first genuine failure in the module
+/// poisoned the lock and turned all 31 subsequent tests into `PoisonError`
+/// panics. That reported two real regressions as 33 red tests, hid which two
+/// were real, and made the module's result depend on `--test-threads` (32 red
+/// serially, 33 in parallel — purely a function of how many tests ran *after*
+/// the poisoning one). Recovering keeps a failure count equal to the number of
+/// actual failures.
+#[cfg(test)]
+pub(crate) fn typed_feedback_test_lock() -> std::sync::MutexGuard<'static, ()> {
+    TYPED_FEEDBACK_TEST_LOCK
+        .lock()
+        .unwrap_or_else(|poisoned| poisoned.into_inner())
+}
 
 #[repr(u32)]
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]