Stores into fixed-size slices are easy to misuse, lead to subtle bugs

[Shnatsel]

I've just run into a very subtle bug with SIMD stores in my own code that may also apply to this crate.

I was trying to make these two lines safe:

let out_data = core::mem::transmute::<*mut i16, *mut __m256i>(data.as_mut_ptr());
_mm256_storeu_si256(out_data, ymm3);

so I made a helper function to wrap it:

fn avx_store(input: __m256i, output: &mut [i16]) {
    unsafe { _mm256_storeu_si256(output.as_mut_ptr() as *mut __m256i, input) }
}

and the original code became

avx_store(ymm3, &mut data[0..16].try_into().unwrap());

And everything broke. I checked and double-checked and triple-checked and started wondering about a compiler bug because everything was so trivial and obviously correct.

Only with outside help I realized that my conversion to a fixed-size slice, &mut data[0..16].try_into().unwrap(), was creating an intermediate array instead of giving me a reference to the original slice. Then the SIMD store would write into that intermediate array and the data would never make it into the output.

Here's the full code if you'd like more context: vstroebel/jpeg-encoder#18

It seems that the function signatures in safe_unaligned_simd sidestep this problem for x86 by accepting dynamically sized arrays for store intrinsics, but it could still be a problem on ARM, e.g. https://docs.rs/safe_unaligned_simd/latest/aarch64-apple-darwin/safe_unaligned_simd/aarch64/fn.vst1_f32.html

[okaneco]

I would love to completely prevent this bug, but I'm not sure it's possible on our library end.

This is more of a Rust language/libs footgun.
Ideally there would be a clippy or compiler lint for a mutable reference to an implicit copy/temporary as that is almost always unintended behavior.

Initially, I did consider a fallible API that took slices because of this possibility. I ultimately decided not to pursue this path as I believe there are higher level logic bugs in your code if you're calling load or store into a slice that's too short. If someone does want that behavior, it seemed better suited to let them wrap that functionality themselves and provide it (this crate was intended to be used directly or as a building block for even higher level usage).

The path forward

Do you have any suggestions?

1. This issue does make me think it would be a good idea to add a warning/explanation section to the readme and lib.rs to warn about this and show proper usage. More people should be aware of this behavior to prevent future issues.

2. It might be worth filing a feature request report for a lint in clippy/rustc with the minimal version of your experience or in an existing issue for more attention on this.

---

I've been bitten by this bug several times while working on image code and the first time feels like a massive betrayal from the language.
Eventually I started to preventatively wrap mutable references in parens to be safe, or avoid any formulation which would cause that ambiguity to begin with.
This happened when working on the png filter auto-vectorization. That's why the code is written the way it is now.

// WRONG, this is a mistake and doesn't do anything because x is a temporary
let x = &mut chunk[..4].try_into().unwrap();
*x = new_chunk;
// Correct and does what we want, needs type annotation because inference fails
let x: &mut [u8; 4] = (&mut chunk[..4]).try_into().unwrap();
*x = new_chunk;

// We settled on this in `png`: https://docs.rs/png/0.18.0/src/png/filter.rs.html#306-316
// Correct, but complete eyesore that most people won't stumble into writing themselves
*TryInto::<&mut [u8; 4]>::try_into(chunk).unwrap() = new_chunk;

I commented on this function's tracking issue 3 weeks ago and now it's in the process of being stabilized. This conversion would save us from writing the TryInto with type annotation, along with avoiding the bug you ran into.
https://doc.rust-lang.org/1.90.0/std/primitive.slice.html#method.as_mut_array

*chunk[..2].as_mut_array().unwrap() = new_chunk;
// Or this, which avoids the copy coercion/lifetime extension
let x = chunk[..2].as_mut_array().unwrap();
*x = new_chunk;

[okaneco]

I wrote some documentation to hopefully help someone avoid this in the future
#34

[Shnatsel]

Now that this is documented and as_chunks is available in std, there's not much else we can do here.

There should be lints for it, but that's for clippy and/or the compiler issue tracker.

Thanks again for documenting it so clearly!

[Shnatsel]

I've opened a feature request for Clippy to flag this pattern: rust-lang/rust-clippy#16507