Re: [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T>
From: Alice Ryhl
Date: Sat Jul 29 2023 - 04:13:29 EST
I suggested it because it seemed less fragile.
That said, after seeing what #[derive(Eq)] expands to, I'm not so sure.
I'd probably investigate why the Eq macro has to expand to what it does.
On 7/29/23 06:11, Benno Lossin wrote:
On 25.07.23 13:57, Alice Ryhl wrote:
Benno Lossin <benno.lossin@xxxxxxxxx> writes:
On 20.07.23 15:34, Alice Ryhl wrote:
Benno Lossin <benno.lossin@xxxxxxxxx> 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@xxxxxxxxx>
---
///
/// 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)?