Auto merge of rust-lang#86155 - alexcrichton:abort-on-unwind, r=nikomatsakis

rustc: Fill out remaining parts of C-unwind ABI

This commit intends to fill out some of the remaining pieces of the
C-unwind ABI. This has a number of other changes with it though to move
this design space forward a bit. Notably contained within here is:

* On `panic=unwind`, the `extern "C"` ABI is now considered as "may
  unwind". This fixes a longstanding soundness issue where if you
  `panic!()` in an `extern "C"` function defined in Rust that's actually
  UB because the LLVM representation for the function has the `nounwind`
  attribute, but then you unwind.

* Whether or not a function unwinds now mainly considers the ABI of the
  function instead of first checking the panic strategy. This fixes a
  miscompile of `extern "C-unwind"` with `panic=abort` because that ABI
  can still unwind.

* The aborting stub for non-unwinding ABIs with `panic=unwind` has been
  reimplemented. Previously this was done as a small tweak during MIR
  generation, but this has been moved to a separate and dedicated MIR
  pass. This new pass will, for appropriate functions and function
  calls, insert a `cleanup` landing pad for any function call that may
  unwind within a function that is itself not allowed to unwind. Note
  that this subtly changes some behavior from before where previously on
  an unwind which was caught-to-abort it would run active destructors in
  the function, and now it simply immediately aborts the process.

* The `#[unwind]` attribute has been removed and all users in tests and
  such are now using `C-unwind` and `#![feature(c_unwind)]`.

I think this is largely the last piece of the RFC to implement.
Unfortunately I believe this is still not stabilizable as-is because
activating the feature gate changes the behavior of the existing `extern
"C"` ABI in a way that has no replacement. My thinking for how to enable
this is that we add support for the `C-unwind` ABI on stable Rust first,
and then after it hits stable we change the behavior of the `C` ABI.
That way anyone straddling stable/beta/nightly can switch to `C-unwind`
safely.

Author: bors

compiler/rustc_attr/src/builtin.rs

@@ -87,50 +87,6 @@ pub enum OptimizeAttr {
     Size,
 }
 
-#[derive(Copy, Clone, PartialEq)]
-pub enum UnwindAttr {
-    Allowed,
-    Aborts,
-}
-
-/// Determine what `#[unwind]` attribute is present in `attrs`, if any.
-pub fn find_unwind_attr(sess: &Session, attrs: &[Attribute]) -> Option<UnwindAttr> {
-    attrs.iter().fold(None, |ia, attr| {
-        if sess.check_name(attr, sym::unwind) {
-            if let Some(meta) = attr.meta() {
-                if let MetaItemKind::List(items) = meta.kind {
-                    if items.len() == 1 {
-                        if items[0].has_name(sym::allowed) {
-                            return Some(UnwindAttr::Allowed);
-                        } else if items[0].has_name(sym::aborts) {
-                            return Some(UnwindAttr::Aborts);
-                        }
-                    }
-
-                    struct_span_err!(
-                        sess.diagnostic(),
-                        attr.span,
-                        E0633,
-                        "malformed `unwind` attribute input"
-                    )
-                    .span_label(attr.span, "invalid argument")
-                    .span_suggestions(
-                        attr.span,
-                        "the allowed arguments are `allowed` and `aborts`",
-                        (vec!["allowed", "aborts"])
-                            .into_iter()
-                            .map(|s| format!("#[unwind({})]", s)),
-                        Applicability::MachineApplicable,
-                    )
-                    .emit();
-                }
-            }
-        }
-
-        ia
-    })
-}
-
 /// Represents the following attributes:
 ///
 /// - `#[stable]`

compiler/rustc_error_codes/src/error_codes/E0633.md

@@ -1,8 +1,10 @@
+#### Note: this error code is no longer emitted by the compiler.
+
 The `unwind` attribute was malformed.
 
 Erroneous code example:
 
-```compile_fail,E0633
+```compile_fail
 #![feature(unwind_attributes)]
 
 #[unwind()] // error: expected one argument

compiler/rustc_feature/src/active.rs

@@ -311,11 +311,6 @@ declare_features! (
     /// Allows `extern "platform-intrinsic" { ... }`.
     (active, platform_intrinsics, "1.4.0", Some(27731), None),
 
-    /// Allows `#[unwind(..)]`.
-    ///
-    /// Permits specifying whether a function should permit unwinding or abort on unwind.
-    (active, unwind_attributes, "1.4.0", Some(58760), None),
-
     /// Allows attributes on expressions and non-item statements.
     (active, stmt_expr_attributes, "1.6.0", Some(15701), None),
 

compiler/rustc_feature/src/builtin_attrs.rs

@@ -419,10 +419,6 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
     ),
     gated!(panic_runtime, AssumedUsed, template!(Word), experimental!(panic_runtime)),
     gated!(needs_panic_runtime, AssumedUsed, template!(Word), experimental!(needs_panic_runtime)),
-    gated!(
-        unwind, AssumedUsed, template!(List: "allowed|aborts"), unwind_attributes,
-        experimental!(unwind),
-    ),
     gated!(
         compiler_builtins, AssumedUsed, template!(Word),
         "the `#[compiler_builtins]` attribute is used to identify the `compiler_builtins` crate \

compiler/rustc_feature/src/removed.rs

@@ -156,6 +156,11 @@ declare_features! (
     (removed, min_type_alias_impl_trait, "1.56.0", Some(63063), None,
      Some("removed in favor of full type_alias_impl_trait")),
 
+    /// Allows `#[unwind(..)]`.
+    ///
+    /// Permits specifying whether a function should permit unwinding or abort on unwind.
+    (removed, unwind_attributes, "1.56.0", Some(58760), None, Some("use the C-unwind ABI instead")),
+
     // -------------------------------------------------------------------------
     // feature-group-end: removed features
     // -------------------------------------------------------------------------

compiler/rustc_middle/src/middle/codegen_fn_attrs.rs

@@ -52,13 +52,9 @@ bitflags! {
         /// `#[rustc_allocator]`: a hint to LLVM that the pointer returned from this
         /// function is never null.
         const ALLOCATOR                 = 1 << 1;
-        /// `#[unwind]`: an indicator that this function may unwind despite what
-        /// its ABI signature may otherwise imply.
-        const UNWIND                    = 1 << 2;
-        /// `#[rust_allocator_nounwind]`, an indicator that an imported FFI
-        /// function will never unwind. Probably obsolete by recent changes with
-        /// #[unwind], but hasn't been removed/migrated yet
-        const RUSTC_ALLOCATOR_NOUNWIND  = 1 << 3;
+        /// An indicator that function will never unwind. Will become obsolete
+        /// once C-unwind is fully stabilized.
+        const NEVER_UNWIND              = 1 << 3;
         /// `#[naked]`: an indicator to LLVM that no function prologue/epilogue
         /// should be generated.
         const NAKED                     = 1 << 4;

compiler/rustc_middle/src/mir/terminator.rs

@@ -237,7 +237,7 @@ pub enum TerminatorKind<'tcx> {
         /// consider it in borrowck. We don't want to accept programs which
         /// pass borrowck only when `panic=abort` or some assertions are disabled
         /// due to release vs. debug mode builds. This needs to be an `Option` because
-        /// of the `remove_noop_landing_pads` and `no_landing_pads` passes.
+        /// of the `remove_noop_landing_pads` and `abort_unwinding_calls` passes.
         unwind: Option<BasicBlock>,
     },
 

compiler/rustc_middle/src/ty/layout.rs

@@ -2601,65 +2601,124 @@ where
     fn adjust_for_abi(&mut self, cx: &C, abi: SpecAbi);
 }
 
+/// Calculates whether a function's ABI can unwind or not.
+///
+/// This takes two primary parameters:
+///
+/// * `codegen_fn_attr_flags` - these are flags calculated as part of the
+///   codegen attrs for a defined function. For function pointers this set of
+///   flags is the empty set. This is only applicable for Rust-defined
+///   functions, and generally isn't needed except for small optimizations where
+///   we try to say a function which otherwise might look like it could unwind
+///   doesn't actually unwind (such as for intrinsics and such).
+///
+/// * `abi` - this is the ABI that the function is defined with. This is the
+///   primary factor for determining whether a function can unwind or not.
+///
+/// Note that in this case unwinding is not necessarily panicking in Rust. Rust
+/// panics are implemented with unwinds on most platform (when
+/// `-Cpanic=unwind`), but this also accounts for `-Cpanic=abort` build modes.
+/// Notably unwinding is disallowed for more non-Rust ABIs unless it's
+/// specifically in the name (e.g. `"C-unwind"`). Unwinding within each ABI is
+/// defined for each ABI individually, but it always corresponds to some form of
+/// stack-based unwinding (the exact mechanism of which varies
+/// platform-by-platform).
+///
+/// Rust functions are classfied whether or not they can unwind based on the
+/// active "panic strategy". In other words Rust functions are considered to
+/// unwind in `-Cpanic=unwind` mode and cannot unwind in `-Cpanic=abort` mode.
+/// Note that Rust supports intermingling panic=abort and panic=unwind code, but
+/// only if the final panic mode is panic=abort. In this scenario any code
+/// previously compiled assuming that a function can unwind is still correct, it
+/// just never happens to actually unwind at runtime.
+///
+/// This function's answer to whether or not a function can unwind is quite
+/// impactful throughout the compiler. This affects things like:
+///
+/// * Calling a function which can't unwind means codegen simply ignores any
+///   associated unwinding cleanup.
+/// * Calling a function which can unwind from a function which can't unwind
+///   causes the `abort_unwinding_calls` MIR pass to insert a landing pad that
+///   aborts the process.
+/// * This affects whether functions have the LLVM `nounwind` attribute, which
+///   affects various optimizations and codegen.
+///
+/// FIXME: this is actually buggy with respect to Rust functions. Rust functions
+/// compiled with `-Cpanic=unwind` and referenced from another crate compiled
+/// with `-Cpanic=abort` will look like they can't unwind when in fact they
+/// might (from a foreign exception or similar).
 pub fn fn_can_unwind(
-    panic_strategy: PanicStrategy,
+    tcx: TyCtxt<'tcx>,
     codegen_fn_attr_flags: CodegenFnAttrFlags,
-    call_conv: Conv,
     abi: SpecAbi,
 ) -> bool {
-    if panic_strategy != PanicStrategy::Unwind {
-        // In panic=abort mode we assume nothing can unwind anywhere, so
-        // optimize based on this!
-        false
-    } else if codegen_fn_attr_flags.contains(CodegenFnAttrFlags::UNWIND) {
-        // If a specific #[unwind] attribute is present, use that.
-        true
-    } else if codegen_fn_attr_flags.contains(CodegenFnAttrFlags::RUSTC_ALLOCATOR_NOUNWIND) {
-        // Special attribute for allocator functions, which can't unwind.
-        false
-    } else {
-        if call_conv == Conv::Rust {
-            // 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() { ... }`).
-            //
-            // In both of these cases, we should refer to the ABI to determine whether or not we
-            // should unwind. See Rust RFC 2945 for more information on this behavior, here:
-            // https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md
-            use SpecAbi::*;
-            match abi {
-                C { unwind } | Stdcall { unwind } | System { unwind } | Thiscall { unwind } => {
-                    unwind
-                }
-                Cdecl
-                | Fastcall
-                | Vectorcall
-                | Aapcs
-                | Win64
-                | SysV64
-                | PtxKernel
-                | Msp430Interrupt
-                | X86Interrupt
-                | AmdGpuKernel
-                | EfiApi
-                | AvrInterrupt
-                | AvrNonBlockingInterrupt
-                | CCmseNonSecureCall
-                | Wasm
-                | RustIntrinsic
-                | PlatformIntrinsic
-                | Unadjusted => false,
-                // In the `if` above, we checked for functions with the Rust calling convention.
-                Rust | RustCall => unreachable!(),
-            }
+    // Special attribute for functions which can't unwind.
+    if codegen_fn_attr_flags.contains(CodegenFnAttrFlags::NEVER_UNWIND) {
+        return false;
+    }
+
+    // Otherwise if this isn't special then unwinding is generally determined by
+    // the ABI of the itself. ABIs like `C` have variants which also
+    // specifically allow unwinding (`C-unwind`), but not all platform-specific
+    // ABIs have such an option. Otherwise the only other thing here is Rust
+    // itself, and those ABIs are determined by the panic strategy configured
+    // for this compilation.
+    //
+    // Unfortunately at this time there's also another caveat. Rust [RFC
+    // 2945][rfc] has been accepted and is in the process of being implemented
+    // and stabilized. In this interim state we need to deal with historical
+    // rustc behavior as well as plan for future rustc behavior.
+    //
+    // Historically functions declared with `extern "C"` were marked at the
+    // codegen layer as `nounwind`. This happened regardless of `panic=unwind`
+    // or not. This is UB for functions in `panic=unwind` mode that then
+    // actually panic and unwind. Note that this behavior is true for both
+    // externally declared functions as well as Rust-defined function.
+    //
+    // To fix this UB rustc would like to change in the future to catch unwinds
+    // from function calls that may unwind within a Rust-defined `extern "C"`
+    // function and forcibly abort the process, thereby respecting the
+    // `nounwind` attribut emitted for `extern "C"`. This behavior change isn't
+    // ready to roll out, so determining whether or not the `C` family of ABIs
+    // unwinds is conditional not only on their definition but also whether the
+    // `#![feature(c_unwind)]` feature gate is active.
+    //
+    // Note that this means that unlike historical compilers rustc now, by
+    // default, unconditionally thinks that the `C` ABI may unwind. This will
+    // prevent some optimization opportunities, however, so we try to scope this
+    // change and only assume that `C` unwinds with `panic=unwind` (as opposed
+    // to `panic=abort`).
+    //
+    // Eventually the check against `c_unwind` here will ideally get removed and
+    // this'll be a little cleaner as it'll be a straightforward check of the
+    // ABI.
+    //
+    // [rfc]: https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md
+    use SpecAbi::*;
+    match abi {
+        C { unwind } | Stdcall { unwind } | System { unwind } | Thiscall { unwind } => {
+            unwind
+                || (!tcx.features().c_unwind && tcx.sess.panic_strategy() == PanicStrategy::Unwind)
         }
+        Cdecl
+        | Fastcall
+        | Vectorcall
+        | Aapcs
+        | Win64
+        | SysV64
+        | PtxKernel
+        | Msp430Interrupt
+        | X86Interrupt
+        | AmdGpuKernel
+        | EfiApi
+        | AvrInterrupt
+        | AvrNonBlockingInterrupt
+        | CCmseNonSecureCall
+        | Wasm
+        | RustIntrinsic
+        | PlatformIntrinsic
+        | Unadjusted => false,
+        Rust | RustCall => tcx.sess.panic_strategy() == PanicStrategy::Unwind,
     }
 }
 
@@ -2695,11 +2754,6 @@ pub fn conv_from_spec_abi(tcx: TyCtxt<'_>, abi: SpecAbi) -> Conv {
     }
 }
 
-pub fn fn_ptr_codegen_fn_attr_flags() -> CodegenFnAttrFlags {
-    // Assume that fn pointers may always unwind
-    CodegenFnAttrFlags::UNWIND
-}
-
 impl<'tcx, C> FnAbiExt<'tcx, C> for call::FnAbi<'tcx, Ty<'tcx>>
 where
     C: LayoutOf<Ty = Ty<'tcx>, TyAndLayout = TyAndLayout<'tcx>>
@@ -2709,7 +2763,7 @@ where
         + HasParamEnv<'tcx>,
 {
     fn of_fn_ptr(cx: &C, sig: ty::PolyFnSig<'tcx>, extra_args: &[Ty<'tcx>]) -> Self {
-        call::FnAbi::new_internal(cx, sig, extra_args, None, fn_ptr_codegen_fn_attr_flags(), false)
+        call::FnAbi::new_internal(cx, sig, extra_args, None, CodegenFnAttrFlags::empty(), false)
     }
 
     fn of_instance(cx: &C, instance: ty::Instance<'tcx>, extra_args: &[Ty<'tcx>]) -> Self {
@@ -2901,12 +2955,7 @@ where
             c_variadic: sig.c_variadic,
             fixed_count: inputs.len(),
             conv,
-            can_unwind: fn_can_unwind(
-                cx.tcx().sess.panic_strategy(),
-                codegen_fn_attr_flags,
-                conv,
-                sig.abi,
-            ),
+            can_unwind: fn_can_unwind(cx.tcx(), codegen_fn_attr_flags, sig.abi),
         };
         fn_abi.adjust_for_abi(cx, sig.abi);
         debug!("FnAbi::new_internal = {:?}", fn_abi);

compiler/rustc_mir/src/interpret/terminator.rs

@@ -18,12 +18,7 @@ use super::{
 
 impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     fn fn_can_unwind(&self, attrs: CodegenFnAttrFlags, abi: Abi) -> bool {
-        layout::fn_can_unwind(
-            self.tcx.sess.panic_strategy(),
-            attrs,
-            layout::conv_from_spec_abi(*self.tcx, abi),
-            abi,
-        )
+        layout::fn_can_unwind(*self.tcx, attrs, abi)
     }
 
     pub(super) fn eval_terminator(
@@ -77,7 +72,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                         (
                             fn_val,
                             caller_abi,
-                            self.fn_can_unwind(layout::fn_ptr_codegen_fn_attr_flags(), caller_abi),
+                            self.fn_can_unwind(CodegenFnAttrFlags::empty(), caller_abi),
                         )
                     }
                     ty::FnDef(def_id, substs) => {

compiler/rustc_mir/src/shim.rs

@@ -16,7 +16,7 @@ use std::fmt;
 use std::iter;
 
 use crate::transform::{
-    add_call_guards, add_moves_for_packed_drops, no_landing_pads, remove_noop_landing_pads,
+    abort_unwinding_calls, add_call_guards, add_moves_for_packed_drops, remove_noop_landing_pads,
     run_passes, simplify,
 };
 use crate::util::elaborate_drops::{self, DropElaborator, DropFlagMode, DropStyle};
@@ -81,10 +81,10 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<'
         MirPhase::Const,
         &[&[
             &add_moves_for_packed_drops::AddMovesForPackedDrops,
-            &no_landing_pads::NoLandingPads,
             &remove_noop_landing_pads::RemoveNoopLandingPads,
             &simplify::SimplifyCfg::new("make_shim"),
             &add_call_guards::CriticalCallEdges,
+            &abort_unwinding_calls::AbortUnwindingCalls,
         ]],
     );