Auto merge of rust-lang#44884 - arielb1:pack-safe, r=nikomatsakis,eddyb

Make accesses to fields of packed structs unsafe

To handle packed structs with destructors (which you'll think are a rare
case, but the `#[repr(packed)] struct Packed<T>(T);` pattern is
ever-popular, which requires handling packed structs with destructors to
avoid monomorphization-time errors), drops of subfields of packed
structs should drop a local move of the field instead of the original
one.

That's it, I think I'll use a strategy suggested by @Zoxc, where this mir
```
drop(packed_struct.field)
```

is replaced by
```
tmp0 = packed_struct.field;
drop tmp0
```

cc rust-lang#27060 - this should deal with that issue after codegen of drop glue
is updated.

The new errors need to be changed to future-compatibility warnings, but
I'll rather do a crater run first with them as errors to assess the
impact.

cc @eddyb

Things which still need to be done for this:
 - [ ] - handle `repr(packed)` structs in `derive` the same way I did in `Span`, and use derive there again
 - [ ] - implement the "fix packed drops" pass and call it in both the MIR shim and validated MIR pipelines
 - [ ] - do a crater run
 - [ ] - convert the errors to compatibility warnings

Author: bors

src/Cargo.lock

@@ -479,6 +479,7 @@ define_dep_nodes!( <'tcx>
     [] BorrowCheck(DefId),
     [] MirBorrowCheck(DefId),
     [] UnsafetyCheckResult(DefId),
+    [] UnsafeDeriveOnReprPacked(DefId),
 
     [] Reachability,
     [] MirKeys,

src/librustc/dep_graph/dep_node.rs

@@ -33,9 +33,28 @@ impl_stable_hash_for!(struct mir::LocalDecl<'tcx> {
 });
 impl_stable_hash_for!(struct mir::UpvarDecl { debug_name, by_ref });
 impl_stable_hash_for!(struct mir::BasicBlockData<'tcx> { statements, terminator, is_cleanup });
-impl_stable_hash_for!(struct mir::UnsafetyViolation { source_info, description, lint_node_id });
+impl_stable_hash_for!(struct mir::UnsafetyViolation { source_info, description, kind });
 impl_stable_hash_for!(struct mir::UnsafetyCheckResult { violations, unsafe_blocks });
 
+impl<'gcx> HashStable<StableHashingContext<'gcx>>
+for mir::UnsafetyViolationKind {
+    #[inline]
+    fn hash_stable<W: StableHasherResult>(&self,
+                                          hcx: &mut StableHashingContext<'gcx>,
+                                          hasher: &mut StableHasher<W>) {
+
+        mem::discriminant(self).hash_stable(hcx, hasher);
+
+        match *self {
+            mir::UnsafetyViolationKind::General => {}
+            mir::UnsafetyViolationKind::ExternStatic(lint_node_id) |
+            mir::UnsafetyViolationKind::BorrowPacked(lint_node_id) => {
+                lint_node_id.hash_stable(hcx, hasher);
+            }
+
+        }
+    }
+}
 impl<'gcx> HashStable<StableHashingContext<'gcx>>
 for mir::Terminator<'gcx> {
     #[inline]

src/librustc/ich/impls_mir.rs

@@ -155,6 +155,12 @@ declare_lint! {
     "safe access to extern statics was erroneously allowed"
 }
 
+declare_lint! {
+    pub SAFE_PACKED_BORROWS,
+    Warn,
+    "safe borrows of fields of packed structs were was erroneously allowed"
+}
+
 declare_lint! {
     pub PATTERNS_IN_FNS_WITHOUT_BODY,
     Warn,
@@ -247,6 +253,7 @@ impl LintPass for HardwiredLints {
             RENAMED_AND_REMOVED_LINTS,
             RESOLVE_TRAIT_ON_DEFAULTED_UNIT,
             SAFE_EXTERN_STATICS,
+            SAFE_PACKED_BORROWS,
             PATTERNS_IN_FNS_WITHOUT_BODY,
             LEGACY_DIRECTORY_OWNERSHIP,
             LEGACY_IMPORTS,

src/librustc/lint/builtin.rs

@@ -1722,11 +1722,18 @@ impl Location {
     }
 }
 
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
+pub enum UnsafetyViolationKind {
+    General,
+    ExternStatic(ast::NodeId),
+    BorrowPacked(ast::NodeId),
+}
+
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
 pub struct UnsafetyViolation {
     pub source_info: SourceInfo,
     pub description: &'static str,
-    pub lint_node_id: Option<ast::NodeId>,
+    pub kind: UnsafetyViolationKind,
 }
 
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]

src/librustc/mir/mod.rs

@@ -19,8 +19,18 @@ use ty::subst::Subst;
 
 use infer::{InferCtxt, InferOk};
 
-#[derive(Copy, Clone)]
-struct InferIsLocal(bool);
+#[derive(Copy, Clone, Debug)]
+enum InferIsLocal {
+    BrokenYes,
+    Yes,
+    No
+}
+
+#[derive(Debug, Copy, Clone)]
+pub enum Conflict {
+    Upstream,
+    Downstream
+}
 
 pub struct OverlapResult<'tcx> {
     pub impl_header: ty::ImplHeader<'tcx>,
@@ -126,32 +136,46 @@ fn overlap<'cx, 'gcx, 'tcx>(selcx: &mut SelectionContext<'cx, 'gcx, 'tcx>,
 }
 
 pub fn trait_ref_is_knowable<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
-                                             trait_ref: ty::TraitRef<'tcx>) -> bool
+                                             trait_ref: ty::TraitRef<'tcx>,
+                                             broken: bool)
+                                             -> Option<Conflict>
 {
-    debug!("trait_ref_is_knowable(trait_ref={:?})", trait_ref);
-
-    // if the orphan rules pass, that means that no ancestor crate can
-    // impl this, so it's up to us.
-    if orphan_check_trait_ref(tcx, trait_ref, InferIsLocal(false)).is_ok() {
-        debug!("trait_ref_is_knowable: orphan check passed");
-        return true;
+    debug!("trait_ref_is_knowable(trait_ref={:?}, broken={:?})", trait_ref, broken);
+    let mode = if broken {
+        InferIsLocal::BrokenYes
+    } else {
+        InferIsLocal::Yes
+    };
+    if orphan_check_trait_ref(tcx, trait_ref, mode).is_ok() {
+        // A downstream or cousin crate is allowed to implement some
+        // substitution of this trait-ref.
+        debug!("trait_ref_is_knowable: downstream crate might implement");
+        return Some(Conflict::Downstream);
     }
 
-    // if the trait is not marked fundamental, then it's always possible that
-    // an ancestor crate will impl this in the future, if they haven't
-    // already
-    if !trait_ref_is_local_or_fundamental(tcx, trait_ref) {
-        debug!("trait_ref_is_knowable: trait is neither local nor fundamental");
-        return false;
+    if trait_ref_is_local_or_fundamental(tcx, trait_ref) {
+        // This is a local or fundamental trait, so future-compatibility
+        // is no concern. We know that downstream/cousin crates are not
+        // allowed to implement a substitution of this trait ref, which
+        // means impls could only come from dependencies of this crate,
+        // which we already know about.
+        return None;
+    }
+    // This is a remote non-fundamental trait, so if another crate
+    // can be the "final owner" of a substitution of this trait-ref,
+    // they are allowed to implement it future-compatibly.
+    //
+    // However, if we are a final owner, then nobody else can be,
+    // and if we are an intermediate owner, then we don't care
+    // about future-compatibility, which means that we're OK if
+    // we are an owner.
+    if orphan_check_trait_ref(tcx, trait_ref, InferIsLocal::No).is_ok() {
+        debug!("trait_ref_is_knowable: orphan check passed");
+        return None;
+    } else {
+        debug!("trait_ref_is_knowable: nonlocal, nonfundamental, unowned");
+        return Some(Conflict::Upstream);
     }
-
-    // find out when some downstream (or cousin) crate could impl this
-    // trait-ref, presuming that all the parameters were instantiated
-    // with downstream types. If not, then it could only be
-    // implemented by an upstream crate, which means that the impl
-    // must be visible to us, and -- since the trait is fundamental
-    // -- we can test.
-    orphan_check_trait_ref(tcx, trait_ref, InferIsLocal(true)).is_err()
 }
 
 pub fn trait_ref_is_local_or_fundamental<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
@@ -189,16 +213,16 @@ pub fn orphan_check<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
         return Ok(());
     }
 
-    orphan_check_trait_ref(tcx, trait_ref, InferIsLocal(false))
+    orphan_check_trait_ref(tcx, trait_ref, InferIsLocal::No)
 }
 
 fn orphan_check_trait_ref<'tcx>(tcx: TyCtxt,
                                 trait_ref: ty::TraitRef<'tcx>,
                                 infer_is_local: InferIsLocal)
                                 -> Result<(), OrphanCheckErr<'tcx>>
 {
-    debug!("orphan_check_trait_ref(trait_ref={:?}, infer_is_local={})",
-           trait_ref, infer_is_local.0);
+    debug!("orphan_check_trait_ref(trait_ref={:?}, infer_is_local={:?})",
+           trait_ref, infer_is_local);
 
     // First, create an ordered iterator over all the type parameters to the trait, with the self
     // type appearing first.
@@ -212,7 +236,9 @@ fn orphan_check_trait_ref<'tcx>(tcx: TyCtxt,
             // uncovered type parameters.
             let uncovered_tys = uncovered_tys(tcx, input_ty, infer_is_local);
             for uncovered_ty in uncovered_tys {
-                if let Some(param) = uncovered_ty.walk().find(|t| is_type_parameter(t)) {
+                if let Some(param) = uncovered_ty.walk()
+                    .find(|t| is_possibly_remote_type(t, infer_is_local))
+                {
                     debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
                     return Err(OrphanCheckErr::UncoveredTy(param));
                 }
@@ -224,11 +250,11 @@ fn orphan_check_trait_ref<'tcx>(tcx: TyCtxt,
 
         // Otherwise, enforce invariant that there are no type
         // parameters reachable.
-        if !infer_is_local.0 {
-            if let Some(param) = input_ty.walk().find(|t| is_type_parameter(t)) {
-                debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
-                return Err(OrphanCheckErr::UncoveredTy(param));
-            }
+        if let Some(param) = input_ty.walk()
+            .find(|t| is_possibly_remote_type(t, infer_is_local))
+        {
+            debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
+            return Err(OrphanCheckErr::UncoveredTy(param));
         }
     }
 
@@ -250,7 +276,7 @@ fn uncovered_tys<'tcx>(tcx: TyCtxt, ty: Ty<'tcx>, infer_is_local: InferIsLocal)
     }
 }
 
-fn is_type_parameter(ty: Ty) -> bool {
+fn is_possibly_remote_type(ty: Ty, _infer_is_local: InferIsLocal) -> bool {
     match ty.sty {
         ty::TyProjection(..) | ty::TyParam(..) => true,
         _ => false,
@@ -273,7 +299,15 @@ fn fundamental_ty(tcx: TyCtxt, ty: Ty) -> bool {
     }
 }
 
-fn ty_is_local_constructor(ty: Ty, infer_is_local: InferIsLocal)-> bool {
+fn def_id_is_local(def_id: DefId, infer_is_local: InferIsLocal) -> bool {
+    match infer_is_local {
+        InferIsLocal::Yes => false,
+        InferIsLocal::No |
+        InferIsLocal::BrokenYes => def_id.is_local()
+    }
+}
+
+fn ty_is_local_constructor(ty: Ty, infer_is_local: InferIsLocal) -> bool {
     debug!("ty_is_local_constructor({:?})", ty);
 
     match ty.sty {
@@ -296,20 +330,19 @@ fn ty_is_local_constructor(ty: Ty, infer_is_local: InferIsLocal)-> bool {
             false
         }
 
-        ty::TyInfer(..) => {
-            infer_is_local.0
-        }
-
-        ty::TyAdt(def, _) => {
-            def.did.is_local()
-        }
+        ty::TyInfer(..) => match infer_is_local {
+            InferIsLocal::No => false,
+            InferIsLocal::Yes |
+            InferIsLocal::BrokenYes => true
+        },
 
-        ty::TyForeign(did) => {
-            did.is_local()
-        }
+        ty::TyAdt(def, _) => def_id_is_local(def.did, infer_is_local),
+        ty::TyForeign(did) => def_id_is_local(did, infer_is_local),
 
         ty::TyDynamic(ref tt, ..) => {
-            tt.principal().map_or(false, |p| p.def_id().is_local())
+            tt.principal().map_or(false, |p| {
+                def_id_is_local(p.def_id(), infer_is_local)
+            })
         }
 
         ty::TyError => {

src/librustc/traits/coherence.rs

@@ -814,7 +814,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
         // terms of `Fn` etc, but we could probably make this more
         // precise still.
         let unbound_input_types = stack.fresh_trait_ref.input_types().any(|ty| ty.is_fresh());
-        if unbound_input_types && self.intercrate {
+        if unbound_input_types && self.intercrate && false {
             debug!("evaluate_stack({:?}) --> unbound argument, intercrate -->  ambiguous",
                    stack.fresh_trait_ref);
             // Heuristics: show the diagnostics when there are no candidates in crate.
@@ -1221,7 +1221,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
         // bound regions
         let trait_ref = predicate.skip_binder().trait_ref;
 
-        coherence::trait_ref_is_knowable(self.tcx(), trait_ref)
+        coherence::trait_ref_is_knowable(self.tcx(), trait_ref, false).is_none()
     }
 
     /// Returns true if the global caches can be used.

src/librustc/traits/select.rs

@@ -169,6 +169,9 @@ define_maps! { <'tcx>
     /// The result of unsafety-checking this def-id.
     [] fn unsafety_check_result: UnsafetyCheckResult(DefId) -> mir::UnsafetyCheckResult,
 
+    /// HACK: when evaluated, this reports a "unsafe derive on repr(packed)" error
+    [] fn unsafe_derive_on_repr_packed: UnsafeDeriveOnReprPacked(DefId) -> (),
+
     /// The signature of functions and closures.
     [] fn fn_sig: FnSignature(DefId) -> ty::PolyFnSig<'tcx>,
 

src/librustc/ty/maps/mod.rs

@@ -768,6 +768,7 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
         DepKind::BorrowCheck => { force!(borrowck, def_id!()); }
         DepKind::MirBorrowCheck => { force!(mir_borrowck, def_id!()); }
         DepKind::UnsafetyCheckResult => { force!(unsafety_check_result, def_id!()); }
+        DepKind::UnsafeDeriveOnReprPacked => { force!(unsafe_derive_on_repr_packed, def_id!()); }
         DepKind::Reachability => { force!(reachable_set, LOCAL_CRATE); }
         DepKind::MirKeys => { force!(mir_keys, LOCAL_CRATE); }
         DepKind::CrateVariances => { force!(crate_variances, LOCAL_CRATE); }

src/librustc/ty/maps/plumbing.rs

@@ -243,6 +243,11 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
             id: LintId::of(LATE_BOUND_LIFETIME_ARGUMENTS),
             reference: "issue #42868 <https://github.com/rust-lang/rust/issues/42868>",
         },
+        FutureIncompatibleInfo {
+            id: LintId::of(SAFE_PACKED_BORROWS),
+            reference: "issue #46043 <https://github.com/rust-lang/rust/issues/46043>",
+        },
+
         ]);
 
     // Register renamed and removed lints

src/librustc_lint/lib.rs

@@ -27,7 +27,8 @@ use syntax_pos::Span;
 use std::fmt;
 use std::iter;
 
-use transform::{add_call_guards, no_landing_pads, simplify};
+use transform::{add_moves_for_packed_drops, add_call_guards};
+use transform::{no_landing_pads, simplify};
 use util::elaborate_drops::{self, DropElaborator, DropStyle, DropFlagMode};
 use util::patch::MirPatch;
 
@@ -114,6 +115,8 @@ fn make_shim<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
         }
     };
     debug!("make_shim({:?}) = untransformed {:?}", instance, result);
+    add_moves_for_packed_drops::add_moves_for_packed_drops(
+        tcx, &mut result, instance.def_id());
     no_landing_pads::no_landing_pads(tcx, &mut result);
     simplify::simplify_cfg(&mut result);
     add_call_guards::CriticalCallEdges.add_call_guards(&mut result);