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 6cf9185dab98d5c6ef7a80a7a8818af2c87efce1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 25 Aug 2019 16:54:09 +0200 Subject: [PATCH 2/4] sync adding nounwind attribute with generating abort-on-panic shim --- src/librustc/hir/mod.rs | 3 --- src/librustc/ty/context.rs | 24 +++++++++++++++++++++- src/librustc_codegen_llvm/attributes.rs | 22 +++++++++----------- src/librustc_mir/build/mod.rs | 27 +------------------------ src/librustc_typeck/collect.rs | 2 -- 5 files changed, 33 insertions(+), 45 deletions(-) diff --git a/src/librustc/hir/mod.rs b/src/librustc/hir/mod.rs index 983048188527f..2066f14c6d796 100644 --- a/src/librustc/hir/mod.rs +++ b/src/librustc/hir/mod.rs @@ -2646,9 +2646,6 @@ bitflags! { /// `#[rustc_allocator]`: a hint to LLVM that the pointer returned from this /// function is never null. const ALLOCATOR = 1 << 1; - /// `#[unwind]`: an indicator that this function may unwind despite what - /// its ABI signature may otherwise imply. - const UNWIND = 1 << 2; /// `#[rust_allocator_nounwind]`, an indicator that an imported FFI /// function will never unwind. Probably obsolete by recent changes with /// #[unwind], but hasn't been removed/migrated yet diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index e72efdb057ab1..3c4f4cf98e2aa 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -65,7 +65,7 @@ use std::ops::{Deref, Bound}; use std::iter; use std::sync::mpsc; use std::sync::Arc; -use rustc_target::spec::abi; +use rustc_target::spec::{abi, PanicStrategy}; use rustc_macros::HashStable; use syntax::ast; use syntax::attr; @@ -1581,6 +1581,28 @@ impl<'tcx> TyCtxt<'tcx> { pub fn has_strict_asm_symbol_naming(&self) -> bool { self.gcx.sess.target.target.arch.contains("nvptx") } + + /// Determine if this function gets a shim to catch panics and abort. + pub fn abort_on_panic_shim(&self, fn_def_id: DefId, _abi: abi::Abi) -> bool { + // Validate `#[unwind]` syntax regardless of platform-specific panic strategy + let attrs = &self.get_attrs(fn_def_id); + let unwind_attr = attr::find_unwind_attr(Some(self.sess.diagnostic()), attrs); + + // We never unwind, so it's not relevant to stop an unwind + if self.sess.panic_strategy() != PanicStrategy::Unwind { return false; } + + // We cannot add landing pads, so don't add one + if self.sess.no_landing_pads() { return false; } + + // For the common case, check the attribute. + match unwind_attr { + Some(attr::UnwindAttr::Allowed) => false, + Some(attr::UnwindAttr::Aborts) => true, + None => + // Default: don't catch panics. + false, // FIXME(#58794), should be `abi != Abi::Rust && abi != Abi::RustCall` + } + } } impl<'tcx> TyCtxt<'tcx> { diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index 07bb890318456..860bb7122e99f 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -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; @@ -267,27 +266,24 @@ pub fn from_fn_attrs( // 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. - // 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 + // 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 + if cx.tcx.is_foreign_item(id) { + // Foreign items like `extern "C" { fn foo(); }` and `extern "Rust" { fn bar(); }` + // are assumed not to unwind. + false + } else if cx.tcx.abort_on_panic_shim(id, sig.abi) { + // Since we are adding a shim to abort on panic, this cannot unwind. false } else { - // Anything else defined in Rust is assumed that it can possibly - // unwind + // Anything else defined in Rust and can possibly unwind. true } } else { - // assume this can possibly unwind, avoiding the application of a - // `nounwind` attribute below. + // Assume this can possibly unwind. true }); diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs index 6a3bb8b8b86a2..7cb85c177df81 100644 --- a/src/librustc_mir/build/mod.rs +++ b/src/librustc_mir/build/mod.rs @@ -11,11 +11,9 @@ use rustc::middle::region; use rustc::mir::*; use rustc::ty::{self, Ty, TyCtxt}; use rustc::util::nodemap::HirIdMap; -use rustc_target::spec::PanicStrategy; use rustc_data_structures::indexed_vec::{IndexVec, Idx}; use std::u32; use rustc_target::spec::abi::Abi; -use syntax::attr::{self, UnwindAttr}; use syntax::symbol::kw; use syntax_pos::Span; @@ -485,29 +483,6 @@ macro_rules! unpack { }; } -fn should_abort_on_panic(tcx: TyCtxt<'_>, fn_def_id: DefId, abi: Abi) -> bool { - // Not callable from C, so we can safely unwind through these - if abi == Abi::Rust || abi == Abi::RustCall { return false; } - - // Validate `#[unwind]` syntax regardless of platform-specific panic strategy - let attrs = &tcx.get_attrs(fn_def_id); - let unwind_attr = attr::find_unwind_attr(Some(tcx.sess.diagnostic()), attrs); - - // We never unwind, so it's not relevant to stop an unwind - if tcx.sess.panic_strategy() != PanicStrategy::Unwind { return false; } - - // We cannot add landing pads, so don't add one - if tcx.sess.no_landing_pads() { return false; } - - // This is a special case: some functions have a C abi but are meant to - // unwind anyway. Don't stop them. - match unwind_attr { - None => false, // FIXME(#58794) - Some(UnwindAttr::Allowed) => false, - Some(UnwindAttr::Aborts) => true, - } -} - /////////////////////////////////////////////////////////////////////////// /// the main entry point for building MIR for a function @@ -599,7 +574,7 @@ where let source_info = builder.source_info(span); let call_site_s = (call_site_scope, source_info); unpack!(block = builder.in_scope(call_site_s, LintLevel::Inherited, |builder| { - if should_abort_on_panic(tcx, fn_def_id, abi) { + if tcx.abort_on_panic_shim(fn_def_id, abi) { builder.schedule_abort(); } diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index 312a598af02bf..541cfd1c5aabb 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -2527,8 +2527,6 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs { codegen_fn_attrs.flags |= CodegenFnAttrFlags::COLD; } else if attr.check_name(sym::rustc_allocator) { codegen_fn_attrs.flags |= CodegenFnAttrFlags::ALLOCATOR; - } else if attr.check_name(sym::unwind) { - codegen_fn_attrs.flags |= CodegenFnAttrFlags::UNWIND; } else if attr.check_name(sym::ffi_returns_twice) { if tcx.is_foreign_item(id) { codegen_fn_attrs.flags |= CodegenFnAttrFlags::FFI_RETURNS_TWICE; From de5c8d9b370a05266314c5f3da1b59a9ba03f1d5 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 25 Aug 2019 21:24:30 +0200 Subject: [PATCH 3/4] make UI test also test extern Rust --- .../{abort-on-c-abi.rs => abort-on-panic.rs} | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) rename src/test/ui/abi/{abort-on-c-abi.rs => abort-on-panic.rs} (62%) diff --git a/src/test/ui/abi/abort-on-c-abi.rs b/src/test/ui/abi/abort-on-panic.rs similarity index 62% rename from src/test/ui/abi/abort-on-c-abi.rs rename to src/test/ui/abi/abort-on-panic.rs index 2f08730ec6132..de9046fb0978d 100644 --- a/src/test/ui/abi/abort-on-c-abi.rs +++ b/src/test/ui/abi/abort-on-panic.rs @@ -19,6 +19,11 @@ extern "C" fn panic_in_ffi() { panic!("Test"); } +#[unwind(aborts)] +extern "Rust" fn panic_in_rust_abi() { + panic!("TestRust"); +} + fn test() { let _ = panic::catch_unwind(|| { panic_in_ffi(); }); // The process should have aborted by now. @@ -26,15 +31,31 @@ fn test() { let _ = io::stdout().flush(); } +fn testrust() { + let _ = panic::catch_unwind(|| { panic_in_rust_abi(); }); + // The process should have aborted by now. + io::stdout().write(b"This should never be printed.\n"); + let _ = io::stdout().flush(); +} + fn main() { let args: Vec = env::args().collect(); if args.len() > 1 && args[1] == "test" { return test(); } + if args.len() > 1 && args[1] == "testrust" { + return testrust(); + } let mut p = Command::new(&args[0]) .stdout(Stdio::piped()) .stdin(Stdio::piped()) .arg("test").spawn().unwrap(); assert!(!p.wait().unwrap().success()); + + let mut p = Command::new(&args[0]) + .stdout(Stdio::piped()) + .stdin(Stdio::piped()) + .arg("testrust").spawn().unwrap(); + assert!(!p.wait().unwrap().success()); } From 2214932a30091c54654422edbd39489c102285f4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 25 Aug 2019 21:02:23 +0200 Subject: [PATCH 4/4] fix and test attribute on extern declarations and functions --- src/librustc_codegen_llvm/attributes.rs | 6 +----- src/test/codegen/extern-functions.rs | 18 +++++++++--------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index 860bb7122e99f..c2e625fca8e46 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -271,11 +271,7 @@ pub fn from_fn_attrs( 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(); }` and `extern "Rust" { fn bar(); }` - // are assumed not to unwind. - false - } else if cx.tcx.abort_on_panic_shim(id, sig.abi) { + if cx.tcx.abort_on_panic_shim(id, sig.abi) { // Since we are adding a shim to abort on panic, this cannot unwind. false } else { diff --git a/src/test/codegen/extern-functions.rs b/src/test/codegen/extern-functions.rs index 80a04524d8f99..45a2c6bfc81e8 100644 --- a/src/test/codegen/extern-functions.rs +++ b/src/test/codegen/extern-functions.rs @@ -4,17 +4,17 @@ #![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)] fn unwinding_extern_fn(); -// CHECK-NOT: nounwind -// CHECK: declare void @aborting_extern_fn +// CHECK: Function Attrs: nounwind +// CHECK-NEXT: declare void @aborting_extern_fn #[unwind(aborts)] - fn aborting_extern_fn(); // FIXME: we don't have the attribute here + fn aborting_extern_fn(); } extern "Rust" { @@ -25,10 +25,10 @@ extern "Rust" { // CHECK: declare void @rust_unwinding_extern_fn #[unwind(allowed)] fn rust_unwinding_extern_fn(); -// CHECK-NOT: nounwind -// CHECK: declare void @rust_aborting_extern_fn +// CHECK: Function Attrs: nounwind +// CHECK-NEXT: declare void @rust_aborting_extern_fn #[unwind(aborts)] - fn rust_aborting_extern_fn(); // FIXME: we don't have the attribute here + fn rust_aborting_extern_fn(); } pub unsafe fn force_declare() {