From 2201adc1080fd5e9161cc8203d849f055650f040 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 26 Aug 2019 09:13:15 +0200 Subject: [PATCH 1/4] fix nounwind attribute logic --- src/librustc_codegen_llvm/attributes.rs | 12 ++++-------- src/test/codegen/extern-functions.rs | 26 +++++++++++++++++++++++-- src/test/codegen/nounwind-extern.rs | 3 +++ src/test/codegen/unwind-extern.rs | 15 ++++++++++++++ 4 files changed, 46 insertions(+), 10 deletions(-) create mode 100644 src/test/codegen/unwind-extern.rs diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index 33b50401b22f1..07bb890318456 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -268,22 +268,18 @@ pub fn from_fn_attrs( // optimize based on this! false } else if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::UNWIND) { - // If a specific #[unwind] attribute is present, use that + // If a specific #[unwind] attribute is present, use that. + // FIXME: We currently assume it can unwind even with `#[unwind(aborts)]`. true } else if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_ALLOCATOR_NOUNWIND) { // Special attribute for allocator functions, which can't unwind false } else if let Some(id) = id { let sig = cx.tcx.normalize_erasing_late_bound_regions(ty::ParamEnv::reveal_all(), &sig); - if cx.tcx.is_foreign_item(id) { - // Foreign items like `extern "C" { fn foo(); }` are assumed not to + if cx.tcx.is_foreign_item(id) && sig.abi != Abi::Rust && sig.abi != Abi::RustCall { + // Foreign non-Rust items like `extern "C" { fn foo(); }` are assumed not to // unwind false - } else if sig.abi != Abi::Rust && sig.abi != Abi::RustCall { - // Any items defined in Rust that *don't* have the `extern` ABI are - // defined to not unwind. We insert shims to abort if an unwind - // happens to enforce this. - false } else { // Anything else defined in Rust is assumed that it can possibly // unwind diff --git a/src/test/codegen/extern-functions.rs b/src/test/codegen/extern-functions.rs index a935d88652267..80a04524d8f99 100644 --- a/src/test/codegen/extern-functions.rs +++ b/src/test/codegen/extern-functions.rs @@ -6,14 +6,36 @@ extern { // CHECK: Function Attrs: nounwind // CHECK-NEXT: declare void @extern_fn - fn extern_fn(); -// CHECK-NOT: Function Attrs: nounwind + fn extern_fn(); // assumed not to unwind +// CHECK-NOT: nounwind // CHECK: declare void @unwinding_extern_fn #[unwind(allowed)] fn unwinding_extern_fn(); +// CHECK-NOT: nounwind +// CHECK: declare void @aborting_extern_fn + #[unwind(aborts)] + fn aborting_extern_fn(); // FIXME: we don't have the attribute here +} + +extern "Rust" { +// CHECK-NOT: nounwind +// CHECK: declare void @rust_extern_fn + fn rust_extern_fn(); +// CHECK-NOT: nounwind +// CHECK: declare void @rust_unwinding_extern_fn + #[unwind(allowed)] + fn rust_unwinding_extern_fn(); +// CHECK-NOT: nounwind +// CHECK: declare void @rust_aborting_extern_fn + #[unwind(aborts)] + fn rust_aborting_extern_fn(); // FIXME: we don't have the attribute here } pub unsafe fn force_declare() { extern_fn(); unwinding_extern_fn(); + aborting_extern_fn(); + rust_extern_fn(); + rust_unwinding_extern_fn(); + rust_aborting_extern_fn(); } diff --git a/src/test/codegen/nounwind-extern.rs b/src/test/codegen/nounwind-extern.rs index 54d6a8d2794ba..7f634a861eeb9 100644 --- a/src/test/codegen/nounwind-extern.rs +++ b/src/test/codegen/nounwind-extern.rs @@ -2,5 +2,8 @@ #![crate_type = "lib"] +// The `nounwind` attribute does not get added by rustc; it is present here because LLVM +// analyses determine that this function does not unwind. + // CHECK: Function Attrs: norecurse nounwind pub extern fn foo() {} diff --git a/src/test/codegen/unwind-extern.rs b/src/test/codegen/unwind-extern.rs new file mode 100644 index 0000000000000..3de3e7e0a584d --- /dev/null +++ b/src/test/codegen/unwind-extern.rs @@ -0,0 +1,15 @@ +// compile-flags: -C opt-level=0 + +#![crate_type = "lib"] +#![feature(unwind_attributes)] + +// make sure these all do *not* get the attribute +// CHECK-NOT: nounwind + +pub extern fn foo() {} // right now we don't abort-on-panic, so we also shouldn't have `nounwind` +#[unwind(allowed)] +pub extern fn foo_allowed() {} + +pub extern "Rust" fn bar() {} +#[unwind(allowed)] +pub extern "Rust" fn bar_allowed() {} From edcfc261f994399c4063c878c95cbdc78f7cc302 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 26 Aug 2019 18:07:55 +0200 Subject: [PATCH 2/4] remove a useless test --- src/test/codegen/nounwind-extern.rs | 9 --------- 1 file changed, 9 deletions(-) delete mode 100644 src/test/codegen/nounwind-extern.rs diff --git a/src/test/codegen/nounwind-extern.rs b/src/test/codegen/nounwind-extern.rs deleted file mode 100644 index 7f634a861eeb9..0000000000000 --- a/src/test/codegen/nounwind-extern.rs +++ /dev/null @@ -1,9 +0,0 @@ -// compile-flags: -O - -#![crate_type = "lib"] - -// The `nounwind` attribute does not get added by rustc; it is present here because LLVM -// analyses determine that this function does not unwind. - -// CHECK: Function Attrs: norecurse nounwind -pub extern fn foo() {} From dcf3a47c2bce4c28af46fa5a29566dd0e9edad7c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 27 Aug 2019 12:42:57 +0200 Subject: [PATCH 3/4] fix nounwind on extern "C" declarations --- src/librustc_codegen_llvm/attributes.rs | 16 ++-------------- src/test/codegen/extern-functions.rs | 6 +++--- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index 07bb890318456..42f0f37c8c852 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -6,7 +6,7 @@ use rustc::hir::{CodegenFnAttrFlags, CodegenFnAttrs}; use rustc::hir::def_id::{DefId, LOCAL_CRATE}; use rustc::session::Session; use rustc::session::config::{Sanitizer, OptLevel}; -use rustc::ty::{self, TyCtxt, PolyFnSig}; +use rustc::ty::{TyCtxt, PolyFnSig}; use rustc::ty::layout::HasTyCtxt; use rustc::ty::query::Providers; use rustc_data_structures::small_c_str::SmallCStr; @@ -14,7 +14,6 @@ use rustc_data_structures::fx::FxHashMap; use rustc_target::spec::PanicStrategy; use rustc_codegen_ssa::traits::*; -use crate::abi::Abi; use crate::attributes; use crate::llvm::{self, Attribute}; use crate::llvm::AttributePlace::Function; @@ -201,7 +200,7 @@ pub fn from_fn_attrs( cx: &CodegenCx<'ll, 'tcx>, llfn: &'ll Value, id: Option, - sig: PolyFnSig<'tcx>, + _sig: PolyFnSig<'tcx>, ) { let codegen_fn_attrs = id.map(|id| cx.tcx.codegen_fn_attrs(id)) .unwrap_or_else(|| CodegenFnAttrs::new()); @@ -274,17 +273,6 @@ pub fn from_fn_attrs( } else if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_ALLOCATOR_NOUNWIND) { // Special attribute for allocator functions, which can't unwind false - } else if let Some(id) = id { - let sig = cx.tcx.normalize_erasing_late_bound_regions(ty::ParamEnv::reveal_all(), &sig); - if cx.tcx.is_foreign_item(id) && sig.abi != Abi::Rust && sig.abi != Abi::RustCall { - // Foreign non-Rust items like `extern "C" { fn foo(); }` are assumed not to - // unwind - false - } else { - // Anything else defined in Rust is assumed that it can possibly - // unwind - true - } } else { // assume this can possibly unwind, avoiding the application of a // `nounwind` attribute below. diff --git a/src/test/codegen/extern-functions.rs b/src/test/codegen/extern-functions.rs index 80a04524d8f99..d8f2edec97cab 100644 --- a/src/test/codegen/extern-functions.rs +++ b/src/test/codegen/extern-functions.rs @@ -4,9 +4,9 @@ #![feature(unwind_attributes)] extern { -// CHECK: Function Attrs: nounwind -// CHECK-NEXT: declare void @extern_fn - fn extern_fn(); // assumed not to unwind +// CHECK-NOT: nounwind +// CHECK: declare void @extern_fn + fn extern_fn(); // CHECK-NOT: nounwind // CHECK: declare void @unwinding_extern_fn #[unwind(allowed)] From 47e4193d705aa1060d530f5ae2081e25e4ff237a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 27 Aug 2019 21:42:23 +0200 Subject: [PATCH 4/4] move a FIXME to a better place --- src/librustc_codegen_llvm/attributes.rs | 1 - src/librustc_typeck/collect.rs | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index 42f0f37c8c852..124d78a29f966 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -268,7 +268,6 @@ pub fn from_fn_attrs( false } else if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::UNWIND) { // If a specific #[unwind] attribute is present, use that. - // FIXME: We currently assume it can unwind even with `#[unwind(aborts)]`. true } else if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_ALLOCATOR_NOUNWIND) { // Special attribute for allocator functions, which can't unwind diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index 312a598af02bf..3f76752e127ae 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -2528,6 +2528,7 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs { } else if attr.check_name(sym::rustc_allocator) { codegen_fn_attrs.flags |= CodegenFnAttrFlags::ALLOCATOR; } else if attr.check_name(sym::unwind) { + // FIXME: We currently assume it can unwind even with `#[unwind(aborts)]`. codegen_fn_attrs.flags |= CodegenFnAttrFlags::UNWIND; } else if attr.check_name(sym::ffi_returns_twice) { if tcx.is_foreign_item(id) {