diff --git a/CHANGELOG.md b/CHANGELOG.md index e30abca731..f88e241303 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ All notable changes to elephc, a PHP-to-native compiler written in Rust. Releases are listed newest first. ## [Unreleased] +- Fixed by-reference `foreach` over indexed and associative array elements silently discarding mutations or stopping early when the parent was replaced (issue #580). ## [0.26.3] - 2026-08-05 - Added tagless `.lfc` source files with per-file PHP/LFC classification across entry points, includes, and autoload; LFC always enables elephc extensions, while `--strict-php` remains PHP-only and now composes with `--define`, callable dispatch, and `eval()`. diff --git a/docs/internals/the-ir.md b/docs/internals/the-ir.md index 8f16f15d17..5c91bd9224 100644 --- a/docs/internals/the-ir.md +++ b/docs/internals/the-ir.md @@ -314,6 +314,7 @@ Ownership operations: | Op | Operand | Result | Effects | Lowering | |---|---|---|---|---| | `Acquire` | refcounted/string/callable value | `Void` or retained value alias | `REFCOUNT_OP`, maybe `WRITES_HEAP` | `__rt_incref`, string persist/retain, callable descriptor retain if added | +| `Acquire` + `Bool` immediate | same | same | same | same; the immediate marks a *lifetime pin* and only opts the pair out of acquire/release cancellation | | `Release` | owned value | `Void` | `REFCOUNT_OP`, maybe `WRITES_HEAP`, debug may fatal | `__rt_decref_any`, `__rt_heap_free_safe`, callable descriptor release | | `Move` | any value | same type | pure validator operation | no machine instruction | | `Borrow` | value with live owner | same type | pure validator operation | no machine instruction | @@ -540,7 +541,9 @@ across a reset point. | `HashNew(key_type, value_type, capacity)` | none | `Heap(Hash)` | `alloc_heap` | | `ArrayLen`, `HashLen` | container | `I64` | `reads_heap` | | `ArrayGet` | array, index | element type | `reads_heap`, `may_warn`, maybe `may_fatal` | +| `ArrayGetForWrite` | array, index (`I64`) | element type, **borrowed** | `reads_heap`, `writes_heap`, `writes_local`, `alloc_heap`, `refcount_op`, `may_warn` | | `HashGet` | hash, key | value type | `reads_heap`, `may_warn`, maybe `may_fatal` | +| `HashGetForWrite` | hash, key | value type, **borrowed** | `reads_heap`, `writes_heap`, `writes_local`, `alloc_heap`, `refcount_op`, `may_warn` | | `ArraySet` | array, index, value | `Void` | `writes_heap`, maybe `alloc_heap`, `refcount_op` | | `HashSet` | hash, key, value | `Void` | `writes_heap`, maybe `alloc_heap`, `refcount_op` | | `ArrayPush`, `HashAppend` | container, value | `Void` | `writes_heap`, maybe `alloc_heap`, `refcount_op` | @@ -556,6 +559,43 @@ All mutating operations must preserve copy-on-write. The builder emits `ArrayEnsureUnique`/`HashEnsureUnique` before mutation unless prior ownership proofs make it unnecessary. +`ArrayGetForWrite` and `HashGetForWrite` are the read side of that rule for a +container element that is about to be mutated through an alias — today, the +source of a by-reference `foreach` (issue #580). Unlike `ArrayGet`/`HashGet` they +take no reference for the caller; they separate the receiver, then split the +element from any co-owner and store the separated container back into the +receiver's element slot, so the result is owned by the parent and unique. That is +what lets `foreach ($a[0] as &$v)` and `foreach ($h['a'] as &$v)` write through +to their sources: the plain retaining read left the element shared, and +`IterStart`'s own copy-on-write split then gave the loop a private copy to mutate +and discard. + +The two differ only in how they address the element slot. `ArrayGetForWrite` +scales an integer key into the indexed payload, so it requires an `I64` key. +`HashGetForWrite` cannot compute an address, so it takes the matching entry's +address from `__rt_hash_get` (returned in `x4` on AArch64, `r8` on x86_64, null +on a miss) and splits the container that entry holds; string and integer keys are +both fine, since the lookup normalizes them. Both require an array or hash +element, each split with its own runtime helper, and both keep the plain read's +missing-key warning and null-container sentinel. Every other shape — a `Mixed` +element in particular, whose read can materialize a fresh box instead of the +slot's own storage — keeps the retaining read. The receiver split only happens +for a receiver that came from a local slot, since the new container has to be +published somewhere. + +Because the result is borrowed, the parent's element slot is its only owner, and +the loop body can drop that parent (`$a = []`, `unset($a)`) while the iterator is +still running. The by-reference `foreach` therefore takes a **lifetime pin** on +the source — an `Acquire` marked with a `Bool(true)` immediate — emitted *after* +`IterStart`, and releases it on every exit: the loop's own exit block for normal +termination and `break`, and `emit_innermost_loop_cleanups` for `break N`, +`return`, and `throw`. The ordering is load-bearing in both directions. Earlier +than `IterStart` and the extra reference makes that instruction's +`__rt_array_ensure_unique` split, handing the loop the private copy this whole +mechanism exists to avoid; later than the last exit and the element outlives the +program's need for it. PHP gets the same effect for free: its by-reference +`foreach` holds a reference to the iterated array itself. + ### Iterables, SPL, and Foreach | Op | Operands | Result | Effects | @@ -979,7 +1019,11 @@ phase commits them, sharing `replace_all_uses`, `resolve_chains`, and scalar slots are not aliased. - **Paired acquire/release cancellation** — an `acquire` whose result is used exactly once, by its `release`, drops both. The single-use guard makes this - refcount-neutral on every path regardless of distance between the two ops. + refcount-neutral on every path regardless of distance between the two ops. An + `acquire` carrying an immediate is a **lifetime pin** and is exempt: its result + is deliberately never read, because the reference exists so the value survives + an interval in which another owner may release it, and that raised refcount is + exactly what the program observes. - **String-literal concat folding** — `str_concat(const_str a, const_str b)` interns `a ++ b` into the data pool and becomes a single `const_str` marked `persistent` so cleanup never frees the literal. Nested concats converge across diff --git a/src/codegen/lower_inst.rs b/src/codegen/lower_inst.rs index d9b1e6540e..21dfb8eed1 100644 --- a/src/codegen/lower_inst.rs +++ b/src/codegen/lower_inst.rs @@ -222,6 +222,7 @@ pub(super) fn lower_instruction(ctx: &mut FunctionContext<'_>, inst_id: InstId) Op::ArrayLen => arrays::lower_array_len(ctx, &inst), Op::ArrayGet => arrays::lower_array_get(ctx, &inst, true), Op::ArrayGetSilent => arrays::lower_array_get(ctx, &inst, false), + Op::ArrayGetForWrite => arrays::lower_array_get_for_write(ctx, &inst), Op::ArrayIsset => builtins::lower_array_isset(ctx, &inst), Op::ArrayElemAddr => arrays::lower_array_elem_addr(ctx, &inst), Op::ArraySet => arrays::lower_array_set(ctx, &inst), @@ -236,6 +237,7 @@ pub(super) fn lower_instruction(ctx: &mut FunctionContext<'_>, inst_id: InstId) Op::HashNew => hashes::lower_hash_new(ctx, &inst), Op::HashLen => hashes::lower_hash_len(ctx, &inst), Op::HashGet => hashes::lower_hash_get(ctx, &inst, true), + Op::HashGetForWrite => hashes::lower_hash_get_for_write(ctx, &inst), Op::HashGetSilent => hashes::lower_hash_get(ctx, &inst, false), Op::HashIsset => builtins::lower_hash_isset(ctx, &inst), Op::HashSet => hashes::lower_hash_set(ctx, &inst), diff --git a/src/codegen/lower_inst/arrays.rs b/src/codegen/lower_inst/arrays.rs index 0bafc10826..5f3b662944 100644 --- a/src/codegen/lower_inst/arrays.rs +++ b/src/codegen/lower_inst/arrays.rs @@ -236,11 +236,122 @@ pub(super) fn lower_array_to_hash(ctx: &mut FunctionContext<'_>, inst: &Instruct store_if_result(ctx, inst) } +/// Selects what an indexed-array element read does with the payload it loads. +#[derive(Clone, Copy)] +enum ArrayGetMode { + /// Plain rvalue read: the result carries a caller reference, so refcounted payloads are + /// increfed on the way out. + Retaining, + /// Copy-on-write fetch for a by-reference `foreach` source: the element is separated from + /// any co-owner through `helper`, the separated container is published back into the parent + /// slot, and the result is handed back BORROWED — the parent slot owns it, the reader does + /// not. + ForWrite { + /// Runtime COW helper matching the element's container kind. + helper: &'static str, + }, +} + /// Lowers an indexed-array element read with PHP null-sentinel fallback on misses. pub(super) fn lower_array_get( ctx: &mut FunctionContext<'_>, inst: &Instruction, warn_on_missing: bool, +) -> Result<()> { + lower_array_get_in_mode(ctx, inst, warn_on_missing, ArrayGetMode::Retaining) +} + +/// Lowers `ArrayGetForWrite`: the same element read as `array_get`, missing-key warning and +/// null-container sentinel fallback included, but the element is copy-on-write separated and +/// returned without a caller reference. +/// +/// A by-reference `foreach` mutates the container it iterates in place, and `iter_start` gets +/// there through `__rt_array_ensure_unique`, which copies whenever the source is shared. The +/// plain `array_get` read hands the loop the parent's container PLUS a reference of its own, so +/// the element sat at refcount 2, the loop copied it, wrote into the copy and dropped it: every +/// write was lost (issue #580). +/// +/// Simply skipping the retain is not enough, and is in fact worse: `__rt_array_ensure_unique` +/// CONSUMES one reference from the source when it splits, so on a genuinely shared element that +/// decrement would come out of the parent's own reference and leave the parent slot dangling. +/// This op therefore does the splits itself — the receiver first, then the element — publishing +/// each back into the slot it came from, exactly as PHP separates `$a` and then `$a[0]` before +/// iterating it by reference. What reaches the loop is unique, so `iter_start`'s own +/// `ensure_unique` is a no-op and the writes land in the container the parent holds. +/// +/// Not to be confused with `RuntimeCallTarget::ArrayFetchForWrite`, which serves the same +/// purpose for boxed `Mixed` containers (issue #555); this is the statically-typed indexed path. +pub(super) fn lower_array_get_for_write( + ctx: &mut FunctionContext<'_>, + inst: &Instruction, +) -> Result<()> { + let array = expect_operand(inst, 0)?; + let elem_ty = indexed_array_element_type(&ctx.value_php_type(array)?, inst)?; + let helper = array_get_for_write_cow_helper(&elem_ty).ok_or_else(|| { + CodegenIrError::unsupported(format!( + "array_get_for_write element PHP type {:?}", + elem_ty + )) + })?; + separate_get_for_write_receiver(ctx, array, "__rt_array_ensure_unique")?; + lower_array_get_in_mode(ctx, inst, true, ArrayGetMode::ForWrite { helper }) +} + +/// Separates the receiver itself before its element slot is rewritten. +/// +/// PHP separates `$a` on the way to separating `$a[0]`, and so does elephc's element WRITE path: +/// `$a[0] = ...` splits the receiver inside `__rt_array_set_*` and stores the unique pointer back +/// to the source local. Fetch-for-write publishes a new element pointer into the receiver's +/// payload, so it owes the same guarantee — otherwise `$b = $a; foreach ($a[0] as &$v)` would +/// mutate storage `$b` still observes. +/// +/// Only receivers that came from a local (or a global mirrored through one) are separated here: +/// the split returns a NEW container, and without a slot to publish it to the caller would keep +/// reading the old one. A chained receiver needs no split at this point anyway — the lowering +/// walks `$a[0][0]` down to its base local and fetches every level for write on the way back up, +/// so an inner level always hands the next one a container that is already unique. +/// +/// `helper` selects the copy-on-write split matching the receiver's own container kind: +/// `__rt_array_ensure_unique` for an indexed receiver, `__rt_hash_ensure_unique` for a hash one. +pub(super) fn separate_get_for_write_receiver( + ctx: &mut FunctionContext<'_>, + array: ValueId, + helper: &str, +) -> Result<()> { + let Some(slot) = source_load_local_slot(ctx, array)? else { + return Ok(()); + }; + let arg_reg = abi::int_arg_reg_name(ctx.emitter.target, 0); + ctx.load_value_to_reg(array, arg_reg)?; + abi::emit_call_label(ctx.emitter, helper); + ctx.store_result_value(array)?; + ctx.store_value_to_local(slot, array)?; + ctx.writeback_global_array_source(array) +} + +/// Returns the copy-on-write helper that separates an element of this type, if there is one. +/// +/// Only the two container shapes need — and survive — the split: an indexed array and a hash, +/// each with its own clone helper. Everything else is rejected. `Mixed` in particular is not a +/// single container to separate: its slot can hold an invoker ref-cell marker whose read +/// materializes a freshly boxed value instead of the slot's own storage. +/// +/// Shared with the hash receiver path: what selects the helper is the ELEMENT's container kind, +/// which is independent of whether the receiver holding it is indexed or associative. +pub(super) fn array_get_for_write_cow_helper(elem_ty: &PhpType) -> Option<&'static str> { + match elem_ty.codegen_repr() { + PhpType::Array(_) => Some("__rt_array_ensure_unique"), + PhpType::AssocArray { .. } => Some("__rt_hash_ensure_unique"), + _ => None, + } +} + +/// Lowers an indexed-array element read under the requested ownership mode. +fn lower_array_get_in_mode( + ctx: &mut FunctionContext<'_>, + inst: &Instruction, + warn_on_missing: bool, + mode: ArrayGetMode, ) -> Result<()> { let array = expect_operand(inst, 0)?; let index = expect_operand(inst, 1)?; @@ -248,12 +359,26 @@ pub(super) fn lower_array_get( require_array_get_result(&elem_ty, inst)?; let result_ty = inst.result_php_type.codegen_repr(); match ctx.emitter.target.arch { - Arch::AArch64 => { - lower_array_get_aarch64(ctx, inst, array, index, &elem_ty, &result_ty, warn_on_missing) - } - Arch::X86_64 => { - lower_array_get_x86_64(ctx, inst, array, index, &elem_ty, &result_ty, warn_on_missing) - } + Arch::AArch64 => lower_array_get_aarch64( + ctx, + inst, + array, + index, + &elem_ty, + &result_ty, + warn_on_missing, + mode, + ), + Arch::X86_64 => lower_array_get_x86_64( + ctx, + inst, + array, + index, + &elem_ty, + &result_ty, + warn_on_missing, + mode, + ), } } @@ -649,6 +774,7 @@ fn lower_array_get_aarch64( elem_ty: &PhpType, result_ty: &PhpType, warn_on_missing: bool, + mode: ArrayGetMode, ) -> Result<()> { let array_reg = abi::symbol_scratch_reg(ctx.emitter); let len_reg = abi::secondary_scratch_reg(ctx.emitter); @@ -672,7 +798,7 @@ fn lower_array_get_aarch64( abi::emit_load_from_address(ctx.emitter, len_reg, array_reg, 0); ctx.emitter.instruction(&format!("cmp {}, {}", result_reg, len_reg)); // compare the requested offset against the indexed-array length ctx.emitter.instruction(&format!("b.ge {}", null_label)); // out-of-range indexed-array offsets read as null - emit_array_get_in_bounds_aarch64(ctx, array_reg, result_reg, elem_ty, result_ty)?; + emit_array_get_in_bounds_aarch64(ctx, array_reg, result_reg, elem_ty, result_ty, mode)?; ctx.emitter.instruction(&format!("b {}", done_label)); // skip the null fallback after a successful indexed-array read ctx.emitter.label(&null_label); if warn_on_missing { @@ -743,6 +869,7 @@ fn lower_array_get_x86_64( elem_ty: &PhpType, result_ty: &PhpType, warn_on_missing: bool, + mode: ArrayGetMode, ) -> Result<()> { let array_reg = abi::symbol_scratch_reg(ctx.emitter); let len_reg = abi::secondary_scratch_reg(ctx.emitter); @@ -766,7 +893,7 @@ fn lower_array_get_x86_64( abi::emit_load_from_address(ctx.emitter, len_reg, array_reg, 0); ctx.emitter.instruction(&format!("cmp {}, {}", result_reg, len_reg)); // compare the requested offset against the indexed-array length ctx.emitter.instruction(&format!("jge {}", null_label)); // out-of-range indexed-array offsets read as null - emit_array_get_in_bounds_x86_64(ctx, array_reg, result_reg, elem_ty, result_ty)?; + emit_array_get_in_bounds_x86_64(ctx, array_reg, result_reg, elem_ty, result_ty, mode)?; ctx.emitter.instruction(&format!("jmp {}", done_label)); // skip the null fallback after a successful indexed-array read ctx.emitter.label(&null_label); if warn_on_missing { @@ -835,7 +962,12 @@ fn emit_array_get_in_bounds_aarch64( index_reg: &str, elem_ty: &PhpType, result_ty: &PhpType, + mode: ArrayGetMode, ) -> Result<()> { + if let ArrayGetMode::ForWrite { helper } = mode { + emit_array_get_for_write_in_bounds_aarch64(ctx, array_reg, index_reg, helper); + return Ok(()); + } match elem_ty { PhpType::Void | PhpType::Never => { abi::emit_load_int_immediate(ctx.emitter, index_reg, 0x7fff_ffff_ffff_fffe); @@ -897,7 +1029,12 @@ fn emit_array_get_in_bounds_x86_64( index_reg: &str, elem_ty: &PhpType, result_ty: &PhpType, + mode: ArrayGetMode, ) -> Result<()> { + if let ArrayGetMode::ForWrite { helper } = mode { + emit_array_get_for_write_in_bounds_x86_64(ctx, array_reg, index_reg, helper); + return Ok(()); + } match elem_ty { PhpType::Void | PhpType::Never => { abi::emit_load_int_immediate(ctx.emitter, index_reg, 0x7fff_ffff_ffff_fffe); @@ -952,6 +1089,51 @@ fn emit_array_get_in_bounds_x86_64( Ok(()) } +/// Emits the in-bounds copy-on-write element fetch for AArch64. +/// +/// Computes the element slot address, separates the container it holds through `helper`, and +/// stores the (possibly new) pointer straight back into that slot. The store is unconditional +/// because the helper returns the original pointer untouched whenever no split was needed, and +/// it is what keeps the refcounts balanced on the split path: the `ensure_unique` helpers drop +/// one reference from the shared original, and the slot it came from is exactly the owner giving +/// that reference up, taking the fresh clone in exchange. +/// +/// The slot address is spilled across the call because both scratch registers used here are +/// caller-saved. Result: the unique element pointer in the integer result register, BORROWED — +/// the parent slot owns it. +fn emit_array_get_for_write_in_bounds_aarch64( + ctx: &mut FunctionContext<'_>, + array_reg: &str, + index_reg: &str, + helper: &str, +) { + let result_reg = abi::int_result_reg(ctx.emitter); + ctx.emitter.instruction(&format!("add {}, {}, #24", array_reg, array_reg)); // skip the indexed-array header to reach element payloads + ctx.emitter.instruction(&format!("add {}, {}, {}, lsl #3", array_reg, array_reg, index_reg)); // address the selected element slot within the payload + abi::emit_push_reg(ctx.emitter, array_reg); // preserve the element slot address across the copy-on-write helper call + abi::emit_load_from_address(ctx.emitter, result_reg, array_reg, 0); + abi::emit_call_label(ctx.emitter, helper); + abi::emit_pop_reg(ctx.emitter, array_reg); // restore the element slot address after the copy-on-write helper call + abi::emit_store_to_address(ctx.emitter, result_reg, array_reg, 0); +} + +/// Emits the in-bounds copy-on-write element fetch for x86_64. Mirrors the AArch64 shape. +fn emit_array_get_for_write_in_bounds_x86_64( + ctx: &mut FunctionContext<'_>, + array_reg: &str, + index_reg: &str, + helper: &str, +) { + let result_reg = abi::int_result_reg(ctx.emitter); + ctx.emitter.instruction(&format!("lea {}, [{} + 24]", array_reg, array_reg)); // skip the indexed-array header to reach element payloads + ctx.emitter.instruction(&format!("lea {}, [{} + {} * 8]", array_reg, array_reg, index_reg)); // address the selected element slot within the payload + abi::emit_push_reg(ctx.emitter, array_reg); // preserve the element slot address across the copy-on-write helper call + abi::emit_load_from_address(ctx.emitter, "rdi", array_reg, 0); + abi::emit_call_label(ctx.emitter, helper); + abi::emit_pop_reg(ctx.emitter, array_reg); // restore the element slot address after the copy-on-write helper call + abi::emit_store_to_address(ctx.emitter, result_reg, array_reg, 0); +} + /// Dereferences descriptor-style ref-cell markers loaded from Mixed array slots. fn emit_mixed_array_get_deref_invoker_ref_cell( ctx: &mut FunctionContext<'_>, diff --git a/src/codegen/lower_inst/hashes.rs b/src/codegen/lower_inst/hashes.rs index 28b0513777..28a73e13dd 100644 --- a/src/codegen/lower_inst/hashes.rs +++ b/src/codegen/lower_inst/hashes.rs @@ -8,6 +8,9 @@ //! Key details: //! - Hash writes may copy-on-write or grow the table, so the returned pointer is //! written back to the source SSA slot and local slot. +//! - `HashGetForWrite` is a lookup that also WRITES: it separates the container the +//! matching entry holds and republishes it into that entry's value slot, whose +//! address comes from `__rt_hash_get`'s entry-address output (issue #580). use crate::codegen::{ abi, emit_box_current_owned_value_as_mixed, emit_box_current_value_as_mixed, @@ -128,6 +131,129 @@ pub(super) fn lower_hash_get( } } +/// Lowers `HashGetForWrite`: the same lookup as `hash_get`, missing-key warning and +/// null-container sentinel fallback included, but the found element is copy-on-write separated +/// in place and returned without a caller reference. +/// +/// This is the hash-receiver counterpart of `arrays::lower_array_get_for_write`, and it exists +/// for the same reason (issue #580): a by-reference `foreach` mutates the container it iterates, +/// `iter_start` reaches that through the ensure-unique helpers, and the plain `hash_get` read +/// hands the loop the parent's container PLUS a reference of its own — refcount 2, so the loop +/// copied, wrote into the copy and dropped it. +/// +/// The two receiver kinds differ only in how the element slot is addressed. An indexed receiver +/// computes it with pointer arithmetic; a hash entry has to be probed for, so the address comes +/// from `__rt_hash_get`'s entry-address output (`x4` / `r8`), which the probe already builds. +/// Everything downstream is identical: split the container the slot holds, store the unique +/// pointer straight back into that slot, hand the result out BORROWED — the entry owns it. +pub(super) fn lower_hash_get_for_write( + ctx: &mut FunctionContext<'_>, + inst: &Instruction, +) -> Result<()> { + let hash = expect_operand(inst, 0)?; + let key = expect_operand(inst, 1)?; + let value_ty = assoc_value_type(&ctx.value_php_type(hash)?, inst)?; + let helper = super::arrays::array_get_for_write_cow_helper(&value_ty).ok_or_else(|| { + CodegenIrError::unsupported(format!("hash_get_for_write value PHP type {:?}", value_ty)) + })?; + super::arrays::separate_get_for_write_receiver(ctx, hash, "__rt_hash_ensure_unique")?; + let result_ty = inst.result_php_type.codegen_repr(); + match ctx.emitter.target.arch { + Arch::AArch64 => lower_hash_get_for_write_aarch64(ctx, inst, hash, key, &result_ty, helper), + Arch::X86_64 => lower_hash_get_for_write_x86_64(ctx, inst, hash, key, &result_ty, helper), + } +} + +/// Lowers the copy-on-write hash element fetch for AArch64 targets. +/// +/// `__rt_hash_get` leaves the matching entry address in `x4` (0 on a miss), so the split works +/// straight on the entry's value slot at `+24`. The slot address is spilled across the helper +/// call because `x4` is caller-saved. The unconditional store back is what keeps the refcounts +/// balanced: the ensure-unique helpers drop one reference from a shared original, and the entry +/// is exactly the owner giving that reference up in exchange for the fresh clone. +fn lower_hash_get_for_write_aarch64( + ctx: &mut FunctionContext<'_>, + inst: &Instruction, + hash: ValueId, + key: ValueId, + result_ty: &PhpType, + helper: &str, +) -> Result<()> { + materialize_hash_key_aarch64(ctx, key)?; + ctx.load_value_to_reg(hash, "x0")?; + let miss = ctx.next_label("hash_get_fw_miss"); + let null_receiver = ctx.next_label("hash_get_fw_null_recv"); + let fallback = ctx.next_label("hash_get_fw_fallback"); + let done = ctx.next_label("hash_get_fw_done"); + crate::codegen::sentinels::emit_branch_if_null_container( + ctx.emitter, + "x0", + "x9", + &null_receiver, + ); + abi::emit_call_label(ctx.emitter, "__rt_hash_get"); + ctx.emitter.instruction(&format!("cbz x0, {}", miss)); // branch to the null fallback when the associative lookup misses + ctx.emitter.instruction("add x4, x4, #24"); // address the matching entry's value slot + abi::emit_push_reg(ctx.emitter, "x4"); // preserve the entry value-slot address across the copy-on-write helper call + abi::emit_load_from_address(ctx.emitter, "x0", "x4", 0); + abi::emit_call_label(ctx.emitter, helper); + abi::emit_pop_reg(ctx.emitter, "x4"); // restore the entry value-slot address after the copy-on-write helper call + abi::emit_store_to_address(ctx.emitter, "x0", "x4", 0); + ctx.emitter.instruction(&format!("b {}", done)); // skip the miss fallback after separating the hash element + ctx.emitter.label(&miss); + emit_undefined_hash_key_warning_aarch64(ctx, key)?; + abi::emit_jump(ctx.emitter, &fallback); + ctx.emitter.label(&null_receiver); + super::arrays::emit_array_offset_on_null_warning(ctx); + ctx.emitter.label(&fallback); + emit_hash_get_miss(ctx, result_ty, false); + ctx.emitter.label(&done); + store_if_result(ctx, inst) +} + +/// Lowers the copy-on-write hash element fetch for x86_64 targets. Mirrors the AArch64 shape, +/// reading the matching entry address from `__rt_hash_get`'s `r8` output. +fn lower_hash_get_for_write_x86_64( + ctx: &mut FunctionContext<'_>, + inst: &Instruction, + hash: ValueId, + key: ValueId, + result_ty: &PhpType, + helper: &str, +) -> Result<()> { + materialize_hash_key_x86_64(ctx, key)?; + ctx.load_value_to_reg(hash, "rdi")?; + let miss = ctx.next_label("hash_get_fw_miss"); + let null_receiver = ctx.next_label("hash_get_fw_null_recv"); + let fallback = ctx.next_label("hash_get_fw_fallback"); + let done = ctx.next_label("hash_get_fw_done"); + crate::codegen::sentinels::emit_branch_if_null_container( + ctx.emitter, + "rdi", + "r9", + &null_receiver, + ); + abi::emit_call_label(ctx.emitter, "__rt_hash_get"); + ctx.emitter.instruction("test rax, rax"); // check whether the associative lookup found a matching key + ctx.emitter.instruction(&format!("jz {}", miss)); // branch to the null fallback when the associative lookup misses + ctx.emitter.instruction("add r8, 24"); // address the matching entry's value slot + abi::emit_push_reg(ctx.emitter, "r8"); // preserve the entry value-slot address across the copy-on-write helper call + abi::emit_load_from_address(ctx.emitter, "rdi", "r8", 0); + abi::emit_call_label(ctx.emitter, helper); + abi::emit_pop_reg(ctx.emitter, "r8"); // restore the entry value-slot address after the copy-on-write helper call + abi::emit_store_to_address(ctx.emitter, "rax", "r8", 0); + ctx.emitter.instruction(&format!("jmp {}", done)); // skip the miss fallback after separating the hash element + ctx.emitter.label(&miss); + emit_undefined_hash_key_warning_x86_64(ctx, key)?; + abi::emit_jump(ctx.emitter, &fallback); + ctx.emitter.label(&null_receiver); + super::arrays::emit_array_offset_on_null_warning(ctx); + ctx.emitter.label(&fallback); + emit_hash_get_miss(ctx, result_ty, false); + ctx.emitter.label(&done); + store_if_result(ctx, inst) +} + /// Lowers an associative-array insert/update through the shared hash runtime helper. pub(super) fn lower_hash_set(ctx: &mut FunctionContext<'_>, inst: &Instruction) -> Result<()> { let hash = expect_operand(inst, 0)?; diff --git a/src/codegen_support/runtime/arrays/hash_get.rs b/src/codegen_support/runtime/arrays/hash_get.rs index 83c9d45c75..585eba4cbf 100644 --- a/src/codegen_support/runtime/arrays/hash_get.rs +++ b/src/codegen_support/runtime/arrays/hash_get.rs @@ -7,6 +7,9 @@ //! //! Key details: //! - Hash helpers must normalize PHP keys and preserve bucket layout, ownership, and iteration conventions. +//! - Besides the borrowed payload, the lookup returns the matching entry's ADDRESS +//! (`x4` / `r8`, null on a miss) so callers that must write the slot back can reach +//! it; the probe already computes that address (issue #580). use crate::codegen_support::emit::Emitter; use crate::codegen_support::platform::Arch; @@ -15,7 +18,15 @@ use crate::codegen_support::platform::Arch; /// Uses `__rt_hash_key_hash` to compute the initial slot and `__rt_hash_key_eq` for equality checks. /// Falls through to `__rt_hash_get_not_found` when the table is null, empty, or the key is absent. /// Input: x0=hash_table_ptr, x1=key_lo, x2=key_hi (key_hi=-1 means integer key, otherwise string key with key_lo=ptr, key_hi=len) -/// Output: x0=found (1 or 0), x1=value_lo, x2=value_hi, x3=value_tag (PhpType tag; null tag on miss) +/// Output: x0=found (1 or 0), x1=value_lo, x2=value_hi, x3=value_tag (PhpType tag; null tag on miss), +/// x4=address of the matching entry (0 on miss) +/// +/// The entry address is what lets a caller WRITE the slot back rather than only read it: the +/// by-reference `foreach` element fetch separates the container the entry holds and republishes +/// the unique pointer into `[x4, #24]` (issue #580). It costs one move on the found path because +/// the probe has already computed the address. Both `x4` and the x86_64 mirror `r8` are +/// caller-saved and were already clobbered by this helper's own probing and key comparisons, so +/// exposing them adds no constraint on existing callers. pub fn emit_hash_get(emitter: &mut Emitter) { if emitter.target.arch == Arch::X86_64 { emit_hash_get_linux_x86_64(emitter); @@ -121,6 +132,7 @@ pub fn emit_hash_get(emitter: &mut Emitter) { emitter.instruction("ldr x1, [x12, #24]"); // x1 = value_lo emitter.instruction("ldr x2, [x12, #32]"); // x2 = value_hi emitter.instruction("ldr x3, [x12, #40]"); // x3 = value_tag + emitter.instruction("mov x4, x12"); // x4 = matching entry address for write-back callers emitter.instruction("ldp x29, x30, [sp, #48]"); // restore frame pointer and return address emitter.instruction("add sp, sp, #64"); // deallocate stack frame emitter.instruction("ret"); // return to caller @@ -131,6 +143,7 @@ pub fn emit_hash_get(emitter: &mut Emitter) { emitter.instruction("mov x1, #0"); // value_lo = 0 emitter.instruction("mov x2, #0"); // value_hi = 0 emitter.instruction("mov x3, #8"); // value_tag = null when lookup misses + emitter.instruction("mov x4, #0"); // no entry address to write back on a miss emitter.instruction("ldp x29, x30, [sp, #48]"); // restore frame pointer and return address emitter.instruction("add sp, sp, #64"); // deallocate stack frame emitter.instruction("ret"); // return to caller @@ -138,7 +151,10 @@ pub fn emit_hash_get(emitter: &mut Emitter) { /// Emits the x86_64 Linux variant of `__rt_hash_get`. /// Uses SysV ABI: rdi=hash_table_ptr, rsi=key_lo, rdx=key_hi (key_hi=-1 means integer key, otherwise string key with rsi=ptr, rdx=len). -/// Returns: rax=found (1 or 0), rdi=value_lo, rsi=value_hi, rcx=value_tag. +/// Returns: rax=found (1 or 0), rdi=value_lo, rsi=value_hi, rcx=value_tag, r8=matching entry address (0 on miss). +/// `r8` mirrors the AArch64 `x4` entry-address output used by the by-reference `foreach` element +/// fetch (issue #580); the probe loop already builds the address there, so only the miss path +/// needs an explicit clear. fn emit_hash_get_linux_x86_64(emitter: &mut Emitter) { emitter.blank(); emitter.comment("--- runtime: hash_get ---"); @@ -220,7 +236,7 @@ fn emit_hash_get_linux_x86_64(emitter: &mut Emitter) { emitter.instruction("mov r8, r11"); // copy the matching probe index before scaling it into a byte offset emitter.instruction("shl r8, 6"); // convert the matching probe index into a 64-byte entry offset emitter.instruction("add r8, r10"); // advance from the hash-table base pointer to the matching entry block - emitter.instruction("add r8, 40"); // skip the fixed 40-byte hash header to land on the matching entry + emitter.instruction("add r8, 40"); // skip the fixed 40-byte hash header to land on the matching entry, kept as the entry-address result emitter.instruction("mov rdi, QWORD PTR [r8 + 24]"); // return the low payload word in the first borrowed-value result register emitter.instruction("mov rsi, QWORD PTR [r8 + 32]"); // return the high payload word in the second borrowed-value result register emitter.instruction("mov rcx, QWORD PTR [r8 + 40]"); // return the runtime value tag in the borrowed-value tag result register @@ -234,6 +250,7 @@ fn emit_hash_get_linux_x86_64(emitter: &mut Emitter) { emitter.instruction("xor edi, edi"); // clear the low payload word for the failed lookup path emitter.instruction("xor esi, esi"); // clear the high payload word for the failed lookup path emitter.instruction("mov ecx, 8"); // return runtime value tag 8 = null for failed hash lookups + emitter.instruction("xor r8d, r8d"); // no entry address to write back on a miss emitter.instruction("add rsp, 48"); // release the lookup spill slots before returning the failed lookup result emitter.instruction("pop rbp"); // restore the caller frame pointer before returning to generated code emitter.instruction("ret"); // return the failed lookup result to generated code diff --git a/src/ir/instr.rs b/src/ir/instr.rs index 67c729bb8a..dd4a7ec480 100644 --- a/src/ir/instr.rs +++ b/src/ir/instr.rs @@ -338,7 +338,9 @@ pub enum Op { HashLen, ArrayGet, ArrayGetSilent, + ArrayGetForWrite, HashGet, + HashGetForWrite, HashGetSilent, ArrayIsset, HashIsset, @@ -596,6 +598,13 @@ impl Op { } ArrayGetSilent | HashGetSilent | ArrayIsset | HashIsset => E::READS_HEAP, ArrayGet | HashGet => E::READS_HEAP | E::MAY_WARN, + // Not a pure read despite the name: the copy-on-write split rewrites the receiver's + // element slot (and the receiver's own local slot), so it must never be treated as + // reorderable or redundant against the plain reads around it. + ArrayGetForWrite | HashGetForWrite => { + E::READS_HEAP | E::WRITES_HEAP | E::WRITES_LOCAL | E::ALLOC_HEAP + | E::REFCOUNT_OP | E::MAY_WARN + } StrPersist | ArrayEnsureUnique | HashEnsureUnique | ArrayCloneShallow | HashCloneShallow | ObjectCloneShallow => { E::READS_HEAP | E::ALLOC_HEAP | E::REFCOUNT_OP @@ -817,7 +826,9 @@ impl Op { HashLen => "hash_len", ArrayGet => "array_get", ArrayGetSilent => "array_get_silent", + ArrayGetForWrite => "array_get_for_write", HashGet => "hash_get", + HashGetForWrite => "hash_get_for_write", HashGetSilent => "hash_get_silent", ArrayIsset => "array_isset", HashIsset => "hash_isset", diff --git a/src/ir/validator.rs b/src/ir/validator.rs index 7bdd82947d..aa0ce7562a 100644 --- a/src/ir/validator.rs +++ b/src/ir/validator.rs @@ -500,6 +500,23 @@ fn validate_opcode_rules( | ArrayGetMixedKeySilent => { check_first_heap(function, inst_id, inst, IrHeapKind::Array, "Heap(Array)") } + // The fetch-for-write element read is emitted from exactly one site (a by-reference + // `foreach` source, issue #580) and writes the copy-on-write split back into the + // receiver's element slot, so its operand shape is pinned tighter than the shared read + // arm above: an indexed receiver and an already int-coerced key, never a runtime-tagged + // one. + ArrayGetForWrite => { + check_count(inst_id, inst, 2, "2")?; + check_operand_type(function, inst_id, inst, 0, IrType::Heap(IrHeapKind::Array), "Heap(Array)")?; + check_operand_type(function, inst_id, inst, 1, IrType::I64, "I64") + } + // The hash counterpart of the fetch-for-write read, emitted from the same single site. + // Its key stays in whatever form `hash_get` accepts (string or integer) rather than being + // int-coerced, because the hash lookup normalizes the key itself. + HashGetForWrite => { + check_count(inst_id, inst, 2, "2")?; + check_operand_type(function, inst_id, inst, 0, IrType::Heap(IrHeapKind::Hash), "Heap(Hash)") + } LoadArrayElemRefCell => { check_count(inst_id, inst, 2, "2")?; check_operand_type(function, inst_id, inst, 0, IrType::Heap(IrHeapKind::Array), "Heap(Array)")?; diff --git a/src/ir_lower/context.rs b/src/ir_lower/context.rs index 79c1e1beae..b8c01ffa67 100644 --- a/src/ir_lower/context.rs +++ b/src/ir_lower/context.rs @@ -38,6 +38,11 @@ pub(crate) struct LoopFrame { pub break_block: BlockId, pub continue_block: BlockId, pub cleanup: Option, + /// Lifetime reference a by-reference `foreach` took on a borrowed element source, so the + /// loop keeps iterating live storage even if the body drops the parent that owned it + /// (issue #580). Released on every exit that skips the loop's own exit block, exactly like + /// `cleanup`. + pub source_pin: Option, } /// Cleanup that must run when control leaves a loop without visiting its exit block. diff --git a/src/ir_lower/expr/array_access.rs b/src/ir_lower/expr/array_access.rs index e0d088fdfa..f9fa95d385 100644 --- a/src/ir_lower/expr/array_access.rs +++ b/src/ir_lower/expr/array_access.rs @@ -19,6 +19,154 @@ pub(super) fn lower_array_access( lower_array_access_with_missing_warning(ctx, array, index, expr, true) } +/// Lowers `$base[...][$index]` as the source of a by-reference `foreach`, separating the element +/// for writing instead of reading a copy of it. +/// +/// A by-reference loop mutates its source container in place, and `iter_start` reaches that +/// through `__rt_array_ensure_unique`, which copies as soon as the source is shared. The plain +/// `array_get` read hands back the parent's container plus a reference of its own, so the +/// element sat at refcount 2 and every write went into a private copy that the loop then +/// dropped (issue #580). `Op::ArrayGetForWrite` performs the copy-on-write split itself — +/// separating the receiver first, then the element, publishing each back into the slot it came +/// from — and returns the element borrowed, so the loop mutates the very container the parent +/// holds, exactly like PHP. +/// +/// Both receiver kinds take this path. An indexed receiver reaches its element slot with pointer +/// arithmetic (`Op::ArrayGetForWrite`); a hash entry has to be probed for, so it goes through +/// `Op::HashGetForWrite`, which separates the container the matching entry holds. The hash form +/// never needed the reference binding the checker rejects for `$r = &$h['k'];` — nothing here +/// aliases a hash slot into a local, the loop simply iterates the parent's own storage. +/// +/// Falls back to the ordinary retaining read whenever the fetch-for-write would not be sound or +/// the codegen cannot express it: a non-container receiver, an indexed receiver with a +/// non-integer key, a `Mixed` element (its read can materialize a fresh box rather than the +/// slot's own storage), or a subscript chain not rooted in a local. The receiver is evaluated +/// exactly once on every path. +pub(crate) fn lower_by_ref_foreach_element_source( + ctx: &mut LoweringContext<'_, '_>, + array: &Expr, + index: &Expr, + expr: &Expr, +) -> LoweredValue { + let array_value = lower_by_ref_foreach_source_receiver(ctx, array); + let Some(op) = element_fetch_for_write_op(ctx, &array_value, index, expr) else { + if value_is_nullable(ctx, array_value.value) { + return lower_nullable_array_access(ctx, array_value, index, expr, true); + } + return lower_array_access_from_value(ctx, array_value, index, expr, true); + }; + let index_value = lower_expr(ctx, index); + // The hash lookup normalizes its own key, so only the indexed slot arithmetic needs an + // int-coerced one. + let index_value = if op == Op::ArrayGetForWrite { + coerce_to_int_at_span(ctx, index_value, Some(index.span)) + } else { + index_value + }; + let read_op = if op == Op::ArrayGetForWrite { + Op::ArrayGet + } else { + Op::HashGet + }; + let result_type = array_access_result_type(ctx, array_value.value, read_op, expr); + let result = ctx.emit_value( + op, + vec![array_value.value, index_value.value], + None, + result_type, + op.default_effects(), + Some(expr.span), + ); + // The separated element belongs to the parent's slot, not to this read — pin that explicitly + // instead of leaving it at the `MaybeOwned` default, which `mark_owned_temporaries` is free + // to promote to `Owned` later, and which would then have the loop release the parent's + // element on the way out. + ctx.builder + .set_value_ownership(result.value, Ownership::Borrowed); + release_coerced_source_if_owned(ctx, index_value, Some(index.span)); + // An intermediate that fell back to a retaining read owns a reference to the container it + // handed us, and holding it for the whole loop would be a leak. Dropping it here is safe + // precisely because `element_fetch_for_write_op` demanded a local-rooted chain: the base + // local keeps every level alive until the loop ends. An intermediate that took the + // fetch-for-write path is already borrowed, so this is a no-op for it. + // `stabilize_borrowed_result_and_release_receiver` is deliberately NOT used — it would + // acquire the borrowed result first, restoring the refcount 2 that causes the copy. + if ctx.value_is_owning_temporary(array_value) { + crate::ir_lower::ownership::release_if_owned(ctx, array_value, Some(expr.span)); + } + result +} + +/// Lowers the receiver of a by-reference `foreach` element source, fetching it for write too +/// when it is itself an eligible element. +/// +/// A chain has to be separated top-down, exactly as PHP does it: `foreach ($a[0][0] as &$v)` +/// separates `$a`, then `$a[0]` into `$a`'s slot, then `$a[0][0]` into `$a[0]`'s slot. Lowering +/// the intermediate with a plain read instead would leave it shared, so publishing the innermost +/// split into it would be visible through every alias of that intermediate. Each level hands the +/// next one a container that is already unique, which is why the inner levels find nothing left +/// to split and why nothing in the chain is owned by the reader. +fn lower_by_ref_foreach_source_receiver( + ctx: &mut LoweringContext<'_, '_>, + array: &Expr, +) -> LoweredValue { + if let ExprKind::ArrayAccess { array: receiver, index } = &array.kind { + return lower_by_ref_foreach_element_source(ctx, receiver, index, array); + } + lower_expr(ctx, array) +} + +/// Returns the fetch-for-write element read a by-reference `foreach` source can take, if any. +/// +/// Requires a statically-known container element — an indexed array or a hash, the two kinds +/// with a copy-on-write helper to split them — and a subscript chain rooted in a plain variable. +/// That last condition is what makes dropping an intermediate receiver safe: a chain over a +/// temporary — `f()[0]` — has no owner once the read returns, so borrowing out of it would leave +/// the loop iterating freed storage. +/// +/// An indexed receiver additionally needs an integer key, because its element slot is reached by +/// scaling the key into the payload. A hash receiver has no such restriction: `__rt_hash_get` +/// normalizes string and integer keys alike and reports the matching entry's address. +fn element_fetch_for_write_op( + ctx: &LoweringContext<'_, '_>, + array_value: &LoweredValue, + index: &Expr, + expr: &Expr, +) -> Option { + if value_is_nullable(ctx, array_value.value) { + return None; + } + let (op, elem_ty) = match ( + array_value.ir_type, + ctx.builder.value_php_type(array_value.value).codegen_repr(), + ) { + (IrType::Heap(IrHeapKind::Array), PhpType::Array(elem_ty)) => { + if index_expr_key_type(ctx, index) != PhpType::Int { + return None; + } + (Op::ArrayGetForWrite, elem_ty) + } + (IrType::Heap(IrHeapKind::Hash), PhpType::AssocArray { value, .. }) => { + (Op::HashGetForWrite, value) + } + _ => return None, + }; + let elem_ty = normalize_value_php_type(*elem_ty).codegen_repr(); + if !matches!(elem_ty, PhpType::Array(_) | PhpType::AssocArray { .. }) { + return None; + } + subscript_chain_is_variable_rooted(expr).then_some(op) +} + +/// Returns whether a subscript expression bottoms out in a plain variable receiver. +fn subscript_chain_is_variable_rooted(expr: &Expr) -> bool { + match &expr.kind { + ExprKind::Variable(_) => true, + ExprKind::ArrayAccess { array, .. } => subscript_chain_is_variable_rooted(array), + _ => false, + } +} + /// Lowers array, hash, string, or ArrayAccess indexing with configurable /// undefined-offset warning behavior for native indexed-array reads. Suppressed /// warnings propagate through the whole subscript chain: PHP's `isset()` and `??` @@ -285,4 +433,3 @@ pub(super) fn array_access_expr_satisfies_array_access( }; type_satisfies_array_access_for_ir(ctx, &ty) } - diff --git a/src/ir_lower/expr/mod.rs b/src/ir_lower/expr/mod.rs index 57858fbbbb..937a6ec181 100644 --- a/src/ir_lower/expr/mod.rs +++ b/src/ir_lower/expr/mod.rs @@ -158,7 +158,7 @@ pub(crate) use indexed_array_literals::{ }; pub(crate) use array_access::{ array_access_element_result_type, index_expr_key_type, - lower_array_access_from_lowered_receiver, + lower_array_access_from_lowered_receiver, lower_by_ref_foreach_element_source, }; pub(crate) use array_access_types::type_satisfies_array_access_for_ir; pub(crate) use instanceof_coercions::coerce_to_int_at_span; diff --git a/src/ir_lower/ownership.rs b/src/ir_lower/ownership.rs index b06fcebf26..8f29fbf1c9 100644 --- a/src/ir_lower/ownership.rs +++ b/src/ir_lower/ownership.rs @@ -36,6 +36,37 @@ pub(crate) fn acquire_if_refcounted( value } +/// Emits an acquire that is marked as a lifetime pin: a reference taken purely so the value +/// outlives an interval, not so it can be read through the acquired result. +/// +/// The `Immediate::Bool(true)` marker is what tells the paired acquire/release peephole to leave +/// this pair alone. That peephole cancels an `Acquire` whose only use is its `Release` on the +/// premise that the raised refcount in between is unobservable — true for a value nobody else +/// touches, false by construction here: the whole point of a pin is that something inside the +/// interval may drop the other owner, and cancelling the pair would hand that interval freed +/// storage (issue #580). +/// +/// Returns the operand unchanged when its type carries no runtime lifetime state, so callers can +/// detect "nothing was pinned" by comparing value ids. +pub(crate) fn acquire_lifetime_pin_if_refcounted( + ctx: &mut LoweringContext<'_, '_>, + value: LoweredValue, + span: Option, +) -> LoweredValue { + let php_type = ctx.builder.value_php_type(value.value); + if Ownership::php_type_needs_lifetime_tracking(&php_type) { + return ctx.emit_value( + Op::Acquire, + vec![value.value], + Some(crate::ir::Immediate::Bool(true)), + php_type, + Op::Acquire.default_effects(), + span, + ); + } + value +} + /// Emits a type-gated release; the backend filters the value's ownership state. pub(crate) fn release_if_owned(ctx: &mut LoweringContext<'_, '_>, value: LoweredValue, span: Option) { let php_type = ctx.builder.value_php_type(value.value); diff --git a/src/ir_lower/stmt/control_exit.rs b/src/ir_lower/stmt/control_exit.rs index afcf2c7920..9c92cc71a2 100644 --- a/src/ir_lower/stmt/control_exit.rs +++ b/src/ir_lower/stmt/control_exit.rs @@ -291,6 +291,12 @@ pub(super) fn emit_innermost_loop_cleanups(ctx: &mut LoweringContext<'_, '_>, co if let Some(cleanup) = frame.cleanup { crate::ir_lower::ownership::release_if_owned(ctx, cleanup.value, Some(cleanup.span)); } + // A by-reference `foreach` over an element source holds a lifetime reference on the + // element for the whole loop; leaving through `break N`, `return`, or `throw` never + // reaches the exit block that would drop it, so drop it here (issue #580). + if let Some(pin) = frame.source_pin { + crate::ir_lower::ownership::release_if_owned(ctx, pin.value, Some(pin.span)); + } } } @@ -335,4 +341,3 @@ pub(super) fn pop_finally_frame_if_active(ctx: &mut LoweringContext<'_, '_>, dep ctx.finally_stack.pop(); } } - diff --git a/src/ir_lower/stmt/loops.rs b/src/ir_lower/stmt/loops.rs index 6e5d909c3e..c78fb2fdfb 100644 --- a/src/ir_lower/stmt/loops.rs +++ b/src/ir_lower/stmt/loops.rs @@ -39,6 +39,7 @@ pub(super) fn lower_while( break_block: exit, continue_block: header, cleanup: None, + source_pin: None, }); lower_block(ctx, body); ctx.loop_stack.pop(); @@ -65,6 +66,7 @@ pub(super) fn lower_do_while( break_block: exit, continue_block: cond_block, cleanup: None, + source_pin: None, }); lower_block(ctx, body); ctx.loop_stack.pop(); @@ -132,6 +134,7 @@ pub(super) fn lower_for( break_block: exit, continue_block: update_block, cleanup: None, + source_pin: None, }); lower_block(ctx, body); ctx.loop_stack.pop(); @@ -145,4 +148,3 @@ pub(super) fn lower_for( ctx.builder.position_at_end(exit); ctx.clear_static_callable_locals(); } - diff --git a/src/ir_lower/stmt/mod.rs b/src/ir_lower/stmt/mod.rs index cf850e4b87..d617291184 100644 --- a/src/ir_lower/stmt/mod.rs +++ b/src/ir_lower/stmt/mod.rs @@ -22,7 +22,8 @@ use crate::ir_lower::effects_lookup; use crate::ir_lower::expr::{ array_access_element_result_type, coerce_container_to_mixed_payload, coerce_to_int_at_span, index_expr_key_type, lower_array_access_from_lowered_receiver, - lower_callable_array_for_assignment, lower_array_literal_with_expected_type, + lower_by_ref_foreach_element_source, lower_callable_array_for_assignment, + lower_array_literal_with_expected_type, lower_closure_for_assignment, lower_expr, reflection_arg_array_binding_for_expr, reflection_class_binding_for_expr, reflection_function_binding_for_expr, reflection_method_binding_for_expr, diff --git a/src/ir_lower/stmt/switches.rs b/src/ir_lower/stmt/switches.rs index a25e6ddd45..5a1097f9c9 100644 --- a/src/ir_lower/stmt/switches.rs +++ b/src/ir_lower/stmt/switches.rs @@ -184,6 +184,7 @@ pub(super) fn lower_switch_bodies( break_block: exit, continue_block: exit, cleanup: None, + source_pin: None, }); for index in 0..=cases.len() { if default.is_some() && default_index == index { @@ -261,4 +262,3 @@ pub(super) fn switch_next_body_block( pub(super) fn span_is_before(span: Span, pivot: Span) -> bool { span.line < pivot.line || (span.line == pivot.line && span.col < pivot.col) } - diff --git a/src/ir_lower/stmt/typed_foreach.rs b/src/ir_lower/stmt/typed_foreach.rs index 1eda1b62b8..6ad8cd5de2 100644 --- a/src/ir_lower/stmt/typed_foreach.rs +++ b/src/ir_lower/stmt/typed_foreach.rs @@ -87,7 +87,8 @@ pub(super) fn lower_foreach( // Apply the checker-computed loop header contract before lowering the source expression so // an iterated-and-mutated array is loaded with its stable payload representation. apply_loop_storage_contracts(ctx, loop_span, Some(array.span)); - let source = lower_expr(ctx, array); + let (source, source_is_borrowed_element) = + lower_foreach_source(ctx, array, value_by_ref); let source_php_ty = ctx.builder.value_php_type(source.value); let source_ty = source_php_ty.codegen_repr(); let key_needs_null_init = key_var.is_some_and(|name| !ctx.local_slots.contains_key(name)); @@ -113,6 +114,15 @@ pub(super) fn lower_foreach( Op::IterStart.default_effects(), Some(array.span), ); + // Take the loop's own lifetime reference on a borrowed element source, AFTER `IterStart`. + // The order is the whole point: `IterStart` splits a by-reference source through + // `__rt_array_ensure_unique`, so a pin taken before it would put the element back at + // refcount 2 and hand the loop a private copy — the very miscompile issue #580 fixes. + // Taken here, the split has already happened and the iterator has already captured the + // pointer, so the pin only keeps that storage alive. + let source_pin = source_is_borrowed_element + .then(|| pin_by_ref_foreach_element_source(ctx, source, array.span)) + .flatten(); if let Some(key_var) = key_var { initialize_foreach_mixed_local_if_needed(ctx, key_var, key_needs_null_init, array.span); } @@ -174,6 +184,7 @@ pub(super) fn lower_foreach( break_block: exit, continue_block: header, cleanup, + source_pin, }); if let Some(key_var) = key_var { let key = ctx.emit_value( @@ -223,6 +234,75 @@ pub(super) fn lower_foreach( if ctx.value_is_owning_temporary(source) { crate::ir_lower::ownership::release_if_owned(ctx, source, Some(array.span)); } + // Normal termination is the exit this block IS, so the pin is dropped here. Every other way + // out — `break`, `break N`, `return`, `throw` — skips this block and is covered by + // `emit_innermost_loop_cleanups` through the loop frame instead. + if let Some(pin) = source_pin { + crate::ir_lower::ownership::release_if_owned(ctx, pin.value, Some(pin.span)); + } +} + +/// Lowers the `foreach` source expression under the loop's binding mode. +/// +/// A by-value loop iterates a copy, so it keeps the ordinary retaining read: the extra +/// reference is what makes `__rt_array_ensure_unique` copy, and copying is precisely the +/// semantics. A by-reference loop mutates the source in place, so an array-element source is +/// fetched for writing instead — otherwise the read's own reference makes the runtime copy the +/// element, and every write lands in a copy the loop then drops (issue #580). +/// +/// Returns the lowered source together with whether it came back borrowed from the +/// fetch-for-write path, which is what tells the caller the loop still owes it a lifetime +/// reference of its own. +fn lower_foreach_source( + ctx: &mut LoweringContext<'_, '_>, + array: &Expr, + value_by_ref: bool, +) -> (LoweredValue, bool) { + if value_by_ref { + if let ExprKind::ArrayAccess { + array: receiver, + index, + } = &array.kind + { + let source = lower_by_ref_foreach_element_source(ctx, receiver, index, array); + let is_borrowed_element = + ctx.builder.value_ownership(source.value) == Ownership::Borrowed; + return (source, is_borrowed_element); + } + } + (lower_expr(ctx, array), false) +} + +/// Takes the by-reference loop's own lifetime reference on a borrowed element source. +/// +/// `Op::ArrayGetForWrite` hands back the parent's element without a reference of its own: the +/// parent's element slot is the only owner. That is exactly what makes the writes land in the +/// parent — and exactly what leaves the iterator holding a dangling pointer as soon as the body +/// drops that parent (`$a = []`, `unset($a)`, a reassignment through an alias). PHP does not +/// have this problem because its by-reference `foreach` holds a reference to the iterated array +/// itself, so the array outlives the variable it came from. +/// +/// Must be called AFTER `Op::IterStart`: that instruction runs `__rt_array_ensure_unique` on a +/// by-reference source, and an extra reference held across it would make the split fire and give +/// the loop a private copy to write into. +/// +/// Returns `None` when the source type carries no runtime lifetime state, in which case there is +/// nothing to pin and nothing to release. +fn pin_by_ref_foreach_element_source( + ctx: &mut LoweringContext<'_, '_>, + source: LoweredValue, + span: Span, +) -> Option { + let pin = + crate::ir_lower::ownership::acquire_lifetime_pin_if_refcounted(ctx, source, Some(span)); + if pin.value == source.value { + return None; + } + // The acquire result is the loop's own reference, so pin it `Owned` explicitly instead of + // leaving it at the `MaybeOwned` default: the cleanup paths below release through + // `release_if_owned`, which the backend filters on this very state. + ctx.builder.set_value_ownership(pin.value, Ownership::Owned); + Some(LoopCleanup { value: pin, span }) } /// Returns the by-value foreach local type when Phase 04 can keep a concrete element. @@ -269,4 +349,3 @@ pub(super) fn initialize_foreach_mixed_local_if_needed( let boxed = ctx.box_value_as_mixed(null, PhpType::Mixed, Some(span)); ctx.store_foreach_initializer_local_only(name, boxed, PhpType::Mixed, Some(span)); } - diff --git a/src/ir_passes/peephole/acquire_release.rs b/src/ir_passes/peephole/acquire_release.rs index f4185fd567..0e947ca250 100644 --- a/src/ir_passes/peephole/acquire_release.rs +++ b/src/ir_passes/peephole/acquire_release.rs @@ -14,6 +14,10 @@ //! - The single-use guard makes this safe regardless of how far apart the two //! ops are or which path the `Release` sits on: the value flows to exactly one //! `Release`, so removing both cannot leak or double-free. +//! - It does NOT make the raised refcount unobservable, which is why an `Acquire` +//! carrying the lifetime-pin marker is skipped: such a pair exists precisely +//! because something between the two ops may release the value's other owner +//! (issue #580). use std::collections::HashMap; @@ -31,6 +35,13 @@ pub(super) fn collect(function: &Function, rewrites: &mut Rewrites) { if inst.op != Op::Acquire { continue; } + // A lifetime pin is an acquire whose result is deliberately never read: it exists so + // the value survives an interval in which its other owner may go away. Cancelling it + // against its release would leave that interval running on freed storage, so the + // marker opts the pair out of this rewrite entirely. + if inst.immediate.is_some() { + continue; + } let Some(acquired) = inst.result else { continue; }; diff --git a/src/ir_passes/tests/peephole_test.rs b/src/ir_passes/tests/peephole_test.rs index beefaf5e7f..79b24cf6f3 100644 --- a/src/ir_passes/tests/peephole_test.rs +++ b/src/ir_passes/tests/peephole_test.rs @@ -240,6 +240,45 @@ fn multi_use_acquire_is_not_cancelled() { assert!(validate_function(&function).is_ok()); } +/// An acquire carrying the lifetime-pin marker is not cancelled even though its only use is its +/// release: the pin exists so the value survives an interval in which another owner may drop it +/// (a by-reference `foreach` over an array element whose body replaces the parent, issue #580), +/// so the raised refcount in between is exactly what the program observes. +#[test] +fn lifetime_pin_acquire_release_pair_is_not_cancelled() { + let mut function = Function::new("acq_pin".to_string(), IrType::Str, PhpType::Str); + { + let mut builder = Builder::new(&mut function); + let entry = builder.create_named_block("entry", vec![]); + builder.set_entry(entry); + builder.position_at_end(entry); + let x = builder.emit_const_str(DataId::from_raw(0)); + let pinned = builder + .emit( + Op::Acquire, + vec![x], + Some(Immediate::Bool(true)), + IrType::Str, + PhpType::Str, + Ownership::Owned, + ) + .expect("acquire result"); + builder.emit( + Op::Release, + vec![pinned], + None, + IrType::Void, + PhpType::Void, + Ownership::NonHeap, + ); + builder.terminate(Terminator::Return { value: Some(x) }); + } + assert!(!run_peephole(&mut function), "a lifetime pin must not cancel"); + assert_eq!(function.instructions[1].op, Op::Acquire, "the pin acquire is preserved"); + assert_eq!(function.instructions[2].op, Op::Release, "the pin release is preserved"); + assert!(validate_function(&function).is_ok()); +} + // --- redundant load / store -------------------------------------------------- /// Adds a scalar `PhpLocal` slot to the function under construction. diff --git a/tests/codegen/types/iterable/foreach.rs b/tests/codegen/types/iterable/foreach.rs index c4b9e8d7c3..cf6add69db 100644 --- a/tests/codegen/types/iterable/foreach.rs +++ b/tests/codegen/types/iterable/foreach.rs @@ -625,3 +625,645 @@ fn test_iterable_variadic_arg_stays_boxed_in_runtime_array() { ); assert_eq!(out, "[[1,2]]"); } + +// --- Issue #580: by-ref foreach over an array-element source --- +// +// `lower_foreach` read its source with a plain rvalue `lower_expr` regardless of the binding +// mode. For an array element that read is an `array_get`, which hands back the parent's own +// container *with a retain*: the element sits at refcount 2 while the loop runs, so `iter_start` +// copy-on-writes it, and the loop mutates a private copy and drops it. Every write was lost. +// A plain local source worked because loading a local yields the local's storage with no retain, +// leaving refcount 1 and no copy. +// +// The by-ref source now takes a fetch-for-write read, which does the copy-on-write split itself +// — receiver first, then element, publishing each back into the slot it came from — and returns +// the element borrowed. Note that merely dropping the retain would be WORSE than the original +// bug: `__rt_array_ensure_unique` consumes one reference from the shared source when it splits, +// so a read that never took one would have the split cannibalize the parent's own reference. +// The plain read's missing-key warning and null-container sentinel are reused unchanged. +// +// Both receiver kinds are covered. `Op::ArrayGetForWrite` reaches an indexed element slot with +// pointer arithmetic; `Op::HashGetForWrite` takes the matching entry's address from +// `__rt_hash_get` and splits the container that entry holds. The hash half never needed the +// reference BINDING the checker rejects for `$r = &$h['k'];` — that binds a hash slot into a +// local, whereas this only separates storage the parent already owns. + +/// Regression for issue #580: by-ref `foreach` over an indexed element must mutate the parent +/// array in place. Pre-fix the loop ran and assigned `$v` but `$a[0]` stayed `[1, 2]`. +#[test] +fn test_regression_580_by_ref_foreach_over_indexed_element_source() { + let out = compile_and_run( + r#" [1, 2]]; +foreach ($h['a'] as &$w) { $w = $w * 10; } +unset($w); +echo implode(',', $h['a']); +"#, + ); + assert_eq!(out, "10,20"); +} + +/// Regression for issue #580: the mutation must be visible to the parent *during* the loop, +/// not merely published at the end — PHP's by-ref binding aliases the element's storage, so +/// reading the parent mid-loop already reflects the write. +#[test] +fn test_regression_580_by_ref_foreach_element_mutation_visible_during_loop() { + let out = compile_and_run( + r#" &$v) { $v = $v + $k; } +unset($v); +echo implode(',', $a[0]); +"#, + ); + assert_eq!(out, "1,3"); +} + +/// Guard for issue #580: a by-VALUE `foreach` over an element source must NOT mutate the +/// parent. The fix gives only the by-ref source a borrowed read; the by-value source must keep +/// its retaining read, or this loop would start writing through to `$a[0]`. +#[test] +fn test_regression_580_by_value_foreach_over_element_source_does_not_mutate() { + let out = compile_and_run( + r#" $row) { + foreach ($a[$k] as &$v) { $v = $v * 2; } + unset($v); +} +echo implode(',', $a[0]), '|', implode(',', $a[1]); +"#, + ); + assert_eq!(out.stdout, "2,4|6,8"); + assert!( + out.stderr.contains("leak summary: clean"), + "expected a clean heap-debug leak summary, got: {}", + out.stderr + ); +} + +/// Regression for issue #580: a HASH element of an indexed source must be separated with the +/// hash copy-on-write helper, not the indexed-array one. +/// +/// `array` reaches the same fetch-for-write path as a nested indexed array, but its +/// element is hash storage: splitting it with `__rt_array_ensure_unique` would hand a hash +/// pointer to the indexed shallow-clone helper. Pairing the helper with the element's container +/// kind is what keeps this shape correct, and it is what lets the hash RECEIVER path reuse the +/// same helper selection. +#[test] +fn test_regression_580_by_ref_foreach_over_hash_element_of_indexed_source() { + let out = compile_and_run_with_heap_debug( + r#" 1, 'y' => 2]]; +foreach ($a[0] as $k => &$v) { $v = $v * 2; } +unset($v); +echo $a[0]['x'], ',', $a[0]['y']; +"#, + ); + assert_eq!(out.stdout, "2,4"); + assert!( + out.stderr.contains("leak summary: clean"), + "expected a clean heap-debug leak summary, got: {}", + out.stderr + ); +} + +/// Guard for issue #580: a missing index must keep warning and skipping the loop instead of +/// reaching the fetch-for-write path with an out-of-bounds slot address. +/// +/// The fetch-for-write read reuses `array_get`'s bounds and null-container guards precisely so +/// this stays true: the miss takes the warning path and materializes the null-container +/// sentinel, which `iter_start` normalizes and `iter_next` reports as empty (issue #556). A +/// fresh read path that computed the element address first would write through a slot past the +/// end of the array. +#[test] +fn test_regression_580_by_ref_foreach_over_missing_element_index_warns_and_skips() { + let out = compile_and_run_with_heap_debug( + r#" [1, 2]]; +foreach ($h['a'] as &$w) { $w = $w * 10; echo $h['a'][0], ';'; } +unset($w); +echo implode(',', $h['a']); +"#, + ); + assert_eq!(out, "10;10;10,20"); +} + +/// Regression for issue #580: a HASH element of a HASH receiver must be separated with the hash +/// copy-on-write helper, pairing the split helper with the element's container kind on the hash +/// receiver path too. +#[test] +fn test_regression_580_by_ref_foreach_over_hash_element_of_hash_source() { + let out = compile_and_run_with_heap_debug( + r#" ['x' => 1, 'y' => 2]]; +foreach ($h['a'] as $k => &$w) { $w = $w * 10; } +unset($w); +echo $h['a']['x'], ',', $h['a']['y']; +"#, + ); + assert_eq!(out.stdout, "10,20"); + assert!( + out.stderr.contains("leak summary: clean"), + "expected a clean heap-debug leak summary, got: {}", + out.stderr + ); +} + +/// Regression for issue #580: a chain mixing hash and hash levels must fetch every level for +/// write, so the innermost split is published into storage the outer levels really share. +#[test] +fn test_regression_580_by_ref_foreach_over_nested_hash_element_source() { + let out = compile_and_run( + r#" ['b' => [1, 2]]]; +foreach ($h['a']['b'] as &$w) { $w = $w * 10; } +unset($w); +echo implode(',', $h['a']['b']); +"#, + ); + assert_eq!(out, "10,20"); +} + +/// Regression for issue #580: selecting a later key of a multi-entry hash must separate that +/// entry's own slot, not whichever entry the probe happens to land on first. +#[test] +fn test_regression_580_by_ref_foreach_hash_element_source_selects_the_right_entry() { + let out = compile_and_run( + r#" [1, 2], 'b' => [3, 4]]; +foreach ($h['b'] as &$w) { $w = $w * 10; } +unset($w); +echo implode(',', $h['a']), '|', implode(',', $h['b']); +"#, + ); + assert_eq!(out, "1,2|30,40"); +} + +/// Guard for issue #580: a by-VALUE `foreach` over a hash element must NOT mutate the parent. +/// Only the by-ref source takes the fetch-for-write read. +#[test] +fn test_regression_580_by_value_foreach_over_hash_element_does_not_mutate() { + let out = compile_and_run( + r#" [1, 2]]; +foreach ($h['a'] as $w) { $w = $w * 10; } +echo implode(',', $h['a']); +"#, + ); + assert_eq!(out, "1,2"); +} + +/// Guard for issue #580: a missing hash KEY must keep warning and skipping the loop instead of +/// separating a slot the probe never found. `__rt_hash_get` reports the miss with a null entry +/// address, which the fetch-for-write read routes to the same warning and null-container +/// sentinel `hash_get` produces. +#[test] +fn test_regression_580_by_ref_foreach_over_missing_hash_key_warns_and_skips() { + let out = compile_and_run_with_heap_debug( + r#" [1, 2]]; +foreach ($h['zz'] as &$w) { $w = $w * 10; } +echo 'after:', implode(',', $h['a']); +"#, + ); + assert_eq!(out.stdout, "after:1,2"); + assert!( + out.stderr.contains("Undefined array key \"zz\""), + "expected the missing-key warning to survive the fetch-for-write read, got: {}", + out.stderr + ); + assert!( + out.stderr.contains("leak summary: clean"), + "expected a clean heap-debug leak summary, got: {}", + out.stderr + ); +} + +/// Regression for issue #580: the hash element must reach the loop mutable when a SECOND owner +/// holds it, the case where a plain non-retaining read would have the split cannibalize the +/// parent's own reference. Mirrors the indexed shared-owner regression. +#[test] +fn test_regression_580_by_ref_foreach_hash_element_shared_with_another_owner() { + let out = compile_and_run_with_heap_debug( + r#" [1, 2]]; +$keep = $h['a']; +foreach ($h['a'] as &$w) { $w = $w * 10; } +unset($w); +echo implode(',', $h['a']), '|', implode(',', $keep); +"#, + ); + assert_eq!(out.stdout, "10,20|1,2"); + assert!( + out.stderr.contains("leak summary: clean"), + "expected a clean heap-debug leak summary, got: {}", + out.stderr + ); +} + +/// Regression for issue #580: the loop's lifetime pin must cover the hash receiver path too. +/// +/// `HashGetForWrite` hands the loop the entry's own container, borrowed, so dropping the parent +/// hash mid-body would leave the iterator on freed storage exactly as it did for an indexed +/// receiver. The pin is keyed on the source's `Borrowed` ownership rather than on the specific +/// op, so it applies here unchanged — this test is what holds that true. +#[test] +fn test_regression_580_by_ref_foreach_hash_element_source_survives_parent_replacement() { + let out = compile_and_run_with_heap_debug( + r#" [1, 2]]; +foreach ($h['a'] as &$w) { + echo $w, ','; + if ($w === 1) $h = []; + $w *= 10; +} +unset($w); +echo 'done'; +"#, + ); + assert_eq!(out.stdout, "1,2,done"); + assert!( + out.stderr.contains("leak summary: clean"), + "expected a clean heap-debug leak summary, got: {}", + out.stderr + ); +} + +/// Guard for issue #580: separating the hash element must separate the RECEIVER first, so a +/// second owner of the hash itself keeps observing the unmutated storage. +#[test] +fn test_regression_580_by_ref_foreach_hash_element_separates_shared_receiver() { + let out = compile_and_run( + r#" [1, 2]]; +$copy = $h; +foreach ($h['a'] as &$w) { $w = $w * 10; } +unset($w); +echo implode(',', $h['a']), '|', implode(',', $copy['a']); +"#, + ); + assert_eq!(out, "10,20|1,2"); +} + +/// Regression for issue #580: the RECEIVER must be separated too, so the write is not visible +/// through an alias of the parent array. +/// +/// PHP separates `$a` on the way to separating `$a[0]`, and elephc's element WRITE path already +/// does (`$a[0] = [9, 9]` splits the receiver inside `__rt_array_set_*`). Fetch-for-write +/// publishes a new element pointer into the receiver's payload, so it owes the same guarantee: +/// without it, `$b` — which shares the outer container — would observe the loop's writes. +#[test] +fn test_regression_580_by_ref_foreach_element_source_separates_aliased_parent() { + let out = compile_and_run_with_heap_debug( + r#"getMessage(), ':'; +} +unset($v); +echo implode(',', $a[0]); +"#, + ); + assert_eq!(out.stdout, "stop:2,4,3"); + assert!( + out.stderr.contains("leak summary: clean"), + "expected a clean heap-debug leak summary, got: {}", + out.stderr + ); +}