fix(gc,vm): generate the two lockstep dispatches from one edge table; one CallFrame init/release

[Nitjsefnie]

What does this PR do?

Replaces the two prose-enforced lockstep invariants in #743 with structure: the GC's walker and clearer are now generated from one edge table, and CallFrame has exactly one init and one release definition instead of five hand-written sites. The bug the prose was failing to prevent is fixed as a side effect — gc_clear_node was missing the VAL_FN → chunk edge that GC_FOR_EACH_CHILD reports.

Related Issues and Pull Requests

Fixes #743

Changes

• src/eigenscript.c: one GC_EDGE_TABLE macro is now expanded twice — GC_EDGE_WALK (:2632) and GC_EDGE_CLEAR (:2643) — so the walker and the clearer cannot drift. The missing VAL_FN → chunk edge is gone by construction rather than by having been spotted.
• src/vm.c: callframe_init (:477) and callframe_release (:508) are the single definitions; the four hand-written push sites and all three teardown sites now route through them. (The issue says five; its fifth citation at :4264 is LOOP_ENV_FRESH, an env-ownership swap on an existing frame rather than a push, and is correctly left alone — thanks for catching that this body had inherited the miscount.)
• src/vm.h: callframe_release exposed non-static (:472) so the third teardown site can reach it, and the stale comment above it corrected — it described a two-site invariant that had already grown a third.
• src/builtins.c: the third teardown site (task_free, :5521) converted from hand-written teardown to callframe_release.
• CHANGELOG.md: ### Fixed entry under [Unreleased].
• .claude/rules/c-runtime-memory.md: the edge-table rule documented, merged with your -Werror=switch sentence from runtime: ValType switches go exhaustive, finishing #738 — first build caught real drift #806.

Interaction with #806, worth your attention. Rebasing onto current main conflicted with 9945c52, which rewrote the two switches this PR replaces. Taking the edge table wholesale would have silently dropped the property you had just landed — that a new ValType is a build error at the GC sites — because the table selects rows by _v->type == VAL_x guards, which -Werror=switch cannot see. So gc_value_is_node (:2472) became an exhaustive default:-free switch instead: a node is exactly a type with table rows, same truth table, and the compile-time gate lands there. Verified by planting an 11th enumerator in ValType and building: eigenscript.c:2474:5: error: enumeration value 'VAL_PROBE_NEW' not handled in switch [-Werror=switch], in gc_value_is_node, alongside your other #806 sites. Probe reverted.

Two invariants left deliberately alone, since both are load-bearing and neither is drift: GC_EDGE_CLEAR ignores IS_NODE (the clear pass must reach non-node children too), and fn.body = NULL after the chunk release is what stops the destructor double-releasing. vm_error_halt and CHECK_ERROR keep their inline teardown — they unwind partially-built frames where callframe_release's preconditions do not hold, so folding them in would be a correctness change wearing a cleanup's clothes.

One thing the issue's framing gets slightly wrong, for the record: the value OP_DISPATCH restores is unstamped (0), not a previous frame's value. Nothing reads a stale frame; the defect is the missing chunk edge, not a stale restore.

Testing

Against merge-base cd9f823 (current main):

• Release (CC=gcc ./build.sh, then cd tests && bash run_all_tests.sh): 3364/3364, exit 0.
• Sanitizers (make asan, then the suite with ASAN_OPTIONS=detect_leaks=1 UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1): 3368/3368, exit 0, zero AddressSanitizer/LeakSanitizer lines in the log.
• Section [87] run directly under leak detection (ASAN_OPTIONS=detect_leaks=1 ../src/eigenscript test_closure_cycles.eigs): 17/17, exit 0. Section [101] (test_spawn_gc.eigs, worker-created cycles): exit 0.
• task_free's converted teardown exercised by a probe script that suspends tasks inside a try so they are still live at exit: zero sanitizer findings, output byte-identical to the pre-rebase run.
• [98] doc-drift gate passes with the CHANGELOG entry in place.

Not run locally: clang, macOS, freestanding, extensions, database, valgrind, tsan, bench — leaving those to CI.

Follow-ups / Known Limitations

• No test in this PR fails on the pre-fix collector, and I would rather say so than have you find it. The missed VAL_FN → chunk edge was masked by the destructor mirror, so the leak never surfaced through the existing gates; [87]'s strict leak gate covers the collector half but does not pin this specific edge. A regression test that fails without the fix would need a cycle whose only chunk reference is the collected one — happy to add it if you want the pin, but I did not want to imply the suite was catching something it was not.
• callframe_init is a static helper on every frame push. If inlining does not fully collapse it, the bench instruction-count gate is the plausible red leg; I have not measured it locally.

Checklist

• make test passes locally — 3364/3364 release, 3368/3368 under ASan+UBSan with leak detection on
• New builtins have signature comments and docs in docs/BUILTINS.md — n/a, no new builtins
• New library functions follow conventions in docs/STDLIB.md — n/a
• New examples have a comment header explaining what they demonstrate — n/a
• CHANGELOG.md updated (if user-facing change)

Generated by Claude Opus 5 (brief, implementation, verification, review), Kimi K3 (implementation)

[InauguralPhysicist]

Reviewed against merge-base cd9f823, table row by row against both original dispatches. This is the right fix for #743 — structure where there was prose — and the #806 interaction was handled exactly right: moving the exhaustiveness gate to gc_value_is_node preserves the build-error property the table's _v->type == VAL_x guards would have silently dropped. Verified findings below; one change requested.

Verified (for the record):

• Every row of GC_EDGE_TABLE matches the original walker and clearer: env parent/slots, chunk functions[]/env_cache, fn closure/chunk, list/dict elements. The only semantic delta is the intended one (the fn→chunk clear edge, with the load-bearing body = NULL).

• The unconditional chunk_decref(fn.body) safety claim checks out against the tree: make_fn (body = NULL, body_count = 0) and OP_CLOSURE (vm.c:3486) are the only writers of fn.body/body_count, so no non-chunk pointer can reach the clear today. Good call documenting the AST-body revival trap at the row.

• No break/continue hazard from the new loop structure: all three GC_FOR_EACH_CHILD call-site bodies are plain blocks. And the cached _e/_c/_v are element copies, so gcu_add's realloc during the step-1 walk can't invalidate them.

• All four real push sites are converted. (The issue counted five, but its :4264 citation was LOOP_ENV_FRESH — an env-ownership swap on an existing frame, not a push — correctly left alone. The PR body's "five hand-written init sites" inherits the miscount; the CHANGELOG's "all four push sites" is the accurate one.)

• All three saved-frame teardown loops share callframe_release, with the #726 g_try_depth fixups retained where they matter. task_sched_thread_free not having one is fine by construction — it runs at fatal exit, where there is no later uncaught error left to silence.

• Reproduced the OP_DISPATCH fix red-then-green locally (release builds of cd9f823 vs 97ffab6):

  define d0(x) as:
      return x
  t is [d0]
  y is 1
  y is 100
  loop while not converged:
      y is y * 2
      junk is dispatch of [t, 0, y]
  print of __loop_iterations__
  

  main prints 1 (frozen after the first dispatch return); this branch prints 34, matching the equivalent-shape loop without dispatch. A plain OP_CALL in the body (junk is d0 of y) is unaffected on both — the bug was dispatch-specific, exactly as described.

Requested change — one regression test:

You're right that the collector half can't be pinned (the destructor mirror masks it; the planted-enumerator probe is the correct evidence there, and I appreciate the straight disclosure). But the OP_DISPATCH stall-counter half is pinnable — it's the user-visible fix your own CHANGELOG entry describes, and the repro above is red on pre-fix main and green here. Please add it as a check (natural home: tests/test_dispatch.eigs, or test_loop_exit.eigs if you prefer it with the other __loop_iterations__ pins) asserting the dispatch-per-iteration loop's __loop_iterations__ matches its no-dispatch twin rather than freezing at 1. Repo rule: every bug fix lands with a test that fails without it — this one is cheap and this PR earned it.

One gate note: ASan+UBSan (full suite) is still running as I post this; it needs to be green before merge, as usual for anything touching the collector.

Everything else — the vm.h owned-set contract comment, the rules-file update merged with the #806 sentence, the deliberate non-conversion of vm_error_halt/CHECK_ERROR and the OP_RETURN family — is exactly how I'd want it done.

[Nitjsefnie]

Test added in 12ab915, appended rather than amended so your row-by-row verification still points at a commit that exists. Also corrected the body's "five hand-written init sites" to four — you're right that :4264 is LOOP_ENV_FRESH, an env-ownership swap rather than a push; the body had inherited the issue's miscount.

I put it in tests/test_dispatch.eigs, not test_loop_exit.eigs. That file is the self-asserting kind — check of [label, cond], gated on rc plus the marker, with the count read back through derive_count, so the tally went 15→18 with no runner edit. test_loop_exit.eigs is print-and-scrape (positional grep -A1, a hardcoded [11/15] header, manual TOTAL=$((TOTAL + 1))), so the comparison would have had to live in shell — and it's also where the OP_DISPATCH pins for #353 and #459 already are.

One correction, and it matters for how the check is written. The invariant as you phrased it — the dispatch loop's __loop_iterations__ matching its no-dispatch twin — isn't true on a fixed build once more than one loop runs, because the counter is cumulative process-wide rather than per-loop. Same program, both binaries:

--- PRE-FIX (cd9f82e) ---        --- FIXED (97ffab6) ---
disp-first: n=17 counter=1       disp-first: n=17 counter=17
plain A:    n=88 counter=89      plain A:    n=88 counter=105
plain B:    n=88 counter=177     plain B:    n=88 counter=193

On the fixed build each loop's delta equals its own iteration count (17, 105−17=88, 193−105=88); pre-fix the dispatch loop contributes 1 instead of 17 and every later reading stays shifted by the shortfall. Raw values coincide only in a single-loop program — which is exactly the shape of your repro, so it reads true there. So I pinned the delta invariant instead: each loop's contribution equals its own counted iterations, checked for both the plain and the dispatch loop, plus a non-vacuity guard that the dispatch loop ran more than once (a 1-iteration loop would satisfy the delta check even with the counter frozen).

Red on pre-fix main, exactly one check flipping:

FAIL: dispatch loop contributes its own iteration count
Dispatch Tests: 17/18 passed

and 18/18 · All tests passed here. The plain-loop and non-vacuity checks pass on both, as controls should.

Incidentally your repro prints 34 on your box and 17 on mine from identical source — the convergence heuristic is entropy-driven — so nothing in the test depends on either number.

Suite: 3367/3367 release, 3371/3371 under ASan+UBSan with leak detection, zero sanitizer findings, [68] Dispatch at 18/18 on both. The previous head came back 16/16 on CI including the ASan job you were waiting on.

What this still doesn't pin, so it's on the record rather than in a commit message: the saved_stall_count half has no user-visible reader I could find, and the "dispatch keeps the caller's budget where OP_CALL zeroes it" asymmetry is real and measurable but pre-existing design, not something this PR changed — I left it unpinned rather than freezing a semantic you may want to revisit.

[InauguralPhysicist]

Approving. Verified the new test red-then-green locally: on a pre-fix binary (current main + an unrelated refactor, vm.c untouched) tests/test_dispatch.eigs fails 17/18 with exactly one check flipping — FAIL: dispatch loop contributes its own iteration count — and on 12ab915 it's 18/18. The delta-invariant framing is correct and better than what I asked for: you're right that the counter is cumulative process-wide, so the raw-value equality I proposed only holds in single-loop programs (my repro's shape, which is why it read true). The non-vacuity guard closes the 1-iteration hole. Good call putting it in test_dispatch.eigs next to the #353/#459 pins with the self-asserting harness, and thank you for appending rather than amending — the row-by-row review does still point at real commits.

The saved_stall_count half staying unpinned is the right call for the reason you gave: it's pre-existing design with no user-visible reader, and freezing it in a test would turn a revisitable choice into a compatibility promise. On the record is the right place for it.

All 16 checks green on the new head. Merging.