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#"