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

Miri engine: support extra function (pointer) values #62245

Merged
merged 8 commits into from
Jul 6, 2019
11 changes: 11 additions & 0 deletions src/librustc_mir/const_eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ impl interpret::MayLeak for ! {
impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, 'tcx> {
type MemoryKinds = !;
type PointerTag = ();
type ExtraFnVal = !;

type FrameExtra = ();
type MemoryExtra = ();
Expand Down Expand Up @@ -370,6 +371,16 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
}))
}

fn call_extra_fn(
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
fn_val: !,
Copy link
Member

Choose a reason for hiding this comment

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

So, my prior understanding was that the never (!) type is never realized (and thus is the "return type" of functions that don't return, like panic!), so I'm confused about how we can use it as a function argument? If we can't get a value of !, then how can we call this function? And if we can't call this function, what is it for?

(really sorry, I told you I wasn't the most qualified reviewer 😰 )

Copy link
Member Author

Choose a reason for hiding this comment

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

If we can't get a value of !, then how can we call this function? And if we can't call this function, what is it for?

We can't call this function indeed. :)
It's a trait method, and the type of fn_val is an associated type. So there might be (and, in fact, there are) other implementations of the trait that pick a non-empty type, and then call_extra_fn takes that type. For example Miri instantiates this trait, and there call_extra_fn looks like

    fn call_extra_fn(
        ecx: &mut InterpretCx<'mir, 'tcx, Self>,
        fn_val: Dlsym,
        args: &[OpTy<'tcx, Tag>],
        dest: Option<PlaceTy<'tcx, Tag>>,
        ret: Option<mir::BasicBlock>,
    ) -> InterpResult<'tcx>

Notice the Dlsym instead of the !.

By using ! in the const_eval instance, I basically opt-out of the ability (that the Machine trait offers) to have "other" function values, and because there are no "other" function values, call_extra_fn does not have to do anything. This surfaces in the type of the method as you said; so we can implement the method trivially without even panicking or so, just by matching on fn_val to "prove" to the compiler that we are in unreachable code.

Copy link
Member Author

@RalfJung RalfJung Jul 5, 2019

Choose a reason for hiding this comment

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

(really sorry, I told you I wasn't the most qualified reviewer cold_sweat )

We could ask @eddyb to also take a look if you want -- but it's hard to get a time slice from his scheduler. ;)

_args: &[OpTy<'tcx>],
_dest: Option<PlaceTy<'tcx>>,
_ret: Option<mir::BasicBlock>,
) -> InterpResult<'tcx> {
match fn_val {}
Copy link
Member

Choose a reason for hiding this comment

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

I think @nikomatsakis or @canndrew brought up the idea of omitting trait methods when they're uncallable due to uninhabited arguments (and presumably they could be null in the vtable and direct calls can be replaced with unreachable).

But for now this seems fine.

}

fn call_intrinsic(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
instance: ty::Instance<'tcx>,
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_mir/interpret/cast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use rustc::mir::interpret::{
};
use rustc::mir::CastKind;

use super::{InterpCx, Machine, PlaceTy, OpTy, Immediate};
use super::{InterpCx, Machine, PlaceTy, OpTy, Immediate, FnVal};

impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
fn type_is_fat_ptr(&self, ty: Ty<'tcx>) -> bool {
Expand Down Expand Up @@ -86,7 +86,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
def_id,
substs,
).ok_or_else(|| InterpError::TooGeneric.into());
let fn_ptr = self.memory.create_fn_alloc(instance?);
let fn_ptr = self.memory.create_fn_alloc(FnVal::Instance(instance?));
self.write_scalar(Scalar::Ptr(fn_ptr.into()), dest)?;
}
_ => bug!("reify fn pointer on {:?}", src.layout.ty),
Expand Down Expand Up @@ -115,7 +115,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
substs,
ty::ClosureKind::FnOnce,
);
let fn_ptr = self.memory.create_fn_alloc(instance);
let fn_ptr = self.memory.create_fn_alloc(FnVal::Instance(instance));
let val = Immediate::Scalar(Scalar::Ptr(fn_ptr.into()).into());
self.write_immediate(val, dest)?;
}
Expand Down
15 changes: 15 additions & 0 deletions src/librustc_mir/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ pub trait Machine<'mir, 'tcx>: Sized {
/// The `default()` is used for pointers to consts, statics, vtables and functions.
type PointerTag: ::std::fmt::Debug + Copy + Eq + Hash + 'static;

/// Machines can define extra (non-instance) things that represent values of function pointers.
/// For example, Miri uses this to return a fucntion pointer from `dlsym`
/// that can later be called to execute the right thing.
type ExtraFnVal: ::std::fmt::Debug + Copy;

/// Extra data stored in every call frame.
type FrameExtra;

Expand Down Expand Up @@ -119,6 +124,16 @@ pub trait Machine<'mir, 'tcx>: Sized {
ret: Option<mir::BasicBlock>,
) -> InterpResult<'tcx, Option<&'mir mir::Body<'tcx>>>;

/// Execute `fn_val`. it is the hook's responsibility to advance the instruction
/// pointer as appropriate.
fn call_extra_fn(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
fn_val: Self::ExtraFnVal,
args: &[OpTy<'tcx, Self::PointerTag>],
dest: Option<PlaceTy<'tcx, Self::PointerTag>>,
ret: Option<mir::BasicBlock>,
) -> InterpResult<'tcx>;

/// Directly process an intrinsic without pushing a stack frame.
/// If this returns successfully, the engine will take care of jumping to the next block.
fn call_intrinsic(
Expand Down
120 changes: 84 additions & 36 deletions src/librustc_mir/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,26 @@ pub enum AllocCheck {
MaybeDead,
}

/// The value of a function pointer.
#[derive(Debug, Copy, Clone)]
pub enum FnVal<'tcx, Other> {
Instance(Instance<'tcx>),
Other(Other),
}

impl<'tcx, Other> FnVal<'tcx, Other> {
pub fn as_instance(self) -> InterpResult<'tcx, Instance<'tcx>> {
match self {
FnVal::Instance(instance) =>
Ok(instance),
FnVal::Other(_) =>
err!(MachineError(
format!("Expected instance function pointer, got 'other' pointer")
)),
}
}
}

// `Memory` has to depend on the `Machine` because some of its operations
// (e.g., `get`) call a `Machine` hook.
pub struct Memory<'mir, 'tcx, M: Machine<'mir, 'tcx>> {
Expand All @@ -69,16 +89,20 @@ pub struct Memory<'mir, 'tcx, M: Machine<'mir, 'tcx>> {
// FIXME: this should not be public, but interning currently needs access to it
pub(super) alloc_map: M::MemoryMap,

/// Map for "extra" function pointers.
extra_fn_ptr_map: FxHashMap<AllocId, M::ExtraFnVal>,

/// To be able to compare pointers with NULL, and to check alignment for accesses
/// to ZSTs (where pointers may dangle), we keep track of the size even for allocations
/// that do not exist any more.
// FIXME: this should not be public, but interning currently needs access to it
pub(super) dead_alloc_map: FxHashMap<AllocId, (Size, Align)>,

/// Extra data added by the machine.
pub extra: M::MemoryExtra,

/// Lets us implement `HasDataLayout`, which is awfully convenient.
pub(super) tcx: TyCtxtAt<'tcx>,
pub tcx: TyCtxtAt<'tcx>,
}

impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> HasDataLayout for Memory<'mir, 'tcx, M> {
Expand All @@ -98,6 +122,7 @@ where
fn clone(&self) -> Self {
Memory {
alloc_map: self.alloc_map.clone(),
extra_fn_ptr_map: self.extra_fn_ptr_map.clone(),
dead_alloc_map: self.dead_alloc_map.clone(),
extra: (),
tcx: self.tcx,
Expand All @@ -109,6 +134,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
pub fn new(tcx: TyCtxtAt<'tcx>) -> Self {
Memory {
alloc_map: M::MemoryMap::default(),
extra_fn_ptr_map: FxHashMap::default(),
dead_alloc_map: FxHashMap::default(),
extra: M::MemoryExtra::default(),
tcx,
Expand All @@ -120,8 +146,21 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
ptr.with_tag(M::tag_static_base_pointer(ptr.alloc_id, &self))
}

pub fn create_fn_alloc(&mut self, instance: Instance<'tcx>) -> Pointer<M::PointerTag> {
let id = self.tcx.alloc_map.lock().create_fn_alloc(instance);
pub fn create_fn_alloc(
&mut self,
fn_val: FnVal<'tcx, M::ExtraFnVal>,
) -> Pointer<M::PointerTag>
{
let id = match fn_val {
FnVal::Instance(instance) => self.tcx.alloc_map.lock().create_fn_alloc(instance),
FnVal::Other(extra) => {
// FIXME(RalfJung): Should we have a cache here?
let id = self.tcx.alloc_map.lock().reserve();
let old = self.extra_fn_ptr_map.insert(id, extra);
assert!(old.is_none());
id
}
};
self.tag_static_base_pointer(Pointer::from(id))
}

Expand Down Expand Up @@ -495,56 +534,65 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
id: AllocId,
liveness: AllocCheck,
) -> InterpResult<'static, (Size, Align)> {
// Regular allocations.
if let Ok(alloc) = self.get(id) {
return Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align));
}
// can't do this in the match argument, we may get cycle errors since the lock would get
// dropped after the match.
// Function pointers.
if let Ok(_) = self.get_fn_alloc(id) {
return if let AllocCheck::Dereferencable = liveness {
// The caller requested no function pointers.
err!(DerefFunctionPointer)
} else {
Ok((Size::ZERO, Align::from_bytes(1).unwrap()))
};
}
// Foreign statics.
// Can't do this in the match argument, we may get cycle errors since the lock would
// be held throughout the match.
let alloc = self.tcx.alloc_map.lock().get(id);
// Could also be a fn ptr or extern static
match alloc {
Some(GlobalAlloc::Function(..)) => {
if let AllocCheck::Dereferencable = liveness {
// The caller requested no function pointers.
err!(DerefFunctionPointer)
} else {
Ok((Size::ZERO, Align::from_bytes(1).unwrap()))
}
}
// `self.get` would also work, but can cause cycles if a static refers to itself
Some(GlobalAlloc::Static(did)) => {
// The only way `get` couldn't have worked here is if this is an extern static
assert!(self.tcx.is_foreign_item(did));
// Use size and align of the type
let ty = self.tcx.type_of(did);
let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap();
Ok((layout.size, layout.align.abi))
return Ok((layout.size, layout.align.abi));
}
_ => {
if let Ok(alloc) = self.get(id) {
Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align))
}
else if let AllocCheck::MaybeDead = liveness {
// Deallocated pointers are allowed, we should be able to find
// them in the map.
Ok(*self.dead_alloc_map.get(&id)
.expect("deallocated pointers should all be recorded in `dead_alloc_map`"))
} else {
err!(DanglingPointerDeref)
}
},
_ => {}
}
// The rest must be dead.
if let AllocCheck::MaybeDead = liveness {
// Deallocated pointers are allowed, we should be able to find
// them in the map.
Ok(*self.dead_alloc_map.get(&id)
.expect("deallocated pointers should all be recorded in `dead_alloc_map`"))
} else {
err!(DanglingPointerDeref)
}
}

pub fn get_fn(&self, ptr: Pointer<M::PointerTag>) -> InterpResult<'tcx, Instance<'tcx>> {
fn get_fn_alloc(&self, id: AllocId) -> InterpResult<'tcx, FnVal<'tcx, M::ExtraFnVal>> {
trace!("reading fn ptr: {}", id);
if let Some(extra) = self.extra_fn_ptr_map.get(&id) {
Ok(FnVal::Other(*extra))
} else {
match self.tcx.alloc_map.lock().get(id) {
Some(GlobalAlloc::Function(instance)) => Ok(FnVal::Instance(instance)),
_ => Err(InterpError::ExecuteMemory.into()),
}
}
}

pub fn get_fn(
&self,
ptr: Scalar<M::PointerTag>,
) -> InterpResult<'tcx, FnVal<'tcx, M::ExtraFnVal>> {
let ptr = self.force_ptr(ptr)?; // We definitely need a pointer value.
if ptr.offset.bytes() != 0 {
return err!(InvalidFunctionPointer);
}
trace!("reading fn ptr: {}", ptr.alloc_id);
match self.tcx.alloc_map.lock().get(ptr.alloc_id) {
Some(GlobalAlloc::Function(instance)) => Ok(instance),
_ => Err(InterpError::ExecuteMemory.into()),
}
self.get_fn_alloc(ptr.alloc_id)
}

pub fn mark_immutable(&mut self, id: AllocId) -> InterpResult<'tcx> {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub use self::eval_context::{

pub use self::place::{Place, PlaceTy, MemPlace, MPlaceTy};

pub use self::memory::{Memory, MemoryKind, AllocCheck};
pub use self::memory::{Memory, MemoryKind, AllocCheck, FnVal};

pub use self::machine::{Machine, AllocMap, MayLeak};

Expand Down
35 changes: 21 additions & 14 deletions src/librustc_mir/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ use rustc::ty::layout::{self, TyLayout, LayoutOf};
use syntax::source_map::Span;
use rustc_target::spec::abi::Abi;

use rustc::mir::interpret::{InterpResult, PointerArithmetic, InterpError, Scalar};
use super::{
InterpCx, Machine, Immediate, OpTy, ImmTy, PlaceTy, MPlaceTy, StackPopCleanup
InterpResult, PointerArithmetic, InterpError, Scalar,
InterpCx, Machine, Immediate, OpTy, ImmTy, PlaceTy, MPlaceTy, StackPopCleanup, FnVal,
};

impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Expand Down Expand Up @@ -76,16 +76,16 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
};

let func = self.eval_operand(func, None)?;
let (fn_def, abi) = match func.layout.ty.sty {
let (fn_val, abi) = match func.layout.ty.sty {
ty::FnPtr(sig) => {
let caller_abi = sig.abi();
let fn_ptr = self.force_ptr(self.read_scalar(func)?.not_undef()?)?;
let instance = self.memory.get_fn(fn_ptr)?;
(instance, caller_abi)
let fn_ptr = self.read_scalar(func)?.not_undef()?;
let fn_val = self.memory.get_fn(fn_ptr)?;
(fn_val, caller_abi)
}
ty::FnDef(def_id, substs) => {
let sig = func.layout.ty.fn_sig(*self.tcx);
(self.resolve(def_id, substs)?, sig.abi())
(FnVal::Instance(self.resolve(def_id, substs)?), sig.abi())
},
_ => {
let msg = format!("can't handle callee of type {:?}", func.layout.ty);
Expand All @@ -94,7 +94,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
};
let args = self.eval_operands(args)?;
self.eval_fn_call(
fn_def,
fn_val,
terminator.source_info.span,
abi,
&args[..],
Expand Down Expand Up @@ -228,14 +228,21 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// Call this function -- pushing the stack frame and initializing the arguments.
fn eval_fn_call(
&mut self,
instance: ty::Instance<'tcx>,
fn_val: FnVal<'tcx, M::ExtraFnVal>,
span: Span,
caller_abi: Abi,
args: &[OpTy<'tcx, M::PointerTag>],
dest: Option<PlaceTy<'tcx, M::PointerTag>>,
ret: Option<mir::BasicBlock>,
) -> InterpResult<'tcx> {
trace!("eval_fn_call: {:#?}", instance);
trace!("eval_fn_call: {:#?}", fn_val);

let instance = match fn_val {
FnVal::Instance(instance) => instance,
FnVal::Other(extra) => {
return M::call_extra_fn(self, extra, args, dest, ret);
}
};

match instance.def {
ty::InstanceDef::Intrinsic(..) => {
Expand Down Expand Up @@ -431,8 +438,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.tcx.data_layout.pointer_align.abi,
)?.expect("cannot be a ZST");
let fn_ptr = self.memory.get(vtable_slot.alloc_id)?
.read_ptr_sized(self, vtable_slot)?.to_ptr()?;
let instance = self.memory.get_fn(fn_ptr)?;
.read_ptr_sized(self, vtable_slot)?.not_undef()?;
let drop_fn = self.memory.get_fn(fn_ptr)?;

// `*mut receiver_place.layout.ty` is almost the layout that we
// want for args[0]: We have to project to field 0 because we want
Expand All @@ -447,7 +454,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
});
trace!("Patched self operand to {:#?}", args[0]);
// recurse with concrete function
self.eval_fn_call(instance, span, caller_abi, &args, dest, ret)
self.eval_fn_call(drop_fn, span, caller_abi, &args, dest, ret)
}
}
}
Expand Down Expand Up @@ -482,7 +489,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let dest = MPlaceTy::dangling(self.layout_of(ty)?, self);

self.eval_fn_call(
instance,
FnVal::Instance(instance),
span,
Abi::Rust,
&[arg.into()],
Expand Down
Loading