Skip to content

Commit 67bf6e2

Browse files
author
Arnaud Riess
committed
fix: for local variable aliasing and fix LocalGet behavior
1 parent 5dfa6c2 commit 67bf6e2

4 files changed

Lines changed: 292 additions & 8 deletions

File tree

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
(module
2+
;; ── Regression test: the digital_root aliasing bug ───────────────────────
3+
;;
4+
;; Computes n % 10 (unsigned) using the exact instruction sequence that lived
5+
;; inside the inner loop of digital_root and exposed the LocalGet aliasing bug.
6+
;;
7+
;; Pattern:
8+
;; local.get 0 <- snapshot of n; stays on stack bottom until i32.sub
9+
;; local.get 0 <- n again, for division (consumed by i32.div_u)
10+
;; i32.const 10
11+
;; i32.div_u <- n / 10; now stack = [n, n/10]
12+
;; local.tee 0 <- stores n/10 back into local 0; keeps n/10 on stack
13+
;; i32.const 10
14+
;; i32.mul <- (n/10)*10
15+
;; i32.sub <- n - (n/10)*10 = n%10
16+
;;
17+
;; With the old buggy LocalGet (pushing the local's VarId directly), the
18+
;; local.tee would emit `v0 = quotient`, mutating the VarId that was already
19+
;; sitting at the bottom of the value stack from the first local.get 0.
20+
;; The subtraction then computed (n/10) - (n/10)*10 instead of n - (n/10)*10,
21+
;; returning -9 for n=10 instead of the correct 0.
22+
(func (export "mod10_via_tee") (param i32) (result i32)
23+
local.get 0 ;; snap of n, survives to i32.sub → [n]
24+
local.get 0 ;; n for division → [n, n]
25+
i32.const 10
26+
i32.div_u ;; n / 10 → [n, n/10]
27+
local.tee 0 ;; local 0 ← n/10, keep on stack → [n, n/10]
28+
i32.const 10
29+
i32.mul ;; (n/10) * 10 → [n, (n/10)*10]
30+
i32.sub ;; n - (n/10)*10 = n % 10 → [n%10]
31+
return)
32+
33+
;; ── local.get snapshot survives a local.set on the same local ───────────
34+
;;
35+
;; Reads `n`, then overwrites local 0 with n*3, then reads the new value.
36+
;; Returns: old_n - new_n = n - 3n = -2n.
37+
;;
38+
;; With the bug: the bottom stack entry aliased v0, so after `local.set 0`
39+
;; wrote v0=3n, the "old" read also became 3n → result was 3n - 3n = 0
40+
;; instead of n - 3n = -2n (wrong for n ≠ 0).
41+
(func (export "preserve_across_set") (param i32) (result i32)
42+
local.get 0 ;; snap of n, survives to i32.sub → [n]
43+
local.get 0 ;; n for multiplication → [n, n]
44+
i32.const 3
45+
i32.mul ;; n * 3 → [n, 3n]
46+
local.set 0 ;; local 0 ← 3n → [n]
47+
local.get 0 ;; new value of local 0 = 3n → [n, 3n]
48+
i32.sub ;; n - 3n = -2n
49+
return)
50+
51+
;; ── local.get snapshot survives a local.tee on the same local ───────────
52+
;;
53+
;; A prior snapshot of `a` must equal `a` after local.tee stores (a + b)
54+
;; into local 0. Returns: a - (a + b) = -b.
55+
;;
56+
;; With the bug: after `local.tee 0` emitted `v0 = a+b`, the bottom stack
57+
;; entry (first local.get 0) also became a+b, so the subtraction computed
58+
;; (a+b) - (a+b) = 0 instead of a - (a+b) = -b (wrong for b ≠ 0).
59+
(func (export "get_snap_vs_tee") (param i32 i32) (result i32)
60+
local.get 0 ;; snap of a, survives to i32.sub → [a]
61+
local.get 0 ;; a for addition → [a, a]
62+
local.get 1 ;; b → [a, a, b]
63+
i32.add ;; a + b → [a, a+b]
64+
local.tee 0 ;; local 0 ← a+b, keep on stack → [a, a+b]
65+
i32.sub ;; a - (a+b) = -b
66+
return)
67+
68+
;; ── local.get snapshot survives both a local.tee and a local.set ────────
69+
;;
70+
;; First tee stores (a + b) into local 0, then a further set overwrites it
71+
;; with (a + b) * 2. The original snapshot of `a` must still equal `a`.
72+
;; Returns: a - (a + b)*2.
73+
;;
74+
;; With the bug: v0 is overwritten twice (first to a+b by tee, then to
75+
;; (a+b)*2 by set). The bottom stack entry, being an alias to v0, ends
76+
;; up as (a+b)*2, so the subtraction returns 0 instead of a - (a+b)*2.
77+
(func (export "get_tee_then_set") (param i32 i32) (result i32)
78+
local.get 0 ;; snap of a, survives both writes → [a]
79+
local.get 0 ;; a for addition → [a, a]
80+
local.get 1 ;; b → [a, a, b]
81+
i32.add ;; a + b → [a, a+b]
82+
local.tee 0 ;; local 0 ← a+b, keep on stack → [a, a+b]
83+
i32.const 2
84+
i32.mul ;; (a+b) * 2 → [a, (a+b)*2]
85+
local.set 0 ;; local 0 ← (a+b)*2 → [a]
86+
local.get 0 ;; (a+b)*2 → [a, (a+b)*2]
87+
i32.sub ;; a - (a+b)*2
88+
return))
Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,189 @@
1+
//! Regression tests for local variable aliasing in the IR builder.
2+
//!
3+
//! These tests pin the exact instruction patterns that exposed the `LocalGet`
4+
//! aliasing bug. The bug: `LocalGet` used to push the local's `VarId`
5+
//! directly onto the value stack. A later `local.tee` or `local.set` on the
6+
//! same local would emit `vN = new_value`, overwriting the variable that was
7+
//! already sitting deeper on the value stack — silently corrupting any
8+
//! subsequent computation that tried to use that "old" stack entry.
9+
//!
10+
//! Fix: `LocalGet` now emits a fresh copy (`vM = vN`) and pushes `vM`, so the
11+
//! snapshot is immutable from the perspective of later mutations to the local.
12+
//!
13+
//! See `data/wat/locals_aliasing.wat` for the minimal WAT reproductions.
14+
15+
use herkos_tests::locals_aliasing;
16+
17+
fn module() -> locals_aliasing::WasmModule {
18+
locals_aliasing::new().expect("module instantiation should succeed")
19+
}
20+
21+
// ── mod10_via_tee ────────────────────────────────────────────────────────────
22+
//
23+
// REGRESSION: the exact inner-loop pattern of `digital_root` that triggered
24+
// the original bug.
25+
//
26+
// Instruction sequence:
27+
// local.get 0 <- snapshot of n (stays at stack bottom until sub)
28+
// local.get 0 <- n for division (consumed by div_u)
29+
// i32.const 10
30+
// i32.div_u <- n / 10
31+
// local.tee 0 <- local 0 ← n/10 ← BUG POINT: overwrote v0
32+
// i32.const 10
33+
// i32.mul <- (n/10)*10
34+
// i32.sub <- n - (n/10)*10 ← used the (now-corrupted) v0
35+
//
36+
// Before the fix, digital_root(10) returned -9:
37+
// n/10 = 1, (1)*10 = 10, but v0 was already 1 → 1 - 10 = -9.
38+
// After the fix it correctly returns 0: 10 - 10 = 0.
39+
40+
#[test]
41+
fn test_mod10_tee_n10_regression() {
42+
// n=10 is the value that returned -9 before the fix.
43+
assert_eq!(module().mod10_via_tee(10).unwrap(), 0);
44+
}
45+
46+
#[test]
47+
fn test_mod10_tee_exact_zero_remainders() {
48+
let mut m = module();
49+
for n in [0u32, 10, 20, 50, 90] {
50+
assert_eq!(m.mod10_via_tee(n as i32).unwrap(), 0, "n={n}");
51+
}
52+
}
53+
54+
#[test]
55+
fn test_mod10_tee_nonzero_remainders() {
56+
let mut m = module();
57+
assert_eq!(m.mod10_via_tee(1).unwrap(), 1);
58+
assert_eq!(m.mod10_via_tee(7).unwrap(), 7);
59+
assert_eq!(m.mod10_via_tee(37).unwrap(), 7);
60+
assert_eq!(m.mod10_via_tee(99).unwrap(), 9);
61+
assert_eq!(m.mod10_via_tee(43).unwrap(), 3);
62+
}
63+
64+
// ── preserve_across_set ──────────────────────────────────────────────────────
65+
//
66+
// A value captured by `local.get` must survive a `local.set` on the same
67+
// local. Returns: old_n - new_n = n - 3n = -2n.
68+
//
69+
// Bug path: both `local.get 0` calls pushed v0 directly. After
70+
// `local.set 0` wrote v0 = 3n, the bottom stack entry (the "old" read) also
71+
// held 3n → subtraction returned 3n - 3n = 0 for all n.
72+
73+
#[test]
74+
fn test_preserve_across_set_basic() {
75+
// n=5: 5 - 15 = -10
76+
assert_eq!(module().preserve_across_set(5).unwrap(), -10);
77+
}
78+
79+
#[test]
80+
fn test_preserve_across_set_one() {
81+
// n=1: 1 - 3 = -2
82+
assert_eq!(module().preserve_across_set(1).unwrap(), -2);
83+
}
84+
85+
#[test]
86+
fn test_preserve_across_set_zero() {
87+
// n=0: 0 - 0 = 0 (passes even with the bug, but confirms no crash)
88+
assert_eq!(module().preserve_across_set(0).unwrap(), 0);
89+
}
90+
91+
#[test]
92+
fn test_preserve_across_set_negative() {
93+
// n=-3: (-3) - (-9) = 6
94+
assert_eq!(module().preserve_across_set(-3).unwrap(), 6);
95+
}
96+
97+
#[test]
98+
fn test_preserve_across_set_varied() {
99+
let mut m = module();
100+
for n in [2, 7, 10, 100i32] {
101+
assert_eq!(
102+
m.preserve_across_set(n).unwrap(),
103+
n.wrapping_mul(-2),
104+
"n={n}"
105+
);
106+
}
107+
}
108+
109+
// ── get_snap_vs_tee ──────────────────────────────────────────────────────────
110+
//
111+
// A prior `local.get` snapshot of `a` must be unaffected by a `local.tee`
112+
// that stores (a + b) into the same local. Returns: a - (a + b) = -b.
113+
//
114+
// Bug path: after `local.tee 0` emitted `v0 = a+b`, the bottom stack entry
115+
// (from the first `local.get 0`) also held a+b → returned 0 instead of -b.
116+
117+
#[test]
118+
fn test_get_snap_vs_tee_basic() {
119+
// a=10, b=3 → -3
120+
assert_eq!(module().get_snap_vs_tee(10, 3).unwrap(), -3);
121+
}
122+
123+
#[test]
124+
fn test_get_snap_vs_tee_zero_b() {
125+
// b=0 → -0 = 0 (passes with bug too, sanity check)
126+
assert_eq!(module().get_snap_vs_tee(5, 0).unwrap(), 0);
127+
}
128+
129+
#[test]
130+
fn test_get_snap_vs_tee_zero_a() {
131+
// a=0, b=7 → -7
132+
assert_eq!(module().get_snap_vs_tee(0, 7).unwrap(), -7);
133+
}
134+
135+
#[test]
136+
fn test_get_snap_vs_tee_negative_b() {
137+
// a=3, b=-4 → a-(a+b) = -(-4) = 4
138+
assert_eq!(module().get_snap_vs_tee(3, -4).unwrap(), 4);
139+
}
140+
141+
#[test]
142+
fn test_get_snap_vs_tee_equal() {
143+
// a=5, b=5 → -5
144+
assert_eq!(module().get_snap_vs_tee(5, 5).unwrap(), -5);
145+
}
146+
147+
// ── get_tee_then_set ─────────────────────────────────────────────────────────
148+
//
149+
// A `local.get` snapshot must survive *two* consecutive overwrites of the same
150+
// local: first by `local.tee` (stores a+b), then by `local.set` (stores
151+
// (a+b)*2). Returns: a - (a+b)*2.
152+
//
153+
// Bug path: v0 was overwritten twice. The bottom stack entry kept tracking
154+
// v0's latest value, eventually equalling (a+b)*2, so the subtraction
155+
// returned 0 instead of a - (a+b)*2.
156+
157+
#[test]
158+
fn test_get_tee_then_set_basic() {
159+
// a=2, b=3: a+b=5, (a+b)*2=10, result = 2 - 10 = -8
160+
assert_eq!(module().get_tee_then_set(2, 3).unwrap(), -8);
161+
}
162+
163+
#[test]
164+
fn test_get_tee_then_set_zero_b() {
165+
// b=0: a - a*2 = -a
166+
assert_eq!(module().get_tee_then_set(4, 0).unwrap(), -4);
167+
}
168+
169+
#[test]
170+
fn test_get_tee_then_set_zero_a() {
171+
// a=0, b=5: 0 - 10 = -10
172+
assert_eq!(module().get_tee_then_set(0, 5).unwrap(), -10);
173+
}
174+
175+
#[test]
176+
fn test_get_tee_then_set_negative() {
177+
// a=1, b=-3: a+b=-2, (a+b)*2=-4, result = 1 - (-4) = 5
178+
assert_eq!(module().get_tee_then_set(1, -3).unwrap(), 5);
179+
}
180+
181+
#[test]
182+
fn test_get_tee_then_set_varied() {
183+
let mut m = module();
184+
let cases: &[(i32, i32)] = &[(0, 0), (1, 1), (5, 2), (10, -3), (-1, 4)];
185+
for &(a, b) in cases {
186+
let expected = a - (a + b) * 2;
187+
assert_eq!(m.get_tee_then_set(a, b).unwrap(), expected, "a={a} b={b}");
188+
}
189+
}

crates/herkos/src/ir/builder/translate.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,21 @@ impl IrBuilder {
5151

5252
// Local variable access
5353
Operator::LocalGet { local_index } => {
54-
let var = self
54+
let src = self
5555
.local_vars
5656
.get(*local_index as usize)
5757
.copied()
5858
.ok_or_else(|| {
5959
anyhow::anyhow!("local.get: local index {} out of range", local_index)
6060
})?;
61-
self.value_stack.push(var);
61+
// Emit a copy rather than pushing the local's VarId directly.
62+
// If we push the local's VarId, a later local.tee/local.set that
63+
// overwrites the same local will corrupt any already-pushed reference
64+
// to it, because the backend emits sequential mutable assignments.
65+
// A fresh variable captures the value at this point in time.
66+
let dest = self.new_var();
67+
self.emit(IrInstr::Assign { dest, src });
68+
self.value_stack.push(dest);
6269
}
6370

6471
Operator::LocalSet { local_index } => {

crates/herkos/tests/e2e.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -654,7 +654,7 @@ fn test_i64_bitwise_and_shifts() -> Result<()> {
654654
let rust_code = transpile_wat(wat)?;
655655
println!("Generated Rust code:\n{}", rust_code);
656656

657-
assert!(rust_code.contains("& v1"));
657+
assert!(rust_code.contains(" & v"));
658658
assert!(rust_code.contains("wrapping_shl"));
659659
assert!(rust_code.contains("rotate_left"));
660660

@@ -715,7 +715,7 @@ fn test_f64_operations() -> Result<()> {
715715
let rust_code = transpile_wat(wat)?;
716716
println!("Generated Rust code:\n{}", rust_code);
717717

718-
assert!(rust_code.contains("v0 / v1"));
718+
assert!(rust_code.contains(" / v"));
719719
assert!(rust_code.contains(".floor()"));
720720
assert!(rust_code.contains(".ceil()"));
721721
assert!(rust_code.contains(".sqrt()"));
@@ -762,15 +762,15 @@ fn test_conversion_ops() -> Result<()> {
762762
println!("Generated Rust code:\n{}", rust_code);
763763

764764
// wrap
765-
assert!(rust_code.contains("v0 as i32"));
765+
assert!(rust_code.contains(" as i32"));
766766
// extend signed
767-
assert!(rust_code.contains("v0 as i64"));
767+
assert!(rust_code.contains(" as i64"));
768768
// extend unsigned
769-
assert!(rust_code.contains("(v0 as u32) as i64"));
769+
assert!(rust_code.contains("as u32) as i64"));
770770
// trunc trapping — delegated to runtime ops
771771
assert!(rust_code.contains("i32_trunc_f64_s("));
772772
// convert
773-
assert!(rust_code.contains("v0 as f64"));
773+
assert!(rust_code.contains(" as f64"));
774774
// reinterpret
775775
assert!(rust_code.contains("to_bits()"));
776776
assert!(rust_code.contains("from_bits"));

0 commit comments

Comments
 (0)