From 14a8d29baa1da364c6f33de42c4048ed573b06ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 21 Jun 2021 13:47:34 +0200 Subject: [PATCH] comp: Do a better effort of computing packedness before bitfield units. Fixes #2067 --- src/ir/comp.rs | 88 +++++--- src/ir/context.rs | 9 +- .../tests/bitfield_pragma_packed.rs | 211 ++++++++++++++++++ tests/headers/bitfield_pragma_packed.h | 9 + 4 files changed, 283 insertions(+), 34 deletions(-) create mode 100644 tests/expectations/tests/bitfield_pragma_packed.rs create mode 100644 tests/headers/bitfield_pragma_packed.h diff --git a/src/ir/comp.rs b/src/ir/comp.rs index d57c272a30..e554f9a89c 100644 --- a/src/ir/comp.rs +++ b/src/ir/comp.rs @@ -1113,21 +1113,17 @@ impl CompInfo { } // empty union case - if self.fields().is_empty() { + if !self.has_fields() { return None; } let mut max_size = 0; // Don't allow align(0) let mut max_align = 1; - for field in self.fields() { - let field_layout = field.layout(ctx); - - if let Some(layout) = field_layout { - max_size = cmp::max(max_size, layout.size); - max_align = cmp::max(max_align, layout.align); - } - } + self.each_known_field_layout(ctx, |layout| { + max_size = cmp::max(max_size, layout.size); + max_align = cmp::max(max_align, layout.align); + }); Some(Layout::new(max_size, max_align)) } @@ -1139,12 +1135,49 @@ impl CompInfo { CompFields::AfterComputingBitfieldUnits { ref fields, .. } => { fields } - CompFields::BeforeComputingBitfieldUnits(_) => { + CompFields::BeforeComputingBitfieldUnits(..) => { panic!("Should always have computed bitfield units first"); } } } + fn has_fields(&self) -> bool { + match self.fields { + CompFields::ErrorComputingBitfieldUnits => false, + CompFields::AfterComputingBitfieldUnits { ref fields, .. } => { + !fields.is_empty() + } + CompFields::BeforeComputingBitfieldUnits(ref raw_fields) => { + !raw_fields.is_empty() + } + } + } + + fn each_known_field_layout( + &self, + ctx: &BindgenContext, + mut callback: impl FnMut(Layout), + ) { + match self.fields { + CompFields::ErrorComputingBitfieldUnits => return, + CompFields::AfterComputingBitfieldUnits { ref fields, .. } => { + for field in fields.iter() { + if let Some(layout) = field.layout(ctx) { + callback(layout); + } + } + } + CompFields::BeforeComputingBitfieldUnits(ref raw_fields) => { + for field in raw_fields.iter() { + let field_ty = ctx.resolve_type(field.0.ty); + if let Some(layout) = field_ty.layout(ctx) { + callback(layout); + } + } + } + } + } + fn has_bitfields(&self) -> bool { match self.fields { CompFields::ErrorComputingBitfieldUnits => false, @@ -1598,23 +1631,17 @@ impl CompInfo { // Even though `libclang` doesn't expose `#pragma packed(...)`, we can // detect it through its effects. if let Some(parent_layout) = layout { - if self.fields().iter().any(|f| match *f { - Field::Bitfields(ref unit) => { - unit.layout().align > parent_layout.align - } - Field::DataMember(ref data) => { - let field_ty = ctx.resolve_type(data.ty()); - field_ty.layout(ctx).map_or(false, |field_ty_layout| { - field_ty_layout.align > parent_layout.align - }) - } - }) { + let mut packed = false; + self.each_known_field_layout(ctx, |layout| { + packed = packed || layout.align > parent_layout.align; + }); + if packed { info!("Found a struct that was defined within `#pragma packed(...)`"); return true; - } else if self.has_own_virtual_method { - if parent_layout.align == 1 { - return true; - } + } + + if self.has_own_virtual_method && parent_layout.align == 1 { + return true; } } @@ -1627,10 +1654,13 @@ impl CompInfo { } /// Compute this compound structure's bitfield allocation units. - pub fn compute_bitfield_units(&mut self, ctx: &BindgenContext) { - // TODO(emilio): If we could detect #pragma packed here we'd fix layout - // tests in divide-by-zero-in-struct-layout.rs - self.fields.compute_bitfield_units(ctx, self.packed_attr) + pub fn compute_bitfield_units( + &mut self, + ctx: &BindgenContext, + layout: Option<&Layout>, + ) { + let packed = self.is_packed(ctx, layout); + self.fields.compute_bitfield_units(ctx, packed) } /// Assign for each anonymous field a generated name. diff --git a/src/ir/context.rs b/src/ir/context.rs index 4faf6cd123..72ce06b10c 100644 --- a/src/ir/context.rs +++ b/src/ir/context.rs @@ -981,12 +981,11 @@ If you encounter an error missing from this list, please file an issue or a PR!" mem::replace(&mut self.need_bitfield_allocation, vec![]); for id in need_bitfield_allocation { self.with_loaned_item(id, |ctx, item| { - item.kind_mut() - .as_type_mut() - .unwrap() - .as_comp_mut() + let ty = item.kind_mut().as_type_mut().unwrap(); + let layout = ty.layout(ctx); + ty.as_comp_mut() .unwrap() - .compute_bitfield_units(ctx); + .compute_bitfield_units(ctx, layout.as_ref()); }); } } diff --git a/tests/expectations/tests/bitfield_pragma_packed.rs b/tests/expectations/tests/bitfield_pragma_packed.rs new file mode 100644 index 0000000000..27cd90f140 --- /dev/null +++ b/tests/expectations/tests/bitfield_pragma_packed.rs @@ -0,0 +1,211 @@ +#![allow( + dead_code, + non_snake_case, + non_camel_case_types, + non_upper_case_globals +)] + +#[repr(C)] +#[derive(Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)] +pub struct __BindgenBitfieldUnit { + storage: Storage, +} +impl __BindgenBitfieldUnit { + #[inline] + pub const fn new(storage: Storage) -> Self { + Self { storage } + } +} +impl __BindgenBitfieldUnit +where + Storage: AsRef<[u8]> + AsMut<[u8]>, +{ + #[inline] + pub fn get_bit(&self, index: usize) -> bool { + debug_assert!(index / 8 < self.storage.as_ref().len()); + let byte_index = index / 8; + let byte = self.storage.as_ref()[byte_index]; + let bit_index = if cfg!(target_endian = "big") { + 7 - (index % 8) + } else { + index % 8 + }; + let mask = 1 << bit_index; + byte & mask == mask + } + #[inline] + pub fn set_bit(&mut self, index: usize, val: bool) { + debug_assert!(index / 8 < self.storage.as_ref().len()); + let byte_index = index / 8; + let byte = &mut self.storage.as_mut()[byte_index]; + let bit_index = if cfg!(target_endian = "big") { + 7 - (index % 8) + } else { + index % 8 + }; + let mask = 1 << bit_index; + if val { + *byte |= mask; + } else { + *byte &= !mask; + } + } + #[inline] + pub fn get(&self, bit_offset: usize, bit_width: u8) -> u64 { + debug_assert!(bit_width <= 64); + debug_assert!(bit_offset / 8 < self.storage.as_ref().len()); + debug_assert!( + (bit_offset + (bit_width as usize)) / 8 <= + self.storage.as_ref().len() + ); + let mut val = 0; + for i in 0..(bit_width as usize) { + if self.get_bit(i + bit_offset) { + let index = if cfg!(target_endian = "big") { + bit_width as usize - 1 - i + } else { + i + }; + val |= 1 << index; + } + } + val + } + #[inline] + pub fn set(&mut self, bit_offset: usize, bit_width: u8, val: u64) { + debug_assert!(bit_width <= 64); + debug_assert!(bit_offset / 8 < self.storage.as_ref().len()); + debug_assert!( + (bit_offset + (bit_width as usize)) / 8 <= + self.storage.as_ref().len() + ); + for i in 0..(bit_width as usize) { + let mask = 1 << i; + let val_bit_is_set = val & mask == mask; + let index = if cfg!(target_endian = "big") { + bit_width as usize - 1 - i + } else { + i + }; + self.set_bit(index + bit_offset, val_bit_is_set); + } + } +} +#[repr(C, packed)] +#[derive(Debug, Default, Copy, Clone)] +pub struct Struct { + pub _bitfield_align_1: [u8; 0], + pub _bitfield_1: __BindgenBitfieldUnit<[u8; 4usize]>, +} +#[test] +fn bindgen_test_layout_Struct() { + assert_eq!( + ::std::mem::size_of::(), + 4usize, + concat!("Size of: ", stringify!(Struct)) + ); + assert_eq!( + ::std::mem::align_of::(), + 1usize, + concat!("Alignment of ", stringify!(Struct)) + ); +} +impl Struct { + #[inline] + pub fn a(&self) -> ::std::os::raw::c_uchar { + unsafe { + ::std::mem::transmute(self._bitfield_1.get(0usize, 1u8) as u8) + } + } + #[inline] + pub fn set_a(&mut self, val: ::std::os::raw::c_uchar) { + unsafe { + let val: u8 = ::std::mem::transmute(val); + self._bitfield_1.set(0usize, 1u8, val as u64) + } + } + #[inline] + pub fn b(&self) -> ::std::os::raw::c_uchar { + unsafe { + ::std::mem::transmute(self._bitfield_1.get(1usize, 1u8) as u8) + } + } + #[inline] + pub fn set_b(&mut self, val: ::std::os::raw::c_uchar) { + unsafe { + let val: u8 = ::std::mem::transmute(val); + self._bitfield_1.set(1usize, 1u8, val as u64) + } + } + #[inline] + pub fn c(&self) -> ::std::os::raw::c_uchar { + unsafe { + ::std::mem::transmute(self._bitfield_1.get(2usize, 6u8) as u8) + } + } + #[inline] + pub fn set_c(&mut self, val: ::std::os::raw::c_uchar) { + unsafe { + let val: u8 = ::std::mem::transmute(val); + self._bitfield_1.set(2usize, 6u8, val as u64) + } + } + #[inline] + pub fn d(&self) -> ::std::os::raw::c_ushort { + unsafe { + ::std::mem::transmute(self._bitfield_1.get(8usize, 16u8) as u16) + } + } + #[inline] + pub fn set_d(&mut self, val: ::std::os::raw::c_ushort) { + unsafe { + let val: u16 = ::std::mem::transmute(val); + self._bitfield_1.set(8usize, 16u8, val as u64) + } + } + #[inline] + pub fn e(&self) -> ::std::os::raw::c_uchar { + unsafe { + ::std::mem::transmute(self._bitfield_1.get(24usize, 8u8) as u8) + } + } + #[inline] + pub fn set_e(&mut self, val: ::std::os::raw::c_uchar) { + unsafe { + let val: u8 = ::std::mem::transmute(val); + self._bitfield_1.set(24usize, 8u8, val as u64) + } + } + #[inline] + pub fn new_bitfield_1( + a: ::std::os::raw::c_uchar, + b: ::std::os::raw::c_uchar, + c: ::std::os::raw::c_uchar, + d: ::std::os::raw::c_ushort, + e: ::std::os::raw::c_uchar, + ) -> __BindgenBitfieldUnit<[u8; 4usize]> { + let mut __bindgen_bitfield_unit: __BindgenBitfieldUnit<[u8; 4usize]> = + Default::default(); + __bindgen_bitfield_unit.set(0usize, 1u8, { + let a: u8 = unsafe { ::std::mem::transmute(a) }; + a as u64 + }); + __bindgen_bitfield_unit.set(1usize, 1u8, { + let b: u8 = unsafe { ::std::mem::transmute(b) }; + b as u64 + }); + __bindgen_bitfield_unit.set(2usize, 6u8, { + let c: u8 = unsafe { ::std::mem::transmute(c) }; + c as u64 + }); + __bindgen_bitfield_unit.set(8usize, 16u8, { + let d: u16 = unsafe { ::std::mem::transmute(d) }; + d as u64 + }); + __bindgen_bitfield_unit.set(24usize, 8u8, { + let e: u8 = unsafe { ::std::mem::transmute(e) }; + e as u64 + }); + __bindgen_bitfield_unit + } +} diff --git a/tests/headers/bitfield_pragma_packed.h b/tests/headers/bitfield_pragma_packed.h new file mode 100644 index 0000000000..b4011ca844 --- /dev/null +++ b/tests/headers/bitfield_pragma_packed.h @@ -0,0 +1,9 @@ +#pragma pack(push, 1) +struct Struct { + unsigned char a : 1; + unsigned char b : 1; + unsigned char c : 6; + unsigned short int d : 16; + unsigned char e : 8; +}; +#pragma pack(pop)