write_to_prefix generates panic path

[jettr]

The write_to_prefix function is generating a panic path in our code, and I am pretty sure it is the bytes[..size].copy_from_slice(self.as_bytes()); line. The compiler doesn't have enough information to elide the slice comparison check.

If the code was written like the following, the compiler should not generate a panic path

    fn write_to_prefix<B: ByteSliceMut>(&self, mut bytes: B) -> Option<()> {
        let source = self.as_bytes();
        if bytes.len() < source.len() {
            return None;
        }

        bytes[..size].copy_from_slice(source);
        Some(())
    }

[joshlf]

What tool(s) are you using to inspect the output? Is it something I could run myself?

If you're able to, could you see whether this version of the code generates a panic path? It's similar to what you wrote, but keeps the same structure of the existing code:

    fn write_to_prefix<B: ByteSliceMut>(&self, mut bytes: B) -> Option<()> {
        let self_bytes = self.as_bytes();
        bytes.get_mut(..self_bytes.len())?.copy_from_slice(self_bytes);
        Some(())
    }

[djkoloski]

There are also some clippy lints that we can turn on to help root out these kinds of silent panic paths:

• clippy::indexing_slicing can detect uses of indexing and slicing which can panic at runtime
• clippy::arithmetic_side_effects forces careful arithmetic operations to avoid panicking

These are extremely pedantic lints, so it's not something we can address in the near term. We can root out panics on demand when they're affecting users.

[joshlf]

I minimized this example and ran it through Godbolt, and found that the panic is optimized out. My guess is that your issue was that the compiler ran out of optimization budget since you're presumably compiling more code than this toy example.

I filed #202 so that we can hopefully handle these cases in a more principled way than to just experiment to find out whether, in practice, panics are being optimized out.

[jettr]

What tool(s) are you using to inspect the output? Is it something I could run myself?

I was manually looking at the LST file for our binary trying to track down all of our panic paths when I discovered this. I think godbolt is a very good alternative to test out potential panic paths though. The example from the second comment should not contain any panic paths either.

[jettr]

I think I just found another panic path in LayoutVerified::new_from_prefix function centered around split_at. I don't think every time we use LayoutVerified::new_from_prefix in the code it causes a panic path, but it does some of the time. It seems that the optimizer doesn't always know that bytes.split_at(mem::size_of::<T>()); is within bounds.

I have tried to think of a solutions, and the one I keep coming back to is making ByteSlice defines its own fn split_at(usize) -> Option<(Self, Self> that returns an Option instead of just a tuple. Then you could re-write the new_from_prefix as follows to eliminate the panic path:

    pub fn new_from_prefix(bytes: B) -> Option<(LayoutVerified<B, T>, B)> {
        if !aligned_to(bytes.deref(), mem::align_of::<T>()) {
            return None;
        }
        let (bytes, suffix) = bytes.split_at(mem::size_of::<T>())?;
        Some((LayoutVerified(bytes, PhantomData), suffix))
    }

Moving the size check into the ByteSlice::split_at should always give the optimizer enough information to optimize out the panic path.

Feel free to go with another implementation, I just wanted to offer at least one possible option to remove the panics centered around split_at use.

[AaronKutch]

ptr::copy_nonoverlapping could be used directly for slice copying, and split_at_unchecked is newly stabilized or manual (self.get_unchecked(..mid), self.get_unchecked(mid..)) could be used (unless you want to avoid unsafe, but isn't this a fundamental well defined function?). edit: or maybe this is already being done according to a linked issue

[jswrenn]

I have tried to think of a solutions, and the one I keep coming back to is making ByteSlice defines its own fn split_at(usize) -> Option<(Self, Self> that returns an Option instead of just a tuple.

Happy to say we went this route in #1071!

ptr::copy_nonoverlapping could be used directly for slice copying

Excellent suggestion. I'll look into whether we can do this.