Skip to content

Commit d296c26

Browse files
jswrennjswrenn
authored
Adopt and document a preference for aligned loads in validation (#2985)
This commit parameterizes `TryFromBytes::is_bit_valid` and `Ptr::read` over alignment, ensuring that well-aligned loads are used at the leaves of validation whenever possible. This carries a performance advantage over unaligned loads on many platforms. This manifests in our API as an optimization for `try_transmute!` and `TryFromBytes::try_read_from_*` for destination types with alignments greater than 1. For trivially-aligned destination types, this commit introduces an optimization FIXME noting that validation could (and should) be performed in-place. Makes progress towards #2981. gherrit-pr-id: G74407a530f46f997b4faaeac52452ebc9892b2ae
1 parent 6b81374 commit d296c26

17 files changed

+246
-85
lines changed

‎src/impls.rs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ const _: () = unsafe { unsafe_impl!(char: Immutable, FromZeros, IntoBytes) };
139139
const _: () = unsafe {
140140
unsafe_impl!(=> TryFromBytes for char; |c| {
141141
let c = c.transmute_with::<Unalign<u32>, invariant::Valid, CastSizedExact, BecauseImmutable>();
142-
let c = c.read_unaligned().into_inner();
142+
let c = c.read().into_inner();
143143
char::from_u32(c).is_some()
144144
});
145145
};
@@ -182,7 +182,7 @@ macro_rules! unsafe_impl_try_from_bytes_for_nonzero {
182182
$(
183183
unsafe_impl!(=> TryFromBytes for $nonzero; |n| {
184184
let n = n.transmute_with::<Unalign<$prim>, invariant::Valid, CastSizedExact, BecauseImmutable>();
185-
$nonzero::new(n.read_unaligned().into_inner()).is_some()
185+
$nonzero::new(n.read().into_inner()).is_some()
186186
});
187187
)*
188188
}
@@ -856,8 +856,11 @@ unsafe impl<T: TryFromBytes + ?Sized> TryFromBytes for UnsafeCell<T> {
856856
{
857857
}
858858

859-
#[inline]
860-
fn is_bit_valid(candidate: Maybe<'_, Self>) -> bool {
859+
#[inline(always)]
860+
fn is_bit_valid<A>(candidate: Maybe<'_, Self, A>) -> bool
861+
where
862+
A: invariant::Alignment,
863+
{
861864
T::is_bit_valid(candidate.transmute::<_, _, BecauseImmutable>())
862865
}
863866
}

‎src/lib.rs‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1713,7 +1713,9 @@ pub unsafe trait TryFromBytes {
17131713
/// [`UnsafeCell`]: core::cell::UnsafeCell
17141714
/// [`Shared`]: invariant::Shared
17151715
#[doc(hidden)]
1716-
fn is_bit_valid(candidate: Maybe<'_, Self>) -> bool;
1716+
fn is_bit_valid<A>(candidate: Maybe<'_, Self, A>) -> bool
1717+
where
1718+
A: invariant::Alignment;
17171719

17181720
/// Attempts to interpret the given `source` as a `&Self`.
17191721
///
@@ -2882,12 +2884,22 @@ pub unsafe trait TryFromBytes {
28822884
/// let bytes = &mut [0x10, 0xC0, 240, 77][..];
28832885
/// assert!(Packet::try_read_from_bytes(bytes).is_err());
28842886
/// ```
2887+
///
2888+
/// # Performance Considerations
2889+
///
2890+
/// In this version of zerocopy, this method reads the `source` into a
2891+
/// well-aligned stack allocation and *then* validates that the allocation
2892+
/// is a valid `Self`. This ensures that validation can be performed using
2893+
/// aligned reads (which carry a performance advantage over unaligned reads
2894+
/// on many platforms) at the cost of an unconditional copy.
28852895
#[must_use = "has no side effects"]
28862896
#[inline]
28872897
fn try_read_from_bytes(source: &[u8]) -> Result<Self, TryReadError<&[u8], Self>>
28882898
where
28892899
Self: Sized,
28902900
{
2901+
// FIXME(#2981): If `align_of::<Self>() == 1`, validate `source` in-place.
2902+
28912903
let candidate = match CoreMaybeUninit::<Self>::read_from_bytes(source) {
28922904
Ok(candidate) => candidate,
28932905
Err(e) => {
@@ -2943,12 +2955,22 @@ pub unsafe trait TryFromBytes {
29432955
/// let bytes = &[0x10, 0xC0, 240, 77, 0, 1, 2, 3, 4, 5, 6][..];
29442956
/// assert!(Packet::try_read_from_prefix(bytes).is_err());
29452957
/// ```
2958+
///
2959+
/// # Performance Considerations
2960+
///
2961+
/// In this version of zerocopy, this method reads the `source` into a
2962+
/// well-aligned stack allocation and *then* validates that the allocation
2963+
/// is a valid `Self`. This ensures that validation can be performed using
2964+
/// aligned reads (which carry a performance advantage over unaligned reads
2965+
/// on many platforms) at the cost of an unconditional copy.
29462966
#[must_use = "has no side effects"]
29472967
#[inline]
29482968
fn try_read_from_prefix(source: &[u8]) -> Result<(Self, &[u8]), TryReadError<&[u8], Self>>
29492969
where
29502970
Self: Sized,
29512971
{
2972+
// FIXME(#2981): If `align_of::<Self>() == 1`, validate `source` in-place.
2973+
29522974
let (candidate, suffix) = match CoreMaybeUninit::<Self>::read_from_prefix(source) {
29532975
Ok(candidate) => candidate,
29542976
Err(e) => {
@@ -3005,12 +3027,22 @@ pub unsafe trait TryFromBytes {
30053027
/// let bytes = &[0, 1, 2, 3, 4, 5, 0x10, 0xC0, 240, 77][..];
30063028
/// assert!(Packet::try_read_from_suffix(bytes).is_err());
30073029
/// ```
3030+
///
3031+
/// # Performance Considerations
3032+
///
3033+
/// In this version of zerocopy, this method reads the `source` into a
3034+
/// well-aligned stack allocation and *then* validates that the allocation
3035+
/// is a valid `Self`. This ensures that validation can be performed using
3036+
/// aligned reads (which carry a performance advantage over unaligned reads
3037+
/// on many platforms) at the cost of an unconditional copy.
30083038
#[must_use = "has no side effects"]
30093039
#[inline]
30103040
fn try_read_from_suffix(source: &[u8]) -> Result<(&[u8], Self), TryReadError<&[u8], Self>>
30113041
where
30123042
Self: Sized,
30133043
{
3044+
// FIXME(#2981): If `align_of::<Self>() == 1`, validate `source` in-place.
3045+
30143046
let (prefix, candidate) = match CoreMaybeUninit::<Self>::read_from_suffix(source) {
30153047
Ok(candidate) => candidate,
30163048
Err(e) => {

‎src/macros.rs‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -983,8 +983,11 @@ macro_rules! cryptocorrosion_derive_traits {
983983
$($field_ty: $crate::FromBytes,)*
984984
)?
985985
{
986-
#[inline]
987-
fn is_bit_valid(_c: $crate::Maybe<'_, Self>) -> bool {
986+
#[inline(always)]
987+
fn is_bit_valid<A>(_: $crate::Maybe<'_, Self, A>) -> bool
988+
where
989+
A: $crate::invariant::Alignment,
990+
{
988991
// SAFETY: This macro only accepts `#[repr(C)]` and
989992
// `#[repr(transparent)]` structs, and this `impl` block
990993
// requires all field types to be `FromBytes`. Thus, all
@@ -1124,8 +1127,11 @@ macro_rules! cryptocorrosion_derive_traits {
11241127
$field_ty: $crate::FromBytes,
11251128
)*
11261129
{
1127-
#[inline]
1128-
fn is_bit_valid(_c: $crate::Maybe<'_, Self>) -> bool {
1130+
#[inline(always)]
1131+
fn is_bit_valid<A>(_: $crate::Maybe<'_, Self, A>) -> bool
1132+
where
1133+
A: $crate::invariant::Alignment,
1134+
{
11291135
// SAFETY: This macro only accepts `#[repr(C)]` unions, and this
11301136
// `impl` block requires all field types to be `FromBytes`.
11311137
// Thus, all initialized byte sequences constitutes valid

‎src/pointer/invariant.rs‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,15 @@ pub trait Aliasing: Sealed {
3939
}
4040

4141
/// The alignment invariant of a [`Ptr`][super::Ptr].
42-
pub trait Alignment: Sealed {}
42+
pub trait Alignment: Sealed {
43+
#[doc(hidden)]
44+
#[must_use]
45+
fn read<T, I, R>(ptr: crate::Ptr<'_, T, I>) -> T
46+
where
47+
T: Copy + Read<I::Aliasing, R>,
48+
I: Invariants<Alignment = Self, Validity = Valid>,
49+
I::Aliasing: Reference;
50+
}
4351

4452
/// The validity invariant of a [`Ptr`][super::Ptr].
4553
///
@@ -132,12 +140,32 @@ impl Reference for Exclusive {}
132140
/// It is unknown whether the pointer is aligned.
133141
pub enum Unaligned {}
134142

135-
impl Alignment for Unaligned {}
143+
impl Alignment for Unaligned {
144+
#[inline(always)]
145+
fn read<T, I, R>(ptr: crate::Ptr<'_, T, I>) -> T
146+
where
147+
T: Copy + Read<I::Aliasing, R>,
148+
I: Invariants<Alignment = Self, Validity = Valid>,
149+
I::Aliasing: Reference,
150+
{
151+
(*ptr.into_unalign().as_ref()).into_inner()
152+
}
153+
}
136154

137155
/// The referent is aligned: for `Ptr<T>`, the referent's address is a multiple
138156
/// of the `T`'s alignment.
139157
pub enum Aligned {}
140-
impl Alignment for Aligned {}
158+
impl Alignment for Aligned {
159+
#[inline(always)]
160+
fn read<T, I, R>(ptr: crate::Ptr<'_, T, I>) -> T
161+
where
162+
T: Copy + Read<I::Aliasing, R>,
163+
I: Invariants<Alignment = Self, Validity = Valid>,
164+
I::Aliasing: Reference,
165+
{
166+
*ptr.as_ref()
167+
}
168+
}
141169

142170
/// Any bit pattern is allowed in the `Ptr`'s referent, including uninitialized
143171
/// bytes.

‎src/pointer/mod.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ use crate::wrappers::ReadOnly;
2828
/// to [`TryFromBytes::is_bit_valid`].
2929
///
3030
/// [`TryFromBytes::is_bit_valid`]: crate::TryFromBytes::is_bit_valid
31-
pub type Maybe<'a, T, Aliasing = invariant::Shared, Alignment = invariant::Unaligned> =
32-
Ptr<'a, ReadOnly<T>, (Aliasing, Alignment, invariant::Initialized)>;
31+
pub type Maybe<'a, T, Alignment = invariant::Unaligned> =
32+
Ptr<'a, ReadOnly<T>, (invariant::Shared, Alignment, invariant::Initialized)>;
3333

3434
/// Checks if the referent is zeroed.
3535
pub(crate) fn is_zeroed<T, I>(ptr: Ptr<'_, T, I>) -> bool

‎src/pointer/ptr.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -584,13 +584,13 @@ mod _conversions {
584584
{
585585
/// Reads the referent.
586586
#[must_use]
587-
#[inline]
588-
pub fn read_unaligned<R>(self) -> T
587+
#[inline(always)]
588+
pub fn read<R>(self) -> T
589589
where
590590
T: Copy,
591591
T: Read<I::Aliasing, R>,
592592
{
593-
(*self.into_unalign().as_ref()).into_inner()
593+
<I::Alignment as Alignment>::read(self)
594594
}
595595

596596
/// Views the value as an aligned reference.

‎src/util/macros.rs‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -135,15 +135,24 @@ macro_rules! unsafe_impl {
135135
fn only_derive_is_allowed_to_implement_this_trait() {}
136136

137137
#[inline]
138-
fn is_bit_valid($candidate: Maybe<'_, Self>) -> bool {
138+
fn is_bit_valid<Alignment>($candidate: Maybe<'_, Self, Alignment>) -> bool
139+
where
140+
Alignment: crate::invariant::Alignment,
141+
{
139142
$is_bit_valid
140143
}
141144
};
142145
(@method TryFromBytes) => {
143146
#[allow(clippy::missing_inline_in_public_items)]
144147
#[cfg_attr(all(coverage_nightly, __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS), coverage(off))]
145148
fn only_derive_is_allowed_to_implement_this_trait() {}
146-
#[inline(always)] fn is_bit_valid(_candidate: Maybe<'_, Self>) -> bool { true }
149+
#[inline(always)]
150+
fn is_bit_valid<Alignment>(_candidate: Maybe<'_, Self, Alignment>) -> bool
151+
where
152+
Alignment: crate::invariant::Alignment,
153+
{
154+
true
155+
}
147156
};
148157
(@method $trait:ident) => {
149158
#[allow(clippy::missing_inline_in_public_items, dead_code)]
@@ -217,8 +226,11 @@ macro_rules! impl_for_transmute_from {
217226
$(<$tyvar:ident $(: $(? $optbound:ident $(+)?)* $($bound:ident $(+)?)* )?>)?
218227
TryFromBytes for $ty:ty [$repr:ty]
219228
) => {
220-
#[inline]
221-
fn is_bit_valid(candidate: $crate::Maybe<'_, Self>) -> bool {
229+
#[inline(always)]
230+
fn is_bit_valid<Alignment>(candidate: $crate::Maybe<'_, Self, Alignment>) -> bool
231+
where
232+
Alignment: $crate::invariant::Alignment,
233+
{
222234
// SAFETY: This macro ensures that `$repr` and `Self` have the same
223235
// size and bit validity. Thus, a bit-valid instance of `$repr` is
224236
// also a bit-valid instance of `Self`.

‎zerocopy-derive/src/derive/try_from_bytes.rs‎

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -369,9 +369,13 @@ pub(crate) fn derive_is_bit_valid(
369369
// enum's tag corresponds to one of the enum's discriminants. Then, we
370370
// check the bit validity of each field of the corresponding variant.
371371
// Thus, this is a sound implementation of `is_bit_valid`.
372-
fn is_bit_valid(
373-
mut candidate: #zerocopy_crate::Maybe<'_, Self>,
374-
) -> #core::primitive::bool {
372+
#[inline]
373+
fn is_bit_valid<___ZcAlignment>(
374+
mut candidate: #zerocopy_crate::Maybe<'_, Self, ___ZcAlignment>,
375+
) -> #core::primitive::bool
376+
where
377+
___ZcAlignment: #zerocopy_crate::invariant::Alignment,
378+
{
375379
#tag_enum
376380

377381
type ___ZerocopyTagPrimitive = #zerocopy_crate::util::macro_util::SizeToTag<
@@ -415,7 +419,7 @@ pub(crate) fn derive_is_bit_valid(
415419
#zerocopy_crate::pointer::cast::CastSized
416420
>()
417421
};
418-
tag_ptr.recall_validity::<_, (_, (_, _))>().read_unaligned::<#zerocopy_crate::BecauseImmutable>()
422+
tag_ptr.recall_validity::<_, (_, (_, _))>().read::<#zerocopy_crate::BecauseImmutable>()
419423
};
420424

421425
let mut raw_enum = candidate.cast::<
@@ -570,9 +574,13 @@ fn derive_try_from_bytes_struct(
570574
// validity of a struct is just the composition of the bit
571575
// validities of its fields, so this is a sound implementation
572576
// of `is_bit_valid`.
573-
fn is_bit_valid(
574-
mut candidate: #zerocopy_crate::Maybe<Self>,
575-
) -> #core::primitive::bool {
577+
#[inline]
578+
fn is_bit_valid<___ZcAlignment>(
579+
mut candidate: #zerocopy_crate::Maybe<'_, Self, ___ZcAlignment>,
580+
) -> #core::primitive::bool
581+
where
582+
___ZcAlignment: #zerocopy_crate::invariant::Alignment,
583+
{
576584
true #(&& {
577585
let field_candidate = #zerocopy_crate::into_inner!(candidate.reborrow().project::<
578586
_,
@@ -614,9 +622,13 @@ fn derive_try_from_bytes_union(ctx: &Ctx, unn: &DataUnion, top_level: Trait) ->
614622
// The bit validity of a union is not yet well defined in Rust,
615623
// but it is guaranteed to be no more strict than this
616624
// definition. See #696 for a more in-depth discussion.
617-
fn is_bit_valid(
618-
mut candidate: #zerocopy_crate::Maybe<'_, Self>,
619-
) -> #core::primitive::bool {
625+
#[inline]
626+
fn is_bit_valid<___ZcAlignment>(
627+
mut candidate: #zerocopy_crate::Maybe<'_, Self, ___ZcAlignment>,
628+
) -> #core::primitive::bool
629+
where
630+
___ZcAlignment: #zerocopy_crate::invariant::Alignment,
631+
{
620632
false #(|| {
621633
// SAFETY:
622634
// - Since `ReadOnly<Self>: Immutable` unconditionally,
@@ -690,9 +702,13 @@ fn try_gen_trivial_is_bit_valid(ctx: &Ctx, top_level: Trait) -> Option<proc_macr
690702
let core = ctx.core_path();
691703
Some(quote!(
692704
// SAFETY: See inline.
693-
fn is_bit_valid(
694-
_candidate: #zerocopy_crate::Maybe<Self>,
695-
) -> #core::primitive::bool {
705+
#[inline(always)]
706+
fn is_bit_valid<___ZcAlignment>(
707+
_candidate: #zerocopy_crate::Maybe<'_, Self, ___ZcAlignment>,
708+
) -> #core::primitive::bool
709+
where
710+
___ZcAlignment: #zerocopy_crate::invariant::Alignment,
711+
{
696712
if false {
697713
fn assert_is_from_bytes<T>()
698714
where
@@ -720,9 +736,13 @@ unsafe fn gen_trivial_is_bit_valid_unchecked(ctx: &Ctx) -> proc_macro2::TokenStr
720736
quote!(
721737
// SAFETY: The caller of `gen_trivial_is_bit_valid_unchecked` has
722738
// promised that all initialized bit patterns are valid for `Self`.
723-
fn is_bit_valid(
724-
_candidate: #zerocopy_crate::Maybe<Self>,
725-
) -> #core::primitive::bool {
739+
#[inline(always)]
740+
fn is_bit_valid<___ZcAlignment>(
741+
_candidate: #zerocopy_crate::Maybe<'_, Self, ___ZcAlignment>,
742+
) -> #core::primitive::bool
743+
where
744+
___ZcAlignment: #zerocopy_crate::invariant::Alignment,
745+
{
726746
true
727747
}
728748
)

‎zerocopy-derive/src/output_tests/expected/from_bytes_enum.expected.rs‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,13 @@
1313
const _: () = {
1414
unsafe impl ::zerocopy::TryFromBytes for Foo {
1515
fn only_derive_is_allowed_to_implement_this_trait() {}
16-
fn is_bit_valid(
17-
_candidate: ::zerocopy::Maybe<Self>,
18-
) -> ::zerocopy::util::macro_util::core_reexport::primitive::bool {
16+
#[inline(always)]
17+
fn is_bit_valid<___ZcAlignment>(
18+
_candidate: ::zerocopy::Maybe<'_, Self, ___ZcAlignment>,
19+
) -> ::zerocopy::util::macro_util::core_reexport::primitive::bool
20+
where
21+
___ZcAlignment: ::zerocopy::invariant::Alignment,
22+
{
1923
if false {
2024
fn assert_is_from_bytes<T>()
2125
where

‎zerocopy-derive/src/output_tests/expected/from_bytes_struct.expected.rs‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,13 @@
1313
const _: () = {
1414
unsafe impl ::zerocopy::TryFromBytes for Foo {
1515
fn only_derive_is_allowed_to_implement_this_trait() {}
16-
fn is_bit_valid(
17-
_candidate: ::zerocopy::Maybe<Self>,
18-
) -> ::zerocopy::util::macro_util::core_reexport::primitive::bool {
16+
#[inline(always)]
17+
fn is_bit_valid<___ZcAlignment>(
18+
_candidate: ::zerocopy::Maybe<'_, Self, ___ZcAlignment>,
19+
) -> ::zerocopy::util::macro_util::core_reexport::primitive::bool
20+
where
21+
___ZcAlignment: ::zerocopy::invariant::Alignment,
22+
{
1923
if false {
2024
fn assert_is_from_bytes<T>()
2125
where

0 commit comments

Comments
 (0)