From c6ef2a34b2e8d7578da9ac4bcdacf4a8ce5ba74c Mon Sep 17 00:00:00 2001 From: mirchaemanuel Date: Tue, 7 Jul 2026 11:30:09 +0200 Subject: [PATCH] fix(ir): release the producer's reference when boxing an owned value as Mixed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #484. __rt_mixed_from_value retains refcounted payloads (objects, arrays, hashes, callables, nested cells) and persists strings, so the boxed cell always carries its own reference or copy — but EIR lowering treated Op::MixedBox as consuming its operand and never released the producer's reference. Boxing an owning temporary therefore left the payload one reference high forever: 'function g(): ?P { return new P(); }' leaked one P per call (and fresh arrays boxed on return leaked identically) even though the boxed cell itself was freed on rebind. A new LoweringContext::box_value_as_mixed helper emits the MixedBox and then releases the operand when it is an owning temporary (same ownership pattern as the throw-of-borrowed fix in #477); the 11 value-carrying emission sites now go through it. Borrowed operands (a boxed local) are untouched — the box's retain is their +1 — and concat-scratch strings remain exempt from heap release, so string boxing behavior is unchanged (its separate persisted-copy leak is issue #485). The two remaining direct emission sites box freshly-created nulls (non-refcounted) and need no release. Regression tests in tests/codegen/runtime_gc/regressions.rs for the object and array flavours. Broad suites green (1872 codegen across callables/closures/ runtime_gc/control/types/exceptions/arrays/oop/generators + 993 error tests). --- CHANGELOG.md | 1 + src/ir_lower/context.rs | 27 +++++++++ src/ir_lower/expr/mod.rs | 74 ++++--------------------- src/ir_lower/function.rs | 3 + src/ir_lower/stmt/mod.rs | 27 +-------- tests/codegen/runtime_gc/regressions.rs | 52 +++++++++++++++++ 6 files changed, 96 insertions(+), 88 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a257441553..fc8d92b77f 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 a per-call heap leak when an owned refcounted value is boxed into a Mixed cell (issue #484): the runtime retains the payload for the box, but the lowering never released the producer's own reference, so `function g(): ?P { return new P(); }` leaked one object per call (and fresh arrays boxed the same way leaked identically) even though the boxed cell itself was freed. Boxing now releases the producer's reference for owning temporaries; borrowed values (e.g. a boxed local) are untouched. - Int-backed enum `from()` / `tryFrom()` now accept a dynamically-typed (`mixed`) argument (issue #449): a `foreach` value over a heterogeneous array, an untyped parameter, etc. are coerced on their runtime type before the enum lookup — integer/numeric-string resolve (or throw `ValueError`), float truncates, bool/null coerce, and array/object/resource/closure throw `TypeError` naming the given type. Previously any `mixed` argument was rejected at compile time. Target-aware on every supported backend. - Int-backed enum `from()` / `tryFrom()` now accept a numeric string (issue #349): `Level::from("1")` coerces the string to the integer backing value (as a distinct EIR coercion lowered before the enum call) and returns the matching case, instead of being rejected at compile time. A numeric string with no matching case throws `ValueError`; a non-numeric string (e.g. `"x"`) throws `TypeError` with PHP's exact argument-type message — matching PHP's coercive typing on every supported target, including PHP-rejected libc `strtod` extensions such as hexadecimal `"0x1"`, `"INF"`, and `"NAN"`. - Fixed an enum `from()` / `tryFrom()` refcount bug (surfaced while fixing #349): the returned case singleton was under-retained, so storing the result into a reassigned variable inside a loop drove the persistent singleton's refcount to zero and freed it — producing garbage reads or a heap crash after a few iterations. `from()`/`tryFrom()` now retain the matched singleton, keeping it alive like direct case access. Affected both backed-enum backings. diff --git a/src/ir_lower/context.rs b/src/ir_lower/context.rs index 1f3eedfdfa..b516076951 100644 --- a/src/ir_lower/context.rs +++ b/src/ir_lower/context.rs @@ -1340,6 +1340,33 @@ impl<'m, 'f> LoweringContext<'m, 'f> { ); } + /// Boxes a value into a Mixed cell and releases the producer's reference when the + /// operand is an owning temporary. `__rt_mixed_from_value` retains refcounted payloads + /// (objects, arrays, hashes, callables, nested cells) and persists strings, so the + /// boxed cell always carries its own reference or copy; keeping the producer's + /// reference too leaked one payload per boxing (issue #484). Borrowed operands (e.g. + /// a loaded local) are left untouched — the box's retain is their +1. + pub(crate) fn box_value_as_mixed( + &mut self, + value: LoweredValue, + php_type: PhpType, + span: Option, + ) -> LoweredValue { + let release_source = self.value_is_owning_temporary(value); + let boxed = self.emit_value( + Op::MixedBox, + vec![value.value], + None, + php_type, + Op::MixedBox.default_effects(), + span, + ); + if release_source { + crate::ir_lower::ownership::release_if_owned(self, value, span); + } + boxed + } + /// Emits a value-producing opcode with computed storage and ownership metadata. pub(crate) fn emit_value( &mut self, diff --git a/src/ir_lower/expr/mod.rs b/src/ir_lower/expr/mod.rs index 17089c37eb..97c7affb52 100644 --- a/src/ir_lower/expr/mod.rs +++ b/src/ir_lower/expr/mod.rs @@ -256,14 +256,7 @@ fn lower_null(ctx: &mut LoweringContext<'_, '_>, expr: &Expr) -> LoweredValue { /// Lowers a nullsafe expression that is known to short-circuit to PHP null. fn lower_boxed_null(ctx: &mut LoweringContext<'_, '_>, expr: &Expr) -> LoweredValue { let null = lower_null(ctx, expr); - ctx.emit_value( - Op::MixedBox, - vec![null.value], - None, - PhpType::Mixed, - Op::MixedBox.default_effects(), - Some(expr.span), - ) + ctx.box_value_as_mixed(null, PhpType::Mixed, Some(expr.span)) } /// Lowers a binary operation. @@ -2827,14 +2820,7 @@ fn lower_named_descriptor_invoker_arg_container( } } } - ctx.emit_value( - Op::MixedBox, - vec![hash.value], - None, - PhpType::Mixed, - Op::MixedBox.default_effects(), - Some(span), - ) + ctx.box_value_as_mixed(hash, PhpType::Mixed, Some(span)) } /// Returns the variable name when this literal argument should be passed by reference. @@ -3635,14 +3621,7 @@ fn build_bound_closure_binding( return None; } let new_this_value = lower_expr(ctx, &new_this); - let boxed_this = ctx.emit_value( - Op::MixedBox, - vec![new_this_value.value], - None, - PhpType::Mixed, - Op::MixedBox.default_effects(), - Some(expr.span), - ); + let boxed_this = ctx.box_value_as_mixed(new_this_value, PhpType::Mixed, Some(expr.span)); signature.return_type = result_type; let bound = StaticCallableBinding::Closure { name, @@ -5717,14 +5696,7 @@ fn coerce_variadic_tail_value( if ctx.builder.value_php_type(value.value).codegen_repr() == PhpType::Mixed { return value; } - ctx.emit_value( - Op::MixedBox, - vec![value.value], - None, - PhpType::Mixed, - Op::MixedBox.default_effects(), - Some(span), - ) + ctx.box_value_as_mixed(value, PhpType::Mixed, Some(span)) } /// Returns true when a call argument uses unpacking syntax. @@ -8078,14 +8050,7 @@ fn lower_untyped_descriptor_invoker_hash_container( } } } - ctx.emit_value( - Op::MixedBox, - vec![hash.value], - None, - PhpType::Mixed, - Op::MixedBox.default_effects(), - Some(span), - ) + ctx.box_value_as_mixed(hash, PhpType::Mixed, Some(span)) } /// Copies an indexed spread source into a descriptor-invoker hash with numeric keys. @@ -8202,14 +8167,7 @@ fn coerce_descriptor_invoker_mixed_value( if ctx.builder.value_php_type(value.value).codegen_repr() == PhpType::Mixed { return value; } - ctx.emit_value( - Op::MixedBox, - vec![value.value], - None, - PhpType::Mixed, - Op::MixedBox.default_effects(), - Some(span), - ) + ctx.box_value_as_mixed(value, PhpType::Mixed, Some(span)) } /// Returns the result storage type for an indirect callable with no static signature. @@ -9053,14 +9011,7 @@ fn lower_nullsafe_method_call( ctx.builder.position_at_end(null_block); let null_value = lower_null(ctx, expr); let null_value = if result_type.codegen_repr() == PhpType::Mixed { - ctx.emit_value( - Op::MixedBox, - vec![null_value.value], - None, - result_type.clone(), - Op::MixedBox.default_effects(), - Some(expr.span), - ) + ctx.box_value_as_mixed(null_value, result_type.clone(), Some(expr.span)) } else { null_value }; @@ -10015,6 +9966,8 @@ fn lower_yield_from_array( Some(span), ) .expect("const_null produces a value"); + // A fresh null is non-refcounted: there is no producer reference to release, + // so this boxes directly rather than via box_value_as_mixed (issue #484). ctx.emit_value( Op::MixedBox, vec![null_value], @@ -10284,14 +10237,7 @@ fn coerce_value_for_temp( return value; } match target_ty { - PhpType::Mixed => ctx.emit_value( - Op::MixedBox, - vec![value.value], - None, - PhpType::Mixed, - Op::MixedBox.default_effects(), - Some(span), - ), + PhpType::Mixed => ctx.box_value_as_mixed(value, PhpType::Mixed, Some(span)), PhpType::Int | PhpType::Bool | PhpType::Void | PhpType::Never => { coerce_to_int_at_span(ctx, value, Some(span)) } diff --git a/src/ir_lower/function.rs b/src/ir_lower/function.rs index 531b8c4fdb..55dd295679 100644 --- a/src/ir_lower/function.rs +++ b/src/ir_lower/function.rs @@ -789,6 +789,9 @@ fn emit_default_return_value(ctx: &mut LoweringContext<'_, '_>) -> crate::ir::Va None, ) .expect("const_null produces a value"); + // A fresh null is non-refcounted: there is no producer reference to + // release, so this boxes directly rather than via box_value_as_mixed + // (issue #484). ctx.emit_value( Op::MixedBox, vec![null_value], diff --git a/src/ir_lower/stmt/mod.rs b/src/ir_lower/stmt/mod.rs index 1d708c5aa2..6f1ce92203 100644 --- a/src/ir_lower/stmt/mod.rs +++ b/src/ir_lower/stmt/mod.rs @@ -1059,14 +1059,7 @@ fn coerce_typed_assign_value( return value; } match target_ty { - PhpType::Mixed => ctx.emit_value( - Op::MixedBox, - vec![value.value], - None, - PhpType::Mixed, - Op::MixedBox.default_effects(), - Some(span), - ), + PhpType::Mixed => ctx.box_value_as_mixed(value, PhpType::Mixed, Some(span)), _ => value, } } @@ -1246,14 +1239,7 @@ fn initialize_foreach_mixed_local_if_needed( ctx.declare_local(name, PhpType::Mixed); ctx.set_local_type(name, PhpType::Mixed); let null = emit_null_value(ctx, Some(span)); - let boxed = ctx.emit_value( - Op::MixedBox, - vec![null.value], - None, - PhpType::Mixed, - Op::MixedBox.default_effects(), - Some(span), - ); + let boxed = ctx.box_value_as_mixed(null, PhpType::Mixed, Some(span)); ctx.store_local(name, boxed, PhpType::Mixed, Some(span)); } @@ -2893,14 +2879,7 @@ fn coerce_to_return_type( coerce_return_scalar_source(ctx, value, span, coerce_to_tagged_scalar) } IrType::Heap(_) if ctx.return_php_type.codegen_repr() == PhpType::Mixed => { - ctx.emit_value( - Op::MixedBox, - vec![value.value], - None, - ctx.return_php_type.clone(), - Op::MixedBox.default_effects(), - span, - ) + ctx.box_value_as_mixed(value, ctx.return_php_type.clone(), span) } IrType::Heap(_) => ctx.emit_value( Op::RuntimeCall, diff --git a/tests/codegen/runtime_gc/regressions.rs b/tests/codegen/runtime_gc/regressions.rs index 6cc3fbeeab..1c6c36a9f7 100644 --- a/tests/codegen/runtime_gc/regressions.rs +++ b/tests/codegen/runtime_gc/regressions.rs @@ -1152,3 +1152,55 @@ echo "done"; "promoting an indexed array literal to hash storage must free the source array (issue #408)" ); } + +/// Regression for #484: boxing an owned object into a Mixed cell (a `?Class` return +/// coerces through `mixed_box`) retains the payload in the runtime, so the lowering must +/// release the producer's own reference. Before the fix `function g(): ?P { return new +/// P(); }` leaked one `P` per call even though the boxed cell itself was freed on rebind. +#[test] +fn test_regression_mixed_boxed_object_return_releases_producer() { + let out = compile_and_run_with_heap_debug( + r#"v; +} +echo $acc; +"#, + ); + assert!(out.success, "program failed: {}", out.stderr); + assert_eq!(out.stdout, "350"); + assert!( + out.stderr.contains("HEAP DEBUG: leak summary: clean"), + "expected the boxed objects to be released, got: {}", + out.stderr + ); +} + +/// Regression for #484 (array flavour): boxing an owned array into a Mixed cell retains +/// it in the runtime; the producer's reference must be released or the array leaks once +/// per boxing (e.g. a `?array`-returning function building a fresh literal). +#[test] +fn test_regression_mixed_boxed_array_return_releases_producer() { + let out = compile_and_run_with_heap_debug( + r#"