From 71e43ed02e5c971875b1fec3faba989a57f32fe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 15 Feb 2017 23:49:58 +0100 Subject: [PATCH] fixup! [wip] Rework how bitfields are handled. --- src/codegen/mod.rs | 11 +- src/codegen/struct_layout.rs | 115 ++++++++++++++++---- tests/expectations/tests/bitfield_align.rs | 40 +++++++ tests/expectations/tests/layout_align.rs | 1 + tests/expectations/tests/layout_eth_conf.rs | 1 + tests/headers/bitfield_align.h | 15 +++ 6 files changed, 158 insertions(+), 25 deletions(-) diff --git a/src/codegen/mod.rs b/src/codegen/mod.rs index a3d292c9dd..e6096d9c21 100644 --- a/src/codegen/mod.rs +++ b/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) diff --git a/src/codegen/struct_layout.rs b/src/codegen/struct_layout.rs index f5d019ab8c..cc84a93b13 100644 --- a/src/codegen/struct_layout.rs +++ b/src/codegen/struct_layout.rs @@ -21,6 +21,7 @@ pub struct StructLayoutTracker<'a, 'ctx: 'a> { padding_count: usize, latest_field_layout: Option, 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: : {} -> {}", + 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,10 +192,8 @@ 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. @@ -171,6 +201,10 @@ impl<'a, 'ctx> StructLayoutTracker<'a, 'ctx> { self.latest_offset += padding_bytes; + debug!("Offset: : {} -> {}", + 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 { 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; } } diff --git a/tests/expectations/tests/bitfield_align.rs b/tests/expectations/tests/bitfield_align.rs index eb5027d902..ffc170b1bd 100644 --- a/tests/expectations/tests/bitfield_align.rs +++ b/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::() , 4usize , concat ! ( + "Size of: " , stringify ! ( Date1 ) )); + assert_eq! (::std::mem::align_of::() , 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::() , 4usize , concat ! ( + "Size of: " , stringify ! ( Date2 ) )); + assert_eq! (::std::mem::align_of::() , 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 } +} diff --git a/tests/expectations/tests/layout_align.rs b/tests/expectations/tests/layout_align.rs index 34010300ca..9085480caa 100644 --- a/tests/expectations/tests/layout_align.rs +++ b/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] diff --git a/tests/expectations/tests/layout_eth_conf.rs b/tests/expectations/tests/layout_eth_conf.rs index 92dcd6c8d6..ae46f5c6be 100644 --- a/tests/expectations/tests/layout_eth_conf.rs +++ b/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() { diff --git a/tests/headers/bitfield_align.h b/tests/headers/bitfield_align.h index 571876546a..82b5309937 100644 --- a/tests/headers/bitfield_align.h +++ b/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; +};