Add KnownLayout::MaybeUninit type by joshlf · Pull Request #1822 · google/zerocopy

joshlf · 2024-10-05T02:47:58Z

build.rs

joshlf · 2024-10-16T17:42:12Z

build.rs

        println!(
            "cargo:rustc-check-cfg=cfg(__ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS)"
        );
+        println!("cargo:rustc-check-cfg=cfg(zerocopy_unstable)");


Remove this now that we're targeting 0.9?

I kept this PR targeting 0.8, since — as a rule — I think it's easier to develop for 0.8 and remove MSRV cruft in the cherrypick to 0.9 — than it is to go the other way around. The zerocopy_unstable CFGs are nonetheless removed, since I've instead doc(hidden)'d the MaybeUninit machinery.

joshlf · 2024-10-16T17:43:11Z

src/lib.rs

+    /// - `Self` and `Self::MaybeUninit` have the same:
+    ///   - Fixed prefix size
+    ///   - Alignment
+    ///   - (For DSTs) trailing slice element size


You can make this more concise by saying that Self::LAYOUT and Self::MaybeUninit::LAYOUT are identical.

joshlf · 2024-10-16T17:44:09Z

src/lib.rs

+    /// - It is valid to perform an `as` cast from `*mut Self` to `*mut
+    ///   Self::MaybeUninit`, and this operation preserves referent size (ie,
+    ///   `size_of_val_raw`).
+    /// - All bytes of `Self::MaybeUninit` may be uninitialized.


Maybe "Self::MaybeUninit has no bit validity constraints - any byte sequence is permitted, including uninitialized bytes"? Importantly, it's not just that uninit is valid, but all other (init) bytes are valid as well.

joshlf · 2024-10-16T17:51:40Z

src/lib.rs

+    // SAFETY: `CoreMaybeUninit<T>` has the same size and alignment as `T`.
+    // Consequently, `[CoreMaybeUninit<T>]` has the same size and alignment as
+    // `[T]`, becuase `CoreMaybeUninit<T>` has the same size and alignment as
+    // `T` [1].


This wording feels redundant.

Also, two issues:

Can you cite the slice layout docs to prove that [T] and T have the same alignment?

I believe you need to be more careful about what "size" means here - it's actually a "for all values" property

What about pointer casts?

What about bit validity?

This may get easier to write if you just define safety in terms of LAYOUT as I suggested above.

joshlf · 2024-10-16T17:52:22Z

src/lib.rs

+    /// SAFETY:
+    /// - By invariant on `KnownLayout::MaybeUninit`, `T` and `T::MaybeUninit`
+    ///   have the same:
+    ///   - Fixed prefix size
+    ///   - Alignment
+    ///   - (For DSTs) trailing slice element size
+    /// - By invariant on `KnownLayout`, it is valid to perform an `as` cast
+    ///   from `*mut T` and `*mut T::MaybeUninit`, and this operation preserves
+    ///   referent size (ie, `size_of_val_raw`).


Make sure to update this if you update the safety requirements on type MaybeUninit

unsafe_impl_known_layout requires:

/// Implements `KnownLayout` for a type in terms of the implementation of /// another type with the same representation. /// /// # Safety /// /// - `$ty` and `$repr` must have the same: /// - Fixed prefix size /// - Alignment /// - (For DSTs) trailing slice element size /// - It must be valid to perform an `as` cast from `*mut $repr` to `*mut $ty`, /// and this operation must preserve referent size (ie, `size_of_val_raw`). macro_rules! unsafe_impl_known_layout {

Does that final bullet point always follow from $ty and $repr having the same KnownLayout::LAYOUT, or is there more to prove?

Yeah I think that follows, but you do need to update the comment to explicitly state that the ::LAYOUT equality is the premise you're using for this proof.

joshlf · 2024-10-16T18:04:51Z

src/wrappers.rs

+
+    /// Creates a `Box<MaybeUninit<T>>`.
+    ///
+    /// This function is useful for allocating large, uninit values on the heap,


Suggested change

/// This function is useful for allocating large, uninit values on the heap,

/// This function is useful for allocating large, uninit values on the heap

joshlf · 2024-10-16T18:18:26Z

src/wrappers.rs

+    /// never to cause a panic or an abort.
+    #[cfg(feature = "alloc")]
+    #[inline]
+    pub fn new_boxed_uninit(meta: T::PointerMetadata) -> Result<Box<Self>, AllocError> {


Can we refactor so this isn't duplicated from FromZeros::new_boxed?

joshlf · 2024-10-16T18:18:42Z

src/wrappers.rs

+    #[inline(always)]
+    pub const unsafe fn assume_init(self) -> T
+    where
+        T: Sized + KnownLayout<PointerMetadata = ()>,


Suggested change

T: Sized + KnownLayout<PointerMetadata = ()>,

T: Sized,

joshlf · 2024-10-16T18:20:02Z

src/wrappers.rs

+    /// The caller must ensure that `self` is in an initialized state. Depending
+    /// on subsequent use, it may also need to be in a valid state.


Let's be careful about bit validity vs safety here. It does need to be in a bit-valid state, but it is okay to not uphold (non-bit validity) safety invariants. Same in the safety comment in the body.

joshlf · 2024-10-16T18:20:26Z

tools/cargo-zerocopy/src/main.rs

-        "--cfg __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS --cfg zerocopy_derive_union_into_bytes "
+        "--cfg __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS --cfg zerocopy_derive_union_into_bytes --cfg zerocopy_unstable "
    } else {
-        "--cfg zerocopy_derive_union_into_bytes "
+        "--cfg zerocopy_derive_union_into_bytes --cfg zerocopy_unstable "


Obsolete; remove these

joshlf · 2024-10-16T18:35:13Z

zerocopy-derive/src/lib.rs

+        // The identifiers of the parameters without trait bounds or type defaults.
+        let param_idents = ast.generics.params.iter().map(|param| match param {
+            GenericParam::Type(ty) => {
+                let ident = &ty.ident;
+                quote!(#ident)
+            }
+            GenericParam::Lifetime(l) => {
+                let ident = &l.lifetime;
+                quote!(#ident)
+            }
+            GenericParam::Const(cnst) => {
+                let ident = &cnst.ident;
+                quote!({#ident})
+            }
+        });


This is duplicated from impl_block below just as a hack. We should refactor so we don't need to duplicate before merging.

joshlf · 2024-10-16T19:24:03Z

zerocopy-derive/src/lib.rs

+    let outer_extras = if let Some(repr) = known_layout_maybe_uninit_struct_repr {
+        // PANICS: TODO
+        let (trailing_field, leading_fields) = fields.split_last().unwrap();
+        let (_name, trailing_field_ty) = trailing_field;
+        let leading_fields_tys = leading_fields.iter().map(|(_name, ty)| ty);
+        let params = params.clone();
+        let bounds = bounds.clone();
+        quote! {
+            #repr
+            #[doc(hidden)]
+            pub struct __ZerocopyKnownLayoutMaybeUninit<#(#params),*>(
+                #(
+                    ::zerocopy::util::macro_util::core_reexport::mem::MaybeUninit<#leading_fields_tys>,
+                )*
+                <#trailing_field_ty as ::zerocopy::KnownLayout>::MaybeUninit
+            ) where #(#bounds,)*;
+        }
+    } else {
+        quote! {}
+    };


This is all a hack - this should live in fn derive_known_layout. It repeats a lot of the logic from that function.

joshlf · 2024-10-18T12:55:15Z

src/wrappers.rs

+    pub fn new(val: T) -> Self
+    where
+        T: Sized + KnownLayout<PointerMetadata = ()>,
+        Self: Sized,


Before we merge this, let's try to make these bounds unnecessary or track somewhere that we want to eventually get rid of them. This will make it painful to use in a generic context.

This is achieved by adding a `MaybeUninit` associated type to `KnownLayout`, whose layout is identical to `Self` except that it admits uninitialized bytes in all positions. For sized types, this is bound to `mem::MaybeUninit<Self>`. For potentially unsized structs, we synthesize a doppelganger with the same `repr`, whose leading fields are wrapped in `mem::MaybeUninit` and whose trailing field is the `MaybeUninit` associated type of struct's original trailing field type. This type-level recursion bottoms out at `[T]`, whose `MaybeUninit` associated type is bound to `[mem::MaybeUninit<T>]`. Makes progress towards #1797

joshlf requested a review from jswrenn October 5, 2024 02:47

joshlf force-pushed the maybe-uninit branch from 570c312 to 278f482 Compare October 5, 2024 03:09

joshlf commented Oct 5, 2024

View reviewed changes

build.rs Outdated Show resolved Hide resolved

joshlf mentioned this pull request Oct 5, 2024

Support MaybeUninit<T: ?Sized> via KnownLayout #1797

Open

jswrenn force-pushed the maybe-uninit branch 4 times, most recently from d5c2b36 to 55749cb Compare October 11, 2024 17:27

jswrenn force-pushed the maybe-uninit branch from 55749cb to 26b510a Compare October 15, 2024 15:18

joshlf commented Oct 16, 2024

View reviewed changes

joshlf commented Oct 18, 2024

View reviewed changes

jswrenn force-pushed the maybe-uninit branch 3 times, most recently from d94e5f7 to cbae3d7 Compare November 7, 2024 16:31

jswrenn changed the base branch from main to v0.8.x November 7, 2024 16:37

jswrenn force-pushed the maybe-uninit branch 11 times, most recently from dfa7e1e to e771b26 Compare November 13, 2024 19:34

jswrenn force-pushed the maybe-uninit branch from e771b26 to 1f25059 Compare November 13, 2024 20:23

jswrenn closed this Nov 13, 2024

	/// This function is useful for allocating large, uninit values on the heap,
	/// This function is useful for allocating large, uninit values on the heap

		/// The caller must ensure that `self` is in an initialized state. Depending
		/// on subsequent use, it may also need to be in a valid state.

Comments

Conversation

joshlf commented Oct 5, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants