Auto merge of #1075 - pepyakin:fix-1034, r=fitzgen

Use `repr(packed)` If struct requires explicit alignment of 1. Fixes #1034
rust-lang · Oct 11, 2017 · c88ddab · c88ddab
2 parents 8315646 + 746c743
commit c88ddab
Show file tree

Hide file tree

Showing 8 changed files with 149 additions and 112 deletions.
diff --git a/bindgen-integration/src/lib.rs b/bindgen-integration/src/lib.rs
@@ -183,8 +183,7 @@ fn test_bitfields_sixth() {
 fn test_bitfield_constructors() {
     use std::mem;
     let mut first = bindings::bitfields::First {
-        _bitfield_1: unsafe { mem::transmute(bindings::bitfields::First::new_bitfield_1(1, 2, 3)) },
-        __bindgen_align: [],
+        _bitfield_1: unsafe { mem::transmute(bindings::bitfields::First::new_bitfield_1(1, 2, 3)) }
     };
     assert!(unsafe {
         first.assert(1, 2, 3)

diff --git a/src/codegen/mod.rs b/src/codegen/mod.rs
@@ -1380,6 +1380,8 @@ impl CodeGenerator for CompInfo {
 
         let used_template_params = item.used_template_params(ctx);
 
+        let mut packed = self.packed();
+
         // generate tuple struct if struct or union is a forward declaration,
         // skip for now if template parameters are needed.
         //
@@ -1397,97 +1399,8 @@ impl CodeGenerator for CompInfo {
             return;
         }
 
-        let mut attributes = vec![];
-        let mut needs_clone_impl = false;
-        let mut needs_default_impl = false;
-        let mut needs_debug_impl = false;
-        let mut needs_partialeq_impl = false;
-        if let Some(comment) = item.comment(ctx) {
-            attributes.push(attributes::doc(comment));
-        }
-        if self.packed() {
-            attributes.push(attributes::repr_list(&["C", "packed"]));
-        } else {
-            attributes.push(attributes::repr("C"));
-        }
-
-        let is_union = self.kind() == CompKind::Union;
-        let mut derives = vec![];
-        if item.can_derive_debug(ctx) {
-            derives.push("Debug");
-        } else {
-            needs_debug_impl = ctx.options().derive_debug &&
-                ctx.options().impl_debug
-        }
-
-        if item.can_derive_default(ctx) {
-            derives.push("Default");
-        } else {
-            needs_default_impl = ctx.options().derive_default;
-        }
-
-        if item.can_derive_copy(ctx) && !item.annotations().disallow_copy() &&
-            ctx.options().derive_copy
-        {
-            derives.push("Copy");
-            if used_template_params.is_some() {
-                // FIXME: This requires extra logic if you have a big array in a
-                // templated struct. The reason for this is that the magic:
-                //     fn clone(&self) -> Self { *self }
-                // doesn't work for templates.
-                //
-                // It's not hard to fix though.
-                derives.push("Clone");
-            } else {
-                needs_clone_impl = true;
-            }
-        }
-
-        if item.can_derive_hash(ctx) {
-            derives.push("Hash");
-        }
-
-        if item.can_derive_partialord(ctx) {
-            derives.push("PartialOrd");
-        }
-
-        if item.can_derive_ord(ctx) {
-            derives.push("Ord");
-        }
-
-        if item.can_derive_partialeq(ctx) {
-            derives.push("PartialEq");
-        } else {
-            needs_partialeq_impl = 
-                ctx.options().derive_partialeq && 
-                ctx.options().impl_partialeq &&
-                ctx.lookup_can_derive_partialeq_or_partialord(item.id())
-                    .map_or(true, |x| {
-                        x == CannotDeriveReason::ArrayTooLarge
-                    });
-        }
-
-        if item.can_derive_eq(ctx) {
-            derives.push("Eq");
-        }
-
-        if !derives.is_empty() {
-            attributes.push(attributes::derives(&derives))
-        }
-
         let canonical_name = item.canonical_name(ctx);
         let canonical_ident = ctx.rust_ident(&canonical_name);
-        let mut tokens = if is_union && self.can_be_rust_union(ctx) {
-            quote! {
-                #( #attributes )*
-                pub union #canonical_ident
-            }
-        } else {
-            quote! {
-                #( #attributes )*
-                pub struct #canonical_ident
-            }
-        };
 
         // Generate the vtable from the method list if appropriate.
         //
@@ -1540,6 +1453,8 @@ impl CodeGenerator for CompInfo {
                 });
             }
         }
+
+        let is_union = self.kind() == CompKind::Union;
         if is_union {
             result.saw_union();
             if !self.can_be_rust_union(ctx) {
@@ -1612,10 +1527,17 @@ impl CodeGenerator for CompInfo {
                 fields.push(padding_field);
             }
 
-            if let Some(align_field) =
-                layout.and_then(|layout| struct_layout.align_struct(layout))
-            {
-                fields.push(align_field);
+            if let Some(layout) = layout {
+                if struct_layout.requires_explicit_align(layout) {
+                    if layout.align == 1 {
+                        packed = true;
+                    } else {
+                        let ty = helpers::blob(Layout::new(0, layout.align));
+                        fields.push(quote! {
+                            pub __bindgen_align: #ty ,
+                        });
+                    }
+                }
             }
         }
 
@@ -1674,6 +1596,95 @@ impl CodeGenerator for CompInfo {
             quote! { }
         };
 
+        let mut attributes = vec![];
+        let mut needs_clone_impl = false;
+        let mut needs_default_impl = false;
+        let mut needs_debug_impl = false;
+        let mut needs_partialeq_impl = false;
+        if let Some(comment) = item.comment(ctx) {
+            attributes.push(attributes::doc(comment));
+        }
+        if packed {
+            attributes.push(attributes::repr_list(&["C", "packed"]));
+        } else {
+            attributes.push(attributes::repr("C"));
+        }
+
+        let mut derives = vec![];
+        if item.can_derive_debug(ctx) {
+            derives.push("Debug");
+        } else {
+            needs_debug_impl = ctx.options().derive_debug &&
+                ctx.options().impl_debug
+        }
+
+        if item.can_derive_default(ctx) {
+            derives.push("Default");
+        } else {
+            needs_default_impl = ctx.options().derive_default;
+        }
+
+        if item.can_derive_copy(ctx) && !item.annotations().disallow_copy() &&
+            ctx.options().derive_copy
+        {
+            derives.push("Copy");
+            if used_template_params.is_some() {
+                // FIXME: This requires extra logic if you have a big array in a
+                // templated struct. The reason for this is that the magic:
+                //     fn clone(&self) -> Self { *self }
+                // doesn't work for templates.
+                //
+                // It's not hard to fix though.
+                derives.push("Clone");
+            } else {
+                needs_clone_impl = true;
+            }
+        }
+
+        if item.can_derive_hash(ctx) {
+            derives.push("Hash");
+        }
+
+        if item.can_derive_partialord(ctx) {
+            derives.push("PartialOrd");
+        }
+
+        if item.can_derive_ord(ctx) {
+            derives.push("Ord");
+        }
+
+        if item.can_derive_partialeq(ctx) {
+            derives.push("PartialEq");
+        } else {
+            needs_partialeq_impl = 
+                ctx.options().derive_partialeq && 
+                ctx.options().impl_partialeq &&
+                ctx.lookup_can_derive_partialeq_or_partialord(item.id())
+                    .map_or(true, |x| {
+                        x == CannotDeriveReason::ArrayTooLarge
+                    });
+        }
+
+        if item.can_derive_eq(ctx) {
+            derives.push("Eq");
+        }
+
+        if !derives.is_empty() {
+            attributes.push(attributes::derives(&derives))
+        }
+
+        let mut tokens = if is_union && self.can_be_rust_union(ctx) {
+            quote! {
+                #( #attributes )*
+                pub union #canonical_ident
+            }
+        } else {
+            quote! {
+                #( #attributes )*
+                pub struct #canonical_ident
+            }
+        };
+
         tokens.append(quote! {
             #generics {
                 #( #fields )*

diff --git a/src/codegen/struct_layout.rs b/src/codegen/struct_layout.rs
@@ -288,18 +288,9 @@ impl<'a> StructLayoutTracker<'a> {
         }
     }
 
-    pub fn align_struct(&self, layout: Layout) -> Option<quote::Tokens> {
-        if self.max_field_align < layout.align &&
+    pub fn requires_explicit_align(&self, layout: Layout) -> bool {
+        self.max_field_align < layout.align &&
             layout.align <= mem::size_of::<*mut ()>()
-        {
-            let ty = helpers::blob(Layout::new(0, layout.align));
-
-            Some(quote! {
-                pub __bindgen_align: #ty ,
-            })
-        } else {
-            None
-        }
     }
 
     fn padding_bytes(&self, layout: Layout) -> usize {

diff --git a/tests/expectations/tests/bitfield-32bit-overflow.rs b/tests/expectations/tests/bitfield-32bit-overflow.rs
@@ -4,11 +4,10 @@
 #![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
 
 
-#[repr(C)]
+#[repr(C, packed)]
 #[derive(Debug, Default, Copy)]
 pub struct MuchBitfield {
     pub _bitfield_1: [u8; 5usize],
-    pub __bindgen_align: [u8; 0usize],
 }
 #[test]
 fn bindgen_test_layout_MuchBitfield() {

diff --git a/tests/expectations/tests/bitfield-method-same-name.rs b/tests/expectations/tests/bitfield-method-same-name.rs
@@ -4,11 +4,10 @@
 #![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
 
 
-#[repr(C)]
+#[repr(C, packed)]
 #[derive(Debug, Default, Copy)]
 pub struct Foo {
     pub _bitfield_1: u8,
-    pub __bindgen_align: [u8; 0usize],
 }
 #[test]
 fn bindgen_test_layout_Foo() {

diff --git a/tests/expectations/tests/issue-1034.rs b/tests/expectations/tests/issue-1034.rs
@@ -0,0 +1,35 @@
+/* automatically generated by rust-bindgen */
+
+
+#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
+
+
+#[repr(C, packed)]
+#[derive(Debug, Default, Copy)]
+pub struct S2 {
+    pub _bitfield_1: u16,
+}
+#[test]
+fn bindgen_test_layout_S2() {
+    assert_eq!(
+        ::std::mem::size_of::<S2>(),
+        2usize,
+        concat!("Size of: ", stringify!(S2))
+    );
+    assert_eq!(
+        ::std::mem::align_of::<S2>(),
+        1usize,
+        concat!("Alignment of ", stringify!(S2))
+    );
+}
+impl Clone for S2 {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+impl S2 {
+    #[inline]
+    pub fn new_bitfield_1() -> u16 {
+        0
+    }
+}
diff --git a/tests/expectations/tests/only_bitfields.rs b/tests/expectations/tests/only_bitfields.rs
@@ -4,11 +4,10 @@
 #![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
 
 
-#[repr(C)]
+#[repr(C, packed)]
 #[derive(Debug, Default, Copy)]
 pub struct C {
     pub _bitfield_1: u8,
-    pub __bindgen_align: [u8; 0usize],
 }
 #[test]
 fn bindgen_test_layout_C() {

diff --git a/tests/headers/issue-1034.h b/tests/headers/issue-1034.h
@@ -0,0 +1,4 @@
+
+struct S2 {
+    unsigned : 11
+};