From eb2e928250066df9e40291fb9fb97308df16046e Mon Sep 17 00:00:00 2001
From: Ralf Jung <post@ralfj.de>
Date: Sun, 24 Nov 2024 12:22:01 +0100
Subject: [PATCH 1/4] target_features: control separately whether enabling and
disabling a target feature is allowed
---
compiler/rustc_codegen_gcc/src/gcc_util.rs | 2 +-
compiler/rustc_codegen_gcc/src/lib.rs | 4 +-
compiler/rustc_codegen_llvm/src/llvm_util.rs | 6 +-
.../rustc_codegen_ssa/src/target_features.rs | 4 +-
compiler/rustc_target/src/target_features.rs | 77 +++++++++++++------
5 files changed, 63 insertions(+), 30 deletions(-)
diff --git a/compiler/rustc_codegen_gcc/src/gcc_util.rs b/compiler/rustc_codegen_gcc/src/gcc_util.rs
index 88e5eefd7a156..058a874501b26 100644
--- a/compiler/rustc_codegen_gcc/src/gcc_util.rs
+++ b/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 --git a/compiler/rustc_codegen_gcc/src/lib.rs b/compiler/rustc_codegen_gcc/src/lib.rs
index 764e84be1feb9..bb0f2fa5b014e 100644
--- a/compiler/rustc_codegen_gcc/src/lib.rs
+++ b/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 --git a/compiler/rustc_codegen_llvm/src/llvm_util.rs b/compiler/rustc_codegen_llvm/src/llvm_util.rs
index bfec7d708cfbc..194438af88c26 100644
--- a/compiler/rustc_codegen_llvm/src/llvm_util.rs
+++ b/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 --git a/compiler/rustc_codegen_ssa/src/target_features.rs b/compiler/rustc_codegen_ssa/src/target_features.rs
index fa600ec7166ad..f4d4a9db1d898 100644
--- a/compiler/rustc_codegen_ssa/src/target_features.rs
+++ b/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 --git a/compiler/rustc_target/src/target_features.rs b/compiler/rustc_target/src/target_features.rs
index 29d3f826a1544..493c3a0ef6bc0 100644
--- a/compiler/rustc_target/src/target_features.rs
+++ b/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,24 +104,37 @@ 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> {
+ 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"),
+ &Stability::Unstable { nightly_feature, .. } => Some(nightly_feature),
+ &Stability::Stable { .. } => None,
+ &Stability::Forbidden { .. } => panic!("forbidden features should not reach this far"),
}
}
}
impl StabilityUncomputed {
- pub fn compute_toggleability(self, target: &Target) -> StabilityComputed {
+ pub fn compute_toggleability(&self, target: &Target) -> StabilityComputed {
use Stability::*;
+ let compute = |f: AllowToggleUncomputed| AllowToggleComputed {
+ enable: f(target, true),
+ disable: f(target, false),
+ };
match self {
- Stable { allow_toggle } => Stable { allow_toggle: allow_toggle(target) },
- Unstable { nightly_feature, allow_toggle } => {
- Unstable { nightly_feature, allow_toggle: allow_toggle(target) }
+ &Stable { allow_toggle } => Stable { allow_toggle: compute(allow_toggle) },
+ &Unstable { nightly_feature, allow_toggle } => {
+ Unstable { nightly_feature, allow_toggle: compute(allow_toggle) }
}
- Forbidden { reason } => Forbidden { reason },
+ &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),
}
}
}
@@ -111,19 +143,20 @@ 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") {
@@ -481,7 +514,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") {
From 1f8236d4c78fc111da14f4e00639e92ca05707de Mon Sep 17 00:00:00 2001
From: Ralf Jung <post@ralfj.de>
Date: Sun, 24 Nov 2024 12:35:50 +0100
Subject: [PATCH 2/4] reject aarch64 target feature toggling that would change
the float ABI
---
compiler/rustc_target/src/spec/mod.rs | 12 ++++++++++
compiler/rustc_target/src/target_features.rs | 24 ++++++++++++++++++-
.../feature-hierarchy.aarch64-sve2.stderr | 7 ++++++
...dfloat-target-feature-flag-disable-neon.rs | 10 ++++++++
...at-target-feature-flag-disable-neon.stderr | 7 ++++++
5 files changed, 59 insertions(+), 1 deletion(-)
create mode 100644 tests/ui/target-feature/feature-hierarchy.aarch64-sve2.stderr
create mode 100644 tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable-neon.rs
create mode 100644 tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable-neon.stderr
diff --git a/compiler/rustc_target/src/spec/mod.rs b/compiler/rustc_target/src/spec/mod.rs
index 7d308c6c662cd..06d2099a446ce 100644
--- a/compiler/rustc_target/src/spec/mod.rs
+++ b/compiler/rustc_target/src/spec/mod.rs
@@ -2615,6 +2615,18 @@ impl TargetOptions {
}
})
}
+
+ pub(crate) fn has_neg_feature(&self, search_feature: &str) -> bool {
+ self.features.split(',').any(|f| {
+ if let Some(f) = f.strip_prefix('-')
+ && f == search_feature
+ {
+ true
+ } else {
+ false
+ }
+ })
+ }
}
impl Default for TargetOptions {
diff --git a/compiler/rustc_target/src/target_features.rs b/compiler/rustc_target/src/target_features.rs
index 493c3a0ef6bc0..e1f7884610c54 100644
--- a/compiler/rustc_target/src/target_features.rs
+++ b/compiler/rustc_target/src/target_features.rs
@@ -290,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"]),
@@ -325,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)
diff --git a/tests/ui/target-feature/feature-hierarchy.aarch64-sve2.stderr b/tests/ui/target-feature/feature-hierarchy.aarch64-sve2.stderr
new file mode 100644
index 0000000000000..d5d7d7aa627ba
--- /dev/null
+++ b/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 --git a/tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable-neon.rs b/tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable-neon.rs
new file mode 100644
index 0000000000000..95c366bb3f5be
--- /dev/null
+++ b/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 --git a/tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable-neon.stderr b/tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable-neon.stderr
new file mode 100644
index 0000000000000..d5d7d7aa627ba
--- /dev/null
+++ b/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
+
From 0c9d42cc56626bb383cbab3bb8529a8f1a117fed Mon Sep 17 00:00:00 2001
From: Ralf Jung <post@ralfj.de>
Date: Sun, 15 Dec 2024 09:25:11 +0100
Subject: [PATCH 3/4] apply review feedback
---
compiler/rustc_target/src/spec/mod.rs | 20 ++--------------
compiler/rustc_target/src/target_features.rs | 24 ++++++++++----------
2 files changed, 14 insertions(+), 30 deletions(-)
diff --git a/compiler/rustc_target/src/spec/mod.rs b/compiler/rustc_target/src/spec/mod.rs
index 06d2099a446ce..f7cf12215f3bf 100644
--- a/compiler/rustc_target/src/spec/mod.rs
+++ b/compiler/rustc_target/src/spec/mod.rs
@@ -2605,27 +2605,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| {
- 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))
}
}
diff --git a/compiler/rustc_target/src/target_features.rs b/compiler/rustc_target/src/target_features.rs
index e1f7884610c54..713b5dc70d7ad 100644
--- a/compiler/rustc_target/src/target_features.rs
+++ b/compiler/rustc_target/src/target_features.rs
@@ -105,10 +105,10 @@ impl<Toggleability> Stability<Toggleability> {
/// - for `#[target_feature]`/`-Ctarget-feature`, check `allow_toggle()`
/// - for `cfg(target_feature)`, check `in_cfg`
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"),
+ match *self {
+ Stability::Unstable { nightly_feature, .. } => Some(nightly_feature),
+ Stability::Stable { .. } => None,
+ Stability::Forbidden { .. } => panic!("forbidden features should not reach this far"),
}
}
}
@@ -120,21 +120,21 @@ impl StabilityUncomputed {
enable: f(target, true),
disable: f(target, false),
};
- match self {
- &Stable { allow_toggle } => Stable { allow_toggle: compute(allow_toggle) },
- &Unstable { nightly_feature, allow_toggle } => {
+ match *self {
+ Stable { allow_toggle } => Stable { allow_toggle: compute(allow_toggle) },
+ Unstable { nightly_feature, allow_toggle } => {
Unstable { nightly_feature, allow_toggle: compute(allow_toggle) }
}
- &Forbidden { reason } => Forbidden { reason },
+ 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),
+ match *self {
+ Stable { allow_toggle } => allow_toggle(target, enable),
+ Unstable { allow_toggle, .. } => allow_toggle(target, enable),
+ Forbidden { reason } => Err(reason),
}
}
}
From 74e2ac406ba620aeff8732d2dde96c0839dcacbf Mon Sep 17 00:00:00 2001
From: Ralf Jung <post@ralfj.de>
Date: Sun, 15 Dec 2024 11:20:00 +0100
Subject: [PATCH 4/4] advice against negative features in target specs
Co-authored-by: Jubilee <workingjubilee@gmail.com>
---
compiler/rustc_target/src/spec/mod.rs | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/compiler/rustc_target/src/spec/mod.rs b/compiler/rustc_target/src/spec/mod.rs
index f7cf12215f3bf..a44d2af6b9086 100644
--- a/compiler/rustc_target/src/spec/mod.rs
+++ b/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>,