Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 45 additions & 6 deletions CGStruct.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1318,13 +1318,52 @@ llvm::Value *CodeGen::genFieldAssignment( FieldAssignmentExpression *expr )
}
else
{
mBuilder->CreateStore( val, fieldPtr );
// Resolve the field's type name, mapping generic params (e.g. T -> Inner).
string fTypeName = fieldType != nullptr ? fieldType->getName() : "";
auto subIt = mTypeSubstitution.find( fTypeName );
if ( subIt != mTypeSubstitution.end() )
fTypeName = subIt->second->getName();

// If a fresh Array<T> rvalue temporary is stored into a field, ownership
// transfers to the struct — untrack it so it is not also released as a
// statement temporary (which would leave the field pointing at freed memory).
if ( fieldType != nullptr && fieldType->getName() == "Array" )
untrackTempArray( val );
if ( expr->mOperation == "=" && isUserStructType( fTypeName ) )
{
// Refcounted user-struct field reassignment (the S1 fix). A struct is
// a heap pointer; the RHS struct literal/call is tracked as a statement
// temporary, so a bare store leaves the field owning an un-counted
// reference that is then freed when the temp is released at end of
// statement — the field dangles and later reads hit freed memory (a
// read-path-dependent use-after-free: the first read may still see the
// value, but any intervening allocation, e.g. print/interpolation,
// reuses the freed block and later reads return stale data). Mirror
// struct initialization: take ownership of the new value and drop the
// previously-held struct. Whether the RHS is a fresh temporary
// (literal/call — ownership transfers) or an existing owner
// (variable/field access — must retain) decides how ownership is taken.
// Retain the new value BEFORE releasing the old so a self-assignment
// (o.inner = o.inner) cannot free a value it is about to keep.
bool srcIsExistingOwner =
( dynamic_cast<VariableExpression*>( (Expression*)expr->mValue ) != nullptr ||
dynamic_cast<FieldAccessExpression*>( (Expression*)expr->mValue ) != nullptr );

llvm::Value *oldVal = mBuilder->CreateLoad(
structType->getElementType( fieldIdx ), fieldPtr,
expr->mFieldName + ".old" );
mBuilder->CreateStore( val, fieldPtr );
if ( srcIsExistingOwner )
mBuilder->CreateCall( getOrDeclareRcRetain(), { val } );
else
untrackTempStruct( val );
mBuilder->CreateCall( getOrDeclareRcRelease(), { oldVal } );
}
else
{
mBuilder->CreateStore( val, fieldPtr );

// If a fresh Array<T> rvalue temporary is stored into a field, ownership
// transfers to the struct — untrack it so it is not also released as a
// statement temporary (which would leave the field pointing at freed memory).
if ( fTypeName == "Array" )
untrackTempArray( val );
}
}

return val;
Expand Down
5 changes: 5 additions & 0 deletions docs/epics/functional-hardening/overview.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,8 @@ All of the following hold on a clean checkout of the epic's final state
| 2026-07-19 | — | readiness review round 1 | 5 findings. C1: +20 gate was commented-out + undefined BASELINE → pinned BASELINE=85, uncommented, added run.baseline. C2: ARC leak glob vacuous → U1 must name tests codegen_arc_*.b (invariant). C3: fix-or-file gameable → cap <=3 structured ### KI- entries. M1/S2: Map acceptance program refined; probe found the bug is DEEPER (Map from module unresolved via import AND --combine, not just bcc auto-include) — U4 scope updated. M2: soft_conflicts [U1,U3] recorded. done_condition re-synced. |
| 2026-07-19 | — | readiness review passed (10/10); status → ready | All 5 findings resolved (BASELINE pinned=85, ARC-glob naming required, fix-or-file cap=3, S2 deepened, soft_conflicts recorded); branch merged to master + pushed (91298d6). |
| 2026-07-19 | 7708a1f6 | launched on devbot | endpoint http://localhost:8000, dir a6b2f628, fully_autonomous, no_progress_threshold 10, 50M tokens, max 400 turns / 14 days. Team: 1 implementer + 1 independent reviewer (per PM). Baseline codegen=85 (master 91298d6). |
| 2026-07-19 | 7708a1f6 | launch baseline confirmed GREEN | Both builds OK (LLVM `build` + parse-only `build-parse`). `run_tests.sh`=195 pass/0 fail (LLVM), 190 pass/0 fail (`BUILD_DIR=build-parse`). `test_codegen.sh`=85 pass/0 fail (78 golden-checked, 7 quarantined). `ctest --test-dir build`=54/54. codegen_*.b count=85 (matches manifest baseline); codegen_arc_*.b=0. All gates green. |
| 2026-07-19 | 7708a1f6 | seeded bugs S1+S2 reproduced | **S1** (struct-field reassignment): `o.inner = Inner{v:99}` write dropped — prints `after 1`, assert fails, exit 1 (bcc). **S2** (Map via module): `Map<string,int> m` from `import collections;` → `error: Failed parse varible` at declaration, fails identically via `bcc` AND `qcc --combine stdlib/collections.b`. Both match design.md S1/S2. |
| 2026-07-19 | 7708a1f6 | U1 spec drafted → spec audit | `specs/017-arc-matrix/spec.md` on branch `epic/functional-hardening/u1-arc-matrix`: 7 `codegen_arc_*.b` + `codegen_struct_field_reassign.b` (S1 fix), all leak-checked. OQ-U1-1 raised (S1 fix lands in CGStruct.cpp → soft conflict [U1,U3], serialize). STOP — handed to reviewer for spec audit. |
| 2026-07-19 | 7708a1f6 | U1 spec APPROVED (SPEC-1/SPEC-2) | Reviewer approved with 2 required changes, folded in: SPEC-1 (S1 test must println reassigned field under golden — fails pre-fix); SPEC-2 (real symptom is read-path-dependent UAF, not a dropped store). Manager: SERIALIZE U1 before U3 CGStruct edits; U2/U4 parallel. |
| 2026-07-19 | 7708a1f6 | U1 IMPLEMENTED → code audit | S1 fixed in `CGStruct.cpp` genFieldAssignment (struct-field `=` now takes ownership of new struct + releases old, retain-before-release; root cause = missing ARC transfer → temp freed at stmt end = UAF; valgrind-confirmed pre/post). 8 new tests (7 `codegen_arc_*.b` + `codegen_struct_field_reassign.b`) w/ goldens, S1 test verified fail-pre-fix/pass-post-fix. Gates: test_codegen 93/0 (86 golden), leak-check 8/8 CLEAN 0 leaks, run_tests 195/190, ctest 54/54. **Handoff note for U3**: generic-method-chain→field (`m.get(k).field` on a generic `V`-returning method) reads empty — U3's method-chain→field shape, not filed (not an ARC bug). STOP — handed to reviewer for code audit. |
194 changes: 194 additions & 0 deletions specs/017-arc-matrix/spec.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
# Spec: Aggregate/field ARC matrix (+ seeded S1 fix)

**Epic**: functional-hardening · **Unit**: U1 · **Branch**: `epic/functional-hardening/u1-arc-matrix`
**Covers**: REQ-001 (aggregate/field ARC matrix, leak-clean) + REQ-005 (seeded fix S1: struct-valued field reassignment)
**Speckit**: `arc-matrix` · **Status**: Approved (spec audit passed; SPEC-1/SPEC-2 folded in)

## Problem

4 of the 5 bugs surfaced in the 2026-07-19 coverage evaluation were refcount
(ARC) handling in **field/aggregate contexts** — nested-field read, nested-field
write, string-field assignment (fixed), and **struct-valued field reassignment**
(seeded, S1, still broken). The suite tests refcounted types (string, Array,
struct) largely in isolation; it under-samples what happens when a refcounted
value is **stored into a struct field, an `Array<struct>` element, a `Map` value,
or an `Option` payload**, then read back and dropped. Those are exactly the ARC
edge cases that leak or double-free.

This unit builds a behavioral **ARC matrix** over that surface, every test
carrying a stdout golden AND run under `--leak-check`, and fixes the seeded
struct-valued field reassignment bug (S1).

### Confirmed seeded bug S1 (must be fixed, must never be filed)

Reproduces today on this branch's base:

```
struct Inner { int v; }
struct Outer { Inner inner; }
fn main() -> int {
Outer o = Outer { inner: Inner { v: 1 } };
o.inner = Inner { v: 99 };
assert o.inner.v == 99, "struct-field reassignment"; // fails: reads back 1
return 0;
}
```

### Corrected diagnosis (SPEC-2 — the real symptom)

The spec-audit reviewer found the assert-only snippet above **passes today**
(exit 0) — so an assert-only S1 test would have no teeth. The true symptom is a
**read-path-dependent use-after-free**, confirmed under valgrind:

- `o.inner = Inner { v: 99 }` **does** store the new pointer into the field.
- But the RHS struct temporary is tracked as a statement temporary and
`__blang_rc_release`'d at end of statement — freeing the very block the field
now points to. The field is left dangling.
- The **first** read right after (e.g. `int x = o.inner.v`) may still see `99`
because the freed block is not yet reused. Any **intervening allocation** —
notably the `println`/string-interpolation format path — reuses the freed
28-byte block, so a **subsequent** read of `o.inner.v` returns stale data
(`0`/garbage). That is why the bug is read-path- and sequence-dependent.

valgrind on the pre-fix binary reports `Invalid read of size 4 ... block was
free'd by __blang_rc_release`. So the fix is **not** "the store is dropped"; the
store is fine — the bug is missing ARC ownership transfer on a struct-valued
field assignment. The fix (in `CGStruct.cpp` `genFieldAssignment`, the
struct-field `=` path) must take ownership of the new struct (untrack a fresh
temporary, or retain an existing-owner source) and release the previously-held
struct — mirroring struct-literal initialization plus an old-value release,
retain-before-release so self-assignment is safe.

### SPEC-1 — the S1 regression test MUST have teeth (fail pre-fix)

Because the assert-only form passes today, `codegen_struct_field_reassign.b`
MUST exercise the genuinely-buggy read path: **`println` the reassigned field
value under a committed stdout golden showing `99`**, with a read sequence that
forces the UAF pre-fix (a read-after-reassignment that follows an intervening
`println`, i.e. two consecutive interpolated reads, whose pre-fix output is a
stale `1`/`0`). The reviewer verifies at code audit that this test **fails
pre-fix** (stale output ≠ golden) and **passes post-fix** (all reads `99`,
leak-clean).

## Scope

**In scope**
- ~7 behavioral ARC tests named `test_files/codegen_arc_*.b` (the `--leak-check`
acceptance glob depends on this prefix — load-bearing invariant), each with a
committed stdout golden and each leak-clean under `--leak-check`.
- The seeded S1 fix, with its regression test
`test_files/codegen_struct_field_reassign.b` (+ golden), also covered by the
leak glob at acceptance.
- Fixing (or, if large/risky/needs-a-language-decision, filing under the bounded
fix-or-file policy) any additional ARC bug a matrix test surfaces.

**Out of scope**
- Operator, interaction, and stdlib-via-`bcc` matrices (U2/U3/U4).
- New language features; harness changes. Reuse `test_codegen.sh` as-is.
- Deep generic-protocol dispatch.

## Named test cases (the matrix)

Each `codegen_arc_*.b` asserts concrete values internally AND prints a
deterministic line sequence captured by a committed `<name>.expected.out`
golden. All run under `./test_codegen.sh --leak-check` with 0 leaks.

| # | File | Shape covered | Key assertions / drop behavior |
|---|------|---------------|-------------------------------|
| 1 | `codegen_arc_string_field.b` | `string` stored into a struct field, read back, struct dropped; **field reassigned** to a new string (old string released, not leaked) | field reads correct both times; reassignment visible; no leak/double-free |
| 2 | `codegen_arc_struct_field.b` | refcounted (heap) `struct` stored into another struct's field, read through, then whole aggregate dropped | inner field values read correct; drop releases inner once |
| 3 | `codegen_arc_array_of_struct.b` | `Array<struct>` — push heap structs, read element fields, mutate an element field, array dropped | element field reads/mutations correct; each element released once at drop |
| 4 | `codegen_arc_map_struct_value.b` | `Map<string, struct>` (inline-defined Map to avoid the S2 module bug owned by U4) — `set`/`get` struct values, read a field of a fetched struct, map dropped | fetched struct field correct; values released at drop |
| 5 | `codegen_arc_option_struct.b` | `Option<struct>` — `some(structValue)`, `match`/unwrap to a field, `none` arm; dropped | unwrapped field correct; payload released once; `none` path leak-clean |
| 6 | `codegen_arc_nested_writethrough.b` | 2-level nesting (`Outer.mid.inner.v`) — read-through AND write-through of a leaf field; also reassign an intermediate refcounted field | leaf reads/writes correct through both levels; intermediates released once |
| 7 | `codegen_arc_self_assign.b` | self-assignment of a refcounted field (`o.s = o.s`) and of a struct field (`o.inner = o.inner`) | value unchanged; **no double-free** (retain-before-release ordering); leak-clean |

**Plus the seeded fix regression test:**

| S1 | `codegen_struct_field_reassign.b` | struct-valued field reassignment (`o.inner = Inner { v: 99 }`), the confirmed S1 repro — **prints the reassigned field twice via `println` under a golden showing `99`** (SPEC-1: fails pre-fix on the UAF read path, passes post-fix) | both interpolated reads show `99`; old inner released; leak-clean |

That is **7 `codegen_arc_*.b` + 1 `codegen_struct_field_reassign.b` = 8 new
`codegen_*.b` tests** toward the epic's ≥ 20 target (85 → 93 after U1).

## The S1 fix (implementation sketch — reviewer confirms during code audit)

The parser/sema accepts `o.inner = Inner { v: 99 }`; codegen for a struct-valued
field assignment (`CGStruct.cpp` `genFieldAssignment`, the LHS-is-field-access
path) must:
1. Evaluate the RHS struct value (a heap pointer for a user struct).
2. Store it into the field slot (currently the store appears to be dropped or
mistargeted — the field still reads the old pointer).
3. ARC discipline: **release** the struct reference previously held in the field
slot, **retain** the new one (or transfer the RHS temporary's owned refcount
into the slot without an extra retain), so the net refcount is balanced — no
leak, no double-free. Verified under `--leak-check`.

**Soft-conflict note (manifest `soft_conflicts: [U1, U3]`):** this fix is
expected to land in `CGStruct.cpp`, which U3 (interaction matrix) may also touch.
Per the workplan, if the fix lands in `CGStruct.cpp` the manager serializes U1
and U3. This is flagged to the manager as an Open Question up front (see below).

## Fix-or-file policy (this unit)

Any ARC bug a matrix test surfaces beyond S1 is, in order of preference:
1. **Fixed** — test passes, leak-clean, committed into the suite; or
2. **Filed** — a structured `### KI-N` entry in
`docs/epics/functional-hardening/known-issues.md` (fenced `Repro:` block +
`Justification:` line); the failing test is NOT committed into the passing
suite. Filing is only for large/risky/language-decision fixes and is raised
to the manager as an Open Question first. Global cap: ≤ 3 `### KI-` entries
across the whole epic. **S1 may never be filed.**

## Acceptance (this unit — reviewer re-runs independently)

```bash
# builds clean, both modes
cmake --build build -j"$(nproc)"
cmake --build build-parse -j"$(nproc)"

# suites green, both modes
./run_tests.sh && BUILD_DIR=build-parse ./run_tests.sh
./test_codegen.sh # all pass, incl. the 8 new tests w/ goldens

# the ARC matrix exists and is leak-clean (0 leaks; glob non-empty)
test -n "$(ls test_files/codegen_arc_*.b 2>/dev/null)"
./test_codegen.sh --leak-check test_files/codegen_arc_*.b test_files/codegen_struct_field_reassign.b

# seeded S1 fixed (+ golden), and NOT present in known-issues
./test_codegen.sh test_files/codegen_struct_field_reassign.b
! grep -q 'struct_field_reassign\|struct-valued field reassign' docs/epics/functional-hardening/known-issues.md 2>/dev/null

# fix-or-file bounded
ki=$(grep -c '^### KI-' docs/epics/functional-hardening/known-issues.md 2>/dev/null || echo 0); test "$ki" -le 3
```

## Success criteria

- **SC-001**: `ls test_files/codegen_arc_*.b` is non-empty (≥ 7 files) and all
pass under `./test_codegen.sh` with committed goldens.
- **SC-002**: `./test_codegen.sh --leak-check test_files/codegen_arc_*.b
test_files/codegen_struct_field_reassign.b` exits 0 with `Leaks: 0`.
- **SC-003**: `codegen_struct_field_reassign.b` passes (S1 fixed) and S1 does
not appear in `known-issues.md`.
- **SC-004**: `codegen_*.b` count is ≥ 93 (baseline 85 + 8); both `run_tests.sh`
modes, `test_codegen.sh`, and `ctest` stay green.
- **SC-005**: any additional ARC bug is fixed or filed (≤ 3 total structured
`### KI-` entries); nothing committed failing.

## Open Questions (raised to the manager before implementation)

- **OQ-U1-1**: The S1 fix is expected to land in `CGStruct.cpp`
(`genFieldAssignment`). Manifest records a soft conflict `[U1, U3]` on that
file. Requesting the manager serialize U1 before U3's `CGStruct` edits (or
confirm U3 has not yet touched it), and that U3 rebase onto U1's merge. No
action needed from me beyond this flag; proceeding with U1 as the earlier
unit.

## Assumptions

- Existing `test_codegen.sh` golden + `--leak-check` machinery is reused
unchanged (Non-goal: no new harness).
- Inline-defined `Map` (copied into the test file, as `codegen_map.b` does) is
used for the `Map<_,struct>` ARC test so U1 does not depend on U4's S2 fix.
- The S1 fix is correctness-only; default (non-sanitizer) build output for
existing tests is unchanged.
28 changes: 28 additions & 0 deletions test_files/codegen_arc_array_of_struct.b
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// ARC matrix (functional-hardening U1 / REQ-001): an Array<struct> holding
// refcounted heap structs — push fresh temporaries (ownership transfers into the
// array), read element fields via index and via for-in, then the array is
// dropped (each element released exactly once). Runs under --leak-check.
struct Item { string name; int qty; }

fn main() -> int {
Array<Item> items = [];
items.push(Item { name: "apple", qty: 3 });
items.push(Item { name: "pear", qty: 5 });
items.push(Item { name: "plum", qty: 7 });
println("len {}", items.length);

// for-in over the aggregate, reading element fields.
for it in items {
println("item {} {}", it.name, it.qty);
}

// index reads (twice, guarding a stale read).
println("idx0 {} {}", items[0].name, items[0].qty);
println("idx2 {} {}", items[2].name, items[2].qty);
println("idx0-again {} {}", items[0].name, items[0].qty);
assert items[0].qty == 3, "elem 0 qty";
assert items[2].name == "plum", "elem 2 name";

println("PASS");
return 0;
}
8 changes: 8 additions & 0 deletions test_files/codegen_arc_array_of_struct.expected.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
len 3
item apple 3
item pear 5
item plum 7
idx0 apple 3
idx2 plum 7
idx0-again apple 3
PASS
Loading
Loading