From 99d30da6a8668181185b24c59e71a521a3c5f388 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 14 Jun 2020 20:20:06 +0200 Subject: [PATCH 1/6] Improve `Instance` docs --- src/librustc_middle/ty/instance.rs | 52 ++++++++++++++++----- src/librustc_trait_selection/traits/util.rs | 2 +- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/librustc_middle/ty/instance.rs b/src/librustc_middle/ty/instance.rs index 1ce079821a22e..d628d6783d5b0 100644 --- a/src/librustc_middle/ty/instance.rs +++ b/src/librustc_middle/ty/instance.rs @@ -9,6 +9,11 @@ use rustc_macros::HashStable; use std::fmt; +/// A monomorphized `InstanceDef`. +/// +/// Monomorphization happens on-the-fly and no monomorphized MIR is ever created. Instead, this type +/// simply couples a potentially generic `InstanceDef` with some substs, and codegen and const eval +/// will do all required substitution as they run. #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)] #[derive(HashStable, Lift)] pub struct Instance<'tcx> { @@ -18,10 +23,26 @@ pub struct Instance<'tcx> { #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable, HashStable)] pub enum InstanceDef<'tcx> { + /// A user-defined callable item. + /// + /// This includes: + /// - `fn` items + /// - closures + /// - generators Item(DefId), + + /// An intrinsic `fn` item (with `"rust-intrinsic"` or `"platform-intrinsic"` ABI). + /// + /// Alongside `Virtual`, this is the only `InstanceDef` that does not have its own callable MIR. + /// Instead, codegen and const eval "magically" evaluate calls to intrinsics purely in the + /// caller. Intrinsic(DefId), - /// `::method` where `method` receives unsizeable `self: Self`. + /// `::method` where `method` receives unsizeable `self: Self` (part of the + /// `unsized_locals` feature). + /// + /// The generated shim will take `Self` via `*mut Self` - conceptually this is `&owned Self` - + /// and dereference the argument to call the original function. VtableShim(DefId), /// `fn()` pointer where the function itself cannot be turned into a pointer. @@ -37,7 +58,8 @@ pub enum InstanceDef<'tcx> { /// (the definition of the function itself). ReifyShim(DefId), - /// `::call_*` + /// `::call_*` (generated `FnTrait` implementation for `fn()` pointers). + /// /// `DefId` is `FnTrait::call_*`. /// /// NB: the (`fn` pointer) type must currently be monomorphic to avoid double substitution @@ -45,19 +67,22 @@ pub enum InstanceDef<'tcx> { // FIXME(#69925) support polymorphic MIR shim bodies properly instead. FnPtrShim(DefId, Ty<'tcx>), - /// `::fn`, "direct calls" of which are implicitly - /// codegen'd as virtual calls. + /// Dynamic dispatch to `::fn`. /// - /// NB: if this is reified to a `fn` pointer, a `ReifyShim` is used - /// (see `ReifyShim` above for more details on that). + /// This `InstanceDef` does not have callable MIR. Calls to `Virtual` instances must be + /// codegen'd as virtual calls through the vtable. + /// + /// If this is reified to a `fn` pointer, a `ReifyShim` is used (see `ReifyShim` above for more + /// details on that). Virtual(DefId, usize), - /// `<[mut closure] as FnOnce>::call_once` - ClosureOnceShim { - call_once: DefId, - }, + /// `<[FnMut closure] as FnOnce>::call_once`. + /// + /// The `DefId` is the ID of the `call_once` method in `FnOnce`. + ClosureOnceShim { call_once: DefId }, /// `core::ptr::drop_in_place::`. + /// /// The `DefId` is for `core::ptr::drop_in_place`. /// The `Option>` is either `Some(T)`, or `None` for empty drop /// glue. @@ -67,7 +92,12 @@ pub enum InstanceDef<'tcx> { // FIXME(#69925) support polymorphic MIR shim bodies properly instead. DropGlue(DefId, Option>), - ///`::clone` shim. + /// Compiler-generated `::clone` implementation. + /// + /// For all types that automatically implement `Copy`, a trivial `Clone` impl is provided too. + /// Additionally, arrays, tuples, and closures get a `Clone` shim even if they aren't `Copy`. + /// + /// The `DefId` is for `Clone::clone`, the `Ty` is the type `T` with the builtin `Clone` impl. /// /// NB: the type must currently be monomorphic to avoid double substitution /// problems with the MIR shim bodies. `Instance::resolve` enforces this. diff --git a/src/librustc_trait_selection/traits/util.rs b/src/librustc_trait_selection/traits/util.rs index 8b5b4128e6672..d3484b8af89fd 100644 --- a/src/librustc_trait_selection/traits/util.rs +++ b/src/librustc_trait_selection/traits/util.rs @@ -302,7 +302,7 @@ pub fn get_vtable_index_of_object_method( ) -> usize { // Count number of methods preceding the one we are selecting and // add them to the total offset. - // Skip over associated types and constants. + // Skip over associated types and constants, as those aren't stored in the vtable. let mut entries = object.vtable_base; for trait_item in tcx.associated_items(object.upcast_trait_ref.def_id()).in_definition_order() { if trait_item.def_id == method_def_id { From 9014df55c65fdb3d89d6273cf0ecdfbec2444cdb Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 14 Jun 2020 20:22:13 +0200 Subject: [PATCH 2/6] validator: print MIR instance on failure --- src/librustc_mir/transform/validate.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 8150c328316cb..625f40cd79206 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -9,7 +9,6 @@ use rustc_middle::{ }, ty::{self, ParamEnv, TyCtxt}, }; -use rustc_span::def_id::DefId; #[derive(Copy, Clone, Debug)] enum EdgeKind { @@ -24,15 +23,14 @@ pub struct Validator { impl<'tcx> MirPass<'tcx> for Validator { fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<'tcx>) { - let def_id = source.def_id(); - let param_env = tcx.param_env(def_id); - TypeChecker { when: &self.when, def_id, body, tcx, param_env }.visit_body(body); + let param_env = tcx.param_env(source.def_id()); + TypeChecker { when: &self.when, source, body, tcx, param_env }.visit_body(body); } } struct TypeChecker<'a, 'tcx> { when: &'a str, - def_id: DefId, + source: MirSource<'tcx>, body: &'a Body<'tcx>, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, @@ -47,7 +45,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { span, &format!( "broken MIR in {:?} ({}) at {:?}:\n{}", - self.def_id, + self.source.instance, self.when, location, msg.as_ref() From 26e17ae8892d0232e8f519e0c79cde44d5fd046b Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 14 Jun 2020 23:01:32 +0200 Subject: [PATCH 3/6] Remove `Adjustment::DerefMove` It does the same thing as `Deref` now --- src/librustc_mir/shim.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index f95fd9b9e90c5..5ee1fc684f021 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -32,13 +32,9 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<' let mut result = match instance { ty::InstanceDef::Item(..) => bug!("item {:?} passed to make_shim", instance), - ty::InstanceDef::VtableShim(def_id) => build_call_shim( - tcx, - instance, - Some(Adjustment::DerefMove), - CallKind::Direct(def_id), - None, - ), + ty::InstanceDef::VtableShim(def_id) => { + build_call_shim(tcx, instance, Some(Adjustment::Deref), CallKind::Direct(def_id), None) + } ty::InstanceDef::FnPtrShim(def_id, ty) => { // FIXME(eddyb) support generating shims for a "shallow type", // e.g. `Foo<_>` or `[_]` instead of requiring a fully monomorphic @@ -136,7 +132,6 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<' enum Adjustment { Identity, Deref, - DerefMove, RefMut, } @@ -701,8 +696,7 @@ fn build_call_shim<'tcx>( let rcvr = rcvr_adjustment.map(|rcvr_adjustment| match rcvr_adjustment { Adjustment::Identity => Operand::Move(rcvr_place()), - Adjustment::Deref => Operand::Move(tcx.mk_place_deref(rcvr_place())), // Can't copy `&mut` - Adjustment::DerefMove => Operand::Move(tcx.mk_place_deref(rcvr_place())), + Adjustment::Deref => Operand::Move(tcx.mk_place_deref(rcvr_place())), Adjustment::RefMut => { // let rcvr = &mut rcvr; let ref_rcvr = local_decls.push( From 58062e1913248a2ad62072f51cc51db00bba66dc Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 14 Jun 2020 23:44:08 +0200 Subject: [PATCH 4/6] shim.rs: improve docs a bit --- src/librustc_mir/shim.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index 5ee1fc684f021..4908e63da8274 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -130,14 +130,28 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<' #[derive(Copy, Clone, Debug, PartialEq)] enum Adjustment { + /// Pass the receiver as-is. Identity, + + /// We get passed `&[mut] self` and call the target with `*self`. + /// + /// This either copies `self` (if `Self: Copy`, eg. for function items), or moves out of it + /// (for `VtableShim`, which effectively is passed `&own Self`). Deref, + + /// We get passed `self: Self` and call the target with `&mut self`. + /// + /// In this case we need to ensure that the `Self` is dropped after the call, as the callee + /// won't do it for us. RefMut, } #[derive(Copy, Clone, Debug, PartialEq)] enum CallKind { + /// Call the `FnPtr` that was passed as the receiver. Indirect, + + /// Call a known `FnDef`. Direct(DefId), } @@ -722,7 +736,10 @@ fn build_call_shim<'tcx>( }); let (callee, mut args) = match call_kind { + // `FnPtr` call has no receiver. Args are untupled below. CallKind::Indirect => (rcvr.unwrap(), vec![]), + + // `FnDef` call with optional receiver. CallKind::Direct(def_id) => { let ty = tcx.type_of(def_id); ( From af97a117e538c5945cb19a4d3a045e509274d9cf Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 14 Jun 2020 23:46:06 +0200 Subject: [PATCH 5/6] shim.rs: call `FnPtr`, not `Self` The `Call` terminator only works with `FnDef` and `FnPtr` types. It happened to work with `Self` so far because it was always substituted with the real type before being used. --- src/librustc_mir/shim.rs | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index 4908e63da8274..92d5b957b0005 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -56,7 +56,7 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<' let sig = tcx.erase_late_bound_regions(&ty.fn_sig(tcx)); let arg_tys = sig.inputs(); - build_call_shim(tcx, instance, Some(adjustment), CallKind::Indirect, Some(arg_tys)) + build_call_shim(tcx, instance, Some(adjustment), CallKind::Indirect(ty), Some(arg_tys)) } // We are generating a call back to our def-id, which the // codegen backend knows to turn to an actual call, be it @@ -147,9 +147,9 @@ enum Adjustment { } #[derive(Copy, Clone, Debug, PartialEq)] -enum CallKind { +enum CallKind<'tcx> { /// Call the `FnPtr` that was passed as the receiver. - Indirect, + Indirect(Ty<'tcx>), /// Call a known `FnDef`. Direct(DefId), @@ -671,7 +671,7 @@ fn build_call_shim<'tcx>( tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>, rcvr_adjustment: Option, - call_kind: CallKind, + call_kind: CallKind<'tcx>, untuple_args: Option<&[Ty<'tcx>]>, ) -> Body<'tcx> { debug!( @@ -684,6 +684,29 @@ fn build_call_shim<'tcx>( let sig = tcx.fn_sig(def_id); let mut sig = tcx.erase_late_bound_regions(&sig); + if let CallKind::Indirect(fnty) = call_kind { + // `sig` determines our local decls, and thus the callee type in the `Call` terminator. This + // can only be an `FnDef` or `FnPtr`, but currently will be `Self` since the types come from + // the implemented `FnX` trait. + + // Apply the opposite adjustment to the MIR input. + let mut inputs_and_output = sig.inputs_and_output.to_vec(); + + // Initial signature is `fn(&? Self, Args) -> Self::Output` where `Args` is a tuple of the + // fn arguments. `Self` may be passed via (im)mutable reference or by-value. + assert_eq!(inputs_and_output.len(), 3); + + // `Self` is always the original fn type `ty`. The MIR call terminator is only defined for + // `FnDef` and `FnPtr` callees, not the `Self` type param. + let self_arg = &mut inputs_and_output[0]; + *self_arg = match rcvr_adjustment.unwrap() { + Adjustment::Identity => fnty, + Adjustment::Deref => tcx.mk_imm_ptr(fnty), + Adjustment::RefMut => tcx.mk_mut_ptr(fnty), + }; + sig.inputs_and_output = tcx.intern_type_list(&inputs_and_output); + } + // FIXME(eddyb) avoid having this snippet both here and in // `Instance::fn_sig` (introduce `InstanceDef::fn_sig`?). if let ty::InstanceDef::VtableShim(..) = instance { @@ -737,7 +760,7 @@ fn build_call_shim<'tcx>( let (callee, mut args) = match call_kind { // `FnPtr` call has no receiver. Args are untupled below. - CallKind::Indirect => (rcvr.unwrap(), vec![]), + CallKind::Indirect(_) => (rcvr.unwrap(), vec![]), // `FnDef` call with optional receiver. CallKind::Direct(def_id) => { From 4cb26ad7a3207ce48341d6a152b7212696bfdc0d Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Mon, 15 Jun 2020 20:54:40 +0200 Subject: [PATCH 6/6] Add test --- src/test/mir-opt/fn-ptr-shim.rs | 15 +++++++++++++++ ...tion-Fn-call.AddMovesForPackedDrops.before.mir | 13 +++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 src/test/mir-opt/fn-ptr-shim.rs create mode 100644 src/test/mir-opt/fn-ptr-shim/rustc.ops-function-Fn-call.AddMovesForPackedDrops.before.mir diff --git a/src/test/mir-opt/fn-ptr-shim.rs b/src/test/mir-opt/fn-ptr-shim.rs new file mode 100644 index 0000000000000..08413c9f6fceb --- /dev/null +++ b/src/test/mir-opt/fn-ptr-shim.rs @@ -0,0 +1,15 @@ +// compile-flags: -Zmir-opt-level=0 -Zvalidate-mir + +// Tests that the `` shim does not create a `Call` terminator with a `Self` callee +// (as only `FnDef` and `FnPtr` callees are allowed in MIR). + +// EMIT_MIR rustc.ops-function-Fn-call.AddMovesForPackedDrops.before.mir +fn main() { + call(noop as fn()); +} + +fn noop() {} + +fn call(f: F) { + f(); +} diff --git a/src/test/mir-opt/fn-ptr-shim/rustc.ops-function-Fn-call.AddMovesForPackedDrops.before.mir b/src/test/mir-opt/fn-ptr-shim/rustc.ops-function-Fn-call.AddMovesForPackedDrops.before.mir new file mode 100644 index 0000000000000..4ecc331afaeb9 --- /dev/null +++ b/src/test/mir-opt/fn-ptr-shim/rustc.ops-function-Fn-call.AddMovesForPackedDrops.before.mir @@ -0,0 +1,13 @@ +// MIR for `std::ops::Fn::call` before AddMovesForPackedDrops + +fn std::ops::Fn::call(_1: *const fn(), _2: Args) -> >::Output { + let mut _0: >::Output; // return place in scope 0 at $SRC_DIR/libcore/ops/function.rs:LL:COL + + bb0: { + _0 = move (*_1)() -> bb1; // scope 0 at $SRC_DIR/libcore/ops/function.rs:LL:COL + } + + bb1: { + return; // scope 0 at $SRC_DIR/libcore/ops/function.rs:LL:COL + } +}