Skip to content

Commit

Permalink
Auto merge of #82834 - nikic:mutable-noalias, r=nagisa
Browse files Browse the repository at this point in the history
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 #54878 and #63818).
  • Loading branch information
bors committed Mar 21, 2021
2 parents f826641 + 68a62b7 commit 97663b6
Show file tree
Hide file tree
Showing 13 changed files with 165 additions and 57 deletions.
65 changes: 47 additions & 18 deletions compiler/rustc_codegen_llvm/src/abi.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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 => {
Expand All @@ -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();
Expand All @@ -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 => {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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());
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_interface/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
61 changes: 34 additions & 27 deletions compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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
}
}
}
};
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
}
}
};
Expand Down
40 changes: 40 additions & 0 deletions compiler/rustc_middle/src/ty/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_target/src/abi/call/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_target/src/abi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
12 changes: 11 additions & 1 deletion compiler/rustc_ty_utils/src/common_traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>>,
Expand All @@ -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
};
}
2 changes: 1 addition & 1 deletion src/doc/reference
Loading

0 comments on commit 97663b6

Please sign in to comment.