From c0b29974298d0bf3e47f03cc2eaf982c38f19dc8 Mon Sep 17 00:00:00 2001 From: Thomas Bertschinger Date: Wed, 21 Feb 2024 22:15:25 -0700 Subject: [PATCH] try to exclude packed attr for types that contain aligned types This patch handles some (but not all) cases of types with a `packed` attribute that contain a type with an `align(N)` attribute. This uses information available in the Clang IR about types' layout to determine if a given type is likely to have an alignment attribute placed on it during the code generation phase. This is just a heuristic; the decision to actually place an alignment attribute depends on information that is not known until code generation, so the logic here may result in both false positives and false negatives. Using the real information from codegen on whether an alignment attribute was placed on a child type is not possible in general, because the order in which types are generated is not guaranteed. In some cases code may be generated for parent types before code is generated for child types contained in that parent. Then, it will be impossible to make an accurate decision regarding whether to remove the `packed` attribute from the parent type. The impact of a false negative is that an outer type may have a `packed` attr even when an inner type as an `align` attr. Such a type would not compile under rustc, but it already would not compile so this has no harmful impact. The impact of a false positive is that an outer type may have its `packed` attr stripped needlessly, because no inner types actually have an `align` attr. Because we only remove the `packed` attr when we can confirm that there will be no change to the type's layout, this also should have no harmful impact. --- .../tests/packed_embeds_aligned.rs | 126 ++++++++++++++++++ .../tests/headers/packed_embeds_aligned.h | 19 +++ bindgen/codegen/mod.rs | 9 +- bindgen/codegen/struct_layout.rs | 4 + bindgen/ir/comp.rs | 78 ++++++++++- bindgen/ir/ty.rs | 12 ++ 6 files changed, 244 insertions(+), 4 deletions(-) create mode 100644 bindgen-tests/tests/expectations/tests/packed_embeds_aligned.rs create mode 100644 bindgen-tests/tests/headers/packed_embeds_aligned.h diff --git a/bindgen-tests/tests/expectations/tests/packed_embeds_aligned.rs b/bindgen-tests/tests/expectations/tests/packed_embeds_aligned.rs new file mode 100644 index 0000000000..5d09c56d78 --- /dev/null +++ b/bindgen-tests/tests/expectations/tests/packed_embeds_aligned.rs @@ -0,0 +1,126 @@ +#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)] +#[repr(C)] +#[repr(align(2))] +#[derive(Debug, Default, Copy, Clone)] +pub struct inner1 { + pub a: ::std::os::raw::c_char, + pub b: ::std::os::raw::c_char, +} +#[test] +fn bindgen_test_layout_inner1() { + const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); + let ptr = UNINIT.as_ptr(); + assert_eq!( + ::std::mem::size_of::(), + 2usize, + concat!("Size of: ", stringify!(inner1)), + ); + assert_eq!( + ::std::mem::align_of::(), + 2usize, + concat!("Alignment of ", stringify!(inner1)), + ); + assert_eq!( + unsafe { ::std::ptr::addr_of!((*ptr).a) as usize - ptr as usize }, + 0usize, + concat!("Offset of field: ", stringify!(inner1), "::", stringify!(a)), + ); + assert_eq!( + unsafe { ::std::ptr::addr_of!((*ptr).b) as usize - ptr as usize }, + 1usize, + concat!("Offset of field: ", stringify!(inner1), "::", stringify!(b)), + ); +} +#[repr(C)] +#[derive(Debug, Default, Copy, Clone)] +pub struct inner2 { + pub a: inner1, + pub b: inner1, +} +#[test] +fn bindgen_test_layout_inner2() { + const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); + let ptr = UNINIT.as_ptr(); + assert_eq!( + ::std::mem::size_of::(), + 4usize, + concat!("Size of: ", stringify!(inner2)), + ); + assert_eq!( + ::std::mem::align_of::(), + 2usize, + concat!("Alignment of ", stringify!(inner2)), + ); + assert_eq!( + unsafe { ::std::ptr::addr_of!((*ptr).a) as usize - ptr as usize }, + 0usize, + concat!("Offset of field: ", stringify!(inner2), "::", stringify!(a)), + ); + assert_eq!( + unsafe { ::std::ptr::addr_of!((*ptr).b) as usize - ptr as usize }, + 2usize, + concat!("Offset of field: ", stringify!(inner2), "::", stringify!(b)), + ); +} +#[repr(C)] +#[derive(Debug, Default, Copy, Clone)] +pub struct outer1 { + pub a: ::std::os::raw::c_short, + pub b: inner1, +} +#[test] +fn bindgen_test_layout_outer1() { + const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); + let ptr = UNINIT.as_ptr(); + assert_eq!( + ::std::mem::size_of::(), + 4usize, + concat!("Size of: ", stringify!(outer1)), + ); + assert_eq!( + ::std::mem::align_of::(), + 2usize, + concat!("Alignment of ", stringify!(outer1)), + ); + assert_eq!( + unsafe { ::std::ptr::addr_of!((*ptr).a) as usize - ptr as usize }, + 0usize, + concat!("Offset of field: ", stringify!(outer1), "::", stringify!(a)), + ); + assert_eq!( + unsafe { ::std::ptr::addr_of!((*ptr).b) as usize - ptr as usize }, + 2usize, + concat!("Offset of field: ", stringify!(outer1), "::", stringify!(b)), + ); +} +#[repr(C)] +#[derive(Debug, Default, Copy, Clone)] +pub struct outer2 { + pub a: ::std::os::raw::c_short, + pub b: inner2, +} +#[test] +fn bindgen_test_layout_outer2() { + const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); + let ptr = UNINIT.as_ptr(); + assert_eq!( + ::std::mem::size_of::(), + 6usize, + concat!("Size of: ", stringify!(outer2)), + ); + assert_eq!( + ::std::mem::align_of::(), + 2usize, + concat!("Alignment of ", stringify!(outer2)), + ); + assert_eq!( + unsafe { ::std::ptr::addr_of!((*ptr).a) as usize - ptr as usize }, + 0usize, + concat!("Offset of field: ", stringify!(outer2), "::", stringify!(a)), + ); + assert_eq!( + unsafe { ::std::ptr::addr_of!((*ptr).b) as usize - ptr as usize }, + 2usize, + concat!("Offset of field: ", stringify!(outer2), "::", stringify!(b)), + ); +} diff --git a/bindgen-tests/tests/headers/packed_embeds_aligned.h b/bindgen-tests/tests/headers/packed_embeds_aligned.h new file mode 100644 index 0000000000..348a97cc6d --- /dev/null +++ b/bindgen-tests/tests/headers/packed_embeds_aligned.h @@ -0,0 +1,19 @@ +struct inner1 { + char a; + char b; +} __attribute__((aligned(2))); + +struct inner2 { + struct inner1 a; + struct inner1 b; +} __attribute__((aligned(2))); + +struct outer1 { + short a; + struct inner1 b; +} __attribute__((packed, aligned(2))); + +struct outer2 { + short a; + struct inner2 b; +} __attribute__((packed, aligned(2))); diff --git a/bindgen/codegen/mod.rs b/bindgen/codegen/mod.rs index b126a847b4..3b9572b6a4 100644 --- a/bindgen/codegen/mod.rs +++ b/bindgen/codegen/mod.rs @@ -2202,8 +2202,13 @@ impl CodeGenerator for CompInfo { // "packed" attr is redundant, and do not include it if so. if packed && !is_opaque && - !(explicit_align.is_some() && - self.already_packed(ctx).unwrap_or(false)) + !((explicit_align.is_some() || self.may_contain_aligned_type()) && + self.already_packed( + ctx, + struct_layout.max_field_align(), + layout, + ) + .unwrap_or(false)) { let n = layout.map_or(1, |l| l.align); assert!(ctx.options().rust_features().repr_packed_n || n == 1); diff --git a/bindgen/codegen/struct_layout.rs b/bindgen/codegen/struct_layout.rs index 7349669871..780afe10b8 100644 --- a/bindgen/codegen/struct_layout.rs +++ b/bindgen/codegen/struct_layout.rs @@ -121,6 +121,10 @@ impl<'a> StructLayoutTracker<'a> { self.is_rust_union } + pub(crate) fn max_field_align(&self) -> usize { + self.max_field_align + } + pub(crate) fn saw_vtable(&mut self) { debug!("saw vtable for {}", self.name); diff --git a/bindgen/ir/comp.rs b/bindgen/ir/comp.rs index bd4d016261..cd77fcff01 100644 --- a/bindgen/ir/comp.rs +++ b/bindgen/ir/comp.rs @@ -10,7 +10,7 @@ use super::item::{IsOpaque, Item}; use super::layout::Layout; use super::template::TemplateParameters; use super::traversal::{EdgeKind, Trace, Tracer}; -use super::ty::RUST_DERIVE_IN_ARRAY_LIMIT; +use super::ty::{TypeKind, RUST_DERIVE_IN_ARRAY_LIMIT}; use crate::clang; use crate::codegen::struct_layout::{align_to, bytes_from_bits_pow2}; use crate::ir::derive::CanDeriveCopy; @@ -1049,6 +1049,10 @@ pub(crate) struct CompInfo { /// Used to indicate when a struct has been forward declared. Usually used /// in headers so that APIs can't modify them directly. is_forward_declaration: bool, + + /// Heuristic for whether this type is likely to contain any types with an + /// explicit alignment attribute + may_contain_aligned_type: bool, } impl CompInfo { @@ -1072,6 +1076,7 @@ impl CompInfo { packed_attr: false, found_unknown_attr: false, is_forward_declaration: false, + may_contain_aligned_type: false, } } @@ -1235,6 +1240,7 @@ impl CompInfo { ty: &clang::Type, location: Option, ctx: &mut BindgenContext, + outer_layout: Option<&Layout>, ) -> Result { use clang_sys::*; assert!( @@ -1252,6 +1258,7 @@ impl CompInfo { } let kind = kind?; + let mut max_field_align: Option = None; debug!("CompInfo::from_ty({:?}, {:?})", kind, cursor); @@ -1330,6 +1337,29 @@ impl CompInfo { ctx, ); + let field_comp_type = ctx.resolve_type(field_type); + + match field_comp_type.kind() { + TypeKind::ResolvedTypeRef(inner_type) => { + let inner_type = ctx.resolve_type(*inner_type); + if let Some(inner_type) = inner_type.as_comp() { + if inner_type.may_contain_aligned_type { + ci.may_contain_aligned_type = true; + } + } + } + _ => {} + } + + if let Ok(field_layout) = + cur.cur_type().fallible_layout(ctx) + { + max_field_align = Some(cmp::max( + max_field_align.unwrap_or(0), + field_layout.align, + )); + } + let comment = cur.raw_comment(); let annotations = Annotations::new(&cur); let name = cur.spelling(); @@ -1418,6 +1448,14 @@ impl CompInfo { Some((inner, ty, public, offset)); } } + if let Ok(field_layout) = + cur.cur_type().fallible_layout(ctx) + { + max_field_align = Some(cmp::max( + max_field_align.unwrap_or(0), + field_layout.align, + )); + } } CXCursor_PackedAttr => { ci.packed_attr = true; @@ -1566,6 +1604,15 @@ impl CompInfo { CXChildVisit_Continue }); + if let Some(outer_layout) = outer_layout { + if let Some(max_field_align) = max_field_align { + if outer_layout.align > max_field_align || max_field_align >= 16 + { + ci.may_contain_aligned_type = true; + } + } + } + if let Some((ty, _, public, offset)) = maybe_anonymous_struct_field { let field = RawField::new(None, ty, None, None, None, public, offset); @@ -1646,7 +1693,12 @@ impl CompInfo { /// "packed" attribute without changing the layout. /// This is useful for types that need an "align(N)" attribute since rustc won't compile /// structs that have both of those attributes. - pub(crate) fn already_packed(&self, ctx: &BindgenContext) -> Option { + pub(crate) fn already_packed( + &self, + ctx: &BindgenContext, + max_field_align: usize, + outer_layout: Option, + ) -> Option { let mut total_size: usize = 0; for field in self.fields().iter() { @@ -1659,6 +1711,18 @@ impl CompInfo { total_size += layout.size; } + if let Some(outer_layout) = outer_layout { + if max_field_align != 0 && + outer_layout.size % max_field_align != 0 + { + return Some(false); + } + + if outer_layout.align < max_field_align { + return Some(false); + } + } + Some(true) } @@ -1667,6 +1731,16 @@ impl CompInfo { self.is_forward_declaration } + /// Returns true if compound type might have a child type with an explicit alignment attribute. + /// + /// Note, this is not 100% accurate because it is based only on Clang layout information, but + /// the final determination to place an alignment attribute happens during code generation and + /// requires information not available at the time this is set. However, this is good enough + /// for most cases. + pub(crate) fn may_contain_aligned_type(&self) -> bool { + self.may_contain_aligned_type + } + /// Compute this compound structure's bitfield allocation units. pub(crate) fn compute_bitfield_units( &mut self, diff --git a/bindgen/ir/ty.rs b/bindgen/ir/ty.rs index 2a24dd0291..27aedf40a3 100644 --- a/bindgen/ir/ty.rs +++ b/bindgen/ir/ty.rs @@ -52,6 +52,15 @@ impl Type { } } + /// Get the underlying `CompInfo` for this type as an immutable reference, or + /// `None` if this is some other kind of type. + pub(crate) fn as_comp(&self) -> Option<&CompInfo> { + match self.kind { + TypeKind::Comp(ref ci) => Some(ci), + _ => None, + } + } + /// Construct a new `Type`. pub(crate) fn new( name: Option, @@ -806,6 +815,7 @@ impl Type { ty, Some(location), ctx, + layout.as_ref(), ) .expect("C'mon"); TypeKind::Comp(complex) @@ -868,6 +878,7 @@ impl Type { ty, Some(location), ctx, + layout.as_ref(), ); match complex { Ok(complex) => TypeKind::Comp(complex), @@ -1134,6 +1145,7 @@ impl Type { ty, Some(location), ctx, + layout.as_ref(), ) .expect("Not a complex type?");