codegen: Track union layout more accurately.

Instead of always generating the _bindgen_union_align method (which
shouldn't be needed at all for Rust structs, since the struct layout
tracker already deals with adding repr(align) as necessary) make sure to
visit all fields appropriately to generate the correct alignment.

Author: emilio

bindgen-integration/src/lib.rs

@@ -260,8 +260,6 @@ fn test_macro_customintkind_path() {
     assert!(v.is::<MacroInteger>())
 }
 
-// https://github.com/rust-lang/rust-bindgen/issues/1973
-#[cfg_attr(target_arch = "aarch64", should_panic)] // This line should be removed after the bug linked above is fixed
 #[test]
 fn test_homogeneous_aggregate_float_union() {
     unsafe {

src/codegen/mod.rs

@@ -1219,7 +1219,7 @@ impl<'a> FieldCodegen<'a> for FieldData {
         ty.append_implicit_template_params(ctx, field_item);
 
         // NB: If supported, we use proper `union` types.
-        let ty = if parent.is_union() && !parent.can_be_rust_union(ctx) {
+        let ty = if parent.is_union() && !struct_layout.is_rust_union() {
             result.saw_bindgen_union();
             if ctx.options().enable_cxx_namespaces {
                 quote! {
@@ -1263,12 +1263,10 @@ impl<'a> FieldCodegen<'a> for FieldData {
             .expect("Each field should have a name in codegen!");
         let field_ident = ctx.rust_ident_raw(field_name.as_str());
 
-        if !parent.is_union() {
-            if let Some(padding_field) =
-                struct_layout.pad_field(&field_name, field_ty, self.offset())
-            {
-                fields.extend(Some(padding_field));
-            }
+        if let Some(padding_field) =
+            struct_layout.saw_field(&field_name, field_ty, self.offset())
+        {
+            fields.extend(Some(padding_field));
         }
 
         let is_private = (!self.is_public() &&
@@ -1433,7 +1431,7 @@ impl<'a> FieldCodegen<'a> for BitfieldUnit {
         let layout = self.layout();
         let unit_field_ty = helpers::bitfield_unit(ctx, layout);
         let field_ty = {
-            if parent.is_union() && !parent.can_be_rust_union(ctx) {
+            if parent.is_union() && !struct_layout.is_rust_union() {
                 result.saw_bindgen_union();
                 if ctx.options().enable_cxx_namespaces {
                     quote! {
@@ -1571,7 +1569,7 @@ impl<'a> FieldCodegen<'a> for Bitfield {
         _accessor_kind: FieldAccessorKind,
         parent: &CompInfo,
         _result: &mut CodegenResult,
-        _struct_layout: &mut StructLayoutTracker,
+        struct_layout: &mut StructLayoutTracker,
         _fields: &mut F,
         methods: &mut M,
         (unit_field_name, bitfield_representable_as_int): (&'a str, &mut bool),
@@ -1612,7 +1610,7 @@ impl<'a> FieldCodegen<'a> for Bitfield {
             self.is_public() && !fields_should_be_private,
         );
 
-        if parent.is_union() && !parent.can_be_rust_union(ctx) {
+        if parent.is_union() && !struct_layout.is_rust_union() {
             methods.extend(Some(quote! {
                 #[inline]
                 #access_spec fn #getter_name(&self) -> #bitfield_ty {
@@ -1768,15 +1766,53 @@ impl CodeGenerator for CompInfo {
             }
         }
 
-        let is_union = self.kind() == CompKind::Union;
-        let layout = item.kind().expect_type().layout(ctx);
-
-        let mut explicit_align = None;
         if is_opaque {
             // Opaque item should not have generated methods, fields.
             debug_assert!(fields.is_empty());
             debug_assert!(methods.is_empty());
+        }
 
+        let is_union = self.kind() == CompKind::Union;
+        let layout = item.kind().expect_type().layout(ctx);
+        let zero_sized = item.is_zero_sized(ctx);
+        let forward_decl = self.is_forward_declaration();
+
+        let mut explicit_align = None;
+
+        // C++ requires every struct to be addressable, so what C++ compilers do
+        // is making the struct 1-byte sized.
+        //
+        // This is apparently not the case for C, see:
+        // https://github.com/rust-lang/rust-bindgen/issues/551
+        //
+        // Just get the layout, and assume C++ if not.
+        //
+        // NOTE: This check is conveniently here to avoid the dummy fields we
+        // may add for unused template parameters.
+        if !forward_decl && zero_sized {
+            let has_address = if is_opaque {
+                // Generate the address field if it's an opaque type and
+                // couldn't determine the layout of the blob.
+                layout.is_none()
+            } else {
+                layout.map_or(true, |l| l.size != 0)
+            };
+
+            if has_address {
+                let layout = Layout::new(1, 1);
+                let ty = helpers::blob(ctx, Layout::new(1, 1));
+                struct_layout.saw_field_with_layout(
+                    "_address",
+                    layout,
+                    /* offset = */ Some(0),
+                );
+                fields.push(quote! {
+                    pub _address: #ty,
+                });
+            }
+        }
+
+        if is_opaque {
             match layout {
                 Some(l) => {
                     explicit_align = Some(l.align);
@@ -1790,7 +1826,7 @@ impl CodeGenerator for CompInfo {
                     warn!("Opaque type without layout! Expect dragons!");
                 }
             }
-        } else if !is_union && !item.is_zero_sized(ctx) {
+        } else if !is_union && !zero_sized {
             if let Some(padding_field) =
                 layout.and_then(|layout| struct_layout.pad_struct(layout))
             {
@@ -1815,57 +1851,26 @@ impl CodeGenerator for CompInfo {
                     }
                 }
             }
-        } else if is_union && !self.is_forward_declaration() {
+        } else if is_union && !forward_decl {
             // TODO(emilio): It'd be nice to unify this with the struct path
             // above somehow.
             let layout = layout.expect("Unable to get layout information?");
-            struct_layout.saw_union(layout);
-
             if struct_layout.requires_explicit_align(layout) {
                 explicit_align = Some(layout.align);
             }
 
-            let ty = helpers::blob(ctx, layout);
-            fields.push(if self.can_be_rust_union(ctx) {
-                quote! {
-                    _bindgen_union_align: #ty ,
-                }
-            } else {
-                quote! {
+            if !struct_layout.is_rust_union() {
+                let ty = helpers::blob(ctx, layout);
+                fields.push(quote! {
                     pub bindgen_union_field: #ty ,
-                }
-            });
+                })
+            }
         }
 
-        // C++ requires every struct to be addressable, so what C++ compilers do
-        // is making the struct 1-byte sized.
-        //
-        // This is apparently not the case for C, see:
-        // https://github.com/rust-lang/rust-bindgen/issues/551
-        //
-        // Just get the layout, and assume C++ if not.
-        //
-        // NOTE: This check is conveniently here to avoid the dummy fields we
-        // may add for unused template parameters.
-        if self.is_forward_declaration() {
+        if forward_decl {
             fields.push(quote! {
                 _unused: [u8; 0],
             });
-        } else if item.is_zero_sized(ctx) {
-            let has_address = if is_opaque {
-                // Generate the address field if it's an opaque type and
-                // couldn't determine the layout of the blob.
-                layout.is_none()
-            } else {
-                layout.map_or(true, |l| l.size != 0)
-            };
-
-            if has_address {
-                let ty = helpers::blob(ctx, Layout::new(1, 1));
-                fields.push(quote! {
-                    pub _address: #ty,
-                });
-            }
         }
 
         let mut generic_param_names = vec![];
@@ -1963,7 +1968,7 @@ impl CodeGenerator for CompInfo {
             attributes.push(attributes::derives(&derives))
         }
 
-        let mut tokens = if is_union && self.can_be_rust_union(ctx) {
+        let mut tokens = if is_union && struct_layout.is_rust_union() {
             quote! {
                 #( #attributes )*
                 pub union #canonical_ident

src/codegen/struct_layout.rs

@@ -19,6 +19,7 @@ pub struct StructLayoutTracker<'a> {
     comp: &'a CompInfo,
     is_packed: bool,
     known_type_layout: Option<Layout>,
+    is_rust_union: bool,
     latest_offset: usize,
     padding_count: usize,
     latest_field_layout: Option<Layout>,
@@ -89,12 +90,15 @@ impl<'a> StructLayoutTracker<'a> {
     ) -> Self {
         let known_type_layout = ty.layout(ctx);
         let is_packed = comp.is_packed(ctx, known_type_layout.as_ref());
+        let is_rust_union = comp.is_union() &&
+            comp.can_be_rust_union(ctx, known_type_layout.as_ref());
         StructLayoutTracker {
             name,
             ctx,
             comp,
             is_packed,
             known_type_layout,
+            is_rust_union,
             latest_offset: 0,
             padding_count: 0,
             latest_field_layout: None,
@@ -103,6 +107,10 @@ impl<'a> StructLayoutTracker<'a> {
         }
     }
 
+    pub fn is_rust_union(&self) -> bool {
+        self.is_rust_union
+    }
+
     pub fn saw_vtable(&mut self) {
         debug!("saw vtable for {}", self.name);
 
@@ -143,18 +151,9 @@ impl<'a> StructLayoutTracker<'a> {
         // actually generate the dummy alignment.
     }
 
-    pub fn saw_union(&mut self, layout: Layout) {
-        debug!("saw union for {}: {:?}", self.name, layout);
-        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);
-    }
-
-    /// Add a padding field if necessary for a given new field _before_ adding
-    /// that field.
-    pub fn pad_field(
+    /// Returns a padding field if necessary for a given new field _before_
+    /// adding that field.
+    pub fn saw_field(
         &mut self,
         field_name: &str,
         field_ty: &Type,
@@ -181,15 +180,27 @@ impl<'a> StructLayoutTracker<'a> {
                 }
             }
         }
+        self.saw_field_with_layout(field_name, field_layout, field_offset)
+    }
 
+    pub fn saw_field_with_layout(
+        &mut self,
+        field_name: &str,
+        field_layout: Layout,
+        field_offset: Option<usize>,
+    ) -> Option<proc_macro2::TokenStream> {
         let will_merge_with_bitfield = self.align_to_latest_field(field_layout);
 
+        let is_union = self.comp.is_union();
         let padding_bytes = match field_offset {
             Some(offset) if offset / 8 > self.latest_offset => {
                 offset / 8 - self.latest_offset
             }
             _ => {
-                if will_merge_with_bitfield || field_layout.align == 0 {
+                if will_merge_with_bitfield ||
+                    field_layout.align == 0 ||
+                    is_union
+                {
                     0
                 } else if !self.is_packed {
                     self.padding_bytes(field_layout)
@@ -203,7 +214,7 @@ impl<'a> StructLayoutTracker<'a> {
 
         self.latest_offset += padding_bytes;
 
-        let padding_layout = if self.is_packed {
+        let padding_layout = if self.is_packed || is_union {
             None
         } else {
             // Otherwise the padding is useless.

src/ir/comp.rs

@@ -1642,7 +1642,12 @@ impl CompInfo {
     /// Requirements:
     ///     1. Current RustTarget allows for `untagged_union`
     ///     2. Each field can derive `Copy`
-    pub fn can_be_rust_union(&self, ctx: &BindgenContext) -> bool {
+    ///     3. It's not zero-sized.
+    pub fn can_be_rust_union(
+        &self,
+        ctx: &BindgenContext,
+        layout: Option<&Layout>,
+    ) -> bool {
         if !ctx.options().rust_features().untagged_union {
             return false;
         }
@@ -1651,12 +1656,22 @@ impl CompInfo {
             return false;
         }
 
-        self.fields().iter().all(|f| match *f {
+        let all_can_copy = self.fields().iter().all(|f| match *f {
             Field::DataMember(ref field_data) => {
                 field_data.ty().can_derive_copy(ctx)
             }
             Field::Bitfields(_) => true,
-        })
+        });
+
+        if !all_can_copy {
+            return false;
+        }
+
+        if layout.map_or(false, |l| l.size == 0) {
+            return false;
+        }
+
+        true
     }
 }
 

tests/expectations/tests/16-byte-alignment.rs

@@ -17,7 +17,6 @@ pub struct rte_ipv4_tuple {
 pub union rte_ipv4_tuple__bindgen_ty_1 {
     pub __bindgen_anon_1: rte_ipv4_tuple__bindgen_ty_1__bindgen_ty_1,
     pub sctp_tag: u32,
-    _bindgen_union_align: u32,
 }
 #[repr(C)]
 #[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
@@ -159,7 +158,6 @@ pub struct rte_ipv6_tuple {
 pub union rte_ipv6_tuple__bindgen_ty_1 {
     pub __bindgen_anon_1: rte_ipv6_tuple__bindgen_ty_1__bindgen_ty_1,
     pub sctp_tag: u32,
-    _bindgen_union_align: u32,
 }
 #[repr(C)]
 #[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
@@ -295,7 +293,6 @@ impl Default for rte_ipv6_tuple {
 pub union rte_thash_tuple {
     pub v4: rte_ipv4_tuple,
     pub v6: rte_ipv6_tuple,
-    _bindgen_union_align: [u128; 3usize],
 }
 #[test]
 fn bindgen_test_layout_rte_thash_tuple() {

tests/expectations/tests/anon-fields-prefix.rs

@@ -11,7 +11,6 @@ pub union color {
     pub u1: color__bindgen_ty_1,
     pub u2: color__bindgen_ty_2,
     pub v3: [::std::os::raw::c_uchar; 3usize],
-    _bindgen_union_align: [u8; 3usize],
 }
 #[repr(C)]
 #[derive(Debug, Default, Copy, Clone)]

tests/expectations/tests/anon_struct_in_union.rs

@@ -14,7 +14,6 @@ pub struct s {
 #[derive(Copy, Clone)]
 pub union s__bindgen_ty_1 {
     pub field: s__bindgen_ty_1_inner,
-    _bindgen_union_align: u32,
 }
 #[repr(C)]
 #[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]

tests/expectations/tests/anon_union.rs

@@ -35,7 +35,6 @@ pub struct TErrorResult_DOMExceptionInfo {
 pub union TErrorResult__bindgen_ty_1 {
     pub mMessage: *mut TErrorResult_Message,
     pub mDOMExceptionInfo: *mut TErrorResult_DOMExceptionInfo,
-    _bindgen_union_align: u64,
 }
 impl Default for TErrorResult__bindgen_ty_1 {
     fn default() -> Self {

tests/expectations/tests/class.rs

@@ -340,7 +340,6 @@ impl Default for IncompleteArrayNonCopiable {
 pub union Union {
     pub d: f32,
     pub i: ::std::os::raw::c_int,
-    _bindgen_union_align: u32,
 }
 #[test]
 fn bindgen_test_layout_Union() {