From 5f647114bc0a5a37ec1223026fe8a1dc791f86b1 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 3 Mar 2025 07:41:50 -0800 Subject: [PATCH 1/3] wasm32: Fix undefined behavior with shift intrinsics This commit fixes an issue where simd shift intrinsic in LLVM are undefined behavior if the shift amount is larger than the bit width of the lane. While in WebAssembly the corresponding instructions are defined as masking out the upper bits we need to represent that explicitly in LLVM IR to ensure that the semantics remain defined. cc rust-lang/rust#137941 --- crates/core_arch/src/wasm32/simd128.rs | 28 ++++++++++++-------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/crates/core_arch/src/wasm32/simd128.rs b/crates/core_arch/src/wasm32/simd128.rs index a774b68cfd..8812245b79 100644 --- a/crates/core_arch/src/wasm32/simd128.rs +++ b/crates/core_arch/src/wasm32/simd128.rs @@ -2318,7 +2318,7 @@ pub fn u8x16_narrow_i16x8(a: v128, b: v128) -> v128 { #[doc(alias("i8x16.shl"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i8x16_shl(a: v128, amt: u32) -> v128 { - unsafe { simd_shl(a.as_i8x16(), simd::i8x16::splat(amt as i8)).v128() } + unsafe { simd_shl(a.as_i8x16(), simd::i8x16::splat((amt & 0x7) as i8)).v128() } } #[stable(feature = "wasm_simd", since = "1.54.0")] @@ -2335,7 +2335,7 @@ pub use i8x16_shl as u8x16_shl; #[doc(alias("i8x16.shr_s"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i8x16_shr(a: v128, amt: u32) -> v128 { - unsafe { simd_shr(a.as_i8x16(), simd::i8x16::splat(amt as i8)).v128() } + unsafe { simd_shr(a.as_i8x16(), simd::i8x16::splat((amt & 0x7) as i8)).v128() } } /// Shifts each lane to the right by the specified number of bits, shifting in @@ -2349,7 +2349,7 @@ pub fn i8x16_shr(a: v128, amt: u32) -> v128 { #[doc(alias("i8x16.shr_u"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn u8x16_shr(a: v128, amt: u32) -> v128 { - unsafe { simd_shr(a.as_u8x16(), simd::u8x16::splat(amt as u8)).v128() } + unsafe { simd_shr(a.as_u8x16(), simd::u8x16::splat((amt & 0x7) as u8)).v128() } } /// Adds two 128-bit vectors as if they were two packed sixteen 8-bit integers. @@ -2686,7 +2686,7 @@ pub use i16x8_extend_high_u8x16 as u16x8_extend_high_u8x16; #[doc(alias("i16x8.shl"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i16x8_shl(a: v128, amt: u32) -> v128 { - unsafe { simd_shl(a.as_i16x8(), simd::i16x8::splat(amt as i16)).v128() } + unsafe { simd_shl(a.as_i16x8(), simd::i16x8::splat((amt & 0xf) as i16)).v128() } } #[stable(feature = "wasm_simd", since = "1.54.0")] @@ -2703,7 +2703,7 @@ pub use i16x8_shl as u16x8_shl; #[doc(alias("i16x8.shr_s"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i16x8_shr(a: v128, amt: u32) -> v128 { - unsafe { simd_shr(a.as_i16x8(), simd::i16x8::splat(amt as i16)).v128() } + unsafe { simd_shr(a.as_i16x8(), simd::i16x8::splat((amt & 0xf) as i16)).v128() } } /// Shifts each lane to the right by the specified number of bits, shifting in @@ -2717,7 +2717,7 @@ pub fn i16x8_shr(a: v128, amt: u32) -> v128 { #[doc(alias("i16x8.shr_u"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn u16x8_shr(a: v128, amt: u32) -> v128 { - unsafe { simd_shr(a.as_u16x8(), simd::u16x8::splat(amt as u16)).v128() } + unsafe { simd_shr(a.as_u16x8(), simd::u16x8::splat((amt & 0xf) as u16)).v128() } } /// Adds two 128-bit vectors as if they were two packed eight 16-bit integers. @@ -3136,7 +3136,7 @@ pub use i32x4_extend_high_u16x8 as u32x4_extend_high_u16x8; #[doc(alias("i32x4.shl"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i32x4_shl(a: v128, amt: u32) -> v128 { - unsafe { simd_shl(a.as_i32x4(), simd::i32x4::splat(amt as i32)).v128() } + unsafe { simd_shl(a.as_i32x4(), simd::i32x4::splat((amt & 0x1f) as i32)).v128() } } #[stable(feature = "wasm_simd", since = "1.54.0")] @@ -3153,7 +3153,7 @@ pub use i32x4_shl as u32x4_shl; #[doc(alias("i32x4.shr_s"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i32x4_shr(a: v128, amt: u32) -> v128 { - unsafe { simd_shr(a.as_i32x4(), simd::i32x4::splat(amt as i32)).v128() } + unsafe { simd_shr(a.as_i32x4(), simd::i32x4::splat((amt & 0x1f) as i32)).v128() } } /// Shifts each lane to the right by the specified number of bits, shifting in @@ -3167,7 +3167,7 @@ pub fn i32x4_shr(a: v128, amt: u32) -> v128 { #[doc(alias("i32x4.shr_u"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn u32x4_shr(a: v128, amt: u32) -> v128 { - unsafe { simd_shr(a.as_u32x4(), simd::u32x4::splat(amt)).v128() } + unsafe { simd_shr(a.as_u32x4(), simd::u32x4::splat(amt & 0x1f)).v128() } } /// Adds two 128-bit vectors as if they were two packed four 32-bit integers. @@ -3502,7 +3502,7 @@ pub use i64x2_extend_high_u32x4 as u64x2_extend_high_u32x4; #[doc(alias("i64x2.shl"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i64x2_shl(a: v128, amt: u32) -> v128 { - unsafe { simd_shl(a.as_i64x2(), simd::i64x2::splat(amt as i64)).v128() } + unsafe { simd_shl(a.as_i64x2(), simd::i64x2::splat((amt & 0x3f) as i64)).v128() } } #[stable(feature = "wasm_simd", since = "1.54.0")] @@ -3519,7 +3519,7 @@ pub use i64x2_shl as u64x2_shl; #[doc(alias("i64x2.shr_s"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i64x2_shr(a: v128, amt: u32) -> v128 { - unsafe { simd_shr(a.as_i64x2(), simd::i64x2::splat(amt as i64)).v128() } + unsafe { simd_shr(a.as_i64x2(), simd::i64x2::splat((amt & 0x3f) as i64)).v128() } } /// Shifts each lane to the right by the specified number of bits, shifting in @@ -3533,7 +3533,7 @@ pub fn i64x2_shr(a: v128, amt: u32) -> v128 { #[doc(alias("i64x2.shr_u"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn u64x2_shr(a: v128, amt: u32) -> v128 { - unsafe { simd_shr(a.as_u64x2(), simd::u64x2::splat(amt as u64)).v128() } + unsafe { simd_shr(a.as_u64x2(), simd::u64x2::splat((amt & 0x3f) as u64)).v128() } } /// Adds two 128-bit vectors as if they were two packed two 64-bit integers. @@ -4344,9 +4344,7 @@ mod tests { }; assert_eq!( bytes, - [ - -1, -2, -3, -4, -5, -6, -7, -8, -9, -10, -11, -12, -13, -14, -15, -16 - ] + [-1, -2, -3, -4, -5, -6, -7, -8, -9, -10, -11, -12, -13, -14, -15, -16] ); } From 70ba9a541b0c254d58e11358210a7a72428e1ae6 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 3 Mar 2025 07:44:53 -0800 Subject: [PATCH 2/3] Fix rustfmt --- crates/core_arch/src/wasm32/simd128.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/core_arch/src/wasm32/simd128.rs b/crates/core_arch/src/wasm32/simd128.rs index 8812245b79..83214df4fe 100644 --- a/crates/core_arch/src/wasm32/simd128.rs +++ b/crates/core_arch/src/wasm32/simd128.rs @@ -4344,7 +4344,9 @@ mod tests { }; assert_eq!( bytes, - [-1, -2, -3, -4, -5, -6, -7, -8, -9, -10, -11, -12, -13, -14, -15, -16] + [ + -1, -2, -3, -4, -5, -6, -7, -8, -9, -10, -11, -12, -13, -14, -15, -16 + ] ); } From 95cfe7891c3efdf9671fa714dfe09ba071c104e5 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 10 Mar 2025 12:02:57 -0700 Subject: [PATCH 3/3] Document safety conditions of simd shifts --- crates/core_arch/src/wasm32/simd128.rs | 40 ++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/crates/core_arch/src/wasm32/simd128.rs b/crates/core_arch/src/wasm32/simd128.rs index 83214df4fe..1a1e7dc780 100644 --- a/crates/core_arch/src/wasm32/simd128.rs +++ b/crates/core_arch/src/wasm32/simd128.rs @@ -2318,6 +2318,24 @@ pub fn u8x16_narrow_i16x8(a: v128, b: v128) -> v128 { #[doc(alias("i8x16.shl"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i8x16_shl(a: v128, amt: u32) -> v128 { + // SAFETY: the safety of this intrinsic relies on the fact that the + // shift amount for each lane is less than the number of bits in the input + // lane. In this case the input has 8-bit lanes but the shift amount above + // is `u32`, so a mask is required to discard all the upper bits of `amt` to + // ensure that the safety condition is met. + // + // Note that this is distinct from the behavior of the native WebAssembly + // instruction here where WebAssembly defines this instruction as performing + // a mask as well. This is nonetheless required since this must have defined + // semantics in LLVM, not just WebAssembly. + // + // Finally note that this mask operation is not actually emitted into the + // final binary itself. LLVM understands that the wasm operation implicitly + // masks, so it knows this mask operation is redundant. + // + // Basically the extra mask here is required as a bridge from the documented + // semantics through LLVM back out to WebAssembly. Both ends have the + // documented semantics, and the mask is required by LLVM in the middle. unsafe { simd_shl(a.as_i8x16(), simd::i8x16::splat((amt & 0x7) as i8)).v128() } } @@ -2335,6 +2353,8 @@ pub use i8x16_shl as u8x16_shl; #[doc(alias("i8x16.shr_s"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i8x16_shr(a: v128, amt: u32) -> v128 { + // SAFETY: see i8x16_shl for more documentation why this is unsafe, + // essentially the shift amount must be valid hence the mask. unsafe { simd_shr(a.as_i8x16(), simd::i8x16::splat((amt & 0x7) as i8)).v128() } } @@ -2349,6 +2369,8 @@ pub fn i8x16_shr(a: v128, amt: u32) -> v128 { #[doc(alias("i8x16.shr_u"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn u8x16_shr(a: v128, amt: u32) -> v128 { + // SAFETY: see i8x16_shl for more documentation why this is unsafe, + // essentially the shift amount must be valid hence the mask. unsafe { simd_shr(a.as_u8x16(), simd::u8x16::splat((amt & 0x7) as u8)).v128() } } @@ -2686,6 +2708,8 @@ pub use i16x8_extend_high_u8x16 as u16x8_extend_high_u8x16; #[doc(alias("i16x8.shl"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i16x8_shl(a: v128, amt: u32) -> v128 { + // SAFETY: see i8x16_shl for more documentation why this is unsafe, + // essentially the shift amount must be valid hence the mask. unsafe { simd_shl(a.as_i16x8(), simd::i16x8::splat((amt & 0xf) as i16)).v128() } } @@ -2703,6 +2727,8 @@ pub use i16x8_shl as u16x8_shl; #[doc(alias("i16x8.shr_s"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i16x8_shr(a: v128, amt: u32) -> v128 { + // SAFETY: see i8x16_shl for more documentation why this is unsafe, + // essentially the shift amount must be valid hence the mask. unsafe { simd_shr(a.as_i16x8(), simd::i16x8::splat((amt & 0xf) as i16)).v128() } } @@ -2717,6 +2743,8 @@ pub fn i16x8_shr(a: v128, amt: u32) -> v128 { #[doc(alias("i16x8.shr_u"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn u16x8_shr(a: v128, amt: u32) -> v128 { + // SAFETY: see i8x16_shl for more documentation why this is unsafe, + // essentially the shift amount must be valid hence the mask. unsafe { simd_shr(a.as_u16x8(), simd::u16x8::splat((amt & 0xf) as u16)).v128() } } @@ -3136,6 +3164,8 @@ pub use i32x4_extend_high_u16x8 as u32x4_extend_high_u16x8; #[doc(alias("i32x4.shl"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i32x4_shl(a: v128, amt: u32) -> v128 { + // SAFETY: see i8x16_shl for more documentation why this is unsafe, + // essentially the shift amount must be valid hence the mask. unsafe { simd_shl(a.as_i32x4(), simd::i32x4::splat((amt & 0x1f) as i32)).v128() } } @@ -3153,6 +3183,8 @@ pub use i32x4_shl as u32x4_shl; #[doc(alias("i32x4.shr_s"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i32x4_shr(a: v128, amt: u32) -> v128 { + // SAFETY: see i8x16_shl for more documentation why this is unsafe, + // essentially the shift amount must be valid hence the mask. unsafe { simd_shr(a.as_i32x4(), simd::i32x4::splat((amt & 0x1f) as i32)).v128() } } @@ -3167,6 +3199,8 @@ pub fn i32x4_shr(a: v128, amt: u32) -> v128 { #[doc(alias("i32x4.shr_u"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn u32x4_shr(a: v128, amt: u32) -> v128 { + // SAFETY: see i8x16_shl for more documentation why this is unsafe, + // essentially the shift amount must be valid hence the mask. unsafe { simd_shr(a.as_u32x4(), simd::u32x4::splat(amt & 0x1f)).v128() } } @@ -3502,6 +3536,8 @@ pub use i64x2_extend_high_u32x4 as u64x2_extend_high_u32x4; #[doc(alias("i64x2.shl"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i64x2_shl(a: v128, amt: u32) -> v128 { + // SAFETY: see i8x16_shl for more documentation why this is unsafe, + // essentially the shift amount must be valid hence the mask. unsafe { simd_shl(a.as_i64x2(), simd::i64x2::splat((amt & 0x3f) as i64)).v128() } } @@ -3519,6 +3555,8 @@ pub use i64x2_shl as u64x2_shl; #[doc(alias("i64x2.shr_s"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn i64x2_shr(a: v128, amt: u32) -> v128 { + // SAFETY: see i8x16_shl for more documentation why this is unsafe, + // essentially the shift amount must be valid hence the mask. unsafe { simd_shr(a.as_i64x2(), simd::i64x2::splat((amt & 0x3f) as i64)).v128() } } @@ -3533,6 +3571,8 @@ pub fn i64x2_shr(a: v128, amt: u32) -> v128 { #[doc(alias("i64x2.shr_u"))] #[stable(feature = "wasm_simd", since = "1.54.0")] pub fn u64x2_shr(a: v128, amt: u32) -> v128 { + // SAFETY: see i8x16_shl for more documentation why this is unsafe, + // essentially the shift amount must be valid hence the mask. unsafe { simd_shr(a.as_u64x2(), simd::u64x2::splat((amt & 0x3f) as u64)).v128() } }