Messages in this thread | | | Date | Sat, 29 Jul 2023 04:11:14 +0000 | From | Benno Lossin <> | Subject | Re: [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T> |
| |
On 25.07.23 13:57, Alice Ryhl wrote: > Benno Lossin <benno.lossin@proton.me> writes: >> On 20.07.23 15:34, Alice Ryhl wrote: >>> Benno Lossin <benno.lossin@proton.me> writes: >>>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid >>>> bit pattern for that type. >>>> >>>> Signed-off-by: Benno Lossin <benno.lossin@proton.me> >>>> --- >>>> /// >>>> /// This is meant to be used with FFI objects that are never interpreted by Rust code. >>>> #[repr(transparent)] >>>> +#[derive(Zeroable)] >>>> pub struct Opaque<T> { >>>> value: UnsafeCell<MaybeUninit<T>>, >>>> _pin: PhantomPinned, >>>> } >>> >>> Does this actually work? I don't think we implement Zeroable for >>> UnsafeCell. >> >> Good catch, this does compile, but only because the current >> implementation of the derive expands to (modulo correct paths): >> ``` >> impl<T> Zeroable for Opaque<T> >> where >> UnsafeCell<MaybeUninit<T>>: Zeroable, >> PhantomPinned: Zeroable, >> {} >> ``` >> This implementation is of course useless, since `UnsafeCell` is never >> `Zeroable` at the moment. We could of course implement that and then this >> should work, but the question is if this is actually the desired output in >> general. I thought before that this would be a good idea, but I forgot that >> if the bounds are never satisfied it would silently compile. >> >> Do you think that we should have this expanded output instead? >> ``` >> impl<T: Zeroable> Zeroable for Foo<T> {} >> const _: () = { >> fn assert_zeroable<T: Zeroable>() {} >> fn ensure_zeroable<T: Zeroable>() { >> assert_zeroable::<Field1>(); >> assert_zeroable::<Field2>(); >> } >> }; >> ``` >> If the input was >> ``` >> #[derive(Zeroable)] >> struct Foo<T> { >> field1: Field1, >> field2: Field2, >> } >> ``` > > Yeah. The way that these macros usually expand is by adding `where T: > Zeroable` to the impl for each generic parameter, and failing > compilation if that is not enough to ensure that all of the fields are > `Zeroable`. > > You might want to consider this expansion instead: > ``` > impl<T: Zeroable> Zeroable for Foo<T> {} > const _: () = { > fn assert_zeroable<T: Zeroable>(arg: &T) {} > fn ensure_zeroable<T: Zeroable>(arg: &Foo<T>) { > assert_zeroable(&arg.field1); > assert_zeroable(&arg.field2); > } > }; > ```
Is there a specific reason you think that I should us references here instead of the expansion from above (where I just use the types and not the fields themselves)?
-- Cheers, Benno
| |