diff --git a/crates/ail/tests/loop_recur_heap_binder_no_leak_pin.rs b/crates/ail/tests/loop_recur_heap_binder_no_leak_pin.rs new file mode 100644 index 0000000..0f3d14c --- /dev/null +++ b/crates/ail/tests/loop_recur_heap_binder_no_leak_pin.rs @@ -0,0 +1,106 @@ +//! RED-pin for the per-iteration heap loop-binder leak across `recur` +//! (bug #49). +//! +//! Property protected: under `--alloc=rc`, when a `(loop ...)` carries a +//! heap-valued loop binder (here a boxed-ADT `Cell` accumulator) and a +//! `recur` REPLACES that binder with a fresh heap value, the superseded +//! prior value MUST be RC-decremented at the `recur` store. The runtime +//! RC stats line at exit must report `live == 0` (equivalently +//! `allocs == frees`) even though stdout is already correct. +//! +//! Root cause: codegen's `Term::Recur` arm in +//! `crates/ailang-codegen/src/lib.rs` (~line 2126) stores each new arg +//! value into the binder alloca with a plain `store` and never emits an +//! RC-dec for the heap value already in that slot. Each `recur` over a +//! heap binder therefore leaks the value it overwrites. This is DISTINCT +//! from the scope-close drop path in `crates/ailang-codegen/src/drop.rs` +//! (commit 8bcdae1), which drops only the loop's FINAL result — the +//! intermediate per-iteration values it never sees. +//! +//! A boxed ADT (not a `Str`) is the deliberate trigger: every ADT value +//! is heap-allocated, so dec'ing the superseded value is unambiguously +//! UB-free. `Str` literals lower to static globals, so a `Str` loop seed +//! must NOT be dec'd; the Str accumulator case is the separate +//! static-vs-heap-Str representation question (the obstacle behind the +//! deliberate Str exclusion in commit 8bcdae1), out of scope for this pin. +//! +//! The fixture threads a `Cell` through three `recur`s. As of HEAD this +//! test fails: stderr reports `ailang_rc_stats: allocs=4 frees=1 live=3` +//! — the three superseded `Cell` slabs leak. RawBuf loop binders do NOT +//! exhibit this leak because `RawBuf.set` mutates in place (own->own, no +//! per-iteration alloc), so their binder is never replaced; the trigger +//! requires a binder that a `recur` rebinds to a *fresh* heap allocation. + +use std::path::Path; +use std::process::Command; + +fn ail_bin() -> &'static str { + env!("CARGO_BIN_EXE_ail") +} + +#[test] +fn alloc_rc_heap_loop_binder_replaced_by_recur_does_not_leak() { + let manifest_dir = env!("CARGO_MANIFEST_DIR"); + let workspace = Path::new(manifest_dir).parent().unwrap().parent().unwrap(); + let src = workspace + .join("examples") + .join("loop_recur_heap_binder_no_leak_pin.ail"); + let tmp = std::env::temp_dir().join(format!( + "ailang_loop_recur_heap_binder_no_leak_pin_{}", + std::process::id() + )); + std::fs::create_dir_all(&tmp).unwrap(); + let out = tmp.join("bin"); + let status = Command::new(ail_bin()) + .args(["build", src.to_str().unwrap(), "--alloc=rc", "-o"]) + .arg(&out) + .status() + .expect("ail build failed to run"); + assert!( + status.success(), + "ail build --alloc=rc failed for loop_recur_heap_binder_no_leak_pin.ail" + ); + let output = Command::new(&out) + .env("AILANG_RC_STATS", "1") + .output() + .expect("execute binary"); + assert!( + output.status.success(), + "binary exited non-zero: status {:?}", + output.status + ); + let stderr = String::from_utf8(output.stderr).expect("stderr utf8"); + let stats_line = stderr + .lines() + .find(|l| l.starts_with("ailang_rc_stats:")) + .unwrap_or_else(|| { + panic!("missing ailang_rc_stats line in stderr; stderr was:\n{stderr}") + }); + let mut allocs: Option = None; + let mut frees: Option = None; + let mut live: Option = None; + for tok in stats_line.split_whitespace() { + if let Some(v) = tok.strip_prefix("allocs=") { + allocs = v.parse().ok(); + } else if let Some(v) = tok.strip_prefix("frees=") { + frees = v.parse().ok(); + } else if let Some(v) = tok.strip_prefix("live=") { + live = v.parse().ok(); + } + } + let allocs = allocs.expect("missing allocs= field"); + let frees = frees.expect("missing frees= field"); + let live = live.expect("missing live= field"); + assert_eq!( + live, 0, + "a heap (boxed-ADT) loop binder replaced by `recur` leaks {live} \ + superseded slab(s) (allocs={allocs} frees={frees}); the \ + Term::Recur arm in crates/ailang-codegen/src/lib.rs (~line \ + 2126) stores the new binder value without RC-dec'ing the prior \ + heap value it overwrites." + ); + assert_eq!( + allocs, frees, + "alloc/free mismatch (allocs={allocs} frees={frees} live={live})" + ); +} diff --git a/crates/ailang-codegen/src/lib.rs b/crates/ailang-codegen/src/lib.rs index 70efb73..a77599d 100644 --- a/crates/ailang-codegen/src/lib.rs +++ b/crates/ailang-codegen/src/lib.rs @@ -884,13 +884,16 @@ struct Emitter<'a> { binder_allocas: BTreeMap, /// loop-recur iter 3: stack of enclosing `Term::Loop` codegen /// frames. Each frame is `(header_block_label, binder_slots)` - /// where `binder_slots` is `(binder_name, alloca_ssa, llvm_ty)` - /// in declaration order. Pushed on `Term::Loop` body entry, - /// popped on exit; `Term::Recur` reads `.last()` for its target - /// header + the binder allocas to store into. Saved/reset/ - /// restored at the lambda-lowering boundary alongside - /// `binder_allocas` (a `recur` cannot cross a lambda boundary). - loop_frames: Vec<(String, Vec<(String, String, String)>)>, + /// where `binder_slots` is `(binder_name, alloca_ssa, llvm_ty, + /// ail_ty)` in declaration order. Pushed on `Term::Loop` body + /// entry, popped on exit; `Term::Recur` reads `.last()` for its + /// target header + the binder allocas to store into. The `ail_ty` + /// component (bug #49) lets the recur store dec the superseded + /// prior value of an RC-heap-non-Str binder before overwriting + /// it. Saved/reset/restored at the lambda-lowering boundary + /// alongside `binder_allocas` (a `recur` cannot cross a lambda + /// boundary). + loop_frames: Vec<(String, Vec<(String, String, String, Type)>)>, /// Side buffer for `alloca` instructions emitted during body /// lowering but hoisted to the fn's entry block. Flushed once /// into `self.body` at `entry_block_end_marker` after @@ -2040,7 +2043,7 @@ impl<'a> Emitter<'a> { let id = self.fresh_id(); let header = format!("loop.header.{id}"); let mut saved: Vec<(String, Option<(String, Type)>)> = Vec::new(); - let mut frame_slots: Vec<(String, String, String)> = Vec::new(); + let mut frame_slots: Vec<(String, String, String, Type)> = Vec::new(); for b in binders { let alloca_name = format!("%loopv_{}_{}", b.name, self.fresh_id()); @@ -2061,7 +2064,7 @@ impl<'a> Emitter<'a> { .binder_allocas .insert(b.name.clone(), (alloca_name.clone(), b.ty.clone())); saved.push((b.name.clone(), prior)); - frame_slots.push((b.name.clone(), alloca_name, llvm_ty)); + frame_slots.push((b.name.clone(), alloca_name, llvm_ty, b.ty.clone())); } self.body.push_str(&format!(" br label %{header}\n")); self.loop_frames.push((header.clone(), frame_slots)); @@ -2123,7 +2126,7 @@ impl<'a> Emitter<'a> { for a in args { lowered.push(self.lower_term(a)?); } - for ((arg_ssa, arg_ty), (_name, alloca_name, llvm_ty)) in + for ((arg_ssa, arg_ty), (_name, alloca_name, llvm_ty, ail_ty)) in lowered.into_iter().zip(slots.iter()) { if &arg_ty != llvm_ty { @@ -2132,6 +2135,56 @@ impl<'a> Emitter<'a> { — typecheck should have rejected this earlier" ))); } + // Bug #49: this `recur` REPLACES the binder's prior + // heap value with `arg_ssa`. Without a dec here the + // superseded value leaks every iteration (the + // scope-close path in drop.rs only ever sees the + // loop's FINAL result). Dec the prior value iff the + // binder type is RC-heap-and-not-Str — the SAME gate + // the scope-close drop path uses (lowers to `ptr` AND + // not Str; the Str exclusion is the static-literal + // representation rule from commit 8bcdae1: a `Str` + // loop seed may be a static global with no rc_header, + // so dec'ing it is UB). Primitives (Int/Bool/Float/ + // Unit) lower to non-`ptr` and are excluded by the + // `ptr` gate. + let is_ptr = + matches!(llvm_type(ail_ty).as_deref(), Ok("ptr")); + let is_str = matches!( + ail_ty, + Type::Con { name, .. } if name == "Str" + ); + if matches!(self.alloc, AllocStrategy::Rc) && is_ptr && !is_str { + // Load the prior value sitting in the alloca and + // guard against the own->own same-pointer case: + // a `recur` that threads the SAME buffer back + // (e.g. RawBuf fill loops, `RawBuf.set` mutating + // in place) rebinds to an identical pointer, and + // dec'ing it would free a live buffer. Skip the + // dec exactly when prior == new; otherwise the + // superseded distinct allocation drops once. + let prior = self.fresh_ssa(); + self.body.push_str(&format!( + " {prior} = load ptr, ptr {alloca_name}, align 8\n" + )); + let same = self.fresh_ssa(); + self.body.push_str(&format!( + " {same} = icmp eq ptr {prior}, {arg_ssa}\n" + )); + let id = self.fresh_id(); + let do_dec = format!("recur.dec.{id}"); + let after = format!("recur.after.{id}"); + self.body.push_str(&format!( + " br i1 {same}, label %{after}, label %{do_dec}\n" + )); + self.start_block(&do_dec); + let drop_call = self.field_drop_call(ail_ty); + self.body.push_str(&format!( + " call void @{drop_call}(ptr {prior})\n" + )); + self.body.push_str(&format!(" br label %{after}\n")); + self.start_block(&after); + } self.body.push_str(&format!( " store {llvm_ty} {arg_ssa}, ptr {alloca_name}\n" )); diff --git a/examples/loop_recur_heap_binder_no_leak_pin.ail b/examples/loop_recur_heap_binder_no_leak_pin.ail new file mode 100644 index 0000000..42b5051 --- /dev/null +++ b/examples/loop_recur_heap_binder_no_leak_pin.ail @@ -0,0 +1,29 @@ +(module loop_recur_heap_binder_no_leak_pin + ; RED for bug #49: a heap loop binder that `recur` REPLACES with a fresh + ; heap value each iteration. Each superseded value is a distinct heap + ; allocation; codegen's Term::Recur arm stores the new binder value into + ; the loop-carried alloca without RC-dec'ing the prior one, so every + ; overwritten value leaks. + ; + ; A boxed ADT (`Cell`), NOT a `Str`, is the deliberate trigger: every + ; ADT value is heap-allocated (there is no static-literal representation + ; for an ADT), so the per-iteration dec is unambiguously UB-free. `Str` + ; literals lower to constexpr-GEPs into static globals (lib.rs Str-lit + ; arm), so a `Str` loop seed is static and must NOT be dec'd — the Str + ; case is the separate static-vs-heap-Str representation question + ; (the same obstacle behind the deliberate Str exclusion in 8bcdae1). + ; + ; allocs: seed Cell(0) + three recur Cells (Cell at i=0,1,2) = 4 heap + ; slabs. The loop returns acc=Cell(2); that final result drops at scope + ; close (the drop.rs path). The three superseded Cells (seed, Cell@i=0, + ; Cell@i=1) must drop at the recur store. Pre-fix: live=3. Post-fix: + ; live=0. Expected stdout: 2. + (data Cell (ctor Cell (con Int))) + (fn main (type (fn-type (params) (ret (con Unit)) (effects IO))) (params) + (body + (let final + (loop (acc (con Cell) (term-ctor Cell Cell 0)) (i (con Int) 0) + (if (app ge i 3) + acc + (recur (term-ctor Cell Cell i) (app + i 1)))) + (match final (case (pat-ctor Cell n) (app print n)))))))