Rollup merge of rust-lang#136147 - RalfJung:required-target-features-check-not-add, r=workingjubilee

ABI-required target features: warn when they are missing in base CPU

Part of rust-lang#135408:
instead of adding ABI-required features to the target we build for LLVM, check that they are already there. Crucially we check this after applying `-Ctarget-cpu` and `-Ctarget-feature`, by reading `sess.unstable_target_features`. This means we can tweak the ABI target feature check without changing the behavior for any existing user; they will get warnings but the target features behave as before.

The test changes here show that we are un-doing the "add all required target features" part. Without the full rust-lang#135408, there is no way to take a way an ABI-required target feature with `-Ctarget-cpu`, so we cannot yet test that part.

Cc ``@workingjubilee``

Author: fmease

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(',') {
@@ -117,51 +109,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().flat_map(|&(enable, feature)| {

compiler/rustc_codegen_gcc/src/lib.rs

@@ -493,9 +493,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

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()

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}
 

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,
+}

compiler/rustc_interface/src/interface.rs

@@ -492,6 +492,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,

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,34 @@ 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, taking into account `-Ctarget-cpu` and `-Ctarget-feature`.
+    // Just double-check that the features we care about are actually on our list.
+    for feature in
+        abi_feature_constraints.required.iter().chain(abi_feature_constraints.incompatible.iter())
+    {
+        assert!(
+            sess.target.rust_target_features().iter().any(|(name, ..)| feature == name),
+            "target feature {feature} is required/incompatible for the current ABI but not a recognized feature for this target"
+        );
+    }
+
+    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;
 

compiler/rustc_target/src/target_features.rs

@@ -108,21 +108,19 @@ impl Stability {
 // per-function level, since we would then allow safe calls from functions with `+soft-float` to
 // functions without that feature!
 //
-// It is important for soundness that features allowed here do *not* change the function call ABI.
-// For example, disabling the `x87` feature on x86 changes how scalar floats are passed as
-// arguments, so enabling toggling that feature would be unsound. In fact, since `-Ctarget-feature`
-// will just allow unknown features (with a warning), we have to explicitly list features that change
-// the ABI as `Forbidden` to ensure using them causes an error. Note that this is only effective if
-// such features can never be toggled via `-Ctarget-cpu`! If that is ever a possibility, we will need
-// extra checks ensuring that the LLVM-computed target features for a CPU did not (un)set a
-// `Forbidden` feature. See https://github.com/rust-lang/rust/issues/116344 for some more context.
-// FIXME: add such "forbidden" features for non-x86 targets.
+// It is important for soundness to consider the interaction of targets features and the function
+// call ABI. For example, disabling the `x87` feature on x86 changes how scalar floats are passed as
+// arguments, so letting people toggle that feature would be unsound. To this end, the
+// `abi_required_features` function computes which target features must and must not be enabled for
+// any given target, and individual features can also be marked as `Forbidden`.
+// See https://github.com/rust-lang/rust/issues/116344 for some more context.
 //
 // The one exception to features that change the ABI is features that enable larger vector
-// registers. Those are permitted to be listed here. This is currently unsound (see
-// https://github.com/rust-lang/rust/issues/116558); in the future we will have to ensure that
-// functions can only use such vectors as arguments/return types if the corresponding target feature
-// is enabled.
+// registers. Those are permitted to be listed here. The `*_FOR_CORRECT_VECTOR_ABI` arrays store
+// information about which target feature is ABI-required for which vector size; this is used to
+// ensure that vectors can only be passed via `extern "C"` when the right feature is enabled. (For
+// the "Rust" ABI we generally pass vectors by-ref exactly to avoid these issues.)
+// Also see https://github.com/rust-lang/rust/issues/116558.
 //
 // Stabilizing a target feature requires t-lang approval.
 

src/tools/miri/tests/pass/shims/x86/intrinsics-x86-pause-without-sse2.rs

@@ -1,5 +1,5 @@
-// We're testing x86 target specific features
-//@only-target: x86_64 i686
+// We're testing x86-32 target specific features. SSE always exists on x86-64.
+//@only-target: i686
 //@compile-flags: -C target-feature=-sse2
 
 #[cfg(target_arch = "x86")]

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"