wasm32: Fix undefined behavior with shift intrinsics #1737
Conversation
| @@ -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() } | |||
There was a problem hiding this comment.
Seems like a good time to add some SAFETY comments to these unsafe blocks?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The documentation of simd_shl says:
https://doc.rust-lang.org/nightly/std/intrinsics/simd/fn.simd_shl.html
Safety
Each element of
rhsmust be less than<int>::BITS.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
"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.
There was a problem hiding this comment.
Ok, I've added some words to explain what's going on here
There was a problem hiding this comment.
That's a lot more detail than I expected, sounds great :) Thanks!
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
alexcrichton
force-pushed
the
fix-ub-shift-wasm
branch
from
2033e4a to
95cfe78
Compare
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