diff --git a/changelog.d/6974-layout-transfer-shape-keyed-intact.md b/changelog.d/6974-layout-transfer-shape-keyed-intact.md new file mode 100644 index 0000000000..d8e568e302 --- /dev/null +++ b/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). diff --git a/crates/perry-runtime/src/gc/layout.rs b/crates/perry-runtime/src/gc/layout.rs index 8922e48e91..aff1978b64 100644 --- a/crates/perry-runtime/src/gc/layout.rs +++ b/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); diff --git a/crates/perry-runtime/src/gc/tests/layout_trace.rs b/crates/perry-runtime/src/gc/tests/layout_trace.rs index e568d067ba..1f52121ece 100644 --- a/crates/perry-runtime/src/gc/tests/layout_trace.rs +++ b/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::(); + std::ptr::copy_nonoverlapping( + src as *const u8, + dst as *mut u8, + header_size + 2 * std::mem::size_of::(), + ); + 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::()) 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();