Skip to content

fix(ir_lower): release the nullable-access hidden temp after its consuming chained read#531

Open
mirchaemanuel wants to merge 1 commit into
illegalstudio:mainfrom
mirchaemanuel:fix/525-nullable-chained-read-release
Open

fix(ir_lower): release the nullable-access hidden temp after its consuming chained read#531
mirchaemanuel wants to merge 1 commit into
illegalstudio:mainfrom
mirchaemanuel:fix/525-nullable-chained-read-release

Conversation

@mirchaemanuel

Copy link
Copy Markdown
Contributor

Fixes #525: a chained subscript read on a ?array receiver leaks the intermediate container on every evaluation.

Root cause (confirmed at source level)

lower_nullable_array_access (src/ir_lower/expr/mod.rs) materializes the intermediate through a hidden owned temp (declare_owned_hidden_tempstore_value_into_temp acquires +1) and returns it via take_owned_temp, which LoadLocals the value and clears the slot without releasing. Because value_is_nullable is false for Mixed, the consuming outer read does not re-enter the nullable path — it goes to lower_array_access_from_value with receiver = the owned-temp load, taking the Op::RuntimeCall arm (__rt_mixed_array_get). store_local knows how to release owned-temp loads; the read path had no analogous release, so the temp's reference leaked (~7 blocks per non-null call in the issue repro: outer array + inner array + string payloads).

The fix (IR-level, all targets)

In lower_array_access_from_value, after emitting the read: when the op is the boxed-Mixed runtime read and the receiver is an owned-temp load, emit a release through the existing release_if_owned primitive. Exactly once (the temp slot is already cleared, so this read is the last consuming use), covering both merge results — the populated container from the non-null path and the boxed Mixed null from the null path (__rt_decref_mixed is null-tolerant).

Safety: __rt_mixed_array_get's documented contract (src/codegen_support/runtime/objects/mixed_array_get.rs) is that every successful return is an owned Mixed — string payloads persisted, child heap pointers retained — so releasing the receiver cannot invalidate the just-extracted result. Typed reads (ArrayGet, StrCharAt, …) can return borrowing payloads and are deliberately untouched.

Evidence (macOS arm64, base v0.26.1 93f576168; all outputs byte-identical pre/post and match PHP 8.4.23)

Program before (live blocks) after
issue repro (30 calls) 205 175 — the −30 is exactly the hidden temp/iteration
chained read isolated from unrelated leaks 146 116 = the store-only floor: #525 construct fully clean
deeper $r[0][0][1] 322 292 (−30, its one hidden temp)
null-receiver case 2 1 (boxed-null temp now freed; no new warnings)
$r[0][0] . $r[0][1] (temp used twice per iter) 294 234 (−60, no double-release)
plain non-nullable chained read 90 90 (unchanged, #524's territory)

The post-fix residual in the raw repro decomposes exactly (arithmetic checked) into two pre-existing, independent leaks that reproduce on clean main with zero nullable chained reads: a ?array-return/store leak (~4 blocks/call) and the Mixed→string cast-source leak (fixed separately in the sibling PR for #527). Both are follow-up candidates.

Tests

2 regression tests in tests/codegen/runtime_gc/regressions.rs (using a ?array parameter so the unrelated pre-existing leaks cannot mask the assertion; both exercise the null and non-null path every iteration). Proven fail-pre-fix (live_blocks=250/400 via stashed-src build), pass post-fix. Focused suites all green: nullsafe 21, nullable 34, nested 119, runtime_gc 124, arrays 337, array_access 24, isset 20, coalesce 52, ternary 36, mixed 221. cargo build/--release zero warnings; debug-profile IR validator clean.

Notes for the maintainer

…g chained read

A chained subscript read on a ?array receiver materializes the
intermediate container through the hidden owned temp created by
lower_nullable_array_access. take_owned_temp clears the backing slot
without releasing, so when the loaded temp value became the receiver of
the consuming read nothing ever dropped its reference, leaking the
returned containers on every non-null call (issue illegalstudio#525).

Release the receiver with an IR-level Op::Release immediately after the
consuming read when it is a load of a one-shot owned hidden temp and the
read went through the boxed Mixed runtime path, whose result is an
owned, independent cell (string payloads persisted, child pointers
retained), so the release cannot invalidate the result. Typed reads
(ArrayGet, StrCharAt, ...) can return payloads borrowing the receiver's
storage and keep it alive, and direct array_get/hash_get receivers are
intentionally untouched.

Covers both merge results exactly once: the populated container from the
non-null path and the boxed Mixed null from the null path.

Fixes illegalstudio#525

Claude-Session: https://claude.ai/code/session_01Jfn7uqEH5Dog1nBzTM5DCW
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chained subscript read on a ?array receiver leaks the returned containers (nullable-access hidden temp is never released)

1 participant