From 097e9e528f45eea89677cf9ac3634c7cee9988aa Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sat, 28 Mar 2020 21:47:50 -0400 Subject: [PATCH 1/2] Add `can_unwind` field to `FnAbi` This is a pure refactoring with no behavior changes. --- src/librustc_codegen_llvm/attributes.rs | 42 +---------------- src/librustc_middle/ty/layout.rs | 60 +++++++++++++++++++++++-- src/librustc_target/abi/call/mod.rs | 2 + 3 files changed, 60 insertions(+), 44 deletions(-) diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index e4d6d7d8af90e..ac2bbd4b2aea4 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -13,8 +13,6 @@ use rustc_middle::ty::query::Providers; use rustc_middle::ty::{self, Ty, TyCtxt}; use rustc_session::config::{OptLevel, Sanitizer}; use rustc_session::Session; -use rustc_target::abi::call::Conv; -use rustc_target::spec::PanicStrategy; use crate::abi::FnAbi; use crate::attributes; @@ -315,45 +313,7 @@ pub fn from_fn_attrs( } sanitize(cx, codegen_fn_attrs.flags, llfn); - unwind( - llfn, - if cx.tcx.sess.panic_strategy() != PanicStrategy::Unwind { - // In panic=abort mode we assume nothing can unwind anywhere, so - // optimize based on this! - false - } else if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::UNWIND) { - // If a specific #[unwind] attribute is present, use that. - true - } else if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_ALLOCATOR_NOUNWIND) { - // Special attribute for allocator functions, which can't unwind. - false - } else { - if fn_abi.conv == Conv::Rust { - // Any Rust method (or `extern "Rust" fn` or `extern - // "rust-call" fn`) is explicitly allowed to unwind - // (unless it has no-unwind attribute, handled above). - true - } else { - // Anything else is either: - // - // 1. A foreign item using a non-Rust ABI (like `extern "C" { fn foo(); }`), or - // - // 2. A Rust item using a non-Rust ABI (like `extern "C" fn foo() { ... }`). - // - // Foreign items (case 1) are assumed to not unwind; it is - // UB otherwise. (At least for now; see also - // rust-lang/rust#63909 and Rust RFC 2753.) - // - // Items defined in Rust with non-Rust ABIs (case 2) are also - // not supposed to unwind. Whether this should be enforced - // (versus stating it is UB) and *how* it would be enforced - // is currently under discussion; see rust-lang/rust#58794. - // - // In either case, we mark item as explicitly nounwind. - false - } - }, - ); + unwind(llfn, fn_abi.can_unwind); // Always annotate functions with the target-cpu they are compiled for. // Without this, ThinLTO won't inline Rust functions into Clang generated diff --git a/src/librustc_middle/ty/layout.rs b/src/librustc_middle/ty/layout.rs index 54fde52ba3a77..17c93922b0023 100644 --- a/src/librustc_middle/ty/layout.rs +++ b/src/librustc_middle/ty/layout.rs @@ -1,4 +1,5 @@ use crate::ich::StableHashingContext; +use crate::middle::codegen_fn_attrs::CodegenFnAttrFlags; use crate::mir::{GeneratorLayout, GeneratorSavedLocal}; use crate::ty::subst::Subst; use crate::ty::{self, subst::SubstsRef, ReprOptions, Ty, TyCtxt, TypeFoldable}; @@ -15,7 +16,7 @@ use rustc_target::abi::call::{ ArgAbi, ArgAttribute, ArgAttributes, Conv, FnAbi, PassMode, Reg, RegKind, }; pub use rustc_target::abi::*; -use rustc_target::spec::{abi::Abi as SpecAbi, HasTargetSpec}; +use rustc_target::spec::{abi::Abi as SpecAbi, HasTargetSpec, PanicStrategy}; use std::cmp; use std::fmt; @@ -2368,11 +2369,55 @@ where sig: ty::PolyFnSig<'tcx>, extra_args: &[Ty<'tcx>], caller_location: Option>, + codegen_fn_attr_flags: CodegenFnAttrFlags, mk_arg_type: impl Fn(Ty<'tcx>, Option) -> ArgAbi<'tcx, Ty<'tcx>>, ) -> Self; fn adjust_for_abi(&mut self, cx: &C, abi: SpecAbi); } +fn fn_can_unwind( + panic_strategy: PanicStrategy, + codegen_fn_attr_flags: CodegenFnAttrFlags, + call_conv: Conv, +) -> bool { + if panic_strategy != PanicStrategy::Unwind { + // In panic=abort mode we assume nothing can unwind anywhere, so + // optimize based on this! + false + } else if codegen_fn_attr_flags.contains(CodegenFnAttrFlags::UNWIND) { + // If a specific #[unwind] attribute is present, use that. + true + } else if codegen_fn_attr_flags.contains(CodegenFnAttrFlags::RUSTC_ALLOCATOR_NOUNWIND) { + // Special attribute for allocator functions, which can't unwind. + false + } else { + if call_conv == Conv::Rust { + // Any Rust method (or `extern "Rust" fn` or `extern + // "rust-call" fn`) is explicitly allowed to unwind + // (unless it has no-unwind attribute, handled above). + true + } else { + // Anything else is either: + // + // 1. A foreign item using a non-Rust ABI (like `extern "C" { fn foo(); }`), or + // + // 2. A Rust item using a non-Rust ABI (like `extern "C" fn foo() { ... }`). + // + // Foreign items (case 1) are assumed to not unwind; it is + // UB otherwise. (At least for now; see also + // rust-lang/rust#63909 and Rust RFC 2753.) + // + // Items defined in Rust with non-Rust ABIs (case 2) are also + // not supposed to unwind. Whether this should be enforced + // (versus stating it is UB) and *how* it would be enforced + // is currently under discussion; see rust-lang/rust#58794. + // + // In either case, we mark item as explicitly nounwind. + false + } + } +} + impl<'tcx, C> FnAbiExt<'tcx, C> for call::FnAbi<'tcx, Ty<'tcx>> where C: LayoutOf, TyAndLayout = TyAndLayout<'tcx>> @@ -2382,7 +2427,12 @@ where + HasParamEnv<'tcx>, { fn of_fn_ptr(cx: &C, sig: ty::PolyFnSig<'tcx>, extra_args: &[Ty<'tcx>]) -> Self { - call::FnAbi::new_internal(cx, sig, extra_args, None, |ty, _| ArgAbi::new(cx.layout_of(ty))) + // Assume that fn pointers may always unwind + let codegen_fn_attr_flags = CodegenFnAttrFlags::UNWIND; + + call::FnAbi::new_internal(cx, sig, extra_args, None, codegen_fn_attr_flags, |ty, _| { + ArgAbi::new(cx.layout_of(ty)) + }) } fn of_instance(cx: &C, instance: ty::Instance<'tcx>, extra_args: &[Ty<'tcx>]) -> Self { @@ -2394,7 +2444,9 @@ where None }; - call::FnAbi::new_internal(cx, sig, extra_args, caller_location, |ty, arg_idx| { + let attrs = cx.tcx().codegen_fn_attrs(instance.def_id()).flags; + + call::FnAbi::new_internal(cx, sig, extra_args, caller_location, attrs, |ty, arg_idx| { let mut layout = cx.layout_of(ty); // Don't pass the vtable, it's not an argument of the virtual fn. // Instead, pass just the data pointer, but give it the type `*const/mut dyn Trait` @@ -2450,6 +2502,7 @@ where sig: ty::PolyFnSig<'tcx>, extra_args: &[Ty<'tcx>], caller_location: Option>, + codegen_fn_attr_flags: CodegenFnAttrFlags, mk_arg_type: impl Fn(Ty<'tcx>, Option) -> ArgAbi<'tcx, Ty<'tcx>>, ) -> Self { debug!("FnAbi::new_internal({:?}, {:?})", sig, extra_args); @@ -2639,6 +2692,7 @@ where c_variadic: sig.c_variadic, fixed_count: inputs.len(), conv, + can_unwind: fn_can_unwind(cx.tcx().sess.panic_strategy(), codegen_fn_attr_flags, conv), }; fn_abi.adjust_for_abi(cx, sig.abi); fn_abi diff --git a/src/librustc_target/abi/call/mod.rs b/src/librustc_target/abi/call/mod.rs index ffc85c153c9cd..72768c31e3077 100644 --- a/src/librustc_target/abi/call/mod.rs +++ b/src/librustc_target/abi/call/mod.rs @@ -546,6 +546,8 @@ pub struct FnAbi<'a, Ty> { pub fixed_count: usize, pub conv: Conv, + + pub can_unwind: bool, } impl<'a, Ty> FnAbi<'a, Ty> { From 036626f249eff9c7fcf2d1392182ec59e5460b57 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Tue, 31 Mar 2020 08:27:09 -0400 Subject: [PATCH 2/2] Address review feedback --- src/librustc_codegen_llvm/abi.rs | 7 +++++++ src/librustc_codegen_llvm/attributes.rs | 18 ++---------------- src/librustc_codegen_llvm/callee.rs | 2 +- src/librustc_codegen_llvm/mono_item.rs | 2 +- 4 files changed, 11 insertions(+), 18 deletions(-) diff --git a/src/librustc_codegen_llvm/abi.rs b/src/librustc_codegen_llvm/abi.rs index 38dd879de7d59..064ca53bd1bd1 100644 --- a/src/librustc_codegen_llvm/abi.rs +++ b/src/librustc_codegen_llvm/abi.rs @@ -396,6 +396,11 @@ impl<'tcx> FnAbiLlvmExt<'tcx> for FnAbi<'tcx, Ty<'tcx>> { llvm::Attribute::NoReturn.apply_llfn(llvm::AttributePlace::Function, llfn); } + // FIXME(eddyb, wesleywiser): apply this to callsites as well? + if !self.can_unwind { + llvm::Attribute::NoUnwind.apply_llfn(llvm::AttributePlace::Function, llfn); + } + let mut i = 0; let mut apply = |attrs: &ArgAttributes, ty: Option<&Type>| { attrs.apply_llfn(llvm::AttributePlace::Argument(i), llfn, ty); @@ -431,6 +436,8 @@ impl<'tcx> FnAbiLlvmExt<'tcx> for FnAbi<'tcx, Ty<'tcx>> { } fn apply_attrs_callsite(&self, bx: &mut Builder<'a, 'll, 'tcx>, callsite: &'ll Value) { + // FIXME(wesleywiser, eddyb): We should apply `nounwind` and `noreturn` as appropriate to this callsite. + let mut i = 0; let mut apply = |attrs: &ArgAttributes, ty: Option<&Type>| { attrs.apply_callsite(llvm::AttributePlace::Argument(i), callsite, ty); diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index ac2bbd4b2aea4..784a3a87e9885 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -10,11 +10,10 @@ use rustc_hir::def_id::{DefId, LOCAL_CRATE}; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::ty::layout::HasTyCtxt; use rustc_middle::ty::query::Providers; -use rustc_middle::ty::{self, Ty, TyCtxt}; +use rustc_middle::ty::{self, TyCtxt}; use rustc_session::config::{OptLevel, Sanitizer}; use rustc_session::Session; -use crate::abi::FnAbi; use crate::attributes; use crate::llvm::AttributePlace::Function; use crate::llvm::{self, Attribute}; @@ -75,12 +74,6 @@ pub fn emit_uwtable(val: &'ll Value, emit: bool) { Attribute::UWTable.toggle_llfn(Function, val, emit); } -/// Tell LLVM whether the function can or cannot unwind. -#[inline] -fn unwind(val: &'ll Value, can_unwind: bool) { - Attribute::NoUnwind.toggle_llfn(Function, val, !can_unwind); -} - /// Tell LLVM if this function should be 'naked', i.e., skip the epilogue and prologue. #[inline] fn naked(val: &'ll Value, is_naked: bool) { @@ -244,12 +237,7 @@ pub(crate) fn default_optimisation_attrs(sess: &Session, llfn: &'ll Value) { /// Composite function which sets LLVM attributes for function depending on its AST (`#[attribute]`) /// attributes. -pub fn from_fn_attrs( - cx: &CodegenCx<'ll, 'tcx>, - llfn: &'ll Value, - instance: ty::Instance<'tcx>, - fn_abi: &FnAbi<'tcx, Ty<'tcx>>, -) { +pub fn from_fn_attrs(cx: &CodegenCx<'ll, 'tcx>, llfn: &'ll Value, instance: ty::Instance<'tcx>) { let codegen_fn_attrs = cx.tcx.codegen_fn_attrs(instance.def_id()); match codegen_fn_attrs.optimize { @@ -313,8 +301,6 @@ pub fn from_fn_attrs( } sanitize(cx, codegen_fn_attrs.flags, llfn); - unwind(llfn, fn_abi.can_unwind); - // Always annotate functions with the target-cpu they are compiled for. // Without this, ThinLTO won't inline Rust functions into Clang generated // functions (because Clang annotates functions this way too). diff --git a/src/librustc_codegen_llvm/callee.rs b/src/librustc_codegen_llvm/callee.rs index 0759823c07a85..a36314448b170 100644 --- a/src/librustc_codegen_llvm/callee.rs +++ b/src/librustc_codegen_llvm/callee.rs @@ -78,7 +78,7 @@ pub fn get_fn(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<'tcx>) -> &'ll Value let llfn = cx.declare_fn(&sym, &fn_abi); debug!("get_fn: not casting pointer!"); - attributes::from_fn_attrs(cx, llfn, instance, &fn_abi); + attributes::from_fn_attrs(cx, llfn, instance); let instance_def_id = instance.def_id(); diff --git a/src/librustc_codegen_llvm/mono_item.rs b/src/librustc_codegen_llvm/mono_item.rs index fe1537fbd021d..a7a9d0c8a0759 100644 --- a/src/librustc_codegen_llvm/mono_item.rs +++ b/src/librustc_codegen_llvm/mono_item.rs @@ -77,7 +77,7 @@ impl PreDefineMethods<'tcx> for CodegenCx<'ll, 'tcx> { debug!("predefine_fn: instance = {:?}", instance); - attributes::from_fn_attrs(self, lldecl, instance, &fn_abi); + attributes::from_fn_attrs(self, lldecl, instance); self.instances.borrow_mut().insert(instance, lldecl); }