Auto merge of #1136 - fitzgen:pragma-pack-bandaid, r=emilio

Detect `#pragma pack(...)` and make `pack(n)` where `n > 1` opaque

This is a bandaid for #537. It does *not* fix the underlying issue, which requires `#[repr(packed = "N")]` support in Rust. However, it does make sure that we don't generate type definitions with the wrong layout, or fail our generated layout tests.

r? @emilio or @pepyakin

Author: bors-servo

src/codegen/mod.rs

@@ -1394,7 +1394,9 @@ impl CodeGenerator for CompInfo {
 
         let used_template_params = item.used_template_params(ctx);
 
-        let mut packed = self.packed();
+        let ty = item.expect_type();
+        let layout = ty.layout(ctx);
+        let mut packed = self.is_packed(ctx, &layout);
 
         // generate tuple struct if struct or union is a forward declaration,
         // skip for now if template parameters are needed.
@@ -1431,7 +1433,7 @@ impl CodeGenerator for CompInfo {
         let is_opaque = item.is_opaque(ctx, &());
         let mut fields = vec![];
         let mut struct_layout =
-            StructLayoutTracker::new(ctx, self, &canonical_name);
+            StructLayoutTracker::new(ctx, self, ty, &canonical_name);
 
         if !is_opaque {
             if item.has_vtable_ptr(ctx) {
@@ -1618,7 +1620,7 @@ impl CodeGenerator for CompInfo {
         if let Some(comment) = item.comment(ctx) {
             attributes.push(attributes::doc(comment));
         }
-        if packed {
+        if packed && !is_opaque {
             attributes.push(attributes::repr_list(&["C", "packed"]));
         } else {
             attributes.push(attributes::repr("C"));

src/codegen/struct_layout.rs

@@ -16,6 +16,7 @@ pub struct StructLayoutTracker<'a> {
     name: &'a str,
     ctx: &'a BindgenContext,
     comp: &'a CompInfo,
+    is_packed: bool,
     latest_offset: usize,
     padding_count: usize,
     latest_field_layout: Option<Layout>,
@@ -81,12 +82,14 @@ impl<'a> StructLayoutTracker<'a> {
     pub fn new(
         ctx: &'a BindgenContext,
         comp: &'a CompInfo,
+        ty: &'a Type,
         name: &'a str,
     ) -> Self {
         StructLayoutTracker {
             name: name,
             ctx: ctx,
             comp: comp,
+            is_packed: comp.is_packed(ctx, &ty.layout(ctx)),
             latest_offset: 0,
             padding_count: 0,
             latest_field_layout: None,
@@ -180,7 +183,7 @@ impl<'a> StructLayoutTracker<'a> {
 
         let will_merge_with_bitfield = self.align_to_latest_field(field_layout);
 
-        let padding_layout = if self.comp.packed() {
+        let padding_layout = if self.is_packed {
             None
         } else {
             let padding_bytes = match field_offset {
@@ -269,7 +272,7 @@ impl<'a> StructLayoutTracker<'a> {
                           self.latest_field_layout.unwrap().align) ||
                  layout.align > mem::size_of::<*mut ()>())
         {
-            let layout = if self.comp.packed() {
+            let layout = if self.is_packed {
                 Layout::new(padding_bytes, 1)
             } else if self.last_field_was_bitfield ||
                        layout.align > mem::size_of::<*mut ()>()
@@ -316,7 +319,7 @@ impl<'a> StructLayoutTracker<'a> {
     ///
     /// This is just to avoid doing the same check also in pad_field.
     fn align_to_latest_field(&mut self, new_field_layout: Layout) -> bool {
-        if self.comp.packed() {
+        if self.is_packed {
             // Skip to align fields when packed.
             return false;
         }

src/ir/comp.rs

@@ -974,8 +974,8 @@ pub struct CompInfo {
     /// size_t)
     has_non_type_template_params: bool,
 
-    /// Whether this struct layout is packed.
-    packed: bool,
+    /// Whether we saw `__attribute__((packed))` on or within this type.
+    packed_attr: bool,
 
     /// Used to know if we've found an opaque attribute that could cause us to
     /// generate a type with invalid layout. This is explicitly used to avoid us
@@ -1007,7 +1007,7 @@ impl CompInfo {
             has_destructor: false,
             has_nonempty_base: false,
             has_non_type_template_params: false,
-            packed: false,
+            packed_attr: false,
             found_unknown_attr: false,
             is_forward_declaration: false,
         }
@@ -1261,7 +1261,7 @@ impl CompInfo {
                     }
                 }
                 CXCursor_PackedAttr => {
-                    ci.packed = true;
+                    ci.packed_attr = true;
                 }
                 CXCursor_TemplateTypeParameter => {
                     let param = Item::type_param(None, cur, ctx)
@@ -1439,8 +1439,31 @@ impl CompInfo {
     }
 
     /// Is this compound type packed?
-    pub fn packed(&self) -> bool {
-        self.packed
+    pub fn is_packed(&self, ctx: &BindgenContext, layout: &Option<Layout>) -> bool {
+        if self.packed_attr {
+            return true
+        }
+
+        // Even though `libclang` doesn't expose `#pragma packed(...)`, we can
+        // detect it through its effects.
+        if let Some(ref parent_layout) = *layout {
+            if self.fields().iter().any(|f| match *f {
+                Field::Bitfields(ref unit) => {
+                    unit.layout().align > parent_layout.align
+                }
+                Field::DataMember(ref data) => {
+                    let field_ty = ctx.resolve_type(data.ty());
+                    field_ty.layout(ctx).map_or(false, |field_ty_layout| {
+                        field_ty_layout.align > parent_layout.align
+                    })
+                }
+            }) {
+                info!("Found a struct that was defined within `#pragma packed(...)`");
+                return true;
+            }
+        }
+
+        false
     }
 
     /// Returns true if compound type has been forward declared
@@ -1504,8 +1527,8 @@ impl DotAttributes for CompInfo {
             )?;
         }
 
-        if self.packed {
-            writeln!(out, "<tr><td>packed</td><td>true</td></tr>")?;
+        if self.packed_attr {
+            writeln!(out, "<tr><td>packed_attr</td><td>true</td></tr>")?;
         }
 
         if self.is_forward_declaration {
@@ -1528,15 +1551,17 @@ impl DotAttributes for CompInfo {
 }
 
 impl IsOpaque for CompInfo {
-    type Extra = ();
+    type Extra = Option<Layout>;
 
-    fn is_opaque(&self, ctx: &BindgenContext, _: &()) -> bool {
-        // Early return to avoid extra computation
+    fn is_opaque(&self, ctx: &BindgenContext, layout: &Option<Layout>) -> bool {
         if self.has_non_type_template_params {
             return true
         }
 
-        self.fields().iter().any(|f| match *f {
+        // Bitfields with a width that is larger than their unit's width have
+        // some strange things going on, and the best we can do is make the
+        // whole struct opaque.
+        if self.fields().iter().any(|f| match *f {
             Field::DataMember(_) => {
                 false
             },
@@ -1548,7 +1573,23 @@ impl IsOpaque for CompInfo {
                     bf.width() / 8 > bitfield_layout.size as u32
                 })
             }
-        })
+        }) {
+            return true;
+        }
+
+        // We don't have `#[repr(packed = "N")]` in Rust yet, so the best we can
+        // do is make this struct opaque.
+        //
+        // See https://github.com/rust-lang-nursery/rust-bindgen/issues/537 and
+        // https://github.com/rust-lang/rust/issues/33158
+        if self.is_packed(ctx, layout) && layout.map_or(false, |l| l.align > 1) {
+            warn!("Found a type that is both packed and aligned to greater than \
+                   1; Rust doesn't have `#[repr(packed = \"N\")]` yet, so we \
+                   are treating it as opaque");
+            return true;
+        }
+
+        false
     }
 }
 

src/ir/ty.rs

@@ -378,7 +378,7 @@ impl IsOpaque for Type {
             TypeKind::TemplateInstantiation(ref inst) => {
                 inst.is_opaque(ctx, item)
             }
-            TypeKind::Comp(ref comp) => comp.is_opaque(ctx, &()),
+            TypeKind::Comp(ref comp) => comp.is_opaque(ctx, &self.layout),
             TypeKind::ResolvedTypeRef(to) => to.is_opaque(ctx, &()),
             _ => false,
         }

tests/expectations/tests/divide-by-zero-in-struct-layout.rs

@@ -30,11 +30,12 @@ impl WithBitfieldAndAttrPacked {
         0
     }
 }
-#[repr(C)]
+#[repr(C, packed)]
 #[derive(Debug, Default, Copy, Clone)]
 pub struct WithBitfieldAndPacked {
     pub _bitfield_1: [u8; 0usize],
     pub a: ::std::os::raw::c_uint,
+    pub __bindgen_padding_0: u8,
 }
 impl WithBitfieldAndPacked {
     #[inline]

tests/expectations/tests/layout.rs

@@ -5,49 +5,8 @@
 
 
 #[repr(C)]
-#[derive(Default)]
-pub struct __IncompleteArrayField<T>(::std::marker::PhantomData<T>);
-impl<T> __IncompleteArrayField<T> {
-    #[inline]
-    pub fn new() -> Self {
-        __IncompleteArrayField(::std::marker::PhantomData)
-    }
-    #[inline]
-    pub unsafe fn as_ptr(&self) -> *const T {
-        ::std::mem::transmute(self)
-    }
-    #[inline]
-    pub unsafe fn as_mut_ptr(&mut self) -> *mut T {
-        ::std::mem::transmute(self)
-    }
-    #[inline]
-    pub unsafe fn as_slice(&self, len: usize) -> &[T] {
-        ::std::slice::from_raw_parts(self.as_ptr(), len)
-    }
-    #[inline]
-    pub unsafe fn as_mut_slice(&mut self, len: usize) -> &mut [T] {
-        ::std::slice::from_raw_parts_mut(self.as_mut_ptr(), len)
-    }
-}
-impl<T> ::std::fmt::Debug for __IncompleteArrayField<T> {
-    fn fmt(&self, fmt: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
-        fmt.write_str("__IncompleteArrayField")
-    }
-}
-impl<T> ::std::clone::Clone for __IncompleteArrayField<T> {
-    #[inline]
-    fn clone(&self) -> Self {
-        Self::new()
-    }
-}
-impl<T> ::std::marker::Copy for __IncompleteArrayField<T> {}
-#[repr(C, packed)]
-#[derive(Debug, Default)]
 pub struct header {
-    pub proto: ::std::os::raw::c_char,
-    pub size: ::std::os::raw::c_uint,
-    pub data: __IncompleteArrayField<::std::os::raw::c_uchar>,
-    pub __bindgen_padding_0: [u8; 11usize],
+    pub _bindgen_opaque_blob: [u8; 16usize],
 }
 #[test]
 fn bindgen_test_layout_header() {
@@ -57,3 +16,8 @@ fn bindgen_test_layout_header() {
         concat!("Size of: ", stringify!(header))
     );
 }
+impl Default for header {
+    fn default() -> Self {
+        unsafe { ::std::mem::zeroed() }
+    }
+}

tests/headers/issue-537.h

@@ -0,0 +1,35 @@
+/// This should not be opaque; we can see the attributes and can pack the
+/// struct.
+struct AlignedToOne {
+    int i;
+} __attribute__ ((packed,aligned(1)));
+
+/// This should be opaque because although we can see the attributes, Rust
+/// doesn't have `#[repr(packed = "N")]` yet.
+struct AlignedToTwo {
+    int i;
+} __attribute__ ((packed,aligned(2)));
+
+#pragma pack(1)
+
+/// This should not be opaque because although `libclang` doesn't give us the
+/// `#pragma pack(1)`, we can detect that alignment is 1 and add
+/// `#[repr(packed)]` to the struct ourselves.
+struct PackedToOne {
+    int x;
+    int y;
+};
+
+#pragma pack()
+
+#pragma pack(2)
+
+/// In this case, even if we can detect the weird alignment triggered by
+/// `#pragma pack(2)`, we can't do anything about it because Rust doesn't have
+/// `#[repr(packed = "N")]`. Therefore, we must make it opaque.
+struct PackedToTwo {
+    int x;
+    int y;
+};
+
+#pragma pack()