-
Notifications
You must be signed in to change notification settings - Fork 13.4k
fix nounwind attribute logic #63909
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.
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
This comment was marked as resolved.
Sorry, something went wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 So, I think this function should not be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is right. The check should say There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 This PR removes the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct, that is, the check should be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} |
This file was deleted.
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
|
Uh oh!
There was an error while loading. Please reload this page.