Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
bit pattern for that type.
Signed-off-by: Benno Lossin <[email protected]>
---
rust/kernel/types.rs | 2 ++
1 file changed, 2 insertions(+)
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index d849e1979ac7..f0f540578d0f 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -11,6 +11,7 @@
ops::{Deref, DerefMut},
ptr::NonNull,
};
+use macros::Zeroable;
/// Used to transfer ownership to and from foreign (non-Rust) languages.
///
@@ -251,6 +252,7 @@ fn drop(&mut self) {
///
/// 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,
--
2.41.0
Benno Lossin <[email protected]> writes:
> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
> bit pattern for that type.
>
> Signed-off-by: Benno Lossin <[email protected]>
> ---
> ///
> /// 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.
I suggest you instead add Opaque to the `impl_zeroable!` macro in
`rust/kernel/init.rs`.
Alice
On 7/19/23 11:21, Benno Lossin wrote:
> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
> bit pattern for that type.
>
> Signed-off-by: Benno Lossin <[email protected]>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
On 20.07.23 15:34, Alice Ryhl wrote:
> Benno Lossin <[email protected]> writes:
>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
>> bit pattern for that type.
>>
>> Signed-off-by: Benno Lossin <[email protected]>
>> ---
>> ///
>> /// 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,
}
```
> I suggest you instead add Opaque to the `impl_zeroable!` macro in
> `rust/kernel/init.rs`.
We would have to do this when using the alternative approach from
above, since we do not want the `Zeroable` bound on `T` for `Opaque`.
I wanted to avoid the `SAFETY` comment, since that is needed for the
`impl_zeroable` macro (as it has an unsafe block inside).
--
Cheers,
Benno
Benno Lossin <[email protected]> writes:
> On 20.07.23 15:34, Alice Ryhl wrote:
>> Benno Lossin <[email protected]> writes:
>>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
>>> bit pattern for that type.
>>>
>>> Signed-off-by: Benno Lossin <[email protected]>
>>> ---
>>> ///
>>> /// 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);
}
};
```
>> I suggest you instead add Opaque to the `impl_zeroable!` macro in
>> `rust/kernel/init.rs`.
>
> We would have to do this when using the alternative approach from
> above, since we do not want the `Zeroable` bound on `T` for `Opaque`.
> I wanted to avoid the `SAFETY` comment, since that is needed for the
> `impl_zeroable` macro (as it has an unsafe block inside).
Indeed, if we expand the derive macro in the standard way, then it
doesn't work for `Opaque` due to the extra unnecessary bound.
Alice
On 25.07.23 13:57, Alice Ryhl wrote:
> Benno Lossin <[email protected]> writes:
>> On 20.07.23 15:34, Alice Ryhl wrote:
>>> Benno Lossin <[email protected]> writes:
>>>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
>>>> bit pattern for that type.
>>>>
>>>> Signed-off-by: Benno Lossin <[email protected]>
>>>> ---
>>>> ///
>>>> /// 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
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 <[email protected]> writes:
>>> On 20.07.23 15:34, Alice Ryhl wrote:
>>>> Benno Lossin <[email protected]> writes:
>>>>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
>>>>> bit pattern for that type.
>>>>>
>>>>> Signed-off-by: Benno Lossin <[email protected]>
>>>>> ---
>>>>> ///
>>>>> /// 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)?
>