Skip to content

Commit ca8f52f

Browse files
authored
[derive] IntoBytes padding error says number of bytes (#2699)
While we're here, always say "padding" instead of "inter-field padding", as all kinds (structs, enums, and unions) can have trailing padding too. As an example, the diff for `zerocopy-derive/tests/ui-stable/enum.stderr` contains: -error[E0277]: `IntoBytes3` has inter-field padding +error[E0277]: `IntoBytes3` has 2 total byte(s) of padding Closes #1717
1 parent 4c66847 commit ca8f52f

File tree

13 files changed

+166
-154
lines changed

13 files changed

+166
-154
lines changed

src/macros.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -999,14 +999,14 @@ macro_rules! cryptocorrosion_derive_traits {
999999
// SAFETY: `#[repr(transparent)]` structs cannot have the same layout as
10001000
// their single non-zero-sized field, and so cannot have any padding
10011001
// outside of that field.
1002-
false
1002+
0
10031003
};
10041004
(
10051005
@struct_padding_check #[repr(C)]
10061006
$(($($tuple_field_ty:ty),*))?
10071007
$({$($field_ty:ty),*})?
10081008
) => {
1009-
$crate::struct_has_padding!(
1009+
$crate::struct_padding!(
10101010
Self,
10111011
[
10121012
$($($tuple_field_ty),*)?
@@ -1087,7 +1087,7 @@ macro_rules! cryptocorrosion_derive_traits {
10871087
(): $crate::util::macro_util::PaddingFree<
10881088
Self,
10891089
{
1090-
$crate::union_has_padding!(
1090+
$crate::union_padding!(
10911091
Self,
10921092
[$($field_ty),*]
10931093
)

src/util/macro_util.rs

Lines changed: 87 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -52,15 +52,15 @@ pub unsafe trait Field<Index> {
5252
#[cfg_attr(
5353
zerocopy_diagnostic_on_unimplemented_1_78_0,
5454
diagnostic::on_unimplemented(
55-
message = "`{T}` has inter-field padding",
55+
message = "`{T}` has {PADDING_BYTES} total byte(s) of padding",
5656
label = "types with padding cannot implement `IntoBytes`",
5757
note = "consider using `zerocopy::Unalign` to lower the alignment of individual fields",
5858
note = "consider adding explicit fields where padding would be",
59-
note = "consider using `#[repr(packed)]` to remove inter-field padding"
59+
note = "consider using `#[repr(packed)]` to remove padding"
6060
)
6161
)]
62-
pub trait PaddingFree<T: ?Sized, const HAS_PADDING: bool> {}
63-
impl<T: ?Sized> PaddingFree<T, false> for () {}
62+
pub trait PaddingFree<T: ?Sized, const PADDING_BYTES: usize> {}
63+
impl<T: ?Sized> PaddingFree<T, 0> for () {}
6464

6565
/// A type whose size is equal to `align_of::<T>()`.
6666
#[repr(C)]
@@ -318,8 +318,8 @@ pub type SizeToTag<const SIZE: usize> = <() as size_to_tag::SizeToTag<SIZE>>::Ta
318318
mod __size_of {
319319
#[diagnostic::on_unimplemented(
320320
message = "`{Self}` is unsized",
321-
label = "`IntoBytes` needs all field types to be `Sized` in order to determine whether there is inter-field padding",
322-
note = "consider using `#[repr(packed)]` to remove inter-field padding",
321+
label = "`IntoBytes` needs all field types to be `Sized` in order to determine whether there is padding",
322+
note = "consider using `#[repr(packed)]` to remove padding",
323323
note = "`IntoBytes` does not require the fields of `#[repr(packed)]` types to be `Sized`"
324324
)]
325325
pub trait Sized: core::marker::Sized {}
@@ -339,74 +339,86 @@ pub use core::mem::size_of;
339339
#[cfg(zerocopy_diagnostic_on_unimplemented_1_78_0)]
340340
pub use __size_of::size_of;
341341

342-
/// Does the struct type `$t` have padding?
342+
/// How many padding bytes does the struct type `$t` have?
343343
///
344-
/// `$ts` is the list of the type of every field in `$t`. `$t` must be a
345-
/// struct type, or else `struct_has_padding!`'s result may be meaningless.
344+
/// `$ts` is the list of the type of every field in `$t`. `$t` must be a struct
345+
/// type, or else `struct_padding!`'s result may be meaningless.
346346
///
347-
/// Note that `struct_has_padding!`'s results are independent of `repcr` since
348-
/// they only consider the size of the type and the sizes of the fields.
349-
/// Whatever the repr, the size of the type already takes into account any
350-
/// padding that the compiler has decided to add. Structs with well-defined
351-
/// representations (such as `repr(C)`) can use this macro to check for padding.
352-
/// Note that while this may yield some consistent value for some `repr(Rust)`
353-
/// structs, it is not guaranteed across platforms or compilations.
347+
/// Note that `struct_padding!`'s results are independent of `repcr` since they
348+
/// only consider the size of the type and the sizes of the fields. Whatever the
349+
/// repr, the size of the type already takes into account any padding that the
350+
/// compiler has decided to add. Structs with well-defined representations (such
351+
/// as `repr(C)`) can use this macro to check for padding. Note that while this
352+
/// may yield some consistent value for some `repr(Rust)` structs, it is not
353+
/// guaranteed across platforms or compilations.
354354
#[doc(hidden)] // `#[macro_export]` bypasses this module's `#[doc(hidden)]`.
355355
#[macro_export]
356-
macro_rules! struct_has_padding {
356+
macro_rules! struct_padding {
357357
($t:ty, [$($ts:ty),*]) => {
358-
$crate::util::macro_util::size_of::<$t>() > 0 $(+ $crate::util::macro_util::size_of::<$ts>())*
358+
$crate::util::macro_util::size_of::<$t>() - (0 $(+ $crate::util::macro_util::size_of::<$ts>())*)
359359
};
360360
}
361361

362-
/// Does the union type `$t` have padding?
362+
/// How many padding bytes does the union type `$t` have?
363363
///
364-
/// `$ts` is the list of the type of every field in `$t`. `$t` must be a
365-
/// union type, or else `union_has_padding!`'s result may be meaningless.
364+
/// `$ts` is the list of the type of every field in `$t`. `$t` must be a union
365+
/// type, or else `union_padding!`'s result may be meaningless.
366366
///
367-
/// Note that `union_has_padding!`'s results are independent of `repr` since
368-
/// they only consider the size of the type and the sizes of the fields.
369-
/// Whatever the repr, the size of the type already takes into account any
370-
/// padding that the compiler has decided to add. Unions with well-defined
371-
/// representations (such as `repr(C)`) can use this macro to check for padding.
372-
/// Note that while this may yield some consistent value for some `repr(Rust)`
373-
/// unions, it is not guaranteed across platforms or compilations.
367+
/// Note that `union_padding!`'s results are independent of `repr` since they
368+
/// only consider the size of the type and the sizes of the fields. Whatever the
369+
/// repr, the size of the type already takes into account any padding that the
370+
/// compiler has decided to add. Unions with well-defined representations (such
371+
/// as `repr(C)`) can use this macro to check for padding. Note that while this
372+
/// may yield some consistent value for some `repr(Rust)` unions, it is not
373+
/// guaranteed across platforms or compilations.
374374
#[doc(hidden)] // `#[macro_export]` bypasses this module's `#[doc(hidden)]`.
375375
#[macro_export]
376-
macro_rules! union_has_padding {
377-
($t:ty, [$($ts:ty),*]) => {
378-
false $(|| $crate::util::macro_util::size_of::<$t>() != $crate::util::macro_util::size_of::<$ts>())*
379-
};
376+
macro_rules! union_padding {
377+
($t:ty, [$($ts:ty),*]) => {{
378+
let mut max = 0;
379+
$({
380+
let padding = $crate::util::macro_util::size_of::<$t>() - $crate::util::macro_util::size_of::<$ts>();
381+
if padding > max {
382+
max = padding;
383+
}
384+
})*
385+
max
386+
}};
380387
}
381388

382-
/// Does the enum type `$t` have padding?
389+
/// How many padding bytes does the enum type `$t` have?
383390
///
384391
/// `$disc` is the type of the enum tag, and `$ts` is a list of fields in each
385392
/// square-bracket-delimited variant. `$t` must be an enum, or else
386-
/// `enum_has_padding!`'s result may be meaningless. An enum has padding if any
387-
/// of its variant structs [1][2] contain padding, and so all of the variants of
388-
/// an enum must be "full" in order for the enum to not have padding.
393+
/// `enum_padding!`'s result may be meaningless. An enum has padding if any of
394+
/// its variant structs [1][2] contain padding, and so all of the variants of an
395+
/// enum must be "full" in order for the enum to not have padding.
389396
///
390-
/// The results of `enum_has_padding!` require that the enum is not
391-
/// `repr(Rust)`, as `repr(Rust)` enums may niche the enum's tag and reduce the
392-
/// total number of bytes required to represent the enum as a result. As long as
393-
/// the enum is `repr(C)`, `repr(int)`, or `repr(C, int)`, this will
394-
/// consistently return whether the enum contains any padding bytes.
397+
/// The results of `enum_padding!` require that the enum is not `repr(Rust)`, as
398+
/// `repr(Rust)` enums may niche the enum's tag and reduce the total number of
399+
/// bytes required to represent the enum as a result. As long as the enum is
400+
/// `repr(C)`, `repr(int)`, or `repr(C, int)`, this will consistently return
401+
/// whether the enum contains any padding bytes.
395402
///
396403
/// [1]: https://doc.rust-lang.org/1.81.0/reference/type-layout.html#reprc-enums-with-fields
397404
/// [2]: https://doc.rust-lang.org/1.81.0/reference/type-layout.html#primitive-representation-of-enums-with-fields
398405
#[doc(hidden)] // `#[macro_export]` bypasses this module's `#[doc(hidden)]`.
399406
#[macro_export]
400-
macro_rules! enum_has_padding {
401-
($t:ty, $disc:ty, $([$($ts:ty),*]),*) => {
402-
false $(
403-
|| $crate::util::macro_util::size_of::<$t>()
404-
!= (
407+
macro_rules! enum_padding {
408+
($t:ty, $disc:ty, $([$($ts:ty),*]),*) => {{
409+
let mut max = 0;
410+
$({
411+
let padding = $crate::util::macro_util::size_of::<$t>()
412+
- (
405413
$crate::util::macro_util::size_of::<$disc>()
406414
$(+ $crate::util::macro_util::size_of::<$ts>())*
407-
)
408-
)*
409-
}
415+
);
416+
if padding > max {
417+
max = padding;
418+
}
419+
})*
420+
max
421+
}};
410422
}
411423

412424
/// Does `t` have alignment greater than or equal to `u`? If not, this macro
@@ -1111,65 +1123,65 @@ mod tests {
11111123
}
11121124

11131125
#[test]
1114-
fn test_struct_has_padding() {
1115-
// Test that, for each provided repr, `struct_has_padding!` reports the
1126+
fn test_struct_padding() {
1127+
// Test that, for each provided repr, `struct_padding!` reports the
11161128
// expected value.
11171129
macro_rules! test {
11181130
(#[$cfg:meta] ($($ts:ty),*) => $expect:expr) => {{
11191131
#[$cfg]
11201132
struct Test($(#[allow(dead_code)] $ts),*);
1121-
assert_eq!(struct_has_padding!(Test, [$($ts),*]), $expect);
1133+
assert_eq!(struct_padding!(Test, [$($ts),*]), $expect);
11221134
}};
11231135
(#[$cfg:meta] $(#[$cfgs:meta])* ($($ts:ty),*) => $expect:expr) => {
11241136
test!(#[$cfg] ($($ts),*) => $expect);
11251137
test!($(#[$cfgs])* ($($ts),*) => $expect);
11261138
};
11271139
}
11281140

1129-
test!(#[repr(C)] #[repr(transparent)] #[repr(packed)] () => false);
1130-
test!(#[repr(C)] #[repr(transparent)] #[repr(packed)] (u8) => false);
1131-
test!(#[repr(C)] #[repr(transparent)] #[repr(packed)] (u8, ()) => false);
1132-
test!(#[repr(C)] #[repr(packed)] (u8, u8) => false);
1141+
test!(#[repr(C)] #[repr(transparent)] #[repr(packed)] () => 0);
1142+
test!(#[repr(C)] #[repr(transparent)] #[repr(packed)] (u8) => 0);
1143+
test!(#[repr(C)] #[repr(transparent)] #[repr(packed)] (u8, ()) => 0);
1144+
test!(#[repr(C)] #[repr(packed)] (u8, u8) => 0);
11331145

1134-
test!(#[repr(C)] (u8, AU64) => true);
1146+
test!(#[repr(C)] (u8, AU64) => 7);
11351147
// Rust won't let you put `#[repr(packed)]` on a type which contains a
11361148
// `#[repr(align(n > 1))]` type (`AU64`), so we have to use `u64` here.
11371149
// It's not ideal, but it definitely has align > 1 on /some/ of our CI
11381150
// targets, and this isn't a particularly complex macro we're testing
11391151
// anyway.
1140-
test!(#[repr(packed)] (u8, u64) => false);
1152+
test!(#[repr(packed)] (u8, u64) => 0);
11411153
}
11421154

11431155
#[test]
1144-
fn test_union_has_padding() {
1145-
// Test that, for each provided repr, `union_has_padding!` reports the
1156+
fn test_union_padding() {
1157+
// Test that, for each provided repr, `union_padding!` reports the
11461158
// expected value.
11471159
macro_rules! test {
11481160
(#[$cfg:meta] {$($fs:ident: $ts:ty),*} => $expect:expr) => {{
11491161
#[$cfg]
11501162
#[allow(unused)] // fields are never read
11511163
union Test{ $($fs: $ts),* }
1152-
assert_eq!(union_has_padding!(Test, [$($ts),*]), $expect);
1164+
assert_eq!(union_padding!(Test, [$($ts),*]), $expect);
11531165
}};
11541166
(#[$cfg:meta] $(#[$cfgs:meta])* {$($fs:ident: $ts:ty),*} => $expect:expr) => {
11551167
test!(#[$cfg] {$($fs: $ts),*} => $expect);
11561168
test!($(#[$cfgs])* {$($fs: $ts),*} => $expect);
11571169
};
11581170
}
11591171

1160-
test!(#[repr(C)] #[repr(packed)] {a: u8} => false);
1161-
test!(#[repr(C)] #[repr(packed)] {a: u8, b: u8} => false);
1172+
test!(#[repr(C)] #[repr(packed)] {a: u8} => 0);
1173+
test!(#[repr(C)] #[repr(packed)] {a: u8, b: u8} => 0);
11621174

11631175
// Rust won't let you put `#[repr(packed)]` on a type which contains a
11641176
// `#[repr(align(n > 1))]` type (`AU64`), so we have to use `u64` here.
11651177
// It's not ideal, but it definitely has align > 1 on /some/ of our CI
11661178
// targets, and this isn't a particularly complex macro we're testing
11671179
// anyway.
1168-
test!(#[repr(C)] #[repr(packed)] {a: u8, b: u64} => true);
1180+
test!(#[repr(C)] #[repr(packed)] {a: u8, b: u64} => 7);
11691181
}
11701182

11711183
#[test]
1172-
fn test_enum_has_padding() {
1184+
fn test_enum_padding() {
11731185
// Test that, for each provided repr, `enum_has_padding!` reports the
11741186
// expected value.
11751187
macro_rules! test {
@@ -1188,7 +1200,7 @@ mod tests {
11881200
$($vs ($($ts),*),)*
11891201
}
11901202
assert_eq!(
1191-
enum_has_padding!(Test, $disc, $([$($ts),*]),*),
1203+
enum_padding!(Test, $disc, $([$($ts),*]),*),
11921204
$expect
11931205
);
11941206
}};
@@ -1204,41 +1216,41 @@ mod tests {
12041216

12051217
test!(#[repr(u8)] #[repr(C)] {
12061218
A(u8),
1207-
} => false);
1219+
} => 0);
12081220
test!(#[repr(u16)] #[repr(C)] {
12091221
A(u8, u8),
12101222
B(U16),
1211-
} => false);
1223+
} => 0);
12121224
test!(#[repr(u32)] #[repr(C)] {
12131225
A(u8, u8, u8, u8),
12141226
B(U16, u8, u8),
12151227
C(u8, u8, U16),
12161228
D(U16, U16),
12171229
E(U32),
1218-
} => false);
1230+
} => 0);
12191231

12201232
// `repr(int)` can pack the discriminant more efficiently
12211233
test!(#[repr(u8)] {
12221234
A(u8, U16),
1223-
} => false);
1235+
} => 0);
12241236
test!(#[repr(u8)] {
12251237
A(u8, U16, U32),
1226-
} => false);
1238+
} => 0);
12271239

12281240
// `repr(C)` cannot
12291241
test!(#[repr(u8, C)] {
12301242
A(u8, U16),
1231-
} => true);
1243+
} => 2);
12321244
test!(#[repr(u8, C)] {
12331245
A(u8, u8, u8, U32),
1234-
} => true);
1246+
} => 4);
12351247

12361248
// And field ordering can always cause problems
12371249
test!(#[repr(u8)] #[repr(C)] {
12381250
A(U16, u8),
1239-
} => true);
1251+
} => 2);
12401252
test!(#[repr(u8)] #[repr(C)] {
12411253
A(U32, u8, u8, u8),
1242-
} => true);
1254+
} => 4);
12431255
}
12441256
}

zerocopy-derive/src/lib.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1489,9 +1489,9 @@ impl PaddingCheck {
14891489
/// passes the padding check encoded by `PaddingCheck`.
14901490
fn validator_macro_ident(&self) -> Ident {
14911491
let s = match self {
1492-
PaddingCheck::Struct => "struct_has_padding",
1493-
PaddingCheck::Union => "union_has_padding",
1494-
PaddingCheck::Enum { .. } => "enum_has_padding",
1492+
PaddingCheck::Struct => "struct_padding",
1493+
PaddingCheck::Union => "union_padding",
1494+
PaddingCheck::Enum { .. } => "enum_padding",
14951495
};
14961496

14971497
Ident::new(s, Span::call_site())

zerocopy-derive/src/output_tests.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -477,7 +477,7 @@ fn test_into_bytes_struct() {
477477
u8: ::zerocopy::IntoBytes,
478478
(): ::zerocopy::util::macro_util::PaddingFree<
479479
Self,
480-
{ ::zerocopy::struct_has_padding!(Self, [u8, u8]) },
480+
{ ::zerocopy::struct_padding!(Self, [u8, u8]) },
481481
>,
482482
{
483483
fn only_derive_is_allowed_to_implement_this_trait() {}
@@ -501,7 +501,7 @@ fn test_into_bytes_struct() {
501501
[Trailing]: ::zerocopy::IntoBytes,
502502
(): ::zerocopy::util::macro_util::PaddingFree<
503503
Self,
504-
{ ::zerocopy::struct_has_padding!(Self, [u8, [Trailing]]) },
504+
{ ::zerocopy::struct_padding!(Self, [u8, [Trailing]]) },
505505
>,
506506
{
507507
fn only_derive_is_allowed_to_implement_this_trait() {}

0 commit comments

Comments
 (0)