Rollup merge of #130308 - davidtwco:tied-target-consolidation, r=wesleywiser

codegen_ssa: consolidate tied target checks

Fixes #105110.
Fixes #105111.

`rustc_codegen_llvm` and `rustc_codegen_gcc` duplicated logic for checking if tied target features were partially enabled. This PR consolidates these checks into `rustc_codegen_ssa` in the `codegen_fn_attrs` query, which also is run pre-monomorphisation for each function, which ensures that this check is run for unused functions, as would be expected.

Also adds a test confirming that enabling one tied feature doesn't imply another - the appropriate error for this was already being emitted. I did a bisect and narrowed it down to two patches it was likely to be - something in #128796, probably #128221 or #128679.
This commit is contained in:
Matthias Krüger 2024-10-10 22:00:45 +02:00 committed by GitHub
commit 13976f1f25
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
19 changed files with 175 additions and 163 deletions

View File

@ -8,9 +8,6 @@ codegen_gcc_invalid_minimum_alignment =
codegen_gcc_lto_not_supported =
LTO is not supported. You may get a linker error.
codegen_gcc_tied_target_features = the target features {$features} must all be either enabled or disabled together
.help = add the missing features in a `target_feature` attribute
codegen_gcc_unwinding_inline_asm =
GCC backend does not support unwinding from inline asm

View File

@ -7,11 +7,9 @@ use rustc_attr::InstructionSetAttr;
#[cfg(feature = "master")]
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc_middle::ty;
use rustc_span::symbol::sym;
use crate::context::CodegenCx;
use crate::errors::TiedTargetFeatures;
use crate::gcc_util::{check_tied_features, to_gcc_features};
use crate::gcc_util::to_gcc_features;
/// Get GCC attribute for the provided inline heuristic.
#[cfg(feature = "master")]
@ -72,26 +70,10 @@ pub fn from_fn_attrs<'gcc, 'tcx>(
}
}
let function_features = codegen_fn_attrs
let mut function_features = codegen_fn_attrs
.target_features
.iter()
.map(|features| features.name.as_str())
.collect::<Vec<&str>>();
if let Some(features) = check_tied_features(
cx.tcx.sess,
&function_features.iter().map(|features| (*features, true)).collect(),
) {
let span = cx
.tcx
.get_attr(instance.def_id(), sym::target_feature)
.map_or_else(|| cx.tcx.def_span(instance.def_id()), |a| a.span);
cx.tcx.dcx().create_err(TiedTargetFeatures { features: features.join(", "), span }).emit();
return;
}
let mut function_features = function_features
.iter()
.flat_map(|feat| to_gcc_features(cx.tcx.sess, feat).into_iter())
.chain(codegen_fn_attrs.instruction_set.iter().map(|x| match *x {
InstructionSetAttr::ArmA32 => "-thumb-mode", // TODO(antoyo): support removing feature.

View File

@ -1,9 +1,6 @@
use rustc_errors::{Diag, DiagCtxtHandle, Diagnostic, EmissionGuarantee, Level};
use rustc_macros::{Diagnostic, Subdiagnostic};
use rustc_span::Span;
use crate::fluent_generated as fluent;
#[derive(Diagnostic)]
#[diag(codegen_gcc_unknown_ctarget_feature_prefix)]
#[note]
@ -45,15 +42,6 @@ pub(crate) struct InvalidMinimumAlignment {
pub err: String,
}
#[derive(Diagnostic)]
#[diag(codegen_gcc_tied_target_features)]
#[help]
pub(crate) struct TiedTargetFeatures {
#[primary_span]
pub span: Span,
pub features: String,
}
#[derive(Diagnostic)]
#[diag(codegen_gcc_copy_bitcode)]
pub(crate) struct CopyBitcode {
@ -78,27 +66,3 @@ pub(crate) struct LtoDylib;
pub(crate) struct LtoBitcodeFromRlib {
pub gcc_err: String,
}
pub(crate) struct TargetFeatureDisableOrEnable<'a> {
pub features: &'a [&'a str],
pub span: Option<Span>,
pub missing_features: Option<MissingFeatures>,
}
#[derive(Subdiagnostic)]
#[help(codegen_gcc_missing_features)]
pub(crate) 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_gcc_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
}
}

View File

@ -1,15 +1,14 @@
#[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;
use rustc_middle::bug;
use rustc_session::Session;
use rustc_target::target_features::RUSTC_SPECIFIC_FEATURES;
use smallvec::{SmallVec, smallvec};
use crate::errors::{
PossibleFeature, TargetFeatureDisableOrEnable, UnknownCTargetFeature,
UnknownCTargetFeaturePrefix,
};
use crate::errors::{PossibleFeature, UnknownCTargetFeature, UnknownCTargetFeaturePrefix};
/// The list of GCC features computed from CLI flags (`-Ctarget-cpu`, `-Ctarget-feature`,
/// `--target` and similar).
@ -185,23 +184,6 @@ pub fn to_gcc_features<'a>(sess: &Session, s: &'a str) -> SmallVec<[&'a str; 2]>
}
}
// 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]> {
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(|feature| enabled != features.get(feature)) {
return Some(tied);
}
}
None
}
fn arch_to_gcc(name: &str) -> &str {
match name {
"M68020" => "68020",

View File

@ -33,9 +33,6 @@ codegen_llvm_lto_proc_macro = lto cannot be used for `proc-macro` crate type wit
codegen_llvm_mismatch_data_layout =
data-layout for target `{$rustc_target}`, `{$rustc_layout}`, differs from LLVM target's `{$llvm_target}` default layout, `{$llvm_layout}`
codegen_llvm_missing_features =
add the missing features in a `target_feature` attribute
codegen_llvm_multiple_source_dicompileunit = multiple source DICompileUnits found
codegen_llvm_multiple_source_dicompileunit_with_llvm_err = multiple source DICompileUnits found: {$llvm_err}
@ -63,9 +60,6 @@ codegen_llvm_serialize_module_with_llvm_err = failed to serialize module {$name}
codegen_llvm_symbol_already_defined =
symbol `{$symbol_name}` is already defined
codegen_llvm_target_feature_disable_or_enable =
the target features {$features} must all be either enabled or disabled together
codegen_llvm_target_machine = could not create LLVM TargetMachine for triple: {$triple}
codegen_llvm_target_machine_with_llvm_err = could not create LLVM TargetMachine for triple: {$triple}: {$llvm_err}

View File

@ -6,12 +6,11 @@ use rustc_hir::def_id::DefId;
use rustc_middle::middle::codegen_fn_attrs::{CodegenFnAttrFlags, PatchableFunctionEntry};
use rustc_middle::ty::{self, TyCtxt};
use rustc_session::config::{BranchProtection, FunctionReturn, OptLevel, PAuthKey, PacRet};
use rustc_span::symbol::sym;
use rustc_target::spec::{FramePointer, SanitizerSet, StackProbeType, StackProtector};
use smallvec::SmallVec;
use crate::context::CodegenCx;
use crate::errors::{MissingFeatures, SanitizerMemtagRequiresMte, TargetFeatureDisableOrEnable};
use crate::errors::SanitizerMemtagRequiresMte;
use crate::llvm::AttributePlace::Function;
use crate::llvm::{self, AllocKindFlags, Attribute, AttributeKind, AttributePlace, MemoryEffects};
use crate::value::Value;
@ -502,26 +501,6 @@ pub(crate) fn llfn_attrs_from_instance<'ll, 'tcx>(
let function_features =
codegen_fn_attrs.target_features.iter().map(|f| f.name.as_str()).collect::<Vec<&str>>();
if let Some(f) = llvm_util::check_tied_features(
cx.tcx.sess,
&function_features.iter().map(|f| (*f, true)).collect(),
) {
let span = cx
.tcx
.get_attrs(instance.def_id(), sym::target_feature)
.next()
.map_or_else(|| cx.tcx.def_span(instance.def_id()), |a| a.span);
cx.tcx
.dcx()
.create_err(TargetFeatureDisableOrEnable {
features: f,
span: Some(span),
missing_features: Some(MissingFeatures),
})
.emit();
return;
}
let function_features = function_features
.iter()
// Convert to LLVMFeatures and filter out unavailable ones

View File

@ -80,30 +80,6 @@ impl<G: EmissionGuarantee> Diagnostic<'_, G> for ParseTargetMachineConfig<'_> {
}
}
pub(crate) struct TargetFeatureDisableOrEnable<'a> {
pub features: &'a [&'a str],
pub span: Option<Span>,
pub missing_features: Option<MissingFeatures>,
}
#[derive(Subdiagnostic)]
#[help(codegen_llvm_missing_features)]
pub(crate) 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_llvm_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_llvm_lto_disallowed)]
pub(crate) struct LtoDisallowed;

View File

@ -6,6 +6,7 @@ use std::{ptr, slice, str};
use libc::c_int;
use rustc_codegen_ssa::base::wants_wasm_eh;
use rustc_codegen_ssa::codegen_attrs::check_tied_features;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::small_c_str::SmallCStr;
use rustc_data_structures::unord::UnordSet;
@ -19,8 +20,8 @@ use rustc_target::target_features::{RUSTC_SPECIAL_FEATURES, RUSTC_SPECIFIC_FEATU
use crate::back::write::create_informational_target_machine;
use crate::errors::{
FixedX18InvalidArch, InvalidTargetFeaturePrefix, PossibleFeature, TargetFeatureDisableOrEnable,
UnknownCTargetFeature, UnknownCTargetFeaturePrefix, UnstableCTargetFeature,
FixedX18InvalidArch, InvalidTargetFeaturePrefix, PossibleFeature, UnknownCTargetFeature,
UnknownCTargetFeaturePrefix, UnstableCTargetFeature,
};
use crate::llvm;
@ -276,25 +277,6 @@ pub(crate) fn to_llvm_features<'a>(sess: &Session, s: &'a str) -> Option<LLVMFea
}
}
/// Given a map from target_features to whether they are enabled or disabled,
/// ensure only valid combinations are allowed.
pub(crate) 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
}
/// Used to generate cfg variables and apply features
/// Must express features in the way Rust understands them
pub fn target_features(sess: &Session, allow_unstable: bool) -> Vec<Symbol> {
@ -685,7 +667,7 @@ pub(crate) fn global_llvm_features(
features.extend(feats);
if diagnostics && let Some(f) = check_tied_features(sess, &featsmap) {
sess.dcx().emit_err(TargetFeatureDisableOrEnable {
sess.dcx().emit_err(rustc_codegen_ssa::errors::TargetFeatureDisableOrEnable {
features: f,
span: None,
missing_features: None,

View File

@ -183,6 +183,8 @@ codegen_ssa_metadata_object_file_write = error writing metadata object file: {$e
codegen_ssa_missing_cpp_build_tool_component = or a necessary component may be missing from the "C++ build tools" workload
codegen_ssa_missing_features = add the missing features in a `target_feature` attribute
codegen_ssa_missing_memory_ordering = Atomic intrinsic missing memory ordering
codegen_ssa_missing_query_depgraph =
@ -238,6 +240,9 @@ codegen_ssa_stripping_debug_info_failed = stripping debug info with `{$util}` fa
codegen_ssa_symbol_file_write_failure = failed to write symbols file: {$error}
codegen_ssa_target_feature_disable_or_enable =
the target features {$features} must all be either enabled or disabled together
codegen_ssa_target_feature_safe_trait = `#[target_feature(..)]` cannot be applied to safe trait method
.label = cannot be applied to safe trait method
.label_def = not an `unsafe` function

View File

@ -1,5 +1,6 @@
use rustc_ast::{MetaItemInner, MetaItemKind, ast, attr};
use rustc_attr::{InlineAttr, InstructionSetAttr, OptimizeAttr, list_contains_name};
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::codes::*;
use rustc_errors::{DiagMessage, SubdiagMessage, struct_span_code_err};
use rustc_hir as hir;
@ -13,13 +14,13 @@ use rustc_middle::middle::codegen_fn_attrs::{
use rustc_middle::mir::mono::Linkage;
use rustc_middle::query::Providers;
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::symbol::Ident;
use rustc_span::{Span, sym};
use rustc_target::spec::{SanitizerSet, abi};
use crate::errors;
use crate::errors::{self, MissingFeatures, TargetFeatureDisableOrEnable};
use crate::target_features::{check_target_feature_trait_unsafe, from_target_feature};
fn linkage_by_name(tcx: TyCtxt<'_>, def_id: LocalDefId, name: &str) -> Linkage {
@ -662,9 +663,49 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
}
}
if let Some(features) = check_tied_features(
tcx.sess,
&codegen_fn_attrs
.target_features
.iter()
.map(|features| (features.name.as_str(), true))
.collect(),
) {
let span = tcx
.get_attrs(did, sym::target_feature)
.next()
.map_or_else(|| tcx.def_span(did), |a| a.span);
tcx.dcx()
.create_err(TargetFeatureDisableOrEnable {
features,
span: Some(span),
missing_features: Some(MissingFeatures),
})
.emit();
}
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 {

View File

@ -9,7 +9,7 @@ use rustc_errors::codes::*;
use rustc_errors::{
Diag, DiagArgValue, DiagCtxtHandle, Diagnostic, EmissionGuarantee, IntoDiagArg, Level,
};
use rustc_macros::Diagnostic;
use rustc_macros::{Diagnostic, Subdiagnostic};
use rustc_middle::ty::Ty;
use rustc_middle::ty::layout::LayoutError;
use rustc_span::{Span, Symbol};
@ -1068,3 +1068,27 @@ pub(crate) struct ErrorCreatingImportLibrary<'a> {
pub lib_name: &'a str,
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
}
}

View File

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

View File

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

View File

@ -0,0 +1,20 @@
//@ revisions: paca pacg
//@ compile-flags: --crate-type=rlib --target=aarch64-unknown-linux-gnu
//@ needs-llvm-components: aarch64
//@[paca] compile-flags: -Ctarget-feature=+paca
//@[paca] error-pattern: the target features paca, pacg must all be either enabled or disabled together
//@[pacg] compile-flags: -Ctarget-feature=+pacg
//@[paca] error-pattern: the target features paca, pacg must all be either enabled or disabled together
#![feature(no_core, lang_items)]
#![no_core]
#[lang="sized"]
trait Sized {}
// In this test, demonstrate that +paca and +pacg both result in the tied feature error if there
// isn't something causing an error.
// See tied-features-no-implication.rs
#[cfg(target_feature = "pacg")]
pub unsafe fn foo() {
}

View File

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

View File

@ -0,0 +1,14 @@
error[E0428]: the name `foo` is defined multiple times
--> $DIR/tied-features-no-implication.rs:28:1
|
LL | fn foo() {}
| -------- previous definition of the value `foo` here
...
LL | pub unsafe fn foo() {
| ^^^^^^^^^^^^^^^^^^^ `foo` redefined here
|
= note: `foo` must be defined only once in the value namespace of this module
error: aborting due to 1 previous error
For more information about this error, try `rustc --explain E0428`.

View File

@ -0,0 +1,29 @@
//@ revisions: paca pacg
//@ compile-flags: --crate-type=rlib --target=aarch64-unknown-linux-gnu
//@ needs-llvm-components: aarch64
//@[paca] compile-flags: -Ctarget-feature=+paca
//@[paca] error-pattern: the target features paca, pacg must all be either enabled or disabled together
//@[pacg] compile-flags: -Ctarget-feature=+pacg
//@[pacg] error-pattern: the name `foo` is defined multiple times
#![feature(no_core, lang_items)]
#![no_core]
#[lang="sized"]
trait Sized {}
// Can't use `compile_error!` here without `core`/`std` but requiring these makes this test only
// work if you have libcore built in the sysroot for `aarch64-unknown-linux-gnu`. Can't run this
// test on any aarch64 platform because they all have different default available features - as
// written, this test depends on `aarch64-unknown-linux-gnu` having -paca,-pacg by default.
// Cause a multiple definition error instead.
fn foo() {}
// Enabling one of the tied features does not imply the other is enabled.
//
// With +paca, this multiple definition doesn't cause an error because +paca hasn't implied
// +pacg. With +pacg, the multiple definition error is emitted (and the tied feature error would
// be).
#[cfg(target_feature = "pacg")]
pub unsafe fn foo() {
}

View File

@ -1,4 +1,3 @@
//@ build-fail
//@ compile-flags: --crate-type=rlib --target=aarch64-unknown-linux-gnu
//@ needs-llvm-components: aarch64
#![feature(no_core, lang_items)]
@ -7,7 +6,6 @@
#[lang="sized"]
trait Sized {}
// FIXME: this should not need to be public.
pub fn main() {
#[target_feature(enable = "pacg")]
//~^ ERROR must all be either enabled or disabled together
@ -25,10 +23,15 @@ pub fn main() {
//~^ ERROR must all be either enabled or disabled together
unsafe fn foo() {}
#[target_feature(enable = "paca,pacg")]
unsafe fn bar() {}
#[target_feature(enable = "paca")]
#[target_feature(enable = "pacg")]
unsafe fn baz() {}
// Confirm that functions which do not end up collected for monomorphisation will still error.
#[target_feature(enable = "paca")]
//~^ ERROR must all be either enabled or disabled together
unsafe fn unused() {}

View File

@ -1,5 +1,5 @@
error: the target features paca, pacg must all be either enabled or disabled together
--> $DIR/tied-features.rs:12:5
--> $DIR/tied-features.rs:10:5
|
LL | #[target_feature(enable = "pacg")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@ -7,12 +7,20 @@ LL | #[target_feature(enable = "pacg")]
= help: add the missing features in a `target_feature` attribute
error: the target features paca, pacg must all be either enabled or disabled together
--> $DIR/tied-features.rs:24:1
--> $DIR/tied-features.rs:22:1
|
LL | #[target_feature(enable = "paca")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: add the missing features in a `target_feature` attribute
error: aborting due to 2 previous errors
error: the target features paca, pacg must all be either enabled or disabled together
--> $DIR/tied-features.rs:35:1
|
LL | #[target_feature(enable = "paca")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: add the missing features in a `target_feature` attribute
error: aborting due to 3 previous errors