Auto merge of rust-lang#135160 - RalfJung:aarch64-softfloat-not-neon, r=Noratrieb

aarch64-softfloat: forbid enabling the neon target feature

This fixes rust-lang#134375 in a rather crude way, by making [the example](https://godbolt.org/z/r56xWo8nT) not build any more on aarch64-unknown-none-softfloat. That is a breaking change since the "neon" aarch64 target feature is stable, but this is justified as a soundness fix. Note that it's not "neon" which is problematic but "fp-armv8"; however, the two are tied together by rustc.

`-Ctarget-feature=+neon` still works, it just causes a warning (but one that we do hope to make a hard error eventually). Only `#[target_feature="neon"]` becomes a hard error with this PR.

More work on the LLVM side will be needed before we can let people use neon without impacting the ABI of float values (and, in particular, the ABI used by automatically inserted calls to libm functions, e.g. for int-to-float casts, which rustc has no control over).

Nominating for `@rust-lang/lang` since it is a breaking change. As-is this PR doesn't have a warning cycle; the hope is that the aarch64-unknown-none-softfloat target is sufficiently niche that there's no huge fallout and we can easily revert if it causes trouble. A warning cycle could be added but would need some dedicated rather hacky check in the target_feature attribute handling logic.

try-job:  dist-various-1

Author: bors

compiler/rustc_codegen_ssa/messages.ftl

@@ -1,5 +1,7 @@
 codegen_ssa_L4Bender_exporting_symbols_unimplemented = exporting symbols not implemented yet for L4Bender
 
+codegen_ssa_aarch64_softfloat_neon = enabling the `neon` target feature on the current target is unsound due to ABI issues
+
 codegen_ssa_add_native_library = failed to add native library {$library_path}: {$error}
 
 codegen_ssa_aix_strip_not_used = using host's `strip` binary to cross-compile to AIX which is not guaranteed to work

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

@@ -299,6 +299,7 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
                 }
                 from_target_feature_attr(
                     tcx,
+                    did,
                     attr,
                     rust_target_features,
                     &mut codegen_fn_attrs.target_features,

compiler/rustc_codegen_ssa/src/errors.rs

@@ -1316,3 +1316,7 @@ pub(crate) struct XcrunSdkPathWarning {
     pub sdk_name: &'static str,
     pub stderr: String,
 }
+
+#[derive(LintDiagnostic)]
+#[diag(codegen_ssa_aarch64_softfloat_neon)]
+pub(crate) struct Aarch64SoftfloatNeon;

compiler/rustc_codegen_ssa/src/target_features.rs

@@ -8,6 +8,7 @@ use rustc_hir::def_id::{DefId, LOCAL_CRATE, LocalDefId};
 use rustc_middle::middle::codegen_fn_attrs::TargetFeature;
 use rustc_middle::query::Providers;
 use rustc_middle::ty::TyCtxt;
+use rustc_session::lint::builtin::AARCH64_SOFTFLOAT_NEON;
 use rustc_session::parse::feature_err;
 use rustc_span::{Span, Symbol, sym};
 use rustc_target::target_features::{self, Stability};
@@ -18,6 +19,7 @@ use crate::errors;
 /// Enabled target features are added to `target_features`.
 pub(crate) fn from_target_feature_attr(
     tcx: TyCtxt<'_>,
+    did: LocalDefId,
     attr: &hir::Attribute,
     rust_target_features: &UnordMap<String, target_features::Stability>,
     target_features: &mut Vec<TargetFeature>,
@@ -92,11 +94,22 @@ pub(crate) fn from_target_feature_attr(
                     // generating code so "it's fine".
                     if !tcx.sess.opts.actually_rustdoc {
                         if abi_feature_constraints.incompatible.contains(&name.as_str()) {
-                            tcx.dcx().emit_err(errors::ForbiddenTargetFeatureAttr {
-                                span: item.span(),
-                                feature: name.as_str(),
-                                reason: "this feature is incompatible with the target ABI",
-                            });
+                            // For "neon" specifically, we emit an FCW instead of a hard error.
+                            // See <https://github.com/rust-lang/rust/issues/134375>.
+                            if tcx.sess.target.arch == "aarch64" && name.as_str() == "neon" {
+                                tcx.emit_node_span_lint(
+                                    AARCH64_SOFTFLOAT_NEON,
+                                    tcx.local_def_id_to_hir_id(did),
+                                    item.span(),
+                                    errors::Aarch64SoftfloatNeon,
+                                );
+                            } else {
+                                tcx.dcx().emit_err(errors::ForbiddenTargetFeatureAttr {
+                                    span: item.span(),
+                                    feature: name.as_str(),
+                                    reason: "this feature is incompatible with the target ABI",
+                                });
+                            }
                         }
                     }
                     target_features.push(TargetFeature { name, implied: name != feature_sym })

compiler/rustc_lint_defs/src/builtin.rs

@@ -16,6 +16,7 @@ declare_lint_pass! {
     /// that are used by other parts of the compiler.
     HardwiredLints => [
         // tidy-alphabetical-start
+        AARCH64_SOFTFLOAT_NEON,
         ABSOLUTE_PATHS_NOT_STARTING_WITH_CRATE,
         AMBIGUOUS_ASSOCIATED_ITEMS,
         AMBIGUOUS_GLOB_IMPORTS,
@@ -5043,14 +5044,14 @@ declare_lint! {
     ///
     /// ```text
     /// error: this function function definition is affected by the wasm ABI transition: it passes an argument of non-scalar type `MyType`
-    /// --> $DIR/wasm_c_abi_transition.rs:17:1
-    ///  |
-    ///  | pub extern "C" fn my_fun(_x: MyType) {}
-    ///  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-    ///  |
-    ///  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
-    ///  = note: for more information, see issue #138762 <https://github.com/rust-lang/rust/issues/138762>
-    ///  = help: the "C" ABI Rust uses on wasm32-unknown-unknown will change to align with the standard "C" ABI for this target
+    ///   --> $DIR/wasm_c_abi_transition.rs:17:1
+    ///    |
+    ///    | pub extern "C" fn my_fun(_x: MyType) {}
+    ///    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    ///    |
+    ///    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+    ///    = note: for more information, see issue #138762 <https://github.com/rust-lang/rust/issues/138762>
+    ///    = help: the "C" ABI Rust uses on wasm32-unknown-unknown will change to align with the standard "C" ABI for this target
     /// ```
     ///
     /// ### Explanation
@@ -5067,3 +5068,44 @@ declare_lint! {
         reference: "issue #138762 <https://github.com/rust-lang/rust/issues/138762>",
     };
 }
+
+declare_lint! {
+    /// The `aarch64_softfloat_neon` lint detects usage of `#[target_feature(enable = "neon")]` on
+    /// softfloat aarch64 targets. Enabling this target feature causes LLVM to alter the ABI of
+    /// function calls, making this attribute unsound to use.
+    ///
+    /// ### Example
+    ///
+    /// ```rust,ignore (needs aarch64-unknown-none-softfloat)
+    /// #[target_feature(enable = "neon")]
+    /// fn with_neon() {}
+    /// ```
+    ///
+    /// This will produce:
+    ///
+    /// ```text
+    /// error: enabling the `neon` target feature on the current target is unsound due to ABI issues
+    ///   --> $DIR/abi-incompatible-target-feature-attribute-fcw.rs:11:18
+    ///    |
+    ///    | #[target_feature(enable = "neon")]
+    ///    |                  ^^^^^^^^^^^^^^^
+    ///    |
+    ///    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+    ///    = note: for more information, see issue #134375 <https://github.com/rust-lang/rust/issues/134375>
+    /// ```
+    ///
+    /// ### Explanation
+    ///
+    /// If a function like `with_neon` above ends up containing calls to LLVM builtins, those will
+    /// not use the correct ABI. This is caused by a lack of support in LLVM for mixing code with
+    /// and without the `neon` target feature. The target feature should never have been stabilized
+    /// on this target due to this issue, but the problem was not known at the time of
+    /// stabilization.
+    pub AARCH64_SOFTFLOAT_NEON,
+    Warn,
+    "detects code that could be affected by ABI issues on aarch64 softfloat targets",
+    @future_incompatible = FutureIncompatibleInfo {
+        reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps,
+        reference: "issue #134375 <https://github.com/rust-lang/rust/issues/134375>",
+    };
+}

compiler/rustc_target/src/target_features.rs

@@ -980,14 +980,16 @@ impl Target {
                 // the use of soft-float, so all we can do here is some crude hacks.
                 match &*self.abi {
                     "softfloat" => {
-                        // This is not fully correct, LLVM actually doesn't let us enforce the softfloat
-                        // ABI properly... see <https://github.com/rust-lang/rust/issues/134375>.
-                        // FIXME: should we forbid "neon" here? But that would be a breaking change.
-                        NOTHING
+                        // LLVM will use float registers when `fp-armv8` is available, e.g. for
+                        // calls to built-ins. The only way to ensure a consistent softfloat ABI
+                        // on aarch64 is to never enable `fp-armv8`, so we enforce that.
+                        // In Rust we tie `neon` and `fp-armv8` together, therefore `neon` is the
+                        // feature we have to mark as incompatible.
+                        FeatureConstraints { required: &[], incompatible: &["neon"] }
                     }
                     _ => {
                         // Everything else is assumed to use a hardfloat ABI. neon and fp-armv8 must be enabled.
-                        // These are Rust feature names and we use "neon" to control both of them.
+                        // `FeatureConstraints` uses Rust feature names, hence only "neon" shows up.
                         FeatureConstraints { required: &["neon"], incompatible: &[] }
                     }
                 }

tests/ui/target-feature/abi-incompatible-target-feature-attribute-fcw.rs

@@ -0,0 +1,14 @@
+//@ compile-flags: --crate-type=lib
+//@ compile-flags: --target=aarch64-unknown-none-softfloat
+//@ needs-llvm-components: aarch64
+#![feature(no_core, lang_items)]
+#![no_core]
+#![deny(aarch64_softfloat_neon)]
+
+#[lang = "sized"]
+pub trait Sized {}
+
+#[target_feature(enable = "neon")]
+//~^ERROR: enabling the `neon` target feature on the current target is unsound
+//~|WARN: previously accepted
+pub unsafe fn my_fun() {}

tests/ui/target-feature/abi-incompatible-target-feature-attribute-fcw.stderr

@@ -0,0 +1,31 @@
+error: enabling the `neon` target feature on the current target is unsound due to ABI issues
+  --> $DIR/abi-incompatible-target-feature-attribute-fcw.rs:11:18
+   |
+LL | #[target_feature(enable = "neon")]
+   |                  ^^^^^^^^^^^^^^^
+   |
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #134375 <https://github.com/rust-lang/rust/issues/134375>
+note: the lint level is defined here
+  --> $DIR/abi-incompatible-target-feature-attribute-fcw.rs:6:9
+   |
+LL | #![deny(aarch64_softfloat_neon)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 1 previous error
+
+Future incompatibility report: Future breakage diagnostic:
+error: enabling the `neon` target feature on the current target is unsound due to ABI issues
+  --> $DIR/abi-incompatible-target-feature-attribute-fcw.rs:11:18
+   |
+LL | #[target_feature(enable = "neon")]
+   |                  ^^^^^^^^^^^^^^^
+   |
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #134375 <https://github.com/rust-lang/rust/issues/134375>
+note: the lint level is defined here
+  --> $DIR/abi-incompatible-target-feature-attribute-fcw.rs:6:9
+   |
+LL | #![deny(aarch64_softfloat_neon)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^
+