Auto merge of #572 - fitzgen:sm-layout-test-failures, r=emilio

Generate better opaque blobs in the face of non-type parameters

When instantiating templates whose definitions have non-type generic parameters, prefer the layout of the instantiation type to the garbage we get from the definition's layout. In general, an instantiation's layout will always be a better choice than the definition's layout, regardless of non-type parameters.

Fixes #569

r? @emilio

Author: bors-servo

src/codegen/mod.rs

@@ -561,7 +561,19 @@ impl CodeGenerator for Type {
                     let layout = self.layout(ctx).unwrap_or_else(Layout::zero);
                     BlobTyBuilder::new(layout).build()
                 } else {
-                    inner_item.to_rust_ty(ctx)
+                    let inner_rust_ty = inner_item.to_rust_ty(ctx);
+
+                    // We get a unit if the inner type is a template definition
+                    // that is opaque or has non-type template parameters and
+                    // doesn't know its layout. Its possible that we have better
+                    // information about the layout, and in the worst case, just
+                    // make sure we don't return a zero-sized type.
+                    if inner_rust_ty == aster::AstBuilder::new().ty().unit() {
+                        let layout = self.layout(ctx).unwrap_or_else(|| Layout::for_size(1));
+                        BlobTyBuilder::new(layout).build()
+                    } else {
+                        inner_rust_ty
+                    }
                 };
 
                 {
@@ -2265,67 +2277,7 @@ impl ToRustTy for Type {
                 aster::AstBuilder::new().ty().path().ids(path).build()
             }
             TypeKind::TemplateInstantiation(ref inst) => {
-                let decl = inst.template_definition();
-                let mut ty = decl.to_rust_ty(ctx).unwrap();
-
-                // If we gave up when making a type for the template definition,
-                // check if maybe we can make a better opaque blob for the
-                // instantiation.
-                if ty == aster::AstBuilder::new().ty().unit().unwrap() {
-                    if let Some(layout) = self.layout(ctx) {
-                        ty = BlobTyBuilder::new(layout).build().unwrap()
-                    }
-                }
-
-                let decl_params = if let Some(params) =
-                    decl.self_template_params(ctx) {
-                    params
-                } else {
-                    // This can happen if we generated an opaque type for a
-                    // partial template specialization, in which case we just
-                    // use the opaque type's layout. If we don't have a layout,
-                    // we cross our fingers and hope for the best :-/
-                    debug_assert!(ctx.resolve_type_through_type_refs(decl)
-                        .is_opaque());
-                    let layout = self.layout(ctx).unwrap_or(Layout::zero());
-                    ty = BlobTyBuilder::new(layout).build().unwrap();
-
-                    vec![]
-                };
-
-                // TODO: If the decl type is a template class/struct
-                // declaration's member template declaration, it could rely on
-                // generic template parameters from its outer template
-                // class/struct. When we emit bindings for it, it could require
-                // *more* type arguments than we have here, and we will need to
-                // reconstruct them somehow. We don't have any means of doing
-                // that reconstruction at this time.
-
-                if let ast::TyKind::Path(_, ref mut path) = ty.node {
-                    let template_args = inst.template_arguments()
-                        .iter()
-                        .zip(decl_params.iter())
-                        // Only pass type arguments for the type parameters that
-                        // the decl uses.
-                        .filter(|&(_, param)| ctx.uses_template_parameter(decl, *param))
-                        .map(|(arg, _)| arg.to_rust_ty(ctx))
-                        .collect::<Vec<_>>();
-
-                    path.segments.last_mut().unwrap().parameters = if 
-                        template_args.is_empty() {
-                        None
-                    } else {
-                        Some(P(ast::PathParameters::AngleBracketed(
-                            ast::AngleBracketedParameterData {
-                                lifetimes: vec![],
-                                types: P::from_vec(template_args),
-                                bindings: P::from_vec(vec![]),
-                            }
-                        )))
-                    }
-                }
-
-                P(ty)
+                inst.to_rust_ty(ctx, self)
             }
             TypeKind::ResolvedTypeRef(inner) => inner.to_rust_ty(ctx),
             TypeKind::TemplateAlias(inner, _) |
@@ -2409,6 +2361,74 @@ impl ToRustTy for Type {
     }
 }
 
+impl ToRustTy for TemplateInstantiation {
+    type Extra = Type;
+
+    fn to_rust_ty(&self, ctx: &BindgenContext, self_ty: &Type) -> P<ast::Ty> {
+        let decl = self.template_definition();
+        let mut ty = decl.to_rust_ty(ctx).unwrap();
+
+        if ty == aster::AstBuilder::new().ty().unit().unwrap() {
+            // If we gave up when making a type for the template definition,
+            // check if maybe we can make a better opaque blob for the
+            // instantiation. If not, at least don't use a zero-sized type.
+            if let Some(layout) = self_ty.layout(ctx) {
+                return BlobTyBuilder::new(layout).build();
+            } else {
+                return quote_ty!(ctx.ext_cx(), u8);
+            }
+        }
+
+        let decl_params = match decl.self_template_params(ctx) {
+            Some(params) => params,
+            None => {
+                // This can happen if we generated an opaque type for a
+                // partial template specialization, in which case we just
+                // use the opaque type's layout. If we don't have a layout,
+                // we cross our fingers and hope for the best :-/
+                debug_assert!(ctx.resolve_type_through_type_refs(decl)
+                              .is_opaque());
+                let layout = self_ty.layout(ctx).unwrap_or(Layout::zero());
+                return BlobTyBuilder::new(layout).build();
+            }
+        };
+
+        // TODO: If the decl type is a template class/struct
+        // declaration's member template declaration, it could rely on
+        // generic template parameters from its outer template
+        // class/struct. When we emit bindings for it, it could require
+        // *more* type arguments than we have here, and we will need to
+        // reconstruct them somehow. We don't have any means of doing
+        // that reconstruction at this time.
+
+        if let ast::TyKind::Path(_, ref mut path) = ty.node {
+            let template_args = self.template_arguments()
+                .iter()
+                .zip(decl_params.iter())
+                // Only pass type arguments for the type parameters that
+                // the decl uses.
+                .filter(|&(_, param)| ctx.uses_template_parameter(decl, *param))
+                .map(|(arg, _)| arg.to_rust_ty(ctx))
+                .collect::<Vec<_>>();
+
+            path.segments.last_mut().unwrap().parameters = if 
+                template_args.is_empty() {
+                None
+            } else {
+                Some(P(ast::PathParameters::AngleBracketed(
+                    ast::AngleBracketedParameterData {
+                        lifetimes: vec![],
+                        types: P::from_vec(template_args),
+                        bindings: P::from_vec(vec![]),
+                    }
+                )))
+            }
+        }
+
+        P(ty)
+    }
+}
+
 impl ToRustTy for FunctionSig {
     type Extra = Item;
 

tests/expectations/tests/issue-569-non-type-template-params-causing-layout-test-failures.rs

@@ -0,0 +1,42 @@
+/* automatically generated by rust-bindgen */
+
+
+#![allow(non_snake_case)]
+
+
+pub const ENUM_VARIANT_1: _bindgen_ty_1 = _bindgen_ty_1::ENUM_VARIANT_1;
+pub const ENUM_VARIANT_2: _bindgen_ty_1 = _bindgen_ty_1::ENUM_VARIANT_2;
+#[repr(u32)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
+pub enum _bindgen_ty_1 { ENUM_VARIANT_1 = 0, ENUM_VARIANT_2 = 1, }
+pub type JS_Alias = u8;
+#[repr(C)]
+pub struct JS_Base {
+    pub f: JS_Alias,
+}
+impl Default for JS_Base {
+    fn default() -> Self { unsafe { ::std::mem::zeroed() } }
+}
+#[repr(C)]
+pub struct JS_AutoIdVector {
+    pub _base: JS_Base,
+}
+#[test]
+fn bindgen_test_layout_JS_AutoIdVector() {
+    assert_eq!(::std::mem::size_of::<JS_AutoIdVector>() , 1usize , concat ! (
+               "Size of: " , stringify ! ( JS_AutoIdVector ) ));
+    assert_eq! (::std::mem::align_of::<JS_AutoIdVector>() , 1usize , concat !
+                ( "Alignment of " , stringify ! ( JS_AutoIdVector ) ));
+}
+impl Default for JS_AutoIdVector {
+    fn default() -> Self { unsafe { ::std::mem::zeroed() } }
+}
+#[test]
+fn __bindgen_test_layout_JS_Base_instantiation_16() {
+    assert_eq!(::std::mem::size_of::<JS_Base>() , 1usize , concat ! (
+               "Size of template specialization: " , stringify ! ( JS_Base )
+               ));
+    assert_eq!(::std::mem::align_of::<JS_Base>() , 1usize , concat ! (
+               "Alignment of template specialization: " , stringify ! (
+               JS_Base ) ));
+}

tests/expectations/tests/non-type-params.rs

@@ -0,0 +1,48 @@
+/* automatically generated by rust-bindgen */
+
+
+#![allow(non_snake_case)]
+
+
+pub type Array16 = u8;
+pub type ArrayInt4 = [u32; 4usize];
+#[repr(C)]
+pub struct UsesArray {
+    pub array_char_16: [u8; 16usize],
+    pub array_bool_8: [u8; 8usize],
+    pub array_int_4: ArrayInt4,
+}
+#[test]
+fn bindgen_test_layout_UsesArray() {
+    assert_eq!(::std::mem::size_of::<UsesArray>() , 40usize , concat ! (
+               "Size of: " , stringify ! ( UsesArray ) ));
+    assert_eq! (::std::mem::align_of::<UsesArray>() , 4usize , concat ! (
+                "Alignment of " , stringify ! ( UsesArray ) ));
+    assert_eq! (unsafe {
+                & ( * ( 0 as * const UsesArray ) ) . array_char_16 as * const
+                _ as usize } , 0usize , concat ! (
+                "Alignment of field: " , stringify ! ( UsesArray ) , "::" ,
+                stringify ! ( array_char_16 ) ));
+    assert_eq! (unsafe {
+                & ( * ( 0 as * const UsesArray ) ) . array_bool_8 as * const _
+                as usize } , 16usize , concat ! (
+                "Alignment of field: " , stringify ! ( UsesArray ) , "::" ,
+                stringify ! ( array_bool_8 ) ));
+    assert_eq! (unsafe {
+                & ( * ( 0 as * const UsesArray ) ) . array_int_4 as * const _
+                as usize } , 24usize , concat ! (
+                "Alignment of field: " , stringify ! ( UsesArray ) , "::" ,
+                stringify ! ( array_int_4 ) ));
+}
+impl Default for UsesArray {
+    fn default() -> Self { unsafe { ::std::mem::zeroed() } }
+}
+#[test]
+fn __bindgen_test_layout_Array_instantiation_18() {
+    assert_eq!(::std::mem::size_of::<[u32; 4usize]>() , 16usize , concat ! (
+               "Size of template specialization: " , stringify ! (
+               [u32; 4usize] ) ));
+    assert_eq!(::std::mem::align_of::<[u32; 4usize]>() , 4usize , concat ! (
+               "Alignment of template specialization: " , stringify ! (
+               [u32; 4usize] ) ));
+}

tests/expectations/tests/opaque_pointer.rs

@@ -35,7 +35,7 @@ impl Default for Opaque {
 #[repr(C)]
 #[derive(Debug, Copy)]
 pub struct WithOpaquePtr {
-    pub whatever: *mut (),
+    pub whatever: *mut u8,
     pub other: u32,
     pub t: OtherOpaque,
 }

tests/expectations/tests/type_alias_empty.rs

@@ -4,4 +4,4 @@
 #![allow(non_snake_case)]
 
 
-pub type bool_constant = ();
+pub type bool_constant = u8;

tests/headers/issue-569-non-type-template-params-causing-layout-test-failures.hpp

@@ -0,0 +1,27 @@
+// bindgen-flags: -- -std=c++14
+
+// Generated by C-Reduce, cleaned up and given names for readability.
+
+template <int, typename>
+struct HasNonTypeTemplateParam {
+    // But doesn't use the non-type template param nor its type param...
+};
+
+enum {
+    ENUM_VARIANT_1,
+    ENUM_VARIANT_2
+};
+
+namespace JS {
+
+template <typename T>
+using Alias = HasNonTypeTemplateParam<ENUM_VARIANT_1, T>;
+
+template <typename U>
+class Base {
+    Alias<U> f;
+};
+
+class AutoIdVector : Base<int> {};
+
+}

tests/headers/non-type-params.hpp

@@ -0,0 +1,17 @@
+// bindgen-flags: -- -std=c++14
+
+template <typename T, unsigned int Capacity>
+struct Array {
+    T elements[Capacity];
+};
+
+template <typename T>
+using Array16 = Array<T, 16>;
+
+using ArrayInt4 = Array<int, 4>;
+
+struct UsesArray {
+    Array16<char>  array_char_16;
+    Array<bool, 8> array_bool_8;
+    ArrayInt4      array_int_4;
+};