From 97ffab6b1a6aabfbe52d8e8ee5dbd4da0027df4f Mon Sep 17 00:00:00 2001 From: Nitjsefnie Date: Sun, 2 Aug 2026 10:39:33 +0200 Subject: [PATCH 1/2] gc+vm: single-definition GC edge table and CallFrame init/release (#743) gc_clear_node missed the VAL_FN -> compiled-chunk edge that GC_FOR_EACH_CHILD reports (body_count == -1, the OP_CLOSURE ref); the ref was only recovered later by the value destructor in the unpin pass -- a third mirror the lockstep comment never named. Both dispatches are now generated from one GC_EDGE_TABLE X-macro (one row per owned edge: walk predicate + clear action side by side), so a new owning edge out of Value/Env/Chunk is added exactly once and the two cannot drift. The value destructor's chunk_decref is documented as the non-cycle mirror. Two safety invariants of the VAL_FN chunk row are now spelled out at the table: GC_EDGE_CLEAR ignores IS_NODE, so the unconditional chunk_decref((EigsChunk *)fn.body) is type-safe ONLY because make_fn sets body = NULL (OP_CLOSURE is its sole real-chunk writer, body_count == -1) and chunk_decref(NULL) no-ops; and the `body = NULL` in that CLEAR is load-bearing -- it makes the value destructor's later chunk_decref a no-op rather than a double-decref (use-after-free). CallFrame init was open-coded at every frame push and release at every saved-frame teardown loop. All 4 push sites (jit_helper_call, vm_run_ex entry, OP_CALL, OP_DISPATCH) now use callframe_init. callframe_release is declared in vm.h (non-static) so ALL THREE saved-frame teardown loops -- task_sched_thread_free and task_do_kill (vm.c) and task_free (builtins.c) -- share the one definition of the owned-field set {env iff owns_env, chunk}; no hand-encoded CallFrame release remains at any teardown site. The live-frame POP paths (OP_RETURN, vm_error_halt, the CHECK_ERROR unwind) keep their inline drops on purpose: they also drain the operand-stack window and restore the per-frame loop-stall globals, and OP_RETURN parks reusable envs / defers the chunk ref -- a plain release would be a subset. The vm.h owned-set comment names all three teardown sites and flags those pop paths for audit. The refactor also fixes a real bug it was designed to catch: the OP_DISPATCH push never stamped saved_stall_count/saved_loop_iter, so on RETURN the interpreted-dispatch path restored those unstamped counter values instead of the correct saved ones. This restore-on-RETURN is new, corrected behavior on the dispatch path, not pre-#743 parity, and a probe makes the change observable: pre-fix a dispatch-per-iteration loop's __loop_iterations__ froze at a wrong value (1) after the first dispatch RETURN; post-fix it tracks the loop (41..44). Rebased onto cd9f82e, which carries #806 (the ValType switches went exhaustive, gc_clear_node and GC_FOR_EACH_CHILD among them). Both conflicts resolve to this table: it supersedes the two hand-written dispatches, and #806's own edit to gc_clear_node's VAL_FN case did NOT add the missing chunk edge, so the bug this closes is still live on main. #806's compile-time forcing is preserved rather than dropped: because the table's rows are selected by `_v->type == VAL_x` guards, which -Werror=switch cannot see, gc_value_is_node now carries the gate (exhaustive switch, no default: arm; a value type is a node exactly when it has rows). Verified by planting an 11th enumerator -- the build fails at gc_value_is_node, as it does at #806's other 15 sites. Gates (umask 022, on the rebase onto cd9f82e): CC=gcc ./build.sh rc=0; release run_all_tests.sh 3364/3364; make asan rc=0; the suite under CI's sanitizer env (ASAN_OPTIONS=detect_leaks=1, UBSAN_OPTIONS halt_on_error + print_stacktrace) 3368/3368, zero LeakSanitizer/AddressSanitizer reports; [87] closure-cycle strict leak gate clean direct under LSan (17/17, rc=0) and [101] test_spawn_gc rc=0. The task_free conversion is proven leak-free under ASan+LSan by a probe that suspends owns_env worker frames inside a try and tears them down via task_free at exit with zero findings (byte-identical output before and after the rebase). Co-Authored-By: Kimi K3 Co-Authored-By: Claude Opus 5 --- .claude/rules/c-runtime-memory.md | 20 +- CHANGELOG.md | 40 ++++ src/builtins.c | 9 +- src/eigenscript.c | 317 +++++++++++++++++------------- src/vm.c | 140 ++++++------- src/vm.h | 19 ++ 6 files changed, 330 insertions(+), 215 deletions(-) diff --git a/.claude/rules/c-runtime-memory.md b/.claude/rules/c-runtime-memory.md index e521f0d5..12b3eb80 100644 --- a/.claude/rules/c-runtime-memory.md +++ b/.claude/rules/c-runtime-memory.md @@ -24,14 +24,18 @@ iteration, or a collector that quietly stops working). ref via `env_incref`/`env_decref`. Never stash a bare `Env*` that outlives its creator. The collector's `GC_FOR_EACH_CHILD` walker and `gc_clear_node` (eigenscript.c) must move in lockstep with the ownership - model: a new owning edge out of Value/Env/Chunk goes into both, and only - *counted* edges may be traversed (an uncounted edge trips the accounting - abort and collection silently stops working). Conservative direction: - missing an edge leaks; inventing one frees live memory. A new `ValType` - or `ASTType` is a **build error** at every switch that must learn about - it (#737/#738: no `default:` arms on closed-enum switches — - `-Werror=switch` enforces exhaustiveness; don't add a `default:` back, - enumerate the no-op cases instead). + model, so both are generated from ONE table: `GC_EDGE_TABLE` + (eigenscript.c). A new owning edge out of Value/Env/Chunk is one new row + there, and only *counted* edges may be walked (an uncounted edge trips + the accounting abort and collection silently stops working). + Conservative direction: missing an edge leaks; inventing one frees live + memory. A new `ValType` or `ASTType` is a **build error** at every switch + that must learn about it (#737/#738: no `default:` arms on closed-enum + switches — `-Werror=switch` enforces exhaustiveness; don't add a + `default:` back, enumerate the no-op cases instead). The table's rows are + selected by `_v->type == VAL_x` guards, which `-Werror=switch` cannot + see, so `gc_value_is_node` carries that gate for it: its switch is + exhaustive, and a value type is a node exactly when it has rows. - **Trace gating**: `g_trace_hist` (assignment history) and `g_trace_obs_hist` (observer snapshots) are compiler-set flags — recording is off unless the program contains a temporal query diff --git a/CHANGELOG.md b/CHANGELOG.md index d94d59be..ca846965 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,46 @@ All notable changes to EigenScript are documented here. ## [Unreleased] +### Fixed + +- **The GC's two lockstep dispatches are generated from one table, and + `CallFrame` init/release have one definition each (#743).** The + lockstep between `GC_FOR_EACH_CHILD` and `gc_clear_node` was enforced + only by prose, and had already drifted: the walker reported a + `VAL_FN`'s compiled-chunk edge (`body_count == -1`, the ref taken in + `OP_CLOSURE`) that `gc_clear_node` never cleared, so that ref was + recovered only later by the value destructor in the unpin pass — a + third mirror the comment never named. Both dispatches now expand one + `GC_EDGE_TABLE` X-macro (one row per owned edge, walk predicate and + clear action side by side), so a new owning edge out of + Value/Env/Chunk is written once and the two cannot drift; the + destructor is documented as the non-cycle mirror, and the two safety + invariants of the chunk row's clear are spelled out at the table + (`make_fn` leaves `body = NULL`, which is what makes the + unconditional `chunk_decref` type-safe; the `body = NULL` in the + clear is load-bearing against a later double-decref). Since rows are + selected by `_v->type == VAL_x` guards, which `-Werror=switch` cannot + see, `gc_value_is_node` carries #738's build-error property for the + table: its switch is exhaustive and a value type is a node exactly + when it has rows (verified by planting an 11th enumerator). + Alongside it, `CallFrame` had five hand-written init/teardown sites: + all four push sites (`jit_helper_call`, `vm_run_ex` entry, `OP_CALL`, + `OP_DISPATCH`) now use `callframe_init`, and all three saved-frame + teardown loops (`task_sched_thread_free` and `task_do_kill` in vm.c, + `task_free` in builtins.c) share `callframe_release` — declared in + vm.h, the one definition of the owned-field set {env iff `owns_env`, + chunk}. The live-frame POP paths keep their inline drops on purpose + (they also drain the operand-stack window, restore the per-frame + stall globals, and park reusable envs — a plain release would be a + subset), and vm.h now flags them for audit. The refactor fixes the + bug it was designed to catch: the `OP_DISPATCH` push never stamped + `saved_stall_count`/`saved_loop_iter`, so interpreted dispatch + restored unstamped counters on RETURN — a dispatch-per-iteration + loop's `__loop_iterations__` froze at 1 after the first dispatch + return, and now tracks the loop. Contributed by @Nitjsefnie (#810); + section [87]'s strict closure-cycle leak gate covers the collector + half. + ### Changed - **Build variants now coexist: per-variant objdirs with dependency diff --git a/src/builtins.c b/src/builtins.c index cbfa7215..3f974770 100644 --- a/src/builtins.c +++ b/src/builtins.c @@ -5512,10 +5512,13 @@ void task_free(Task *t) { CallFrame *f = &t->saved_frames[i]; /* Same rebalance as task_do_kill: a task torn down while suspended * inside a try never runs its TRY_ENDs, and the leftover process- - * global depth silences every later uncaught error (#726). */ + * global depth silences every later uncaught error (#726). The + * g_try_depth fixup is frame bookkeeping, not an owned ref, so it + * stays here; the owned-field drop (env iff owns_env, then chunk) + * goes through vm.c's shared callframe_release (#743) — this is the + * third saved-frame teardown loop, matching the two in vm.c. */ g_try_depth -= f->try_count; - if (f->owns_env && f->env) env_decref(f->env); - if (f->chunk) chunk_decref(f->chunk); + callframe_release(f); } if (g_try_depth < 0) g_try_depth = 0; free(t->saved_frames); diff --git a/src/eigenscript.c b/src/eigenscript.c index 6912e669..85bdd129 100644 --- a/src/eigenscript.c +++ b/src/eigenscript.c @@ -968,7 +968,12 @@ void free_value(Value *v) { free(v->data.fn.body); } /* body_count == -1 means bytecode fn: body is a chunk ptr; - * this fn holds a ref on it (taken in OP_CLOSURE). */ + * this fn holds a ref on it (taken in OP_CLOSURE). This is the + * non-cycle mirror of the VAL_FN rows of GC_EDGE_TABLE (the + * collector section below): a fn collected as cyclic garbage already + * dropped this ref in gc_clear_node (which NULLs body first), + * so the decref below is a no-op for collected fns and the + * real drop for fns that die by ordinary refcounting. */ if (v->data.fn.body_count == -1) chunk_decref((struct EigsChunk *)v->data.fn.body); { @@ -2454,10 +2459,35 @@ static int gcu_add(GcU *u, void *obj, int kind) { /* Only these value types can hold references (and so participate in an * env-involving cycle). Everything else is a leaf: dropped by ordinary - * decrefs when its garbage owner is cleared. */ + * decrefs when its garbage owner is cleared. + * + * A value type is a node EXACTLY WHEN it has rows in GC_EDGE_TABLE below, + * so this switch is also the ValType exhaustiveness gate for that table: + * the table's rows are selected by `_v->type == VAL_x` guards, which + * -Werror=switch cannot see, so without this a new ValType would compile + * silently against both dispatches (the #737/#738 build-error property — + * closed-enum switches carry no `default:` arm; enumerate no-op cases + * instead). Adding a ValType breaks the build here: decide node vs leaf, + * and give a node its GC_EDGE_TABLE rows. */ static int gc_value_is_node(Value *v) { - return v && !v->arena && - (v->type == VAL_LIST || v->type == VAL_DICT || v->type == VAL_FN); + if (!v || v->arena) return 0; + switch (v->type) { + /* Nodes: each has one or more GC_EDGE_TABLE rows. */ + case VAL_LIST: + case VAL_DICT: + case VAL_FN: + return 1; + /* Leaves: no owned edges out, so no rows and never a cycle member. */ + case VAL_NUM: + case VAL_STR: + case VAL_NULL: + case VAL_JSON_RAW: + case VAL_BUILTIN: + case VAL_BUFFER: + case VAL_TEXT_BUILDER: + return 0; + } + return 0; /* unreachable for valid ValType values */ } /* An env child edge worth traversing: real, heap, and not the global @@ -2466,144 +2496,163 @@ static int gc_env_is_node(Env *e) { return e && e->heap_allocated && e != g_global_env; } -/* Invoke BODY for every OWNED edge out of node n. Each edge reported - * here corresponds to exactly one counted reference; reporting anything - * uncounted here would corrupt the root derivation (the abort check - * below catches that as internal > refcount). Chunk edges: a VAL_FN owns - * one ref on its compiled chunk (taken in OP_CLOSURE); a chunk owns one - * creator ref per nested functions[] entry and one ref on its parked - * env_cache. Chunk constants are literal leaves (never containers/fns) - * and are not traversed. */ -#define GC_FOR_EACH_CHILD(u, n, CHILD_OBJ, CHILD_KIND, BODY) \ - do { \ - if ((u)->kind[n] == GC_KIND_ENV) { \ - Env *_e = (Env *)(u)->objs[n]; \ - if (gc_env_is_node(_e->parent)) { \ - void *CHILD_OBJ = _e->parent; \ - int CHILD_KIND = GC_KIND_ENV; BODY \ - } \ - for (int _i = 0; _i < _e->count; _i++) { \ - EigsSlot _s = _e->values[_i]; \ - if (slot_is_ptr(_s)) { \ - Value *_v = slot_as_ptr(_s); \ - if (gc_value_is_node(_v)) { \ - void *CHILD_OBJ = _v; \ - int CHILD_KIND = GC_KIND_VAL; BODY \ - } \ - } \ - } \ - } else if ((u)->kind[n] == GC_KIND_CHUNK) { \ - EigsChunk *_c = (EigsChunk *)(u)->objs[n]; \ - for (int _i = 0; _i < _c->fn_count; _i++) { \ - if (_c->functions[_i]) { \ - void *CHILD_OBJ = _c->functions[_i]; \ - int CHILD_KIND = GC_KIND_CHUNK; BODY \ - } \ - } \ - if (gc_env_is_node(_c->env_cache)) { \ - void *CHILD_OBJ = _c->env_cache; \ - int CHILD_KIND = GC_KIND_ENV; BODY \ - } \ - } else { \ - Value *_v = (Value *)(u)->objs[n]; \ - switch (_v->type) { \ - case VAL_FN: \ - if (gc_env_is_node(_v->data.fn.closure)) { \ - void *CHILD_OBJ = _v->data.fn.closure; \ - int CHILD_KIND = GC_KIND_ENV; BODY \ - } \ - if (_v->data.fn.body_count == -1 && _v->data.fn.body) { \ - void *CHILD_OBJ = (EigsChunk *)_v->data.fn.body; \ - int CHILD_KIND = GC_KIND_CHUNK; BODY \ - } \ - break; \ - case VAL_LIST: \ - for (int _i = 0; _i < _v->data.list.count; _i++) { \ - Value *_c2 = _v->data.list.items[_i]; \ - if (gc_value_is_node(_c2)) { \ - void *CHILD_OBJ = _c2; \ - int CHILD_KIND = GC_KIND_VAL; BODY \ - } \ - } \ - break; \ - case VAL_DICT: \ - for (int _i = 0; _i < _v->data.dict.count; _i++) { \ - Value *_c2 = _v->data.dict.vals[_i]; \ - if (gc_value_is_node(_c2)) { \ - void *CHILD_OBJ = _c2; \ - int CHILD_KIND = GC_KIND_VAL; BODY \ - } \ - } \ - break; \ - /* Leaf types: no outgoing edges. Enumerated rather than \ - * covered by a `default:` so -Werror=switch makes a new \ - * ValType a build error at every expansion of this macro. */ \ - case VAL_NUM: case VAL_STR: case VAL_NULL: \ - case VAL_JSON_RAW: case VAL_BUILTIN: \ - case VAL_BUFFER: case VAL_TEXT_BUILDER: \ - break; \ +/* ========================================================================= + * GC edge table — the SINGLE definition of every owned edge out of a GC + * node (#743). The two dispatches that must move in lockstep are both + * generated from this table: + * + * - GC_FOR_EACH_CHILD, the collector's walker (U build, internal + * refcounting, root marking), expands GC_EDGE_WALK over the rows; + * - gc_clear_node, the cycle-breaker, expands GC_EDGE_CLEAR. + * + * A new owning edge out of Value/Env/Chunk is ONE new row here, so the + * two can no longer drift apart (previously the walker reported VAL_FN's + * compiled-chunk edge while gc_clear_node never cleared it). + * + * Row: X(GUARD, COUNT, CHILD, CHILD_KIND, IS_NODE, CLEAR) + * GUARD which container the row belongs to (_e/_c/_v alias the + * node, _k is its GC_KIND_*) + * COUNT number of edge slots (1 for a scalar edge) + * CHILD child object expr (evaluated only when IS_NODE holds) + * CHILD_KIND GC_KIND_* of the child + * IS_NODE counted-edge-and-worth-traversing predicate. Each reported + * edge corresponds to exactly one counted reference; + * reporting anything uncounted would corrupt the root + * derivation (the abort check below catches that as + * internal > refcount). Slots failing IS_NODE are leaf refs: + * never walked, but the clear still drops them. + * CLEAR per-slot statement dropping the edge (leaf refs included) + * + * Chunk edges: a VAL_FN owns one ref on its compiled chunk (taken in + * OP_CLOSURE); a chunk owns one creator ref per nested functions[] entry + * and one ref on its parked env_cache. Chunk constants are literal leaves + * (never containers/fns) and are not traversed. A fn that dies by ordinary + * refcounting (never cycle-broken) instead drops its closure/chunk refs + * from free_value — the value destructor is the non-cycle mirror of this + * table for VAL_FN's two edges. + * ========================================================================= */ +#define GC_EDGE_TABLE(X, ...) \ + /* Env: the parent link, then every value slot. */ \ + X((_k == GC_KIND_ENV), 1, \ + _e->parent, GC_KIND_ENV, \ + gc_env_is_node(_e->parent), \ + { Env *_o = _e->parent; \ + _e->parent = NULL; \ + env_decref(_o); }, __VA_ARGS__) \ + X((_k == GC_KIND_ENV), _e->count, \ + slot_as_ptr(_e->values[_i]), GC_KIND_VAL, \ + (slot_is_ptr(_e->values[_i]) && \ + gc_value_is_node(slot_as_ptr(_e->values[_i]))), \ + { EigsSlot _o = _e->values[_i]; \ + _e->values[_i] = slot_null(); \ + slot_decref(_o); }, __VA_ARGS__) \ + /* Chunk: creator refs on nested functions[], parked env_cache. */ \ + X((_k == GC_KIND_CHUNK), _c->fn_count, \ + _c->functions[_i], GC_KIND_CHUNK, \ + (_c->functions[_i] != NULL), \ + { EigsChunk *_o = _c->functions[_i]; \ + _c->functions[_i] = NULL; \ + chunk_decref(_o); }, __VA_ARGS__) \ + X((_k == GC_KIND_CHUNK), 1, \ + _c->env_cache, GC_KIND_ENV, \ + gc_env_is_node(_c->env_cache), \ + { Env *_o = _c->env_cache; \ + _c->env_cache = NULL; \ + env_decref(_o); }, __VA_ARGS__) \ + /* VAL_FN: closure env, then the compiled-chunk ref taken in \ + * OP_CLOSURE (body_count == -1 — the edge gc_clear_node once missed). \ + * Two invariants keep the chunk row's CLEAR safe (GC_EDGE_CLEAR ignores \ + * IS_NODE — see its macro — so the CLEAR statement runs for EVERY \ + * cleared VAL_FN regardless of body_count, unlike the guarded WALK): \ + * (1) chunk_decref((EigsChunk *)fn.body) runs unconditionally, but is \ + * type-safe ONLY because make_fn sets body = NULL (body_count == 0) \ + * and OP_CLOSURE is the sole writer that makes it a real chunk (and \ + * sets body_count == -1); chunk_decref(NULL) no-ops for the \ + * never-closured case. A future VAL_FN constructor that put a \ + * NON-chunk pointer in body (e.g. reviving AST bodies, body_count \ + * != -1) would turn this into a type-confused decref — guard the \ + * CLEAR on body_count == -1 if that ever happens. \ + * (2) The `body = NULL` below is LOAD-BEARING: it makes free_value's \ + * later `body_count == -1` chunk_decref a no-op instead of a \ + * double-decref (use-after-free) when a collected fn is then freed. \ + * A "simplification" that drops it reintroduces the double free. */ \ + X((_k == GC_KIND_VAL && _v->type == VAL_FN), 1, \ + _v->data.fn.closure, GC_KIND_ENV, \ + gc_env_is_node(_v->data.fn.closure), \ + { Env *_o = _v->data.fn.closure; \ + _v->data.fn.closure = NULL; \ + env_decref(_o); }, __VA_ARGS__) \ + X((_k == GC_KIND_VAL && _v->type == VAL_FN), 1, \ + (EigsChunk *)_v->data.fn.body, GC_KIND_CHUNK, \ + (_v->data.fn.body_count == -1 && _v->data.fn.body != NULL), \ + { EigsChunk *_o = (EigsChunk *)_v->data.fn.body; \ + _v->data.fn.body = NULL; /* invariant (2): load-bearing */ \ + chunk_decref(_o); }, __VA_ARGS__) \ + /* VAL_LIST / VAL_DICT: every element slot. */ \ + X((_k == GC_KIND_VAL && _v->type == VAL_LIST), _v->data.list.count, \ + _v->data.list.items[_i], GC_KIND_VAL, \ + gc_value_is_node(_v->data.list.items[_i]), \ + { Value *_o = _v->data.list.items[_i]; \ + _v->data.list.items[_i] = NULL; \ + val_decref(_o); }, __VA_ARGS__) \ + X((_k == GC_KIND_VAL && _v->type == VAL_DICT), _v->data.dict.count, \ + _v->data.dict.vals[_i], GC_KIND_VAL, \ + gc_value_is_node(_v->data.dict.vals[_i]), \ + { Value *_o = _v->data.dict.vals[_i]; \ + _v->data.dict.vals[_i] = NULL; \ + val_decref(_o); }, __VA_ARGS__) + +#define GC_EDGE_WALK(GUARD, COUNT, CHILD, CHILD_KIND, IS_NODE, CLEAR, \ + OUT_OBJ, OUT_KIND, BODY) \ + if (GUARD) { \ + for (int _i = 0; _i < (COUNT); _i++) { \ + if (IS_NODE) { \ + void *OUT_OBJ = (void *)(CHILD); \ + int OUT_KIND = (CHILD_KIND); \ + BODY \ } \ } \ + } + +#define GC_EDGE_CLEAR(GUARD, COUNT, CHILD, CHILD_KIND, IS_NODE, CLEAR, \ + _x1, _x2, _x3) \ + if (GUARD) { \ + for (int _i = 0; _i < (COUNT); _i++) \ + CLEAR \ + } + +/* Invoke BODY for every OWNED node edge out of node n (the GC_EDGE_TABLE + * rows whose IS_NODE predicate holds). */ +#define GC_FOR_EACH_CHILD(u, n, CHILD_OBJ, CHILD_KIND, BODY) \ + do { \ + int _k = (u)->kind[n]; \ + Env *_e = (Env *)(u)->objs[n]; \ + EigsChunk *_c = (EigsChunk *)(u)->objs[n]; \ + Value *_v = (Value *)(u)->objs[n]; \ + GC_EDGE_TABLE(GC_EDGE_WALK, CHILD_OBJ, CHILD_KIND, BODY) \ } while (0) /* Clear every outgoing edge of a garbage node (exactly the edges - * GC_FOR_EACH_CHILD reports, plus leaf refs) so the cycle is broken; - * the node itself stays allocated (pinned) until the unpin pass. */ + * GC_EDGE_TABLE lists, leaf refs included) so the cycle is broken; the + * node itself stays allocated (pinned) until the unpin pass. */ static void gc_clear_node(void *obj, int kind) { + int _k = kind; + Env *_e = (Env *)obj; + EigsChunk *_c = (EigsChunk *)obj; + Value *_v = (Value *)obj; + GC_EDGE_TABLE(GC_EDGE_CLEAR, 0, 0, 0) + /* Container bookkeeping that is not an edge. */ if (kind == GC_KIND_ENV) { - Env *e = obj; - for (int i = 0; i < e->count; i++) { - EigsSlot s = e->values[i]; - e->values[i] = slot_null(); - slot_decref(s); - } - e->count = 0; - e->binding_version++; - if (++e->hash.generation == 0) { - memset(e->hash.generations, 0, - (e->hash.mask + 1) * sizeof(uint32_t)); - e->hash.generation = 1; - } - Env *p = e->parent; - e->parent = NULL; - env_decref(p); - } else if (kind == GC_KIND_CHUNK) { - EigsChunk *c = obj; - for (int i = 0; i < c->fn_count; i++) { - EigsChunk *fc = c->functions[i]; - c->functions[i] = NULL; /* chunk_decref(NULL) no-ops later */ - chunk_decref(fc); - } - Env *cached = c->env_cache; - c->env_cache = NULL; - env_decref(cached); - } else { - Value *v = obj; - switch (v->type) { - case VAL_FN: { - Env *clo = v->data.fn.closure; - v->data.fn.closure = NULL; - env_decref(clo); - break; - } - case VAL_LIST: - for (int i = 0; i < v->data.list.count; i++) - val_decref(v->data.list.items[i]); - v->data.list.count = 0; - break; - case VAL_DICT: - for (int i = 0; i < v->data.dict.count; i++) - val_decref(v->data.dict.vals[i]); - v->data.dict.count = 0; - break; - /* Leaf types: no outgoing edges to clear. Enumerated rather than - * covered by a `default:` so -Werror=switch keeps this in lockstep - * with GC_FOR_EACH_CHILD when a ValType is added. */ - case VAL_NUM: case VAL_STR: case VAL_NULL: - case VAL_JSON_RAW: case VAL_BUILTIN: - case VAL_BUFFER: case VAL_TEXT_BUILDER: - break; + _e->count = 0; + _e->binding_version++; + if (++_e->hash.generation == 0) { + memset(_e->hash.generations, 0, + (_e->hash.mask + 1) * sizeof(uint32_t)); + _e->hash.generation = 1; } + } else if (kind == GC_KIND_VAL) { + if (_v->type == VAL_LIST) _v->data.list.count = 0; + else if (_v->type == VAL_DICT) _v->data.dict.count = 0; } } diff --git a/src/vm.c b/src/vm.c index c3e9ae90..5a9944d9 100644 --- a/src/vm.c +++ b/src/vm.c @@ -456,6 +456,60 @@ static inline Env *vm_take_call_env(EigsChunk *fn_chunk, Env *closure, return NULL; } +/* ---- CallFrame init / release (#743) ------------------------------------ + * CallFrame rides the task save/restore memcpy as POD (vm.h), but two of + * its fields carry COUNTED refs — env (only when owns_env) and chunk — + * and every push must stamp the rest (ip, bp, call_serial, try state, + * saved loop-stall counters, call_argc). This init used to be open-coded + * at every frame-push site and the release at every saved-frame teardown + * loop: a field added to the struct could be missed at one site with no + * compiler check (saved_stall_count/saved_loop_iter were never stamped + * at the OP_DISPATCH push, so RETURN restored stale counters there). + * callframe_init is the single init used by every push site; + * callframe_release the single release used by the teardown loops. */ + +/* Push-time init: stamps every field and takes the frame's chunk ref + * (this chunk_incref pairs with the chunk_decref in callframe_release / + * the OP_RETURN family). env is ADOPTED, not incref'd: the caller hands + * over the call-env ref it made (owns_env=1) or lends a borrowed env it + * does not own (base frames, owns_env=0). try_handlers stay unstamped on + * purpose — POD garbage guarded by try_count == 0, as before. */ +static void callframe_init(CallFrame *f, EigsChunk *chunk, Env *env, + Value *closure_val, int owns_env, int call_argc) { + f->chunk = chunk; + f->call_serial = ++g_call_serial_next; /* #539 v2 */ + chunk_incref(chunk); /* frame's ref — released when this frame pops */ + f->ip = chunk->code; + f->bp = g_vm.sp; + f->env = env; + f->fn_env = env; + f->closure_val = closure_val; + f->owns_env = owns_env; + f->is_try = 0; + f->try_count = 0; + /* Scope loop-stall globals per call frame: a callee's loops must not + * inherit caller's accumulated stall count, or a hot helper (e.g. + * fmt_num's padding loop) called from a converging outer loop will + * exit early once the global crosses the threshold. Call sites that + * grant the callee a fresh budget zero the globals right after this + * returns; the base frame and OP_DISPATCH keep the incoming values. */ + f->saved_stall_count = g_loop_stall_count; + f->saved_loop_iter = g_loop_iterations; + f->call_argc = call_argc; +} + +/* Pop/teardown-time release: drop the frame's counted refs — its env iff + * owned, and its chunk ref. The owned-field set has exactly this one + * definition. The OP_RETURN family deliberately does NOT use it: Stage + * 5i parks reusable call envs instead of decref'ing, and the JIT return + * helpers defer the chunk ref to vm_run's -1 sentinel handler. Declared in + * vm.h (non-static): builtins.c's task_free is the third saved-frame teardown + * loop and calls the same helper across the translation unit. */ +void callframe_release(CallFrame *f) { + if (f->owns_env && f->env) env_decref(f->env); + if (f->chunk) chunk_decref(f->chunk); +} + /* ---- Dict field inline cache ---- * * Stage 5h: 2-way set-associative (64 sets × 2 ways = the same 128 @@ -2143,20 +2197,7 @@ int jit_helper_call(EigsChunk *caller_chunk, int argc, int resume_off) { * only on the deep-bail paths, where the interpreter takes * over mid-thunk. */ CallFrame *frame = &g_vm.frames[g_vm.frame_count++]; - frame->chunk = fn_chunk; - frame->call_serial = ++g_call_serial_next; /* #539 v2 */ - chunk_incref(fn_chunk); - frame->ip = fn_chunk->code; - frame->bp = g_vm.sp; - frame->env = call_env; - frame->fn_env = call_env; - frame->closure_val = fn_val; - frame->owns_env = 1; - frame->is_try = 0; - frame->try_count = 0; - frame->saved_stall_count = g_loop_stall_count; - frame->saved_loop_iter = g_loop_iterations; - frame->call_argc = argc; + callframe_init(frame, fn_chunk, call_env, fn_val, 1, argc); g_loop_stall_count = 0; g_loop_iterations = 0; fn_chunk->exec_count++; @@ -2506,26 +2547,13 @@ static Value *vm_run_ex(EigsChunk *chunk, Env *env, Task *resume) { env_reserve_slots(env, chunk->local_count); } frame = &g_vm.frames[g_vm.frame_count++]; - frame->chunk = chunk; - frame->call_serial = ++g_call_serial_next; /* #539 v2 */ - chunk_incref(chunk); /* frame's ref — released when this frame pops */ - frame->ip = chunk->code; - frame->bp = g_vm.sp; - frame->env = env; - frame->fn_env = env; - frame->closure_val = NULL; - frame->owns_env = 0; - frame->is_try = 0; - frame->try_count = 0; - frame->saved_stall_count = g_loop_stall_count; - frame->saved_loop_iter = g_loop_iterations; /* Entry paths via vm_execute (thread_entry, call_eigs_fn, dispatch, * HTTP handlers, module-level) have already bound every param in - * env before getting here. Mark all slots as caller-supplied so - * OP_DEFAULT_PARAM doesn't re-fire defaults over them — and so a - * stale value left by a prior frame at this depth can't clobber - * explicit args. */ - frame->call_argc = chunk->param_count; + * env before getting here. call_argc = param_count marks all slots + * as caller-supplied so OP_DEFAULT_PARAM doesn't re-fire defaults + * over them — and so a stale value left by a prior frame at this + * depth can't clobber explicit args. */ + callframe_init(frame, chunk, env, NULL, 0, chunk->param_count); /* #297: JIT hotness bookkeeping is shared per-chunk state and feeds JIT * compilation, which is gated off under MT (#296). Skip it while * multithreaded — pointless work, and the bare ++ / registry write race @@ -3655,25 +3683,7 @@ static Value *vm_run_ex(EigsChunk *chunk, Env *env, Task *resume) { DISPATCH(); } frame = &g_vm.frames[g_vm.frame_count++]; - frame->chunk = fn_chunk; - frame->call_serial = ++g_call_serial_next; /* #539 v2 */ - chunk_incref(fn_chunk); /* frame's ref — the fn value popped - * above may die mid-call */ - frame->ip = fn_chunk->code; - frame->bp = g_vm.sp; - frame->env = call_env; - frame->fn_env = call_env; - frame->closure_val = fn_val; - frame->owns_env = 1; /* OP_CALL created this env, free on return */ - frame->is_try = 0; - frame->try_count = 0; - /* Scope loop-stall globals per call frame: a callee's loops must - * not inherit caller's accumulated stall count, or a hot helper - * (e.g. fmt_num's padding loop) called from a converging outer - * loop will exit early once the global crosses the threshold. */ - frame->saved_stall_count = g_loop_stall_count; - frame->saved_loop_iter = g_loop_iterations; - frame->call_argc = (int)argc; + callframe_init(frame, fn_chunk, call_env, fn_val, 1, (int)argc); g_loop_stall_count = 0; g_loop_iterations = 0; if (!g_vm_multithreaded) fn_chunk->exec_count++; /* #297: shared, JIT-only */ @@ -5559,18 +5569,12 @@ static Value *vm_run_ex(EigsChunk *chunk, Env *env, Task *resume) { DISPATCH(); } frame = &g_vm.frames[g_vm.frame_count++]; - frame->chunk = fn_chunk; - frame->call_serial = ++g_call_serial_next; /* #539 v2 */ - chunk_incref(fn_chunk); /* frame's ref */ - frame->ip = fn_chunk->code; - frame->bp = g_vm.sp; - frame->env = call_env; - frame->fn_env = call_env; - frame->closure_val = fn; - frame->owns_env = 1; - frame->is_try = 0; - frame->try_count = 0; - frame->call_argc = 1; + /* callframe_init also stamps saved_stall_count/saved_loop_iter, + * which this site used to leave unset — RETURN then restored + * stale counters from a previous frame at this depth (#743). + * The globals are deliberately NOT zeroed here (dispatch keeps + * the caller's budget, matching pre-#743 behavior). */ + callframe_init(frame, fn_chunk, call_env, fn, 1, 1); if (!g_vm_multithreaded) fn_chunk->exec_count++; /* #297: shared, JIT-only */ /* JIT hook (mirror of OP_CALL bytecode-fn path; #533 task gate, @@ -5747,11 +5751,8 @@ void task_sched_thread_free(void) { free(m->saved_stack); } if (m->saved_frames) { - for (int i = 0; i < m->saved_frame_count; i++) { - CallFrame *f = &m->saved_frames[i]; - if (f->owns_env && f->env) env_decref(f->env); - if (f->chunk) chunk_decref(f->chunk); - } + for (int i = 0; i < m->saved_frame_count; i++) + callframe_release(&m->saved_frames[i]); free(m->saved_frames); } if (m->mbox) { @@ -6095,8 +6096,7 @@ int task_do_kill(int tid) { * gate suppress the diagnostic of every later uncaught error in * the process — confirmed, the program exits 1 in silence (#726). */ g_try_depth -= f->try_count; - if (f->owns_env && f->env) env_decref(f->env); - if (f->chunk) chunk_decref(f->chunk); + callframe_release(f); } if (g_try_depth < 0) g_try_depth = 0; free(t->saved_frames); t->saved_frames = NULL; t->saved_frame_count = 0; diff --git a/src/vm.h b/src/vm.h index a2d06c0b..a7c21256 100644 --- a/src/vm.h +++ b/src/vm.h @@ -424,6 +424,19 @@ typedef struct EigsChunk { #define MAX_TRY_HANDLERS 8 typedef struct { + /* #743: the OWNED fields of a frame are exactly {env (iff owns_env), + * chunk}; their init and release have a single definition in vm.c — + * callframe_init (every frame push) and callframe_release (declared + * below, non-static for cross-TU use). callframe_release covers ALL + * THREE saved-frame teardown loops: task_sched_thread_free and + * task_do_kill (vm.c) and task_free (builtins.c). Add an owned field to + * one of those two helpers, never to an individual teardown site. The + * live-frame POP paths — the OP_RETURN family, vm_error_halt, and the + * CHECK_ERROR unwind — deliberately inline the same drops because they + * ALSO drain the operand-stack window and restore the per-frame + * loop-stall globals (and OP_RETURN parks reusable envs / defers the + * chunk ref to a -1 sentinel); a new owned field must be audited there + * too. */ EigsChunk *chunk; uint8_t *ip; /* instruction pointer */ int bp; /* base pointer into value stack */ @@ -452,6 +465,12 @@ typedef struct { * (POD — rides the task save/restore memcpy). */ } CallFrame; +/* #743: drop a saved frame's owned refs — env iff owns_env, then chunk. The + * single release definition (see the CallFrame comment above for the owned-set + * invariant); non-static so builtins.c's task_free reaches the same helper as + * the two vm.c teardown loops. Live-frame POP paths do NOT use it. */ +void callframe_release(CallFrame *f); + /* ---- VM State ---- */ #define VM_STACK_MAX 65536 #define VM_FRAMES_MAX 4096 From 12ab91563528f2cc560b0b35fbb2973cff436770 Mon Sep 17 00:00:00 2001 From: Nitjsefnie Date: Sun, 2 Aug 2026 14:43:38 +0200 Subject: [PATCH 2/2] tests: pin the OP_DISPATCH loop-counter stamping (#743) Lands per review on #810: the collector half of #743 cannot be pinned (the value destructor masks the missed edge, which is why the planted-enumerator probe is the evidence there), but the loop-counter half is user-visible and red on pre-fix main, so it gets a regression test. Three checks in section [68], mid-file with the other OP_DISPATCH pins; the runner derives its tally from the file's own "Tests: N" line, so no run_all_tests.sh change is needed. The pin is on the DELTA, not on a count. Measuring first showed __loop_iterations__ is a cumulative process-wide counter, not a per-loop one: on this box a dispatch-per-iteration loop reads 105 where the plain loop before it read 88, because the reading carries every earlier loop's iterations. So "the dispatch loop's counter equals its no-dispatch twin's counter" is not true as a raw-value comparison in a program with more than one loop -- what holds is that each loop CONTRIBUTES exactly its own iteration count. Both loops are asserted against that invariant, comparing each loop's delta with a count it keeps by hand, so the pin survives any retuning of the convergence heuristic. Hardcoding the number would have been wrong anyway: the reviewer's box prints 34 where this one prints 17. Two shapes the test needs to stay honest. A third check asserts the dispatch loop ran more than one iteration -- without it a single-iteration loop would satisfy the delta check even with the counter frozen at 1, and the pin would be vacuous. And a warm-up loop establishes the baseline the two measured loops subtract, because __loop_iterations__ is an undefined variable until some loop has run (the pre-fix binary is what surfaced that: it failed on the baseline read before ever reaching the assertion). Mutation-proved rather than assumed. Against a release build of upstream/main at cd9f82e (pre-fix, built in a worktree): "FAIL: dispatch loop contributes its own iteration count", Dispatch Tests: 17/18, rc=1. Against this branch: 18/18, "All tests passed", rc=0. Exactly one check flips -- the plain-loop and non-vacuity checks pass on both, as they should. Gates (umask 022): CC=gcc ./build.sh rc=0, release run_all_tests.sh 3367/3367; make asan rc=0, suite under CI's sanitizer env (ASAN_OPTIONS=detect_leaks=1, UBSAN_OPTIONS halt_on_error + print_stacktrace) 3371/3371 with zero LeakSanitizer/AddressSanitizer reports and no leak-tally note; test_dispatch.eigs run directly under ASan+LSan rc=0. Co-Authored-By: Claude Opus 5 --- tests/test_dispatch.eigs | 52 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/tests/test_dispatch.eigs b/tests/test_dispatch.eigs index 383a64ca..3578cd84 100644 --- a/tests/test_dispatch.eigs +++ b/tests/test_dispatch.eigs @@ -5,7 +5,8 @@ # are always immediate-num and dropped the slow path — exposed by DMG's # per-instruction `dispatch of [_op_table, op, _exec_ctx]` where `op` # comes from a fetch8 call). Also pins the float-discipline promise on -# the dispatch key. +# the dispatch key, and the loop counters the OP_DISPATCH push owes its +# CallFrame (#743). pass_count is 0 fail_count is 0 @@ -108,6 +109,55 @@ catch e: caught_var is e check of ["literal/variable forms agree", contains of [caught_var.message, "table must be a list"]] +# ---- #743: the OP_DISPATCH push must stamp the frame's loop counters ---- +# The push left saved_stall_count/saved_loop_iter unset, so every dispatch +# RETURN restored an unstamped value over the live counters and a +# dispatch-per-iteration loop's __loop_iterations__ froze at 1. +# +# __loop_iterations__ is cumulative across loops, so the invariant is on the +# DELTA a loop contributes: exactly its own iteration count. That is the +# property a plain loop already has, asserted here for both — pinning the +# dispatch loop against its no-dispatch twin rather than against a constant +# the convergence heuristic could retune (pre-fix the delta is 1 whatever +# the loop does; the count itself is machine-dependent). + +define d743(x) as: + return x + +t743 is [d743] + +# Warm-up loop: __loop_iterations__ is undefined until a loop has run, so +# this one exists to establish the baseline the two measured loops subtract. +w743 is 1 +w743 is 100 +loop while not converged: + w743 is w743 * 2 +plain_before is __loop_iterations__ + +plain_n is 0 +p743 is 1 +p743 is 100 +loop while not converged: + p743 is p743 * 2 + plain_n is plain_n + 1 +plain_delta is __loop_iterations__ - plain_before + +disp_before is __loop_iterations__ +disp_n is 0 +v743 is 1 +v743 is 100 +loop while not converged: + v743 is v743 * 2 + disp_n is disp_n + 1 + junk743 is dispatch of [t743, 0, v743] +disp_delta is __loop_iterations__ - disp_before + +check of ["plain loop contributes its own iteration count", plain_delta == plain_n] +check of ["dispatch loop contributes its own iteration count", disp_delta == disp_n] +# Guards the pin against vacuity: a 1-iteration loop would satisfy the +# check above even with the counter frozen at 1. +check of ["dispatch loop ran more than one iteration", disp_n > 1] + # #459 facet 3: the parenthesized #355 form is ONE argument on the normal # call path (never OP_DISPATCH); builtin_dispatch given a single 3-element # list agrees with the opcode by contract — pinned here.