From edac4e62f60cc4ef597a39b9ef9582ec3aef4383 Mon Sep 17 00:00:00 2001 From: Guillaume Loulier Date: Sat, 27 Jun 2026 15:56:03 +0200 Subject: [PATCH] fix: preserve foreach value ownership for concrete array element types Fixes #405: appending a foreach value into a new array and returning it corrupted the array because the foreach value was always typed as Mixed, forcing array_to_mixed conversion and a type mismatch with the caller's expected element layout. Three changes: 1. foreach_value_type now returns the concrete element type (Str, Int, Bool, Float, Callable) for typed arrays instead of always Mixed. Non-scalar element types (Iterable, Object, etc.) still widen to Mixed. 2. lower_iter_current_value persists the borrowed string slot from an indexed array into an owned heap copy when the result type is Str, so the subsequent release_if_owned does not free the source array's element. For DynamicIterable/Hash paths that always produce a Mixed box, a new unbox_dynamic_iter_current_value_if_needed helper unboxes the Mixed cell into the concrete result type. 3. initialize_foreach_concrete_local_if_needed initializes concrete-typed foreach variables (Str, Int, Bool, Float) to a safe empty value before the first iteration, preventing the first release from reading uninitialized stack memory. Regression tests cover foreach value append with function return, literal array source, inline append, iterating the result, and integer arrays. --- src/codegen_ir/lower_inst/iterators.rs | 88 +++++++++++++++++++- src/ir_lower/stmt/mod.rs | 70 +++++++++++++++- tests/codegen/types/iterable/foreach.rs | 102 ++++++++++++++++++++++++ 3 files changed, 253 insertions(+), 7 deletions(-) diff --git a/src/codegen_ir/lower_inst/iterators.rs b/src/codegen_ir/lower_inst/iterators.rs index c348eba714..d75da55def 100644 --- a/src/codegen_ir/lower_inst/iterators.rs +++ b/src/codegen_ir/lower_inst/iterators.rs @@ -183,6 +183,7 @@ pub(super) fn lower_iter_current_value( let iterator = expect_operand(inst, 0)?; let offset = ctx.value_frame_offset(iterator)?; let result_ty = iter_current_result_type(ctx, inst)?; + let mut produced_mixed_box = false; match iterator_source_kind(ctx, iterator, inst)? { IteratorSourceKind::Indexed { elem } => { match ctx.emitter.target.arch { @@ -190,13 +191,24 @@ pub(super) fn lower_iter_current_value( Arch::X86_64 => load_current_array_value_x86_64(ctx, offset, &elem)?, } box_current_indexed_value_if_needed(ctx, &elem, &result_ty)?; + // The indexed iterator loads a borrowed string slot directly from the + // source array. When the EIR result type is Str (not boxed Mixed), the + // value is treated as an owning temporary by the lowering, so persist + // it into an owned heap copy before the caller releases it. + if elem.codegen_repr() == PhpType::Str && result_ty.codegen_repr() == PhpType::Str { + abi::emit_call_label(ctx.emitter, "__rt_str_persist"); + } + } + IteratorSourceKind::Hash => { + match ctx.emitter.target.arch { + Arch::AArch64 => load_current_hash_value_as_mixed_aarch64(ctx, offset), + Arch::X86_64 => load_current_hash_value_as_mixed_x86_64(ctx, offset), + } + produced_mixed_box = true; } - IteratorSourceKind::Hash => match ctx.emitter.target.arch { - Arch::AArch64 => load_current_hash_value_as_mixed_aarch64(ctx, offset), - Arch::X86_64 => load_current_hash_value_as_mixed_x86_64(ctx, offset), - }, IteratorSourceKind::DynamicIterable | IteratorSourceKind::DynamicMixed => { lower_dynamic_iter_current_value(ctx, inst, offset)?; + produced_mixed_box = true; } IteratorSourceKind::Object { class_name, .. } => { let return_ty = emit_object_iterator_method_call(ctx, offset, &class_name, "current")?; @@ -207,6 +219,9 @@ pub(super) fn lower_iter_current_value( box_iterator_method_result_if_needed(ctx, inst, &return_ty)?; } } + if produced_mixed_box { + unbox_dynamic_iter_current_value_if_needed(ctx, &result_ty)?; + } store_if_result(ctx, inst) } @@ -218,6 +233,71 @@ fn iter_current_result_type(ctx: &FunctionContext<'_>, inst: &Instruction) -> Re Ok(ctx.value_php_type(result)?.codegen_repr()) } +/// Unboxes a runtime-produced Mixed cell into the concrete EIR result type when the +/// iterator value has a known concrete type (e.g. `string` from `array`) +/// but the runtime lowering path always produces a boxed Mixed pointer. +fn unbox_dynamic_iter_current_value_if_needed( + ctx: &mut FunctionContext<'_>, + result_ty: &PhpType, +) -> Result<()> { + if matches!(result_ty.codegen_repr(), PhpType::Mixed | PhpType::Union(_) | PhpType::Void | PhpType::Never) { + return Ok(()); + } + let result_reg = abi::int_result_reg(ctx.emitter); + match ctx.emitter.target.arch { + Arch::AArch64 => { + ctx.emitter.instruction("mov x9, x0"); // save the Mixed box pointer across the unbox call + ctx.emitter.instruction("mov x0, x9"); // pass the Mixed box pointer to the unbox helper + abi::emit_call_label(ctx.emitter, "__rt_mixed_unbox"); + // x0 = tag, x1 = lo, x2 = hi + // AArch64 string result: x1 = ptr, x2 = len — already in place + match result_ty.codegen_repr() { + PhpType::Str => { + // x1 and x2 already hold the string pointer and length + } + PhpType::Int | PhpType::Bool | PhpType::Callable => { + if result_reg != "x1" { + ctx.emitter.instruction(&format!("mov {}, x1", result_reg)); // move the unboxed scalar payload into the integer result register + } + } + PhpType::Float => { + ctx.emitter.instruction("fmov d0, x1"); // move the unboxed float bits into the float result register + } + other if other.is_refcounted() => { + if result_reg != "x1" { + ctx.emitter.instruction(&format!("mov {}, x1", result_reg)); // move the unboxed refcounted pointer into the integer result register + } + } + _ => {} + } + } + Arch::X86_64 => { + ctx.emitter.instruction("mov r10, rax"); // save the Mixed box pointer across the unbox call + ctx.emitter.instruction("mov rax, r10"); // pass the Mixed box pointer to the unbox helper + abi::emit_call_label(ctx.emitter, "__rt_mixed_unbox"); + // rax = tag, rdi = lo, rdx = hi (x86_64 __rt_mixed_unbox output convention) + match result_ty.codegen_repr() { + PhpType::Str => { + // x86_64 string result: rax = ptr, rdx = len + ctx.emitter.instruction("mov rax, rdi"); // move the unboxed string pointer into the string result register + // rdx already holds the length from __rt_mixed_unbox + } + PhpType::Int | PhpType::Bool | PhpType::Callable => { + ctx.emitter.instruction("mov rax, rdi"); // move the unboxed scalar payload into the integer result register + } + PhpType::Float => { + ctx.emitter.instruction("movq xmm0, rdi"); // move the unboxed float bits into the float result register + } + other if other.is_refcounted() => { + ctx.emitter.instruction("mov rax, rdi"); // move the unboxed refcounted pointer into the integer result register + } + _ => {} + } + } + } + Ok(()) +} + /// Boxes an indexed iterator element only when the EIR result expects `Mixed`. fn box_current_indexed_value_if_needed( ctx: &mut FunctionContext<'_>, diff --git a/src/ir_lower/stmt/mod.rs b/src/ir_lower/stmt/mod.rs index 8af509c06c..f4d61183ca 100644 --- a/src/ir_lower/stmt/mod.rs +++ b/src/ir_lower/stmt/mod.rs @@ -1081,8 +1081,7 @@ fn lower_foreach( if value_ty == PhpType::Mixed { initialize_foreach_mixed_local_if_needed(ctx, value_var, value_needs_null_init, array.span); } else if value_needs_null_init { - ctx.declare_local(value_var, value_ty.clone()); - ctx.set_local_type(value_var, value_ty); + initialize_foreach_concrete_local_if_needed(ctx, value_var, value_ty.clone(), array.span); } } let header = ctx.builder.create_named_block("foreach.next", Vec::new()); @@ -1170,7 +1169,8 @@ fn lower_foreach( /// Returns the by-value foreach local type when Phase 04 can keep a concrete element. fn foreach_value_type(source_ty: &PhpType) -> PhpType { match source_ty.codegen_repr() { - PhpType::Array(elem) if elem.codegen_repr() == PhpType::Callable => PhpType::Callable, + PhpType::Array(elem) => foreach_element_value_type(elem.codegen_repr()), + PhpType::AssocArray { value, .. } => foreach_element_value_type(value.codegen_repr()), PhpType::Object(class_name) if class_name == "Phar" || class_name == "PharData" => { PhpType::Object("PharFileInfo".to_string()) } @@ -1178,6 +1178,15 @@ fn foreach_value_type(source_ty: &PhpType) -> PhpType { } } +/// Returns the foreach value type for a concrete array element type, widening +/// non-scalar element types to `Mixed` so the runtime always boxes them. +fn foreach_element_value_type(elem_ty: PhpType) -> PhpType { + match elem_ty { + PhpType::Str | PhpType::Int | PhpType::Bool | PhpType::Float | PhpType::Callable => elem_ty, + _ => PhpType::Mixed, + } +} + /// Returns the local value type used when a foreach binds the value by reference. fn foreach_ref_value_type(source_ty: &PhpType) -> PhpType { match source_ty.codegen_repr() { @@ -1214,6 +1223,61 @@ fn initialize_foreach_mixed_local_if_needed( ctx.store_local(name, boxed, PhpType::Mixed, Some(span)); } +/// Initializes a fresh foreach loop variable to a safe empty value before the first +/// iteration when the foreach value has a concrete type (e.g. `string` from +/// `array`). Without this, the first `release` of the previous slot value +/// would read uninitialized stack memory. +fn initialize_foreach_concrete_local_if_needed( + ctx: &mut LoweringContext<'_, '_>, + name: &str, + value_ty: PhpType, + span: Span, +) { + ctx.declare_local(name, value_ty.clone()); + ctx.set_local_type(name, value_ty.clone()); + match value_ty.codegen_repr() { + PhpType::Str => { + let empty_str_id = ctx.intern_string(""); + let empty = ctx.emit_value( + Op::ConstStr, + Vec::new(), + Some(Immediate::Data(empty_str_id)), + PhpType::Str, + Op::ConstStr.default_effects(), + Some(span), + ); + ctx.store_local(name, empty, PhpType::Str, Some(span)); + } + PhpType::Int | PhpType::Bool => { + let zero = ctx.emit_value( + Op::ConstI64, + Vec::new(), + Some(Immediate::I64(0)), + value_ty.clone(), + Op::ConstI64.default_effects(), + Some(span), + ); + ctx.store_local(name, zero, value_ty, Some(span)); + } + PhpType::Float => { + let zero = ctx.emit_value( + Op::ConstF64, + Vec::new(), + Some(Immediate::F64(0.0)), + value_ty.clone(), + Op::ConstF64.default_effects(), + Some(span), + ); + ctx.store_local(name, zero, value_ty, Some(span)); + } + _ => { + // For other refcounted or complex types, fall back to null. + let null = emit_null_value(ctx, Some(span)); + ctx.store_local(name, null, value_ty, Some(span)); + } + } +} + /// Lowers a `switch` with source-ordered pattern evaluation and PHP fallthrough. fn lower_switch( ctx: &mut LoweringContext<'_, '_>, diff --git a/tests/codegen/types/iterable/foreach.rs b/tests/codegen/types/iterable/foreach.rs index 034c6282fe..de64b5274b 100644 --- a/tests/codegen/types/iterable/foreach.rs +++ b/tests/codegen/types/iterable/foreach.rs @@ -609,3 +609,105 @@ fn test_iterable_variadic_arg_stays_boxed_in_runtime_array() { ); assert_eq!(out, "[[1,2]]"); } + +/// Regression test for issue #405: appending a foreach value into a new array and +/// returning it from a function must not corrupt the array. The foreach value is a +/// borrowed string slot from the source array; without an owned copy the appended +/// elements dangle after the source temporary is freed. +#[test] +fn test_foreach_value_append_returned_from_function() { + let out = compile_and_run( + "