Auto merge of #65020 - pnkfelix:targetted-fix-for-always-marking-rust-abi-unwind-issue-64655, r=alexcrichton

Always mark rust and rust-call abi's as unwind

PR #63909 identified a bug that had been injected by PR #55982. As discussed on #64655 (comment) , we started marking extern items as nounwind, *even* extern items that said they were using "Rust" or "rust-call" ABI.

This is a more targeted variant of PR #63909 that fixes the above bug.

Fix #64655

----

I personally suspect we will want PR #63909 to land in the long-term

But:
 *  it is not certain that PR #63909 *will* land,
 * more importantly, PR #63909 almost certainly will not be backported to beta/stable.

The identified bug was more severe than I think anyone realized (apart from perhaps @gnzlbg, as noted [here](#63909 (comment))).

Thus, I was motivated to write this PR, which fixes *just* the issue with extern rust/rust-call functions, and deliberately avoids injecting further deviation from current behavior (you can see further notes on this in the comments of the code added here).

Author: bors

src/librustc_codegen_llvm/attributes.rs

@@ -275,25 +275,51 @@ 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, 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 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() { ... }`).
+            //
+            // 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 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
         }
     } 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
     });
 

src/test/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs

@@ -0,0 +1,65 @@
+// run-pass
+// ignore-wasm32-bare compiled with panic=abort by default
+// ignore-emscripten no threads support
+
+// rust-lang/rust#64655: with panic=unwind, a panic from a subroutine
+// 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.
+
+// 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);
+}

src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs

@@ -0,0 +1,83 @@
+// run-pass
+// ignore-wasm32-bare compiled with panic=abort by default
+// ignore-emscripten no threads support
+
+// rust-lang/rust#64655: with panic=unwind, a panic from a subroutine
+// 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.
+
+// 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);
+}