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

Let miri and const eval execute intrinsics' fallback bodies #124293

Merged
merged 3 commits into from
May 4, 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
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/const_eval/dummy_machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ impl<'mir, 'tcx: 'mir> interpret::Machine<'mir, 'tcx> for DummyMachine {
_destination: &interpret::MPlaceTy<'tcx, Self::Provenance>,
_target: Option<BasicBlock>,
_unwind: UnwindAction,
) -> interpret::InterpResult<'tcx> {
) -> interpret::InterpResult<'tcx, Option<ty::Instance<'tcx>>> {
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
unimplemented!()
}

Expand Down
31 changes: 24 additions & 7 deletions compiler/rustc_const_eval/src/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -459,16 +459,26 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
dest: &MPlaceTy<'tcx, Self::Provenance>,
target: Option<mir::BasicBlock>,
_unwind: mir::UnwindAction,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>> {
// Shared intrinsics.
if ecx.emulate_intrinsic(instance, args, dest, target)? {
return Ok(());
return Ok(None);
}
let intrinsic_name = ecx.tcx.item_name(instance.def_id());

// CTFE-specific intrinsics.
let Some(ret) = target else {
throw_unsup_format!("intrinsic `{intrinsic_name}` is not supported at compile-time");
// Handle diverging intrinsics. We can't handle any of them (that are not already
// handled above), but check if there is a fallback body.
if ecx.tcx.intrinsic(instance.def_id()).unwrap().must_be_overridden {
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
throw_unsup_format!(
"intrinsic `{intrinsic_name}` is not supported at compile-time"
);
}
return Ok(Some(ty::Instance {
def: ty::InstanceDef::Item(instance.def_id()),
args: instance.args,
}));
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
};
match intrinsic_name {
sym::ptr_guaranteed_cmp => {
Expand Down Expand Up @@ -536,14 +546,21 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
// not the optimization stage.)
sym::is_val_statically_known => ecx.write_scalar(Scalar::from_bool(false), dest)?,
_ => {
throw_unsup_format!(
"intrinsic `{intrinsic_name}` is not supported at compile-time"
);
// We haven't handled the intrinsic, let's see if we can use a fallback body.
if ecx.tcx.intrinsic(instance.def_id()).unwrap().must_be_overridden {
throw_unsup_format!(
"intrinsic `{intrinsic_name}` is not supported at compile-time"
);
}
return Ok(Some(ty::Instance {
def: ty::InstanceDef::Item(instance.def_id()),
args: instance.args,
}));
}
}

ecx.go_to_block(ret);
Ok(())
Ok(None)
}

fn assert_panic(
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
self.copy_op(&self.project_index(&input, index)?, dest)?;
}
sym::likely | sym::unlikely | sym::black_box => {
sym::black_box => {
// These just return their argument
self.copy_op(&args[0], dest)?;
}
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_const_eval/src/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,14 +216,17 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {

/// Directly process an intrinsic without pushing a stack frame. It is the hook's
/// responsibility to advance the instruction pointer as appropriate.
///
/// Returns `None` if the intrinsic was fully handled.
/// Otherwise, returns an `Instance` of the function that implements the intrinsic.
fn call_intrinsic(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx, Self::Provenance>],
destination: &MPlaceTy<'tcx, Self::Provenance>,
target: Option<mir::BasicBlock>,
unwind: mir::UnwindAction,
) -> InterpResult<'tcx>;
) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>>;

/// Called to evaluate `Assert` MIR terminators that trigger a panic.
fn assert_panic(
Expand Down
18 changes: 16 additions & 2 deletions compiler/rustc_const_eval/src/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -539,14 +539,28 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
ty::InstanceDef::Intrinsic(def_id) => {
assert!(self.tcx.intrinsic(def_id).is_some());
// FIXME: Should `InPlace` arguments be reset to uninit?
M::call_intrinsic(
if let Some(fallback) = M::call_intrinsic(
self,
instance,
&self.copy_fn_args(args),
destination,
target,
unwind,
)
)? {
assert!(!self.tcx.intrinsic(fallback.def_id()).unwrap().must_be_overridden);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only assert here, so backends are forced to choose a way to report the lack of intrinsic implementation, as miri and ctfe differ here in the message we want to send.

assert!(matches!(fallback.def, ty::InstanceDef::Item(_)));
return self.eval_fn_call(
FnVal::Instance(fallback),
(caller_abi, caller_fn_abi),
args,
with_caller_location,
destination,
target,
unwind,
);
} else {
Ok(())
}
}
ty::InstanceDef::VTableShim(..)
| ty::InstanceDef::ReifyShim(..)
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_resolve/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ pub(crate) fn registered_tools(tcx: TyCtxt<'_>, (): ()) -> RegisteredTools {
}
// We implicitly add `rustfmt`, `clippy`, `diagnostic` to known tools,
// but it's not an error to register them explicitly.
let predefined_tools = [sym::clippy, sym::rustfmt, sym::diagnostic];
let predefined_tools = [sym::clippy, sym::rustfmt, sym::diagnostic, sym::miri];
registered_tools.extend(predefined_tools.iter().cloned().map(Ident::with_dummy_span));
registered_tools
}
Expand Down
12 changes: 10 additions & 2 deletions library/core/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,7 @@ pub const unsafe fn assume(b: bool) {
#[unstable(feature = "core_intrinsics", issue = "none")]
#[rustc_intrinsic]
#[rustc_nounwind]
#[cfg_attr(not(bootstrap), miri::intrinsic_fallback_checks_ub)]
pub const fn likely(b: bool) -> bool {
b
}
Expand All @@ -1006,6 +1007,7 @@ pub const fn likely(b: bool) -> bool {
#[unstable(feature = "core_intrinsics", issue = "none")]
#[rustc_intrinsic]
#[rustc_nounwind]
#[cfg_attr(not(bootstrap), miri::intrinsic_fallback_checks_ub)]
pub const fn unlikely(b: bool) -> bool {
b
}
Expand Down Expand Up @@ -2526,6 +2528,7 @@ extern "rust-intrinsic" {
#[rustc_nounwind]
#[rustc_do_not_const_check]
#[inline]
#[cfg_attr(not(bootstrap), miri::intrinsic_fallback_checks_ub)]
pub const fn ptr_guaranteed_cmp<T>(ptr: *const T, other: *const T) -> u8 {
(ptr == other) as u8
}
Expand Down Expand Up @@ -2818,8 +2821,10 @@ pub const fn ub_checks() -> bool {
#[unstable(feature = "core_intrinsics", issue = "none")]
#[rustc_nounwind]
#[rustc_intrinsic]
#[cfg_attr(not(bootstrap), miri::intrinsic_fallback_checks_ub)]
pub const unsafe fn const_allocate(_size: usize, _align: usize) -> *mut u8 {
// const eval overrides this function, but runtime code should always just return null pointers.
// const eval overrides this function, but runtime code for now just returns null pointers.
// See <https://github.com/rust-lang/rust/issues/93935>.
crate::ptr::null_mut()
}

Expand All @@ -2837,7 +2842,10 @@ pub const unsafe fn const_allocate(_size: usize, _align: usize) -> *mut u8 {
#[unstable(feature = "core_intrinsics", issue = "none")]
#[rustc_nounwind]
#[rustc_intrinsic]
pub const unsafe fn const_deallocate(_ptr: *mut u8, _size: usize, _align: usize) {}
#[cfg_attr(not(bootstrap), miri::intrinsic_fallback_checks_ub)]
pub const unsafe fn const_deallocate(_ptr: *mut u8, _size: usize, _align: usize) {
// Runtime NOP
}

/// `ptr` must point to a vtable.
/// The intrinsic will return the size stored in that vtable.
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -986,7 +986,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
dest: &MPlaceTy<'tcx, Provenance>,
ret: Option<mir::BasicBlock>,
unwind: mir::UnwindAction,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>> {
ecx.call_intrinsic(instance, args, dest, ret, unwind)
}

Expand Down
7 changes: 4 additions & 3 deletions src/tools/miri/src/shims/intrinsics/atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,13 @@ pub enum AtomicOp {
impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {}
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
/// Calls the atomic intrinsic `intrinsic`; the `atomic_` prefix has already been removed.
/// Returns `Ok(true)` if the intrinsic was handled.
fn emulate_atomic_intrinsic(
&mut self,
intrinsic_name: &str,
args: &[OpTy<'tcx, Provenance>],
dest: &MPlaceTy<'tcx, Provenance>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, bool> {
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
let this = self.eval_context_mut();

let intrinsic_structure: Vec<_> = intrinsic_name.split('_').collect();
Expand Down Expand Up @@ -113,9 +114,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
this.atomic_rmw_op(args, dest, AtomicOp::Max, rw_ord(ord)?)?;
}

_ => throw_unsup_format!("unimplemented intrinsic: `atomic_{intrinsic_name}`"),
_ => return Ok(false),
}
Ok(())
Ok(true)
}
}

Expand Down
53 changes: 27 additions & 26 deletions src/tools/miri/src/shims/intrinsics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use rustc_middle::{
ty::{self, FloatTy},
};
use rustc_target::abi::Size;
use rustc_span::{sym, Symbol};

use crate::*;
use atomic::EvalContextExt as _;
Expand All @@ -26,12 +27,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
dest: &MPlaceTy<'tcx, Provenance>,
ret: Option<mir::BasicBlock>,
_unwind: mir::UnwindAction,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>> {
let this = self.eval_context_mut();

// See if the core engine can handle this intrinsic.
if this.emulate_intrinsic(instance, args, dest, ret)? {
return Ok(());
return Ok(None);
}
let intrinsic_name = this.tcx.item_name(instance.def_id());
let intrinsic_name = intrinsic_name.as_str();
Expand All @@ -48,32 +49,50 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {

// All remaining supported intrinsics have a return place.
let ret = match ret {
// FIXME: add fallback body support once we actually have a diverging intrinsic with a fallback body
None => throw_unsup_format!("unimplemented (diverging) intrinsic: `{intrinsic_name}`"),
Some(p) => p,
};

// Some intrinsics are special and need the "ret".
match intrinsic_name {
"catch_unwind" => return this.handle_catch_unwind(args, dest, ret),
"catch_unwind" => {
this.handle_catch_unwind(args, dest, ret)?;
return Ok(None);
}
_ => {}
}

// The rest jumps to `ret` immediately.
this.emulate_intrinsic_by_name(intrinsic_name, instance.args, args, dest)?;
if !this.emulate_intrinsic_by_name(intrinsic_name, instance.args, args, dest)? {
// We haven't handled the intrinsic, let's see if we can use a fallback body.
if this.tcx.intrinsic(instance.def_id()).unwrap().must_be_overridden {
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
throw_unsup_format!("unimplemented intrinsic: `{intrinsic_name}`")
}
let intrinsic_fallback_checks_ub = Symbol::intern("intrinsic_fallback_checks_ub");
if this.tcx.get_attrs_by_path(instance.def_id(), &[sym::miri, intrinsic_fallback_checks_ub]).next().is_none() {
throw_unsup_format!("miri can only use intrinsic fallback bodies that check UB. After verifying that `{intrinsic_name}` does so, add the `#[miri::intrinsic_fallback_checks_ub]` attribute to it; also ping @rust-lang/miri when you do that");
}
return Ok(Some(ty::Instance {
def: ty::InstanceDef::Item(instance.def_id()),
args: instance.args,
}))
Copy link
Member

Choose a reason for hiding this comment

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

A few lines above we have "Handle intrinsics without return place" for diverging intrinsics -- shouldn't that also check for a fallback body?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably. Let's add that when we have such an intrinsic with a fallback body

Copy link
Member

Choose a reason for hiding this comment

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

Let's at least add a FIXME then.

}

trace!("{:?}", this.dump_place(&dest.clone().into()));
this.go_to_block(ret);
Ok(())
Ok(None)
}

/// Emulates a Miri-supported intrinsic (not supported by the core engine).
/// Returns `Ok(true)` if the intrinsic was handled.
fn emulate_intrinsic_by_name(
&mut self,
intrinsic_name: &str,
generic_args: ty::GenericArgsRef<'tcx>,
args: &[OpTy<'tcx, Provenance>],
dest: &MPlaceTy<'tcx, Provenance>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, bool> {
let this = self.eval_context_mut();

if let Some(name) = intrinsic_name.strip_prefix("atomic_") {
Expand All @@ -84,24 +103,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
}

match intrinsic_name {
// Miri overwriting CTFE intrinsics.
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
"ptr_guaranteed_cmp" => {
let [left, right] = check_arg_count(args)?;
let left = this.read_immediate(left)?;
let right = this.read_immediate(right)?;
let val = this.wrapping_binary_op(mir::BinOp::Eq, &left, &right)?;
// We're type punning a bool as an u8 here.
this.write_scalar(val.to_scalar(), dest)?;
}
"const_allocate" => {
// For now, for compatibility with the run-time implementation of this, we just return null.
// See <https://github.com/rust-lang/rust/issues/93935>.
this.write_null(dest)?;
}
"const_deallocate" => {
// complete NOP
}

// Raw memory accesses
"volatile_load" => {
let [place] = check_arg_count(args)?;
Expand Down Expand Up @@ -425,9 +426,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
throw_machine_stop!(TerminationInfo::Abort(format!("trace/breakpoint trap")))
}

name => throw_unsup_format!("unimplemented intrinsic: `{name}`"),
_ => return Ok(false),
}

Ok(())
Ok(true)
}
}
7 changes: 4 additions & 3 deletions src/tools/miri/src/shims/intrinsics/simd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@ pub(crate) enum MinMax {
impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {}
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
/// Calls the simd intrinsic `intrinsic`; the `simd_` prefix has already been removed.
/// Returns `Ok(true)` if the intrinsic was handled.
fn emulate_simd_intrinsic(
&mut self,
intrinsic_name: &str,
generic_args: ty::GenericArgsRef<'tcx>,
args: &[OpTy<'tcx, Provenance>],
dest: &MPlaceTy<'tcx, Provenance>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, bool> {
let this = self.eval_context_mut();
match intrinsic_name {
#[rustfmt::skip]
Expand Down Expand Up @@ -743,9 +744,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
}
}

name => throw_unsup_format!("unimplemented intrinsic: `simd_{name}`"),
_ => return Ok(false),
}
Ok(())
Ok(true)
}

fn fminmax_op(
Expand Down
14 changes: 14 additions & 0 deletions src/tools/miri/tests/fail/intrinsic_fallback_checks_ub.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#![feature(rustc_attrs, effects)]

#[rustc_intrinsic]
#[rustc_nounwind]
#[rustc_do_not_const_check]
#[inline]
pub const fn ptr_guaranteed_cmp<T>(ptr: *const T, other: *const T) -> u8 {
(ptr == other) as u8
}

fn main() {
ptr_guaranteed_cmp::<()>(std::ptr::null(), std::ptr::null());
//~^ ERROR: can only use intrinsic fallback bodies that check UB.
}
14 changes: 14 additions & 0 deletions src/tools/miri/tests/fail/intrinsic_fallback_checks_ub.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: unsupported operation: miri can only use intrinsic fallback bodies that check UB. After verifying that `ptr_guaranteed_cmp` does so, add the `#[miri::intrinsic_fallback_checks_ub]` attribute to it; also ping @rust-lang/miri when you do that
--> $DIR/intrinsic_fallback_checks_ub.rs:LL:CC
|
LL | ptr_guaranteed_cmp::<()>(std::ptr::null(), std::ptr::null());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ miri can only use intrinsic fallback bodies that check UB. After verifying that `ptr_guaranteed_cmp` does so, add the `#[miri::intrinsic_fallback_checks_ub]` attribute to it; also ping @rust-lang/miri when you do that
|
= help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support
= note: BACKTRACE:
= note: inside `main` at $DIR/intrinsic_fallback_checks_ub.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

Loading