Auto merge of #133417 - RalfJung:aarch64-float-abi, r=workingjubilee

reject aarch64 target feature toggling that would change the float ABI

~~Stacked on top of #133099. Only the last two commits are new.~~

The first new commit lays the groundwork for separately controlling whether a feature may be enabled or disabled. The second commit uses that to make it illegal to *disable* the `neon` feature (which is only possible via `-Ctarget-feature`, and so the new check just adds a warning). Enabling the `neon` feature remains allowed on targets that don't disable `neon` or `fp-armv8`, which is all our built-in targets. This way, the entire PR is not a breaking change.

Fixes #131058 for hardfloat targets (together with #133102 which fixed it for softfloat targets).

Part of #116344.

Author: bors

Diff for: compiler/rustc_codegen_gcc/src/gcc_util.rs

@@ -96,7 +96,7 @@ pub(crate) fn global_gcc_features(sess: &Session, diagnostics: bool) -> Vec<Stri
                     }
                     Some((_, stability, _)) => {
                         if let Err(reason) =
-                            stability.compute_toggleability(&sess.target).allow_toggle()
+                            stability.toggle_allowed(&sess.target, enable_disable == '+')
                         {
                             sess.dcx().emit_warn(ForbiddenCTargetFeature { feature, reason });
                         } else if stability.requires_nightly().is_some() {

Diff for: compiler/rustc_codegen_gcc/src/lib.rs

@@ -483,9 +483,9 @@ fn target_features_cfg(
         .rust_target_features()
         .iter()
         .filter(|(_, gate, _)| gate.in_cfg())
-        .filter_map(|&(feature, gate, _)| {
+        .filter_map(|(feature, gate, _)| {
             if sess.is_nightly_build() || allow_unstable || gate.requires_nightly().is_none() {
-                Some(feature)
+                Some(*feature)
             } else {
                 None
             }

Diff for: compiler/rustc_codegen_llvm/src/llvm_util.rs

@@ -373,9 +373,9 @@ pub fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec<Symbol>
         .rust_target_features()
         .iter()
         .filter(|(_, gate, _)| gate.in_cfg())
-        .filter_map(|&(feature, gate, _)| {
+        .filter_map(|(feature, gate, _)| {
             if sess.is_nightly_build() || allow_unstable || gate.requires_nightly().is_none() {
-                Some(feature)
+                Some(*feature)
             } else {
                 None
             }
@@ -718,7 +718,7 @@ pub(crate) fn global_llvm_features(
                         }
                         Some((_, stability, _)) => {
                             if let Err(reason) =
-                                stability.compute_toggleability(&sess.target).allow_toggle()
+                                stability.toggle_allowed(&sess.target, enable_disable == '+')
                             {
                                 sess.dcx().emit_warn(ForbiddenCTargetFeature { feature, reason });
                             } else if stability.requires_nightly().is_some() {

Diff for: compiler/rustc_codegen_ssa/src/target_features.rs

@@ -65,7 +65,7 @@ pub(crate) fn from_target_feature_attr(
 
             // Only allow target features whose feature gates have been enabled
             // and which are permitted to be toggled.
-            if let Err(reason) = stability.allow_toggle() {
+            if let Err(reason) = stability.toggle_allowed(/*enable*/ true) {
                 tcx.dcx().emit_err(errors::ForbiddenTargetFeatureAttr {
                     span: item.span(),
                     feature,
@@ -160,7 +160,7 @@ pub(crate) fn provide(providers: &mut Providers) {
                     .target
                     .rust_target_features()
                     .iter()
-                    .map(|&(a, b, _)| (a.to_string(), b.compute_toggleability(target)))
+                    .map(|(a, b, _)| (a.to_string(), b.compute_toggleability(target)))
                     .collect()
             }
         },

Diff for: compiler/rustc_target/src/spec/mod.rs

@@ -2213,6 +2213,10 @@ pub struct TargetOptions {
     /// `-Ctarget-cpu` but can be overwritten with `-Ctarget-features`.
     /// Corresponds to `llc -mattr=$features`.
     /// Note that these are LLVM feature names, not Rust feature names!
+    ///
+    /// Generally it is a bad idea to use negative target features because they often interact very
+    /// poorly with how `-Ctarget-cpu` works. Instead, try to use a lower "base CPU" and enable the
+    /// features you want to use.
     pub features: StaticCow<str>,
     /// Direct or use GOT indirect to reference external data symbols
     pub direct_access_external_data: Option<bool>,
@@ -2605,15 +2609,11 @@ impl TargetOptions {
     }
 
     pub(crate) fn has_feature(&self, search_feature: &str) -> bool {
-        self.features.split(',').any(|f| {
-            if let Some(f) = f.strip_prefix('+')
-                && f == search_feature
-            {
-                true
-            } else {
-                false
-            }
-        })
+        self.features.split(',').any(|f| f.strip_prefix('+').is_some_and(|f| f == search_feature))
+    }
+
+    pub(crate) fn has_neg_feature(&self, search_feature: &str) -> bool {
+        self.features.split(',').any(|f| f.strip_prefix('-').is_some_and(|f| f == search_feature))
     }
 }
 

Diff for: compiler/rustc_target/src/target_features.rs

@@ -20,7 +20,7 @@ pub const RUSTC_SPECIAL_FEATURES: &[&str] = &["backchain"];
 /// `Toggleability` is the type storing whether (un)stable features can be toggled:
 /// this is initially a function since it can depend on `Target`, but for stable hashing
 /// it needs to be something hashable to we have to make the type generic.
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone)]
 pub enum Stability<Toggleability> {
     /// This target feature is stable, it can be used in `#[target_feature]` and
     /// `#[cfg(target_feature)]`.
@@ -44,11 +44,21 @@ pub enum Stability<Toggleability> {
     Forbidden { reason: &'static str },
 }
 
-/// `Stability` where `allow_toggle` has not been computed yet.
 /// Returns `Ok` if the toggle is allowed, `Err` with an explanation of not.
-pub type StabilityUncomputed = Stability<fn(&Target) -> Result<(), &'static str>>;
+/// The `bool` indicates whether the feature is being enabled (`true`) or disabled.
+pub type AllowToggleUncomputed = fn(&Target, bool) -> Result<(), &'static str>;
+
+/// The computed result of whether a feature can be enabled/disabled on the current target.
+#[derive(Debug, Clone)]
+pub struct AllowToggleComputed {
+    enable: Result<(), &'static str>,
+    disable: Result<(), &'static str>,
+}
+
+/// `Stability` where `allow_toggle` has not been computed yet.
+pub type StabilityUncomputed = Stability<AllowToggleUncomputed>;
 /// `Stability` where `allow_toggle` has already been computed.
-pub type StabilityComputed = Stability<Result<(), &'static str>>;
+pub type StabilityComputed = Stability<AllowToggleComputed>;
 
 impl<CTX, Toggleability: HashStable<CTX>> HashStable<CTX> for Stability<Toggleability> {
     #[inline]
@@ -69,11 +79,20 @@ impl<CTX, Toggleability: HashStable<CTX>> HashStable<CTX> for Stability<Toggleab
     }
 }
 
+impl<CTX> HashStable<CTX> for AllowToggleComputed {
+    #[inline]
+    fn hash_stable(&self, hcx: &mut CTX, hasher: &mut StableHasher) {
+        let AllowToggleComputed { enable, disable } = self;
+        enable.hash_stable(hcx, hasher);
+        disable.hash_stable(hcx, hasher);
+    }
+}
+
 impl<Toggleability> Stability<Toggleability> {
     /// Returns whether the feature can be used in `cfg(target_feature)` ever.
     /// (It might still be nightly-only even if this returns `true`, so make sure to also check
     /// `requires_nightly`.)
-    pub fn in_cfg(self) -> bool {
+    pub fn in_cfg(&self) -> bool {
         !matches!(self, Stability::Forbidden { .. })
     }
 
@@ -85,8 +104,8 @@ impl<Toggleability> Stability<Toggleability> {
     /// Before calling this, ensure the feature is even permitted for this use:
     /// - for `#[target_feature]`/`-Ctarget-feature`, check `allow_toggle()`
     /// - for `cfg(target_feature)`, check `in_cfg`
-    pub fn requires_nightly(self) -> Option<Symbol> {
-        match self {
+    pub fn requires_nightly(&self) -> Option<Symbol> {
+        match *self {
             Stability::Unstable { nightly_feature, .. } => Some(nightly_feature),
             Stability::Stable { .. } => None,
             Stability::Forbidden { .. } => panic!("forbidden features should not reach this far"),
@@ -95,35 +114,49 @@ impl<Toggleability> Stability<Toggleability> {
 }
 
 impl StabilityUncomputed {
-    pub fn compute_toggleability(self, target: &Target) -> StabilityComputed {
+    pub fn compute_toggleability(&self, target: &Target) -> StabilityComputed {
         use Stability::*;
-        match self {
-            Stable { allow_toggle } => Stable { allow_toggle: allow_toggle(target) },
+        let compute = |f: AllowToggleUncomputed| AllowToggleComputed {
+            enable: f(target, true),
+            disable: f(target, false),
+        };
+        match *self {
+            Stable { allow_toggle } => Stable { allow_toggle: compute(allow_toggle) },
             Unstable { nightly_feature, allow_toggle } => {
-                Unstable { nightly_feature, allow_toggle: allow_toggle(target) }
+                Unstable { nightly_feature, allow_toggle: compute(allow_toggle) }
             }
             Forbidden { reason } => Forbidden { reason },
         }
     }
+
+    pub fn toggle_allowed(&self, target: &Target, enable: bool) -> Result<(), &'static str> {
+        use Stability::*;
+        match *self {
+            Stable { allow_toggle } => allow_toggle(target, enable),
+            Unstable { allow_toggle, .. } => allow_toggle(target, enable),
+            Forbidden { reason } => Err(reason),
+        }
+    }
 }
 
 impl StabilityComputed {
     /// Returns whether the feature may be toggled via `#[target_feature]` or `-Ctarget-feature`.
     /// (It might still be nightly-only even if this returns `true`, so make sure to also check
     /// `requires_nightly`.)
-    pub fn allow_toggle(self) -> Result<(), &'static str> {
-        match self {
+    pub fn toggle_allowed(&self, enable: bool) -> Result<(), &'static str> {
+        let allow_toggle = match self {
             Stability::Stable { allow_toggle } => allow_toggle,
             Stability::Unstable { allow_toggle, .. } => allow_toggle,
-            Stability::Forbidden { reason } => Err(reason),
-        }
+            Stability::Forbidden { reason } => return Err(reason),
+        };
+        if enable { allow_toggle.enable } else { allow_toggle.disable }
     }
 }
 
 // Constructors for the list below, defaulting to "always allow toggle".
-const STABLE: StabilityUncomputed = Stability::Stable { allow_toggle: |_target| Ok(()) };
+const STABLE: StabilityUncomputed = Stability::Stable { allow_toggle: |_target, _enable| Ok(()) };
 const fn unstable(nightly_feature: Symbol) -> StabilityUncomputed {
-    Stability::Unstable { nightly_feature, allow_toggle: |_target| Ok(()) }
+    Stability::Unstable { nightly_feature, allow_toggle: |_target, _enable| Ok(()) }
 }
 
 // Here we list target features that rustc "understands": they can be used in `#[target_feature]`
@@ -184,7 +217,7 @@ const ARM_FEATURES: &[(&str, StabilityUncomputed, ImpliedFeatures)] = &[
         "fpregs",
         Stability::Unstable {
             nightly_feature: sym::arm_target_feature,
-            allow_toggle: |target: &Target| {
+            allow_toggle: |target: &Target, _enable| {
                 // Only allow toggling this if the target has `soft-float` set. With `soft-float`,
                 // `fpregs` isn't needed so changing it cannot affect the ABI.
                 if target.has_feature("soft-float") {
@@ -257,6 +290,7 @@ const AARCH64_FEATURES: &[(&str, StabilityUncomputed, ImpliedFeatures)] = &[
     ("flagm", STABLE, &[]),
     // FEAT_FLAGM2
     ("flagm2", unstable(sym::aarch64_unstable_target_feature), &[]),
+    ("fp-armv8", Stability::Forbidden { reason: "Rust ties `fp-armv8` to `neon`" }, &[]),
     // FEAT_FP16
     // Rust ties FP and Neon: https://github.com/rust-lang/rust/pull/91608
     ("fp16", STABLE, &["neon"]),
@@ -292,7 +326,28 @@ const AARCH64_FEATURES: &[(&str, StabilityUncomputed, ImpliedFeatures)] = &[
     // FEAT_MTE & FEAT_MTE2
     ("mte", STABLE, &[]),
     // FEAT_AdvSimd & FEAT_FP
-    ("neon", STABLE, &[]),
+    (
+        "neon",
+        Stability::Stable {
+            allow_toggle: |target, enable| {
+                if target.abi == "softfloat" {
+                    // `neon` has no ABI implications for softfloat targets, we can allow this.
+                    Ok(())
+                } else if enable
+                    && !target.has_neg_feature("fp-armv8")
+                    && !target.has_neg_feature("neon")
+                {
+                    // neon is enabled by default, and has not been disabled, so enabling it again
+                    // is redundant and we can permit it. Forbidding this would be a breaking change
+                    // since this feature is stable.
+                    Ok(())
+                } else {
+                    Err("unsound on hard-float targets because it changes float ABI")
+                }
+            },
+        },
+        &[],
+    ),
     // FEAT_PAUTH (address authentication)
     ("paca", STABLE, &[]),
     // FEAT_PAUTH (generic authentication)
@@ -481,7 +536,7 @@ const X86_FEATURES: &[(&str, StabilityUncomputed, ImpliedFeatures)] = &[
         "x87",
         Stability::Unstable {
             nightly_feature: sym::x87_target_feature,
-            allow_toggle: |target: &Target| {
+            allow_toggle: |target: &Target, _enable| {
                 // Only allow toggling this if the target has `soft-float` set. With `soft-float`,
                 // `fpregs` isn't needed so changing it cannot affect the ABI.
                 if target.has_feature("soft-float") {

Diff for: tests/ui/target-feature/feature-hierarchy.aarch64-sve2.stderr

@@ -0,0 +1,7 @@
+warning: target feature `neon` cannot be toggled with `-Ctarget-feature`: unsound on hard-float targets because it changes float ABI
+   |
+   = note: 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 #116344 <https://github.com/rust-lang/rust/issues/116344>
+
+warning: 1 warning emitted
+

Diff for: tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable-neon.rs

@@ -0,0 +1,10 @@
+//@ compile-flags: --target=aarch64-unknown-linux-gnu --crate-type=lib
+//@ needs-llvm-components: aarch64
+//@ compile-flags: -Ctarget-feature=-neon
+// For now this is just a warning.
+//@ build-pass
+#![feature(no_core, lang_items)]
+#![no_core]
+
+#[lang = "sized"]
+pub trait Sized {}

Diff for: tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable-neon.stderr

@@ -0,0 +1,7 @@
+warning: target feature `neon` cannot be toggled with `-Ctarget-feature`: unsound on hard-float targets because it changes float ABI
+   |
+   = note: 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 #116344 <https://github.com/rust-lang/rust/issues/116344>
+
+warning: 1 warning emitted
+