show warning when required target feature is missing, rather than imp…

…licitly enabling it
rust-lang · Jan 12, 2025 · 1a6422e · 1a6422e
1 parent 03a25ea
commit 1a6422e
Show file tree

Hide file tree

Showing 15 changed files with 82 additions and 120 deletions.
diff --git a/compiler/rustc_codegen_gcc/src/gcc_util.rs b/compiler/rustc_codegen_gcc/src/gcc_util.rs
@@ -1,10 +1,8 @@
-use std::iter::FromIterator;
-
 #[cfg(feature = "master")]
 use gccjit::Context;
 use rustc_codegen_ssa::codegen_attrs::check_tied_features;
 use rustc_codegen_ssa::errors::TargetFeatureDisableOrEnable;
-use rustc_data_structures::fx::{FxHashMap, FxHashSet};
+use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::unord::UnordSet;
 use rustc_session::Session;
 use rustc_target::target_features::RUSTC_SPECIFIC_FEATURES;
@@ -45,12 +43,6 @@ pub(crate) fn global_gcc_features(sess: &Session, diagnostics: bool) -> Vec<Stri
     let known_features = sess.target.rust_target_features();
     let mut featsmap = FxHashMap::default();
 
-    // Ensure that all ABI-required features are enabled, and the ABI-forbidden ones
-    // are disabled.
-    let abi_feature_constraints = sess.target.abi_required_features();
-    let abi_incompatible_set =
-        FxHashSet::from_iter(abi_feature_constraints.incompatible.iter().copied());
-
     // Compute implied features
     let mut all_rust_features = vec![];
     for feature in sess.opts.cg.target_feature.split(',') {
@@ -119,51 +111,11 @@ pub(crate) fn global_gcc_features(sess: &Session, diagnostics: bool) -> Vec<Stri
                 }
             }
 
-            // Ensure that the features we enable/disable are compatible with the ABI.
-            if enable {
-                if abi_incompatible_set.contains(feature) {
-                    sess.dcx().emit_warn(ForbiddenCTargetFeature {
-                        feature,
-                        enabled: "enabled",
-                        reason: "this feature is incompatible with the target ABI",
-                    });
-                }
-            } else {
-                // FIXME: we have to request implied features here since
-                // negative features do not handle implied features above.
-                for &required in abi_feature_constraints.required.iter() {
-                    let implied = sess.target.implied_target_features(std::iter::once(required));
-                    if implied.contains(feature) {
-                        sess.dcx().emit_warn(ForbiddenCTargetFeature {
-                            feature,
-                            enabled: "disabled",
-                            reason: "this feature is required by the target ABI",
-                        });
-                    }
-                }
-            }
-
             // FIXME(nagisa): figure out how to not allocate a full hashset here.
             featsmap.insert(feature, enable);
         }
     }
 
-    // To be sure the ABI-relevant features are all in the right state, we explicitly
-    // (un)set them here. This means if the target spec sets those features wrong,
-    // we will silently correct them rather than silently producing wrong code.
-    // (The target sanity check tries to catch this, but we can't know which features are
-    // enabled in GCC by default so we can't be fully sure about that check.)
-    // We add these at the beginning of the list so that `-Ctarget-features` can
-    // still override it... that's unsound, but more compatible with past behavior.
-    all_rust_features.splice(
-        0..0,
-        abi_feature_constraints
-            .required
-            .iter()
-            .map(|&f| (true, f))
-            .chain(abi_feature_constraints.incompatible.iter().map(|&f| (false, f))),
-    );
-
     // Translate this into GCC features.
     let feats = all_rust_features
         .iter()

diff --git a/compiler/rustc_codegen_gcc/src/lib.rs b/compiler/rustc_codegen_gcc/src/lib.rs
@@ -492,9 +492,10 @@ fn target_features_cfg(
     sess.target
         .rust_target_features()
         .iter()
-        .filter(|(_, gate, _)| gate.in_cfg())
         .filter_map(|(feature, gate, _)| {
-            if sess.is_nightly_build() || allow_unstable || gate.requires_nightly().is_none() {
+            if allow_unstable
+                || (gate.in_cfg() && (sess.is_nightly_build() || gate.requires_nightly().is_none()))
+            {
                 Some(*feature)
             } else {
                 None

diff --git a/compiler/rustc_codegen_llvm/src/llvm_util.rs b/compiler/rustc_codegen_llvm/src/llvm_util.rs
@@ -319,7 +319,6 @@ pub fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec<Symbol>
         sess.target
             .rust_target_features()
             .iter()
-            .filter(|(_, gate, _)| gate.in_cfg())
             .filter(|(feature, _, _)| {
                 // skip checking special features, as LLVM may not understand them
                 if RUSTC_SPECIAL_FEATURES.contains(feature) {
@@ -388,9 +387,13 @@ pub fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec<Symbol>
     sess.target
         .rust_target_features()
         .iter()
-        .filter(|(_, gate, _)| gate.in_cfg())
         .filter_map(|(feature, gate, _)| {
-            if sess.is_nightly_build() || allow_unstable || gate.requires_nightly().is_none() {
+            // The `allow_unstable` set is used by rustc internally to determined which target
+            // features are truly available, so we want to return even perma-unstable "forbidden"
+            // features.
+            if allow_unstable
+                || (gate.in_cfg() && (sess.is_nightly_build() || gate.requires_nightly().is_none()))
+            {
                 Some(*feature)
             } else {
                 None
@@ -670,12 +673,6 @@ pub(crate) fn global_llvm_features(
         // Will only be filled when `diagnostics` is set!
         let mut featsmap = FxHashMap::default();
 
-        // Ensure that all ABI-required features are enabled, and the ABI-forbidden ones
-        // are disabled.
-        let abi_feature_constraints = sess.target.abi_required_features();
-        let abi_incompatible_set =
-            FxHashSet::from_iter(abi_feature_constraints.incompatible.iter().copied());
-
         // Compute implied features
         let mut all_rust_features = vec![];
         for feature in sess.opts.cg.target_feature.split(',') {
@@ -746,52 +743,11 @@ pub(crate) fn global_llvm_features(
                     }
                 }
 
-                // Ensure that the features we enable/disable are compatible with the ABI.
-                if enable {
-                    if abi_incompatible_set.contains(feature) {
-                        sess.dcx().emit_warn(ForbiddenCTargetFeature {
-                            feature,
-                            enabled: "enabled",
-                            reason: "this feature is incompatible with the target ABI",
-                        });
-                    }
-                } else {
-                    // FIXME: we have to request implied features here since
-                    // negative features do not handle implied features above.
-                    for &required in abi_feature_constraints.required.iter() {
-                        let implied =
-                            sess.target.implied_target_features(std::iter::once(required));
-                        if implied.contains(feature) {
-                            sess.dcx().emit_warn(ForbiddenCTargetFeature {
-                                feature,
-                                enabled: "disabled",
-                                reason: "this feature is required by the target ABI",
-                            });
-                        }
-                    }
-                }
-
                 // FIXME(nagisa): figure out how to not allocate a full hashset here.
                 featsmap.insert(feature, enable);
             }
         }
 
-        // To be sure the ABI-relevant features are all in the right state, we explicitly
-        // (un)set them here. This means if the target spec sets those features wrong,
-        // we will silently correct them rather than silently producing wrong code.
-        // (The target sanity check tries to catch this, but we can't know which features are
-        // enabled in LLVM by default so we can't be fully sure about that check.)
-        // We add these at the beginning of the list so that `-Ctarget-features` can
-        // still override it... that's unsound, but more compatible with past behavior.
-        all_rust_features.splice(
-            0..0,
-            abi_feature_constraints
-                .required
-                .iter()
-                .map(|&f| (true, f))
-                .chain(abi_feature_constraints.incompatible.iter().map(|&f| (false, f))),
-        );
-
         // Translate this into LLVM features.
         let feats = all_rust_features
             .iter()

diff --git a/compiler/rustc_interface/messages.ftl b/compiler/rustc_interface/messages.ftl
@@ -1,3 +1,8 @@
+interface_abi_required_feature =
+    target feature `{$feature}` must be {$enabled} to ensure that the ABI of the current target can be implemented correctly
+    .note = this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+interface_abi_required_feature_issue = for more information, see issue #116344 <https://github.com/rust-lang/rust/issues/116344>
+
 interface_cant_emit_mir =
     could not emit MIR: {$error}
 

diff --git a/compiler/rustc_interface/src/errors.rs b/compiler/rustc_interface/src/errors.rs
@@ -103,3 +103,12 @@ pub struct IgnoringOutDir;
 #[derive(Diagnostic)]
 #[diag(interface_multiple_output_types_to_stdout)]
 pub struct MultipleOutputTypesToStdout;
+
+#[derive(Diagnostic)]
+#[diag(interface_abi_required_feature)]
+#[note]
+#[note(interface_abi_required_feature_issue)]
+pub(crate) struct AbiRequiredTargetFeature<'a> {
+    pub feature: &'a str,
+    pub enabled: &'a str,
+}
diff --git a/compiler/rustc_interface/src/interface.rs b/compiler/rustc_interface/src/interface.rs
@@ -484,6 +484,8 @@ pub fn run_compiler<R: Send>(config: Config, f: impl FnOnce(&Compiler) -> R + Se
             }
             sess.lint_store = Some(Lrc::new(lint_store));
 
+            util::check_abi_required_features(&sess);
+
             let compiler = Compiler {
                 sess,
                 codegen_backend,

diff --git a/compiler/rustc_interface/src/util.rs b/compiler/rustc_interface/src/util.rs
@@ -18,21 +18,25 @@ use rustc_session::{EarlyDiagCtxt, Session, filesearch};
 use rustc_span::edit_distance::find_best_match_for_name;
 use rustc_span::edition::Edition;
 use rustc_span::source_map::SourceMapInputs;
-use rustc_span::sym;
+use rustc_span::{Symbol, sym};
 use rustc_target::spec::Target;
 use tracing::info;
 
 use crate::errors;
 
 /// Function pointer type that constructs a new CodegenBackend.
-pub type MakeBackendFn = fn() -> Box<dyn CodegenBackend>;
+type MakeBackendFn = fn() -> Box<dyn CodegenBackend>;
 
 /// Adds `target_feature = "..."` cfgs for a variety of platform
 /// specific features (SSE, NEON etc.).
 ///
 /// This is performed by checking whether a set of permitted features
 /// is available on the target machine, by querying the codegen backend.
-pub fn add_configuration(cfg: &mut Cfg, sess: &mut Session, codegen_backend: &dyn CodegenBackend) {
+pub(crate) fn add_configuration(
+    cfg: &mut Cfg,
+    sess: &mut Session,
+    codegen_backend: &dyn CodegenBackend,
+) {
     let tf = sym::target_feature;
 
     let unstable_target_features = codegen_backend.target_features_cfg(sess, true);
@@ -48,6 +52,25 @@ pub fn add_configuration(cfg: &mut Cfg, sess: &mut Session, codegen_backend: &dy
     }
 }
 
+/// Ensures that all target features required by the ABI are present.
+/// Must be called after `unstable_target_features` has been populated!
+pub(crate) fn check_abi_required_features(sess: &Session) {
+    let abi_feature_constraints = sess.target.abi_required_features();
+    // We check this against `unstable_target_features` as that is conveniently already
+    // back-translated to rustc feature names.
+
+    for feature in abi_feature_constraints.required {
+        if !sess.unstable_target_features.contains(&Symbol::intern(feature)) {
+            sess.dcx().emit_warn(errors::AbiRequiredTargetFeature { feature, enabled: "enabled" });
+        }
+    }
+    for feature in abi_feature_constraints.incompatible {
+        if sess.unstable_target_features.contains(&Symbol::intern(feature)) {
+            sess.dcx().emit_warn(errors::AbiRequiredTargetFeature { feature, enabled: "disabled" });
+        }
+    }
+}
+
 pub static STACK_SIZE: OnceLock<usize> = OnceLock::new();
 pub const DEFAULT_STACK_SIZE: usize = 8 * 1024 * 1024;
 

diff --git a/tests/codegen/target-feature-overrides.rs b/tests/codegen/target-feature-overrides.rs
@@ -39,8 +39,8 @@ pub unsafe fn banana() -> u32 {
 }
 
 // CHECK: attributes [[APPLEATTRS]]
-// COMPAT-SAME: "target-features"="+x87,+sse2,+avx,+avx2,{{.*}}"
-// INCOMPAT-SAME: "target-features"="+x87,+sse2,-avx2,-avx,+avx,{{.*}}"
+// COMPAT-SAME: "target-features"="+avx,+avx2,{{.*}}"
+// INCOMPAT-SAME: "target-features"="-avx2,-avx,+avx,{{.*}}"
 // CHECK: attributes [[BANANAATTRS]]
-// COMPAT-SAME: "target-features"="+x87,+sse2,+avx,+avx2,{{.*}}"
-// INCOMPAT-SAME: "target-features"="+x87,+sse2,-avx2,-avx"
+// COMPAT-SAME: "target-features"="+avx,+avx2,{{.*}}"
+// INCOMPAT-SAME: "target-features"="-avx2,-avx"
diff --git a/tests/codegen/tied-features-strength.rs b/tests/codegen/tied-features-strength.rs
@@ -11,11 +11,10 @@
 // ENABLE_SVE: attributes #0 = { {{.*}} "target-features"="{{((\+outline-atomics,?)|(\+v8a,?)|(\+fpmr,?)?|(\+sve,?)|(\+neon,?)|(\+fp-armv8,?))*}}" }
 
 //@ [DISABLE_SVE] compile-flags: -C target-feature=-sve -Copt-level=0
-// DISABLE_SVE: attributes #0 = { {{.*}} "target-features"="{{((\+outline-atomics,?)|(\+v8a,?)|(\+fpmr,?)?|(-sve,?)|(\+neon,?)|(\+fp-armv8,?))*}}" }
+// DISABLE_SVE: attributes #0 = { {{.*}} "target-features"="{{((\+outline-atomics,?)|(\+v8a,?)|(\+fpmr,?)?|(-sve,?)|(\+neon,?))*}}" }
 
 //@ [DISABLE_NEON] compile-flags: -C target-feature=-neon -Copt-level=0
-// `neon` and `fp-armv8` get enabled as target base features, but then disabled again at the end of the list.
-// DISABLE_NEON: attributes #0 = { {{.*}} "target-features"="{{((\+outline-atomics,?)|(\+v8a,?)|(\+fp-armv8,?)|(\+neon,?))*}},-neon,-fp-armv8{{(,\+fpmr)?}}" }
+// DISABLE_NEON: attributes #0 = { {{.*}} "target-features"="{{((\+outline-atomics,?)|(\+v8a,?)|(\+fpmr,?)?|(-fp-armv8,?)|(-neon,?))*}}" }
 
 //@ [ENABLE_NEON] compile-flags: -C target-feature=+neon -Copt-level=0
 // ENABLE_NEON: attributes #0 = { {{.*}} "target-features"="{{((\+outline-atomics,?)|(\+v8a,?)|(\+fpmr,?)?|(\+fp-armv8,?)|(\+neon,?))*}}" }

diff --git a/tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable-implied.stderr b/tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable-implied.stderr
@@ -1,4 +1,4 @@
-warning: target feature `sse` cannot be disabled with `-Ctarget-feature`: this feature is required by the target ABI
+warning: target feature `sse2` must be enabled to ensure that the ABI of the current target can be implemented correctly
    |
    = 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>

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
@@ -1,4 +1,4 @@
-warning: target feature `neon` cannot be disabled with `-Ctarget-feature`: this feature is required by the target ABI
+warning: target feature `neon` must be enabled to ensure that the ABI of the current target can be implemented correctly
    |
    = 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>

diff --git a/tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable.stderr b/tests/ui/target-feature/forbidden-hardfloat-target-feature-flag-disable.stderr
@@ -1,11 +1,11 @@
-warning: unstable feature specified for `-Ctarget-feature`: `x87`
-   |
-   = note: this feature is not stably supported; its behavior can change in the future
-
-warning: target feature `x87` cannot be disabled with `-Ctarget-feature`: this feature is required by the target ABI
+warning: target feature `x87` must be enabled to ensure that the ABI of the current target can be implemented correctly
    |
    = 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: unstable feature specified for `-Ctarget-feature`: `x87`
+   |
+   = note: this feature is not stably supported; its behavior can change in the future
+
 warning: 2 warnings emitted
 
diff --git a/tests/ui/target-feature/forbidden-target-feature-flag.stderr b/tests/ui/target-feature/forbidden-target-feature-flag.stderr
@@ -1,7 +1,12 @@
+warning: target feature `soft-float` must be disabled to ensure that the ABI of the current target can be implemented correctly
+   |
+   = 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: target feature `soft-float` cannot be enabled with `-Ctarget-feature`: unsound 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
+warning: 2 warnings emitted
 
diff --git a/tests/ui/target-feature/target-cpu-lacks-required-target-feature.rs b/tests/ui/target-feature/target-cpu-lacks-required-target-feature.rs
@@ -0,0 +1,10 @@
+//@ compile-flags: --target=i686-unknown-linux-gnu --crate-type=lib
+//@ needs-llvm-components: x86
+//@ compile-flags: -Ctarget-cpu=pentium
+// For now this is just a warning.
+//@ build-pass
+#![feature(no_core, lang_items)]
+#![no_core]
+
+#[lang = "sized"]
+pub trait Sized {}
diff --git a/...ure/feature-hierarchy.aarch64-sve2.stderr → ...-cpu-lacks-required-target-feature.stderr b/...ure/feature-hierarchy.aarch64-sve2.stderr → ...-cpu-lacks-required-target-feature.stderr
@@ -1,4 +1,4 @@
-warning: target feature `neon` cannot be disabled with `-Ctarget-feature`: this feature is required by the target ABI
+warning: target feature `sse2` must be enabled to ensure that the ABI of the current target can be implemented correctly
    |
    = 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>