Derive support for pod structs with enums in them? about bytemuck HOT 10 OPEN

Hey @fu5ha The docs of the CheckedBitPattern derive claim that it's a subtrait of NoUninit, but the code doesn't actually... have that bound. Didn't you add a lot of that stuff?

I think this is just a docs issue for the derive that didn't get updated as the design evolved throughout the PR.

@Vrixyz the short answer is that bytes_of does indeed require NoUninit -- CheckedBitPattern is irrelevant here, because if we already have a safe value that we can view the bytes_of, then that value must be valid, therefore it must have a valid bit pattern, and since bytes_of takes an immutable ref, we can't modify that bit pattern so there's no safety or validity concern there. However, bytes_of does create a reference to all the bytes representing the backing value, and thus in order to satisfy validity constraints, all of those bytes must be initialized, which is not generizably-true for enums with fields.

I don't believe it's worth it to implement support in the derive macro for detecting cases where this is safe, because they're incredibly specific and checking them sufficiently is probably very hard. However, we could/should add this to docs somewhere. This is only possible because rust's fielded #[repr(C)] enums are well-defined as being c-style tagged-enums with an integer discriminant followed by a union of variant field-types

Therefore should be safe to implement NoUninit for an enum with fields if and only if:

• The enum is marked #[repr(C)]
• Every variant's field is the same size in bytes
• Every variant's field is itself NoUninit (i.e. has no padding bytes)

The best way to guarantee the second two would be to make every variant of the enum contain a single tuple-struct. Each variant's field struct could then derive NoUninit and CheckedBitPattern. However, you still have the problem of making each of these variant structs have the same size.

Internally at embark we do a similar thing but making our own tagged unions with raw tag and union, and I developed a helper struct for this purpose, which looks like the following:

/// This type adds some `const PAD` number of "explicit" or "manual" padding
/// bytes to the end of a struct.
///
/// This is useful to make a type not have *real* padding bytes,
/// and therefore be able to be marked as [`bytemuck::NoUninit`]. Specifically,
/// it's used in the `ffi_union` macro to equalize the size of all
/// fields of a union and therefore remove any "real" padding bytes from the union, making
/// it safe to store in WASM memory and pass through the ark module host memory utility functions.
/// It may also be useful in other places.
#[derive(Copy, Clone)]
#[repr(C)]
pub struct TransparentPad<T, const PAD: usize>(pub T, [u8; PAD]);

// SAFETY: Since `[u8; N]` is always Zeroable, this is safe
unsafe impl<T: Zeroable, const PAD: usize> Zeroable for TransparentPad<T, PAD> {}

// SAFETY: Since `[u8; N]` is always AnyBitPattern, this is safe
unsafe impl<T: AnyBitPattern, const PAD: usize> AnyBitPattern for TransparentPad<T, PAD> {}

#[doc(hidden)] // it is only for us in this crate
#[macro_export]
macro_rules! impl_checked_bit_pattern_for_transparent_pad {
    ($inner:ident) => {
        // SAFETY: The extra padding is always AnyBitPattern, so implies CheckedBitPattern, and this just passes
        // down the necessary safety checks to the inner type.
        unsafe impl<const PAD: usize> bytemuck::CheckedBitPattern
            for $crate::TransparentPad<$inner, PAD>
        {
            type Bits = $crate::TransparentPad<<$inner as bytemuck::CheckedBitPattern>::Bits, PAD>;

            fn is_valid_bit_pattern(bits: &Self::Bits) -> bool {
                <$inner as bytemuck::CheckedBitPattern>::is_valid_bit_pattern(&bits.0)
            }
        }
    };
}

impl<T, const PAD: usize> TransparentPad<T, PAD> {
    pub fn new(inner: T) -> Self {
        Self(inner, [0u8; PAD])
    }
}

impl<T, const PAD: usize> AsRef<T> for TransparentPad<T, PAD> {
    fn as_ref(&self) -> &T {
        &self.0
    }
}

impl<T, const PAD: usize> AsMut<T> for TransparentPad<T, PAD> {
    fn as_mut(&mut self) -> &mut T {
        &mut self.0
    }
}

impl<T, const PAD: usize> core::ops::Deref for TransparentPad<T, PAD> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl<T, const PAD: usize> core::ops::DerefMut for TransparentPad<T, PAD> {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0
    }
}

We use the mentioned ffi_union macro to then auto-wrap the union's fields as TransparentPad<VariantStruct, AUTO_DERIVED_PADDING> so that every field has the same size and explicit padding bytes. You could do the same, though it would probably be easier to just do it manually for a single occurrence. Unfortunately, doing so is definitely a bit error-prone and manual, so care would need to be taken. You'd just need to figure out the proper number of padding bytes for each field to add so that they all have the same size as whatever the largest variant is.

from bytemuck.

Ah, good point! Yeah, if it auto assigns a u8 discriminant and your payload is align(>1) then you'll get padding between discriminant and payload. So, you need to make sure align(discriminant) >= align(payload).

from bytemuck.

Is this intended as the restriction of why enums are not supported also

Yes.

Here's how things could go wrong:

enum MyEnum {
  Whatever = 0,
  AnotherTag = 1,
}
// this is unsound
unsafe impl Pod for MyEnum {}

let mut my_enum = MyEnum::Whatever;
let x: &mut u8 = cast_mut(&mut my_enum);
*x = u8::MAX; // now `my_enum` holds an illegal bit pattern
match my_enum {
  // this match expression is gonna go sideways
}

Still, many enums can correctly be Zeroable.

from bytemuck.

As a side note: your TestWithEnum struct will actually have a padding byte as the 6th byte, which I hope the Pod derive would detect and crash on, even if it did otherwise accept the enum.

from bytemuck.

Almost sounds like it might make sense to have a read only variant of Pod (maybe called NoPadding ?) that enums, char, bool and co. could implement.

from bytemuck.

You're not the first to propose such a thing.

When I was first developing bytemuck I asked in the Discord and everyone considered the ability to convert &T to &[u8] a "not very useful" thing on its own, so I didn't make a separate trait for it.

At this point, it would be a breaking change to insert into the existing Pod: Zeroable trait tree, but just as a totally separate trait would be fine. We could even have it blanket impl for all Pod types.

However, no one has cared enough about it to write the PR. NoPadding would probably be a good name.

from bytemuck.

The description of this issue is starting to get obsolete, as current state has improved a lot since its creation. The PR mentioned above #91 then #94 creating NoUninit, AnyBitPattern, and CheckedBitPattern ; included in release 1.9.0.

Coming from gschup/ggrs#40 ; we're "mucking" player input, and it could be a good ergonomic boost to muck that as an enum.

My current blocker as noted on gschup/ggrs#41 (comment) ; is bytes_of (https://docs.rs/bytemuck/latest/bytemuck/fn.bytes_of.html) ; which has a type restriction on NoUninit. I feel like this type restriction could be relaxed to CheckedBitPattern ? But as NoUninit is full of unsafety warnings, I wanted to start the discussion with you before diving in.

from bytemuck.

Hey @fu5ha The docs of the CheckedBitPattern derive claim that it's a subtrait of NoUninit, but the code doesn't actually... have that bound. Didn't you add a lot of that stuff?

from bytemuck.

Actually now that I think about it, if this is something that has real demand, it might be possible to port the functionality of the ffi_union macro we have to handle this for rerp-c enums under defined circumstances (i.e. the ones defined above)

from bytemuck.

Thank you for chiming in ! Very enlightening information 😄

I made progress on my understanding, and I documented my findings there https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=b1e536d5e1a25b4719390dc14a27168a.

Summary: I think your described approach can work 👍, BUT Repr(C) discriminant can get padding applied 😱, leading to uninitialized memory, and that's undefined behaviour from what I understand 🙅.

So the minor correction to your approach would be that the enum has to be marked #[repr(u*)] (where * is adapted depending on final alignment).

from bytemuck.