Skip to content

Commit

Permalink
Derive trivial is_bit_valid when possible
Browse files Browse the repository at this point in the history
When deriving `FromBytes`, the implied `TryFromBytes` derive's
`is_bit_valid` impl is generated as always returning `true`. This is
faster to codegen, is faster to compile, and is friendlier on the
optimizer.

Makes progress on #5
  • Loading branch information
joshlf committed May 19, 2024
1 parent 3aef801 commit e80cb64
Show file tree
Hide file tree
Showing 6 changed files with 554 additions and 164 deletions.
377 changes: 215 additions & 162 deletions zerocopy-derive/src/lib.rs

Large diffs are not rendered by default.

12 changes: 12 additions & 0 deletions zerocopy-derive/tests/enum_from_bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,18 @@ include!("include.rs");
// `Variant128` has a discriminant of -128) since Rust won't automatically wrap
// a signed discriminant around without you explicitly telling it to.

#[test]
fn test_trivial_is_bit_valid() {
// Since we derive `FromBytes`, the implied `TryFromBytes` derive's
// `is_bit_valid` impl is trivial - it unconditionally returns `true`.
util::test_trivial_is_bit_valid::<FooU8>();
util::test_trivial_is_bit_valid::<FooI8>();
util::test_trivial_is_bit_valid::<FooU8Align>();
util::test_trivial_is_bit_valid::<FooI8Align>();
util::test_trivial_is_bit_valid::<FooU16>();
util::test_trivial_is_bit_valid::<FooI16>();
}

#[derive(imp::FromBytes)]
#[repr(u8)]
enum FooU8 {
Expand Down
269 changes: 269 additions & 0 deletions zerocopy-derive/tests/enum_try_from_bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,3 +176,272 @@ fn test_weird_discriminants() {
<WeirdDiscriminants as imp::TryFromBytes>::try_read_from(&[0xFF; SIZE][..]).is_err()
);
}

#[derive(imp::TryFromBytes)]
#[repr(u8)]
enum FooU8 {
Variant0,
Variant1,
Variant2,
Variant3,
Variant4,
Variant5,
Variant6,
Variant7,
Variant8,
Variant9,
Variant10,
Variant11,
Variant12,
Variant13,
Variant14,
Variant15,
Variant16,
Variant17,
Variant18,
Variant19,
Variant20,
Variant21,
Variant22,
Variant23,
Variant24,
Variant25,
Variant26,
Variant27,
Variant28,
Variant29,
Variant30,
Variant31,
Variant32,
Variant33,
Variant34,
Variant35,
Variant36,
Variant37,
Variant38,
Variant39,
Variant40,
Variant41,
Variant42,
Variant43,
Variant44,
Variant45,
Variant46,
Variant47,
Variant48,
Variant49,
Variant50,
Variant51,
Variant52,
Variant53,
Variant54,
Variant55,
Variant56,
Variant57,
Variant58,
Variant59,
Variant60,
Variant61,
Variant62,
Variant63,
Variant64,
Variant65,
Variant66,
Variant67,
Variant68,
Variant69,
Variant70,
Variant71,
Variant72,
Variant73,
Variant74,
Variant75,
Variant76,
Variant77,
Variant78,
Variant79,
Variant80,
Variant81,
Variant82,
Variant83,
Variant84,
Variant85,
Variant86,
Variant87,
Variant88,
Variant89,
Variant90,
Variant91,
Variant92,
Variant93,
Variant94,
Variant95,
Variant96,
Variant97,
Variant98,
Variant99,
Variant100,
Variant101,
Variant102,
Variant103,
Variant104,
Variant105,
Variant106,
Variant107,
Variant108,
Variant109,
Variant110,
Variant111,
Variant112,
Variant113,
Variant114,
Variant115,
Variant116,
Variant117,
Variant118,
Variant119,
Variant120,
Variant121,
Variant122,
Variant123,
Variant124,
Variant125,
Variant126,
Variant127,
Variant128,
Variant129,
Variant130,
Variant131,
Variant132,
Variant133,
Variant134,
Variant135,
Variant136,
Variant137,
Variant138,
Variant139,
Variant140,
Variant141,
Variant142,
Variant143,
Variant144,
Variant145,
Variant146,
Variant147,
Variant148,
Variant149,
Variant150,
Variant151,
Variant152,
Variant153,
Variant154,
Variant155,
Variant156,
Variant157,
Variant158,
Variant159,
Variant160,
Variant161,
Variant162,
Variant163,
Variant164,
Variant165,
Variant166,
Variant167,
Variant168,
Variant169,
Variant170,
Variant171,
Variant172,
Variant173,
Variant174,
Variant175,
Variant176,
Variant177,
Variant178,
Variant179,
Variant180,
Variant181,
Variant182,
Variant183,
Variant184,
Variant185,
Variant186,
Variant187,
Variant188,
Variant189,
Variant190,
Variant191,
Variant192,
Variant193,
Variant194,
Variant195,
Variant196,
Variant197,
Variant198,
Variant199,
Variant200,
Variant201,
Variant202,
Variant203,
Variant204,
Variant205,
Variant206,
Variant207,
Variant208,
Variant209,
Variant210,
Variant211,
Variant212,
Variant213,
Variant214,
Variant215,
Variant216,
Variant217,
Variant218,
Variant219,
Variant220,
Variant221,
Variant222,
Variant223,
Variant224,
Variant225,
Variant226,
Variant227,
Variant228,
Variant229,
Variant230,
Variant231,
Variant232,
Variant233,
Variant234,
Variant235,
Variant236,
Variant237,
Variant238,
Variant239,
Variant240,
Variant241,
Variant242,
Variant243,
Variant244,
Variant245,
Variant246,
Variant247,
Variant248,
Variant249,
Variant250,
Variant251,
Variant252,
Variant253,
Variant254,
Variant255,
}

#[test]
fn test_trivial_is_bit_valid() {
// Though we don't derive `FromBytes`, `FooU8` *could* soundly implement
// `FromBytes`. Therefore, `TryFromBytes` derive's `is_bit_valid` impl is
// trivial - it unconditionally returns `true`.
util::test_trivial_is_bit_valid::<FooU8>();
}
50 changes: 48 additions & 2 deletions zerocopy-derive/tests/include.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,14 @@ mod imp {
#[allow(unused)]
pub use {
::core::{
assert_eq, cell::UnsafeCell, convert::TryFrom, marker::PhantomData, mem::ManuallyDrop,
option::IntoIter, prelude::v1::*, primitive::*,
assert_eq,
cell::UnsafeCell,
convert::TryFrom,
marker::PhantomData,
mem::{ManuallyDrop, MaybeUninit},
option::IntoIter,
prelude::v1::*,
primitive::*,
},
::std::prelude::v1::*,
::zerocopy::*,
Expand Down Expand Up @@ -80,4 +86,44 @@ pub mod util {
};
};
}

#[macro_export]
macro_rules! test_trivial_is_bit_valid {
($x:ty => $name:ident) => {
#[test]
fn $name() {
util::test_trivial_is_bit_valid::<$x>();
}
};
}

// Under some circumstances, our `TryFromBytes` derive generates a trivial
// `is_bit_valid` impl that unconditionally returns `true`. This test
// attempts to validate that this is, indeed, the behavior of our
// `TryFromBytes` derive. It is not foolproof, but is likely to catch some
// mistakes.
//
// As of this writing, this happens in two cases:
// - When deriving `TryFromBytes` thanks to a top-level
// `#[derive(FromBytes)]`
// - When deriving `TryFromBytes` on an enum type that *could* soundly
// implement `FromBytes`
pub fn test_trivial_is_bit_valid<T: super::imp::TryFromBytes>() {
// This test works based on the insight that a trivial `is_bit_valid`
// impl should never load any bytes from memory. Thus, while it is
// technically a violation of `is_bit_valid`'s safety precondition to
// pass a pointer to uninitialized memory, the `is_bit_valid` impl we
// expect our derives to generate should never touch this memory, and
// thus should never exhibit UB. By contrast, if our derives are
// spuriously generating non-trivial `is_bit_valid` impls, this should
// cause UB which may be caught by Miri.

let buf = super::imp::MaybeUninit::<T>::uninit();
let ptr = super::imp::Ptr::from_ref(&buf);
// SAFETY: TODO
let ptr = unsafe { ptr.cast_unsized(|p| p as *mut T) };
// SAFETY: This is intentionally unsound; see the preceding comment.
let ptr = unsafe { ptr.assume_initialized() };
assert!(<T as super::imp::TryFromBytes>::is_bit_valid(ptr));
}
}
5 changes: 5 additions & 0 deletions zerocopy-derive/tests/struct_from_bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@ include!("include.rs");
struct Zst;

util_assert_impl_all!(Zst: imp::FromBytes);
test_trivial_is_bit_valid!(Zst => test_zst_trivial_is_bit_valid);

#[derive(imp::FromBytes)]
struct One {
a: u8,
}

util_assert_impl_all!(One: imp::FromBytes);
test_trivial_is_bit_valid!(One => test_one_trivial_is_bit_valid);

#[derive(imp::FromBytes)]
struct Two {
Expand All @@ -34,6 +36,7 @@ struct Two {
}

util_assert_impl_all!(Two: imp::FromBytes);
test_trivial_is_bit_valid!(Two => test_two_trivial_is_bit_valid);

#[derive(imp::FromBytes)]
struct Unsized {
Expand All @@ -55,6 +58,7 @@ struct TypeParams<'a, T: ?imp::Sized, I: imp::Iterator> {
util_assert_impl_all!(TypeParams<'static, (), imp::IntoIter<()>>: imp::FromBytes);
util_assert_impl_all!(TypeParams<'static, util::AU16, imp::IntoIter<()>>: imp::FromBytes);
util_assert_impl_all!(TypeParams<'static, [util::AU16], imp::IntoIter<()>>: imp::FromBytes);
test_trivial_is_bit_valid!(TypeParams<'static, (), imp::IntoIter<()>> => test_type_params_trivial_is_bit_valid);

// Deriving `FromBytes` should work if the struct has bounded parameters.

Expand All @@ -70,3 +74,4 @@ where
T: 'a + 'b + imp::FromBytes;

util_assert_impl_all!(WithParams<'static, 'static, u8, 42>: imp::FromBytes);
test_trivial_is_bit_valid!(WithParams<'static, 'static, u8, 42> => test_with_params_trivial_is_bit_valid);

0 comments on commit e80cb64

Please sign in to comment.