🐛 Fix bounds check for signed types #24

Coincidentally, this also fixes #22
wrenger · Nov 11, 2023 · 8de2078 · 8de2078
1 parent 61c775b
commit 8de2078
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 24 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "bitfield-struct"
-version = "0.5.4"
+version = "0.5.5"
 edition = "2021"
 authors = ["Lars Wrenger <lars@wrenger.net>"]
 description = "Struct-like procedural macro for bitfields."

diff --git a/src/lib.rs b/src/lib.rs
@@ -449,7 +449,7 @@ impl Member {
             mut default,
             into,
             from,
-        } = parse_field(&attrs, &ty, ignore)?;
+        } = parse_field(&base_ty, &attrs, &ty, ignore)?;
 
         if bits > 0 && !ignore {
             if offset + bits > base_bits {
@@ -540,8 +540,17 @@ impl ToTokens for Member {
             bits,
             base_ty,
             default: _,
-            inner: Some(MemberInner { ident, ty, attrs, vis, into, from }),
-        } = self else {
+            inner:
+                Some(MemberInner {
+                    ident,
+                    ty,
+                    attrs,
+                    vis,
+                    into,
+                    from,
+                }),
+        } = self
+        else {
             return Default::default();
         };
 
@@ -621,7 +630,12 @@ struct Field {
 }
 
 /// Parses the `bits` attribute that allows specifying a custom number of bits.
-fn parse_field(attrs: &[syn::Attribute], ty: &syn::Type, ignore: bool) -> syn::Result<Field> {
+fn parse_field(
+    base_ty: &syn::Type,
+    attrs: &[syn::Attribute],
+    ty: &syn::Type,
+    ignore: bool,
+) -> syn::Result<Field> {
     fn malformed(mut e: syn::Error, attr: &syn::Attribute) -> syn::Error {
         e.combine(syn::Error::new(attr.span(), "malformed #[bits] attribute"));
         e
@@ -641,8 +655,8 @@ fn parse_field(attrs: &[syn::Attribute], ty: &syn::Type, ignore: bool) -> syn::R
             bits: ty_bits,
             ty: ty.clone(),
             default: quote!(0),
-            into: TokenStream::new(),
-            from: TokenStream::new(),
+            into: quote!(),
+            from: quote!(),
         },
         TypeClass::UInt => Field {
             bits: ty_bits,
@@ -654,7 +668,7 @@ fn parse_field(attrs: &[syn::Attribute], ty: &syn::Type, ignore: bool) -> syn::R
         TypeClass::Other => Field {
             bits: ty_bits,
             ty: ty.clone(),
-            default: TokenStream::new(),
+            default: quote!(),
             into: quote!(#ty::into_bits(this)),
             from: quote!(#ty::from_bits(this)),
         },
@@ -663,11 +677,12 @@ fn parse_field(attrs: &[syn::Attribute], ty: &syn::Type, ignore: bool) -> syn::R
     // Find and parse the bits attribute
     for attr in attrs {
         let syn::Attribute {
-                style: syn::AttrStyle::Outer,
-                meta: syn::Meta::List(syn::MetaList { path, tokens, .. }),
-                ..
-        } = attr else {
-            continue
+            style: syn::AttrStyle::Outer,
+            meta: syn::Meta::List(syn::MetaList { path, tokens, .. }),
+            ..
+        } = attr
+        else {
+            continue;
         };
         if path.is_ident("bits") {
             let span = tokens.span();
@@ -678,6 +693,7 @@ fn parse_field(attrs: &[syn::Attribute], ty: &syn::Type, ignore: bool) -> syn::R
                 from,
             } = syn::parse2(tokens.clone()).map_err(|e| malformed(e, attr))?;
 
+            // bit size
             if let Some(bits) = bits {
                 if bits == 0 {
                     return Err(syn::Error::new(span, "bits cannot bit 0"));
@@ -687,13 +703,15 @@ fn parse_field(attrs: &[syn::Attribute], ty: &syn::Type, ignore: bool) -> syn::R
                 }
                 ret.bits = bits;
             }
+            // padding
             if ignore && (into.is_some() || from.is_some()) {
                 return Err(syn::Error::new(
                     default.span(),
                     "'into' and 'from' are not supported on padding",
                 ));
             }
 
+            // conversion
             if let Some(into) = into {
                 ret.into = quote!(#into(this));
             }
@@ -726,9 +744,11 @@ fn parse_field(attrs: &[syn::Attribute], ty: &syn::Type, ignore: bool) -> syn::R
         if ret.into.is_empty() {
             // Bounds check and remove leading ones from negative values
             ret.into = quote! {{
-                #[allow(unused_comparisons)]
-                debug_assert!(if this >= 0 { this & !#mask == 0 } else { !this & !#mask == 0 }, "value out of bounds");
-                (this & #mask) as _
+                debug_assert!({
+                    let m = #ty::MIN >> (#ty::BITS - #bits);
+                    m <= this && this <= -(m + 1)
+                });
+                (this as #base_ty & #mask)
             }};
         }
         if ret.from.is_empty() {
@@ -857,7 +877,7 @@ impl Parse for Params {
 
 /// Returns the number of bits for a given type
 fn type_bits(ty: &syn::Type) -> (TypeClass, usize) {
-    let syn::Type::Path(syn::TypePath{ path, .. }) = ty else {
+    let syn::Type::Path(syn::TypePath { path, .. }) = ty else {
         return (TypeClass::Other, 0);
     };
     let Some(ident) = path.get_ident() else {

diff --git a/tests/test.rs b/tests/test.rs
@@ -163,6 +163,24 @@ fn negative() {
     assert_eq!(v.negative(), -4);
 }
 
+#[test]
+fn negative_signed() {
+    #[bitfield(u64)]
+    struct MyBitfield {
+        negative: i32,
+        #[bits(10)]
+        positive: u16,
+        #[bits(22)]
+        __: u32,
+    }
+
+    let val = MyBitfield::new()
+        .with_negative(-1)
+        .with_positive(0b11_1111_1111);
+    assert_eq!(val.negative(), -1);
+    assert_eq!(val.positive(), 0b11_1111_1111);
+}
+
 #[test]
 fn entirely_negative() {
     #[bitfield(u32)]