From dc06c06f46a1b12d8eaf3a07b8e4e5efc7a3faff Mon Sep 17 00:00:00 2001 From: Luca Versari Date: Sat, 13 Jul 2024 19:35:05 +0200 Subject: [PATCH] Emit error when calling/declaring functions with unavailable vectors. On some architectures, vector types may have a different ABI when relevant target features are enabled. As discussed in https://github.com/rust-lang/lang-team/issues/235, this turns out to very easily lead to unsound code. This commit makes it an error to declare or call functions using those vector types in a context in which the corresponding target features are disabled, if using an ABI for which the difference is relevant. --- Cargo.lock | 1 + compiler/rustc_monomorphize/Cargo.toml | 1 + compiler/rustc_monomorphize/messages.ftl | 7 ++ compiler/rustc_monomorphize/src/collector.rs | 3 + .../src/collector/abi_check.rs | 97 +++++++++++++++++++ compiler/rustc_monomorphize/src/errors.rs | 18 ++++ library/core/src/primitive_docs.rs | 21 +--- tests/assembly/simd-bitmask.rs | 1 + tests/assembly/simd-intrinsic-gather.rs | 1 + tests/assembly/simd-intrinsic-mask-load.rs | 1 + tests/assembly/simd-intrinsic-mask-reduce.rs | 1 + tests/assembly/simd-intrinsic-mask-store.rs | 1 + tests/assembly/simd-intrinsic-scatter.rs | 1 + tests/assembly/simd-intrinsic-select.rs | 1 + tests/ui/simd-abi-checks.rs | 69 +++++++++++++ tests/ui/simd-abi-checks.stderr | 74 ++++++++++++++ 16 files changed, 278 insertions(+), 20 deletions(-) create mode 100644 compiler/rustc_monomorphize/src/collector/abi_check.rs create mode 100644 tests/ui/simd-abi-checks.rs create mode 100644 tests/ui/simd-abi-checks.stderr diff --git a/Cargo.lock b/Cargo.lock index 701ffc0e7d1a8..b84c12346529b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4138,6 +4138,7 @@ dependencies = [ name = "rustc_monomorphize" version = "0.0.0" dependencies = [ + "rustc_abi", "rustc_data_structures", "rustc_errors", "rustc_fluent_macro", diff --git a/compiler/rustc_monomorphize/Cargo.toml b/compiler/rustc_monomorphize/Cargo.toml index c7f1b9fa78454..6c881fd7e06ba 100644 --- a/compiler/rustc_monomorphize/Cargo.toml +++ b/compiler/rustc_monomorphize/Cargo.toml @@ -5,6 +5,7 @@ edition = "2021" [dependencies] # tidy-alphabetical-start +rustc_abi = { path = "../rustc_abi" } rustc_data_structures = { path = "../rustc_data_structures" } rustc_errors = { path = "../rustc_errors" } rustc_fluent_macro = { path = "../rustc_fluent_macro" } diff --git a/compiler/rustc_monomorphize/messages.ftl b/compiler/rustc_monomorphize/messages.ftl index 7210701d4828c..4de71c676a6d5 100644 --- a/compiler/rustc_monomorphize/messages.ftl +++ b/compiler/rustc_monomorphize/messages.ftl @@ -1,3 +1,10 @@ +monomorphize_abi_error_disabled_vector_type_call = + ABI error: this function call uses a {$required_feature} vector type, which is not enabled in the caller + .help = consider enabling it globally (-C target-feature=+{$required_feature}) or locally (#[target_feature(enable="{$required_feature}")]) +monomorphize_abi_error_disabled_vector_type_def = + ABI error: this function definition uses a {$required_feature} vector type, which is not enabled + .help = consider enabling it globally (-C target-feature=+{$required_feature}) or locally (#[target_feature(enable="{$required_feature}")]) + monomorphize_couldnt_dump_mono_stats = unexpected error occurred while dumping monomorphization stats: {$error} diff --git a/compiler/rustc_monomorphize/src/collector.rs b/compiler/rustc_monomorphize/src/collector.rs index b4d084d4dffc4..3f4d2fc6e31ef 100644 --- a/compiler/rustc_monomorphize/src/collector.rs +++ b/compiler/rustc_monomorphize/src/collector.rs @@ -205,6 +205,7 @@ //! this is not implemented however: a mono item will be produced //! regardless of whether it is actually needed or not. +mod abi_check; mod move_check; use std::path::PathBuf; @@ -766,6 +767,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirUsedCollector<'a, 'tcx> { self.used_mentioned_items.insert(MentionedItem::Fn(callee_ty)); let callee_ty = self.monomorphize(callee_ty); self.check_fn_args_move_size(callee_ty, args, *fn_span, location); + abi_check::check_call_site_abi(tcx, callee_ty, *fn_span, self.body.source.instance); visit_fn_use(self.tcx, callee_ty, true, source, &mut self.used_items) } mir::TerminatorKind::Drop { ref place, .. } => { @@ -1207,6 +1209,7 @@ fn collect_items_of_instance<'tcx>( mentioned_items: &mut MonoItems<'tcx>, mode: CollectionMode, ) { + abi_check::check_instance_abi(tcx, instance); let body = tcx.instance_mir(instance.def); // Naively, in "used" collection mode, all functions get added to *both* `used_items` and // `mentioned_items`. Mentioned items processing will then notice that they have already been diff --git a/compiler/rustc_monomorphize/src/collector/abi_check.rs b/compiler/rustc_monomorphize/src/collector/abi_check.rs new file mode 100644 index 0000000000000..ecc378ecd3d96 --- /dev/null +++ b/compiler/rustc_monomorphize/src/collector/abi_check.rs @@ -0,0 +1,97 @@ +use rustc_abi::Abi; +use rustc_middle::ty::{self, Instance, InstanceKind, ParamEnv, Ty, TyCtxt}; +use rustc_span::def_id::DefId; +use rustc_span::{Span, Symbol}; +use rustc_target::abi::call::{FnAbi, PassMode}; + +use crate::errors::{AbiErrorDisabledVectorTypeCall, AbiErrorDisabledVectorTypeDef}; + +// Represents the least-constraining feature that is required for vector types up to a certain size +// to have their "proper" ABI. +const X86_VECTOR_FEATURES: &'static [(u64, &'static str)] = + &[(128, "sse"), (256, "avx"), (512, "avx512f")]; + +fn do_check_abi<'tcx>( + tcx: TyCtxt<'tcx>, + abi: &FnAbi<'tcx, Ty<'tcx>>, + target_feature_def: DefId, + emit_err: impl Fn(&'static str), +) { + let feature_def = if tcx.sess.target.arch == "x86" || tcx.sess.target.arch == "x86_64" { + X86_VECTOR_FEATURES + } else if tcx.sess.target.arch == "aarch64" { + // ABI on aarch64 does not depend on target features. + return; + } else { + // FIXME: add support for non-tier1 architectures + return; + }; + let codegen_attrs = tcx.codegen_fn_attrs(target_feature_def); + for arg_abi in abi.args.iter().chain(std::iter::once(&abi.ret)) { + let size = arg_abi.layout.size; + if matches!(arg_abi.layout.abi, Abi::Vector { .. }) + && !matches!(arg_abi.mode, PassMode::Indirect { .. }) + { + let feature = match feature_def.iter().find(|(bits, _)| size.bits() <= *bits) { + Some((_, feature)) => feature, + None => panic!("Unknown vector size: {}; arg = {:?}", size.bits(), arg_abi), + }; + let feature_sym = Symbol::intern(feature); + if !tcx.sess.unstable_target_features.contains(&feature_sym) + && !codegen_attrs.target_features.iter().any(|x| x.name == feature_sym) + { + emit_err(feature); + } + } + } +} + +/// Checks that the ABI of a given instance of a function does not contain vector-passed arguments +/// or return values for which the corresponding target feature is not enabled. +pub fn check_instance_abi<'tcx>(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) { + let param_env = ParamEnv::reveal_all(); + let Ok(abi) = tcx.fn_abi_of_instance(param_env.and((instance, ty::List::empty()))) else { + // An error will be reported during codegen if we cannot determine the ABI of this + // function. + return; + }; + do_check_abi(tcx, abi, instance.def_id(), |required_feature| { + tcx.dcx().emit_err(AbiErrorDisabledVectorTypeDef { + span: tcx.def_span(instance.def_id()), + required_feature, + }); + }) +} + +/// Checks that a call expression does not try to pass a vector-passed argument which requires a +/// target feature that the caller does not have, as doing so causes UB because of ABI mismatch. +pub fn check_call_site_abi<'tcx>( + tcx: TyCtxt<'tcx>, + ty: Ty<'tcx>, + span: Span, + caller: InstanceKind<'tcx>, +) { + let param_env = ParamEnv::reveal_all(); + let callee_abi = match *ty.kind() { + ty::FnPtr(..) => tcx.fn_abi_of_fn_ptr(param_env.and((ty.fn_sig(tcx), ty::List::empty()))), + ty::FnDef(def_id, args) => { + // Intrinsics are handled separately by the compiler. + if tcx.intrinsic(def_id).is_some() { + return; + } + let instance = ty::Instance::expect_resolve(tcx, param_env, def_id, args, span); + tcx.fn_abi_of_instance(param_env.and((instance, ty::List::empty()))) + } + _ => { + panic!("Invalid function call"); + } + }; + + let Ok(callee_abi) = callee_abi else { + // ABI failed to compute; this will not get through codegen. + return; + }; + do_check_abi(tcx, callee_abi, caller.def_id(), |required_feature| { + tcx.dcx().emit_err(AbiErrorDisabledVectorTypeCall { span, required_feature }); + }) +} diff --git a/compiler/rustc_monomorphize/src/errors.rs b/compiler/rustc_monomorphize/src/errors.rs index d5fae6e23cb45..e99321252bdc9 100644 --- a/compiler/rustc_monomorphize/src/errors.rs +++ b/compiler/rustc_monomorphize/src/errors.rs @@ -92,3 +92,21 @@ pub(crate) struct StartNotFound; pub(crate) struct UnknownCguCollectionMode<'a> { pub mode: &'a str, } + +#[derive(Diagnostic)] +#[diag(monomorphize_abi_error_disabled_vector_type_def)] +#[help] +pub struct AbiErrorDisabledVectorTypeDef<'a> { + #[primary_span] + pub span: Span, + pub required_feature: &'a str, +} + +#[derive(Diagnostic)] +#[diag(monomorphize_abi_error_disabled_vector_type_call)] +#[help] +pub struct AbiErrorDisabledVectorTypeCall<'a> { + #[primary_span] + pub span: Span, + pub required_feature: &'a str, +} diff --git a/library/core/src/primitive_docs.rs b/library/core/src/primitive_docs.rs index 89936dc12ac36..f391d008e3d2c 100644 --- a/library/core/src/primitive_docs.rs +++ b/library/core/src/primitive_docs.rs @@ -1752,8 +1752,7 @@ mod prim_ref {} /// /// For two signatures to be considered *ABI-compatible*, they must use a compatible ABI string, /// must take the same number of arguments, the individual argument types and the return types must -/// be ABI-compatible, and the target feature requirements must be met (see the subsection below for -/// the last point). The ABI string is declared via `extern "ABI" fn(...) -> ...`; note that +/// be ABI-compatible. The ABI string is declared via `extern "ABI" fn(...) -> ...`; note that /// `fn name(...) -> ...` implicitly uses the `"Rust"` ABI string and `extern fn name(...) -> ...` /// implicitly uses the `"C"` ABI string. /// @@ -1821,24 +1820,6 @@ mod prim_ref {} /// Behavior since transmuting `None::>` to `NonZero` violates the non-zero /// requirement. /// -/// #### Requirements concerning target features -/// -/// Under some conditions, the signature used by the caller and the callee can be ABI-incompatible -/// even if the exact same ABI string and types are being used. As an example, the -/// `std::arch::x86_64::__m256` type has a different `extern "C"` ABI when the `avx` feature is -/// enabled vs when it is not enabled. -/// -/// Therefore, to ensure ABI compatibility when code using different target features is combined -/// (such as via `#[target_feature]`), we further require that one of the following conditions is -/// met: -/// -/// - The function uses the `"Rust"` ABI string (which is the default without `extern`). -/// - Caller and callee are using the exact same set of target features. For the callee we consider -/// the features enabled (via `#[target_feature]` and `-C target-feature`/`-C target-cpu`) at the -/// declaration site; for the caller we consider the features enabled at the call site. -/// - Neither any argument nor the return value involves a SIMD type (`#[repr(simd)]`) that is not -/// behind a pointer indirection (i.e., `*mut __m256` is fine, but `(i32, __m256)` is not). -/// /// ### Trait implementations /// /// In this documentation the shorthand `fn(T₁, T₂, …, Tₙ)` is used to represent non-variadic diff --git a/tests/assembly/simd-bitmask.rs b/tests/assembly/simd-bitmask.rs index 9a355cc162f67..8d99c3694b3c5 100644 --- a/tests/assembly/simd-bitmask.rs +++ b/tests/assembly/simd-bitmask.rs @@ -1,3 +1,4 @@ +//@ ignore-test //@ revisions: x86 x86-avx2 x86-avx512 aarch64 //@ [x86] compile-flags: --target=x86_64-unknown-linux-gnu -C llvm-args=-x86-asm-syntax=intel //@ [x86] needs-llvm-components: x86 diff --git a/tests/assembly/simd-intrinsic-gather.rs b/tests/assembly/simd-intrinsic-gather.rs index 2cbb6cfbb50d9..28af5693b7267 100644 --- a/tests/assembly/simd-intrinsic-gather.rs +++ b/tests/assembly/simd-intrinsic-gather.rs @@ -1,3 +1,4 @@ +//@ ignore-test //@ revisions: x86-avx512 //@ [x86-avx512] compile-flags: --target=x86_64-unknown-linux-gnu -C llvm-args=-x86-asm-syntax=intel //@ [x86-avx512] compile-flags: -C target-feature=+avx512f,+avx512vl,+avx512bw,+avx512dq diff --git a/tests/assembly/simd-intrinsic-mask-load.rs b/tests/assembly/simd-intrinsic-mask-load.rs index b650e1cee3036..812bfbb2e81f4 100644 --- a/tests/assembly/simd-intrinsic-mask-load.rs +++ b/tests/assembly/simd-intrinsic-mask-load.rs @@ -1,3 +1,4 @@ +//@ ignore-test //@ revisions: x86-avx2 x86-avx512 //@ [x86-avx2] compile-flags: --target=x86_64-unknown-linux-gnu -C llvm-args=-x86-asm-syntax=intel //@ [x86-avx2] compile-flags: -C target-feature=+avx2 diff --git a/tests/assembly/simd-intrinsic-mask-reduce.rs b/tests/assembly/simd-intrinsic-mask-reduce.rs index 61d7aa590938c..423605df02599 100644 --- a/tests/assembly/simd-intrinsic-mask-reduce.rs +++ b/tests/assembly/simd-intrinsic-mask-reduce.rs @@ -1,3 +1,4 @@ +//@ ignore-test // verify that simd mask reductions do not introduce additional bit shift operations //@ revisions: x86 aarch64 //@ [x86] compile-flags: --target=x86_64-unknown-linux-gnu -C llvm-args=-x86-asm-syntax=intel diff --git a/tests/assembly/simd-intrinsic-mask-store.rs b/tests/assembly/simd-intrinsic-mask-store.rs index 95a3b28b96796..a5e38cc68be67 100644 --- a/tests/assembly/simd-intrinsic-mask-store.rs +++ b/tests/assembly/simd-intrinsic-mask-store.rs @@ -1,3 +1,4 @@ +//@ ignore-test //@ revisions: x86-avx2 x86-avx512 //@ [x86-avx2] compile-flags: --target=x86_64-unknown-linux-gnu -C llvm-args=-x86-asm-syntax=intel //@ [x86-avx2] compile-flags: -C target-feature=+avx2 diff --git a/tests/assembly/simd-intrinsic-scatter.rs b/tests/assembly/simd-intrinsic-scatter.rs index 679972d9b86f7..af88a8f6a5ca3 100644 --- a/tests/assembly/simd-intrinsic-scatter.rs +++ b/tests/assembly/simd-intrinsic-scatter.rs @@ -1,3 +1,4 @@ +//@ ignore-test //@ revisions: x86-avx512 //@ [x86-avx512] compile-flags: --target=x86_64-unknown-linux-gnu -C llvm-args=-x86-asm-syntax=intel //@ [x86-avx512] compile-flags: -C target-feature=+avx512f,+avx512vl,+avx512bw,+avx512dq diff --git a/tests/assembly/simd-intrinsic-select.rs b/tests/assembly/simd-intrinsic-select.rs index 57fd36fd9e302..a94b3654067a4 100644 --- a/tests/assembly/simd-intrinsic-select.rs +++ b/tests/assembly/simd-intrinsic-select.rs @@ -1,3 +1,4 @@ +//@ ignore-test //@ revisions: x86-avx2 x86-avx512 aarch64 //@ [x86-avx2] compile-flags: --target=x86_64-unknown-linux-gnu -C llvm-args=-x86-asm-syntax=intel //@ [x86-avx2] compile-flags: -C target-feature=+avx2 diff --git a/tests/ui/simd-abi-checks.rs b/tests/ui/simd-abi-checks.rs new file mode 100644 index 0000000000000..b604a825babfb --- /dev/null +++ b/tests/ui/simd-abi-checks.rs @@ -0,0 +1,69 @@ +//@ only-x86_64 +//@ build-fail + +#![feature(avx512_target_feature)] +#![feature(portable_simd)] +#![allow(improper_ctypes_definitions)] + +use std::arch::x86_64::*; + +#[repr(transparent)] +struct Wrapper(__m256); + +unsafe extern "C" fn w(_: Wrapper) { + //~^ ABI error: this function definition uses a avx vector type, which is not enabled + todo!() +} + +unsafe extern "C" fn f(_: __m256) { + //~^ ABI error: this function definition uses a avx vector type, which is not enabled + todo!() +} + +unsafe extern "C" fn g() -> __m256 { + //~^ ABI error: this function definition uses a avx vector type, which is not enabled + todo!() +} + +#[target_feature(enable = "avx2")] +unsafe extern "C" fn favx(_: __m256) { + todo!() +} + +#[target_feature(enable = "avx")] +unsafe extern "C" fn gavx() -> __m256 { + todo!() +} + +fn as_f64x8(d: __m512d) -> std::simd::f64x8 { + unsafe { std::mem::transmute(d) } +} + +unsafe fn test() { + let arg = std::mem::transmute([0.0f64; 8]); + as_f64x8(arg); +} + +fn main() { + unsafe { + f(g()); + //~^ ERROR ABI error: this function call uses a avx vector type, which is not enabled in the caller + //~| ERROR ABI error: this function call uses a avx vector type, which is not enabled in the caller + } + + unsafe { + favx(gavx()); + //~^ ERROR ABI error: this function call uses a avx vector type, which is not enabled in the caller + //~| ERROR ABI error: this function call uses a avx vector type, which is not enabled in the caller + } + + unsafe { + test(); + } + + unsafe { + w(Wrapper(g())); + //~^ ERROR ABI error: this function call uses a avx vector type, which is not enabled in the caller + //~| ERROR ABI error: this function call uses a avx vector type, which is not enabled in the caller + } +} diff --git a/tests/ui/simd-abi-checks.stderr b/tests/ui/simd-abi-checks.stderr new file mode 100644 index 0000000000000..8a0cdb08e922e --- /dev/null +++ b/tests/ui/simd-abi-checks.stderr @@ -0,0 +1,74 @@ +error: ABI error: this function call uses a avx vector type, which is not enabled in the caller + --> $DIR/simd-abi-checks.rs:49:11 + | +LL | f(g()); + | ^^^ + | + = help: consider enabling it globally (-C target-feature=+avx) or locally (#[target_feature(enable="avx")]) + +error: ABI error: this function call uses a avx vector type, which is not enabled in the caller + --> $DIR/simd-abi-checks.rs:49:9 + | +LL | f(g()); + | ^^^^^^ + | + = help: consider enabling it globally (-C target-feature=+avx) or locally (#[target_feature(enable="avx")]) + +error: ABI error: this function call uses a avx vector type, which is not enabled in the caller + --> $DIR/simd-abi-checks.rs:55:14 + | +LL | favx(gavx()); + | ^^^^^^ + | + = help: consider enabling it globally (-C target-feature=+avx) or locally (#[target_feature(enable="avx")]) + +error: ABI error: this function call uses a avx vector type, which is not enabled in the caller + --> $DIR/simd-abi-checks.rs:55:9 + | +LL | favx(gavx()); + | ^^^^^^^^^^^^ + | + = help: consider enabling it globally (-C target-feature=+avx) or locally (#[target_feature(enable="avx")]) + +error: ABI error: this function call uses a avx vector type, which is not enabled in the caller + --> $DIR/simd-abi-checks.rs:65:19 + | +LL | w(Wrapper(g())); + | ^^^ + | + = help: consider enabling it globally (-C target-feature=+avx) or locally (#[target_feature(enable="avx")]) + +error: ABI error: this function call uses a avx vector type, which is not enabled in the caller + --> $DIR/simd-abi-checks.rs:65:9 + | +LL | w(Wrapper(g())); + | ^^^^^^^^^^^^^^^ + | + = help: consider enabling it globally (-C target-feature=+avx) or locally (#[target_feature(enable="avx")]) + +error: ABI error: this function definition uses a avx vector type, which is not enabled + --> $DIR/simd-abi-checks.rs:23:1 + | +LL | unsafe extern "C" fn g() -> __m256 { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider enabling it globally (-C target-feature=+avx) or locally (#[target_feature(enable="avx")]) + +error: ABI error: this function definition uses a avx vector type, which is not enabled + --> $DIR/simd-abi-checks.rs:18:1 + | +LL | unsafe extern "C" fn f(_: __m256) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider enabling it globally (-C target-feature=+avx) or locally (#[target_feature(enable="avx")]) + +error: ABI error: this function definition uses a avx vector type, which is not enabled + --> $DIR/simd-abi-checks.rs:13:1 + | +LL | unsafe extern "C" fn w(_: Wrapper) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider enabling it globally (-C target-feature=+avx) or locally (#[target_feature(enable="avx")]) + +error: aborting due to 9 previous errors +