vm + cranelift: register aliasing on shadow-rebind from Ref#291
Merged
Conversation
The bytecode compiler's Stmt::Let new-binding path was aliasing the new local to the source local's register whenever the RHS was a bare Ref. Expr::Ref(name) resolves to the source's register directly with no MOVE (free read), so add_local(new_name, src_reg) made b share a's slot. A later b = <expr> then clobbered a along with b. a = 5 b = a -- b aliased a's register b = 99 -- pre-fix: writes to the shared slot, corrupts a [a b] -- pre-fix: [99, 99], post-fix: [5, 99] The tree-walker was unaffected (named bindings, no register aliasing). Cranelift inherited the bug because it lowers VM bytecode. Fixing the VM compiler fixes both backends. The bug pre-dates Phase 2b (#261/#273/#276). Originally surfaced as a 'Zipf slope corruption' regression report citing those PRs, but a v0.11.1 bisect confirms VM and Cranelift have the same alias hole on both releases. Fix: when the compiled RHS register is already owned by an existing local, allocate a fresh register and emit OP_MOVE before add_local. One extra MOVE per shadow-from-ref. No effect on b = +a 1 / b = mset a k v which already allocate fresh registers from the arithmetic/builtin ops. Type hints (reg_record_type, reg_is_num, reg_is_str) transfer to the new register so downstream peepholes still fire.
21 tests across tree, VM, Cranelift covering: - Number shadow-rebind via arithmetic (the Zipf-slope repro shape) - Number shadow + bare literal overwrite (minimal repro) - Map shadow + mset rebind - List shadow + += rebind - Text shadow + + rebind - Transitive a-to-b-to-c shadow - Non-aliasing happy path (b = +a 1) to lock in the locals.iter() guard doesn't trip when the RHS already allocated a fresh register Tree-walker is the reference; VM and Cranelift assertions catch regressions on the bytecode + JIT backends.
Number, Map, List, Text shadow-then-rebind shapes with -- run / -- out assertions so the examples_engines harness exercises them on every engine. Doubles as an in-context learning example for agents who hit the b = a; b = <new> pattern in future.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
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.
Summary
The VM bytecode compiler's
Stmt::Letnew-binding path was aliasing the new local to the source local's register whenever the RHS was a bareRef. The classic shape:Expr::Ref(name)resolves to the source's register directly with no MOVE (free read).add_local(new_name, src_reg)then made the new local share that register, so a later rebind clobbered the source. The tree-walker was always correct (named bindings, no register aliasing). Cranelift inherited the bug because it lowers VM bytecode, so fixing the VM compiler fixes both backends.Originally surfaced as a 'Phase 2b silent miscompile' report citing #261 / #273 / #276 and a numeric Zipf-slope corruption on a Moby Dick sample. A v0.11.1 bisect (one tag before Phase 2b landed) shows VM and Cranelift have the same alias hole on both releases. So this is a long-standing bug in the compiler, not a Phase 2b regression. The reframed PR title and branch (
fix/vm-shadow-rebind-alias) reflect what's actually being fixed.This is the kind of bug that should never ship: silent miscompile on a trivial numeric rebind pattern, no error, no warning, wrong answer. One extra MOVE per shadow-from-ref is cheap insurance against the token-cost of debug retries and lost trust in agents.
Repro
Before:
```
$ ilo --run-vm 'go>L n;a=5;b=a;b=99;[a b]' go
[99, 99]
$ ilo --run-vm 'go>L n;z=3.14;t=z;t = *t 2;[t z]' go
[6.28, 6.28]
```
After:
```
$ ilo --run-vm 'go>L n;a=5;b=a;b=99;[a b]' go
[5, 99]
$ ilo --run-vm 'go>L n;z=3.14;t=z;t = *t 2;[t z]' go
[6.28, 3.14]
```
Same outputs on
--run-tree(unchanged) and--run-cranelift(now matches).What's in the diff
vm: avoid register aliasing on shadow-rebind from Ref(src/vm/mod.rs): when the compiled RHS register is already owned by an existing local, allocate a fresh register and emitOP_MOVEbeforeadd_local. Type hints (reg_record_type,reg_is_num,reg_is_str) transfer to the new register so downstream peepholes still fire. No effect onb = +a 1/b = mset a k vbecause those already allocate fresh registers from the arithmetic / builtin ops.test: cross-engine regression coverage for shadow-rebind alias(tests/regression_shadow_rebind_alias.rs): 21 tests across tree, VM, Cranelift covering Number / Map / List / Text shadow-then-rebind, transitive chains (a to b to c), and the non-aliasing happy path so the new guard doesn't trip when the RHS already allocated a fresh register.example: shadow-rebind-alias pins the four typed shapes(examples/shadow-rebind-alias.ilo): four shapes with-- run/-- outassertions so the examples_engines harness exercises them on every engine. Doubles as an in-context learning example for agents who hit theb = a; b = <new>pattern.Test plan
a=5;b=a;b=99andz=3.14;t=z;t = *t 2).cargo test --release --features cranelift: 125 test binaries pass, 0 failures.cargo fmt --check: clean.cargo clippy --release --features cranelift --all-targets -- -D warnings: clean.shadow-rebind-alias.iloacross tree, VM, Cranelift.Follow-ups
+=xs vand baremset m k vshould mutate the named binding. They don't, and shouldn't: that's the assignment form's job (xs = +=xs v/m = mset m k v). Worth a one-line clarification in the spec so future personas don't re-litigate the same misread.