From 516d0b4983ec958ca3884831bc453fa8aa9d4fdc Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Sun, 16 Jun 2024 13:19:50 -0700 Subject: [PATCH] Enforce C,packed, not just packed, on ULE types (#5049) Fixes https://github.com/unicode-org/icu4x/issues/5039 Caused by https://github.com/rust-lang/rust/pull/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. --- .../src/provider/exceptions_builder.rs | 2 +- .../properties/src/provider/bidi_data.rs | 2 +- utils/zerovec/derive/examples/derives.rs | 4 +- utils/zerovec/derive/src/make_ule.rs | 2 +- utils/zerovec/derive/src/ule.rs | 4 +- utils/zerovec/derive/src/utils.rs | 42 +++++++++++++++---- utils/zerovec/derive/src/varule.rs | 4 +- utils/zerovec/src/flexzerovec/slice.rs | 2 +- utils/zerovec/src/ule/mod.rs | 4 +- utils/zerovec/src/ule/option.rs | 6 +-- utils/zerovec/src/ule/tuple.rs | 6 +-- 11 files changed, 51 insertions(+), 27 deletions(-) diff --git a/components/casemap/src/provider/exceptions_builder.rs b/components/casemap/src/provider/exceptions_builder.rs index 80c5c8f1e48..133f92b9925 100644 --- a/components/casemap/src/provider/exceptions_builder.rs +++ b/components/casemap/src/provider/exceptions_builder.rs @@ -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, diff --git a/components/properties/src/provider/bidi_data.rs b/components/properties/src/provider/bidi_data.rs index 465ed4ebb7c..08ba0ce1505 100644 --- a/components/properties/src/provider/bidi_data.rs +++ b/components/properties/src/provider/bidi_data.rs @@ -156,7 +156,7 @@ pub enum CheckedBidiPairedBracketType { #[doc(hidden)] /// 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): diff --git a/utils/zerovec/derive/examples/derives.rs b/utils/zerovec/derive/examples/derives.rs index 40f821023d9..e53c444aaa2 100644 --- a/utils/zerovec/derive/examples/derives.rs +++ b/utils/zerovec/derive/examples/derives.rs @@ -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, @@ -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), diff --git a/utils/zerovec/derive/src/make_ule.rs b/utils/zerovec/derive/src/make_ule.rs index b31913f0886..6bd26f489c3 100644 --- a/utils/zerovec/derive/src/make_ule.rs +++ b/utils/zerovec/derive/src/make_ule.rs @@ -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", diff --git a/utils/zerovec/derive/src/ule.rs b/utils/zerovec/derive/src/ule.rs index 6a03c008f4f..755e66a229b 100644 --- a/utils/zerovec/derive/src/ule.rs +++ b/utils/zerovec/derive/src/ule.rs @@ -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(); } diff --git a/utils/zerovec/derive/src/utils.rs b/utils/zerovec/derive/src/utils.rs index e4155076f72..0b28bf6f717 100644 --- a/utils/zerovec/derive/src/utils.rs +++ b/utils/zerovec/derive/src/utils.rs @@ -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::() - .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::() { + 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 @@ -60,7 +84,7 @@ pub fn repr_for(f: &Fields) -> TokenStream2 { if f.len() == 1 { quote!(transparent) } else { - quote!(packed) + quote!(C, packed) } } diff --git a/utils/zerovec/derive/src/varule.rs b/utils/zerovec/derive/src/varule.rs index 4a586f9547b..82fd702578e 100644 --- a/utils/zerovec/derive/src/varule.rs +++ b/utils/zerovec/derive/src/varule.rs @@ -15,10 +15,10 @@ pub fn derive_impl( input: &DeriveInput, custom_varule_validator: Option, ) -> 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(); } diff --git a/utils/zerovec/src/flexzerovec/slice.rs b/utils/zerovec/src/flexzerovec/slice.rs index 41cb7116f90..8e8f7570646 100644 --- a/utils/zerovec/src/flexzerovec/slice.rs +++ b/utils/zerovec/src/flexzerovec/slice.rs @@ -13,7 +13,7 @@ use core::ops::Range; const USIZE_WIDTH: usize = mem::size_of::(); /// 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 diff --git a/utils/zerovec/src/ule/mod.rs b/utils/zerovec/src/ule/mod.rs index 5a6d9cd4713..757307ada94 100644 --- a/utils/zerovec/src/ule/mod.rs +++ b/utils/zerovec/src/ule/mod.rs @@ -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 /// @@ -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 /// diff --git a/utils/zerovec/src/ule/option.rs b/utils/zerovec/src/ule/option.rs index 9b0dc5b28a1..e8bd0fae498 100644 --- a/utils/zerovec/src/ule/option.rs +++ b/utils/zerovec/src/ule/option.rs @@ -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(bool, MaybeUninit); impl OptionULE { @@ -62,11 +62,11 @@ impl core::fmt::Debug for OptionULE { // 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. diff --git a/utils/zerovec/src/ule/tuple.rs b/utils/zerovec/src/ule/tuple.rs index 3e0f291b3fa..457a10f41b0 100644 --- a/utils/zerovec/src/ule/tuple.rs +++ b/utils/zerovec/src/ule/tuple.rs @@ -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.