Skip to content

Commit

Permalink
Rollup merge of rust-lang#129613 - RalfJung:interpret-target-feat, r=…
Browse files Browse the repository at this point in the history
…saethlin

interpret: do not make const-eval query result depend on tcx.sess

The check against calling functions with missing target features uses `tcx.sess` to determine which target features are available. However, this can differ between different crates in a crate graph, so the same const-eval query can come to different conclusions about whether a constant evaluates successfully or not -- which is bad, we should consistently get the same result everywhere.
  • Loading branch information
matthiaskrgr authored Aug 28, 2024
2 parents 3456b1d + 7a290fc commit 39e840f
Show file tree
Hide file tree
Showing 8 changed files with 70 additions and 62 deletions.
3 changes: 0 additions & 3 deletions compiler/rustc_const_eval/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
44 changes: 7 additions & 37 deletions compiler/rustc_const_eval/src/interpret/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand All @@ -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.
Expand Down
22 changes: 17 additions & 5 deletions compiler/rustc_const_eval/src/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -238,6 +233,13 @@ pub trait Machine<'tcx>: Sized {
unwind: mir::UnwindAction,
) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>>;

/// 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>,
Expand Down Expand Up @@ -617,6 +619,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>,
Expand Down
42 changes: 37 additions & 5 deletions src/tools/miri/src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
4 changes: 3 additions & 1 deletion tests/ui/consts/const-eval/const_fn_target_feature.rs
Original file line number Diff line number Diff line change
@@ -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"]

Expand All @@ -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() {}
Expand Down
9 changes: 0 additions & 9 deletions tests/ui/consts/const-eval/const_fn_target_feature.stderr

This file was deleted.

4 changes: 3 additions & 1 deletion tests/ui/consts/const-eval/const_fn_target_feature_wasm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down

0 comments on commit 39e840f

Please sign in to comment.