From 5305ed5148b8815f71457d976117ce3329b7a9c4 Mon Sep 17 00:00:00 2001 From: mirchaemanuel Date: Sat, 11 Jul 2026 19:16:35 +0200 Subject: [PATCH] fix(ir): release loop-carried locals widened to boxed storage after their store was lowered (#534) A local stored inside a loop while its slot still looked like a plain scalar skipped the release-old path, but a later store on a back-edge path (e.g. the inner for counter's checked-add update) could widen the slot to boxed Mixed storage. Each outer iteration's re-initialization then orphaned the previous iteration's box, leaking exactly N_outer-1 blocks. Lowering now emits a deferred release_local_slot before such stores; the backend releases the occupant using the slot's FINAL widened storage type (null-guarded, so the zero-initialized first iteration is safe), and lowering prunes the op to a nop when the slot never widens so scalar slots stay eligible for dead-store elimination. Claude-Session: https://claude.ai/code/session_01Jfn7uqEH5Dog1nBzTM5DCW --- docs/internals/memory-model.md | 2 +- src/codegen/frame.rs | 6 +- src/codegen/lower_inst.rs | 38 ++++++++ src/ir/builder.rs | 32 +++++++ src/ir/instr.rs | 3 + src/ir/validator.rs | 5 +- src/ir_lower/context.rs | 49 +++++++++- src/ir_lower/function.rs | 3 + src/ir_passes/peephole/load_store.rs | 1 + tests/codegen/runtime_gc/regressions.rs | 120 ++++++++++++++++++++++++ 10 files changed, 250 insertions(+), 9 deletions(-) diff --git a/docs/internals/memory-model.md b/docs/internals/memory-model.md index 782d87dac6..da10bed691 100644 --- a/docs/internals/memory-model.md +++ b/docs/internals/memory-model.md @@ -278,7 +278,7 @@ When one of these checks trips, the program exits with a fatal heap-debug error ### When memory is freed -- **Variable reassignment**: when a heap-backed local/global/static slot is overwritten, codegen releases the previous owner through the appropriate runtime path (`__rt_heap_free_safe` for persisted strings, `__rt_decref_*` for refcounted arrays / hashes / objects) +- **Variable reassignment**: when a heap-backed local/global/static slot is overwritten, codegen releases the previous owner through the appropriate runtime path (`__rt_heap_free_safe` for persisted strings, `__rt_decref_*` for refcounted arrays / hashes / objects). When a store inside a loop is lowered before a later store has widened the slot to boxed storage (e.g. an inner `for` counter re-initialized by the outer body but widened Int→Mixed by its `++` update), lowering emits a deferred `release_local_slot` and the backend decides against the slot's final widened storage type, so the previous iteration's box is still released - **`unset()`**: releases the current heap-backed value before nulling the slot - **Targeted cycle collection**: when decref reaches a container/object graph that may only be keeping itself alive, `__rt_gc_collect_cycles` counts heap-only incoming edges, marks externally reachable blocks, and deep-frees the remaining unreachable array/hash/object island - **Generator frame release**: Generator frames are object-kind heap blocks, but their custom Mixed slots and active `yield from` delegate are released by a Generator-specific branch in object deep-free diff --git a/src/codegen/frame.rs b/src/codegen/frame.rs index d7928e7a1d..663bc9bc46 100644 --- a/src/codegen/frame.rs +++ b/src/codegen/frame.rs @@ -534,7 +534,7 @@ fn local_slot_has_store(function: &Function, slot: LocalSlotId) -> bool { } /// Returns PHP-visible locals whose slot is rewritten to a ref-cell pointer. -fn promoted_ref_cell_local_slots(function: &Function) -> HashSet { +pub(super) fn promoted_ref_cell_local_slots(function: &Function) -> HashSet { let mut slots = function .instructions .iter() @@ -583,7 +583,7 @@ fn loaded_local_slot(function: &Function, value: ValueId) -> Option /// .rodata, out-of-range) and frees plausible live heap blocks, so it safely handles /// the zero-length owned strings that `__rt_str_persist` now allocates. The previous /// `cbz len` guard skipped them and leaked every owned empty string at scope exit. -fn emit_main_string_cleanup(ctx: &mut FunctionContext<'_>, offset: usize) { +pub(super) fn emit_main_string_cleanup(ctx: &mut FunctionContext<'_>, offset: usize) { let (ptr_reg, _) = abi::string_result_regs(ctx.emitter); let result_reg = abi::int_result_reg(ctx.emitter); abi::load_at_offset(ctx.emitter, ptr_reg, offset); @@ -604,7 +604,7 @@ fn emit_main_string_cleanup(ctx: &mut FunctionContext<'_>, offset: usize) { } /// Releases a refcounted local when the slot contains a non-null heap pointer. -fn emit_main_refcounted_cleanup(ctx: &mut FunctionContext<'_>, offset: usize, ty: &PhpType) { +pub(super) fn emit_main_refcounted_cleanup(ctx: &mut FunctionContext<'_>, offset: usize, ty: &PhpType) { let result_reg = abi::int_result_reg(ctx.emitter); let done = ctx.next_label("main_refcounted_cleanup_done"); abi::load_at_offset(ctx.emitter, result_reg, offset); diff --git a/src/codegen/lower_inst.rs b/src/codegen/lower_inst.rs index ba362e3f93..11356864c4 100644 --- a/src/codegen/lower_inst.rs +++ b/src/codegen/lower_inst.rs @@ -79,6 +79,7 @@ pub(super) fn lower_instruction(ctx: &mut FunctionContext<'_>, inst_id: InstId) Op::PromoteLocalRefCell => lower_promote_local_ref_cell(ctx, &inst), Op::AliasLocalRefCell => lower_alias_local_ref_cell(ctx, &inst), Op::ReleaseLocalRefCell => lower_release_local_ref_cell(ctx, &inst), + Op::ReleaseLocalSlot => lower_release_local_slot(ctx, &inst), Op::LoadGlobal => lower_load_global(ctx, &inst), Op::StoreGlobal => lower_store_global(ctx, &inst), Op::ExternGlobalLoad => lower_extern_global_load(ctx, &inst), @@ -6361,6 +6362,43 @@ fn release_local_ref_cell_owner( Ok(()) } +/// Lowers a deferred `release_local_slot`: releases the refcounted value currently +/// held in a local frame slot, typed by the slot's FINAL storage type. +/// +/// Lowering emits this op before a retaining loop store when the slot's storage +/// type still looked untracked at that point but a later store on a back-edge +/// path could widen it (issue #534: an inner `for` counter widened Int→Mixed by +/// its checked-add update leaked one Mixed box per outer iteration when the +/// outer body re-initialized it). Only here, after widening finished, is the +/// decision sound. The slot is either zero (prologue zero-initializes cleanup +/// locals, and the null-guarded release helpers skip zero) or an owned value +/// boxed by a previous retaining store, so releasing it is always balanced. +fn lower_release_local_slot(ctx: &mut FunctionContext<'_>, inst: &Instruction) -> Result<()> { + let slot = expect_local_slot(inst)?; + // Slots rewritten to ref-cell pointers hold the cell address, not an owned + // value; the ref-cell owner machinery releases those instead. + if local_slot_stores_ref_cell_pointer(ctx, slot) + || super::frame::promoted_ref_cell_local_slots(ctx.function).contains(&slot) + { + return Ok(()); + } + let ty = ctx.local_php_type(slot)?.codegen_repr(); + let offset = ctx.local_offset(slot)?; + match ty { + // Owned strings are freed through the validating helper, which skips + // null/uninitialized slots and non-heap (.rodata) literal pointers. + PhpType::Str => super::frame::emit_main_string_cleanup(ctx, offset), + PhpType::Callable => super::frame::emit_main_refcounted_cleanup(ctx, offset, &ty), + other if other.is_refcounted() => { + super::frame::emit_main_refcounted_cleanup(ctx, offset, &other) + } + // The slot never widened to refcounted storage: nothing can be owned. + // Lowering normally prunes these, so this arm is only a safety net. + _ => {} + } + Ok(()) +} + /// Lowers `unset($local)` by breaking any promoted alias and writing PHP null locally. fn lower_unset_local(ctx: &mut FunctionContext<'_>, inst: &Instruction) -> Result<()> { let slot = expect_local_slot(inst)?; diff --git a/src/ir/builder.rs b/src/ir/builder.rs index 401cd1577e..57be074752 100644 --- a/src/ir/builder.rs +++ b/src/ir/builder.rs @@ -106,6 +106,38 @@ impl<'f> Builder<'f> { self.func.locals[slot.as_raw() as usize].php_type.clone() } + /// Neutralizes deferred `release_local_slot` ops whose slot never widened to + /// lifetime-tracked storage. + /// + /// Lowering emits `release_local_slot` before a loop store when the slot's + /// storage type LOOKS untracked at that point but a later store on a + /// back-edge path may still widen it (e.g. a `for` counter widened + /// Int→Mixed by its checked-add update). Once the whole body is lowered the + /// final storage types are known, so ops guarding slots that stayed + /// untracked are rewritten to `nop`s. This keeps scalar slots eligible for + /// dead-store elimination and load forwarding, which conservatively exclude + /// any slot named by an unknown op. + pub fn prune_untracked_release_local_slot_ops(&mut self) { + for inst in &mut self.func.instructions { + if inst.op != Op::ReleaseLocalSlot { + continue; + } + let Some(Immediate::LocalSlot(slot)) = inst.immediate else { + continue; + }; + let storage_type = &self.func.locals[slot.as_raw() as usize].php_type; + if Ownership::php_type_needs_lifetime_tracking(storage_type) { + continue; + } + // The slot's final storage is a plain scalar: the deferred release + // can never free anything, so erase it instead of shipping a no-op + // that pessimizes slot analyses in later passes. + inst.op = Op::Nop; + inst.immediate = None; + inst.effects = Op::Nop.default_effects(); + } + } + /// Returns the semantic role of a local slot. pub fn local_kind(&self, slot: LocalSlotId) -> LocalKind { self.func.locals[slot.as_raw() as usize].kind diff --git a/src/ir/instr.rs b/src/ir/instr.rs index 389ff8e84a..752ece8d08 100644 --- a/src/ir/instr.rs +++ b/src/ir/instr.rs @@ -196,6 +196,7 @@ pub enum Op { PromoteLocalRefCell, AliasLocalRefCell, ReleaseLocalRefCell, + ReleaseLocalSlot, LoadGlobal, StoreGlobal, LoadStaticLocal, @@ -438,6 +439,7 @@ impl Op { }, AliasLocalRefCell => E::READS_LOCAL | E::WRITES_LOCAL, ReleaseLocalRefCell => E::READS_LOCAL | E::WRITES_LOCAL | E::WRITES_HEAP | E::REFCOUNT_OP, + ReleaseLocalSlot => E::READS_LOCAL | E::WRITES_HEAP | E::REFCOUNT_OP, LoadGlobal | LoadStaticProperty | ScopedConstantGet | ClassAttrNames | ClassAttrArgs | ClassGetAttributes | CatchCurrent => E::READS_GLOBAL, StoreGlobal | StoreStaticLocal | StoreStaticProperty | InitStaticLocal | IncludeOnceMark @@ -550,6 +552,7 @@ impl Op { PromoteLocalRefCell => "promote_local_ref_cell", AliasLocalRefCell => "alias_local_ref_cell", ReleaseLocalRefCell => "release_local_ref_cell", + ReleaseLocalSlot => "release_local_slot", LoadGlobal => "load_global", StoreGlobal => "store_global", LoadStaticLocal => "load_static_local", diff --git a/src/ir/validator.rs b/src/ir/validator.rs index 508a134912..c33fe75190 100644 --- a/src/ir/validator.rs +++ b/src/ir/validator.rs @@ -278,7 +278,7 @@ fn validate_instruction_immediate(inst_id: InstId, inst: &Instruction) -> Result require_immediate(inst_id, inst, "data id", |imm| matches!(imm, Imm::Data(_))) } LoadLocal | StoreLocal | UnsetLocal | LoadRefCell | StoreRefCell | ReleaseLocalRefCell - | BindRefCellPtr + | ReleaseLocalSlot | BindRefCellPtr | LoadStaticLocal | StoreStaticLocal | InitStaticLocal | InvokerRefArg => require_immediate(inst_id, inst, "local slot", |imm| { matches!(imm, Imm::LocalSlot(_)) }), @@ -395,7 +395,8 @@ fn validate_opcode_rules(function: &Function, inst_id: InstId, inst: &Instructio LoadLocal | LoadRefCell | LoadGlobal | LoadStaticLocal | LoadStaticProperty | ExternGlobalLoad => { check_count(inst_id, inst, 0, "0") } - UnsetLocal | PromoteLocalRefCell | AliasLocalRefCell | ReleaseLocalRefCell => { + UnsetLocal | PromoteLocalRefCell | AliasLocalRefCell | ReleaseLocalRefCell + | ReleaseLocalSlot => { check_count(inst_id, inst, 0, "0") } StoreLocal | StoreGlobal | StoreStaticLocal | InitStaticLocal | StoreStaticProperty | ExternGlobalStore diff --git a/src/ir_lower/context.rs b/src/ir_lower/context.rs index 6bce5860a6..72eb633b83 100644 --- a/src/ir_lower/context.rs +++ b/src/ir_lower/context.rs @@ -615,6 +615,49 @@ impl<'m, 'f> LoweringContext<'m, 'f> { crate::ir_lower::ownership::release_if_owned(self, previous, span); } + /// Releases the previous occupant of a slot that a retaining `store_local` is + /// about to overwrite, deciding against the FINAL storage type when needed. + /// + /// When the slot's storage type already needs lifetime tracking this emits the + /// eager load+release pair immediately. When it does not, the slot can STILL be + /// widened to refcounted storage by a store lowered later that reaches this one + /// through a loop back-edge (e.g. an inner `for` counter re-initialized by the + /// outer body but widened Int→Mixed by its checked-add update). The storage + /// type visible here is stale in that case, so inside loops a deferred + /// `release_local_slot` is emitted instead: the backend releases the occupant + /// using the final widened storage type, and `prune_untracked_release_local_slot_ops` + /// erases the op when the slot never widens (issue #534: without this, the + /// previous outer iteration's Mixed box leaked on every re-initialization). + fn release_stored_local_value_before_retaining_store( + &mut self, + name: &str, + slot: LocalSlotId, + span: Option, + ) { + let storage_type = self.builder.local_php_type(slot); + if Ownership::php_type_needs_lifetime_tracking(&storage_type) { + self.release_stored_local_value(name, slot, span); + return; + } + if self.loop_stack.is_empty() { + // Outside loops no back-edge can execute a later widening store before + // this one, so the untracked storage type is final for this path. + return; + } + // Ref-bound locals keep a cell pointer in the frame slot and are released + // through the ref-cell owner machinery, never through a raw slot release. + if self.is_ref_bound_local(name) { + return; + } + self.emit_void( + Op::ReleaseLocalSlot, + Vec::new(), + Some(Immediate::LocalSlot(slot)), + Op::ReleaseLocalSlot.default_effects(), + span, + ); + } + /// Emits a store to a PHP local slot, updates type facts, and returns the stored value. pub(crate) fn store_local(&mut self, name: &str, value: LoweredValue, php_type: PhpType, span: Option) -> LoweredValue { self.clear_static_callable_local(name); @@ -661,7 +704,7 @@ impl<'m, 'f> LoweringContext<'m, 'f> { && local_kind_uses_plain_store_cleanup(previous_kind) && previous_slot.is_some_and(|slot| self.initialized_slots.contains(&slot)) { - self.release_stored_local_value(name, slot, span); + self.release_stored_local_value_before_retaining_store(name, slot, span); } // A loop-carried slot can exist globally without being definitely initialized // on this CFG path. Release the runtime occupant before overwriting it. @@ -670,7 +713,7 @@ impl<'m, 'f> LoweringContext<'m, 'f> { && previous_slot.is_some_and(|slot| !self.initialized_slots.contains(&slot)) && !self.loop_stack.is_empty() { - self.release_stored_local_value(name, slot, span); + self.release_stored_local_value_before_retaining_store(name, slot, span); } // A first syntactic store inside a loop body (main or function) can still // overwrite a prior runtime iteration's value: the slot has no straight-line @@ -684,7 +727,7 @@ impl<'m, 'f> LoweringContext<'m, 'f> { && previous_slot.is_none() && !self.loop_stack.is_empty() { - self.release_stored_local_value(name, slot, span); + self.release_stored_local_value_before_retaining_store(name, slot, span); } let value = if (uses_global || previous_kind == LocalKind::PhpLocal) && !transfer_callable_source_to_store diff --git a/src/ir_lower/function.rs b/src/ir_lower/function.rs index 588df57ead..d9abdc60f5 100644 --- a/src/ir_lower/function.rs +++ b/src/ir_lower/function.rs @@ -622,6 +622,9 @@ fn lower_body_into_function( crate::ir_lower::stmt::lower_stmt(&mut ctx, stmt); } terminate_open_block(&mut ctx); + // Final storage types are now known: erase deferred loop-store releases that + // guard slots which never widened to lifetime-tracked storage (issue #534). + ctx.builder.prune_untracked_release_local_slot_ops(); ctx.into_closures() } diff --git a/src/ir_passes/peephole/load_store.rs b/src/ir_passes/peephole/load_store.rs index d66f685a60..230f7cadd2 100644 --- a/src/ir_passes/peephole/load_store.rs +++ b/src/ir_passes/peephole/load_store.rs @@ -103,6 +103,7 @@ fn escaping_slots(function: &Function) -> HashSet { Op::PromoteLocalRefCell | Op::AliasLocalRefCell | Op::ReleaseLocalRefCell + | Op::ReleaseLocalSlot | Op::InvokerRefArg ) { for slot in slots_of(inst) { diff --git a/tests/codegen/runtime_gc/regressions.rs b/tests/codegen/runtime_gc/regressions.rs index bdf32578f7..199aa233a1 100644 --- a/tests/codegen/runtime_gc/regressions.rs +++ b/tests/codegen/runtime_gc/regressions.rs @@ -1230,3 +1230,123 @@ echo "done"; "promoting an indexed array literal to hash storage must free the source array (issue #408)" ); } + +/// Regression test for issue #534: a refcounted local assigned inside a NESTED +/// inner loop must not leak one block per OUTER iteration. The inner counter's +/// re-initialization (`$k = 0`) is lowered while the slot still looks like an +/// Int, but the `$k++` update widens the slot to boxed Mixed storage, so the +/// re-init store must still release the previous outer iteration's box. +#[test] +fn test_regression_534_nested_loop_local_does_not_leak_per_outer_iteration() { + let out = compile_and_run_with_heap_debug( + r#"