From 7a290fce907da1cda0687fe5e48372c08fe40ab8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 26 Aug 2024 17:01:46 +0200 Subject: [PATCH] interpret: do not make const-eval query result depend on tcx.sess --- compiler/rustc_const_eval/messages.ftl | 3 -- .../rustc_const_eval/src/interpret/call.rs | 44 +++---------------- .../rustc_const_eval/src/interpret/machine.rs | 22 +++++++--- src/tools/miri/src/machine.rs | 42 +++++++++++++++--- .../target_feature_wasm.rs | 4 +- .../const-eval/const_fn_target_feature.rs | 4 +- .../const-eval/const_fn_target_feature.stderr | 9 ---- .../const_fn_target_feature_wasm.rs | 4 +- 8 files changed, 70 insertions(+), 62 deletions(-) rename src/tools/miri/tests/{pass/function_calls => panic}/target_feature_wasm.rs (61%) delete mode 100644 tests/ui/consts/const-eval/const_fn_target_feature.stderr diff --git a/compiler/rustc_const_eval/messages.ftl b/compiler/rustc_const_eval/messages.ftl index d7d64180ec9c1..db788b6b15199 100644 --- a/compiler/rustc_const_eval/messages.ftl +++ b/compiler/rustc_const_eval/messages.ftl @@ -402,9 +402,6 @@ const_eval_unallowed_mutable_refs = const_eval_unallowed_op_in_const_context = {$msg} -const_eval_unavailable_target_features_for_fn = - calling a function that requires unavailable target features: {$unavailable_feats} - const_eval_uninhabited_enum_variant_read = read discriminant of an uninhabited enum variant const_eval_uninhabited_enum_variant_written = diff --git a/compiler/rustc_const_eval/src/interpret/call.rs b/compiler/rustc_const_eval/src/interpret/call.rs index 917a2fa7c6dd4..4b21a3a6a3df6 100644 --- a/compiler/rustc_const_eval/src/interpret/call.rs +++ b/compiler/rustc_const_eval/src/interpret/call.rs @@ -311,34 +311,6 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { Ok(()) } - fn check_fn_target_features(&self, instance: ty::Instance<'tcx>) -> InterpResult<'tcx, ()> { - // Calling functions with `#[target_feature]` is not unsafe on WASM, see #84988 - let attrs = self.tcx.codegen_fn_attrs(instance.def_id()); - if !self.tcx.sess.target.is_like_wasm - && attrs - .target_features - .iter() - .any(|feature| !self.tcx.sess.target_features.contains(&feature.name)) - { - throw_ub_custom!( - fluent::const_eval_unavailable_target_features_for_fn, - unavailable_feats = attrs - .target_features - .iter() - .filter(|&feature| !feature.implied - && !self.tcx.sess.target_features.contains(&feature.name)) - .fold(String::new(), |mut s, feature| { - if !s.is_empty() { - s.push_str(", "); - } - s.push_str(feature.name.as_str()); - s - }), - ); - } - Ok(()) - } - /// The main entry point for creating a new stack frame: performs ABI checks and initializes /// arguments. #[instrument(skip(self), level = "trace")] @@ -360,20 +332,18 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { throw_unsup_format!("calling a c-variadic function is not supported"); } - if M::enforce_abi(self) { - if caller_fn_abi.conv != callee_fn_abi.conv { - throw_ub_custom!( - fluent::const_eval_incompatible_calling_conventions, - callee_conv = format!("{:?}", callee_fn_abi.conv), - caller_conv = format!("{:?}", caller_fn_abi.conv), - ) - } + if caller_fn_abi.conv != callee_fn_abi.conv { + throw_ub_custom!( + fluent::const_eval_incompatible_calling_conventions, + callee_conv = format!("{:?}", callee_fn_abi.conv), + caller_conv = format!("{:?}", caller_fn_abi.conv), + ) } // Check that all target features required by the callee (i.e., from // the attribute `#[target_feature(enable = ...)]`) are enabled at // compile time. - self.check_fn_target_features(instance)?; + M::check_fn_target_features(self, instance)?; if !callee_fn_abi.can_unwind { // The callee cannot unwind, so force the `Unreachable` unwind handling. diff --git a/compiler/rustc_const_eval/src/interpret/machine.rs b/compiler/rustc_const_eval/src/interpret/machine.rs index 761ab81e22842..1c71780bb450a 100644 --- a/compiler/rustc_const_eval/src/interpret/machine.rs +++ b/compiler/rustc_const_eval/src/interpret/machine.rs @@ -173,11 +173,6 @@ pub trait Machine<'tcx>: Sized { false } - /// Whether function calls should be [ABI](CallAbi)-checked. - fn enforce_abi(_ecx: &InterpCx<'tcx, Self>) -> bool { - true - } - /// Whether Assert(OverflowNeg) and Assert(Overflow) MIR terminators should actually /// check for overflow. fn ignore_optional_overflow_checks(_ecx: &InterpCx<'tcx, Self>) -> bool; @@ -238,6 +233,13 @@ pub trait Machine<'tcx>: Sized { unwind: mir::UnwindAction, ) -> InterpResult<'tcx, Option>>; + /// Check whether the given function may be executed on the current machine, in terms of the + /// target features is requires. + fn check_fn_target_features( + _ecx: &InterpCx<'tcx, Self>, + _instance: ty::Instance<'tcx>, + ) -> InterpResult<'tcx>; + /// Called to evaluate `Assert` MIR terminators that trigger a panic. fn assert_panic( ecx: &mut InterpCx<'tcx, Self>, @@ -614,6 +616,16 @@ pub macro compile_time_machine(<$tcx: lifetime>) { unreachable!("unwinding cannot happen during compile-time evaluation") } + #[inline(always)] + fn check_fn_target_features( + _ecx: &InterpCx<$tcx, Self>, + _instance: ty::Instance<$tcx>, + ) -> InterpResult<$tcx> { + // For now we don't do any checking here. We can't use `tcx.sess` because that can differ + // between crates, and we need to ensure that const-eval always behaves the same. + Ok(()) + } + #[inline(always)] fn call_extra_fn( _ecx: &mut InterpCx<$tcx, Self>, diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index a7493d48d6a2d..8d4866517f2b2 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -946,16 +946,48 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { ecx.machine.validation == ValidationMode::Deep } - #[inline(always)] - fn enforce_abi(_ecx: &MiriInterpCx<'tcx>) -> bool { - true - } - #[inline(always)] fn ignore_optional_overflow_checks(ecx: &MiriInterpCx<'tcx>) -> bool { !ecx.tcx.sess.overflow_checks() } + fn check_fn_target_features( + ecx: &MiriInterpCx<'tcx>, + instance: ty::Instance<'tcx>, + ) -> InterpResult<'tcx> { + let attrs = ecx.tcx.codegen_fn_attrs(instance.def_id()); + if attrs + .target_features + .iter() + .any(|feature| !ecx.tcx.sess.target_features.contains(&feature.name)) + { + let unavailable = attrs + .target_features + .iter() + .filter(|&feature| { + !feature.implied && !ecx.tcx.sess.target_features.contains(&feature.name) + }) + .fold(String::new(), |mut s, feature| { + if !s.is_empty() { + s.push_str(", "); + } + s.push_str(feature.name.as_str()); + s + }); + let msg = format!( + "calling a function that requires unavailable target features: {unavailable}" + ); + // On WASM, this is not UB, but instead gets rejected during validation of the module + // (see #84988). + if ecx.tcx.sess.target.is_like_wasm { + throw_machine_stop!(TerminationInfo::Abort(msg)); + } else { + throw_ub_format!("{msg}"); + } + } + Ok(()) + } + #[inline(always)] fn find_mir_or_eval_fn( ecx: &mut MiriInterpCx<'tcx>, diff --git a/src/tools/miri/tests/pass/function_calls/target_feature_wasm.rs b/src/tools/miri/tests/panic/target_feature_wasm.rs similarity index 61% rename from src/tools/miri/tests/pass/function_calls/target_feature_wasm.rs rename to src/tools/miri/tests/panic/target_feature_wasm.rs index 5056f32de445e..c67d2983f78fc 100644 --- a/src/tools/miri/tests/pass/function_calls/target_feature_wasm.rs +++ b/src/tools/miri/tests/panic/target_feature_wasm.rs @@ -2,7 +2,9 @@ //@compile-flags: -C target-feature=-simd128 fn main() { - // Calling functions with `#[target_feature]` is not unsound on WASM, see #84988 + // Calling functions with `#[target_feature]` is not unsound on WASM, see #84988. + // But if the compiler actually uses the target feature, it will lead to an error when the module is loaded. + // We emulate this with an "unsupported" error. assert!(!cfg!(target_feature = "simd128")); simd128_fn(); } diff --git a/tests/ui/consts/const-eval/const_fn_target_feature.rs b/tests/ui/consts/const-eval/const_fn_target_feature.rs index ee669abb51e59..8db41ba11c048 100644 --- a/tests/ui/consts/const-eval/const_fn_target_feature.rs +++ b/tests/ui/consts/const-eval/const_fn_target_feature.rs @@ -1,6 +1,7 @@ //@ only-x86_64 // Set the base cpu explicitly, in case the default has been changed. //@ compile-flags: -C target-cpu=x86-64 -C target-feature=+ssse3 +//@ check-pass #![crate_type = "lib"] @@ -9,7 +10,8 @@ const A: () = unsafe { ssse3_fn() }; // error (avx2 not enabled at compile time) const B: () = unsafe { avx2_fn() }; -//~^ ERROR evaluation of constant value failed +// FIXME: currently we do not detect this UB, since we don't want the result of const-eval +// to depend on `tcx.sess` which can differ between crates in a crate graph. #[target_feature(enable = "ssse3")] const unsafe fn ssse3_fn() {} diff --git a/tests/ui/consts/const-eval/const_fn_target_feature.stderr b/tests/ui/consts/const-eval/const_fn_target_feature.stderr deleted file mode 100644 index d3a00b57ebb50..0000000000000 --- a/tests/ui/consts/const-eval/const_fn_target_feature.stderr +++ /dev/null @@ -1,9 +0,0 @@ -error[E0080]: evaluation of constant value failed - --> $DIR/const_fn_target_feature.rs:11:24 - | -LL | const B: () = unsafe { avx2_fn() }; - | ^^^^^^^^^ calling a function that requires unavailable target features: avx2 - -error: aborting due to 1 previous error - -For more information about this error, try `rustc --explain E0080`. diff --git a/tests/ui/consts/const-eval/const_fn_target_feature_wasm.rs b/tests/ui/consts/const-eval/const_fn_target_feature_wasm.rs index deed09e4b2af7..8ddfe61943cb7 100644 --- a/tests/ui/consts/const-eval/const_fn_target_feature_wasm.rs +++ b/tests/ui/consts/const-eval/const_fn_target_feature_wasm.rs @@ -7,7 +7,9 @@ #[cfg(target_feature = "simd128")] compile_error!("simd128 target feature should be disabled"); -// Calling functions with `#[target_feature]` is not unsound on WASM, see #84988 +// Calling functions with `#[target_feature]` is not unsound on WASM, see #84988. +// (It can still lead to a runtime error though so we'd be in our right to abort execution, +// just not to declare it UB.) const A: () = simd128_fn(); #[target_feature(enable = "simd128")]