Direct users towards using Rust feature names in CLI

If they are trying to use features rustc doesn't yet know about,
request a feature request.

Additionally, also warn against using feature names without leading `+`
or `-` signs.

Author: nagisa

compiler/rustc_codegen_llvm/src/attributes.rs

@@ -382,10 +382,7 @@ pub fn from_fn_attrs<'ll, 'tcx>(
     let mut function_features = function_features
         .iter()
         .flat_map(|feat| {
-            llvm_util::to_llvm_feature(cx.tcx.sess, feat)
-                .into_iter()
-                .map(|f| format!("+{}", f))
-                .collect::<Vec<String>>()
+            llvm_util::to_llvm_features(cx.tcx.sess, feat).into_iter().map(|f| format!("+{}", f))
         })
         .chain(codegen_fn_attrs.instruction_set.iter().map(|x| match x {
             InstructionSetAttr::ArmA32 => "-thumb-mode".to_string(),

compiler/rustc_codegen_llvm/src/llvm_util.rs

@@ -2,14 +2,18 @@ use crate::back::write::create_informational_target_machine;
 use crate::{llvm, llvm_util};
 use libc::c_int;
 use libloading::Library;
-use rustc_codegen_ssa::target_features::{supported_target_features, tied_target_features};
+use rustc_codegen_ssa::target_features::{
+    supported_target_features, tied_target_features, RUSTC_SPECIFIC_FEATURES,
+};
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
+use rustc_data_structures::small_c_str::SmallCStr;
 use rustc_fs_util::path_to_c_string;
 use rustc_middle::bug;
 use rustc_session::config::PrintRequest;
 use rustc_session::Session;
 use rustc_span::symbol::Symbol;
 use rustc_target::spec::{MergeFunctions, PanicStrategy};
+use smallvec::{smallvec, SmallVec};
 use std::ffi::{CStr, CString};
 use tracing::debug;
 
@@ -155,45 +159,46 @@ pub fn time_trace_profiler_finish(file_name: &Path) {
     }
 }
 
-// WARNING: the features after applying `to_llvm_feature` must be known
+// WARNING: the features after applying `to_llvm_features` must be known
 // to LLVM or the feature detection code will walk past the end of the feature
 // array, leading to crashes.
+//
 // To find a list of LLVM's names, check llvm-project/llvm/include/llvm/Support/*TargetParser.def
 // where the * matches the architecture's name
 // Beware to not use the llvm github project for this, but check the git submodule
 // found in src/llvm-project
 // Though note that Rust can also be build with an external precompiled version of LLVM
 // which might lead to failures if the oldest tested / supported LLVM version
 // doesn't yet support the relevant intrinsics
-pub fn to_llvm_feature<'a>(sess: &Session, s: &'a str) -> Vec<&'a str> {
+pub fn to_llvm_features<'a>(sess: &Session, s: &'a str) -> SmallVec<[&'a str; 2]> {
     let arch = if sess.target.arch == "x86_64" { "x86" } else { &*sess.target.arch };
     match (arch, s) {
         ("x86", "sse4.2") => {
             if get_version() >= (14, 0, 0) {
-                vec!["sse4.2", "crc32"]
+                smallvec!["sse4.2", "crc32"]
             } else {
-                vec!["sse4.2"]
+                smallvec!["sse4.2"]
             }
         }
-        ("x86", "pclmulqdq") => vec!["pclmul"],
-        ("x86", "rdrand") => vec!["rdrnd"],
-        ("x86", "bmi1") => vec!["bmi"],
-        ("x86", "cmpxchg16b") => vec!["cx16"],
-        ("x86", "avx512vaes") => vec!["vaes"],
-        ("x86", "avx512gfni") => vec!["gfni"],
-        ("x86", "avx512vpclmulqdq") => vec!["vpclmulqdq"],
-        ("aarch64", "fp") => vec!["fp-armv8"],
-        ("aarch64", "fp16") => vec!["fullfp16"],
-        ("aarch64", "fhm") => vec!["fp16fml"],
-        ("aarch64", "rcpc2") => vec!["rcpc-immo"],
-        ("aarch64", "dpb") => vec!["ccpp"],
-        ("aarch64", "dpb2") => vec!["ccdp"],
-        ("aarch64", "frintts") => vec!["fptoint"],
-        ("aarch64", "fcma") => vec!["complxnum"],
-        ("aarch64", "pmuv3") => vec!["perfmon"],
-        ("aarch64", "paca") => vec!["pauth"],
-        ("aarch64", "pacg") => vec!["pauth"],
-        (_, s) => vec![s],
+        ("x86", "pclmulqdq") => smallvec!["pclmul"],
+        ("x86", "rdrand") => smallvec!["rdrnd"],
+        ("x86", "bmi1") => smallvec!["bmi"],
+        ("x86", "cmpxchg16b") => smallvec!["cx16"],
+        ("x86", "avx512vaes") => smallvec!["vaes"],
+        ("x86", "avx512gfni") => smallvec!["gfni"],
+        ("x86", "avx512vpclmulqdq") => smallvec!["vpclmulqdq"],
+        ("aarch64", "fp") => smallvec!["fp-armv8"],
+        ("aarch64", "fp16") => smallvec!["fullfp16"],
+        ("aarch64", "fhm") => smallvec!["fp16fml"],
+        ("aarch64", "rcpc2") => smallvec!["rcpc-immo"],
+        ("aarch64", "dpb") => smallvec!["ccpp"],
+        ("aarch64", "dpb2") => smallvec!["ccdp"],
+        ("aarch64", "frintts") => smallvec!["fptoint"],
+        ("aarch64", "fcma") => smallvec!["complxnum"],
+        ("aarch64", "pmuv3") => smallvec!["perfmon"],
+        ("aarch64", "paca") => smallvec!["pauth"],
+        ("aarch64", "pacg") => smallvec!["pauth"],
+        (_, s) => smallvec![s],
     }
 }
 
@@ -207,7 +212,6 @@ pub fn check_tied_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);
         }
@@ -221,15 +225,11 @@ pub fn target_features(sess: &Session) -> Vec<Symbol> {
         supported_target_features(sess)
             .iter()
             .filter_map(|&(feature, gate)| {
-                if sess.is_nightly_build() || gate.is_none() {
-                    Some(feature)
-                } else {
-                    None
-                }
+                if sess.is_nightly_build() || gate.is_none() { Some(feature) } else { None }
             })
             .filter(|feature| {
-                for llvm_feature in to_llvm_feature(sess, feature) {
-                    let cstr = CString::new(llvm_feature).unwrap();
+                for llvm_feature in to_llvm_features(sess, feature) {
+                    let cstr = SmallCStr::new(llvm_feature);
                     if unsafe { llvm::LLVMRustHasFeature(target_machine, cstr.as_ptr()) } {
                         return true;
                     }
@@ -302,9 +302,9 @@ fn print_target_features(sess: &Session, tm: &llvm::TargetMachine) {
     let mut rustc_target_features = supported_target_features(sess)
         .iter()
         .filter_map(|(feature, _gate)| {
-            for llvm_feature in to_llvm_feature(sess, *feature) {
+            for llvm_feature in to_llvm_features(sess, *feature) {
                 // LLVM asserts that these are sorted. LLVM and Rust both use byte comparison for these strings.
-                match target_features.binary_search_by_key(&llvm_feature, |(f, _d)| (*f)).ok().map(
+                match target_features.binary_search_by_key(&llvm_feature, |(f, _d)| f).ok().map(
                     |index| {
                         let (_f, desc) = target_features.remove(index);
                         (*feature, desc)
@@ -374,14 +374,7 @@ pub fn target_cpu(sess: &Session) -> &str {
 
 /// The list of LLVM features computed from CLI flags (`-Ctarget-cpu`, `-Ctarget-feature`,
 /// `--target` and similar).
-// FIXME(nagisa): Cache the output of this somehow? Maybe make this a query? We're calling this
-// for every function that has `#[target_feature]` on it. The global features won't change between
-// the functions; only crates, maybe…
-pub fn llvm_global_features(sess: &Session) -> Vec<String> {
-    // FIXME(nagisa): this should definitely be available more centrally and to other codegen backends.
-    /// These features control behaviour of rustc rather than llvm.
-    const RUSTC_SPECIFIC_FEATURES: &[&str] = &["crt-static"];
-
+pub(crate) fn global_llvm_features(sess: &Session, diagnostics: bool) -> Vec<String> {
     // Features that come earlier are overriden by conflicting features later in the string.
     // Typically we'll want more explicit settings to override the implicit ones, so:
     //
@@ -427,42 +420,108 @@ pub fn llvm_global_features(sess: &Session) -> Vec<String> {
         Some(_) | None => {}
     };
 
-    fn strip(s: &str) -> &str {
-        s.strip_prefix(&['+', '-']).unwrap_or(s)
+    // Features implied by an implicit or explicit `--target`.
+    features.extend(
+        sess.target
+            .features
+            .split(',')
+            .filter(|v| !v.is_empty() && backend_feature_name(v).is_some())
+            .map(String::from),
+    );
+
+    // -Ctarget-features
+    let supported_features = supported_target_features(sess);
+    let feats = sess
+        .opts
+        .cg
+        .target_feature
+        .split(',')
+        .filter_map(|s| {
+            let enable_disable = match s.chars().next() {
+                None => return None,
+                Some(c @ '+' | c @ '-') => c,
+                Some(_) => {
+                    if diagnostics {
+                        let mut diag = sess.struct_warn(&format!(
+                            "unknown feature specified for `-Ctarget-feature`: `{}`",
+                            s
+                        ));
+                        diag.note("features must begin with a `+` to enable or `-` to disable it");
+                        diag.emit();
+                    }
+                    return None;
+                }
+            };
+
+            let feature = backend_feature_name(s)?;
+            // Warn against use of LLVM specific feature names on the CLI.
+            if diagnostics && !supported_features.iter().any(|&(v, _)| v == feature) {
+                let rust_feature = supported_features.iter().find_map(|&(rust_feature, _)| {
+                    let llvm_features = to_llvm_features(sess, rust_feature);
+                    if llvm_features.contains(&feature) && !llvm_features.contains(&rust_feature) {
+                        Some(rust_feature)
+                    } else {
+                        None
+                    }
+                });
+                let mut diag = sess.struct_warn(&format!(
+                    "unknown feature specified for `-Ctarget-feature`: `{}`",
+                    feature
+                ));
+                diag.note("it is still passed through to the codegen backend");
+                if let Some(rust_feature) = rust_feature {
+                    diag.help(&format!("you might have meant: `{}`", rust_feature));
+                } else {
+                    diag.note("consider filing a feature request");
+                }
+                diag.emit();
+            }
+            Some((enable_disable, feature))
+        })
+        .collect::<SmallVec<[(char, &str); 8]>>();
+
+    if diagnostics {
+        // FIXME(nagisa): figure out how to not allocate a full hashset here.
+        let featmap = feats.iter().map(|&(flag, feat)| (feat, flag == '+')).collect();
+        if let Some(f) = check_tied_features(sess, &featmap) {
+            sess.err(&format!(
+                "target features {} must all be enabled or disabled together",
+                f.join(", ")
+            ));
+        }
     }
 
-    let filter = |s: &str| {
-        // features must start with a `+` or `-`.
-        let feature = match s.strip_prefix(&['+', '-'][..]) {
-            None => return vec![],
-            // Rustc-specific feature requests like `+crt-static` or `-crt-static`
-            // are not passed down to LLVM.
-            Some(feature) if RUSTC_SPECIFIC_FEATURES.contains(&feature) => return vec![],
-            Some(feature) => feature,
-        };
-        // ... otherwise though we run through `to_llvm_feature` when
+    features.extend(feats.into_iter().flat_map(|(enable_disable, feature)| {
+        // rustc-specific features do not get passed down to LLVM…
+        if RUSTC_SPECIFIC_FEATURES.contains(&feature) {
+            return SmallVec::<[_; 2]>::new();
+        }
+        // ... otherwise though we run through `to_llvm_feature when
         // passing requests down to LLVM. This means that all in-language
         // features also work on the command line instead of having two
         // different names when the LLVM name and the Rust name differ.
-        to_llvm_feature(sess, feature).iter().map(|f| format!("{}{}", &s[..1], f)).collect()
-    };
-
-    // Features implied by an implicit or explicit `--target`.
-    features.extend(sess.target.features.split(',').flat_map(&filter));
+        to_llvm_features(sess, feature)
+            .into_iter()
+            .map(|f| format!("{}{}", enable_disable, f))
+            .collect()
+    }));
+    features
+}
 
-    // -Ctarget-features
-    let feats: Vec<&str> = sess.opts.cg.target_feature.split(',').collect();
-    // LLVM enables based on the last occurence of a feature
-    if let Some(f) =
-        check_tied_features(sess, &feats.iter().map(|f| (strip(f), !f.starts_with("-"))).collect())
-    {
-        sess.err(&format!(
-            "target features {} must all be enabled or disabled together",
-            f.join(", ")
-        ));
+/// Returns a feature name for the given `+feature` or `-feature` string.
+///
+/// Only allows features that are backend specific (i.e. not [`RUSTC_SPECIFIC_FEATURES`].)
+fn backend_feature_name(s: &str) -> Option<&str> {
+    // features must start with a `+` or `-`.
+    let feature = s.strip_prefix(&['+', '-'][..]).unwrap_or_else(|| {
+        bug!("target feature `{}` must begin with a `+` or `-`", s);
+    });
+    // Rustc-specific feature requests like `+crt-static` or `-crt-static`
+    // are not passed down to LLVM.
+    if RUSTC_SPECIFIC_FEATURES.contains(&feature) {
+        return None;
     }
-    features.extend(feats.iter().flat_map(&filter));
-    features
+    Some(feature)
 }
 
 pub fn tune_cpu(sess: &Session) -> Option<&str> {

compiler/rustc_codegen_ssa/src/target_features.rs

@@ -4,6 +4,9 @@ use rustc_session::Session;
 use rustc_span::symbol::sym;
 use rustc_span::symbol::Symbol;
 
+/// Features that control behaviour of rustc, rather than the codegen.
+pub const RUSTC_SPECIFIC_FEATURES: &[&str] = &["crt-static"];
+
 // When adding features to the below lists
 // check whether they're named already elsewhere in rust
 // e.g. in stdarch and whether the given name matches LLVM's

src/test/ui/target-feature/missing-plusminus.rs

@@ -0,0 +1,2 @@
+// compile-flags: -Ctarget-feature=banana --crate-type=rlib
+// build-pass

src/test/ui/target-feature/missing-plusminus.stderr

@@ -0,0 +1,18 @@
+warning: unknown feature specified for `-Ctarget-feature`: `banana`
+   |
+   = note: features must begin with a `+` to enable or `-` to disable it
+
+warning: unknown feature specified for `-Ctarget-feature`: `banana`
+   |
+   = note: features must begin with a `+` to enable or `-` to disable it
+
+warning: unknown feature specified for `-Ctarget-feature`: `banana`
+   |
+   = note: features must begin with a `+` to enable or `-` to disable it
+
+warning: unknown feature specified for `-Ctarget-feature`: `banana`
+   |
+   = note: features must begin with a `+` to enable or `-` to disable it
+
+warning: 4 warnings emitted
+

src/test/ui/target-feature/similar-feature-suggestion.rs

@@ -0,0 +1,6 @@
+// compile-flags: -Ctarget-feature=+rdrnd --crate-type=rlib --target=x86_64-unknown-linux-gnu
+// build-pass
+// needs-llvm-components: x86
+
+#![feature(no_core)]
+#![no_core]

src/test/ui/target-feature/similar-feature-suggestion.stderr

@@ -0,0 +1,22 @@
+warning: unknown feature specified for `-Ctarget-feature`: `rdrnd`
+   |
+   = note: it is still passed through to the codegen backend
+   = help: you might have meant: `rdrand`
+
+warning: unknown feature specified for `-Ctarget-feature`: `rdrnd`
+   |
+   = note: it is still passed through to the codegen backend
+   = help: did you mean: `rdrand`
+
+warning: unknown feature specified for `-Ctarget-feature`: `rdrnd`
+   |
+   = note: it is still passed through to the codegen backend
+   = help: did you mean: `rdrand`
+
+warning: unknown feature specified for `-Ctarget-feature`: `rdrnd`
+   |
+   = note: it is still passed through to the codegen backend
+   = help: did you mean: `rdrand`
+
+warning: 4 warnings emitted
+

src/test/ui/target-feature/tied-features-cli.one.stderr

@@ -1,4 +1,4 @@
-error: Target features paca, pacg must all be enabled or disabled together
+error: target features paca, pacg must all be enabled or disabled together
 
 error: aborting due to previous error
 

src/test/ui/target-feature/tied-features-cli.rs

@@ -1,9 +1,20 @@
-// only-aarch64
-// revisions: one two three four
-//[one] compile-flags: -C target-feature=+paca
-//[two] compile-flags: -C target-feature=-pacg,+pacg
-//[three] compile-flags: -C target-feature=+paca,+pacg,-paca
-//[four] check-pass
-//[four] compile-flags: -C target-feature=-paca,+pacg -C target-feature=+paca
+// revisions: one two three
+// compile-flags: --crate-type=rlib --target=aarch64-unknown-linux-gnu
+// needs-llvm-components: aarch64
+//
+//
+// [one] check-fail
+// [one] compile-flags: -C target-feature=+paca
+// [two] check-fail
+// [two] compile-flags: -C target-feature=-pacg,+pacg
+// [three] check-fail
+// [three] compile-flags: -C target-feature=+paca,+pacg,-paca
+// [four] build-pass
+// [four] compile-flags: -C target-feature=-paca,+pacg -C target-feature=+paca
+#![feature(no_core, lang_items)]
+#![no_core]
+
+#[lang="sized"]
+trait Sized {}
 
 fn main() {}

src/test/ui/target-feature/tied-features-cli.three.stderr

@@ -1,4 +1,4 @@
-error: Target features paca, pacg must all be enabled or disabled together
+error: target features paca, pacg must all be enabled or disabled together
 
 error: aborting due to previous error