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

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

Merged
merged 1 commit into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
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 @@ -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>,
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.
Comment on lines +13 to +14
Copy link
Member

Choose a reason for hiding this comment

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

How could we fix this? Wouldn't we have to make the const ABI the same across the entire crate graph? And since we don't have a global coordination, wouldn't we have to ban everything except the minimum target ABI?

Copy link
Member Author

@RalfJung RalfJung Aug 26, 2024

Choose a reason for hiding this comment

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

We could also just say that this is not UB during const. 🤷 The vendor intrinsics this lets you call are anyway all non-const, and very unlikely to ever be const.

Copy link
Member

Choose a reason for hiding this comment

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

and very unlikely to ever be const.

Are we sure libs/libs-api knows that we expect that?

Copy link
Member Author

@RalfJung RalfJung Aug 27, 2024

Choose a reason for hiding this comment

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

I don't know... at least I don't recall anyone suggesting we make them const. There's also a lot of them.

Portable SIMD APIs are much more likely to be implemented in const.


#[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
Loading