Skip to content

wasm32: Fix undefined behavior with shift intrinsics #1737

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 16, 2025
Merged

wasm32: Fix undefined behavior with shift intrinsics #1737

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5f64711
wasm32: Fix undefined behavior with shift intrinsics
alexcrichton Mar 3, 2025
70ba9a5
Fix rustfmt
alexcrichton Mar 3, 2025
95cfe78
Document safety conditions of simd shifts
alexcrichton Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 52 additions & 12 deletions crates/core_arch/src/wasm32/simd128.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2318,7 +2318,25 @@ 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() }
// 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() }
Copy link
Member

@RalfJung RalfJung Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good time to add some SAFETY comments to these unsafe blocks?

Copy link
Member Author

@alexcrichton alexcrichton Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a suggestion of what to write in there? I'm somewhat skeptical that would have helped here since english-prose beforehand would have said "the wasm intrinsic has no UB and unsafe here is only because we can't declare safe intrinsics". Basically english-prose before this change wouldn't have been likely to cross-reference the definiton with LLVM and realize the mask here is necessary.

I can certainly add in that the mask is required for safety, but are you envisioning something longer-form? Or something copy/pastable to other intrinsic definition sites? For example while the basic operations like shifts have possible UB in LLVM I suspect most of the wasm-specific intrinsics are mostly undocumented in LLVM at this time.

Copy link
Member

@bjorn3 bjorn3 Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation of simd_shl says:

https://doc.rust-lang.org/nightly/std/intrinsics/simd/fn.simd_shl.html

Safety

Each element of rhs must be less than <int>::BITS.

Copy link
Member Author

@alexcrichton alexcrichton Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'm of course not doubting that, my point is that assuming an author of a SAFETY comment would have cross-referenced every single intrinsic in this file is, in my opinion, a bit of a reach. It's obvious to do such a cross reference when one such intrinsic when this is under the spotlight, I'm not doubting that.

My point is that @RalfJung your comment seems to indicate a sentiment along the lines of "surely this bug would not have happened with a SAFETY comment, so let's fix that issue while we're at it". I'm not doubting one should be written, but I'd like to confirm if my suspicion here is correct. Basically I don't know how such an audit could be done for the rest of the file reasonably.

Copy link
Member

@RalfJung RalfJung Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"surely this bug would not have happened with a SAFETY comment, so let's fix that issue while we're at it"

I wouldn't say "surely", but I think it would have increased the chances.

A simple "shift amount is masked and therefore less than 16" or so should suffice.

Copy link
Member Author

@alexcrichton alexcrichton Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've added some words to explain what's going on here

Copy link
Member

@RalfJung RalfJung Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot more detail than I expected, sounds great :) Thanks!

}

#[stable(feature = "wasm_simd", since = "1.54.0")]
Expand All @@ -2335,7 +2353,9 @@ 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() }
// 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() }
}

/// Shifts each lane to the right by the specified number of bits, shifting in
Expand All @@ -2349,7 +2369,9 @@ 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() }
// 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() }
}

/// Adds two 128-bit vectors as if they were two packed sixteen 8-bit integers.
Expand Down Expand Up @@ -2686,7 +2708,9 @@ 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() }
// 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() }
}

#[stable(feature = "wasm_simd", since = "1.54.0")]
Expand All @@ -2703,7 +2727,9 @@ 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() }
// 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() }
}

/// Shifts each lane to the right by the specified number of bits, shifting in
Expand All @@ -2717,7 +2743,9 @@ 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() }
// 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() }
}

/// Adds two 128-bit vectors as if they were two packed eight 16-bit integers.
Expand Down Expand Up @@ -3136,7 +3164,9 @@ 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() }
// 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() }
}

#[stable(feature = "wasm_simd", since = "1.54.0")]
Expand All @@ -3153,7 +3183,9 @@ 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() }
// 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() }
}

/// Shifts each lane to the right by the specified number of bits, shifting in
Expand All @@ -3167,7 +3199,9 @@ 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() }
// 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() }
}

/// Adds two 128-bit vectors as if they were two packed four 32-bit integers.
Expand Down Expand Up @@ -3502,7 +3536,9 @@ 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() }
// 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() }
}

#[stable(feature = "wasm_simd", since = "1.54.0")]
Expand All @@ -3519,7 +3555,9 @@ 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() }
// 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() }
}

/// Shifts each lane to the right by the specified number of bits, shifting in
Expand All @@ -3533,7 +3571,9 @@ 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() }
// 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() }
}

/// Adds two 128-bit vectors as if they were two packed two 64-bit integers.
Expand Down