From 9fbb2a9b347d44074fff8aab27cd161a9cd54c74 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 2 Oct 2019 21:43:24 +0200 Subject: [PATCH 1/6] Fix missing calls to drop on unwind with lto=fat; issue 64655. --- src/librustc_codegen_llvm/attributes.rs | 51 ++++++++++++++++++------- 1 file changed, 38 insertions(+), 13 deletions(-) diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index 33b50401b22f1..4189904a8018c 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -273,25 +273,50 @@ pub fn from_fn_attrs( } 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 { + } else if let Some(_) = id { + // rust-lang/rust#64655, rust-lang/rust#63909: to minimize + // risk associated with changing cases where nounwind + // attribute is attached, this code is deliberately mimicking + // old control flow based on whether `id` is `Some` or `None`. + // + // However, in the long term we should either: + // - fold this into final else (i.e. stop inspecting `id`) + // - or better still: whole-heartedly adopt Rust PR #63909. + // + // see also Rust RFC 2753. + 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 + if sig.abi == Abi::Rust || sig.abi == Abi::RustCall { + // Any Rust method (or `extern "Rust" fn` or `extern + // "rust-call" fn`) is explicitly allowed to unwind + // (unless it has no-unwind attribute, handled above). true + } else { + // Anything else is either: + // + // 1. A foreign item (like `extern "C" { fn foo(); }`), or + // + // 2. A Rust item using a non-Rust ABI (like `extern "C" fn foo() { ... }`). + // + // Foreign items (case 1) are assumed to not unwind; it is + // UB otherwise. (At least for now; see also + // rust-lang/rust#63909 and Rust RFC 2753.) + // + // Items defined in Rust with non-Rust ABIs (case 2) are + // defined to not unwind. We insert shims to abort if an + // unwind happens to enforce this. + // + // In either case, we mark item as explicitly nounwind. + false } } else { // assume this can possibly unwind, avoiding the application of a // `nounwind` attribute below. + // + // (But: See comments in previous branch. Specifically, it is + // unclear whether there is real value in the assumption this + // can unwind. The conservatism here may just be papering over + // a real problem by making some UB a bit harder to hit.) true }); From a3719728325e7a6fa511aee5c8191c248262a9af Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 2 Oct 2019 21:43:40 +0200 Subject: [PATCH 2/6] Regression tests for issue 64655. --- ...llow-unwind-when-calling-panic-directly.rs | 63 ++++++++++++++ ...sue-64655-extern-rust-must-allow-unwind.rs | 82 +++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 src/test/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs create mode 100644 src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs diff --git a/src/test/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs b/src/test/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs new file mode 100644 index 0000000000000..ac715e4328fd7 --- /dev/null +++ b/src/test/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs @@ -0,0 +1,63 @@ +// run-pass + +// rust-lang/rust#64655: with panic=unwind, a panic from a subroutine +// should still run desstructors as it unwindws the stack. However, +// bugs with how the nounwind LLVM attribute was applied led to this +// simple case being mishandled *if* you had fat LTO turned on. + +// Unlike issue-64655-extern-rust-must-allow-unwind.rs, the issue +// embodied in this test cropped up regardless of optimization level. +// Therefore it seemed worthy of being enshrined as a dedicated unit +// test. + +// LTO settings cannot be combined with -C prefer-dynamic +// no-prefer-dynamic + +// The revisions just enumerate lto settings (the opt-level appeared irrelevant in practice) + +// revisions: no thin fat +//[no]compile-flags: -C lto=no +//[thin]compile-flags: -C lto=thin +//[fat]compile-flags: -C lto=fat + +#![feature(core_panic)] + +// (For some reason, reproducing the LTO issue requires pulling in std +// explicitly this way.) +#![no_std] +extern crate std; + +fn main() { + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::boxed::Box; + + static SHARED: AtomicUsize = AtomicUsize::new(0); + + assert_eq!(SHARED.fetch_add(0, Ordering::SeqCst), 0); + + let old_hook = std::panic::take_hook(); + + std::panic::set_hook(Box::new(|_| { } )); // no-op on panic. + + let handle = std::thread::spawn(|| { + struct Droppable; + impl Drop for Droppable { + fn drop(&mut self) { + SHARED.fetch_add(1, Ordering::SeqCst); + } + } + + let _guard = Droppable; + let s = "issue-64655-allow-unwind-when-calling-panic-directly.rs"; + core::panicking::panic(&("???", s, 17, 4)); + }); + + let wait = handle.join(); + + // reinstate handler to ease observation of assertion failures. + std::panic::set_hook(old_hook); + + assert!(wait.is_err()); + + assert_eq!(SHARED.fetch_add(0, Ordering::SeqCst), 1); +} diff --git a/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs b/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs new file mode 100644 index 0000000000000..4f7e8e3072f6e --- /dev/null +++ b/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs @@ -0,0 +1,82 @@ +// run-pass + +// rust-lang/rust#64655: with panic=unwind, a panic from a subroutine +// should still run desstructors as it unwindws the stack. However, +// bugs with how the nounwind LLVM attribute was applied led to this +// simple case being mishandled *if* you had optimization *and* fat +// LTO turned on. + +// This test is the closest thing to a "regression test" we can do +// without actually spawning subprocesses and comparing stderr +// results. +// +// This test takes the code from the above issue and adapts it to +// better fit our test infrastructure: +// +// * Instead of relying on println! to observe whether the destructor +// is run, we instead run the code in a spawned thread and +// communicate the destructor's operation via a synchronous atomic +// in static memory. +// +// * To keep the output from confusing a casual user, we override the +// panic hook to be a no-op (rather than printing a message to +// stderr). +// +// (pnkfelix has confirmed by hand that these additions do not mask +// the underlying bug.) + +// LTO settings cannot be combined with -C prefer-dynamic +// no-prefer-dynamic + +// The revisions combine each lto setting with each optimization +// setting; pnkfelix observed three differing behaviors at opt-levels +// 0/1/2+3 for this test, so it seems prudent to be thorough. + +// revisions: no0 no1 no2 no3 thin0 thin1 thin2 thin3 fat0 fat1 fat2 fat3 + +//[no0]compile-flags: -C opt-level=0 -C lto=no +//[no1]compile-flags: -C opt-level=1 -C lto=no +//[no2]compile-flags: -C opt-level=2 -C lto=no +//[no3]compile-flags: -C opt-level=3 -C lto=no +//[thin0]compile-flags: -C opt-level=0 -C lto=thin +//[thin1]compile-flags: -C opt-level=1 -C lto=thin +//[thin2]compile-flags: -C opt-level=2 -C lto=thin +//[thin3]compile-flags: -C opt-level=3 -C lto=thin +//[fat0]compile-flags: -C opt-level=0 -C lto=fat +//[fat1]compile-flags: -C opt-level=1 -C lto=fat +//[fat2]compile-flags: -C opt-level=2 -C lto=fat +//[fat3]compile-flags: -C opt-level=3 -C lto=fat + +fn main() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + static SHARED: AtomicUsize = AtomicUsize::new(0); + + assert_eq!(SHARED.fetch_add(0, Ordering::SeqCst), 0); + + let old_hook = std::panic::take_hook(); + + std::panic::set_hook(Box::new(|_| { } )); // no-op on panic. + + let handle = std::thread::spawn(|| { + struct Droppable; + impl Drop for Droppable { + fn drop(&mut self) { + SHARED.fetch_add(1, Ordering::SeqCst); + } + } + + let _guard = Droppable; + None::<()>.expect("???"); + }); + + let wait = handle.join(); + + // reinstate handler to ease observation of assertion failures. + std::panic::set_hook(old_hook); + + assert!(wait.is_err()); + + assert_eq!(SHARED.fetch_add(0, Ordering::SeqCst), 1); +} + From 3adcc3ed222bdc6fc669281c42ef933aa3f6116b Mon Sep 17 00:00:00 2001 From: Felix S Klock II Date: Wed, 2 Oct 2019 22:04:30 +0200 Subject: [PATCH 3/6] fix typo --- src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs b/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs index 4f7e8e3072f6e..18e441625ab9d 100644 --- a/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs +++ b/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs @@ -1,7 +1,7 @@ // run-pass // rust-lang/rust#64655: with panic=unwind, a panic from a subroutine -// should still run desstructors as it unwindws the stack. However, +// should still run desstructors as it unwinds the stack. However, // bugs with how the nounwind LLVM attribute was applied led to this // simple case being mishandled *if* you had optimization *and* fat // LTO turned on. From e7e6dec06a050c5bc4e7a93e6700fad28570779a Mon Sep 17 00:00:00 2001 From: Felix S Klock II Date: Thu, 3 Oct 2019 10:46:01 +0200 Subject: [PATCH 4/6] Apply suggestions from code review Co-Authored-By: Mazdak Farrokhzad Co-Authored-By: Ralf Jung --- src/librustc_codegen_llvm/attributes.rs | 4 ++-- .../issue-64655-allow-unwind-when-calling-panic-directly.rs | 4 ++-- .../ui/extern/issue-64655-extern-rust-must-allow-unwind.rs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index 4189904a8018c..73d43289d19c5 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -281,7 +281,7 @@ pub fn from_fn_attrs( // // However, in the long term we should either: // - fold this into final else (i.e. stop inspecting `id`) - // - or better still: whole-heartedly adopt Rust PR #63909. + // - adopt Rust PR #63909. // // see also Rust RFC 2753. @@ -294,7 +294,7 @@ pub fn from_fn_attrs( } else { // Anything else is either: // - // 1. A foreign item (like `extern "C" { fn foo(); }`), or + // 1. A foreign item using a non-Rust ABI (like `extern "C" { fn foo(); }`), or // // 2. A Rust item using a non-Rust ABI (like `extern "C" fn foo() { ... }`). // diff --git a/src/test/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs b/src/test/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs index ac715e4328fd7..63e373e5b59bb 100644 --- a/src/test/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs +++ b/src/test/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs @@ -1,7 +1,7 @@ // run-pass // rust-lang/rust#64655: with panic=unwind, a panic from a subroutine -// should still run desstructors as it unwindws the stack. However, +// should still run destructors as it unwinds the stack. However, // bugs with how the nounwind LLVM attribute was applied led to this // simple case being mishandled *if* you had fat LTO turned on. @@ -54,7 +54,7 @@ fn main() { let wait = handle.join(); - // reinstate handler to ease observation of assertion failures. + // Reinstate handler to ease observation of assertion failures. std::panic::set_hook(old_hook); assert!(wait.is_err()); diff --git a/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs b/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs index 18e441625ab9d..346d176ad3bcc 100644 --- a/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs +++ b/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs @@ -1,7 +1,7 @@ // run-pass // rust-lang/rust#64655: with panic=unwind, a panic from a subroutine -// should still run desstructors as it unwinds the stack. However, +// should still run destructors as it unwinds the stack. However, // bugs with how the nounwind LLVM attribute was applied led to this // simple case being mishandled *if* you had optimization *and* fat // LTO turned on. @@ -13,7 +13,7 @@ // This test takes the code from the above issue and adapts it to // better fit our test infrastructure: // -// * Instead of relying on println! to observe whether the destructor +// * Instead of relying on `println!` to observe whether the destructor // is run, we instead run the code in a spawned thread and // communicate the destructor's operation via a synchronous atomic // in static memory. From 5e7e8cd41f5591285bf764e3a8fc87ed89db21ba Mon Sep 17 00:00:00 2001 From: Felix S Klock II Date: Thu, 3 Oct 2019 11:08:28 +0200 Subject: [PATCH 5/6] Update attributes.rs Some comment refinements inspired by review feedback. --- src/librustc_codegen_llvm/attributes.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index 73d43289d19c5..423a2df3523ed 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -281,7 +281,7 @@ pub fn from_fn_attrs( // // However, in the long term we should either: // - fold this into final else (i.e. stop inspecting `id`) - // - adopt Rust PR #63909. + // - or, adopt Rust PR #63909. // // see also Rust RFC 2753. @@ -302,9 +302,10 @@ pub fn from_fn_attrs( // UB otherwise. (At least for now; see also // rust-lang/rust#63909 and Rust RFC 2753.) // - // Items defined in Rust with non-Rust ABIs (case 2) are - // defined to not unwind. We insert shims to abort if an - // unwind happens to enforce this. + // Items defined in Rust with non-Rust ABIs (case 2) are also + // not supposed to unwind. Whether this should be enforced + // (versus stating it is UB) and *how* it would be enforced + // is currently under discussion; see rust-lang/rust#58794. // // In either case, we mark item as explicitly nounwind. false From 71e5f7893490d3fa4f9e55ebf5a13cffc2bb1607 Mon Sep 17 00:00:00 2001 From: Felix S Klock II Date: Thu, 3 Oct 2019 11:10:13 +0200 Subject: [PATCH 6/6] Update issue-64655-extern-rust-must-allow-unwind.rs placate tidy --- src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs b/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs index 346d176ad3bcc..4d521e4703960 100644 --- a/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs +++ b/src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs @@ -79,4 +79,3 @@ fn main() { assert_eq!(SHARED.fetch_add(0, Ordering::SeqCst), 1); } -