Skip to content

Commit b4ec36e

Browse files
avrabeclaude
andauthored
fix(arm): home i64 params per AAPCS register-pairs — close #518 silent miscompile (#518, #242) (#531)
An i64 binop reading an i64 PARAM silently miscompiled on BOTH ARM selectors: an i64 param occupies an AAPCS register PAIR (param0 = R0:R1), but both selectors treated a param as a single i32-width register. Found by footgun/adversarial differential testing; flip-independent, pre-existing. Root cause + fix, by selector: - DIRECT (select_with_stack): `infer_i64_locals` learns i64-ness only from LocalSet/Tee, so a read-only i64 param stayed is_i64=false → its implicit hi register was unreserved and a following i64.const was allocated INTO it (movw r1,#K clobbered R1 = hi of an i64 param in R0:R1). Fixes: (a) seed `i64_locals` from `self.params_i64`; (b) reserve the hi half of a live i64 param in the #193 reservation (live_param_regs); (c) map params to registers via the new `aapcs_param_regs` (even-aligned pairs — (i32,i64) -> R0,R2:R3, not the sequential R1:R2 the old index_to_reg used). All-i32 signatures map identically to index_to_reg, so non-i64-param functions are byte-identical. - OPTIMIZED (ir_to_arm): the I64Load param-home guard `num_params >= 2/4` conflated register-count with param-count and dropped the param (read from a fresh R4:R5 pair). Since ir_to_arm lacks per-param TYPES it cannot compute AAPCS pairing — decline any function that reads an i64 param to the direct selector (the #359/#188/#507 honest-degradation pattern). Two i64-param sub-cases are declined LOUDLY (Ok-or-Err, never silent wrong-code), their correct lowering tracked as follow-ups: - an i64 param AAPCS-passed PAST R3 (stack) — the open #503-i64 case; - an i64 param in a FRAME-BACKING function (has a call, or the pair-exhaustion retry) — the param_slots path sizes an i64 param's slot from a width set that excludes params, dropping the high half. So no silent wrong-code remains: every i64-param function is either correctly compiled (leaf, register-resident, the common case) or loud-skipped. Gate: scripts/repro/i64_param_518_differential.py flipped to the correctness gate (EXPECT_MISCOMPILE=False) — 11 leaf cases match wasmtime on BOTH paths across the full AAPCS matrix (single i64, (i32,i64), (i64,i64), second-i64-param), PLUS a decline-contract check (i64_param_518_decline.wat): d_past_r3 + d_call loud-skip, d_leaf emitted + executes correctly. Frozen anchors 3/3 byte-identical (control_step 0x00210A55 / flight_algo 0x07FDF307 / divseam — no i64 params). Updated the #94 hi32-extract byte-size test to source its i64 from a sign-extended i32 param (a non-param i64 that stays on the optimized path) — the old i64-param shape was itself exercising the now-declined miscompile, masked by a size-only assertion. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent dd0281b commit b4ec36e

5 files changed

Lines changed: 360 additions & 37 deletions

File tree

crates/synth-backend/src/arm_backend.rs

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1434,9 +1434,18 @@ mod tests {
14341434
..CompileConfig::default()
14351435
};
14361436

1437-
// Optimized path: `(local.get 0) >>> 32; wrap_i64`
1437+
// #518: the i64 value must NOT come from an i64 PARAM — the optimized
1438+
// path now declines i64-param functions to the direct selector (it homed
1439+
// an i64 param in R4:R5 instead of R0:R1, a silent miscompile this test's
1440+
// byte-size-only assertion masked). The canonical #94 case is a u64 from
1441+
// an FFI return, not a param, anyway. Source the i64 from a sign-extended
1442+
// i32 param (`extend_i32_s`): a runtime, non-constant-foldable i64 that
1443+
// stays on the optimized path, so the shift-by-32 hi-extract peephole is
1444+
// still exercised on CORRECT code.
1445+
// Optimized path: `(i64.extend_i32_s (local.get 0)) >>> 32; wrap_i64`
14381446
let ops_hi32 = vec![
1439-
WasmOp::LocalGet(0), // i64 param in R0:R1
1447+
WasmOp::LocalGet(0), // i32 param in R0
1448+
WasmOp::I64ExtendI32S,
14401449
WasmOp::I64Const(32),
14411450
WasmOp::I64ShrU,
14421451
WasmOp::I32WrapI64,
@@ -1445,11 +1454,11 @@ mod tests {
14451454
.compile_function("hi32_extract", &ops_hi32, &config)
14461455
.unwrap();
14471456

1448-
// Generic path: `(local.get 0) >>> 7; wrap_i64` — same shape, but the
1449-
// shift amount is not a multiple of 32, so it falls through to the
1450-
// 38-byte runtime shift.
1457+
// Generic path: `... >>> 7; wrap_i64` — same shape, but the shift amount
1458+
// is not a multiple of 32, so it falls through to the runtime shift.
14511459
let ops_generic = vec![
14521460
WasmOp::LocalGet(0),
1461+
WasmOp::I64ExtendI32S,
14531462
WasmOp::I64Const(7),
14541463
WasmOp::I64ShrU,
14551464
WasmOp::I32WrapI64,

crates/synth-synthesis/src/instruction_selector.rs

Lines changed: 172 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,56 @@ fn index_to_reg(index: u8) -> Reg {
132132
reg
133133
}
134134

135+
/// AAPCS core-register assignment for a function's first parameters (#518).
136+
///
137+
/// Walks the declared param widths left-to-right over the 4 core argument
138+
/// registers R0..R3. An i32/f32 param takes the next register; an i64/f64 param
139+
/// takes an EVEN-ALIGNED consecutive pair (rounding the cursor up to an even
140+
/// index first) and the returned register is its LOW half (hi = [`i64_pair_hi`]).
141+
/// Returns the lo register for every param that lands wholly within R0..R3; a
142+
/// param that would spill past R3 is stack-passed and is absent from the map
143+
/// (an i64 there is the open #503-i64 case, declined by the caller).
144+
///
145+
/// For an all-i32 signature this is exactly `index_to_reg(i)` for each `i`, so
146+
/// the param→register mapping is byte-identical to the legacy sequential scheme
147+
/// whenever no i64/f64 param is present — the #518 fix only diverges for the
148+
/// i64-param functions that were miscompiled.
149+
fn aapcs_param_regs(num_params: u32, params_i64: &[bool]) -> std::collections::HashMap<u32, Reg> {
150+
let mut map = std::collections::HashMap::new();
151+
let mut next: u8 = 0; // next free core arg-register index (0..=3)
152+
for i in 0..num_params {
153+
let is_wide = params_i64.get(i as usize).copied().unwrap_or(false);
154+
if is_wide {
155+
if !next.is_multiple_of(2) {
156+
next += 1; // even-align the pair
157+
}
158+
if next <= 2 {
159+
// the pair (next, next+1) lands wholly within R0..R3
160+
map.insert(i, index_to_reg(next));
161+
next += 2;
162+
} else {
163+
next = 4; // spilled to the stack (#503-i64)
164+
}
165+
} else if next <= 3 {
166+
map.insert(i, index_to_reg(next));
167+
next += 1;
168+
} else {
169+
next = 4; // i32 stack param (#359 incoming-arg path)
170+
}
171+
}
172+
map
173+
}
174+
175+
/// True if any declared i64/f64 param of this function lands (wholly or its
176+
/// even-aligned start) PAST R3 — i.e. AAPCS passes it on the stack (#518/#503).
177+
/// The register-pair homing below only covers i64 params resident in R0..R3; a
178+
/// stack-passed 64-bit param is the open #503-i64 follow-up and is declined.
179+
fn i64_param_overflows_core_regs(num_params: u32, params_i64: &[bool]) -> bool {
180+
let regs = aapcs_param_regs(num_params, params_i64);
181+
(0..num_params)
182+
.any(|i| params_i64.get(i as usize).copied().unwrap_or(false) && !regs.contains_key(&i))
183+
}
184+
135185
/// A virtual-stack entry (#171). A value normally lives in a physical register
136186
/// ([`StackVal::Reg`] — for an i64, this is the `lo`; the `hi` is the
137187
/// conventional [`i64_pair_hi`]). When register pressure exhausts the
@@ -5382,6 +5432,41 @@ impl InstructionSelector {
53825432
)));
53835433
}
53845434

5435+
// #518 Ok-or-Err (#180/#185): an i64/f64 PARAM is correctly homed below
5436+
// (AAPCS register-pair, `aapcs_param_regs`) only for the LEAF,
5437+
// register-resident case. Two sub-cases are declined LOUDLY here rather
5438+
// than silently miscompiled (the #518 defect): (1) an i64 param that
5439+
// AAPCS passes PAST R3 on the stack — the open #503-i64 follow-up;
5440+
// (2) a function that FRAME-BACKS its params — `has_call` (params spill
5441+
// to the frame to survive the call's caller-saved clobber, #204/#193) or
5442+
// the pair-exhaustion retry (`param_backing_on_exhaustion`) — where the
5443+
// `param_slots` path would size an i64 param's slot from `i64_set`, which
5444+
// does not include params, dropping the high half. Both fall back to a
5445+
// loud skip (warning + absent symbol), never wrong code. The common
5446+
// leaf-in-registers case is FIXED below. (Empty `params_i64` ⇒ all-i32 ⇒
5447+
// this is a no-op and every existing fixture stays byte-identical.)
5448+
let has_i64_param = self.params_i64.iter().take(num_params as usize).any(|&w| w);
5449+
if has_i64_param {
5450+
if i64_param_overflows_core_regs(num_params, &self.params_i64) {
5451+
return Err(synth_core::Error::synthesis(
5452+
"#518/#503: an i64/f64 param is AAPCS-passed past R3 (on the \
5453+
stack); 64-bit stack params are not yet lowered"
5454+
.to_string(),
5455+
));
5456+
}
5457+
let has_call = wasm_ops
5458+
.iter()
5459+
.any(|op| matches!(op, Call(_) | CallIndirect { .. }));
5460+
if has_call || self.param_backing_on_exhaustion {
5461+
return Err(synth_core::Error::synthesis(
5462+
"#518: an i64/f64 param in a frame-backing function (contains a \
5463+
call, or the register-pair-exhaustion retry) is not yet \
5464+
lowered — the param_slots path drops the high half"
5465+
.to_string(),
5466+
));
5467+
}
5468+
}
5469+
53855470
// #359: size of the outgoing stack-argument region = the max over all
53865471
// Call/CallIndirect sites of `max(0, arg_count - 4) * 4`, rounded up to 8.
53875472
// Reserved at the BOTTOM of the frame (offset 0) by `compute_local_layout`.
@@ -5505,11 +5590,20 @@ impl InstructionSelector {
55055590
// Map of local index -> register
55065591
let mut local_to_reg: std::collections::HashMap<u32, Reg> =
55075592
std::collections::HashMap::new();
5508-
// First 4 params are in r0-r3
5593+
// First 4 params are in r0-r3.
5594+
// #518: register homes follow the AAPCS core-register assignment
5595+
// (`aapcs_param_regs`) — an i64 param takes an even-aligned pair, so e.g.
5596+
// `(i32, i64)` puts the i64 in R2:R3, not the sequential R1:R2 that the
5597+
// old `index_to_reg(i)` mapping wrongly used. For an all-i32 signature
5598+
// this is identical to `index_to_reg(i)`, so non-i64-param functions are
5599+
// byte-identical. (Frame-backing i64 params are declined above, so any
5600+
// `param_slots` entry reached here is i32 — its sequential `index_to_reg`
5601+
// already equals the AAPCS reg; left unchanged to minimise the diff.)
55095602
// #204/#193: a param the function reads is spilled to a frame slot at
55105603
// entry and accessed only through it, so it can never be clobbered in
55115604
// its home register between reads. Params with no slot (unused) stay
55125605
// register-backed via local_to_reg.
5606+
let param_aapcs = aapcs_param_regs(num_params, &self.params_i64);
55135607
for i in 0..num_params.min(4) {
55145608
if let Some(&(off, is_i64)) = layout.param_slots.get(&i) {
55155609
let reg = index_to_reg(i as u8);
@@ -5530,11 +5624,27 @@ impl InstructionSelector {
55305624
source_line: None,
55315625
});
55325626
} else {
5533-
local_to_reg.insert(i, index_to_reg(i as u8));
5627+
let reg = param_aapcs
5628+
.get(&i)
5629+
.copied()
5630+
.unwrap_or_else(|| index_to_reg(i as u8));
5631+
local_to_reg.insert(i, reg);
55345632
}
55355633
}
55365634

5537-
let i64_locals = infer_i64_locals(wasm_ops, &self.func_ret_i64, &self.type_ret_i64);
5635+
let mut i64_locals = infer_i64_locals(wasm_ops, &self.func_ret_i64, &self.type_ret_i64);
5636+
// #518: an i64/f64 PARAM is 64-bit by signature even if it is only READ
5637+
// (never `local.set`/`tee`), which `infer_i64_locals` — driven by
5638+
// LocalSet/Tee/Call result widths — cannot see. Seed those indices so a
5639+
// `LocalGet` of an i64 param pushes a `StackVal::i64` (whose hi register
5640+
// is reserved via `i64_pair_hi`) instead of an i32 entry that left the hi
5641+
// unreserved — the exact direct-path #518 mechanism (a following
5642+
// `i64.const` was then allocated into the param's hi register).
5643+
for (k, &wide) in self.params_i64.iter().take(num_params as usize).enumerate() {
5644+
if wide {
5645+
i64_locals.insert(k as u32);
5646+
}
5647+
}
55385648

55395649
// VCR-RA local promotion (#390, #242): choose non-param i32 locals to keep
55405650
// in callee-saved registers (r4..r8) instead of frame slots, and seed them
@@ -5573,11 +5683,23 @@ impl InstructionSelector {
55735683
}
55745684
}
55755685
let live_param_regs = |at: usize| -> Vec<Reg> {
5576-
param_regs
5577-
.iter()
5578-
.filter(|(p, _)| param_last_read.get(p).is_some_and(|&last| last >= at))
5579-
.map(|(_, r)| *r)
5580-
.collect::<Vec<_>>()
5686+
let mut out = Vec::new();
5687+
for (p, r) in &param_regs {
5688+
if param_last_read.get(p).is_some_and(|&last| last >= at) {
5689+
out.push(*r);
5690+
// #518: an i64 param occupies a register PAIR (lo in `r`, hi in
5691+
// `i64_pair_hi(r)`). Reserve the hi half too — otherwise a
5692+
// constant/temp allocated into it clobbers the param's high
5693+
// word before the i64 op reads it (the direct-path #518 bug:
5694+
// `movw r1,#K` overwrote R1 = hi of an i64 param in R0:R1).
5695+
if i64_locals.contains(p)
5696+
&& let Ok(hi) = i64_pair_hi(*r)
5697+
{
5698+
out.push(hi);
5699+
}
5700+
}
5701+
}
5702+
out
55815703
};
55825704

55835705
for (idx, op) in wasm_ops.iter().enumerate() {
@@ -10415,6 +10537,48 @@ mod tests {
1041510537
use super::*;
1041610538
use crate::rules::RuleDatabase;
1041710539

10540+
/// #518: the AAPCS core-register assignment for parameters. An i64 takes an
10541+
/// even-aligned consecutive pair (lo returned); a param that spills past R3 is
10542+
/// absent. For an all-i32 signature it must equal `index_to_reg(i)`, so
10543+
/// non-i64-param functions stay byte-identical.
10544+
#[test]
10545+
fn test_aapcs_param_regs_518() {
10546+
let m = |np: u32, w: &[bool]| aapcs_param_regs(np, w);
10547+
// single i64 -> R0:R1 (lo = R0)
10548+
assert_eq!(m(1, &[true]).get(&0), Some(&Reg::R0));
10549+
// (i32, i64) -> R0, then even-aligned R2:R3 (R1 skipped)
10550+
let mixed = m(2, &[false, true]);
10551+
assert_eq!(mixed.get(&0), Some(&Reg::R0));
10552+
assert_eq!(mixed.get(&1), Some(&Reg::R2));
10553+
// (i64, i64) -> R0:R1, R2:R3
10554+
let two = m(2, &[true, true]);
10555+
assert_eq!(two.get(&0), Some(&Reg::R0));
10556+
assert_eq!(two.get(&1), Some(&Reg::R2));
10557+
// (i32, i32, i64) -> R0, R1, even-aligned R2:R3
10558+
let m3 = m(3, &[false, false, true]);
10559+
assert_eq!(m3.get(&2), Some(&Reg::R2));
10560+
// (i64, i32) -> R0:R1, R2
10561+
let i64_i32 = m(2, &[true, false]);
10562+
assert_eq!(i64_i32.get(&0), Some(&Reg::R0));
10563+
assert_eq!(i64_i32.get(&1), Some(&Reg::R2));
10564+
// all-i32 must match the legacy sequential index_to_reg (byte-identity)
10565+
let all_i32 = m(4, &[false, false, false, false]);
10566+
for i in 0..4u32 {
10567+
assert_eq!(all_i32.get(&i), Some(&index_to_reg(i as u8)));
10568+
}
10569+
// (i32, i32, i32, i64): the i64 even-aligns to R4:R5 -> past R3 -> absent
10570+
let overflow = m(4, &[false, false, false, true]);
10571+
assert_eq!(overflow.get(&3), None);
10572+
assert!(i64_param_overflows_core_regs(
10573+
4,
10574+
&[false, false, false, true]
10575+
));
10576+
// the in-register cases never report overflow
10577+
assert!(!i64_param_overflows_core_regs(1, &[true]));
10578+
assert!(!i64_param_overflows_core_regs(2, &[false, true]));
10579+
assert!(!i64_param_overflows_core_regs(2, &[true, true]));
10580+
}
10581+
1041810582
#[test]
1041910583
fn test_register_allocation() {
1042010584
let mut regs = RegisterState::new();

0 commit comments

Comments
 (0)