Rust: simd_insert and simd_extract allow garbage data

[workingjubilee]

It appears that with simd_insert and simd_extract that I can produce garbage data in a way that is probably due to unsound OOB memory access. These are unsafe functions but the related simd_shuffle functions fail to monomorphize. Miri provokes an ICE. I thiiiink simd_extract and simd_insert might not require const arguments on purpose, but I believe something may need to be patched re: Miri. cc @RalfJung

I was in the middle of constructing tests for rustc's simd intrinsics. I tried this code: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=dfc24d97ffa77e6fbd4a65c16b713cf9

#![allow(non_camel_case_types)]
#![feature(repr_simd, platform_intrinsics)]

#[repr(simd)]
#[derive(Copy, Clone, Debug)]
struct f32x4(f32, f32, f32, f32);

extern "platform-intrinsic" {
    pub fn simd_insert<T, E>(x: T, idx: u32, y: E) -> T;
    pub fn simd_extract<T, E>(x: T, idx: u32) -> E;
}

fn main() {
    let x = f32x4(-1.0, 0.0, f32::INFINITY, f32::NAN);
    unsafe {
        let ins: f32x4 = simd_insert(x, 5, f32::NEG_INFINITY);
        let ext: f32 = simd_extract(x, 9);
        println!("{:?}", x);   // f32x4(-1.0, 0.0, inf, NaN)
        println!("{:?}", ins); // f32x4(0.000000000000000000000000000000000000000000001, 0.0,
                               // 12499248000000000.0, 0.000000000000000000000000000000000000000045915)
        println!("{}", ext);   // 0.000000000000000000000000000000000000000030658
    }
}

I (perhaps overly naively) expected to see this happen: "failure to monomorphize because blah blah blah"
Instead, this happened: I got some totally wild garbage data!

rustc --version:

rustc 1.48.0-nightly (ef663a8a4 2020-09-30) running on x86_64-unknown-linux-gnu

The Miri ICE:

thread 'rustc' panicked at 'Index `5` must be in bounds of vector type `f32`: `[0, 4)`', /rustc/ef663a8a48ea6b98b43cbfaefd99316b36b16825/compiler/rustc_mir/src/interpret/intrinsics.rs:393:17

[lzutao]

@rustbot modify labels: requires-nightly

[RalfJung]

Yeah the Miri ICE should definitely be fixed.

I guess the question is whether these intrinsics should fail to monomorphize, or whether using them with OOB indices is UB. I assume the latter, but it would be good to get someone to confirm... who would know about SIMD stuff?

Also, this UB should be added to the docs for those intrinsics, probably in stdarch.

[bjorn3]

simd_shuffle checks if the indices are in bound: https://github.com/rust-lang/rust/blob/6f56fbdc1c58992a9db630f5cd2ba9882d32e84b/compiler/rustc_codegen_llvm/src/intrinsic.rs#L882-L902

The simd_insert and simd_extract intrinsics are codegened at https://github.com/rust-lang/rust/blob/6f56fbdc1c58992a9db630f5cd2ba9882d32e84b/compiler/rustc_codegen_llvm/src/intrinsic.rs#L915-L938

[bjorn3]

@rustbot modify labels: +A-simd +A-codegen +T-compiler

[RalfJung]

The insert and extract intrinsics do not even have ´rustc_args_required_cons`, so they cannot be checked at monmorphization time. Thus UB is likely the only option.

[bjorn3]

That is just an oversight in stdarch. All users of it are constant. (either fixed or using the constify family of macros)

[RalfJung]

Well, but codegen does not seem to rely on them being constants either, so why would we require that?

[bjorn3]

While LLVM allows variable indexes, it will generate way more efficient code when indexes are known at compile time. https://godbolt.org/z/zvorEa Other codegen backends, like cg_clif, may also not allow variable indexes.

[RalfJung]

Sure, having more efficient code when some things are statically known is expected.

Other codegen backends, like cg_clif, may also not allow variable indexes.

I guess this is a question for one or several of the Rust teams then, whether it is reasonable to restrict these intrinsics to compile-time known indices even though supporting run-time indices is possible (and doesn't seem too hard, judging from what LLVM generates).

We can either

• add rustc_args_required_cons as well as post-monomorphization bounds checks (similar to the shuffle intrinsics), or
• fix docs + Miri to account for OOB indices being UB.

Cc @rust-lang/project-portable-simd -- this is not really about portable SIMD but hopefully still reaches the right people.

[bjorn3]

add rustc_args_required_cons as well as post-monomorphization bounds checks (similar to the shuffle intrinsics), or

rustc_args_required_const is only applied to the extern "platform-intrinsic" definition. It doesn't require any change in the compiler. This means that it is perfectly fine for stdarch to use it and stdsimd to not use it for example. If we do choose to always require it to be const in the compiler itself, it would be possible to change the post-monomorphization error to an error in the intrinsic checker.

[RalfJung]

It doesn't require changes to the compiler but, AFAIK, it is only usually added when the compiler requires these constants, and actively exploits that for type checking and/or codegen.

[nagisa]

I don't see a problem with restricting the intrinsics to constant indices _now_ and implementing the necessary code to verify the indices are in bounds. GCC for example has a similar restriction. Once there's an actual known use-case for non-constant indices in these operations we could consider relaxing the operation (while also implementing bound checking similar to one we do when indexing into slices today).

[RalfJung]

@jyn514 why did you mark this as I-unsound? Many intrinsics are unsafe to use, that does not make them unsound.

@nagisa

Once there's an actual known use-case for non-constant indices in these operations we could consider relaxing the operation (while also implementing bound checking similar to one we do when indexing into slices today).

I'd expect the intrinsic to be unchecked, and OOB indexing to be UB -- that is also the case, on the MIR level, with slice indexing today. Bounds checks are added during MIR construction.

[jyn514]

Sorry, I saw 'unsound' in the message description and wasn't thinking.

[bjorn3]

None of the platform-intrinsics are fundamentally unsafe to use. Safe intrinsics just didn't exist when they were introduced. I think the original plan was even to directly expose all platform-intrinsics to the user. There are several tests that invalid usage of them give nice compilation errors.

[camelid]

Assigning P-medium and removing I-prioritize as discussed in the prioritization working group.

[RalfJung]

None of the platform-intrinsics are fundamentally unsafe to use. Safe intrinsics just didn't exist when they were introduced. I think the original plan was even to directly expose all platform-intrinsics to the user. There are several tests that invalid usage of them give nice compilation errors.

Not for these two though it seems...

And indeed I don't think there is precedent for having such an intrinsic be checked. Instead, what we usually do is expose a safe function to the user which first does the check and then calls the unsafe intrinsic.

AFAIK none of the other SIMD intrinsics have any reasonable chance of causing UB (they all just operate on pure values), except for the shuffle intrinsics -- which however require statically known indices to even perform code generation. So that does not tell us anything about the intended behavior of simd_insert and sim_extract. Or are there other intrinsics that could cause UB but have some checks applied to them to avoid that?

[workingjubilee]

I think this is Rust unsoundness and not just unsound calling code if the intent is that it is not supposed to break in this particular manner.

Cc @rust-lang/project-portable-simd -- this is not rally about portable SIMD but hopefully still reaches the right people.

:telephone_receiver: Hello!
Did you know Arm assigns very different meanings in terms of operations to "insert" and "extract" as concepts? Arm names these intrinsics as "vset" and "vget", with "vext" being more like a shuffle or interleaving operation, but the LLVM intrinsics are based on the Intel conceptualization (which Arm does give an honorable mention to in their documentation), so I actually took a long moment to figure out which one was in use because I had been reading about Arm for the entire past ~2 weeks, and frankly Neon makes more sense to me than SSE, so far.

Right, where were we? I believe simd_extract and simd_insert as implemented by the Rust compiler are intended to mimic simd_shuffleN in all regards on this matter, because the intrinsics that these are expected to compile to do require constants, and so it is unexpected behavior to compile to the dynamic extraction.

[RalfJung]

Well, looks like the experts agree they should be constant. Fine for me, I was just trying to help explore the design space. :)

So looks like the fix here is to add rustc_args_required_const to these functions and add compile-time checks similar to the shuffle intrinsics? (I am using the names rustc uses here as I know basically nothing about the assembly instructions these compile to.)

[programmerjake]

I'll note that LLVM's language reference states that the element index is specifically allowed to be a variable:
https://llvm.org/docs/LangRef.html#extractelement-instruction

The first operand of an ‘extractelement’ instruction is a value of vector type. The second operand is an index indicating the position from which to extract the element. The index may be a variable of any integer type.

[Lokathor]

Yeah, likely the codegen will just be different in the case of a variable index is all.

[RalfJung]

https://github.com/rust-lang/rust/issues/70271 is somewhat related.