librustc: handle repr on structs, require it for ffi, unify with packed

As of RFC 18, struct layout is undefined. Opting into a C-compatible struct
layout is now down with #[repr(C)]. For consistency, specifying a packed
layout is now also down with #[repr(packed)]. Both can be specified.

[breaking-change]

Author: emberian

src/librustc/middle/lint.rs

@@ -60,6 +60,7 @@ use std::u16;
 use std::u32;
 use std::u64;
 use std::u8;
+use std::cell::RefCell;
 use collections::SmallIntMap;
 use syntax::abi;
 use syntax::ast_map;
@@ -482,6 +483,9 @@ struct Context<'a> {
     /// Level of lints for certain NodeIds, stored here because the body of
     /// the lint needs to run in trans.
     node_levels: HashMap<(ast::NodeId, Lint), (Level, LintSource)>,
+
+    /// ids of structs which have had a representation note emitted with ctypes
+    checked_ffi_structs: RefCell<NodeSet>,
 }
 
 pub fn emit_lint(level: Level, src: LintSource, msg: &str, span: Span,
@@ -923,26 +927,55 @@ fn check_type_limits(cx: &Context, e: &ast::Expr) {
 }
 
 fn check_item_ctypes(cx: &Context, it: &ast::Item) {
-    fn check_ty(cx: &Context, ty: &ast::Ty) {
-        match ty.node {
+    fn check_ty(cx: &Context, aty: &ast::Ty) {
+        match aty.node {
             ast::TyPath(_, _, id) => {
                 match cx.tcx.def_map.borrow().get_copy(&id) {
                     ast::DefPrimTy(ast::TyInt(ast::TyI)) => {
-                        cx.span_lint(CTypes, ty.span,
+                        cx.span_lint(CTypes, aty.span,
                                 "found rust type `int` in foreign module, while \
                                 libc::c_int or libc::c_long should be used");
                     }
                     ast::DefPrimTy(ast::TyUint(ast::TyU)) => {
-                        cx.span_lint(CTypes, ty.span,
+                        cx.span_lint(CTypes, aty.span,
                                 "found rust type `uint` in foreign module, while \
                                 libc::c_uint or libc::c_ulong should be used");
                     }
                     ast::DefTy(def_id) => {
-                        if !adt::is_ffi_safe(cx.tcx, def_id) {
-                            cx.span_lint(CTypes, ty.span,
-                                         "found enum type without foreign-function-safe \
-                                          representation annotation in foreign module");
-                            // hmm... this message could be more helpful
+                        match adt::is_ffi_safe(cx.tcx, def_id) {
+                            Ok(_) => { },
+                            Err(types) => {
+                                // in the enum case, we don't care about
+                                // "fields".
+
+                                let ty = ty::get(ty::lookup_item_type(cx.tcx, def_id).ty);
+
+                                match ty.sty {
+                                    ty::ty_struct(_, _) => {
+                                        cx.span_lint(CTypes, aty.span, "found struct without \
+                                                     FFI-safe representation used in FFI");
+
+                                        for def_id in types.iter() {
+                                            if !cx.checked_ffi_structs.borrow_mut()
+                                                                      .insert(def_id.node) {
+                                                return;
+                                            }
+
+                                            match cx.tcx.map.opt_span(def_id.node) {
+                                                Some(sp) => cx.tcx.sess.span_note(sp, "consider \
+                                                    adding `#[repr(C)]` to this type"),
+                                                None => { }
+                                            }
+                                        }
+                                    },
+                                    ty::ty_enum(_, _) => {
+                                        cx.span_lint(CTypes, aty.span,
+                                                     "found enum without FFI-safe representation \
+                                                     annotation used in FFI");
+                                    }
+                                    _ => { }
+                                }
+                            }
                         }
                     }
                     _ => ()
@@ -1100,6 +1133,7 @@ static obsolete_attrs: &'static [(&'static str, &'static str)] = &[
     ("fast_ffi", "Remove it"),
     ("fixed_stack_segment", "Remove it"),
     ("rust_stack", "Remove it"),
+    ("packed", "Use `#[repr(packed)]` instead")
 ];
 
 static other_attrs: &'static [&'static str] = &[
@@ -1109,7 +1143,7 @@ static other_attrs: &'static [&'static str] = &[
     "allow", "deny", "forbid", "warn", // lint options
     "deprecated", "experimental", "unstable", "stable", "locked", "frozen", //item stability
     "cfg", "doc", "export_name", "link_section",
-    "no_mangle", "static_assert", "unsafe_no_drop_flag", "packed",
+    "no_mangle", "static_assert", "unsafe_no_drop_flag",
     "simd", "repr", "deriving", "unsafe_destructor", "link", "phase",
     "macro_export", "must_use", "automatically_derived",
 
@@ -1178,6 +1212,10 @@ fn check_unused_attribute(cx: &Context, attrs: &[ast::Attribute]) {
         // FIXME: #14408 whitelist docs since rustdoc looks at them
         "doc",
 
+        // Just because a struct isn't used for FFI in *this* crate, doesn't
+        // mean it won't ever be.
+        "repr",
+
         // FIXME: #14406 these are processed in trans, which happens after the
         // lint pass
         "address_insignificant",
@@ -1189,7 +1227,6 @@ fn check_unused_attribute(cx: &Context, attrs: &[ast::Attribute]) {
         "no_builtins",
         "no_mangle",
         "no_split_stack",
-        "packed",
         "static_assert",
         "thread_local",
 
@@ -1977,6 +2014,7 @@ pub fn check_crate(tcx: &ty::ctxt,
         negated_expr_id: -1,
         checked_raw_pointers: NodeSet::new(),
         node_levels: HashMap::new(),
+        checked_ffi_structs: RefCell::new(NodeSet::new()),
     };
 
     // Install default lint levels, followed by the command line levels, and

src/librustc/middle/trans/adt.rs

@@ -45,10 +45,11 @@
 
 #![allow(unsigned_negate)]
 
-use std::container::Map;
 use libc::c_ulonglong;
-use std::num::{Bitwise};
+use std::container::Map;
+use std::num::Bitwise;
 use std::rc::Rc;
+use std;
 
 use lib::llvm::{ValueRef, True, IntEQ, IntNE};
 use middle::trans::_match;
@@ -165,7 +166,7 @@ fn represent_type_uncached(cx: &CrateContext, t: ty::t) -> Repr {
         }
         ty::ty_enum(def_id, ref substs) => {
             let cases = get_cases(cx.tcx(), def_id, substs);
-            let hint = ty::lookup_repr_hint(cx.tcx(), def_id);
+            let hint = ty::lookup_enum_repr_hint(cx.tcx(), def_id);
 
             if cases.len() == 0 {
                 // Uninhabitable; represent as unit
@@ -254,31 +255,60 @@ fn represent_type_uncached(cx: &CrateContext, t: ty::t) -> Repr {
 
 /// Determine, without doing translation, whether an ADT must be FFI-safe.
 /// For use in lint or similar, where being sound but slightly incomplete is acceptable.
-pub fn is_ffi_safe(tcx: &ty::ctxt, def_id: ast::DefId) -> bool {
+/// Returns Ok(()) if it is, and Err(causes) which is a vector of the DefId's
+/// of the types that are unsafe (either the type being checked itself if it
+/// lacks a repr attribute, or the fields of a struct).
+pub fn is_ffi_safe(tcx: &ty::ctxt, def_id: ast::DefId) -> std::result::Result<(), Vec<ast::DefId>> {
     match ty::get(ty::lookup_item_type(tcx, def_id).ty).sty {
         ty::ty_enum(def_id, _) => {
             let variants = ty::enum_variants(tcx, def_id);
             // Univariant => like struct/tuple.
             if variants.len() <= 1 {
-                return true;
+                return Ok(());
             }
-            let hint = ty::lookup_repr_hint(tcx, def_id);
+            let hint = ty::lookup_enum_repr_hint(tcx, def_id);
             // Appropriate representation explicitly selected?
             if hint.is_ffi_safe() {
-                return true;
+                return Ok(());
             }
             // Option<Box<T>> and similar are used in FFI.  Rather than try to
             // resolve type parameters and recognize this case exactly, this
             // overapproximates -- assuming that if a non-C-like enum is being
             // used in FFI then the user knows what they're doing.
             if variants.iter().any(|vi| !vi.args.is_empty()) {
-                return true;
+                return Ok(());
             }
-            false
-        }
-        // struct, tuple, etc.
+            Err(vec![def_id])
+        },
+        ty::ty_struct(def_id, _) => {
+            let struct_is_safe =
+                ty::lookup_struct_repr_hint(tcx, def_id).contains(&attr::ReprExtern);
+            let mut fields_are_safe = true;
+            let mut bad_fields = Vec::new();
+
+            if !struct_is_safe {
+                bad_fields.push(def_id);
+            }
+
+            for field in ty::lookup_struct_fields(tcx, def_id).iter() {
+                match is_ffi_safe(tcx, field.id) {
+                    Ok(_) => { }
+                    Err(types) => {
+                        fields_are_safe = false;
+                        bad_fields.extend(types.move_iter())
+                    }
+                }
+            }
+
+            if struct_is_safe && fields_are_safe {
+                Ok(())
+            } else {
+                Err(bad_fields)
+            }
+        },
+        // tuple, etc.
         // (is this right in the present of typedefs?)
-        _ => true
+        _ => Ok(())
     }
 }
 
@@ -370,6 +400,9 @@ fn range_to_inttype(cx: &CrateContext, hint: Hint, bounds: &IntBounds) -> IntTyp
         }
         attr::ReprAny => {
             attempts = choose_shortest;
+        },
+        attr::ReprPacked => {
+            cx.tcx.sess.bug("range_to_inttype: found ReprPacked on an enum");
         }
     }
     for &ity in attempts.iter() {

src/librustc/middle/ty.rs

@@ -3928,26 +3928,38 @@ pub fn has_attr(tcx: &ctxt, did: DefId, attr: &str) -> bool {
     found
 }
 
-/// Determine whether an item is annotated with `#[packed]`
+/// Determine whether an item is annotated with `#[repr(packed)]`
 pub fn lookup_packed(tcx: &ctxt, did: DefId) -> bool {
-    has_attr(tcx, did, "packed")
+    lookup_struct_repr_hint(tcx, did).contains(&attr::ReprPacked)
 }
 
 /// Determine whether an item is annotated with `#[simd]`
 pub fn lookup_simd(tcx: &ctxt, did: DefId) -> bool {
     has_attr(tcx, did, "simd")
 }
 
-// Obtain the representation annotation for a definition.
-pub fn lookup_repr_hint(tcx: &ctxt, did: DefId) -> attr::ReprAttr {
+/// Obtain the representation annotation for an enum definition.
+pub fn lookup_enum_repr_hint(tcx: &ctxt, did: DefId) -> attr::ReprAttr {
     let mut acc = attr::ReprAny;
     ty::each_attr(tcx, did, |meta| {
-        acc = attr::find_repr_attr(tcx.sess.diagnostic(), meta, acc);
+        acc = attr::find_enum_repr_attr(tcx.sess.diagnostic(), meta, acc);
         true
     });
     return acc;
 }
 
+/// Obtain the representation annotation for a struct definition.
+pub fn lookup_struct_repr_hint(tcx: &ctxt, did: DefId) -> Vec<attr::ReprAttr> {
+    let mut acc = Vec::new();
+
+    ty::each_attr(tcx, did, |meta| {
+        acc.extend(attr::find_struct_repr_attrs(tcx.sess.diagnostic(), meta).move_iter());
+        true
+    });
+
+    acc
+}
+
 // Look up a field ID, whether or not it's local
 // Takes a list of type substs in case the struct is generic
 pub fn lookup_field_type(tcx: &ctxt,

src/librustc/middle/typeck/check/mod.rs

@@ -3803,6 +3803,9 @@ pub fn check_enum_variants(ccx: &CrateCtxt,
                         ccx.tcx.sess.span_note(sp, "discriminant type specified here");
                     }
                 }
+                attr::ReprPacked => {
+                    ccx.tcx.sess.bug("range_to_inttype: found ReprPacked on an enum");
+                }
             }
             disr_vals.push(current_disr_val);
 
@@ -3816,7 +3819,7 @@ pub fn check_enum_variants(ccx: &CrateCtxt,
         return variants;
     }
 
-    let hint = ty::lookup_repr_hint(ccx.tcx, ast::DefId { krate: ast::LOCAL_CRATE, node: id });
+    let hint = ty::lookup_enum_repr_hint(ccx.tcx, ast::DefId { krate: ast::LOCAL_CRATE, node: id });
     if hint != attr::ReprAny && vs.len() <= 1 {
         let msg = if vs.len() == 1 {
             "unsupported representation for univariant enum"

src/libstd/rt/backtrace.rs

@@ -544,7 +544,7 @@ mod imp {
     static IMAGE_FILE_MACHINE_IA64: libc::DWORD = 0x0200;
     static IMAGE_FILE_MACHINE_AMD64: libc::DWORD = 0x8664;
 
-    #[packed]
+    #[repr(packed)]
     struct SYMBOL_INFO {
         SizeOfStruct: libc::c_ulong,
         TypeIndex: libc::c_ulong,

src/libsyntax/attr.rs

@@ -464,7 +464,7 @@ pub fn require_unique_names(diagnostic: &SpanHandler, metas: &[@MetaItem]) {
  * present (before fields, if any) with that type; reprensentation
  * optimizations which would remove it will not be done.
  */
-pub fn find_repr_attr(diagnostic: &SpanHandler, attr: &Attribute, acc: ReprAttr)
+pub fn find_enum_repr_attr(diagnostic: &SpanHandler, attr: &Attribute, acc: ReprAttr)
     -> ReprAttr {
     let mut acc = acc;
     match attr.node.value.node {
@@ -496,7 +496,7 @@ pub fn find_repr_attr(diagnostic: &SpanHandler, attr: &Attribute, acc: ReprAttr)
                         }
                     }
                     // Not a word:
-                    _ => diagnostic.span_err(item.span, "unrecognized representation hint")
+                    _ => diagnostic.span_err(item.span, "unrecognized enum representation hint")
                 }
             }
         }
@@ -506,6 +506,44 @@ pub fn find_repr_attr(diagnostic: &SpanHandler, attr: &Attribute, acc: ReprAttr)
     acc
 }
 
+/// A struct `repr` attribute can only take the values `C` and `packed`, or
+/// possibly both.
+pub fn find_struct_repr_attrs(diagnostic: &SpanHandler, attr: &Attribute) -> Vec<ReprAttr> {
+    let mut attrs = Vec::new();
+    match attr.node.value.node {
+        ast::MetaList(ref s, ref items) if s.equiv(&("repr")) => {
+            mark_used(attr);
+            for item in items.iter() {
+                match item.node {
+                    ast::MetaWord(ref word) => {
+                        let hint = match word.get() {
+                            "C" => Some(ReprExtern),
+                            "packed" => Some(ReprPacked),
+                            _ => {
+                                // Not a word we recognize
+                                diagnostic.span_err(item.span,
+                                                    "unrecognized struct representation hint");
+                                None
+                            }
+                        };
+
+                        match hint {
+                            Some(h) => attrs.push(h),
+                            None => { }
+                        }
+                    }
+                    // Not a word:
+                    _ => diagnostic.span_err(item.span, "unrecognized representation hint")
+                }
+            }
+        }
+        // Not a "repr" hint: ignore.
+        _ => { }
+    }
+
+    attrs
+}
+
 fn int_type_of_word(s: &str) -> Option<IntType> {
     match s {
         "i8" => Some(SignedInt(ast::TyI8)),
@@ -526,15 +564,17 @@ fn int_type_of_word(s: &str) -> Option<IntType> {
 pub enum ReprAttr {
     ReprAny,
     ReprInt(Span, IntType),
-    ReprExtern
+    ReprExtern,
+    ReprPacked,
 }
 
 impl ReprAttr {
     pub fn is_ffi_safe(&self) -> bool {
         match *self {
             ReprAny => false,
             ReprInt(_sp, ity) => ity.is_ffi_safe(),
-            ReprExtern => true
+            ReprExtern => true,
+            ReprPacked => false
         }
     }
 }
@@ -559,7 +599,7 @@ impl IntType {
             SignedInt(ast::TyI16) | UnsignedInt(ast::TyU16) |
             SignedInt(ast::TyI32) | UnsignedInt(ast::TyU32) |
             SignedInt(ast::TyI64) | UnsignedInt(ast::TyU64) => true,
-            _ => false
+            SignedInt(ast::TyI) | UnsignedInt(ast::TyU) => false
         }
     }
 }