From 755204f11e0eee42c702908b16c395d6490bbb41 Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Thu, 4 Jun 2026 23:59:49 +0200 Subject: [PATCH] =?UTF-8?q?fix(encoder):=20CMP/ADDS/SUBS=20immediates=20vi?= =?UTF-8?q?a=20ThumbExpandImm=20=E2=80=94=20close=20the=20silent-miscompil?= =?UTF-8?q?e=20class=20(#255)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gale #255: encode_thumb32_cmp_imm / _adds / _subs packed the raw `i:imm3:imm8` field with no ThumbExpandImm expansion — correct only for imm <= 0xFF, silently mis-encoding any larger value (e.g. `cmp #N` comparing the wrong constant → wrong branch). Same class as #251 (ORR/EOR NOP) and #253 (ADD/SUB frame). Fix (gale's suggested shared helper): add `try_thumb_expand_imm(u32) -> Option` — the correct ARMv7-M ThumbExpandImm reverse-encoder (zero-extended byte, the three replicated-byte patterns, and the 8-bit-rotated form). CMP/ADDS/SUBS now encode any valid modified immediate CORRECTLY and return Err on a genuinely unrepresentable one (CMP/S-forms have no plain-imm12 fallback), forcing the selector to materialize into a register rather than silently mis-compile. Note: gale's example `cmp #1000` is actually representable (1000 = 0xFA ror 30, field 0xF7A) — the old code mis-encoded it (raw 0x3E8); this encodes it right. The genuine error case is e.g. `#0x101` (bits too far apart for an 8-bit window). GATE: clippy clean; backend suite passes; three frozen differentials RESULT- identical (control_step 0x00210A55, flight_seam 0x07FDF307, div_const 338/338) — the fixed paths weren't exercised by fixtures (the comparison lowering uses reg-reg CMP), so this is purely defensive hardening of a latent class. Tests try_thumb_expand_imm_encodes_modified_immediates + cmp_adds_subs_immediate_error_on_non_modified_imm. Closes #255. Part of #242. (A follow-up can retrofit AND/ORR/EOR to share this helper too, extending their fold range beyond 0xFF.) Co-Authored-By: Claude Opus 4.8 --- crates/synth-backend/src/arm_encoder.rs | 131 ++++++++++++++++++++++-- 1 file changed, 122 insertions(+), 9 deletions(-) diff --git a/crates/synth-backend/src/arm_encoder.rs b/crates/synth-backend/src/arm_encoder.rs index 3a2f331..f0c7d1c 100644 --- a/crates/synth-backend/src/arm_encoder.rs +++ b/crates/synth-backend/src/arm_encoder.rs @@ -6093,9 +6093,16 @@ impl ArmEncoder { let rd_bits = reg_to_bits(rd); let rn_bits = reg_to_bits(rn); - let i_bit = (imm >> 11) & 1; - let imm3 = (imm >> 8) & 0x7; - let imm8 = imm & 0xFF; + // ADDS.W (flag-setting) has only the modified-immediate form — error on + // an un-encodable value rather than silently add the wrong constant. + let field = try_thumb_expand_imm(imm).ok_or_else(|| { + synth_core::Error::synthesis( + "ADDS immediate is not a valid ThumbExpandImm — materialize into a register", + ) + })?; + let i_bit = (field >> 11) & 1; + let imm3 = (field >> 8) & 0x7; + let imm8 = field & 0xFF; // ADDS.W Rd, Rn, #imm (with S=1) // First halfword: 1111 0 i 0 1000 1 Rn = F110 | i<<10 | Rn @@ -6112,9 +6119,16 @@ impl ArmEncoder { let rd_bits = reg_to_bits(rd); let rn_bits = reg_to_bits(rn); - let i_bit = (imm >> 11) & 1; - let imm3 = (imm >> 8) & 0x7; - let imm8 = imm & 0xFF; + // SUBS.W (flag-setting) has only the modified-immediate form — error on + // an un-encodable value rather than silently subtract the wrong constant. + let field = try_thumb_expand_imm(imm).ok_or_else(|| { + synth_core::Error::synthesis( + "SUBS immediate is not a valid ThumbExpandImm — materialize into a register", + ) + })?; + let i_bit = (field >> 11) & 1; + let imm3 = (field >> 8) & 0x7; + let imm8 = field & 0xFF; // SUBS.W Rd, Rn, #imm (with S=1) // First halfword: 1111 0 i 0 1101 1 Rn = F1B0 | i<<10 | Rn @@ -6217,9 +6231,17 @@ impl ArmEncoder { fn encode_thumb32_cmp_imm(&self, rn: &Reg, imm: u32) -> Result> { let rn_bits = reg_to_bits(rn); - let i_bit = (imm >> 11) & 1; - let imm3 = (imm >> 8) & 0x7; - let imm8 = imm & 0xFF; + // CMP.W has only the modified-immediate form (no plain-imm12 like ADDW), + // so an un-encodable immediate MUST be materialized into a register by + // the selector. Error rather than silently compare the wrong constant. + let field = try_thumb_expand_imm(imm).ok_or_else(|| { + synth_core::Error::synthesis( + "CMP immediate is not a valid ThumbExpandImm — materialize into a register", + ) + })?; + let i_bit = (field >> 11) & 1; + let imm3 = (field >> 8) & 0x7; + let imm8 = field & 0xFF; // CMP.W Rn, #imm let hw1: u16 = (0xF1B0 | (i_bit << 10) | rn_bits) as u16; @@ -6636,6 +6658,43 @@ impl ArmEncoder { } /// Convert register to bit encoding (0-15) +/// Reverse of the ARMv7-M `ThumbExpandImm`: given a 32-bit immediate, return the +/// 12-bit `i:imm3:imm8` field if it is a representable modified immediate, else +/// `None` (the caller must materialize the value into a register). This is the +/// shared correct path for the data-processing immediate encoders — without it +/// they pack raw bits and silently mis-encode any value `> 0xFF` that isn't a +/// modified immediate (the silent-miscompile class behind #251/#253/#255). +fn try_thumb_expand_imm(value: u32) -> Option { + // i:imm3 = 0000 → 8-bit value, zero-extended (00000000 00000000 00000000 XY). + if value <= 0xFF { + return Some(value); + } + let b0 = value & 0xFF; // byte 0 + let b1 = (value >> 8) & 0xFF; // byte 1 + // 0x00XY00XY (i:imm3 = 0001) — XY in bytes 0 and 2 + if value == (b0 << 16) | b0 { + return Some(0x100 | b0); + } + // 0xXY00XY00 (i:imm3 = 0010) — XY in bytes 1 and 3 + if value == (b1 << 24) | (b1 << 8) { + return Some(0x200 | b1); + } + // 0xXYXYXYXY (i:imm3 = 0011) — XY in all four bytes + if value == (b0 << 24) | (b0 << 16) | (b0 << 8) | b0 { + return Some(0x300 | b0); + } + // An 8-bit value with bit 7 set, rotated right by 8..=31. `rotate_left(rot)` + // undoes the encoded right rotation; if the result is `1bbbbbbb` (0x80..=0xFF) + // the value is representable. imm12[11:7] = rot, imm12[6:0] = low 7 bits. + for rot in 8..=31u32 { + let unrot = value.rotate_left(rot); + if (0x80..=0xFF).contains(&unrot) { + return Some((rot << 7) | (unrot & 0x7F)); + } + } + None +} + fn reg_to_bits(reg: &Reg) -> u32 { match reg { Reg::R0 => 0, @@ -9042,6 +9101,60 @@ mod tests { ); } + /// #255: the shared ThumbExpandImm reverse-encoder underpinning the + /// data-processing immediate fix. Encodable modified immediates round-trip to + /// the expected `i:imm3:imm8` field; a genuinely non-modified value is `None` + /// (caller must materialize into a register). Note `1000 = 0xFA ror 30` *is* + /// representable (field 0xF7A) — the old encoder mis-encoded it (raw 0x3E8); + /// this encodes it correctly. + #[test] + fn try_thumb_expand_imm_encodes_modified_immediates() { + assert_eq!(try_thumb_expand_imm(0x7e), Some(0x07e)); // zero-extended byte + assert_eq!(try_thumb_expand_imm(0xff), Some(0x0ff)); + assert_eq!(try_thumb_expand_imm(0x0001_0001), Some(0x101)); // 0x00XY00XY + assert_eq!(try_thumb_expand_imm(0xff00_ff00), Some(0x2ff)); // 0xXY00XY00 + assert_eq!(try_thumb_expand_imm(0xffff_ffff), Some(0x3ff)); // 0xXYXYXYXY + assert_eq!(try_thumb_expand_imm(0x100), Some(0xf80)); // 0x80 ror 31 + assert_eq!(try_thumb_expand_imm(0x8000_0000), Some(0x400)); // 0x80 ror 8 + assert_eq!(try_thumb_expand_imm(1000), Some(0xf7a)); // 0xFA ror 30 + // Genuinely unrepresentable (bits too far apart for an 8-bit window). + assert_eq!(try_thumb_expand_imm(0x101), None); + assert_eq!(try_thumb_expand_imm(0x12345), None); + } + + /// #255: CMP/ADDS/SUBS encode any valid modified immediate correctly, and + /// ERROR (not silently mis-encode) on a genuinely unrepresentable one, + /// forcing the selector to materialize into a register — closing the + /// silent-miscompile class of #251/#253. + #[test] + fn cmp_adds_subs_immediate_error_on_non_modified_imm() { + let encoder = ArmEncoder::new_thumb2(); + // cmp r0, #0xff → valid → Ok; cmp r0, #1000 → valid (0xFA ror 30) → Ok. + assert!(encoder.encode_thumb32_cmp_imm(&Reg::R0, 0xff).is_ok()); + assert!(encoder.encode_thumb32_cmp_imm(&Reg::R0, 1000).is_ok()); + // cmp r0, #0x101 → NOT a modified immediate → Err (materialize-reg). + assert!( + encoder.encode_thumb32_cmp_imm(&Reg::R0, 0x101).is_err(), + "cmp #0x101 must error, not compare the wrong constant" + ); + assert!( + encoder + .encode_thumb32_adds(&Reg::R0, &Reg::R0, 0x101) + .is_err() + ); + assert!( + encoder + .encode_thumb32_subs(&Reg::R0, &Reg::R0, 0x101) + .is_err() + ); + // ...but a valid modified immediate still encodes. + assert!( + encoder + .encode_thumb32_adds(&Reg::R0, &Reg::R0, 0x80) + .is_ok() + ); + } + /// Latent miscompile fix: ADD/SUB with a >0xFF immediate (e.g. /// `add sp, sp, #frame` for a >=256-byte frame) used ADD.W (T3), whose /// `i:imm3:imm8` is a ThumbExpandImm modified immediate — so `#256` silently