fix(runtime): root call-argument lists across the allocations that can move them

changelog.d/10587-argument-list-roots.md

@@ -0,0 +1,14 @@
+Fixed three GC rooting gaps in #10532's dynamic-call argument-list handling:
+`Reflect.apply` held the callee, receiver and arguments in plain Rust locals
+across the one closure-rebind shape that allocates (a concise/object-literal
+method's `this` clone); `CreateListFromArrayLike`'s array-like path reused a
+raw source-object pointer read once before its per-index allocating loop
+instead of re-deriving it after each allocation; and a `(...fixed, ...rest)`
+body that also synthesizes `arguments` could hand its callee the rest
+array's pre-move address once the `arguments` array's allocation moved it. A
+new `rebind_explicit_this_allocates` predicate lets `Reflect.apply`'s common
+(non-cloning) path stay allocation-free and unrooted; only the one shape that
+actually allocates pays for rooting, so the fix measures ~0% instead of the
++38.8% an earlier, unconditionally-rooted attempt cost. New regression tests
+arm a named collection point at each fixed allocation and assert the callee
+observes post-collection addresses.

crates/perry-runtime/src/closure/dispatch.rs

@@ -22,7 +22,7 @@ mod value_call;
 
 pub(crate) use bound::{
     bound_function_lazy_name, bound_method_source_func_ptr, coerce_call_this, rebind_explicit_this,
-    reify_function_method_value,
+    rebind_explicit_this_allocates, reify_function_method_value,
 };
 pub use bound::{dispatch_bound_function, dispatch_bound_method, js_function_bind};
 

crates/perry-runtime/src/closure/dispatch/bound.rs

@@ -412,6 +412,37 @@ pub(crate) fn coerce_call_this(target: f64, this_arg: f64) -> f64 {
 ///   - bound functions and non-closure values
 ///     (`clone_closure_rebind_this` no-ops on these — it only rewrites a
 ///     `CAPTURES_THIS` slot).
+/// Does [`rebind_explicit_this`] ALLOCATE for this target?
+///
+/// Only the clone does, and it happens for one shape: a non-arrow closure that
+/// captures `this` in a reserved slot and may be re-bound. Everything else —
+/// a plain function, an arrow, a bound function, a non-closure value — comes
+/// back unchanged, allocation-free. A caller that has GC values in Rust locals
+/// can therefore skip rooting them entirely for the common callee, and pay for
+/// handles only on the shape that can collect underneath it.
+///
+/// Kept in step with `clone_closure_rebind_this`'s early-outs by
+/// `rebind_predicate_tests` below, which asserts the two agree on every shape.
+#[inline]
+pub(crate) fn rebind_explicit_this_allocates(target: f64) -> bool {
+    let bits = target.to_bits();
+    if bits & 0xFFFF_0000_0000_0000 != 0x7FFD_0000_0000_0000 {
+        return false;
+    }
+    let ptr = (bits & 0x0000_FFFF_FFFF_FFFF) as usize;
+    if ptr < 0x100000 || !crate::closure::is_closure_ptr(ptr) {
+        return false;
+    }
+    let header = ptr as *const ClosureHeader;
+    if crate::closure::closure_is_arrow(header) {
+        return false;
+    }
+    let raw_count = unsafe { (*header).capture_count };
+    raw_count & CAPTURES_THIS_FLAG != 0
+        && raw_count & NO_THIS_REBIND_FLAG == 0
+        && crate::closure::real_capture_count(raw_count) > 0
+}
+
 #[inline]
 pub(crate) fn rebind_explicit_this(target: f64, this_arg: f64) -> f64 {
     let bits = target.to_bits();
@@ -761,3 +792,82 @@ pub(crate) unsafe fn reify_function_method_value(receiver: f64, method: &'static
     crate::gc::runtime_write_barrier_root_heap_word(closure as u64);
     f64::from_bits(crate::value::JSValue::pointer(closure as *mut u8).bits())
 }
+
+#[cfg(test)]
+mod rebind_predicate_tests {
+    use super::*;
+
+    // Distinct bodies on purpose: arrow-ness is registered per func_ptr, and
+    // two `extern "C"` bodies with identical machine code get folded to one
+    // address by the linker — which silently makes every case in this test the
+    // same closure body.
+    extern "C" fn arrow_probe(_closure: *const ClosureHeader) -> f64 {
+        1.0
+    }
+
+    extern "C" fn method_probe(_closure: *const ClosureHeader) -> f64 {
+        2.0
+    }
+
+    fn closure_value(body: *const u8, capture_count: u32) -> f64 {
+        let closure = crate::closure::js_closure_alloc(body, capture_count);
+        f64::from_bits(crate::value::JSValue::pointer(closure as *mut u8).bits())
+    }
+
+    /// The predicate exists to let `Reflect.apply` skip rooting when nothing
+    /// can collect, so it has to answer exactly the question
+    /// `rebind_explicit_this` answers with its clone: saying "no" where the
+    /// rebind allocates is a rooting hole, and saying "yes" everywhere is the
+    /// per-call cost it was written to avoid.
+    #[test]
+    fn the_predicate_agrees_with_what_the_rebind_actually_does() {
+        let receiver = f64::from_bits(crate::value::TAG_UNDEFINED);
+        let method_body = method_probe as *const u8;
+        let arrow_body = arrow_probe as *const u8;
+        crate::closure::js_register_closure_arrow_function(arrow_body);
+
+        // The one shape that clones, and the shapes that look like it but
+        // return the target untouched.
+        let method = closure_value(method_body, CAPTURES_THIS_FLAG | 1);
+        let cases = [
+            ("concise method with a reserved `this`", method, true),
+            (
+                "arrow with a captured this",
+                closure_value(arrow_body, CAPTURES_THIS_FLAG | 1),
+                false,
+            ),
+            (
+                "generator step closure (no rebind)",
+                closure_value(method_body, CAPTURES_THIS_FLAG | NO_THIS_REBIND_FLAG | 1),
+                false,
+            ),
+            (
+                "captures `this` but has no capture slots",
+                closure_value(method_body, CAPTURES_THIS_FLAG),
+                false,
+            ),
+            ("plain closure", closure_value(method_body, 0), false),
+            ("a plain number", 42.0, false),
+            (
+                "undefined",
+                f64::from_bits(crate::value::TAG_UNDEFINED),
+                false,
+            ),
+        ];
+        for (what, value, clones) in cases {
+            let rebound_differs =
+                rebind_explicit_this(value, receiver).to_bits() != value.to_bits();
+            assert_eq!(
+                rebound_differs,
+                clones,
+                "premise: {what} must {} clone",
+                if clones { "" } else { "not" }
+            );
+            assert_eq!(
+                rebind_explicit_this_allocates(value),
+                rebound_differs,
+                "{what}: the predicate and the rebind must agree"
+            );
+        }
+    }
+}

crates/perry-runtime/src/closure/mod.rs

@@ -40,23 +40,23 @@ pub(crate) fn closure_side_table_census() -> Vec<crate::gc::census::SideTableRow
     rows
 }
 pub use registry::{
-    build_rest_array, closure_arity, closure_is_arrow, closure_is_bound_method, closure_length,
-    dispatch_rest_bundled, dispatch_with_arity, is_registered_arrow_function,
-    is_registered_async_function, is_registered_async_generator_function,
-    is_registered_generator_function, is_registered_strict_function, js_register_closure_arity,
-    js_register_closure_arrow_function, js_register_closure_async_function,
-    js_register_closure_async_generator_function, js_register_closure_generator_function,
-    js_register_closure_length, js_register_closure_rest, js_register_closure_rest_and_arguments,
-    js_register_closure_strict_function, js_register_closure_synthetic_arguments,
-    js_register_closure_trusted_direct, lookup_closure_arity, lookup_closure_length,
-    lookup_closure_rest, lookup_closure_rest_full, real_capture_count, resolve_strategy,
-    DispatchStrategy, BOUND_FUNCTION_FUNC_PTR, BOUND_METHOD_FUNC_PTR, CAPTURES_THIS_FLAG,
-    CLOSURE_MAGIC, NO_THIS_REBIND_FLAG,
+    build_rest_array, build_rest_array_rooted, closure_arity, closure_is_arrow,
+    closure_is_bound_method, closure_length, dispatch_rest_bundled, dispatch_with_arity,
+    is_registered_arrow_function, is_registered_async_function,
+    is_registered_async_generator_function, is_registered_generator_function,
+    is_registered_strict_function, js_register_closure_arity, js_register_closure_arrow_function,
+    js_register_closure_async_function, js_register_closure_async_generator_function,
+    js_register_closure_generator_function, js_register_closure_length, js_register_closure_rest,
+    js_register_closure_rest_and_arguments, js_register_closure_strict_function,
+    js_register_closure_synthetic_arguments, js_register_closure_trusted_direct,
+    lookup_closure_arity, lookup_closure_length, lookup_closure_rest, lookup_closure_rest_full,
+    real_capture_count, resolve_strategy, DispatchStrategy, BOUND_FUNCTION_FUNC_PTR,
+    BOUND_METHOD_FUNC_PTR, CAPTURES_THIS_FLAG, CLOSURE_MAGIC, NO_THIS_REBIND_FLAG,
 };
 
 pub(crate) use dispatch::{
     bound_function_lazy_name, bound_method_source_func_ptr, coerce_call_this, rebind_explicit_this,
-    reify_function_method_value, reset_throw_not_callable_counter,
+    rebind_explicit_this_allocates, reify_function_method_value, reset_throw_not_callable_counter,
 };
 pub use dispatch::{
     clean_closure_ptr, dispatch_bound_function, dispatch_bound_method, get_valid_func_ptr,

crates/perry-runtime/src/closure/registry.rs

@@ -977,13 +977,21 @@ pub fn closure_length(closure: *const ClosureHeader) -> Option<u32> {
 #[inline(always)]
 pub unsafe fn build_rest_array(values: &[f64], arguments_object: bool) -> f64 {
     let scope = crate::gc::RuntimeHandleScope::new();
-    let value_handles: Vec<_> = values
-        .iter()
-        .map(|value| scope.root_nanbox_f64(*value))
-        .collect();
+    let value_handles = scope.root_nanbox_f64_slice(values);
+    build_rest_array_rooted(&value_handles, arguments_object)
+}
+
+/// [`build_rest_array`] for a caller that already holds its values in handles.
+/// The array allocation and every push can collect, so the values have to be
+/// read out of the handles anyway — a caller that has them keeps one rooting
+/// pass instead of two.
+pub unsafe fn build_rest_array_rooted(
+    values: &[crate::gc::RuntimeHandle<'_>],
+    arguments_object: bool,
+) -> f64 {
     let arr = crate::array::js_array_alloc(values.len() as u32);
     let mut cur = arr;
-    for handle in value_handles.iter() {
+    for handle in values.iter() {
         cur = crate::array::js_array_push_f64(cur, handle.get_nanbox_f64());
     }
     if arguments_object {
@@ -1021,19 +1029,30 @@ pub unsafe fn dispatch_rest_bundled(
         .map(|value| arg_scope.root_nanbox_f64(*value))
         .collect();
 
-    let rest_slice: &[f64] = if kind == RestDispatchKind::SyntheticArguments {
-        args
-    } else if provided > k {
-        &args[k..]
-    } else {
-        &[]
-    };
-    let rest_double = build_rest_array(rest_slice, kind == RestDispatchKind::SyntheticArguments);
-    let all_arguments_double = if kind == RestDispatchKind::UserRestAndArguments {
-        Some(build_rest_array(args, true))
+    // Both arrays are built from the arguments this scope already roots, so
+    // they read current values however many times the builder collects — and
+    // the second array's allocation can move the first, which the body is
+    // about to receive, so the first takes a handle too (#10532 review).
+    let rest_handles: &[crate::gc::RuntimeHandle<'_>] =
+        if kind == RestDispatchKind::SyntheticArguments {
+            &arg_handles
+        } else if provided > k {
+            &arg_handles[k..]
+        } else {
+            &[]
+        };
+    let rest_handle = arg_scope.root_nanbox_f64(build_rest_array_rooted(
+        rest_handles,
+        kind == RestDispatchKind::SyntheticArguments,
+    ));
+    crate::gc::collection_point("closure.rest_bundle.between_arrays");
+    let all_arguments_handle = if kind == RestDispatchKind::UserRestAndArguments {
+        Some(arg_scope.root_nanbox_f64(build_rest_array_rooted(&arg_handles, true)))
     } else {
         None
     };
+    let rest_double = rest_handle.get_nanbox_f64();
+    let all_arguments_double = all_arguments_handle.map(|handle| handle.get_nanbox_f64());
 
     // Read fixed args, padding with undefined when caller under-supplied.
     macro_rules! a {

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

@@ -0,0 +1,56 @@
+//! Named collection points for rooting regression tests.
+//!
+//! A runtime helper that holds a GC value in a Rust local across an allocation
+//! only goes wrong when a moving collection lands in that exact window, which
+//! an allocation-trigger test cannot aim at a specific allocation inside one
+//! call. A test arms a site by name; the next time the helper passes that site
+//! it runs a copying minor, so the test observes what the helper holds after a
+//! collection at precisely that point. The same one-shot shape as
+//! `set.rs`'s `test_force_next_set_helper_gc`, shared instead of repeated.
+//!
+//! Outside `cfg(test)` a collection point is an empty inline function.
+
+#[cfg(test)]
+crate::perry_thread_local! {
+    static ARMED_SITE: std::cell::Cell<Option<(&'static str, u32)>> = const { std::cell::Cell::new(None) };
+}
+
+/// Run one copying minor the next time `collection_point(site)` is reached on
+/// this thread.
+#[cfg(test)]
+pub(crate) fn arm_collection_point(site: &'static str) {
+    arm_collection_point_after(site, 0);
+}
+
+/// Like [`arm_collection_point`], but skips the first `skip` hits on `site`
+/// before firing on hit number `skip + 1`. A helper called once per loop
+/// iteration only exposes ONE named site, so this is how a test puts the
+/// forced collection on a LATER iteration than the first -- e.g. to check
+/// that a value already read (and copied into a plain, unrooted local) on an
+/// earlier iteration survives a collection triggered by a later one.
+#[cfg(test)]
+pub(crate) fn arm_collection_point_after(site: &'static str, skip: u32) {
+    ARMED_SITE.with(|armed| armed.set(Some((site, skip))));
+}
+
+#[cfg(test)]
+pub(crate) fn collection_point(site: &'static str) {
+    let hit = ARMED_SITE.with(|armed| match armed.get() {
+        Some((armed_site, 0)) if armed_site == site => {
+            armed.set(None);
+            true
+        }
+        Some((armed_site, remaining)) if armed_site == site => {
+            armed.set(Some((armed_site, remaining - 1)));
+            false
+        }
+        _ => false,
+    });
+    if hit {
+        let _ = super::gc_collect_minor();
+    }
+}
+
+#[cfg(not(test))]
+#[inline(always)]
+pub(crate) fn collection_point(_site: &'static str) {}

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

@@ -49,6 +49,10 @@ pub(crate) use policy::young_generation_holds_a_nursery;
 pub use policy::*;
 mod progress;
 pub use progress::*;
+mod collection_points;
+pub(crate) use collection_points::collection_point;
+#[cfg(test)]
+pub(crate) use collection_points::{arm_collection_point, arm_collection_point_after};
 mod heap_budget;
 pub(crate) use heap_budget::*;
 mod pressure;

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

@@ -3,6 +3,7 @@ use super::support::*;
 use std::cell::Cell;
 mod arraylike_callbacks;
 mod bound_method_builder;
+mod call_argument_lists;
 mod callback_scanners;
 mod fs_options_object;
 mod generator_attach_prototype;