fix(codegen): dec superseded heap loop-binder values across recur (ADT leg)

A heap value threaded through a `(loop ...)`/`recur` as a loop binder
leaked: the `Term::Recur` arm stored each new arg into the binder's
loop-carried alloca with a plain `store` and never RC-dec'd the heap
value already in that slot, so every iteration's superseded value was
lost. The scope-close drop path (drop.rs, commit 8bcdae1) only ever
sees the loop's FINAL result, never the intermediates.

The recur store now decs the prior value before overwriting it, gated
by the SAME predicate the scope-close path uses — RC-heap AND lowers
to `ptr` AND not `Str`. The binder slot tuple is widened to carry the
binder's AILang type so the gate and the typed drop-symbol lookup
(`field_drop_call`) are available at the store. A same-pointer guard
(`icmp eq prior, new`) skips the dec when a `recur` threads the
identical pointer back — the own->own case (RawBuf fill loops, where
`RawBuf.set` mutates in place), so a retained buffer is never freed.

Scope: the boxed-ADT leg only. Every ADT value is heap-allocated, so
dec'ing a superseded ADT binder is unambiguously UB-free. A `Str`
accumulator is deliberately NOT covered: `Str` literals lower to
constexpr-GEPs into static globals (no rc_header), so a `Str` loop
seed may be static and dec'ing it would be UB. That is the same
static-vs-heap-`Str` obstacle behind 8bcdae1's deliberate `Str`
exclusion; the `Str` accumulator leak remains open in #49 pending a
runtime representation decision.

RED test in crates/ail/tests/loop_recur_heap_binder_no_leak_pin.rs
threads a boxed `Cell` through three recurs: pre-fix allocs=5 frees=2
live=3, post-fix live=0, stdout 2. Verified: zero-recur ADT control
live=0 (no spurious dec), Int loops (isqrt/collatz/gcd) live=0
(non-ptr, no dec), RawBuf fill loops live=0 out 35/16 (same-pointer
guard), Str accumulator unchanged (live=3, no crash).

refs #49
This commit is contained in:
2026-05-30 17:48:51 +02:00
parent a92bcaf969
commit f488d314e8
3 changed files with 198 additions and 10 deletions
@@ -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<u64> = None;
let mut frees: Option<u64> = None;
let mut live: Option<i64> = 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})"
);
}
+63 -10
View File
@@ -884,13 +884,16 @@ struct Emitter<'a> {
binder_allocas: BTreeMap<String, (String, Type)>,
/// 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"
));
@@ -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)))))))