[derive] Fix handling of repr(packed) (google#1781)

While we're here, improve error messages and add tests for deriving
`IntoBytes` on unions.

Closes google#1763

Author: joshlf

src/lib.rs

@@ -6268,7 +6268,7 @@ mod tests {
         assert_impl_all!(Foo<u8>: Unaligned);
 
         #[derive(IntoBytes, FromBytes, Unaligned)]
-        #[repr(packed)]
+        #[repr(C, packed)]
         #[allow(dead_code)] // We never construct this type
         struct Bar<T, U> {
             _t: T,

zerocopy-derive/src/lib.rs

@@ -753,22 +753,21 @@ fn derive_into_bytes_struct(ast: &DeriveInput, strct: &DataStruct) -> Result<Tok
     let is_packed_1 = repr.is_packed_1();
     let num_fields = strct.fields().len();
 
-    let (padding_check, require_unaligned_fields) = if is_transparent || is_packed_1 {
+    let (padding_check, require_unaligned_fields) = if is_transparent || (is_c && is_packed_1) {
         // No padding check needed.
         // - repr(transparent): The layout and ABI of the whole struct is the
         //   same as its only non-ZST field (meaning there's no padding outside
         //   of that field) and we require that field to be `IntoBytes` (meaning
         //   there's no padding in that field).
-        // - repr(packed): Any inter-field padding bytes are removed, meaning
+        // - repr(C, packed): Any inter-field padding bytes are removed, meaning
         //   that any padding bytes would need to come from the fields, all of
         //   which we require to be `IntoBytes` (meaning they don't have any
         //   padding).
         (None, false)
-    } else if is_c && num_fields <= 1 {
+    } else if is_c && !repr.is_align_gt_1() && num_fields <= 1 {
         // No padding check needed. A repr(C) struct with zero or one field has
-        // no padding.
-        //
-        // TODO(#1763): This is probably unsound! Fix it.
+        // no padding unless #[repr(align)] explicitly adds padding, which we
+        // check for in this branch's condition.
         (None, false)
     } else if ast.generics.params.is_empty() {
         // Since there are no generics, we can emit a padding check. This is
@@ -777,16 +776,15 @@ fn derive_into_bytes_struct(ast: &DeriveInput, strct: &DataStruct) -> Result<Tok
         //
         // TODO(#1764): This is probably unsound! Fix it.
         (Some(PaddingCheck::Struct), false)
-    } else if is_c {
+    } else if is_c && !repr.is_align_gt_1() {
         // We can't use a padding check since there are generic type arguments.
         // Instead, we require all field types to implement `Unaligned`. This
         // ensures that the `repr(C)` layout algorithm will not insert any
-        // padding.
+        // padding unless #[repr(align)] explicitly adds padding, which we check
+        // for in this branch's condition.
         //
         // TODO(#10): Support type parameters for non-transparent, non-packed
         // structs without requiring `Unaligned`.
-        //
-        // TODO(#1763): This is probably unsound! Fix it.
         (None, true)
     } else {
         return Err(Error::new(Span::call_site(), "must have a non-align #[repr(...)] attribute or #[repr(packed)] in order to guarantee this type's memory layout"));
@@ -842,9 +840,16 @@ fn derive_into_bytes_union(ast: &DeriveInput, unn: &DataUnion) -> Result<TokenSt
         return Err(Error::new(Span::call_site(), "unsupported on types with type parameters"));
     }
 
+    // Because we don't support generics, we don't need to worry about
+    // special-casing different reprs. So long as there is *some* repr which
+    // guarantees the layout, our `PaddingCheck::Union` guarantees that there is
+    // no padding.
     let repr = StructUnionRepr::from_attrs(&ast.attrs)?;
     if !repr.is_c() && !repr.is_transparent() && !repr.is_packed_1() {
-        return Err(Error::new(Span::call_site(), "must have a non-align #[repr(...)] attribute in order to guarantee this type's memory layout"));
+        return Err(Error::new(
+            Span::call_site(),
+            "must be #[repr(C)], #[repr(packed)], or #[repr(transparent)]",
+        ));
     }
 
     Ok(impl_block(

zerocopy-derive/src/repr.rs

@@ -132,9 +132,11 @@ impl<Prim, Packed> Repr<Prim, Packed> {
             None
         }
     }
-}
 
-impl<Prim, Packed> Repr<Prim, Packed> {
+    pub(crate) fn is_align_gt_1(&self) -> bool {
+        self.get_align().map(|n| n.t.get() > 1).unwrap_or(false)
+    }
+
     /// When deriving `Unaligned`, validate that the decorated type has no
     /// `#[repr(align(N))]` attribute where `N > 1`. If no such attribute exists
     /// (including if `N == 1`), this returns `Ok(())`, and otherwise it returns

zerocopy-derive/tests/struct_to_bytes.rs

@@ -102,37 +102,6 @@ struct CPackedGeneric<T, U: ?imp::Sized> {
 util_assert_impl_all!(CPackedGeneric<u8, util::AU16>: imp::IntoBytes);
 util_assert_impl_all!(CPackedGeneric<u8, [util::AU16]>: imp::IntoBytes);
 
-#[derive(imp::IntoBytes)]
-#[repr(packed)]
-struct Packed {
-    a: u8,
-    // NOTE: The `u16` type is not guaranteed to have alignment 2, although it
-    // does on many platforms. However, to fix this would require a custom type
-    // with a `#[repr(align(2))]` attribute, and `#[repr(packed)]` types are not
-    // allowed to transitively contain `#[repr(align(...))]` types. Thus, we
-    // have no choice but to use `u16` here. Luckily, these tests run in CI on
-    // platforms on which `u16` has alignment 2, so this isn't that big of a
-    // deal.
-    b: u16,
-}
-
-util_assert_impl_all!(Packed: imp::IntoBytes);
-
-#[derive(imp::IntoBytes)]
-#[repr(packed)]
-struct PackedGeneric<T, U: ?imp::Sized> {
-    t: T,
-    // Unsized types stored in `repr(packed)` structs must not be dropped
-    // because dropping them in-place might be unsound depending on the
-    // alignment of the outer struct. Sized types can be dropped by first being
-    // moved to an aligned stack variable, but this isn't possible with unsized
-    // types.
-    u: imp::ManuallyDrop<U>,
-}
-
-util_assert_impl_all!(PackedGeneric<u8, util::AU16>: imp::IntoBytes);
-util_assert_impl_all!(PackedGeneric<u8, [util::AU16]>: imp::IntoBytes);
-
 #[derive(imp::IntoBytes)]
 #[repr(C)]
 struct ReprCGenericOneField<T: ?imp::Sized> {
@@ -188,7 +157,7 @@ util_assert_impl_all!(WithParams<'static, 'static, u8, 42>: imp::IntoBytes);
 pub struct IndexEntryFlags(u8);
 
 #[derive(imp::IntoBytes)]
-#[repr(packed)]
+#[repr(C, packed)]
 pub struct IndexEntry<const SIZE_BLOCK_ID: usize> {
     block_number: imp::native_endian::U64,
     flags: IndexEntryFlags,

zerocopy-derive/tests/ui-msrv/msrv_specific.rs

@@ -0,0 +1,37 @@
+// Copyright 2024 The Fuchsia Authors
+//
+// Licensed under a BSD-style license <LICENSE-BSD>, Apache License, Version 2.0
+// <LICENSE-APACHE or https://www.apache.org/licenses/LICENSE-2.0>, or the MIT
+// license <LICENSE-MIT or https://opensource.org/licenses/MIT>, at your option.
+// This file may not be copied, modified, or distributed except according to
+// those terms.
+
+// This file contains tests which trigger errors on MSRV during a different
+// compiler pass compared to the stable or nightly toolchains.
+
+#[macro_use]
+extern crate zerocopy;
+
+#[path = "../include.rs"]
+mod util;
+
+use self::util::util::AU16;
+use zerocopy::IntoBytes;
+
+fn main() {}
+
+// `repr(C, packed(2))` is not equivalent to `repr(C, packed)`.
+#[derive(IntoBytes)]
+#[repr(C, packed(2))]
+struct IntoBytes1<T> {
+    t0: T,
+    // Add a second field to avoid triggering the "repr(C) struct with one
+    // field" special case.
+    t1: T,
+}
+
+fn is_into_bytes_1<T: IntoBytes>() {
+    if false {
+        is_into_bytes_1::<IntoBytes1<AU16>>();
+    }
+}

zerocopy-derive/tests/ui-msrv/msrv_specific.stderr

@@ -0,0 +1,25 @@
+warning: unused `#[macro_use]` import
+  --> tests/ui-msrv/msrv_specific.rs:12:1
+   |
+12 | #[macro_use]
+   | ^^^^^^^^^^^^
+   |
+   = note: `#[warn(unused_imports)]` on by default
+
+error[E0277]: the trait bound `AU16: Unaligned` is not satisfied
+  --> tests/ui-msrv/msrv_specific.rs:35:9
+   |
+35 |         is_into_bytes_1::<IntoBytes1<AU16>>();
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Unaligned` is not implemented for `AU16`
+   |
+note: required because of the requirements on the impl of `zerocopy::IntoBytes` for `IntoBytes1<AU16>`
+  --> tests/ui-msrv/msrv_specific.rs:24:10
+   |
+24 | #[derive(IntoBytes)]
+   |          ^^^^^^^^^
+note: required by a bound in `is_into_bytes_1`
+  --> tests/ui-msrv/msrv_specific.rs:33:23
+   |
+33 | fn is_into_bytes_1<T: IntoBytes>() {
+   |                       ^^^^^^^^^ required by this bound in `is_into_bytes_1`
+   = note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

zerocopy-derive/tests/ui-msrv/struct.stderr

@@ -20,62 +20,78 @@ error: must have a non-align #[repr(...)] attribute or #[repr(packed)] in order
     |
     = note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
 
+error: must have a non-align #[repr(...)] attribute or #[repr(packed)] in order to guarantee this type's memory layout
+   --> tests/ui-msrv/struct.rs:164:10
+    |
+164 | #[derive(IntoBytes)]
+    |          ^^^^^^^^^
+    |
+    = note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: must have a non-align #[repr(...)] attribute or #[repr(packed)] in order to guarantee this type's memory layout
+   --> tests/ui-msrv/struct.rs:187:10
+    |
+187 | #[derive(IntoBytes)]
+    |          ^^^^^^^^^
+    |
+    = note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
+
 error: cannot derive `Unaligned` on type with alignment greater than 1
-   --> tests/ui-msrv/struct.rs:167:11
+   --> tests/ui-msrv/struct.rs:198:11
     |
-167 | #[repr(C, align(2))]
+198 | #[repr(C, align(2))]
     |           ^^^^^
 
 error: this conflicts with another representation hint
-   --> tests/ui-msrv/struct.rs:171:8
+   --> tests/ui-msrv/struct.rs:202:8
     |
-171 | #[repr(transparent, align(2))]
+202 | #[repr(transparent, align(2))]
     |        ^^^^^^^^^^^
 
 error: this conflicts with another representation hint
-   --> tests/ui-msrv/struct.rs:177:16
+   --> tests/ui-msrv/struct.rs:208:16
     |
-177 | #[repr(packed, align(2))]
+208 | #[repr(packed, align(2))]
     |                ^^^^^
 
 error: this conflicts with another representation hint
-   --> tests/ui-msrv/struct.rs:181:18
+   --> tests/ui-msrv/struct.rs:212:18
     |
-181 | #[repr(align(1), align(2))]
+212 | #[repr(align(1), align(2))]
     |                  ^^^^^
 
 error: this conflicts with another representation hint
-   --> tests/ui-msrv/struct.rs:185:18
+   --> tests/ui-msrv/struct.rs:216:18
     |
-185 | #[repr(align(2), align(4))]
+216 | #[repr(align(2), align(4))]
     |                  ^^^^^
 
 error: must have #[repr(C)], #[repr(transparent)], or #[repr(packed)] attribute in order to guarantee this type's alignment
-   --> tests/ui-msrv/struct.rs:188:10
+   --> tests/ui-msrv/struct.rs:219:10
     |
-188 | #[derive(Unaligned)]
+219 | #[derive(Unaligned)]
     |          ^^^^^^^^^
     |
     = note: this error originates in the derive macro `Unaligned` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: must have #[repr(C)], #[repr(transparent)], or #[repr(packed)] attribute in order to guarantee this type's alignment
-   --> tests/ui-msrv/struct.rs:191:10
+   --> tests/ui-msrv/struct.rs:222:10
     |
-191 | #[derive(Unaligned)]
+222 | #[derive(Unaligned)]
     |          ^^^^^^^^^
     |
     = note: this error originates in the derive macro `Unaligned` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: this conflicts with another representation hint
-   --> tests/ui-msrv/struct.rs:201:8
+   --> tests/ui-msrv/struct.rs:232:8
     |
-201 | #[repr(C, packed(2))]
+232 | #[repr(C, packed(2))]
     |        ^
 
 error[E0692]: transparent struct cannot have other repr hints
-   --> tests/ui-msrv/struct.rs:171:8
+   --> tests/ui-msrv/struct.rs:202:8
     |
-171 | #[repr(transparent, align(2))]
+202 | #[repr(transparent, align(2))]
     |        ^^^^^^^^^^^  ^^^^^^^^
 
 error[E0277]: the size for values of type `[u8]` cannot be known at compilation time

zerocopy-derive/tests/ui-msrv/union.stderr

@@ -6,45 +6,61 @@ error: unsupported on types with type parameters
    |
    = note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
 
+error: must be #[repr(C)], #[repr(packed)], or #[repr(transparent)]
+  --> tests/ui-msrv/union.rs:47:10
+   |
+47 | #[derive(IntoBytes)]
+   |          ^^^^^^^^^
+   |
+   = note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: must be #[repr(C)], #[repr(packed)], or #[repr(transparent)]
+  --> tests/ui-msrv/union.rs:53:10
+   |
+53 | #[derive(IntoBytes)]
+   |          ^^^^^^^^^
+   |
+   = note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)
+
 error: cannot derive `Unaligned` on type with alignment greater than 1
-  --> tests/ui-msrv/union.rs:51:11
+  --> tests/ui-msrv/union.rs:64:11
    |
-51 | #[repr(C, align(2))]
+64 | #[repr(C, align(2))]
    |           ^^^^^
 
 error: this conflicts with another representation hint
-  --> tests/ui-msrv/union.rs:67:16
+  --> tests/ui-msrv/union.rs:80:16
    |
-67 | #[repr(packed, align(2))]
+80 | #[repr(packed, align(2))]
    |                ^^^^^
 
 error: this conflicts with another representation hint
-  --> tests/ui-msrv/union.rs:73:18
+  --> tests/ui-msrv/union.rs:86:18
    |
-73 | #[repr(align(1), align(2))]
+86 | #[repr(align(1), align(2))]
    |                  ^^^^^
 
 error: this conflicts with another representation hint
-  --> tests/ui-msrv/union.rs:79:18
+  --> tests/ui-msrv/union.rs:92:18
    |
-79 | #[repr(align(2), align(4))]
+92 | #[repr(align(2), align(4))]
    |                  ^^^^^
 
 error: must have #[repr(C)], #[repr(transparent)], or #[repr(packed)] attribute in order to guarantee this type's alignment
-  --> tests/ui-msrv/union.rs:84:10
+  --> tests/ui-msrv/union.rs:97:10
    |
-84 | #[derive(Unaligned)]
+97 | #[derive(Unaligned)]
    |          ^^^^^^^^^
    |
    = note: this error originates in the derive macro `Unaligned` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: must have #[repr(C)], #[repr(transparent)], or #[repr(packed)] attribute in order to guarantee this type's alignment
-  --> tests/ui-msrv/union.rs:90:10
-   |
-90 | #[derive(Unaligned)]
-   |          ^^^^^^^^^
-   |
-   = note: this error originates in the derive macro `Unaligned` (in Nightly builds, run with -Z macro-backtrace for more info)
+   --> tests/ui-msrv/union.rs:103:10
+    |
+103 | #[derive(Unaligned)]
+    |          ^^^^^^^^^
+    |
+    = note: this error originates in the derive macro `Unaligned` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error[E0277]: the trait bound `UnsafeCell<()>: zerocopy::Immutable` is not satisfied
   --> tests/ui-msrv/union.rs:24:10