[RFC, V1] try to exclude packed attr for types that contain aligned types

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)));

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);

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);
 

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<clang::Cursor>,
         ctx: &mut BindgenContext,
+        outer_layout: Option<&Layout>,
     ) -> Result<Self, ParseError> {
         use clang_sys::*;
         assert!(
@@ -1252,6 +1258,7 @@ impl CompInfo {
         }
 
         let kind = kind?;
+        let mut max_field_align: Option<usize> = 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<bool> {
+    pub(crate) fn already_packed(
+        &self,
+        ctx: &BindgenContext,
+        max_field_align: usize,
+        outer_layout: Option<Layout>,
+    ) -> Option<bool> {
         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,

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<String>,
@@ -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?");