Rollup merge of #140920 - RalfJung:target-feature-unification, r=nnethercote,WaffleLapkin

Extract some shared code from codegen backend target feature handling

There's a bunch of code duplication between the GCC and LLVM backends in target feature handling. This moves that into new shared helper functions in `rustc_codegen_ssa`.

The first two commits should be purely refactoring. I am fairly sure the LLVM-side behavior stays the same; if the GCC side deliberately diverges from this then I may have missed that. I did account for one divergence, which I do not know is deliberate or not: GCC does not seem to use the `-Ctarget-feature` flag to populate `cfg(target_feature)`. That seems odd, since the `-Ctarget-feature` flag is used to populate the return value of `global_gcc_features` which controls the target features actually used by GCC. ``@GuillaumeGomez`` ``@antoyo`` is there a reason `target_config` ignores `-Ctarget-feature` but `global_gcc_features`  does not? The second commit also cleans up a bunch of unneeded complexity added in https://github.com/rust-lang/rust/pull/135927.

The third commit extracts some shared logic out of the functions that populate `cfg(target_feature)` and the backend target feature set, respectively. This one actually has some slight functional changes:
- Before, with `-Ctarget-feature=-feat`, if there is some other feature `x` that implies `feat` we would *not* add `-x` to the backend target feature set. Now, we do. This fixes rust-lang/rust#134792.
- The logic that removes `x` from `cfg(target_feature)` in this case also changed a bit, avoiding a large number of calls to the (uncached) `sess.target.implied_target_features` (if there were a large number of positive features listed before a negative feature) but instead constructing a full inverse implication map when encountering the first negative feature. Ideally this would be done with queries but the backend target feature logic runs before `tcx` so we can't use that...
- Previously, if feature "a" implied "b" and "b" was unstable, then using `-Ctarget-feature=+a` would also emit a warning about `b`. I had to remove this since when accounting for negative implications, this emits a ton of warnings in a bunch of existing tests... I assume this was unintentional anyway.

The fourth commit increases consistency of the GCC backend with the LLVM backend.

The last commit does some further cleanup:
- Get rid of RUSTC_SPECIAL_FEATURES. It was only needed for s390x "backchain", but since LLVM 19 that is always a regular target feature so we don't need this hack any more. The hack also has various unintended side-effects so we don't want to keep it. Fixes https://github.com/rust-lang/rust/issues/142412.
- Move RUSTC_SPECIFIC_FEATURES handling into the shared parse_rust_feature_flag helper so all consumers of `-Ctarget-feature` that only care about actual target features (and not "crt-static") have it. Previously, we actually set `cfg(target_feature = "crt-static")` twice: once in the backend target feature logic, and once specifically for that one feature. IIUC, some targets are meant to ignore `-Ctarget-feature=+crt-static`, it seems like before this PR that flag still incorrectly enabled `cfg(target_feature = "crt-static")` (but I didn't test this).
- Move fixed_x18 handling together with retpoline handling.
- Forbid setting fixed_x18 as a regular target feature, even unstably. It must be set via the `-Z` flag.

``@bjorn3`` I did not touch the cranelift backend here, since AFAIK it doesn't really support target features. But if you ever do, please use the new helpers. :)

Cc ``@workingjubilee``

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

@@ -6,7 +6,6 @@ use rustc_ast::{LitKind, MetaItem, MetaItemInner, attr};
 use rustc_attr_data_structures::{
     AttributeKind, InlineAttr, InstructionSetAttr, OptimizeAttr, find_attr,
 };
-use rustc_data_structures::fx::FxHashMap;
 use rustc_hir::def::DefKind;
 use rustc_hir::def_id::{DefId, LOCAL_CRATE, LocalDefId};
 use rustc_hir::weak_lang_items::WEAK_LANG_ITEMS;
@@ -18,13 +17,15 @@ use rustc_middle::mir::mono::Linkage;
 use rustc_middle::query::Providers;
 use rustc_middle::span_bug;
 use rustc_middle::ty::{self as ty, TyCtxt};
+use rustc_session::lint;
 use rustc_session::parse::feature_err;
-use rustc_session::{Session, lint};
 use rustc_span::{Ident, Span, sym};
 use rustc_target::spec::SanitizerSet;
 
 use crate::errors;
-use crate::target_features::{check_target_feature_trait_unsafe, from_target_feature_attr};
+use crate::target_features::{
+    check_target_feature_trait_unsafe, check_tied_features, from_target_feature_attr,
+};
 
 fn linkage_by_name(tcx: TyCtxt<'_>, def_id: LocalDefId, name: &str) -> Linkage {
     use rustc_middle::mir::mono::Linkage::*;
@@ -590,25 +591,6 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
     codegen_fn_attrs
 }
 
-/// Given a map from target_features to whether they are enabled or disabled, ensure only valid
-/// combinations are allowed.
-pub fn check_tied_features(
-    sess: &Session,
-    features: &FxHashMap<&str, bool>,
-) -> Option<&'static [&'static str]> {
-    if !features.is_empty() {
-        for tied in sess.target.tied_target_features() {
-            // Tied features must be set to the same value, or not set at all
-            let mut tied_iter = tied.iter();
-            let enabled = features.get(tied_iter.next().unwrap());
-            if tied_iter.any(|f| enabled != features.get(f)) {
-                return Some(tied);
-            }
-        }
-    }
-    None
-}
-
 /// Checks if the provided DefId is a method in a trait impl for a trait which has track_caller
 /// applied to the method prototype.
 fn should_inherit_track_caller(tcx: TyCtxt<'_>, def_id: DefId) -> bool {

compiler/rustc_codegen_ssa/src/errors.rs

@@ -1203,30 +1203,6 @@ pub(crate) struct ErrorCreatingImportLibrary<'a> {
     pub error: String,
 }
 
-pub struct TargetFeatureDisableOrEnable<'a> {
-    pub features: &'a [&'a str],
-    pub span: Option<Span>,
-    pub missing_features: Option<MissingFeatures>,
-}
-
-#[derive(Subdiagnostic)]
-#[help(codegen_ssa_missing_features)]
-pub struct MissingFeatures;
-
-impl<G: EmissionGuarantee> Diagnostic<'_, G> for TargetFeatureDisableOrEnable<'_> {
-    fn into_diag(self, dcx: DiagCtxtHandle<'_>, level: Level) -> Diag<'_, G> {
-        let mut diag = Diag::new(dcx, level, fluent::codegen_ssa_target_feature_disable_or_enable);
-        if let Some(span) = self.span {
-            diag.span(span);
-        };
-        if let Some(missing_features) = self.missing_features {
-            diag.subdiagnostic(missing_features);
-        }
-        diag.arg("features", self.features.join(", "));
-        diag
-    }
-}
-
 #[derive(Diagnostic)]
 #[diag(codegen_ssa_aix_strip_not_used)]
 pub(crate) struct AixStripNotUsed;
@@ -1269,3 +1245,68 @@ pub(crate) struct XcrunSdkPathWarning {
 #[derive(LintDiagnostic)]
 #[diag(codegen_ssa_aarch64_softfloat_neon)]
 pub(crate) struct Aarch64SoftfloatNeon;
+
+#[derive(Diagnostic)]
+#[diag(codegen_ssa_unknown_ctarget_feature_prefix)]
+#[note]
+pub(crate) struct UnknownCTargetFeaturePrefix<'a> {
+    pub feature: &'a str,
+}
+
+#[derive(Subdiagnostic)]
+pub(crate) enum PossibleFeature<'a> {
+    #[help(codegen_ssa_possible_feature)]
+    Some { rust_feature: &'a str },
+    #[help(codegen_ssa_consider_filing_feature_request)]
+    None,
+}
+
+#[derive(Diagnostic)]
+#[diag(codegen_ssa_unknown_ctarget_feature)]
+#[note]
+pub(crate) struct UnknownCTargetFeature<'a> {
+    pub feature: &'a str,
+    #[subdiagnostic]
+    pub rust_feature: PossibleFeature<'a>,
+}
+
+#[derive(Diagnostic)]
+#[diag(codegen_ssa_unstable_ctarget_feature)]
+#[note]
+pub(crate) struct UnstableCTargetFeature<'a> {
+    pub feature: &'a str,
+}
+
+#[derive(Diagnostic)]
+#[diag(codegen_ssa_forbidden_ctarget_feature)]
+#[note]
+#[note(codegen_ssa_forbidden_ctarget_feature_issue)]
+pub(crate) struct ForbiddenCTargetFeature<'a> {
+    pub feature: &'a str,
+    pub enabled: &'a str,
+    pub reason: &'a str,
+}
+
+pub struct TargetFeatureDisableOrEnable<'a> {
+    pub features: &'a [&'a str],
+    pub span: Option<Span>,
+    pub missing_features: Option<MissingFeatures>,
+}
+
+#[derive(Subdiagnostic)]
+#[help(codegen_ssa_missing_features)]
+pub struct MissingFeatures;
+
+impl<G: EmissionGuarantee> Diagnostic<'_, G> for TargetFeatureDisableOrEnable<'_> {
+    fn into_diag(self, dcx: DiagCtxtHandle<'_>, level: Level) -> Diag<'_, G> {
+        let mut diag = Diag::new(dcx, level, fluent::codegen_ssa_target_feature_disable_or_enable);
+        if let Some(span) = self.span {
+            diag.span(span);
+        };
+        if let Some(missing_features) = self.missing_features {
+            diag.subdiagnostic(missing_features);
+        }
+        diag.arg("features", self.features.join(", "));
+        diag
+    }
+}

compiler/rustc_codegen_ssa/src/target_features.rs

@@ -1,5 +1,5 @@
 use rustc_attr_data_structures::InstructionSetAttr;
-use rustc_data_structures::fx::FxIndexSet;
+use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet};
 use rustc_data_structures::unord::{UnordMap, UnordSet};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
@@ -8,11 +8,12 @@ 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::features::StabilityExt;
+use rustc_session::Session;
 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};
+use rustc_target::target_features::{self, RUSTC_SPECIFIC_FEATURES, Stability};
+use smallvec::SmallVec;
 
 use crate::errors;
 
@@ -67,7 +68,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.is_toggle_permitted(tcx.sess) {
+            if let Err(reason) = stability.toggle_allowed() {
                 tcx.dcx().emit_err(errors::ForbiddenTargetFeatureAttr {
                     span: item.span(),
                     feature,
@@ -88,7 +89,7 @@ pub(crate) fn from_target_feature_attr(
                 let feature_sym = Symbol::intern(feature);
                 for &name in tcx.implied_target_features(feature_sym) {
                     // But ensure the ABI does not forbid enabling this.
-                    // Here we do assume that LLVM doesn't add even more implied features
+                    // Here we do assume that the backend doesn't add even more implied features
                     // we don't know about, at least no features that would have ABI effects!
                     // We skip this logic in rustdoc, where we want to allow all target features of
                     // all targets, so we can't check their ABI compatibility and anyway we are not
@@ -156,6 +157,276 @@ pub(crate) fn check_target_feature_trait_unsafe(tcx: TyCtxt<'_>, id: LocalDefId,
     }
 }
 
+/// Parse the value of `-Ctarget-feature`, also expanding implied features,
+/// and call the closure for each (expanded) Rust feature. If the list contains
+/// a syntactically invalid item (not starting with `+`/`-`), the error callback is invoked.
+fn parse_rust_feature_flag<'a>(
+    sess: &'a Session,
+    err_callback: impl Fn(&'a str),
+    mut callback: impl FnMut(
+        /* base_feature */ &'a str,
+        /* with_implied */ FxHashSet<&'a str>,
+        /* enable */ bool,
+    ),
+) {
+    // A cache for the backwards implication map.
+    let mut inverse_implied_features: Option<FxHashMap<&str, FxHashSet<&str>>> = None;
+
+    for feature in sess.opts.cg.target_feature.split(',') {
+        if let Some(base_feature) = feature.strip_prefix('+') {
+            // Skip features that are not target features, but rustc features.
+            if RUSTC_SPECIFIC_FEATURES.contains(&base_feature) {
+                return;
+            }
+
+            callback(base_feature, sess.target.implied_target_features(base_feature), true)
+        } else if let Some(base_feature) = feature.strip_prefix('-') {
+            // Skip features that are not target features, but rustc features.
+            if RUSTC_SPECIFIC_FEATURES.contains(&base_feature) {
+                return;
+            }
+
+            // If `f1` implies `f2`, then `!f2` implies `!f1` -- this is standard logical
+            // contraposition. So we have to find all the reverse implications of `base_feature` and
+            // disable them, too.
+
+            let inverse_implied_features = inverse_implied_features.get_or_insert_with(|| {
+                let mut set: FxHashMap<&str, FxHashSet<&str>> = FxHashMap::default();
+                for (f, _, is) in sess.target.rust_target_features() {
+                    for i in is.iter() {
+                        set.entry(i).or_default().insert(f);
+                    }
+                }
+                set
+            });
+
+            // Inverse implied target features have their own inverse implied target features, so we
+            // traverse the map until there are no more features to add.
+            let mut features = FxHashSet::default();
+            let mut new_features = vec![base_feature];
+            while let Some(new_feature) = new_features.pop() {
+                if features.insert(new_feature) {
+                    if let Some(implied_features) = inverse_implied_features.get(&new_feature) {
+                        new_features.extend(implied_features)
+                    }
+                }
+            }
+
+            callback(base_feature, features, false)
+        } else if !feature.is_empty() {
+            err_callback(feature)
+        }
+    }
+}
+
+/// Utility function for a codegen backend to compute `cfg(target_feature)`, or more specifically,
+/// to populate `sess.unstable_target_features` and `sess.target_features` (these are the first and
+/// 2nd component of the return value, respectively).
+///
+/// `target_base_has_feature` should check whether the given feature (a Rust feature name!) is
+/// enabled in the "base" target machine, i.e., without applying `-Ctarget-feature`.
+///
+/// We do not have to worry about RUSTC_SPECIFIC_FEATURES here, those are handled elsewhere.
+pub fn cfg_target_feature(
+    sess: &Session,
+    mut target_base_has_feature: impl FnMut(&str) -> bool,
+) -> (Vec<Symbol>, Vec<Symbol>) {
+    // Compute which of the known target features are enabled in the 'base' target machine. We only
+    // consider "supported" features; "forbidden" features are not reflected in `cfg` as of now.
+    let mut features: UnordSet<Symbol> = sess
+        .target
+        .rust_target_features()
+        .iter()
+        .filter(|(feature, _, _)| target_base_has_feature(feature))
+        .map(|(feature, _, _)| Symbol::intern(feature))
+        .collect();
+
+    // Add enabled and remove disabled features.
+    parse_rust_feature_flag(
+        sess,
+        /* err_callback */
+        |_| {
+            // Errors are already emitted in `flag_to_backend_features`; avoid duplicates.
+        },
+        |_base_feature, new_features, enabled| {
+            // Iteration order is irrelevant since this only influences an `UnordSet`.
+            #[allow(rustc::potential_query_instability)]
+            if enabled {
+                features.extend(new_features.into_iter().map(|f| Symbol::intern(f)));
+            } else {
+                // Remove `new_features` from `features`.
+                for new in new_features {
+                    features.remove(&Symbol::intern(new));
+                }
+            }
+        },
+    );
+
+    // Filter enabled features based on feature gates.
+    let f = |allow_unstable| {
+        sess.target
+            .rust_target_features()
+            .iter()
+            .filter_map(|(feature, gate, _)| {
+                // The `allow_unstable` set is used by rustc internally to determine 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(Symbol::intern(feature))
+                } else {
+                    None
+                }
+            })
+            .filter(|feature| features.contains(&feature))
+            .collect()
+    };
+
+    (f(true), f(false))
+}
+
+/// Given a map from target_features to whether they are enabled or disabled, ensure only valid
+/// combinations are allowed.
+pub fn check_tied_features(
+    sess: &Session,
+    features: &FxHashMap<&str, bool>,
+) -> Option<&'static [&'static str]> {
+    if !features.is_empty() {
+        for tied in sess.target.tied_target_features() {
+            // Tied features must be set to the same value, or not set at all
+            let mut tied_iter = tied.iter();
+            let enabled = features.get(tied_iter.next().unwrap());
+            if tied_iter.any(|f| enabled != features.get(f)) {
+                return Some(tied);
+            }
+        }
+    }
+    None
+}
+
+/// Translates the `-Ctarget-feature` flag into a backend target feature list.
+///
+/// `to_backend_features` converts a Rust feature name into a list of backend feature names; this is
+/// used for diagnostic purposes only.
+///
+/// `extend_backend_features` extends the set of backend features (assumed to be in mutable state
+/// accessible by that closure) to enable/disable the given Rust feature name.
+pub fn flag_to_backend_features<'a, const N: usize>(
+    sess: &'a Session,
+    diagnostics: bool,
+    to_backend_features: impl Fn(&'a str) -> SmallVec<[&'a str; N]>,
+    mut extend_backend_features: impl FnMut(&'a str, /* enable */ bool),
+) {
+    let known_features = sess.target.rust_target_features();
+
+    // Compute implied features
+    let mut rust_features = vec![];
+    parse_rust_feature_flag(
+        sess,
+        /* err_callback */
+        |feature| {
+            if diagnostics {
+                sess.dcx().emit_warn(errors::UnknownCTargetFeaturePrefix { feature });
+            }
+        },
+        |base_feature, new_features, enable| {
+            rust_features.extend(
+                UnordSet::from(new_features).to_sorted_stable_ord().iter().map(|&&s| (enable, s)),
+            );
+            // Check feature validity.
+            if diagnostics {
+                let feature_state = known_features.iter().find(|&&(v, _, _)| v == base_feature);
+                match feature_state {
+                    None => {
+                        // This is definitely not a valid Rust feature name. Maybe it is a backend
+                        // feature name? If so, give a better error message.
+                        let rust_feature =
+                            known_features.iter().find_map(|&(rust_feature, _, _)| {
+                                let backend_features = to_backend_features(rust_feature);
+                                if backend_features.contains(&base_feature)
+                                    && !backend_features.contains(&rust_feature)
+                                {
+                                    Some(rust_feature)
+                                } else {
+                                    None
+                                }
+                            });
+                        let unknown_feature = if let Some(rust_feature) = rust_feature {
+                            errors::UnknownCTargetFeature {
+                                feature: base_feature,
+                                rust_feature: errors::PossibleFeature::Some { rust_feature },
+                            }
+                        } else {
+                            errors::UnknownCTargetFeature {
+                                feature: base_feature,
+                                rust_feature: errors::PossibleFeature::None,
+                            }
+                        };
+                        sess.dcx().emit_warn(unknown_feature);
+                    }
+                    Some((_, stability, _)) => {
+                        if let Err(reason) = stability.toggle_allowed() {
+                            sess.dcx().emit_warn(errors::ForbiddenCTargetFeature {
+                                feature: base_feature,
+                                enabled: if enable { "enabled" } else { "disabled" },
+                                reason,
+                            });
+                        } else if stability.requires_nightly().is_some() {
+                            // An unstable feature. Warn about using it. It makes little sense
+                            // to hard-error here since we just warn about fully unknown
+                            // features above.
+                            sess.dcx().emit_warn(errors::UnstableCTargetFeature {
+                                feature: base_feature,
+                            });
+                        }
+                    }
+                }
+            }
+        },
+    );
+
+    if diagnostics {
+        // FIXME(nagisa): figure out how to not allocate a full hashmap here.
+        if let Some(f) = check_tied_features(
+            sess,
+            &FxHashMap::from_iter(rust_features.iter().map(|&(enable, feature)| (feature, enable))),
+        ) {
+            sess.dcx().emit_err(errors::TargetFeatureDisableOrEnable {
+                features: f,
+                span: None,
+                missing_features: None,
+            });
+        }
+    }
+
+    // Add this to the backend features.
+    for (enable, feature) in rust_features {
+        extend_backend_features(feature, enable);
+    }
+}
+
+/// Computes the backend target features to be added to account for retpoline flags.
+/// Used by both LLVM and GCC since their target features are, conveniently, the same.
+pub fn retpoline_features_by_flags(sess: &Session, features: &mut Vec<String>) {
+    // -Zretpoline without -Zretpoline-external-thunk enables
+    // retpoline-indirect-branches and retpoline-indirect-calls target features
+    let unstable_opts = &sess.opts.unstable_opts;
+    if unstable_opts.retpoline && !unstable_opts.retpoline_external_thunk {
+        features.push("+retpoline-indirect-branches".into());
+        features.push("+retpoline-indirect-calls".into());
+    }
+    // -Zretpoline-external-thunk (maybe, with -Zretpoline too) enables
+    // retpoline-external-thunk, retpoline-indirect-branches and
+    // retpoline-indirect-calls target features
+    if unstable_opts.retpoline_external_thunk {
+        features.push("+retpoline-external-thunk".into());
+        features.push("+retpoline-indirect-branches".into());
+        features.push("+retpoline-indirect-calls".into());
+    }
+}
+
 pub(crate) fn provide(providers: &mut Providers) {
     *providers = Providers {
         rust_target_features: |tcx, cnum| {
@@ -182,7 +453,8 @@ pub(crate) fn provide(providers: &mut Providers) {
                                     Stability::Unstable { .. } | Stability::Forbidden { .. },
                                 )
                                 | (Stability::Forbidden { .. }, Stability::Forbidden { .. }) => {
-                                    // The stability in the entry is at least as good as the new one, just keep it.
+                                    // The stability in the entry is at least as good as the new
+                                    // one, just keep it.
                                 }
                                 _ => {
                                     // Overwrite stabilite.