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.

2 files changed, 84 insertions, 68 deletions

diff --git a/src/codegen/mod.rs b/src/codegen/mod.rs
index cad2f47e..6a7660f6 100644
--- a/src/codegen/mod.rs
+++ b/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
diff --git a/src/codegen/struct_layout.rs b/src/codegen/struct_layout.rs
index 4536e889..2e4b9735 100644
--- a/src/codegen/struct_layout.rs
+++ b/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.