Skip to content

Are the wasm simd shl/shr intrinsics sound? #137941

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.

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

Closed
RalfJung opened this issue Mar 3, 2025 · 5 comments
Closed

Are the wasm simd shl/shr intrinsics sound? #137941

RalfJung opened this issue Mar 3, 2025 · 5 comments
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Mar 3, 2025

I found this in stdarch:

/// Shifts each lane to the left by the specified number of bits.
///
/// Only the low bits of the shift amount are used if the shift amount is
/// greater than the lane width.
#[inline]
#[cfg_attr(test, assert_instr(i16x8.shl))]
#[target_feature(enable = "simd128")]
#[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() }
}

However, simd_shl is UB when the shift amount exceeds the bitwidth. So the implementation does not seem to match what the doc comment says?

Cc @alexcrichton @Amanieu @workingjubilee

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 3, 2025
@hanna-kruppe
Copy link
Contributor

Yes, this is a plain bug, it needs to mask amt to be sound (if LLVM doesn’t exploit the UB then the lowering to wasm opcodes will behave as expected). Similar problem for the right-shift intrinsics using simd_shr.

@alexcrichton
Copy link
Member

Yes the documentation reflects the wasm intrinsic, and agreed it's a bug that should be fixed. If codegen regresses then that would ideally lead to an issue to improve it on LLVM's side, but the definition here should nevertheless change even if it regresses slightly.

@alexcrichton
Copy link
Member

Also, to dispel any doubt since this is simd-related, this is indeed a problem where with today's nightly:

use core::arch::wasm32::*;

#[no_mangle]
pub extern "C" fn foo(a: v128) -> v128 {
    i8x16_shl(a, 100)
}

compiles to

define dso_local <4 x i32> @foo(<4 x i32> %a) unnamed_addr #0 !dbg !5 {
  ret <4 x i32> undef, !dbg !26
}

alexcrichton added a commit to alexcrichton/stdarch that referenced this issue Mar 3, 2025
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
Copy link
Member

(sorry for multiple comments)

I've posted a fix to rust-lang/stdarch#1737 and additionally confirmed that adding this shift doesn't actually affect codegen, so once that's merged and ready I think this can be closed on the update of rust-lang/rust's stdarch submodule.

@Noratrieb Noratrieb added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-wasm Target: WASM (WebAssembly), http://webassembly.org/ C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 4, 2025
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 4, 2025
@Noratrieb Noratrieb removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 4, 2025
alexcrichton added a commit to alexcrichton/stdarch that referenced this issue Mar 10, 2025
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
github-merge-queue bot pushed a commit to rust-lang/stdarch that referenced this issue Mar 16, 2025
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
@hanna-kruppe
Copy link
Contributor

This can be closed now, the stdarch PR was merged and the submodule in this repository has been updated to include it. (Can be verified by checking source of the intrinsics in nightly docs, e.g., https://doc.rust-lang.org/nightly/core/arch/wasm/fn.i8x16_shl.html).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants