diff --git a/changelog.d/8140-buffer-receiver-array-iterators.md b/changelog.d/8140-buffer-receiver-array-iterators.md new file mode 100644 index 0000000000..6ad6b33410 --- /dev/null +++ b/changelog.d/8140-buffer-receiver-array-iterators.md @@ -0,0 +1,16 @@ +### Fixed + +- `.values()` / `.keys()` / `.entries()` on a Buffer-backed `Uint8Array` no + longer yield an empty iterator when codegen cannot statically prove the + receiver (`holder.u.values()`, or any fully dynamic receiver). + `array_iter_obj_raw` opens with `clean_arr_ptr`, which #8041 widened to reject + every *tracked* non-array; `buffer_alloc` stamps a real `GC_TYPE_BUFFER` + header, so a Buffer receiver was nulled exactly as a `GC_TYPE_TYPED_ARRAY` one + is and every branch below the funnel became unreachable. `keys` is the proof + this was a regression rather than a standing gap — it only reads `length`, so + it answered correctly before #8041. The receiver is now resolved in + `typed_array_iter_arr`, above that funnel, and receiver-tag gated so an + ordinary array reaches neither registry — strictly fewer probes than before, + which asked `lookup_typed_array_kind` unconditionally. `ArrayBuffer` / + `SharedArrayBuffer` / `DataView` are excluded, matching node's `TypeError` + (#8117). diff --git a/crates/perry-runtime/src/array/iter_object.rs b/crates/perry-runtime/src/array/iter_object.rs index 083c70a172..7a41f76b48 100644 --- a/crates/perry-runtime/src/array/iter_object.rs +++ b/crates/perry-runtime/src/array/iter_object.rs @@ -197,18 +197,58 @@ unsafe fn array_iter_obj_raw(arr: *const ArrayHeader, kind: i32) -> i64 { js_nanbox_get_pointer(nanboxed) } -/// #3148: materialize a TypedArray receiver to a plain Array (element-typed -/// reads) before building the iterator object, so `int32arr.values()` / -/// `.keys()` / `.entries()` yield the numeric elements rather than the raw -/// byte buffer reinterpreted as f64. +/// #3148/#8140: materialize a %TypedArray% *or* Buffer-backed `Uint8Array` +/// receiver to a plain Array (element-typed reads) before building the iterator +/// object, so `int32arr.values()` / `.keys()` / `.entries()` yield the numeric +/// elements rather than the raw byte buffer reinterpreted as f64. +/// +/// **This MUST stay above `array_iter_obj_raw`**, which opens with +/// `clean_arr_ptr`. Since #8041 that funnel returns null for every *tracked* +/// non-`GC_TYPE_ARRAY` allocation, and `buffer_alloc` stamps `GC_TYPE_BUFFER` +/// through `arena_alloc_gc_old` — so a Buffer receiver is nulled exactly as a +/// `GC_TYPE_TYPED_ARRAY` one is, and every branch below that funnel (including +/// its own Map/Set arm) is unreachable for it. The observable was an EMPTY +/// iterator: `u8.values()`, `.keys()` and `.entries()` all yielded nothing. +/// `keys` in particular was *correct* before #8041 — it only ever reads +/// `length` — so this is a regression, not a standing gap. +/// +/// Perry's `new Uint8Array([…])` is a `BufferHeader`, not a +/// `TypedArrayHeader` (`buffer::js_uint8array_new`), so it is absent from the +/// typed-array registry and `lookup_typed_array_kind` never answers for it. +/// That is the same registry gap `buffer_receiver_as_uint8_typed_array` +/// documents for `sort`/`with`/`toSorted`/`toReversed`; here the plain-array +/// materialization the typed-array arm already performs is the natural answer, +/// so no `TypedArrayHeader` copy is needed. +/// +/// `ArrayBuffer` / `SharedArrayBuffer` / `DataView` are deliberately excluded: +/// none has `%TypedArray%.prototype`, so node throws +/// `… is not a function` rather than answering elements, and routing them here +/// would invent an iterator node does not have. +/// +/// Receiver-tag gated with the #7765 idiom (`js_array_get_f64`, and #8130's +/// `collection_foreach_reroute`): an ordinary array is excluded by one +/// already-warm GC-header byte and reaches NEITHER registry — strictly fewer +/// probes than before this change, which asked `lookup_typed_array_kind` +/// unconditionally. The registries remain the layout proof for everything else. #[inline] fn typed_array_iter_arr(arr: *const ArrayHeader) -> *const ArrayHeader { - if crate::typedarray::lookup_typed_array_kind(arr as usize).is_some() { - crate::typedarray::typed_array_to_array(arr as *const crate::typedarray::TypedArrayHeader) - as *const ArrayHeader - } else { - arr + if crate::array::array_receiver_gc_tag(arr).0 == crate::gc::GC_TYPE_ARRAY { + return arr; } + let addr = arr as usize; + if crate::typedarray::lookup_typed_array_kind(addr).is_some() { + return crate::typedarray::typed_array_to_array( + arr as *const crate::typedarray::TypedArrayHeader, + ) as *const ArrayHeader; + } + if crate::buffer::is_registered_buffer(addr) + && !crate::buffer::is_any_array_buffer(addr) + && !crate::buffer::is_data_view(addr) + { + return crate::buffer::buffer_to_array(addr as *const crate::buffer::BufferHeader) + as *const ArrayHeader; + } + arr } /// `%TypedArray%.prototype.values/keys/entries` begin with `ValidateTypedArray` diff --git a/crates/perry-runtime/src/array/typed_array_receiver_tests.rs b/crates/perry-runtime/src/array/typed_array_receiver_tests.rs index 4c7c64e4cd..08c2a18b9a 100644 --- a/crates/perry-runtime/src/array/typed_array_receiver_tests.rs +++ b/crates/perry-runtime/src/array/typed_array_receiver_tests.rs @@ -516,3 +516,194 @@ fn an_array_buffer_receiver_is_not_treated_as_a_uint8array() { "a DataView receiver must not be served as a Uint8Array" ); } + +// --------------------------------------------------------------------------- +// #8140: the `.values()` / `.keys()` / `.entries()` iterator entry points must +// resolve a Buffer-backed `Uint8Array` receiver BEFORE the array-only funnel. +// +// The precondition is already pinned by +// `a_new_uint8array_is_a_buffer_not_a_registry_typed_array` above: perry's +// `new Uint8Array([…])` is a `BufferHeader`, absent from the typed-array +// registry (so the #3148 `lookup_typed_array_kind` delegations never answer for +// it) and nulled by `clean_arr_ptr` (so a post-clean branch is unreachable). +// `buffer_alloc` stamps a real `GC_TYPE_BUFFER` GcHeader through +// `arena_alloc_gc_old`, which is why #8041's "reject every TRACKED non-array" +// catches it and its predecessor "reject GC_TYPE_OBJECT / GC_TYPE_CLOSURE" +// did not. +// +// `array_iter_obj_raw` opens with that funnel, so EVERY branch below it — the +// fs-dir arm, the Map/Set arm, and the iterator construction itself — was +// unreachable for a Buffer receiver, and all three methods yielded an EMPTY +// iterator. Measured against node v26.5.1 on `new Uint8Array([3,1,2])`: +// +// method node perry pre-fix +// .values() [3,1,2] [] +// .keys() [0,1,2] [] +// .entries() [[0,3],[1,1],[2,2]] [] +// +// `keys` is the sharpest evidence this is a REGRESSION and not a standing gap: +// it only ever reads `length`, so it answered correctly before #8041. +// +// Reachable from BOTH a property-read receiver (`holder.u.values()`, which +// codegen fuses to `js_array_values_iter_obj`) and a fully dynamic one +// (`opaque(u).values()`), so it is not a narrow static-typing corner. +// --------------------------------------------------------------------------- + +/// Drive an iterator object returned by `js_array_*_iter_obj` to exhaustion, +/// rendering each yielded value so the expectations below read as the node +/// output they were taken from. +fn drain_iter(iter: i64) -> Vec { + let mut out = Vec::new(); + unsafe { + let obj = iter as *mut crate::object::ObjectHeader; + assert!(!obj.is_null(), "iter_obj must return an iterator object"); + for _ in 0..64 { + let result = crate::array::dispatch_array_iterator_method(obj, "next"); + let robj = + crate::value::js_nanbox_get_pointer(result) as *mut crate::object::ObjectHeader; + assert!(!robj.is_null(), "next() must return a result object"); + let value = crate::object::js_object_get_field(robj, 0); + let done = crate::object::js_object_get_field(robj, 1); + if done.bits() == crate::value::TAG_TRUE { + break; + } + out.push(render(f64::from_bits(value.bits()))); + } + } + out +} + +/// `3` for a number, `[0,3]` for a 2-element pair array — enough to tell the +/// three iterator kinds apart, and to tell a correct element from a raw byte +/// reinterpreted as an f64 (which renders as `1.5e-323`, never as `3`). +fn render(v: f64) -> String { + let bits = v.to_bits(); + if (bits >> 48) == 0x7FFD { + let inner = crate::value::js_nanbox_get_pointer(v) as *const ArrayHeader; + let cleaned = crate::array::header::clean_arr_ptr(inner); + if !cleaned.is_null() { + let n = unsafe { (*cleaned).length } as usize; + let parts: Vec = (0..n) + .map(|i| render(crate::array::js_array_get_f64(cleaned, i as u32))) + .collect(); + return format!("[{}]", parts.join(",")); + } + } + let n = f64::from_bits(bits); + if n.is_finite() && n.fract() == 0.0 { + format!("{}", n as i64) + } else { + format!("{n:?}") + } +} + +#[test] +fn js_array_values_iter_obj_yields_a_buffer_receivers_bytes() { + let _serialized = crate::array::test_serialize(); + let buf = uint8_buffer(&[3.0, 1.0, 2.0]); + assert_eq!( + drain_iter(crate::array::js_array_values_iter_obj(buf)), + vec!["3", "1", "2"], + "node yields [3,1,2]; an EMPTY list is #8140 — the funnel nulled the \ + receiver before the Buffer question was ever asked" + ); +} + +#[test] +fn js_array_keys_iter_obj_yields_a_buffer_receivers_indices() { + let _serialized = crate::array::test_serialize(); + let buf = uint8_buffer(&[3.0, 1.0, 2.0]); + assert_eq!( + drain_iter(crate::array::js_array_keys_iter_obj(buf)), + vec!["0", "1", "2"], + "node yields [0,1,2]. `keys` reads only `length`, so it was CORRECT \ + before #8041 — this case is the proof that #8140 is a regression" + ); +} + +#[test] +fn js_array_entries_iter_obj_yields_a_buffer_receivers_pairs() { + let _serialized = crate::array::test_serialize(); + let buf = uint8_buffer(&[3.0, 1.0, 2.0]); + assert_eq!( + drain_iter(crate::array::js_array_entries_iter_obj(buf)), + vec!["[0,3]", "[1,1]", "[2,2]"], + "node yields [[0,3],[1,1],[2,2]]" + ); +} + +#[test] +fn a_typed_array_receiver_still_yields_element_typed_values() { + let _serialized = crate::array::test_serialize(); + // The pre-existing #3148 arm must survive the reordering. `70000 & 0xFFFF` + // is 4464, so a raw-f64 reinterpretation cannot produce this answer. + let ta = typed(UINT16, &[70000.0, 2.0]); + assert_eq!( + drain_iter(crate::array::js_array_values_iter_obj(as_array(ta))), + vec!["4464", "2"], + "the typed-array materialization must still run element-typed reads" + ); +} + +#[test] +fn a_plain_array_iterator_never_probes_the_typed_array_registry() { + let _serialized = crate::array::test_serialize(); + let mut arr = js_array_alloc(3); + for v in [7.0, 8.0, 9.0] { + arr = js_array_push_f64(arr, v); + } + + // Prime anything built lazily on first touch, then measure ONLY the + // receiver-resolution call. `drain_iter`/`render` read elements through + // `js_array_get_f64`, which runs its own #8109 typed-array probe once per + // element — measuring across the drain counts those and says nothing about + // the subject. (Measured: it reports exactly +1 per element, so a naive + // window would have "failed" here for the wrong reason.) + let _ = drain_iter(crate::array::js_array_values_iter_obj(arr)); + let before = crate::typedarray::test_typed_array_registry_probe_count(); + let iter = crate::array::js_array_values_iter_obj(arr); + let after = crate::typedarray::test_typed_array_registry_probe_count(); + + assert_eq!( + drain_iter(iter), + vec!["7", "8", "9"], + "the control receiver must keep iterating its own elements" + ); + assert_eq!( + after, before, + "a GC_TYPE_ARRAY receiver must never reach lookup_typed_array_kind — \ + delete the receiver-tag gate at the top of `typed_array_iter_arr` and \ + this is what fails, even though the ANSWER above stays correct" + ); +} + +#[test] +fn an_array_buffer_or_data_view_receiver_gets_no_element_iterator() { + let _serialized = crate::array::test_serialize(); + // `ArrayBuffer` / `SharedArrayBuffer` / `DataView` have no + // %TypedArray%.prototype, so node throws `… is not a function` rather than + // answering elements. The Buffer arm must decline them and leave the + // pre-existing behaviour untouched, exactly as + // `buffer_receiver_as_uint8_typed_array` does. + let ab = crate::buffer::buffer_alloc(4); + unsafe { (*ab).length = 4 }; + crate::buffer::mark_as_array_buffer(ab as usize); + assert!( + drain_iter(crate::array::js_array_values_iter_obj( + ab as *mut ArrayHeader + )) + .is_empty(), + "an ArrayBuffer receiver must not be served a Uint8Array iterator" + ); + + let dv = crate::buffer::buffer_alloc(4); + unsafe { (*dv).length = 4 }; + crate::buffer::mark_as_data_view(dv as usize); + assert!( + drain_iter(crate::array::js_array_values_iter_obj( + dv as *mut ArrayHeader + )) + .is_empty(), + "a DataView receiver must not be served a Uint8Array iterator" + ); +} diff --git a/crates/perry-runtime/src/typedarray/mod.rs b/crates/perry-runtime/src/typedarray/mod.rs index 4be5650b7d..0d2b952d4a 100644 --- a/crates/perry-runtime/src/typedarray/mod.rs +++ b/crates/perry-runtime/src/typedarray/mod.rs @@ -350,10 +350,31 @@ pub fn unregister_typed_array(ptr: *const TypedArrayHeader) { crate::typedarray_props::typed_array_clear_no_extend(owner); } +#[cfg(test)] +thread_local! { +/// Every entry into [`lookup_typed_array_kind`], i.e. every caller that could +/// not rule a typed array out more cheaply. Mirrors +/// `map::TEST_MAP_REGISTRY_PROBES` (#7765) and exists for the same reason: the +/// receiver-tag gates that let a `GC_TYPE_ARRAY` receiver skip this probe are +/// asserted against it, so deleting a gate fails a test even though the ANSWER +/// stays correct. A fast path nobody can prove ran is not a fast path. +/// +/// Per THREAD, not per process: the registry is thread-local and `cargo test` +/// runs each case on its own thread in one process. + static TEST_TA_REGISTRY_PROBES: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn test_typed_array_registry_probe_count() -> u64 { + TEST_TA_REGISTRY_PROBES.with(|c| c.get()) +} + /// Returns Some(kind) if the (already-stripped) address is a registered /// typed array, else None. #[inline] pub fn lookup_typed_array_kind(addr: usize) -> Option { + #[cfg(test)] + TEST_TA_REGISTRY_PROBES.with(|c| c.set(c.get().wrapping_add(1))); // Nothing has ever been registered ⟹ nothing to find. Checked ahead of the // #5525 cache because it is the only arm that costs neither a cache-slot // load nor a negative-entry write-back: a program with no typed arrays runs