v0.0.133 — Iterative array builders no longer leak closure return-value root (closes #570)#574
Conversation
…ue root (closes #570) The lifted-closure epilogue in vera/codegen/closures.py restores its entry $gc_sp and then, when the return type is a heap pointer (Vera ADT, String, Array<T>), pushes the return as a fresh root so generic stash-then-GC callers stay sound. The iterative array builders in vera/wasm/calls_arrays.py consume the return synchronously (store into a rooted dst[idx], or in-place overwrite a rooted accumulator slot), so the per-call root is redundant. Accumulating one leaked slot per iteration overflowed the 16 KiB / 4 096-entry shadow stack; a 5 000-element array_map<_, ADT> trapped at iteration ~4 000. Per-callsite unwind: * array_map / array_mapi: emit a 4-byte $gc_sp decrement after storing the return into dst[idx]. * array_fold: emit the same unwind BEFORE the gc_sp - 8 overwrite math. Without it the second iteration overwrite addressed the previous-call leaked slot instead of the accumulator pre-call slot — a second symptom of the same bug class that this fix incidentally closes. * array_sort_by: same 4-byte unwind after the comparator Ordering tag is read via i32.load offset=0. Insertion sort issues up to n*(n-1)/2 comparisons; the leak surfaces at ~200 reverse-sorted elements. Builders that take a Bool-returning predicate (array_filter, array_find, array_any, array_all) and builders without a callback (array_flatten, array_reverse, array_range) are unaffected — Bool is excluded from the closure ret_is_pointer flag, so no post-restore root push happens. New TestIterativeBuilderShadowStack (4 tests) in tests/test_codegen_closures.py exercises each fixed builder at the size that previously overflowed. Co-Authored-By: Claude <noreply@anthropic.invalid>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR fixes a GC shadow‑stack leak in iterative array builders by emitting per‑callsite ChangesIterative Array Builder Shadow‑Stack Fix
sequenceDiagram
autonumber
actor Compiler
participant WasmGen as WasmCodegen
participant GC as ShadowStack
participant Runtime as WasmRuntime
Compiler->>WasmGen: translate array_* with closure call
WasmGen->>Runtime: emit call_indirect (closure)
WasmGen->>GC: push root (if closure returns heap ptr)
Runtime-->>WasmGen: closure result (ADT / primitive)
WasmGen->>WasmGen: store result into dst/accumulator
WasmGen->>GC: emit per‑iteration unwind (pop root) when needed
note over WasmGen,GC: ensures no lingering roots per iteration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #574 +/- ##
==========================================
+ Coverage 90.93% 90.97% +0.03%
==========================================
Files 59 59
Lines 22596 22620 +24
Branches 259 259
==========================================
+ Hits 20548 20578 +30
+ Misses 2041 2035 -6
Partials 7 7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@HISTORY.md`:
- Line 270: Update the tagged-release counts to reflect the new v0.0.133
release: change the "132 tagged releases" text in the Stage 11 row(s) of
HISTORY.md to "133 tagged releases", update the roll-up footer total in
HISTORY.md to 133, and also bump the tagged-release total in ROADMAP.md to 133
so all three places (Stage 11 row(s), HISTORY.md footer, and ROADMAP.md) remain
consistent with the new release.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8bd61d25-441c-4daf-88fa-f539e990f46d
⛔ Files ignored due to path filters (4)
docs/index.htmlis excluded by!docs/**docs/index.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**
📒 Files selected for processing (10)
CHANGELOG.mdHISTORY.mdKNOWN_ISSUES.mdREADME.mdROADMAP.mdTESTING.mdpyproject.tomltests/test_codegen_closures.pyvera/__init__.pyvera/wasm/calls_arrays.py
💤 Files with no reviewable changes (1)
- KNOWN_ISSUES.md
…rom forward-looking files ROADMAP had grown a 12-version retrospective paragraph in the bug-killing-campaign intro plus historical residue scattered through priority entries, the VeraBench section, Phase 3a/3b/4c items, the CI table, and a duplicate Completed-Phases table that restated HISTORY.md verbatim. None of this was forward-looking; it was "what shipped two PRs ago" stuff that belongs in HISTORY (per- release table) or CHANGELOG (long lists per version). Changes: * ROADMAP intro: drop the "Vera v0.0.132 delivers..." version peg (will go stale every release; not load-bearing). * ROADMAP campaign paragraph: collapse 12 sentences of release retrospective to 2 — count of remaining issues + pointer to HISTORY/CHANGELOG. * ROADMAP priority table: drop "splits out from #346 closed as superseded after v0.0.132 shipped..." and "phase 1 ... shipped in v0.0.117" framing; keep only what an agent picking up the issue needs to know. * ROADMAP VeraBench section: replace per-rerun delta narrative with a high-variance disclaimer. * ROADMAP Phase 3a/3b/4c entries: drop "during the v0.0.121 review cycle", "the v0.0.119 homepage redesign moved the score from 3+2 to 2+1", "shipped in v0.0.117/v0.0.118" — keep current state only. CI table loses "the tag/release gap that hit v0.0.113". * ROADMAP Completed Phases: replace 11-row table (which duplicated HISTORY.md Stages 1–9) with a one-liner pointing to HISTORY. * KNOWN_ISSUES Bugs table: tightened all three entries. The #573 entry was a 6-clause sentence with parenthetical-of-paren- thetical recapping the v0.0.132 host_gc_sweep design discussion; trimmed to symptom + recommended fix. #549 and #568 trimmed similarly. * HISTORY by-the-numbers row: bump v0.0.132 column to v0.0.133 (3,706 tests, 133 releases). No content removed — anything trimmed from these three files is either redundant with HISTORY/CHANGELOG or now lives in the underlying GitHub issue body. Co-Authored-By: Claude <noreply@anthropic.invalid>
CI 'uv lock --check' failed on PR #574 because the version bump 0.0.132 -> 0.0.133 wasn't reflected in uv.lock. Single-line bump of the vera package metadata; no dependency resolution change. Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
array_map,array_mapi,array_fold,array_sort_by) no longer leak the closure return-value root onto the GC shadow stack. Pre-fix: a 5 000-elementarray_map<_, ADT>trapped at iteration ~4 000.vera/wasm/calls_arrays.py: each builder emits a 4-byte$gc_spdecrement immediately after consuming the closure's heap-pointer return.array_foldadditionally moves the unwind ahead of itsgc_sp - 8overwrite math (the math was assuming zero post-call slots; with the leak it drifted onto leaked slots from prior iterations — second symptom of the same bug class, incidentally closed).Boolis excluded from the closure'sret_is_pointerflag invera/codegen/closures.py, so no post-restore root push happens.Closes
Closes #570.
Test plan
pytest tests/ -v— 3,706 passed, 14 skippedmypy vera/— cleanpython scripts/check_conformance.py— 82 / 82python scripts/check_examples.py— 33 / 33pytest tests/test_browser.py— 99 / 99 (Python ↔ JS runtime parity)TestIterativeBuilderShadowStack(4 tests intests/test_codegen_closures.py) covers each fixed builder at the size that previously overflowed:array_mapover 5 000MkBoxreturns (sum = 12 497 500)array_mapiover 5 000 with(elem + idx)Box (sum = 24 995 000)array_foldwith 5 000-element Box accumulator (sum = 12 497 500)array_sort_byover 200 reverse-sorted ints withOrderingcomparator (length = 200; ~19 900 comparisons)Why this is a clean fix
The root cause is a real soundness contract in the lifted-closure epilogue: when the return is a heap pointer, the closure pushes it onto the shadow stack as a fresh root after restoring its entry sp, so a generic stash-then-GC caller stays sound. Iterative array builders are special — they consume the return synchronously into an already-rooted destination (
dst[idx]or in-place accumulator slot) — so the per-call root is pure overhead. Each builder unwinds its own callback's post-push; no change to the closure epilogue, no change to other call sites.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation