Skip to content

Commit

Permalink
Rollup merge of #117616 - RalfJung:unstable-target-features, r=compil…
Browse files Browse the repository at this point in the history
…er-errors

warn when using an unstable feature with -Ctarget-feature

Setting or unsetting the wrong target features can cause ABI incompatibility (#116344, #116558). We need to carefully audit features for their ABI impact before stabilization. I just learned that we currently accept arbitrary unstable features on stable and if they are in the list of Rust target features, even unstable, then we don't even warn about that!1 That doesn't seem great, so I propose we introduce a warning here.

This has an obvious loophole via `-Ctarget-cpu`. I'm not sure how to best deal with that, but it seems better to fix what we can and think about the other cases later, maybe once we have a better idea for how to resolve the general mess that are ABI-affecting target features.
  • Loading branch information
matthiaskrgr authored Nov 7, 2023
2 parents cd5b5e0 + 84abf83 commit f8c6770
Show file tree
Hide file tree
Showing 8 changed files with 70 additions and 23 deletions.
8 changes: 6 additions & 2 deletions compiler/rustc_codegen_llvm/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ codegen_llvm_target_machine = could not create LLVM TargetMachine for triple: {$
codegen_llvm_target_machine_with_llvm_err = could not create LLVM TargetMachine for triple: {$triple}: {$llvm_err}
codegen_llvm_unknown_ctarget_feature =
unknown feature specified for `-Ctarget-feature`: `{$feature}`
.note = it is still passed through to the codegen backend
unknown and unstable feature specified for `-Ctarget-feature`: `{$feature}`
.note = it is still passed through to the codegen backend, but use of this feature might be unsound and the behavior of this feature can change in the future
.possible_feature = you might have meant: `{$rust_feature}`
.consider_filing_feature_request = consider filing a feature request
Expand All @@ -87,6 +87,10 @@ codegen_llvm_unknown_ctarget_feature_prefix =
codegen_llvm_unknown_debuginfo_compression = unknown debuginfo compression algorithm {$algorithm} - will fall back to uncompressed debuginfo
codegen_llvm_unstable_ctarget_feature =
unstable feature specified for `-Ctarget-feature`: `{$feature}`
.note = this feature is not stably supported; its behavior can change in the future
codegen_llvm_write_bytecode = failed to write bytecode to {$path}: {$err}
codegen_llvm_write_ir = failed to write LLVM IR to {$path}
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_codegen_llvm/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ pub(crate) struct UnknownCTargetFeature<'a> {
pub rust_feature: PossibleFeature<'a>,
}

#[derive(Diagnostic)]
#[diag(codegen_llvm_unstable_ctarget_feature)]
#[note]
pub(crate) struct UnstableCTargetFeature<'a> {
pub feature: &'a str,
}

#[derive(Subdiagnostic)]
pub(crate) enum PossibleFeature<'a> {
#[help(codegen_llvm_possible_feature)]
Expand Down
47 changes: 28 additions & 19 deletions compiler/rustc_codegen_llvm/src/llvm_util.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::back::write::create_informational_target_machine;
use crate::errors::{
PossibleFeature, TargetFeatureDisableOrEnable, UnknownCTargetFeature,
UnknownCTargetFeaturePrefix,
UnknownCTargetFeaturePrefix, UnstableCTargetFeature,
};
use crate::llvm;
use libc::c_int;
Expand Down Expand Up @@ -531,25 +531,34 @@ pub(crate) fn global_llvm_features(sess: &Session, diagnostics: bool) -> Vec<Str
};

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)
// Warn against use of LLVM specific feature names and unstable features on the CLI.
if diagnostics {
let feature_state = supported_features.iter().find(|&&(v, _)| v == feature);
if feature_state.is_none() {
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 unknown_feature = if let Some(rust_feature) = rust_feature {
UnknownCTargetFeature {
feature,
rust_feature: PossibleFeature::Some { rust_feature },
}
} else {
None
}
});
let unknown_feature = if let Some(rust_feature) = rust_feature {
UnknownCTargetFeature {
feature,
rust_feature: PossibleFeature::Some { rust_feature },
}
} else {
UnknownCTargetFeature { feature, rust_feature: PossibleFeature::None }
};
sess.emit_warning(unknown_feature);
UnknownCTargetFeature { feature, rust_feature: PossibleFeature::None }
};
sess.emit_warning(unknown_feature);
} else if feature_state.is_some_and(|(_name, feature_gate)| feature_gate.is_some())
{
// An unstable feature. Warn about using it.
sess.emit_warning(UnstableCTargetFeature { feature });
}
}

if diagnostics {
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_codegen_ssa/src/target_features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,15 @@ pub const RUSTC_SPECIFIC_FEATURES: &[&str] = &["crt-static"];
// check whether they're named already elsewhere in rust
// e.g. in stdarch and whether the given name matches LLVM's
// if it doesn't, to_llvm_feature in llvm_util in rustc_codegen_llvm needs to be adapted
//
// When adding a new feature, be particularly mindful of features that affect function ABIs. Those
// need to be treated very carefully to avoid introducing unsoundness! This often affects features
// that enable/disable hardfloat support (see https://github.com/rust-lang/rust/issues/116344 for an
// example of this going wrong), but features enabling new SIMD registers are also a concern (see
// https://github.com/rust-lang/rust/issues/116558 for an example of this going wrong).
//
// Stabilizing a target feature (setting the 2nd component of the pair to `None`) requires t-lang
// approval.

const ARM_ALLOWED_FEATURES: &[(&str, Option<Symbol>)] = &[
// tidy-alphabetical-start
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
warning: unstable feature specified for `-Ctarget-feature`: `nontrapping-fptoint`
|
= note: this feature is not stably supported; its behavior can change in the future

warning: 1 warning emitted

4 changes: 2 additions & 2 deletions tests/ui/target-feature/similar-feature-suggestion.stderr
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
warning: unknown feature specified for `-Ctarget-feature`: `rdrnd`
warning: unknown and unstable feature specified for `-Ctarget-feature`: `rdrnd`
|
= note: it is still passed through to the codegen backend
= note: it is still passed through to the codegen backend, but use of this feature might be unsound and the behavior of this feature can change in the future
= help: you might have meant: `rdrand`

warning: 1 warning emitted
Expand Down
6 changes: 6 additions & 0 deletions tests/ui/target-feature/unstable-feature.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// compile-flags: -Ctarget-feature=+vaes --crate-type=rlib --target=x86_64-unknown-linux-gnu
// build-pass
// needs-llvm-components: x86

#![feature(no_core)]
#![no_core]
6 changes: 6 additions & 0 deletions tests/ui/target-feature/unstable-feature.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
warning: unstable feature specified for `-Ctarget-feature`: `vaes`
|
= note: this feature is not stably supported; its behavior can change in the future

warning: 1 warning emitted

0 comments on commit f8c6770

Please sign in to comment.