fixup! [wip] Rework how bitfields are handled.

Author: emilio

src/codegen/mod.rs

@@ -2,7 +2,8 @@ mod helpers;
 mod struct_layout;
 
 use self::helpers::{BlobTyBuilder, attributes};
-use self::struct_layout::{align_to, bytes_from_bits, StructLayoutTracker};
+use self::struct_layout::{align_to, bytes_from_bits};
+use self::struct_layout::{bytes_from_bits_pow2, StructLayoutTracker};
 use aster;
 
 use ir::annotations::FieldAccessorKind;
@@ -767,8 +768,8 @@ impl<'a> Bitfield<'a> {
                 field_size_in_bits = align_to(field_size_in_bits, field_align);
                 // Push the new field.
                 let ty =
-                    BlobTyBuilder::new(Layout::new(bytes_from_bits(field_size_in_bits),
-                                                   bytes_from_bits(last_field_align)))
+                    BlobTyBuilder::new(Layout::new(bytes_from_bits_pow2(field_size_in_bits),
+                                                   bytes_from_bits_pow2(last_field_align)))
                         .build();
 
                 let field = StructFieldBuilder::named(&last_field_name)
@@ -810,8 +811,8 @@ impl<'a> Bitfield<'a> {
         if field_size_in_bits != 0 {
             // Push the last field.
             let ty =
-                BlobTyBuilder::new(Layout::new(bytes_from_bits(field_size_in_bits),
-                                               bytes_from_bits(last_field_align)))
+                BlobTyBuilder::new(Layout::new(bytes_from_bits_pow2(field_size_in_bits),
+                                               bytes_from_bits_pow2(last_field_align)))
                     .build();
 
             let field = StructFieldBuilder::named(&last_field_name)

src/codegen/struct_layout.rs

@@ -21,6 +21,7 @@ pub struct StructLayoutTracker<'a, 'ctx: 'a> {
     padding_count: usize,
     latest_field_layout: Option<Layout>,
     max_field_align: usize,
+    last_field_was_bitfield: bool,
 }
 
 /// Returns a size aligned to a given value.
@@ -37,8 +38,17 @@ pub fn align_to(size: usize, align: usize) -> usize {
     size + align - rem
 }
 
+/// Returns the amount of bytes from a given amount of bytes, rounding up.
+pub fn bytes_from_bits(n: usize) -> usize {
+    if n % 8 == 0 {
+        return n / 8;
+    }
+
+    n / 8 + 1
+}
+
 /// Returns the lower power of two byte count that can hold at most n bits.
-pub fn bytes_from_bits(mut n: usize) -> usize {
+pub fn bytes_from_bits_pow2(mut n: usize) -> usize {
     if n == 0 {
         return 0;
     }
@@ -63,6 +73,20 @@ fn test_align_to() {
     assert_eq!(align_to(17, 4), 20);
 }
 
+#[test]
+fn test_bytes_from_bits_pow2() {
+    assert_eq!(bytes_from_bits_pow2(0), 0);
+    for i in 1..9 {
+        assert_eq!(bytes_from_bits_pow2(i), 1);
+    }
+    for i in 9..17 {
+        assert_eq!(bytes_from_bits_pow2(i), 2);
+    }
+    for i in 17..33 {
+        assert_eq!(bytes_from_bits_pow2(i), 4);
+    }
+}
+
 #[test]
 fn test_bytes_from_bits() {
     assert_eq!(bytes_from_bits(0), 0);
@@ -72,8 +96,8 @@ fn test_bytes_from_bits() {
     for i in 9..17 {
         assert_eq!(bytes_from_bits(i), 2);
     }
-    for i in 17..33 {
-        assert_eq!(bytes_from_bits(i), 4);
+    for i in 17..25 {
+        assert_eq!(bytes_from_bits(i), 3);
     }
 }
 
@@ -86,6 +110,7 @@ impl<'a, 'ctx> StructLayoutTracker<'a, 'ctx> {
             padding_count: 0,
             latest_field_layout: None,
             max_field_align: 0,
+            last_field_was_bitfield: false,
         }
     }
 
@@ -97,26 +122,33 @@ impl<'a, 'ctx> StructLayoutTracker<'a, 'ctx> {
     }
 
     pub fn saw_base(&mut self, base_ty: &Type) {
-        self.align_to_latest_field();
-
         if let Some(layout) = base_ty.layout(self.ctx) {
+            self.align_to_latest_field(layout);
+
             self.latest_offset += self.padding_bytes(layout) + layout.size;
             self.latest_field_layout = Some(layout);
             self.max_field_align = cmp::max(self.max_field_align, layout.align);
         }
     }
+
     pub fn saw_bitfield_batch(&mut self, layout: Layout) {
-        self.align_to_latest_field();
+        self.align_to_latest_field(layout);
+
+        self.latest_offset += layout.size;
+
+        debug!("Offset: <bitfield>: {} -> {}",
+               self.latest_offset - layout.size,
+               self.latest_offset);
 
-        self.latest_offset += self.padding_bytes(layout) + layout.size;
         self.latest_field_layout = Some(layout);
+        self.last_field_was_bitfield = true;
         // NB: We intentionally don't update the max_field_align here, since our
         // bitfields code doesn't necessarily guarantee it, so we need to
         // actually generate the dummy alignment.
     }
 
     pub fn saw_union(&mut self, layout: Layout) {
-        self.align_to_latest_field();
+        self.align_to_latest_field(layout);
 
         self.latest_offset += self.padding_bytes(layout) + layout.size;
         self.latest_field_layout = Some(layout);
@@ -151,7 +183,7 @@ impl<'a, 'ctx> StructLayoutTracker<'a, 'ctx> {
             }
         }
 
-        self.align_to_latest_field();
+        let will_merge_with_bitfield = self.align_to_latest_field(field_layout);
 
         let padding_layout = if self.comp.packed() {
             None
@@ -160,17 +192,19 @@ impl<'a, 'ctx> StructLayoutTracker<'a, 'ctx> {
                 Some(offset) if offset / 8 > self.latest_offset => {
                     offset / 8 - self.latest_offset
                 }
-                _ if field_layout.align == 0 => 0,
-                _ => {
-                    self.padding_bytes(field_layout)
-                }
+                _ if will_merge_with_bitfield || field_layout.align == 0 => 0,
+                _ => self.padding_bytes(field_layout),
             };
 
             // Otherwise the padding is useless.
             let need_padding = padding_bytes >= field_layout.align;
 
             self.latest_offset += padding_bytes;
 
+            debug!("Offset: <padding>: {} -> {}",
+                   self.latest_offset - padding_bytes,
+                   self.latest_offset);
+
             debug!("align field {} to {}/{} with {} padding bytes {:?}",
                 field_name,
                 self.latest_offset,
@@ -188,14 +222,22 @@ impl<'a, 'ctx> StructLayoutTracker<'a, 'ctx> {
         self.latest_offset += field_layout.size;
         self.latest_field_layout = Some(field_layout);
         self.max_field_align = cmp::max(self.max_field_align, field_layout.align);
+        self.last_field_was_bitfield = false;
+
+        debug!("Offset: {}: {} -> {}",
+               field_name,
+               self.latest_offset - field_layout.size,
+               self.latest_offset);
 
         padding_layout.map(|layout| self.padding_field(layout))
     }
 
     pub fn pad_struct(&mut self, layout: Layout) -> Option<ast::StructField> {
         if layout.size < self.latest_offset {
-            warn!("calculate struct layout incorrect, too more {} bytes",
-                  self.latest_offset - layout.size);
+            if cfg!(debug_assertions) {
+                panic!("calculate struct layout incorrect, too more {} bytes",
+                       self.latest_offset - layout.size);
+            }
 
             return None
         }
@@ -204,12 +246,19 @@ impl<'a, 'ctx> StructLayoutTracker<'a, 'ctx> {
 
         // We always pad to get to the correct size if the struct is one of
         // those we can't align properly.
+        //
+        // Note that if the last field we saw was a bitfield, we may need to pad
+        // regardless, because bitfields don't respect alignment as strictly as
+        // other fields.
         if padding_bytes > 0 &&
             (padding_bytes >= layout.align ||
+             (self.last_field_was_bitfield &&
+                padding_bytes >= self.latest_field_layout.unwrap().align) ||
              layout.align > mem::size_of::<*mut ()>()) {
             let layout = if self.comp.packed() {
                 Layout::new(padding_bytes, 1)
-            } else if layout.align > mem::size_of::<*mut ()>() {
+            } else if self.last_field_was_bitfield ||
+                      layout.align > mem::size_of::<*mut ()>() {
                 // We've already given up on alignment here.
                 Layout::for_size(padding_bytes)
             } else {
@@ -252,11 +301,37 @@ impl<'a, 'ctx> StructLayoutTracker<'a, 'ctx> {
         StructFieldBuilder::named(padding_field_name).pub_().build_ty(ty)
     }
 
-    fn align_to_latest_field(&mut self) {
+    /// Returns whether the new field is known to merge with a bitfield.
+    ///
+    /// This is just to avoid doing the same check also in pad_field.
+    fn align_to_latest_field(&mut self, new_field_layout: Layout) -> bool {
         if self.comp.packed() {
-            // skip to align field when packed
-        } else if let Some(layout) = self.latest_field_layout.take() {
-            self.latest_offset += self.padding_bytes(layout);
+            // Skip to align fields when packed.
+            return false;
         }
+
+        let layout = match self.latest_field_layout {
+            Some(l) => l,
+            None => return false,
+        };
+
+        // If it was, we may or may not need to align, depending on what the
+        // current field alignment and the bitfield size and alignment are.
+        debug!("align_to_bitfield? {}: {:?} {:?}", self.last_field_was_bitfield,
+               layout, new_field_layout);
+
+        if self.last_field_was_bitfield &&
+            new_field_layout.align <= layout.size % layout.align &&
+            new_field_layout.size <= layout.size % layout.align {
+            // The new field will be coalesced into some of the remaining bits.
+            //
+            // FIXME(emilio): I think this may not catch everything?
+            debug!("Will merge with bitfield");
+            return true;
+        }
+
+        // Else, just align the obvious way.
+        self.latest_offset += self.padding_bytes(layout);
+        return false;
     }
 }

tests/expectations/tests/bitfield_align.rs

@@ -72,3 +72,43 @@ fn bindgen_test_layout_C() {
 impl Clone for C {
     fn clone(&self) -> Self { *self }
 }
+#[repr(C)]
+#[derive(Debug, Default, Copy)]
+pub struct Date1 {
+    pub _bitfield_1: [u8; 2usize],
+    pub _bitfield_2: u8,
+    pub __bindgen_align: [u16; 0usize],
+}
+#[test]
+fn bindgen_test_layout_Date1() {
+    assert_eq!(::std::mem::size_of::<Date1>() , 4usize , concat ! (
+               "Size of: " , stringify ! ( Date1 ) ));
+    assert_eq! (::std::mem::align_of::<Date1>() , 2usize , concat ! (
+                "Alignment of " , stringify ! ( Date1 ) ));
+}
+impl Clone for Date1 {
+    fn clone(&self) -> Self { *self }
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy)]
+pub struct Date2 {
+    pub _bitfield_1: [u8; 2usize],
+    pub _bitfield_2: u8,
+    pub byte: ::std::os::raw::c_uchar,
+    pub __bindgen_align: [u16; 0usize],
+}
+#[test]
+fn bindgen_test_layout_Date2() {
+    assert_eq!(::std::mem::size_of::<Date2>() , 4usize , concat ! (
+               "Size of: " , stringify ! ( Date2 ) ));
+    assert_eq! (::std::mem::align_of::<Date2>() , 2usize , concat ! (
+                "Alignment of " , stringify ! ( Date2 ) ));
+    assert_eq! (unsafe {
+                & ( * ( 0 as * const Date2 ) ) . byte as * const _ as usize }
+                , 3usize , concat ! (
+                "Alignment of field: " , stringify ! ( Date2 ) , "::" ,
+                stringify ! ( byte ) ));
+}
+impl Clone for Date2 {
+    fn clone(&self) -> Self { *self }
+}

tests/expectations/tests/layout_align.rs

@@ -71,6 +71,7 @@ pub struct rte_eth_link {
     /**< ETH_SPEED_NUM_ */
     pub link_speed: u32,
     pub _bitfield_1: u8,
+    pub __bindgen_padding_0: [u8; 3usize],
     pub __bindgen_align: [u64; 0usize],
 }
 #[test]

tests/expectations/tests/layout_eth_conf.rs

@@ -140,6 +140,7 @@ pub struct rte_eth_txmode {
     pub mq_mode: rte_eth_tx_mq_mode,
     pub pvid: u16,
     pub _bitfield_1: u8,
+    pub __bindgen_padding_0: u8,
 }
 #[test]
 fn bindgen_test_layout_rte_eth_txmode() {

tests/headers/bitfield_align.h

@@ -24,3 +24,18 @@ struct C {
   unsigned b2 : 1;
   unsigned baz;
 };
+
+struct Date1 {
+   unsigned short nWeekDay  : 3;    // 0..7   (3 bits)
+   unsigned short nMonthDay : 6;    // 0..31  (6 bits)
+   unsigned short nMonth    : 5;    // 0..12  (5 bits)
+   unsigned short nYear     : 8;    // 0..100 (8 bits)
+};
+
+struct Date2 {
+   unsigned short nWeekDay  : 3;    // 0..7   (3 bits)
+   unsigned short nMonthDay : 6;    // 0..31  (6 bits)
+   unsigned short nMonth    : 5;    // 0..12  (5 bits)
+   unsigned short nYear     : 8;    // 0..100 (8 bits)
+   unsigned char byte;
+};