Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

warn when using an unstable feature with -Ctarget-feature #117616

Merged
merged 2 commits into from
Nov 7, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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}`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"and"? or "or"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"and"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? I don't totally understand how something can both be unknown and unstable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's unknown it's clearly not stable, and hence it is unstable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does that need clarification? Like, if it's clearly not stable by virtue of being uknown, why are we saying it's unknown and unstable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth emphasizing that unknown implies unstable. Unstable has a particular meaning in the Rust context, and that implication is very clear to us, but might not be clear at all to users.

It is extremely unusual that we allow unstable things to even be done on stable Rust, making it even more important to point this out explicitly.

.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
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

Loading