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
rust-lang · Dec 12, 2018 · 1091eee · 1091eee
1 parent bd47d68
commit 1091eee
Show file tree

Hide file tree

Showing 10 changed files with 61 additions and 46 deletions.
diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs
@@ -15,14 +15,15 @@ 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;
 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<DefId>,
+    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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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
@@ -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
@@ -10,6 +10,7 @@
 
 // run-pass
 // ignore-wasm32-bare no libc to test ffi with
+// ignore-emscripten blows the JS stack
 
 #![feature(rustc_private)]