Auto merge of rust-lang#82834 - nikic:mutable-noalias, r=nagisa

Enable mutable noalias for LLVM >= 12

Enable mutable noalias by default on LLVM 12, as previously known miscompiles have been resolved. Now it's time to find the next one ;)

 * The `-Z mutable-noalias` option no longer has an explicit default and accepts `-Z mutable-noalias=yes` and `-Z mutable-noalias=no` to override the LLVM version based default behavior.
 * The decision on whether to apply the noalias attribute is moved into rustc_codegen_llvm. rustc_middle only provides us with the necessary information to make the decision.
 * `noalias` is not emitted for types that are `!Unpin`, as a heuristic for self-referential structures (see rust-lang#54878 and rust-lang#63818).

Author: bors

compiler/rustc_codegen_llvm/src/abi.rs

@@ -1,6 +1,7 @@
 use crate::builder::Builder;
 use crate::context::CodegenCx;
 use crate::llvm::{self, AttributePlace};
+use crate::llvm_util;
 use crate::type_::Type;
 use crate::type_of::LayoutLlvmExt;
 use crate::value::Value;
@@ -41,12 +42,29 @@ impl ArgAttributeExt for ArgAttribute {
 }
 
 pub trait ArgAttributesExt {
-    fn apply_attrs_to_llfn(&self, idx: AttributePlace, llfn: &Value);
-    fn apply_attrs_to_callsite(&self, idx: AttributePlace, callsite: &Value);
+    fn apply_attrs_to_llfn(&self, idx: AttributePlace, cx: &CodegenCx<'_, '_>, llfn: &Value);
+    fn apply_attrs_to_callsite(
+        &self,
+        idx: AttributePlace,
+        cx: &CodegenCx<'_, '_>,
+        callsite: &Value,
+    );
+}
+
+fn should_use_mutable_noalias(cx: &CodegenCx<'_, '_>) -> bool {
+    // LLVM prior to version 12 has known miscompiles in the presence of
+    // noalias attributes (see #54878). Only enable mutable noalias by
+    // default for versions we believe to be safe.
+    cx.tcx
+        .sess
+        .opts
+        .debugging_opts
+        .mutable_noalias
+        .unwrap_or_else(|| llvm_util::get_version() >= (12, 0, 0))
 }
 
 impl ArgAttributesExt for ArgAttributes {
-    fn apply_attrs_to_llfn(&self, idx: AttributePlace, llfn: &Value) {
+    fn apply_attrs_to_llfn(&self, idx: AttributePlace, cx: &CodegenCx<'_, '_>, llfn: &Value) {
         let mut regular = self.regular;
         unsafe {
             let deref = self.pointee_size.bytes();
@@ -62,6 +80,9 @@ impl ArgAttributesExt for ArgAttributes {
                 llvm::LLVMRustAddAlignmentAttr(llfn, idx.as_uint(), align.bytes() as u32);
             }
             regular.for_each_kind(|attr| attr.apply_llfn(idx, llfn));
+            if regular.contains(ArgAttribute::NoAliasMutRef) && should_use_mutable_noalias(cx) {
+                llvm::Attribute::NoAlias.apply_llfn(idx, llfn);
+            }
             match self.arg_ext {
                 ArgExtension::None => {}
                 ArgExtension::Zext => {
@@ -74,7 +95,12 @@ impl ArgAttributesExt for ArgAttributes {
         }
     }
 
-    fn apply_attrs_to_callsite(&self, idx: AttributePlace, callsite: &Value) {
+    fn apply_attrs_to_callsite(
+        &self,
+        idx: AttributePlace,
+        cx: &CodegenCx<'_, '_>,
+        callsite: &Value,
+    ) {
         let mut regular = self.regular;
         unsafe {
             let deref = self.pointee_size.bytes();
@@ -98,6 +124,9 @@ impl ArgAttributesExt for ArgAttributes {
                 );
             }
             regular.for_each_kind(|attr| attr.apply_callsite(idx, callsite));
+            if regular.contains(ArgAttribute::NoAliasMutRef) && should_use_mutable_noalias(cx) {
+                llvm::Attribute::NoAlias.apply_callsite(idx, callsite);
+            }
             match self.arg_ext {
                 ArgExtension::None => {}
                 ArgExtension::Zext => {
@@ -419,13 +448,13 @@ impl<'tcx> FnAbiLlvmExt<'tcx> for FnAbi<'tcx, Ty<'tcx>> {
 
         let mut i = 0;
         let mut apply = |attrs: &ArgAttributes| {
-            attrs.apply_attrs_to_llfn(llvm::AttributePlace::Argument(i), llfn);
+            attrs.apply_attrs_to_llfn(llvm::AttributePlace::Argument(i), cx, llfn);
             i += 1;
             i - 1
         };
         match self.ret.mode {
             PassMode::Direct(ref attrs) => {
-                attrs.apply_attrs_to_llfn(llvm::AttributePlace::ReturnValue, llfn);
+                attrs.apply_attrs_to_llfn(llvm::AttributePlace::ReturnValue, cx, llfn);
             }
             PassMode::Indirect { ref attrs, extra_attrs: _, on_stack } => {
                 assert!(!on_stack);
@@ -480,18 +509,18 @@ impl<'tcx> FnAbiLlvmExt<'tcx> for FnAbi<'tcx, Ty<'tcx>> {
         // FIXME(wesleywiser, eddyb): We should apply `nounwind` and `noreturn` as appropriate to this callsite.
 
         let mut i = 0;
-        let mut apply = |attrs: &ArgAttributes| {
-            attrs.apply_attrs_to_callsite(llvm::AttributePlace::Argument(i), callsite);
+        let mut apply = |cx: &CodegenCx<'_, '_>, attrs: &ArgAttributes| {
+            attrs.apply_attrs_to_callsite(llvm::AttributePlace::Argument(i), cx, callsite);
             i += 1;
             i - 1
         };
         match self.ret.mode {
             PassMode::Direct(ref attrs) => {
-                attrs.apply_attrs_to_callsite(llvm::AttributePlace::ReturnValue, callsite);
+                attrs.apply_attrs_to_callsite(llvm::AttributePlace::ReturnValue, &bx.cx, callsite);
             }
             PassMode::Indirect { ref attrs, extra_attrs: _, on_stack } => {
                 assert!(!on_stack);
-                let i = apply(attrs);
+                let i = apply(bx.cx, attrs);
                 unsafe {
                     llvm::LLVMRustAddStructRetCallSiteAttr(
                         callsite,
@@ -517,12 +546,12 @@ impl<'tcx> FnAbiLlvmExt<'tcx> for FnAbi<'tcx, Ty<'tcx>> {
         }
         for arg in &self.args {
             if arg.pad.is_some() {
-                apply(&ArgAttributes::new());
+                apply(bx.cx, &ArgAttributes::new());
             }
             match arg.mode {
                 PassMode::Ignore => {}
                 PassMode::Indirect { ref attrs, extra_attrs: None, on_stack: true } => {
-                    let i = apply(attrs);
+                    let i = apply(bx.cx, attrs);
                     unsafe {
                         llvm::LLVMRustAddByValCallSiteAttr(
                             callsite,
@@ -533,22 +562,22 @@ impl<'tcx> FnAbiLlvmExt<'tcx> for FnAbi<'tcx, Ty<'tcx>> {
                 }
                 PassMode::Direct(ref attrs)
                 | PassMode::Indirect { ref attrs, extra_attrs: None, on_stack: false } => {
-                    apply(attrs);
+                    apply(bx.cx, attrs);
                 }
                 PassMode::Indirect {
                     ref attrs,
                     extra_attrs: Some(ref extra_attrs),
                     on_stack: _,
                 } => {
-                    apply(attrs);
-                    apply(extra_attrs);
+                    apply(bx.cx, attrs);
+                    apply(bx.cx, extra_attrs);
                 }
                 PassMode::Pair(ref a, ref b) => {
-                    apply(a);
-                    apply(b);
+                    apply(bx.cx, a);
+                    apply(bx.cx, b);
                 }
                 PassMode::Cast(_) => {
-                    apply(&ArgAttributes::new());
+                    apply(bx.cx, &ArgAttributes::new());
                 }
             }
         }

compiler/rustc_interface/src/tests.rs

@@ -566,7 +566,7 @@ fn test_debugging_options_tracking_hash() {
     tracked!(merge_functions, Some(MergeFunctions::Disabled));
     tracked!(mir_emit_retag, true);
     tracked!(mir_opt_level, Some(4));
-    tracked!(mutable_noalias, true);
+    tracked!(mutable_noalias, Some(true));
     tracked!(new_llvm_pass_manager, true);
     tracked!(no_codegen, true);
     tracked!(no_generate_arange_section, true);

compiler/rustc_middle/src/query/mod.rs

@@ -993,6 +993,10 @@ rustc_queries! {
     query is_freeze_raw(env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool {
         desc { "computing whether `{}` is freeze", env.value }
     }
+    /// Query backing `TyS::is_unpin`.
+    query is_unpin_raw(env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool {
+        desc { "computing whether `{}` is `Unpin`", env.value }
+    }
     /// Query backing `TyS::needs_drop`.
     query needs_drop_raw(env: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool {
         desc { "computing whether `{}` needs drop", env.value }

compiler/rustc_middle/src/ty/layout.rs

@@ -11,7 +11,7 @@ use rustc_hir as hir;
 use rustc_hir::lang_items::LangItem;
 use rustc_index::bit_set::BitSet;
 use rustc_index::vec::{Idx, IndexVec};
-use rustc_session::{DataTypeKind, FieldInfo, SizeKind, VariantInfo};
+use rustc_session::{config::OptLevel, DataTypeKind, FieldInfo, SizeKind, VariantInfo};
 use rustc_span::symbol::{Ident, Symbol};
 use rustc_span::DUMMY_SP;
 use rustc_target::abi::call::{
@@ -2318,31 +2318,30 @@ where
             ty::Ref(_, ty, mt) if offset.bytes() == 0 => {
                 let address_space = addr_space_of_ty(ty);
                 let tcx = cx.tcx();
-                let is_freeze = ty.is_freeze(tcx.at(DUMMY_SP), cx.param_env());
-                let kind = match mt {
-                    hir::Mutability::Not => {
-                        if is_freeze {
-                            PointerKind::Frozen
-                        } else {
-                            PointerKind::Shared
+                let kind = if tcx.sess.opts.optimize == OptLevel::No {
+                    // Use conservative pointer kind if not optimizing. This saves us the
+                    // Freeze/Unpin queries, and can save time in the codegen backend (noalias
+                    // attributes in LLVM have compile-time cost even in unoptimized builds).
+                    PointerKind::Shared
+                } else {
+                    match mt {
+                        hir::Mutability::Not => {
+                            if ty.is_freeze(tcx.at(DUMMY_SP), cx.param_env()) {
+                                PointerKind::Frozen
+                            } else {
+                                PointerKind::Shared
+                            }
                         }
-                    }
-                    hir::Mutability::Mut => {
-                        // Previously we would only emit noalias annotations for LLVM >= 6 or in
-                        // panic=abort mode. That was deemed right, as prior versions had many bugs
-                        // in conjunction with unwinding, but later versions didn’t seem to have
-                        // said issues. See issue #31681.
-                        //
-                        // Alas, later on we encountered a case where noalias would generate wrong
-                        // code altogether even with recent versions of LLVM in *safe* code with no
-                        // unwinding involved. See #54462.
-                        //
-                        // For now, do not enable mutable_noalias by default at all, while the
-                        // issue is being figured out.
-                        if tcx.sess.opts.debugging_opts.mutable_noalias {
-                            PointerKind::UniqueBorrowed
-                        } else {
-                            PointerKind::Shared
+                        hir::Mutability::Mut => {
+                            // References to self-referential structures should not be considered
+                            // noalias, as another pointer to the structure can be obtained, that
+                            // is not based-on the original reference. We consider all !Unpin
+                            // types to be potentially self-referential here.
+                            if ty.is_unpin(tcx.at(DUMMY_SP), cx.param_env()) {
+                                PointerKind::UniqueBorrowed
+                            } else {
+                                PointerKind::Shared
+                            }
                         }
                     }
                 };
@@ -2775,10 +2774,14 @@ where
                     // and can be marked as both `readonly` and `noalias`, as
                     // LLVM's definition of `noalias` is based solely on memory
                     // dependencies rather than pointer equality
+                    //
+                    // Due to miscompiles in LLVM < 12, we apply a separate NoAliasMutRef attribute
+                    // for UniqueBorrowed arguments, so that the codegen backend can decide
+                    // whether or not to actually emit the attribute.
                     let no_alias = match kind {
-                        PointerKind::Shared => false,
+                        PointerKind::Shared | PointerKind::UniqueBorrowed => false,
                         PointerKind::UniqueOwned => true,
-                        PointerKind::Frozen | PointerKind::UniqueBorrowed => !is_return,
+                        PointerKind::Frozen => !is_return,
                     };
                     if no_alias {
                         attrs.set(ArgAttribute::NoAlias);
@@ -2787,6 +2790,10 @@ where
                     if kind == PointerKind::Frozen && !is_return {
                         attrs.set(ArgAttribute::ReadOnly);
                     }
+
+                    if kind == PointerKind::UniqueBorrowed && !is_return {
+                        attrs.set(ArgAttribute::NoAliasMutRef);
+                    }
                 }
             }
         };

compiler/rustc_middle/src/ty/util.rs

@@ -741,6 +741,46 @@ impl<'tcx> ty::TyS<'tcx> {
         }
     }
 
+    /// Checks whether values of this type `T` implement the `Unpin` trait.
+    pub fn is_unpin(&'tcx self, tcx_at: TyCtxtAt<'tcx>, param_env: ty::ParamEnv<'tcx>) -> bool {
+        self.is_trivially_unpin() || tcx_at.is_unpin_raw(param_env.and(self))
+    }
+
+    /// Fast path helper for testing if a type is `Unpin`.
+    ///
+    /// Returning true means the type is known to be `Unpin`. Returning
+    /// `false` means nothing -- could be `Unpin`, might not be.
+    fn is_trivially_unpin(&self) -> bool {
+        match self.kind() {
+            ty::Int(_)
+            | ty::Uint(_)
+            | ty::Float(_)
+            | ty::Bool
+            | ty::Char
+            | ty::Str
+            | ty::Never
+            | ty::Ref(..)
+            | ty::RawPtr(_)
+            | ty::FnDef(..)
+            | ty::Error(_)
+            | ty::FnPtr(_) => true,
+            ty::Tuple(_) => self.tuple_fields().all(Self::is_trivially_unpin),
+            ty::Slice(elem_ty) | ty::Array(elem_ty, _) => elem_ty.is_trivially_unpin(),
+            ty::Adt(..)
+            | ty::Bound(..)
+            | ty::Closure(..)
+            | ty::Dynamic(..)
+            | ty::Foreign(_)
+            | ty::Generator(..)
+            | ty::GeneratorWitness(_)
+            | ty::Infer(_)
+            | ty::Opaque(..)
+            | ty::Param(_)
+            | ty::Placeholder(_)
+            | ty::Projection(_) => false,
+        }
+    }
+
     /// If `ty.needs_drop(...)` returns `true`, then `ty` is definitely
     /// non-copy and *might* have a destructor attached; if it returns
     /// `false`, then `ty` definitely has no destructor (i.e., no drop glue).

compiler/rustc_session/src/options.rs

@@ -997,8 +997,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
         (default: no)"),
     mir_opt_level: Option<usize> = (None, parse_opt_uint, [TRACKED],
         "MIR optimization level (0-4; default: 1 in non optimized builds and 2 in optimized builds)"),
-    mutable_noalias: bool = (false, parse_bool, [TRACKED],
-        "emit noalias metadata for mutable references (default: no)"),
+    mutable_noalias: Option<bool> = (None, parse_opt_bool, [TRACKED],
+        "emit noalias metadata for mutable references (default: yes for LLVM >= 12, otherwise no)"),
     new_llvm_pass_manager: bool = (false, parse_bool, [TRACKED],
         "use new LLVM pass manager (default: no)"),
     nll_facts: bool = (false, parse_bool, [UNTRACKED],

compiler/rustc_target/src/abi/call/mod.rs

@@ -65,7 +65,10 @@ mod attr_impl {
             const NoCapture = 1 << 2;
             const NonNull   = 1 << 3;
             const ReadOnly  = 1 << 4;
-            const InReg     = 1 << 8;
+            const InReg     = 1 << 5;
+            // NoAlias on &mut arguments can only be used with LLVM >= 12 due to miscompiles
+            // in earlier versions. FIXME: Remove this distinction once possible.
+            const NoAliasMutRef = 1 << 6;
         }
     }
 }

compiler/rustc_target/src/abi/mod.rs

@@ -1112,7 +1112,7 @@ pub enum PointerKind {
     /// `&T` where `T` contains no `UnsafeCell`, is `noalias` and `readonly`.
     Frozen,
 
-    /// `&mut T`, when we know `noalias` is safe for LLVM.
+    /// `&mut T` which is `noalias` but not `readonly`.
     UniqueBorrowed,
 
     /// `Box<T>`, unlike `UniqueBorrowed`, it also has `noalias` on returns.

compiler/rustc_ty_utils/src/common_traits.rs

@@ -18,6 +18,10 @@ fn is_freeze_raw<'tcx>(tcx: TyCtxt<'tcx>, query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>
     is_item_raw(tcx, query, LangItem::Freeze)
 }
 
+fn is_unpin_raw<'tcx>(tcx: TyCtxt<'tcx>, query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> bool {
+    is_item_raw(tcx, query, LangItem::Unpin)
+}
+
 fn is_item_raw<'tcx>(
     tcx: TyCtxt<'tcx>,
     query: ty::ParamEnvAnd<'tcx, Ty<'tcx>>,
@@ -37,5 +41,11 @@ fn is_item_raw<'tcx>(
 }
 
 pub(crate) fn provide(providers: &mut ty::query::Providers) {
-    *providers = ty::query::Providers { is_copy_raw, is_sized_raw, is_freeze_raw, ..*providers };
+    *providers = ty::query::Providers {
+        is_copy_raw,
+        is_sized_raw,
+        is_freeze_raw,
+        is_unpin_raw,
+        ..*providers
+    };
 }