diff --git a/changelog.d/8120-uint8array-specialized-dispatch.md b/changelog.d/8120-uint8array-specialized-dispatch.md new file mode 100644 index 0000000000..dda5211ab5 --- /dev/null +++ b/changelog.d/8120-uint8array-specialized-dispatch.md @@ -0,0 +1,74 @@ +### Fixed + +- **Reassigning a `Uint8Array` binding dropped element writes and read + `undefined` (#8111).** The `Uint8Array`-specialized twin of #8100. + `js_uint8array_get`, `js_uint8array_index_get_value` and `js_uint8array_set` + (`typedarray/access.rs`) are a SEPARATE emission path from the two helpers + #8109 fixed: codegen picks them from `is_uint8array_receiver` + (`perry-codegen/src/expr/index_{get,set}.rs`), which reads + `receiver_class_name` rather than the `local_type_hint` predicate #8100 is + about — but it fires for a reassigned `Uint8Array` local just the same. + + Each had a three-way shape (registered typed array of the right kind / + registered buffer / fall off the end) and TWO of those arms answered for a + receiver that is perfectly readable: the trailing arm (a plain array, object + or string) and the wrong-KIND arm (a registered typed array that is not + `Uint8Array` / `Uint8ClampedArray`). Reads answered `0` / `undefined`; the + store was dropped with no trace at all. Unlike #8100 these helpers were + memory-SAFE — they validated before dereferencing — they just answered + wrongly, silently, in the shipped default configuration. + + Measured against the pinned node oracle (`v26.5.1`, `.node-version`), both + arms exit 0: + + ``` + node perry before + store: [5,10] [9,10] <- write DROPPED + dyn: 10 undefined + read: 5 10 2 10 undefined undefined 2 10 + obj store+read: 42 8 undefined undefined + string read: h i undefined undefined + wrong-kind: 77 12 2 undefined undefined 2 + ``` + + Correct in BOTH arms: a real `Uint8Array` (`3 250 2`), a `Buffer` + (`200 2`), a plain array (`5 10`), a plain object (`5 10`), a + `Uint8ClampedArray` (`255 2`). `Q.length` and `Q.at(1)` were already right, so + the binding really did hold the plain array — only the specialized element + accessors were wrong. + + The read arms now delegate to `js_typed_array_get`, which owns #8109's + `classify_element_read_receiver` dispatch, so this path INHERITS it rather + than growing a third classifier — including the property that a + `GC_TYPE_TYPED_ARRAY` / `GC_TYPE_NATIVE_TYPED_VIEW` header still wins on a + registry miss, so a lookup failure can cost the diversion but never the + element read. The store arm asks the same classifier directly, because + `js_dyn_index_set` has its own return-value contract and the value arrives as + an `i32`. `js_uint8array_get`'s ABI is a byte-typed `i32`, so a recovered + value there goes through `ToNumber` rather than having its NaN-box bits + reinterpreted, and `undefined` still collapses to the `0` byte sentinel + (#6088). + + The wrong-KIND arms are REMOVED rather than kept as the issue suggested: + `js_typed_array_{get,set}` are kind-generic (they read `(*ta).kind`), node + reads and writes the real element there, and `js_typed_array_set` even + performs the spec's `ToBigInt` `TypeError` for a Number written into a bigint + view. `Uint8Array` / `Uint8ClampedArray` behaviour is unchanged and pinned by + two control tests (`300 -> 44` wrapping, `300 -> 255` clamping). + + RESIDUAL, documented in-code and not introduced here: codegen narrows + `js_uint8array_set`'s value to `i32` at the call site (`fptosi`, + `perry-codegen/src/expr/arrays_finds.rs`) because the DECLARED receiver is a + byte view, so a fractional value written through a reassigned binding arrives + already truncated — `Q[0] = 1.5` stores `1` where node stores `1.5`. Closing + that needs an f64-valued entry point plus a codegen change, not a + runtime-side dispatch. Every integer store is exact. + + Validation: the issue's probe is byte-identical to node post-fix. 7 unit tests + in `crates/perry-runtime/src/typedarray/element_read_receiver_tests.rs`, each + asserting the recovered VALUE rather than "did not panic"; + sabotage-verified after a real rebuild — reverting `access.rs` turns 4 of them + red, and the 3 that stay green are the intended controls. + `cargo test -p perry-runtime --lib`: 2380 passed / 0 failed / 4 ignored + (`main`: 2361 / 0 / 4). `perry-codegen`: 1434 passed / 11 failed, identical to + the `main` baseline (#8092). diff --git a/crates/perry-runtime/src/typedarray/access.rs b/crates/perry-runtime/src/typedarray/access.rs index a0e91b7d76..e68c103ab9 100644 --- a/crates/perry-runtime/src/typedarray/access.rs +++ b/crates/perry-runtime/src/typedarray/access.rs @@ -570,24 +570,36 @@ pub extern "C" fn js_typed_array_copy_within( ta } +/// `uint8array[i]` / `buffer[i]` read as a raw byte. See +/// [`js_uint8array_index_get_value`] for the JS-value twin and for the #8111 +/// stale-static-hint dispatch both share. #[no_mangle] pub extern "C" fn js_uint8array_get(target: *const TypedArrayHeader, index: i32) -> i32 { let addr = strip_nanbox(target as u64); if addr < 0x1000 || index < 0 { return 0; } - if let Some(kind) = lookup_typed_array_kind(addr) { - if !matches!(kind, KIND_UINT8 | KIND_UINT8_CLAMPED) { - return 0; - } - let value = js_typed_array_get(addr as *const TypedArrayHeader, index); - if value.to_bits() == crate::value::TAG_UNDEFINED { - 0 - } else { - value as i32 - } + let value = if lookup_typed_array_kind(addr).is_some() { + js_typed_array_get(addr as *const TypedArrayHeader, index) } else if crate::buffer::is_registered_buffer(addr) { - crate::buffer::js_buffer_get(addr as *const crate::buffer::BufferHeader, index) + return crate::buffer::js_buffer_get(addr as *const crate::buffer::BufferHeader, index); + } else { + // #8111: not a registered typed array and not a registered buffer, so + // the `Uint8Array` / `Buffer` static type codegen keyed on was a stale + // hint. `js_typed_array_get` performs #8109's + // `classify_element_read_receiver` dispatch on our behalf. + js_typed_array_get(addr as *const TypedArrayHeader, index) + }; + // This accessor's ABI is a byte-typed i32, so an `undefined` (absent + // receiver, or out of range) collapses to the `0` byte sentinel the caller + // expects (#6088), and a recovered non-number element takes ToNumber + // rather than having its NaN-box bits reinterpreted as an integer. + if value.to_bits() == crate::value::TAG_UNDEFINED { + return 0; + } + let n = jsvalue_to_f64(value); + if n.is_finite() { + n as i32 } else { 0 } @@ -601,6 +613,14 @@ pub extern "C" fn js_uint8array_get(target: *const TypedArrayHeader, index: i32) /// i32 accessor is forced to return (#6088). `js_typed_array_get` and /// `js_buffer_index_get_value` both already yield `undefined` for out-of-range, /// so each arm forwards directly. +/// +/// #8111: codegen picks this helper from `is_uint8array_receiver` +/// (`perry-codegen/src/expr/index_get.rs`), which reads `receiver_class_name` +/// and so still fires for a local DECLARED `Uint8Array` whose binding was +/// reassigned — the `Uint8Array`-specialized twin of the #8100 defect. Two +/// arms used to answer `undefined` for a receiver that is perfectly +/// readable: a registered typed array of the WRONG kind, and anything the +/// registry does not know at all. Both now dispatch. #[no_mangle] pub extern "C" fn js_uint8array_index_get_value( target: *const TypedArrayHeader, @@ -611,15 +631,18 @@ pub extern "C" fn js_uint8array_index_get_value( if addr < 0x1000 || index < 0 { return undefined; } - if let Some(kind) = lookup_typed_array_kind(addr) { - if !matches!(kind, KIND_UINT8 | KIND_UINT8_CLAMPED) { - return undefined; - } + if lookup_typed_array_kind(addr).is_some() { js_typed_array_get(addr as *const TypedArrayHeader, index) } else if crate::buffer::is_registered_buffer(addr) { crate::buffer::js_buffer_index_get_value(addr as *const crate::buffer::BufferHeader, index) } else { - undefined + // #8111: the `Uint8Array` static type was a stale hint — the binding + // was reassigned. Answering `undefined` here dropped the real element. + // `js_typed_array_get` owns #8109's `classify_element_read_receiver` + // dispatch, so this arm inherits it: header-wins-on-registry-miss for + // a real typed array, ordinary `[[Get]]` for a plain array / object / + // string, `undefined` for a masked-away non-pointer. + js_typed_array_get(addr as *const TypedArrayHeader, index) } } @@ -631,18 +654,53 @@ pub extern "C" fn js_uint8array_index_get_value( static KEEP_JS_UINT8ARRAY_INDEX_GET_VALUE: extern "C" fn(*const TypedArrayHeader, i32) -> f64 = js_uint8array_index_get_value; +/// `uint8array[i] = v` / `buffer[i] = v`. See +/// [`js_uint8array_index_get_value`] for the #8111 stale-static-hint dispatch +/// this shares, and the trailing arm below for the one residual its i32 value +/// ABI cannot close. #[no_mangle] pub extern "C" fn js_uint8array_set(target: *mut TypedArrayHeader, index: i32, value: i32) { let addr = strip_nanbox(target as u64); if addr < 0x1000 || index < 0 { return; } - if let Some(kind) = lookup_typed_array_kind(addr) { - if !matches!(kind, KIND_UINT8 | KIND_UINT8_CLAMPED) { - return; - } - js_typed_array_set(addr as *mut TypedArrayHeader, index, value as f64); + if lookup_typed_array_kind(addr).is_some() { + js_typed_array_set(addr as *mut TypedArrayHeader, index, f64::from(value)); } else if crate::buffer::is_registered_buffer(addr) { crate::buffer::js_buffer_set(addr as *mut crate::buffer::BufferHeader, index, value); + } else { + // #8111, store side. The read helpers above recover the element; a + // dropped STORE is the same defect one call earlier, and it is the + // more damaging half — `Q[0] = 5` left no trace at all. + // + // The receiver question is the one #8109 already answers, so ask it + // the same way rather than adding a third classifier. Only a + // POSITIVELY identified receiver is diverted: + // + // * `TypedArray` — a `GC_TYPE_TYPED_ARRAY` / `GC_TYPE_NATIVE_TYPED_ + // VIEW` header the registry missed; `js_typed_array_set` is + // kind-generic (it reads `(*ta).kind`). + // * `Ordinary` — a plain array, object, or anything else indexable. + // `js_dyn_index_set` is the `[[Set]]` this access would have taken + // if the stale static hint had never existed. + // * `Absent` — nothing indexable; drop the store, which is what a + // write through a non-object primitive does in sloppy mode. + // + // RESIDUAL (pre-existing, not introduced here): codegen narrows this + // helper's `value` to i32 at the call site (`fptosi`, perry-codegen + // `expr/arrays_finds.rs`'s `Uint8ArraySet` arm) because the declared + // receiver is a byte view. A FRACTIONAL value written through a + // reassigned binding therefore arrives already truncated, so + // `Q[0] = 1.5` stores `1` where node stores `1.5`. Closing that needs + // an f64-valued entry point, not a runtime-side dispatch. + match crate::typedarray::classify_element_read_receiver(target as u64) { + ElementReadReceiver::TypedArray(ta) => { + js_typed_array_set(ta as *mut TypedArrayHeader, index, f64::from(value)); + } + ElementReadReceiver::Ordinary(receiver) => { + crate::value::js_dyn_index_set(receiver, f64::from(index), f64::from(value)); + } + ElementReadReceiver::Absent => {} + } } } diff --git a/crates/perry-runtime/src/typedarray/element_read_receiver_tests.rs b/crates/perry-runtime/src/typedarray/element_read_receiver_tests.rs index d3322b959c..e01ae96141 100644 --- a/crates/perry-runtime/src/typedarray/element_read_receiver_tests.rs +++ b/crates/perry-runtime/src/typedarray/element_read_receiver_tests.rs @@ -219,3 +219,143 @@ fn classify_element_read_receiver_rejects_garbage_bits() { ElementReadReceiver::Absent )); } + +// -------------------------------------------------------------------------- +// #8111: the `Uint8Array`-specialized twin of the same defect. +// +// `js_uint8array_get` / `js_uint8array_index_get_value` / `js_uint8array_set` +// are a SEPARATE emission path: codegen picks them from +// `is_uint8array_receiver` (`perry-codegen/src/expr/index_{get,set}.rs`), +// which keys on `receiver_class_name` rather than the `local_type_hint` +// predicate #8100 is about — but it fires for a reassigned `Uint8Array` local +// just the same. Each helper had a three-way shape (registered typed array of +// the right kind / registered buffer / fall off the end) and TWO of those +// arms answered for a receiver that is perfectly readable: +// +// * the trailing arm — a plain array or object — answered `0` / +// `undefined` / dropped the store; +// * the wrong-KIND arm — a registered typed array that is not +// `Uint8Array` / `Uint8ClampedArray` — did the same, although +// `js_typed_array_get` / `js_typed_array_set` are kind-generic and node +// reads and writes the real element there. +// +// The store half matters most: `Q[0] = 5` left no trace at all. +// +// Each test asserts the RECOVERED VALUE, never merely "did not panic". The +// pre-fix code answers `0` / `undefined` / no-op for every one of them, so +// none can pass against the old body. +// -------------------------------------------------------------------------- + +use crate::typedarray::access::{ + js_uint8array_get, js_uint8array_index_get_value, js_uint8array_set, +}; + +/// A plain array handed to a `Uint8Array`-specialized helper, exactly as +/// codegen emits it (`unbox_to_i64` masks the NaN-box tag off). +fn as_u8(arr: *mut ArrayHeader) -> *const TypedArrayHeader { + as_typed(arr) +} + +/// A registered typed array with the NaN-box tag masked off, the shape every +/// one of these helpers is actually handed. +fn as_recv(ta: *mut TypedArrayHeader) -> *const TypedArrayHeader { + ((ta as u64) & POINTER_MASK) as *const TypedArrayHeader +} + +#[test] +fn js_uint8array_index_get_value_reads_a_plain_array_receiver() { + let _serialized = crate::array::test_serialize(); + let arr = plain_array(&[9.0, 10.0]); + assert_eq!(js_uint8array_index_get_value(as_u8(arr), 0), 9.0); + assert_eq!(js_uint8array_index_get_value(as_u8(arr), 1), 10.0); + // Out of range is `undefined`, the IntegerIndexedExotic answer node + // prints — not the `0` byte sentinel. + assert!(is_undefined(js_uint8array_index_get_value(as_u8(arr), 2))); +} + +#[test] +fn js_uint8array_set_stores_into_a_plain_array_receiver() { + let _serialized = crate::array::test_serialize(); + let arr = plain_array(&[9.0, 10.0]); + js_uint8array_set(as_u8(arr) as *mut TypedArrayHeader, 0, 5); + assert_eq!( + js_uint8array_index_get_value(as_u8(arr), 0), + 5.0, + "the store must land in the plain array — it was silently dropped" + ); + // And it is visible through the ordinary array accessor too, i.e. it is a + // real `[[Set]]` and not a shadow write somewhere else. + assert_eq!(crate::array::js_array_get_element(arr as i64, 0), 5.0); + assert_eq!(crate::array::js_array_get_element(arr as i64, 1), 10.0); +} + +#[test] +fn js_uint8array_get_reads_a_plain_array_receiver_as_a_byte() { + let _serialized = crate::array::test_serialize(); + let arr = plain_array(&[9.0, 10.0]); + assert_eq!(js_uint8array_get(as_u8(arr), 0), 9); + assert_eq!(js_uint8array_get(as_u8(arr), 1), 10); + // This accessor's ABI is a byte-typed i32, so out of range stays the `0` + // sentinel (#6088) rather than becoming `undefined`. + assert_eq!(js_uint8array_get(as_u8(arr), 2), 0); +} + +#[test] +fn uint8_helpers_read_and_write_a_wrong_kind_typed_array() { + let _serialized = crate::array::test_serialize(); + // A real Int32Array behind a `Uint8Array` static hint. `js_typed_array_ + // {get,set}` are kind-generic, so there is nothing unsafe about serving + // it — the old `!matches!(kind, UINT8 | UINT8_CLAMPED)` arm just answered + // `undefined` and dropped the store. node reads and writes the element. + let ta = typed(KIND_INT32, &[11.0, 12.0]); + let recv = as_recv(ta); + + assert_eq!(js_uint8array_index_get_value(recv, 1), 12.0); + js_uint8array_set(recv as *mut TypedArrayHeader, 0, 77); + assert_eq!(js_uint8array_index_get_value(recv, 0), 77.0); + // Through the kind-correct accessor as well: the lane really holds 77. + assert_eq!(js_typed_array_get(ta, 0), 77.0); +} + +#[test] +fn js_uint8array_index_get_value_is_undefined_for_a_non_pointer_receiver() { + let _serialized = crate::array::test_serialize(); + // `Q = 42 as any` — codegen masks the tag off, so the helper sees a small + // integer. Nothing indexable: `undefined`, and a store is dropped. + let bogus = 42u64 as *const TypedArrayHeader; + assert!(is_undefined(js_uint8array_index_get_value(bogus, 0))); + assert_eq!(js_uint8array_get(bogus, 0), 0); + js_uint8array_set(bogus as *mut TypedArrayHeader, 0, 5); +} + +// -------------------------------------------------------------------------- +// Controls: the receivers these helpers were WRITTEN for must be untouched. +// -------------------------------------------------------------------------- + +#[test] +fn uint8_helpers_still_serve_a_real_uint8_array() { + let _serialized = crate::array::test_serialize(); + let ta = typed(KIND_UINT8, &[3.0, 4.0]); + let recv = as_recv(ta); + + assert_eq!(js_uint8array_index_get_value(recv, 0), 3.0); + assert_eq!(js_uint8array_get(recv, 1), 4); + js_uint8array_set(recv as *mut TypedArrayHeader, 1, 250); + assert_eq!(js_uint8array_index_get_value(recv, 1), 250.0); + // A Uint8Array lane is 1 byte: 300 wraps to 44. If the store had been + // diverted to a plain-array `[[Set]]` it would read back 300. + js_uint8array_set(recv as *mut TypedArrayHeader, 0, 300); + assert_eq!(js_uint8array_index_get_value(recv, 0), 44.0); + assert!(is_undefined(js_uint8array_index_get_value(recv, 2))); +} + +#[test] +fn uint8_helpers_still_serve_a_uint8_clamped_array() { + let _serialized = crate::array::test_serialize(); + let ta = typed(KIND_UINT8_CLAMPED, &[1.0, 2.0]); + let recv = as_recv(ta); + // Clamped, not wrapped: 300 -> 255. The kind-specific store is still the + // one running. + js_uint8array_set(recv as *mut TypedArrayHeader, 0, 300); + assert_eq!(js_uint8array_index_get_value(recv, 0), 255.0); +}