Skip to content

Commit

Permalink
Enforce C,packed, not just packed, on ULE types (#5049)
Browse files Browse the repository at this point in the history
Fixes #5039



Caused by rust-lang/rust#125360. We were
assuming that `packed` meant `C, packed` already. This is an assumption
I've seen throughout the Rust ecosystem so there may be reasons to
revert.
  • Loading branch information
Manishearth committed Jun 16, 2024
1 parent aa1308a commit 6d65cb5
Show file tree
Hide file tree
Showing 13 changed files with 53 additions and 29 deletions.
2 changes: 1 addition & 1 deletion components/calendar/src/provider/chinese_based.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ impl<'data> ChineseBasedCacheV1<'data> {
derive(databake::Bake),
databake(path = icu_calendar::provider),
)]
#[repr(packed)]
#[repr(C, packed)]
pub struct PackedChineseBasedYearInfo(pub u8, pub u8, pub u8);

impl PackedChineseBasedYearInfo {
Expand Down
2 changes: 1 addition & 1 deletion components/calendar/src/provider/islamic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ impl<'data> IslamicCacheV1<'data> {
databake(path = icu_calendar::provider),
)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize))]
#[repr(packed)]
#[repr(C, packed)]
pub struct PackedIslamicYearInfo(pub u8, pub u8);

impl fmt::Debug for PackedIslamicYearInfo {
Expand Down
2 changes: 1 addition & 1 deletion components/casemap/src/provider/exceptions_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ impl ExceptionHeader {
/// In this struct the RESERVED bit is still allowed to be set, and it will produce a different
/// exception header, but it will not have any other effects.
#[derive(Copy, Clone, PartialEq, Eq, ULE)]
#[repr(packed)]
#[repr(C, packed)]
pub struct ExceptionHeaderULE {
slot_presence: SlotPresence,
bits: ExceptionBitsULE,
Expand Down
2 changes: 1 addition & 1 deletion components/properties/src/provider/bidi_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ pub enum CheckedBidiPairedBracketType {
/// numbers and a byte has 8 bits
/// needed for datagen but not intended for users
#[derive(Copy, Clone, Hash, PartialEq, Eq, Debug)]
#[repr(packed)]
#[repr(C, packed)]
pub struct MirroredPairedBracketDataULE([u8; 3]);

// Safety (based on the safety checklist on the ULE trait):
Expand Down
4 changes: 2 additions & 2 deletions utils/zerovec/derive/examples/derives.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use zerovec::ule::AsULE;
use zerovec::ule::EncodeAsVarULE;
use zerovec::*;

#[repr(packed)]
#[repr(C, packed)]
#[derive(ule::ULE, Copy, Clone)]
pub struct FooULE {
a: u8,
Expand Down Expand Up @@ -40,7 +40,7 @@ impl AsULE for Foo {
}
}

#[repr(packed)]
#[repr(C, packed)]
#[derive(ule::VarULE)]
pub struct RelationULE {
/// This maps to (AndOr, Polarity, Operand),
Expand Down
2 changes: 1 addition & 1 deletion utils/zerovec/derive/src/make_ule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ fn make_ule_enum_impl(
attrs: ZeroVecAttrs,
) -> TokenStream2 {
// We could support more int reprs in the future if needed
if !utils::has_valid_repr(&input.attrs, |r| r == "u8") {
if !utils::ReprInfo::compute(&input.attrs).u8 {
return Error::new(
input.span(),
"#[make_ule] can only be applied to #[repr(u8)] enums",
Expand Down
4 changes: 2 additions & 2 deletions utils/zerovec/derive/src/ule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ use syn::spanned::Spanned;
use syn::{Data, DeriveInput, Error};

pub fn derive_impl(input: &DeriveInput) -> TokenStream2 {
if !utils::has_valid_repr(&input.attrs, |r| r == "packed" || r == "transparent") {
if !utils::ReprInfo::compute(&input.attrs).cpacked_or_transparent() {
return Error::new(
input.span(),
"derive(ULE) must be applied to a #[repr(packed)] or #[repr(transparent)] type",
"derive(ULE) must be applied to a #[repr(C, packed)] or #[repr(transparent)] type",
)
.to_compile_error();
}
Expand Down
42 changes: 33 additions & 9 deletions utils/zerovec/derive/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,38 @@ use syn::punctuated::Punctuated;
use syn::spanned::Spanned;
use syn::{Attribute, Error, Field, Fields, Ident, Index, Result, Token};

// Check that there are repr attributes satisfying the given predicate
pub fn has_valid_repr(attrs: &[Attribute], predicate: impl Fn(&Ident) -> bool + Copy) -> bool {
attrs.iter().filter(|a| a.path().is_ident("repr")).any(|a| {
a.parse_args::<IdentListAttribute>()
.ok()
.and_then(|s| s.idents.iter().find(|s| predicate(s)).map(|_| ()))
.is_some()
})
#[derive(Default)]
pub struct ReprInfo {
pub c: bool,
pub transparent: bool,
pub u8: bool,
pub packed: bool,
}

impl ReprInfo {
pub fn compute(attrs: &[Attribute]) -> Self {
let mut info = ReprInfo::default();
for attr in attrs.iter().filter(|a| a.path().is_ident("repr")) {
if let Ok(pieces) = attr.parse_args::<IdentListAttribute>() {
for piece in pieces.idents.iter() {
if piece == "C" || piece == "c" {
info.c = true;
} else if piece == "transparent" {
info.transparent = true;
} else if piece == "packed" {
info.packed = true;
} else if piece == "u8" {
info.u8 = true;
}
}
}
}
info
}

pub fn cpacked_or_transparent(self) -> bool {
(self.c && self.packed) || self.transparent
}
}

// An attribute that is a list of idents
Expand Down Expand Up @@ -60,7 +84,7 @@ pub fn repr_for(f: &Fields) -> TokenStream2 {
if f.len() == 1 {
quote!(transparent)
} else {
quote!(packed)
quote!(C, packed)
}
}

Expand Down
4 changes: 2 additions & 2 deletions utils/zerovec/derive/src/varule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ pub fn derive_impl(
input: &DeriveInput,
custom_varule_validator: Option<TokenStream2>,
) -> TokenStream2 {
if !utils::has_valid_repr(&input.attrs, |r| r == "packed" || r == "transparent") {
if !utils::ReprInfo::compute(&input.attrs).cpacked_or_transparent() {
return Error::new(
input.span(),
"derive(VarULE) must be applied to a #[repr(packed)] or #[repr(transparent)] type",
"derive(VarULE) must be applied to a #[repr(C, packed)] or #[repr(transparent)] type",
)
.to_compile_error();
}
Expand Down
2 changes: 1 addition & 1 deletion utils/zerovec/src/flexzerovec/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use core::ops::Range;
const USIZE_WIDTH: usize = mem::size_of::<usize>();

/// A zero-copy "slice" that efficiently represents `[usize]`.
#[repr(packed)]
#[repr(C, packed)]
pub struct FlexZeroSlice {
// Hard Invariant: 1 <= width <= USIZE_WIDTH (which is target_pointer_width)
// Soft Invariant: width == the width of the largest element
Expand Down
4 changes: 2 additions & 2 deletions utils/zerovec/src/ule/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ use core::{mem, slice};
/// 6. Acknowledge the following note about the equality invariant.
///
/// If the ULE type is a struct only containing other ULE types (or other types which satisfy invariants 1 and 2,
/// like `[u8; N]`), invariants 1 and 2 can be achieved via `#[repr(packed)]` or `#[repr(transparent)]`.
/// like `[u8; N]`), invariants 1 and 2 can be achieved via `#[repr(C, packed)]` or `#[repr(transparent)]`.
///
/// # Equality invariant
///
Expand Down Expand Up @@ -271,7 +271,7 @@ where
/// 7. Acknowledge the following note about the equality invariant.
///
/// If the ULE type is a struct only containing other ULE/VarULE types (or other types which satisfy invariants 1 and 2,
/// like `[u8; N]`), invariants 1 and 2 can be achieved via `#[repr(packed)]` or `#[repr(transparent)]`.
/// like `[u8; N]`), invariants 1 and 2 can be achieved via `#[repr(C, packed)]` or `#[repr(transparent)]`.
///
/// # Equality invariant
///
Expand Down
6 changes: 3 additions & 3 deletions utils/zerovec/src/ule/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use core::mem::{self, MaybeUninit};
// Invariants:
// The MaybeUninit is zeroed when None (bool = false),
// and is valid when Some (bool = true)
#[repr(packed)]
#[repr(C, packed)]
pub struct OptionULE<U>(bool, MaybeUninit<U>);

impl<U: Copy> OptionULE<U> {
Expand Down Expand Up @@ -62,11 +62,11 @@ impl<U: Copy + core::fmt::Debug> core::fmt::Debug for OptionULE<U> {

// Safety (based on the safety checklist on the ULE trait):
// 1. OptionULE does not include any uninitialized or padding bytes.
// (achieved by `#[repr(packed)]` on a struct containing only ULE fields,
// (achieved by `#[repr(C, packed)]` on a struct containing only ULE fields,
// in the context of this impl. The MaybeUninit is valid for all byte sequences, and we only generate
/// zeroed or valid-T byte sequences to fill it)
// 2. OptionULE is aligned to 1 byte.
// (achieved by `#[repr(packed)]` on a struct containing only ULE fields, in the context of this impl)
// (achieved by `#[repr(C, packed)]` on a struct containing only ULE fields, in the context of this impl)
// 3. The impl of validate_byte_slice() returns an error if any byte is not valid.
// 4. The impl of validate_byte_slice() returns an error if there are extra bytes.
// 5. The other ULE methods use the default impl.
Expand Down
6 changes: 3 additions & 3 deletions utils/zerovec/src/ule/tuple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,15 @@ use core::mem;
macro_rules! tuple_ule {
($name:ident, $len:literal, [ $($t:ident $i:tt),+ ]) => {
#[doc = concat!("ULE type for tuples with ", $len, " elements.")]
#[repr(packed)]
#[repr(C, packed)]
#[allow(clippy::exhaustive_structs)] // stable
pub struct $name<$($t),+>($(pub $t),+);

// Safety (based on the safety checklist on the ULE trait):
// 1. TupleULE does not include any uninitialized or padding bytes.
// (achieved by `#[repr(packed)]` on a struct containing only ULE fields)
// (achieved by `#[repr(C, packed)]` on a struct containing only ULE fields)
// 2. TupleULE is aligned to 1 byte.
// (achieved by `#[repr(packed)]` on a struct containing only ULE fields)
// (achieved by `#[repr(C, packed)]` on a struct containing only ULE fields)
// 3. The impl of validate_byte_slice() returns an error if any byte is not valid.
// 4. The impl of validate_byte_slice() returns an error if there are extra bytes.
// 5. The other ULE methods use the default impl.
Expand Down

0 comments on commit 6d65cb5

Please sign in to comment.