Skip to content

Commit 8894a68

Browse files
committedAug 28, 2019
Auto merge of #63909 - RalfJung:nounwind, r=<try>
fix nounwind attribute logic With #62603 landed, we no longer add abort-on-panic shims to `extern "C"` functions. However, that means we should also not emit `nounwind` for `extern fn` because we are not actually catching those panics. The comment justifying that `nounwind` in the source is just factually wrong. I also noticed that we annotate `extern` *declarations* (of any ABI) with `nounwind`, which seems wrong? I removed this. Code like mozjpeg (the code that prompted the removal of the abort-on-panic shim) throws a panic from Rust code that enters C code (meaning the `extern "C"` *definition* must not have `nounwind`, or else we got UB), but then the panic bubbles through the C code and re-enters Rust code (meaning the `extern "C"` *declaration* that gets called from the Rust side must not have `nounwind` either). This should be beta-backported if #62603 gets backported.
2 parents ac21131 + 47e4193 commit 8894a68

File tree

5 files changed

+44
-29
lines changed

5 files changed

+44
-29
lines changed
 

‎src/librustc_codegen_llvm/attributes.rs

+3-20
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,14 @@ use rustc::hir::{CodegenFnAttrFlags, CodegenFnAttrs};
66
use rustc::hir::def_id::{DefId, LOCAL_CRATE};
77
use rustc::session::Session;
88
use rustc::session::config::{Sanitizer, OptLevel};
9-
use rustc::ty::{self, TyCtxt, PolyFnSig};
9+
use rustc::ty::{TyCtxt, PolyFnSig};
1010
use rustc::ty::layout::HasTyCtxt;
1111
use rustc::ty::query::Providers;
1212
use rustc_data_structures::small_c_str::SmallCStr;
1313
use rustc_data_structures::fx::FxHashMap;
1414
use rustc_target::spec::PanicStrategy;
1515
use rustc_codegen_ssa::traits::*;
1616

17-
use crate::abi::Abi;
1817
use crate::attributes;
1918
use crate::llvm::{self, Attribute};
2019
use crate::llvm::AttributePlace::Function;
@@ -201,7 +200,7 @@ pub fn from_fn_attrs(
201200
cx: &CodegenCx<'ll, 'tcx>,
202201
llfn: &'ll Value,
203202
id: Option<DefId>,
204-
sig: PolyFnSig<'tcx>,
203+
_sig: PolyFnSig<'tcx>,
205204
) {
206205
let codegen_fn_attrs = id.map(|id| cx.tcx.codegen_fn_attrs(id))
207206
.unwrap_or_else(|| CodegenFnAttrs::new());
@@ -268,27 +267,11 @@ pub fn from_fn_attrs(
268267
// optimize based on this!
269268
false
270269
} else if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::UNWIND) {
271-
// If a specific #[unwind] attribute is present, use that
270+
// If a specific #[unwind] attribute is present, use that.
272271
true
273272
} else if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_ALLOCATOR_NOUNWIND) {
274273
// Special attribute for allocator functions, which can't unwind
275274
false
276-
} else if let Some(id) = id {
277-
let sig = cx.tcx.normalize_erasing_late_bound_regions(ty::ParamEnv::reveal_all(), &sig);
278-
if cx.tcx.is_foreign_item(id) {
279-
// Foreign items like `extern "C" { fn foo(); }` are assumed not to
280-
// unwind
281-
false
282-
} else if sig.abi != Abi::Rust && sig.abi != Abi::RustCall {
283-
// Any items defined in Rust that *don't* have the `extern` ABI are
284-
// defined to not unwind. We insert shims to abort if an unwind
285-
// happens to enforce this.
286-
false
287-
} else {
288-
// Anything else defined in Rust is assumed that it can possibly
289-
// unwind
290-
true
291-
}
292275
} else {
293276
// assume this can possibly unwind, avoiding the application of a
294277
// `nounwind` attribute below.

‎src/librustc_typeck/collect.rs

+1
Original file line numberDiff line numberDiff line change
@@ -2528,6 +2528,7 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs {
25282528
} else if attr.check_name(sym::rustc_allocator) {
25292529
codegen_fn_attrs.flags |= CodegenFnAttrFlags::ALLOCATOR;
25302530
} else if attr.check_name(sym::unwind) {
2531+
// FIXME: We currently assume it can unwind even with `#[unwind(aborts)]`.
25312532
codegen_fn_attrs.flags |= CodegenFnAttrFlags::UNWIND;
25322533
} else if attr.check_name(sym::ffi_returns_twice) {
25332534
if tcx.is_foreign_item(id) {

‎src/test/codegen/extern-functions.rs

+25-3
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,38 @@
44
#![feature(unwind_attributes)]
55

66
extern {
7-
// CHECK: Function Attrs: nounwind
8-
// CHECK-NEXT: declare void @extern_fn
7+
// CHECK-NOT: nounwind
8+
// CHECK: declare void @extern_fn
99
fn extern_fn();
10-
// CHECK-NOT: Function Attrs: nounwind
10+
// CHECK-NOT: nounwind
1111
// CHECK: declare void @unwinding_extern_fn
1212
#[unwind(allowed)]
1313
fn unwinding_extern_fn();
14+
// CHECK-NOT: nounwind
15+
// CHECK: declare void @aborting_extern_fn
16+
#[unwind(aborts)]
17+
fn aborting_extern_fn(); // FIXME: we don't have the attribute here
18+
}
19+
20+
extern "Rust" {
21+
// CHECK-NOT: nounwind
22+
// CHECK: declare void @rust_extern_fn
23+
fn rust_extern_fn();
24+
// CHECK-NOT: nounwind
25+
// CHECK: declare void @rust_unwinding_extern_fn
26+
#[unwind(allowed)]
27+
fn rust_unwinding_extern_fn();
28+
// CHECK-NOT: nounwind
29+
// CHECK: declare void @rust_aborting_extern_fn
30+
#[unwind(aborts)]
31+
fn rust_aborting_extern_fn(); // FIXME: we don't have the attribute here
1432
}
1533

1634
pub unsafe fn force_declare() {
1735
extern_fn();
1836
unwinding_extern_fn();
37+
aborting_extern_fn();
38+
rust_extern_fn();
39+
rust_unwinding_extern_fn();
40+
rust_aborting_extern_fn();
1941
}

‎src/test/codegen/nounwind-extern.rs

-6
This file was deleted.

‎src/test/codegen/unwind-extern.rs

+15
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// compile-flags: -C opt-level=0
2+
3+
#![crate_type = "lib"]
4+
#![feature(unwind_attributes)]
5+
6+
// make sure these all do *not* get the attribute
7+
// CHECK-NOT: nounwind
8+
9+
pub extern fn foo() {} // right now we don't abort-on-panic, so we also shouldn't have `nounwind`
10+
#[unwind(allowed)]
11+
pub extern fn foo_allowed() {}
12+
13+
pub extern "Rust" fn bar() {}
14+
#[unwind(allowed)]
15+
pub extern "Rust" fn bar_allowed() {}

0 commit comments

Comments
 (0)