v0.0.122: bound conservative GC scan against $heap_ptr (#515)#542
Merged
Conversation
Closes #515. $gc_collect was faulting under sustained allocation pressure: the Phase 2 worklist-seeding code accepts a shadow-stack value as a heap pointer if it satisfies three guards (in heap range, properly aligned, below $heap_ptr). None of those guards prove the word at val-4 is actually an object header. A non-pointer i32 in payload data (e.g. a bit-packed Nat row in Conway-style code) can satisfy all three; Phase 2b then reads garbage as obj_size = header >> 1, sets the mark bit (corrupting a random heap word), and walks obj_ptr + 0..obj_size past heap_ptr and past the linear-memory boundary, trapping with "memory fault at wasm address 0x... in linear memory of size 0x..." — $gc_collect itself at the top of the stack. Two layers of defence in _emit_gc_collect (vera/codegen/assembly.py): * Layer 2 (early skip): before either marking or scanning a worklist entry, compute obj_size and verify obj_ptr + obj_size <= heap_ptr. If not, skip the entry — no mark store, no scan loop — preventing both the trap and the mark-bit corruption. * Layer 1 (per-iter): inside the conservative scan loop, also check obj_ptr + scan_ptr + 4 <= heap_ptr before each i32.load. Cheap relative to the load itself, and protects any future caller that reaches this loop without the Layer-2 check. Verified end-to-end with a 470-line Conway implementation that pre-fix crashed reliably at generation 56: now runs cleanly through every generation. Structural regression test in tests/test_codegen.py::TestGarbageCollection::test_gc_collect_bounds_check_against_heap_ptr asserts both bound checks survive in the emitted WAT. Behavioural reproducers for #515 are heavily layout-sensitive (string-pool offsets, allocation order, so a structural assertion is the durable regression guard. Doc sweep: KNOWN_ISSUES (remove #515 row); ROADMAP (drop #515 from queue, intro updated to ten remain); HISTORY (add v0.0.122 row); CHANGELOG (new section). Site assets regenerated. Co-Authored-By: Claude <noreply@anthropic.invalid> EOF )
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughv0.0.122: adds heap-pointer bounds checks to the conservative GC mark/scan in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #542 +/- ##
=======================================
Coverage 91.02% 91.02%
=======================================
Files 58 58
Lines 22033 22033
Branches 259 259
=======================================
Hits 20055 20055
Misses 1971 1971
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
HISTORY.md (1)
296-296:⚠️ Potential issue | 🟡 MinorTagged releases count not incremented in footer.
The footer still says "121 tagged releases" but this PR adds v0.0.122, so it should say "122 tagged releases". Based on learnings, this count must also be updated in
ROADMAP.md's footer section.Proposed fix
-Total: **810+ commits, 121 tagged releases, 40 active development days.** +Total: **810+ commits, 122 tagged releases, 40 active development days.**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@HISTORY.md` at line 296, Update the footer string "121 tagged releases" to "122 tagged releases" in HISTORY.md and make the same change in ROADMAP.md's footer; search for the literal "121 tagged releases" (or the footer block containing "Total: **... commits, 121 tagged releases, ...**") and replace the count so both files reflect the new v0.0.122 release.vera/codegen/assembly.py (1)
680-717:⚠️ Potential issue | 🔴 CriticalThis still marks interior payload words as though they were headers.
The new
obj_ptr + obj_size <= heap_ptrgate only rejects false positives whose decoded size runs past the heap. A conservative root that numerically lands atheader+12of a live object still gets through if the payload word atheader+8happens to decode to a small in-boundsobj_size; we then set bit 0 atobj_ptr - 4, which is a payload word, not an object header. So the collector can still corrupt live data even though it no longer walks out of bounds. Please add a real object-start/header validation step before the mark store.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vera/codegen/assembly.py` around lines 680 - 717, The code currently decodes a size from whatever word sits at $header (which may be an interior payload) and then sets the mark bit at $obj_ptr-4; instead, load and validate the actual candidate header at potential_header_ptr = ($obj_ptr - 4) before setting the mark: compute potential_header_ptr, i32.load that word into a new local (e.g. reuse $header or a new local like $candidate_header), check its header-format/tag (ensure low tag bit is 0 or matches your header-tag invariant) and decode obj_size from that candidate header (local.get $candidate_header; i32.const 1; i32.shr_u) and re-run the bounds check (potential_header_ptr + obj_size <= global.get $heap_ptr) and already-marked check against the candidate header; if any check fails br $m_loop, otherwise proceed to set the mark by writing back to potential_header_ptr using the validated candidate header before the i32.store that sets the mark bit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Line 42: Update the ROADMAP entry for issue `#348` to reflect the current
worklist size: change the description that currently says a "fixed 1,024-entry
cap" to state the collector uses a 16 KiB worklist (4,096 i32 entries) and that
overflow at that size is the tracked limitation; reference the implementation
location vera/codegen/assembly.py and the GC worklist allocation/constant (the
worklist buffer size / worklist allocation constant) so readers know where the
limit is defined.
In `@tests/test_codegen.py`:
- Around line 1924-1927: The current assertions only check for comment markers
in the WebAssembly text (wat) but must assert the actual bound-check opcodes;
update the checks that reference the "Layer 2 (issue `#515`)" and "Layer 1 (issue
`#515`)" markers to also verify the emitted bound-check instructions are present
in wat (use the existing wat variable and the two marker strings to locate the
relevant region, then assert the opcode sequence for the bound check is present,
e.g., that the region contains the expected instruction like "i32.lt_u" (and the
following conditional branch such as "br_if") or the concrete opcode pattern
your compiler emits for bounds checking). Ensure both Layer 2 (header sanity
check) and Layer 1 (per-iteration scan) assertions are strengthened to look for
these opcodes adjacent to their marker comments rather than solely the comments.
---
Outside diff comments:
In `@HISTORY.md`:
- Line 296: Update the footer string "121 tagged releases" to "122 tagged
releases" in HISTORY.md and make the same change in ROADMAP.md's footer; search
for the literal "121 tagged releases" (or the footer block containing "Total:
**... commits, 121 tagged releases, ...**") and replace the count so both files
reflect the new v0.0.122 release.
In `@vera/codegen/assembly.py`:
- Around line 680-717: The code currently decodes a size from whatever word sits
at $header (which may be an interior payload) and then sets the mark bit at
$obj_ptr-4; instead, load and validate the actual candidate header at
potential_header_ptr = ($obj_ptr - 4) before setting the mark: compute
potential_header_ptr, i32.load that word into a new local (e.g. reuse $header or
a new local like $candidate_header), check its header-format/tag (ensure low tag
bit is 0 or matches your header-tag invariant) and decode obj_size from that
candidate header (local.get $candidate_header; i32.const 1; i32.shr_u) and
re-run the bounds check (potential_header_ptr + obj_size <= global.get
$heap_ptr) and already-marked check against the candidate header; if any check
fails br $m_loop, otherwise proceed to set the mark by writing back to
potential_header_ptr using the validated candidate header before the i32.store
that sets the mark bit.
🪄 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: 20dfd6f8-acd8-4bdc-8d0c-b47b5426001a
⛔ 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.pyvera/__init__.pyvera/codegen/assembly.py
💤 Files with no reviewable changes (1)
- KNOWN_ISSUES.md
Two parallel updates I missed in the main #515 commit: * Module map: assembly.py grew 774 -> 856 lines with the Layer 1 + Layer 2 bound checks. * GC architecture description: Phase 2 ("Mark") now documents the two-layer bound check explicitly, and explains *why* it's needed (the seeding guards don't prove the word at val-4 is actually a header, so a payload word that satisfies the guards can lead the marker past heap_ptr. This is the single most subtle thing about the conservative GC and was previously elided in the README. Co-Authored-By: Claude <noreply@anthropic.invalid> EOF )
7 tasks
Three doc/test fixes from CodeRabbit on PR #542 plus the uv.lock refresh required by the version bump. Fourth CodeRabbit finding (assembly.py outside-diff suggestion to load the header from obj_ptr-4) verified as already-implemented; the existing header local IS the value loaded from obj_ptr-4 -- replying inline on the PR. Changes: * ROADMAP #348: "fixed 1024-entry cap" -> "16 KiB / 4,096-entry cap (gc_worklist_size = 16384 in vera/codegen/assembly.py)". CodeRabbit verified: assembly.py line 269 sets gc_worklist_size = 16384 (= 4096 entries x 4 bytes). * HISTORY.md footer: "121 tagged releases" -> "122 tagged releases". Stale after the v0.0.122 row landed in the Stage 9 release table. * tests/test_codegen.py: strengthen the #515 regression test to assert the actual opcode pattern adjacent to each marker comment rather than just the marker text. A refactor that left the comment in place but deleted the underlying bound check would silently pass the old version; the new version pins the exact opcode sequence. Fragility tradeoff: the test now fails if anyone reorders the bound check, but the assertion message shows the drift verbatim so the fix is one diff away. * uv.lock: regenerated via uv lock. Required by the version bump (pyproject.toml 0.0.121 -> 0.0.122) -- lint job failed on uv lock --check. Co-Authored-By: Claude <noreply@anthropic.invalid>
Owner
Author
|
@coderabbitai thanks. Three of four findings actioned in 75682a1:
For the fourth (outside-diff in local.get $obj_ptr
i32.const 4
i32.sub
i32.load
local.set $header
Also pushed |
|
Tip For best results, initiate chat on the files or code changes.
The three fixes are clear and the test improvement (asserting actual opcodes adjacent to the On the fourth: you're correct, and I was wrong to suggest it. ✏️ Learnings added
🧠 Learnings used |
aallan
added a commit
that referenced
this pull request
Apr 27, 2026
Three fixes from the post-rebase CodeRabbit pass on #544: * HISTORY.md footer: "122 tagged releases" -> "123 tagged releases". CodeRabbit reported it as "121 -> 123" but the prior fix in #542 had already taken it to 122; bumping it the rest of the way for this release. * tests/test_runtime_traps.py: add captured.err == "" assertion in test_json_mode_does_not_tee_to_stdout. Mirrors the JSON-mode invariant that TestStdoutOnTrap522 already pins on its four JSON tests -- in JSON mode the entire output is the envelope on stdout and stderr must stay empty so downstream consumers parsing our output see exactly one machine-readable document. * uv.lock: regenerated for the 0.0.122 -> 0.0.123 version bump. Lint job was failing on uv lock --check. Three other CodeRabbit findings verified as either incorrect or out of scope (replying inline on the PR): * HISTORY.md "collapse multi-line entries" -- the v0.0.122 / v0.0.123 rows are already single rows; CodeRabbit was reading wrapped display as multi-line content. * ROADMAP.md "33 examples -> 32 examples" -- ls examples/*.vera reports 33; check_doc_counts.py and the live count agree. CodeRabbit's expected count is wrong. * tests/test_codegen.py opcode assertions -- already strengthened in #542 (commit 75682a1 "PR #542 review fixes"). Markers stay as discoverability anchors; opcode patterns are the contract. * vera/cli.py isatty() gating on the trailing newline -- pre-existing behaviour from before #543, unchanged by this PR. Out of scope: changing the trailing-newline policy is a separate concern with its own backwards-compat surface. * vera/codegen/assembly.py header-validity check -- same finding as the one already discussed and resolved on #542. No separate HEADER_TAG invariant exists in this codebase (header is (size << 1) | mark with no marker bit beyond mark itself). The obj_ptr + obj_size <= heap_ptr bound check is the strongest validation possible without introducing a new tag scheme. Co-Authored-By: Claude <noreply@anthropic.invalid>
This was referenced May 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #515.
Summary
$gc_collectwas faulting under sustained allocation pressure withgc_collectitself at the top of the stack — the collector, not the program, was the crashing frame. Symptom:memory fault at wasm address 0x... in linear memory of size 0x.... Reliably reproduced with a 470-line Conway implementation crashing at generation 56.Root cause
The Phase 2 worklist-seeding code in
_emit_gc_collectaccepts a shadow-stack value as a heap pointer if it satisfies three guards:val >= gc_heap_start + 4)(val - gc_heap_start) % 8 == 4)$heap_ptrNone of those guards prove the word at
val - 4is actually an object header. A non-pointer i32 in payload data (e.g. a bit-packedNatrow in Conway-style code, a hash, anything else with bits in heap range) can satisfy all three; Phase 2b then reads garbage asobj_size = header >> 1, sets the mark bit (corrupting a random heap word!), and walksobj_ptr + 0..obj_sizepast$heap_ptrand past the linear-memory boundary.This is the classic conservative-GC false-positive failure mode.
Fix
Two layers of defence in
_emit_gc_collect(vera/codegen/assembly.py):obj_sizeand verifyobj_ptr + obj_size <= heap_ptr. If not, skip the entry entirely (no mark store, no scan loop). Catches the bug at the cheapest possible point and prevents the mark-bit corruption that would otherwise persist across the cycle.obj_ptr + scan_ptr + 4 <= heap_ptrbefore eachi32.load. Negligible cost relative to the load it guards, and protects any future caller that reaches this loop without the Layer-2 check.Tests
tests/test_codegen.py::TestGarbageCollection::test_gc_collect_bounds_check_against_heap_ptrasserts both bound checks survive in the emitted WAT. Behavioural reproducers for GC collect itself faults with out-of-bounds memory access under sustained allocation pressure #515 are heavily layout-sensitive (string-pool offsets, allocation order), so a structural assertion is the durable regression guard.Verification
Test plan
pytest tests/ -q— 3,555 passed, 14 skippedmypy vera/— cleanpython scripts/check_conformance.py— all 81 passpython scripts/check_examples.py— all 33 passpython scripts/check_version_sync.py— version 0.0.122 consistentCo-Authored-By: Claude noreply@anthropic.invalid
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Documentation