diff --git a/CHANGELOG.md b/CHANGELOG.md index b31f875751..009a8e57ab 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 the caught-exception leak (issue #448): every handled `throw` leaked its throwable (~48 bytes per `catch`), because the catch binding moved the in-flight exception into the variable's slot without ever scheduling a release — the slot escaped rebind, function-epilogue, and program-exit cleanup alike. Catch-bound slots now follow the ordinary owned-local lifecycle (released on rebind and at the frame epilogue, zero-initialized so untaken catch paths stay safe), a variable-less `catch` consumes the reference through a hidden temporary, and rethrowing a caught variable (`throw $e`) retains it so inner and outer bindings each own their reference. Long-running throw/catch loops now keep a flat heap. - Fixed a constant-propagation miscompile with by-reference calls in a `match` subject (issue #384): `echo match(bump($i)) { ... } . "|" . $i` kept the pre-call constant for `$i` and printed the stale value, because a call's unknown write set was treated as "no writes" instead of forcing conservative invalidation. A read sequenced after any by-reference-mutating call in the same expression now observes the post-call value, matching PHP; pure calls (`gettype`, `strlen`, …) keep their operands foldable. - Fixed PHP parser/codegen parity for parenthesis-free object instantiation (issue #371): `new Foo`, `new self`, `new static`, `new parent`, and `new $class` now compile with empty constructor arguments, while immediate postfix forms such as `new Foo->bar`, `new Foo::bar()`, `new Foo?->bar`, and `new Foo[0]` are rejected instead of being misparsed. - Fixed three PHP parity regressions in the EIR backend: indexed array elements such as `$a[0]` can now be passed to by-reference parameters with copy-on-write storage split before mutation (issue #360); `IteratorAggregate::getIterator()` may declare the marker `Traversable` return type while `foreach` still dispatches through the returned object's concrete `Iterator` methods (issue #385); and catchable private/protected method access plus readonly-property write errors now preserve PHP's receiver/RHS evaluation order before throwing `Error` (issue #383). The merge also removes a duplicate `_spl_error_class_id` data symbol that could make post-merge user assembly fail to assemble. diff --git a/src/codegen/runtime/arrays/decref_any.rs b/src/codegen/runtime/arrays/decref_any.rs index 9f10d80b61..018f44c8ee 100644 --- a/src/codegen/runtime/arrays/decref_any.rs +++ b/src/codegen/runtime/arrays/decref_any.rs @@ -133,6 +133,8 @@ fn emit_decref_any_linux_x86_64(emitter: &mut Emitter) { emitter.instruction("je __rt_decref_any_object"); // objects release through the x86_64 object decref helper emitter.instruction("cmp r10, 5"); // does this heap-backed payload point at a boxed mixed cell? emitter.instruction("je __rt_decref_any_mixed"); // mixed cells release through the x86_64 mixed decref helper + emitter.instruction("cmp r10, 6"); // does this heap-backed payload point at a throwable object (issue #448)? + emitter.instruction("je __rt_decref_any_object"); // throwables release through the x86_64 object decref helper like plain objects emitter.instruction("jmp __rt_decref_any_done"); // unknown/raw heap kinds need no release work in the current x86_64 bootstrap runtime emitter.label("__rt_decref_any_string"); diff --git a/src/codegen/runtime/arrays/decref_object.rs b/src/codegen/runtime/arrays/decref_object.rs index 8b6cc978e6..6bac63ef14 100644 --- a/src/codegen/runtime/arrays/decref_object.rs +++ b/src/codegen/runtime/arrays/decref_object.rs @@ -110,8 +110,12 @@ fn emit_decref_object_linux_x86_64(emitter: &mut Emitter) { emitter.instruction(&format!("cmp r11d, 0x{:x}", X86_64_HEAP_MAGIC_HI32)); // ignore foreign pointers that do not carry the elephc x86_64 heap marker emitter.instruction("jne __rt_decref_object_skip"); // only elephc-owned objects participate in x86_64 decref bookkeeping emitter.instruction("and r10, 0xff"); // isolate the low-byte uniform heap kind tag for a final ownership sanity check - emitter.instruction("cmp r10, 4"); // is this heap-backed payload really an object instance? + emitter.instruction("cmp r10, 4"); // is this heap-backed payload a plain object instance? + emitter.instruction("je __rt_decref_object_counted"); // plain objects release through the shared refcount bookkeeping + emitter.instruction("cmp r10, 6"); // is this heap-backed payload a throwable object instance (issue #448)? emitter.instruction("jne __rt_decref_object_skip"); // other heap kinds must not be released through the object decref helper + + emitter.label("__rt_decref_object_counted"); emitter.instruction("mov r10d, DWORD PTR [rax - 12]"); // load the 32-bit object refcount from the uniform heap header emitter.instruction("sub r10d, 1"); // decrement the object refcount for the releasing x86_64 owner emitter.instruction("mov DWORD PTR [rax - 12], r10d"); // store the decremented object refcount back into the uniform heap header diff --git a/src/codegen/runtime/arrays/object_free_deep.rs b/src/codegen/runtime/arrays/object_free_deep.rs index 5a8ceda5e1..5c18eee0cc 100644 --- a/src/codegen/runtime/arrays/object_free_deep.rs +++ b/src/codegen/runtime/arrays/object_free_deep.rs @@ -283,8 +283,12 @@ fn emit_object_free_deep_linux_x86_64(emitter: &mut Emitter) { emitter.instruction(&format!("cmp r11d, 0x{:x}", X86_64_HEAP_MAGIC_HI32)); // ignore foreign pointers that do not carry the elephc x86_64 heap marker emitter.instruction("jne __rt_object_free_deep_done"); // only elephc-owned objects participate in x86_64 deep-free bookkeeping emitter.instruction("and r10, 0xff"); // isolate the low-byte uniform heap kind tag for a final ownership sanity check - emitter.instruction("cmp r10, 4"); // is this heap-backed payload really an object instance? + emitter.instruction("cmp r10, 4"); // is this heap-backed payload a plain object instance? + emitter.instruction("je __rt_object_free_deep_kind_ok"); // plain objects release through the shared deep-free walk + emitter.instruction("cmp r10, 6"); // is this heap-backed payload a throwable object instance (issue #448)? emitter.instruction("jne __rt_object_free_deep_done"); // other heap kinds must not be released through the object deep-free helper + + emitter.label("__rt_object_free_deep_kind_ok"); emitter.instruction("push rbp"); // preserve the caller frame pointer before reserving object deep-free spill slots emitter.instruction("mov rbp, rsp"); // establish a stable frame base for the saved object pointer, descriptor pointer, count, and loop index emitter.instruction("sub rsp, 32"); // reserve local storage for the object pointer, descriptor pointer, property count, and loop index diff --git a/src/codegen_ir/frame.rs b/src/codegen_ir/frame.rs index ea18af5e00..44203b8fae 100644 --- a/src/codegen_ir/frame.rs +++ b/src/codegen_ir/frame.rs @@ -346,7 +346,8 @@ fn emit_main_local_epilogue_cleanup(ctx: &mut FunctionContext<'_>) { } } -/// Returns main local slots that receive owned refcounted values through `StoreLocal`. +/// Returns main local slots that receive owned refcounted values through `StoreLocal` +/// or take ownership of a caught exception through `CatchBind`. fn main_cleanup_locals(ctx: &FunctionContext<'_>) -> Vec<(String, LocalSlotId, PhpType, usize)> { let param_names = ctx .function @@ -449,10 +450,14 @@ fn ref_cell_owner_locals(ctx: &FunctionContext<'_>) -> Vec<(String, LocalSlotId, locals } -/// Returns true when a local slot is written by an explicit EIR `StoreLocal`. +/// Returns true when a local slot is written by an explicit EIR `StoreLocal` or takes +/// ownership of the in-flight exception through a `CatchBind` (issue #448). Both make +/// the slot own a reference that the epilogue must release (and that the prologue must +/// zero-initialize, since a catch may never fire on some paths). fn local_slot_has_store(function: &Function, slot: LocalSlotId) -> bool { function.instructions.iter().any(|inst| { - inst.op == Op::StoreLocal && matches!(inst.immediate, Some(Immediate::LocalSlot(candidate)) if candidate == slot) + matches!(inst.op, Op::StoreLocal | Op::CatchBind) + && matches!(inst.immediate, Some(Immediate::LocalSlot(candidate)) if candidate == slot) }) } @@ -557,7 +562,8 @@ fn emit_function_local_epilogue_cleanup(ctx: &mut FunctionContext<'_>) { } } -/// Returns function local slots that receive owned refcounted values through `StoreLocal`. +/// Returns function local slots that receive owned refcounted values through `StoreLocal` +/// or take ownership of a caught exception through `CatchBind`. /// /// `include_returned` controls whether slots directly returned by a `Return` terminator are /// part of the set. The zero-initialization pass requests them (`true`) so a returned slot @@ -607,7 +613,7 @@ fn function_cleanup_locals( locals } -/// Returns whether a local kind can own values through ordinary `StoreLocal`. +/// Returns whether a local kind can own values through ordinary `StoreLocal` or `CatchBind`. fn local_kind_needs_epilogue_cleanup(kind: LocalKind) -> bool { matches!( kind, diff --git a/src/ir_lower/stmt/mod.rs b/src/ir_lower/stmt/mod.rs index bef17b5bba..15e31162d5 100644 --- a/src/ir_lower/stmt/mod.rs +++ b/src/ir_lower/stmt/mod.rs @@ -1465,6 +1465,17 @@ fn lower_include_once_guard(ctx: &mut LoweringContext<'_, '_>, label: &str, body /// Lowers a throwing statement into a terminator. fn lower_throw(ctx: &mut LoweringContext<'_, '_>, expr: &Expr) { let value = lower_expr(ctx, expr); + // The in-flight exception cell owns one reference to the thrown object. Throwing + // an owning temporary (e.g. `throw new E()`, `throw f()`) transfers that + // reference; throwing a borrowed value — a load of an owned local such as a + // rethrown catch variable (`throw $e`) — must retain it, so the local's own + // release (rebind or epilogue) stays balanced with the catch-side release of + // the in-flight reference (issue #448). + let value = if ctx.value_is_owning_temporary(value) { + value + } else { + crate::ir_lower::ownership::acquire_if_refcounted(ctx, value, Some(expr.span)) + }; terminate_throw(ctx, value.value); } @@ -1703,18 +1714,30 @@ fn lower_current_exception(ctx: &mut LoweringContext<'_, '_>, span: Span) -> Low ) } -/// Binds and clears the active exception for a matched catch clause. +/// Binds and clears the active exception for a matched catch clause. The bound slot +/// takes over the in-flight reference (the runtime cell is zeroed by the bind), so it +/// participates in the owned-local lifecycle: the previous binding is released before +/// the rebind, and the frame epilogue releases whatever the slot still holds (issue +/// #448). A variable-less catch consumes the reference through a hidden owned +/// temporary so it flows through the same lifecycle instead of leaking. fn lower_catch_bind(ctx: &mut LoweringContext<'_, '_>, catch: &CatchClause, span: Span) { - let (immediate, php_type) = catch.variable.as_ref().map_or((None, PhpType::Void), |variable| { - let php_type = catch_variable_type(catch); - let slot = ctx.declare_local(variable, php_type.clone()); - ctx.set_local_type(variable, php_type.clone()); - (Some(Immediate::LocalSlot(slot)), php_type) - }); + let php_type = catch_variable_type(catch); + let variable = match catch.variable.as_ref() { + Some(variable) => variable.clone(), + None => ctx.declare_owned_hidden_temp(php_type.clone()), + }; + let slot = ctx.declare_local(&variable, php_type.clone()); + ctx.set_local_type(&variable, php_type.clone()); + // Release the slot's previous exception before rebinding: a catch that fires + // again (next loop iteration, later try block) would otherwise overwrite the + // only owned reference. Catch-bound slots are zero-initialized in the prologue, + // so the release is a runtime no-op the first time through. + ctx.release_stored_local_value(&variable, slot, Some(span)); + ctx.mark_local_initialized(&variable); ctx.builder.emit_with_effects( Op::CatchBind, Vec::new(), - immediate, + Some(Immediate::LocalSlot(slot)), IrType::Void, php_type, Ownership::NonHeap, diff --git a/tests/codegen/runtime_gc/regressions.rs b/tests/codegen/runtime_gc/regressions.rs index c871eac725..59f70cdea1 100644 --- a/tests/codegen/runtime_gc/regressions.rs +++ b/tests/codegen/runtime_gc/regressions.rs @@ -1083,3 +1083,201 @@ echo "done"; "promoting an indexed array literal to hash storage must free the source array (issue #408)" ); } + +/// Regression for #448: a caught exception object must be released once the catch +/// handler is done with it. Before the fix the catch binding moved the throwable into +/// the variable's slot without ever scheduling a release (the slot is written by +/// `CatchBind`, not `StoreLocal`, so it escaped every cleanup point), leaking one +/// ~48-byte block per catch. A throw/catch loop must leave the heap clean. +#[test] +fn test_regression_caught_exception_released_per_catch() { + let out = compile_and_run_with_heap_debug( + r#"getMessage()); +"#, + ); + assert!(out.success, "program failed: {}", out.stderr); + assert_eq!(out.stdout, "5|4"); + // Exactly the 5 escaped exceptions remain live: the retaining array itself is + // released by main's epilogue (its element teardown semantics are pre-existing + // and identical to a no-try control pushing fresh objects). The catch slot's own + // cleanup must not free them a second time — a double free would abort the run + // with a bad-refcount fatal before this assertion. + assert!( + out.stderr.contains("HEAP DEBUG: leak summary: live_blocks=5"), + "expected exactly the 5 escaped exceptions to remain live, got: {}", + out.stderr + ); +} + +/// Regression for #448: `finally` on both paths — a caught exception with a finally +/// block, and a propagating exception whose finally runs before the outer catch — +/// must keep the heap flat like the plain catch shapes. +#[test] +fn test_regression_finally_paths_release_exception() { + let out = compile_and_run_with_heap_debug( + r#"