From 610136fbb6cc30252032b892bc5bf528f34b485e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 15 Aug 2026 08:58:34 +0200 Subject: [PATCH] docs(array): correct the receiver-backing claim behind a recurring bug family MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `array_receiver_gc_tag`'s doc said `Buffer` and `TypedArray` payloads are `std::alloc`-backed with no `GcHeader`. That describes only the external backing. Arena-backed buffers and typed arrays carry a real header with a correct `obj_type` — they are pinned, not untracked, and conflating those two is why the same silent-drop defect kept being reintroduced against this comment across five PRs. State both backings, and point at `arena_payload_has_gc_type` as the predicate that establishes which one an address has, so the next reader does not open-code a floor. Comment-only; no behaviour change. --- .../8142-clean-arr-ptr-backing-comment.md | 30 +++++++++++++++++++ crates/perry-runtime/src/array/header.rs | 28 ++++++++++++++--- 2 files changed, 54 insertions(+), 4 deletions(-) create mode 100644 changelog.d/8142-clean-arr-ptr-backing-comment.md diff --git a/changelog.d/8142-clean-arr-ptr-backing-comment.md b/changelog.d/8142-clean-arr-ptr-backing-comment.md new file mode 100644 index 0000000000..706a62d11e --- /dev/null +++ b/changelog.d/8142-clean-arr-ptr-backing-comment.md @@ -0,0 +1,30 @@ +### Fixed + +- **Corrected the `array_receiver_gc_tag` doc comment that caused a recurring + silent-drop bug family.** The comment stated flatly that `Buffer` and + `TypedArray` payloads are `std::alloc`-backed with no `GcHeader`. That is + true for only one of the two backings these receivers actually have, and + reading it as universal is how the same defect kept being reintroduced — + five separate PRs (#8090, #8109, #8119, #8120, #8130) each fixed one + instance of a receiver being nulled or misread at an array funnel, and the + sweep in `gc-handoff/ARRAY-SWEEP-NOTES.md` found more (#8137, #8138). + + Both backings exist and reach these funnels: + + - **arena-backed** — `buffer/header.rs`'s `arena_alloc_gc_old(…, + GC_TYPE_BUFFER)` and `typedarray/mod.rs`'s `GC_TYPE_TYPED_ARRAY` site. + These carry a genuine `GcHeader` with a correct `obj_type`. They are + *pinned*, which is a different property from being *untracked* — the + conflation is the root of the confusion. This is the population #8041 + began nulling. + - **external** — `EXTERNAL_BUFFER_REGISTRY` / `EXTERNAL_UINT8ARRAY_REGISTRY` + addresses and `shared_sab::alloc_shared_sab`'s `alloc_zeroed`. For these + the eight bytes below the payload really are allocator bookkeeping. + + The tag is therefore authoritative only once the address is known to be + arena-backed, which `typedarray::arena_payload_has_gc_type` already does + properly (range check, `HeapSpace::Unknown` rejection against the *header* + address, `gc_type_info` validation). The comment now says so and points at + it, so the next reader does not open-code a floor instead. + + Comment-only; no behaviour change. diff --git a/crates/perry-runtime/src/array/header.rs b/crates/perry-runtime/src/array/header.rs index e84949bd0c..1b03de7a09 100644 --- a/crates/perry-runtime/src/array/header.rs +++ b/crates/perry-runtime/src/array/header.rs @@ -73,10 +73,30 @@ pub(crate) fn array_object_flags(arr: *const ArrayHeader) -> u16 { /// `arr` is too low to carry a header — `0` is not a legal `obj_type`, so it /// reads as "unknown" at every call site. /// -/// A non-zero tag is NOT proof that a real header exists. `Buffer` and -/// `TypedArray` payloads are `std::alloc`-backed, so the eight bytes below them -/// are allocator bookkeeping and can read as any value. Use the answer only -/// where a wrong tag is harmless: +/// A non-zero tag is NOT proof that a real header exists — but the reason is +/// narrower than this comment used to claim, and getting it wrong in either +/// direction has cost this file a recurring bug family (#8137/#8138, swept in +/// `gc-handoff/ARRAY-SWEEP-NOTES.md`). +/// +/// `Buffer` and `TypedArray` receivers come in BOTH backings: +/// +/// * **arena-backed** — `buffer/header.rs`'s `arena_alloc_gc_old(…, +/// GC_TYPE_BUFFER)` and `typedarray/mod.rs`'s `GC_TYPE_TYPED_ARRAY` site. +/// These carry a genuine `GcHeader` with a correct `obj_type`; they are +/// pinned rather than movable, which is a different property from being +/// untracked. This is the population #8041 started nulling. +/// * **external** — `EXTERNAL_BUFFER_REGISTRY` / +/// `EXTERNAL_UINT8ARRAY_REGISTRY` addresses, plus +/// `shared_sab::alloc_shared_sab`'s `alloc_zeroed`. For these the eight +/// bytes below the payload really are allocator bookkeeping and can read +/// as any value. +/// +/// So the tag is authoritative only once the address is known to be +/// arena-backed. `typedarray::arena_payload_has_gc_type` is the predicate +/// that does it properly: it range-checks, rejects `HeapSpace::Unknown` for +/// the HEADER address specifically, and validates via `gc_type_info` before +/// trusting the byte. Do not open-code a floor instead. Use a bare tag read +/// only where a wrong tag is harmless: /// /// * to *skip* a registry probe whose answer for that receiver would have been /// `false` anyway — the caller must already have routed real buffers and