Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix nounwind attribute logic #63909

Closed
wants to merge 4 commits into from
Closed

fix nounwind attribute logic #63909

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2201adc
fix nounwind attribute logic
RalfJung Aug 26, 2019
edcfc26
remove a useless test
RalfJung Aug 26, 2019
dcf3a47
fix nounwind on extern "C" declarations
RalfJung Aug 27, 2019
47e4193
move a FIXME to a better place
RalfJung Aug 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions src/librustc_codegen_llvm/attributes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]`.
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
26 changes: 24 additions & 2 deletions src/test/codegen/extern-functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the test seems dubious to me. Wouldn't code that bubbles Rust panics through C (the kind of code for which #62603 got merged) also have an extern "C" declaration like this where the panic enters back into Rust?

This comment was marked as resolved.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a stable vs nightly difference in semantics. That would be a bad mess.

We document panicking from extern "C" as UB, but (similar to #62825) are willing to patch things such that code exploiting this UB is not broken until we provide adequate alternatives to said code.

So, I think this function should not be unwind. I am waiting for @alexcrichton to confirm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I think this function should not be unwind

This is right. The check should say CHECK-NOT: nounwind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is correct, all imported FFI functions, by default, should be tagged with nounwind

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't this mean that when e.g. mozjpeg calls C that calls Rust (the situation due to which #62603 was accepted), and a panic passes from C to Rust, we still violate nounwind?

This PR removes the nounwind on the first edge the panic crosses (Rust -> C), but there's another edge (C -> Rust) and we should not have nounwind there either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR removes the nounwind on the first edge the panic crosses (Rust -> C), but there's another edge (C -> Rust) and we should not have nounwind there either.

Yes, that's correct, that is, the check should be CHECK-NOT: nounwind. Otherwise, the second example in #63943, which is equivalent to mozjpeg use case, would still be miscompiled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have implemented that.

// 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();
}
6 changes: 0 additions & 6 deletions src/test/codegen/nounwind-extern.rs
View file
Open in desktop

This file was deleted.

15 changes: 15 additions & 0 deletions src/test/codegen/unwind-extern.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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`
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
#[unwind(allowed)]
pub extern fn foo_allowed() {}
RalfJung marked this conversation as resolved.
Show resolved Hide resolved

pub extern "Rust" fn bar() {}
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
#[unwind(allowed)]
pub extern "Rust" fn bar_allowed() {}
RalfJung marked this conversation as resolved.
Show resolved Hide resolved