From 1091eee65b91a9968f2a0b2f0c7d6e8e27d8d033 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 15 Nov 2018 06:17:58 -0800 Subject: [PATCH] rustc: Switch `extern` functions to abort by default on panic This was intended to land way back in 1.24, but it was backed out due to breakage which has long since been fixed. An unstable `#[unwind]` attribute can be used to tweak the behavior here, but this is currently simply switching rustc's internal default to abort-by-default if an `extern` function panics, making our codegen sound primarily (as currently you can produce UB with safe code) Closes #52652 --- src/librustc_codegen_llvm/attributes.rs | 57 ++++++++++++-------- src/librustc_codegen_llvm/callee.rs | 2 +- src/librustc_codegen_llvm/context.rs | 1 - src/librustc_codegen_llvm/declare.rs | 11 +--- src/librustc_codegen_llvm/intrinsic.rs | 2 +- src/librustc_codegen_llvm/mono_item.rs | 7 ++- src/librustc_mir/build/mod.rs | 7 +-- src/test/codegen/nounwind-extern.rs | 16 ++++++ src/test/run-pass/abort-on-c-abi.rs | 3 -- src/test/run-pass/extern/extern-call-deep.rs | 1 + 10 files changed, 61 insertions(+), 46 deletions(-) create mode 100644 src/test/codegen/nounwind-extern.rs diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index ffe5e123f9b8b..30edc4744ecd4 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -15,7 +15,7 @@ use rustc::hir::{CodegenFnAttrFlags, CodegenFnAttrs}; use rustc::hir::def_id::{DefId, LOCAL_CRATE}; use rustc::session::Session; use rustc::session::config::Sanitizer; -use rustc::ty::TyCtxt; +use rustc::ty::{self, TyCtxt, PolyFnSig}; use rustc::ty::layout::HasTyCtxt; use rustc::ty::query::Providers; use rustc_data_structures::sync::Lrc; @@ -23,6 +23,7 @@ use rustc_data_structures::fx::FxHashMap; use rustc_target::spec::PanicStrategy; use rustc_codegen_ssa::traits::*; +use abi::Abi; use attributes; use llvm::{self, Attribute}; use llvm::AttributePlace::Function; @@ -60,7 +61,7 @@ pub fn emit_uwtable(val: &'ll Value, emit: bool) { /// Tell LLVM whether the function can or cannot unwind. #[inline] -pub fn unwind(val: &'ll Value, can_unwind: bool) { +fn unwind(val: &'ll Value, can_unwind: bool) { Attribute::NoUnwind.toggle_llfn(Function, val, !can_unwind); } @@ -150,9 +151,10 @@ pub fn non_lazy_bind(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, '_>, + cx: &CodegenCx<'ll, 'tcx>, llfn: &'ll Value, id: Option, + sig: PolyFnSig<'tcx>, ) { let codegen_fn_attrs = id.map(|id| cx.tcx.codegen_fn_attrs(id)) .unwrap_or_else(|| CodegenFnAttrs::new()); @@ -194,28 +196,37 @@ pub fn from_fn_attrs( llvm::AttributePlace::ReturnValue, llfn); } - let can_unwind = if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::UNWIND) { - Some(true) + 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) { - Some(false) - - // Perhaps questionable, but we assume that anything defined - // *in Rust code* may unwind. Foreign items like `extern "C" { - // fn foo(); }` are assumed not to unwind **unless** they have - // a `#[unwind]` attribute. - } else if id.map(|id| !cx.tcx.is_foreign_item(id)).unwrap_or(false) { - Some(true) - } else { - None - }; - - match can_unwind { - Some(false) => attributes::unwind(llfn, false), - Some(true) if cx.tcx.sess.panic_strategy() == PanicStrategy::Unwind => { - attributes::unwind(llfn, true); + // 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 + // 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 + true } - Some(true) | None => {} - } + } else { + // assume this can possibly unwind, avoiding the application of a + // `nounwind` attribute below. + true + }); // 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_codegen_llvm/callee.rs b/src/librustc_codegen_llvm/callee.rs index f13eeb6692c35..87185a20c5091 100644 --- a/src/librustc_codegen_llvm/callee.rs +++ b/src/librustc_codegen_llvm/callee.rs @@ -94,7 +94,7 @@ pub fn get_fn( if instance.def.is_inline(tcx) { attributes::inline(cx, llfn, attributes::InlineAttr::Hint); } - attributes::from_fn_attrs(cx, llfn, Some(instance.def.def_id())); + attributes::from_fn_attrs(cx, llfn, Some(instance.def.def_id()), sig); let instance_def_id = instance.def_id(); diff --git a/src/librustc_codegen_llvm/context.rs b/src/librustc_codegen_llvm/context.rs index 6c90937de3f9b..0bd6146f5aaa0 100644 --- a/src/librustc_codegen_llvm/context.rs +++ b/src/librustc_codegen_llvm/context.rs @@ -409,7 +409,6 @@ impl MiscMethods<'tcx> for CodegenCx<'ll, 'tcx> { )); let llfn = self.declare_fn("rust_eh_unwind_resume", sig); - attributes::unwind(llfn, true); attributes::apply_target_cpu_attr(self, llfn); unwresume.set(Some(llfn)); llfn diff --git a/src/librustc_codegen_llvm/declare.rs b/src/librustc_codegen_llvm/declare.rs index c23aab409a9a8..2964f2e58470f 100644 --- a/src/librustc_codegen_llvm/declare.rs +++ b/src/librustc_codegen_llvm/declare.rs @@ -26,8 +26,7 @@ use rustc::ty::{self, PolyFnSig}; use rustc::ty::layout::LayoutOf; use rustc::session::config::Sanitizer; use rustc_data_structures::small_c_str::SmallCStr; -use rustc_target::spec::PanicStrategy; -use abi::{Abi, FnType, FnTypeExt}; +use abi::{FnType, FnTypeExt}; use attributes; use context::CodegenCx; use type_::Type; @@ -86,10 +85,6 @@ fn declare_raw_fn( _ => {}, } - if cx.tcx.sess.panic_strategy() != PanicStrategy::Unwind { - attributes::unwind(llfn, false); - } - attributes::non_lazy_bind(cx.sess(), llfn); llfn @@ -132,10 +127,6 @@ impl DeclareMethods<'tcx> for CodegenCx<'ll, 'tcx> { llvm::Attribute::NoReturn.apply_llfn(Function, llfn); } - if sig.abi != Abi::Rust && sig.abi != Abi::RustCall { - attributes::unwind(llfn, false); - } - fty.apply_attrs_llfn(llfn); llfn diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index 59608b1193976..2b82ebe0bc25c 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -1081,7 +1081,7 @@ fn gen_fn<'ll, 'tcx>( Abi::Rust )); let llfn = cx.define_internal_fn(name, rust_fn_sig); - attributes::from_fn_attrs(cx, llfn, None); + attributes::from_fn_attrs(cx, llfn, None, rust_fn_sig); let bx = Builder::new_block(cx, llfn, "entry-block"); codegen(bx); llfn diff --git a/src/librustc_codegen_llvm/mono_item.rs b/src/librustc_codegen_llvm/mono_item.rs index 1d5bcc4ba3986..9c69d7d8cf4ff 100644 --- a/src/librustc_codegen_llvm/mono_item.rs +++ b/src/librustc_codegen_llvm/mono_item.rs @@ -82,7 +82,12 @@ impl PreDefineMethods<'tcx> for CodegenCx<'ll, 'tcx> { if instance.def.is_inline(self.tcx) { attributes::inline(self, lldecl, attributes::InlineAttr::Hint); } - attributes::from_fn_attrs(self, lldecl, Some(instance.def.def_id())); + attributes::from_fn_attrs( + self, + lldecl, + Some(instance.def.def_id()), + mono_sig, + ); self.instances.borrow_mut().insert(instance, lldecl); } diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs index b0601b3bbefb1..ab52d9be6b81b 100644 --- a/src/librustc_mir/build/mod.rs +++ b/src/librustc_mir/build/mod.rs @@ -628,12 +628,7 @@ fn should_abort_on_panic<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>, // unwind anyway. Don't stop them. let attrs = &tcx.get_attrs(fn_def_id); match attr::find_unwind_attr(Some(tcx.sess.diagnostic()), attrs) { - None => { - // FIXME(rust-lang/rust#48251) -- Had to disable - // abort-on-panic for backwards compatibility reasons. - false - } - + None => true, Some(UnwindAttr::Allowed) => false, Some(UnwindAttr::Aborts) => true, } diff --git a/src/test/codegen/nounwind-extern.rs b/src/test/codegen/nounwind-extern.rs new file mode 100644 index 0000000000000..ed07cf1d6b20d --- /dev/null +++ b/src/test/codegen/nounwind-extern.rs @@ -0,0 +1,16 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// compile-flags: -O + +#![crate_type = "lib"] + +// CHECK: Function Attrs: norecurse nounwind +pub extern fn foo() {} diff --git a/src/test/run-pass/abort-on-c-abi.rs b/src/test/run-pass/abort-on-c-abi.rs index 17b2ee39c884b..12b5b786766d6 100644 --- a/src/test/run-pass/abort-on-c-abi.rs +++ b/src/test/run-pass/abort-on-c-abi.rs @@ -15,14 +15,11 @@ // ignore-cloudabi no env and process // ignore-emscripten no processes -#![feature(unwind_attributes)] - use std::{env, panic}; use std::io::prelude::*; use std::io; use std::process::{Command, Stdio}; -#[unwind(aborts)] extern "C" fn panic_in_ffi() { panic!("Test"); } diff --git a/src/test/run-pass/extern/extern-call-deep.rs b/src/test/run-pass/extern/extern-call-deep.rs index f6eff601c4b7a..12df0f32134a2 100644 --- a/src/test/run-pass/extern/extern-call-deep.rs +++ b/src/test/run-pass/extern/extern-call-deep.rs @@ -10,6 +10,7 @@ // run-pass // ignore-wasm32-bare no libc to test ffi with +// ignore-emscripten blows the JS stack #![feature(rustc_private)]