From 5fe694da7b975551519b8405b55c4a59ffdacecc Mon Sep 17 00:00:00 2001 From: Nathan Corbyn Date: Tue, 9 Jun 2020 19:41:09 +0100 Subject: [PATCH 1/5] Make libcore pass -Zvalidate-mir --- src/librustc_mir/transform/validate.rs | 58 ++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 9 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 8150c328316cb..9adb3d04480db 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -1,15 +1,20 @@ //! Validates the MIR to ensure that invariants are upheld. use super::{MirPass, MirSource}; +use rustc_hir::Constness; +use rustc_hir::lang_items::FnOnceTrait; +use rustc_infer::infer::TyCtxtInferExt; use rustc_middle::mir::visit::Visitor; use rustc_middle::{ mir::{ BasicBlock, Body, Location, Operand, Rvalue, Statement, StatementKind, Terminator, TerminatorKind, }, - ty::{self, ParamEnv, TyCtxt}, + ty::{self, ParamEnv, ToPredicate, Ty, TyCtxt}, }; use rustc_span::def_id::DefId; +use rustc_trait_selection::traits; +use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt; #[derive(Copy, Clone, Debug)] enum EdgeKind { @@ -83,6 +88,48 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { self.fail(location, format!("encountered jump to invalid basic block {:?}", bb)) } } + + fn check_ty_callable(&self, location: Location, ty: Ty<'tcx>) { + if let ty::FnPtr(..) | ty::FnDef(..) = ty.kind { + // We have a `FnPtr` or `FnDef` which is trivially safe to call. + } else { + let fn_once_trait = self.tcx.require_lang_item(FnOnceTrait, None); + // We haven't got a `FnPtr` or `FnDef` but we are still safe to call it if it + // implements `FnOnce` (as `Fn: FnMut` and `FnMut: FnOnce`). + let item_def_id = self + .tcx + .associated_items(fn_once_trait) + .in_definition_order() + .next() + .unwrap() + .def_id; + self.tcx.infer_ctxt().enter(|infcx| { + let trait_ref = ty::TraitRef { + def_id: fn_once_trait, + substs: self.tcx.mk_substs_trait( + ty, + infcx.fresh_substs_for_item(self.body.span, item_def_id), + ), + }; + let predicate = ty::PredicateKind::Trait( + ty::Binder::bind(ty::TraitPredicate { trait_ref }), + Constness::NotConst, + ) + .to_predicate(self.tcx); + let obligation = traits::Obligation::new( + traits::ObligationCause::dummy(), + self.param_env, + predicate, + ); + if !infcx.predicate_may_hold(&obligation) { + self.fail( + location, + format!("encountered non-callable type {} in `Call` terminator", ty), + ); + } + }); + } + } } impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { @@ -151,14 +198,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { } } TerminatorKind::Call { func, destination, cleanup, .. } => { - let func_ty = func.ty(&self.body.local_decls, self.tcx); - match func_ty.kind { - ty::FnPtr(..) | ty::FnDef(..) => {} - _ => self.fail( - location, - format!("encountered non-callable type {} in `Call` terminator", func_ty), - ), - } + self.check_ty_callable(location, &func.ty(&self.body.local_decls, self.tcx)); if let Some((_, target)) = destination { self.check_edge(location, *target, EdgeKind::Normal); } From 1cae892b297109ae7b831a6526c64d0f25dbe029 Mon Sep 17 00:00:00 2001 From: Nathan Corbyn Date: Tue, 9 Jun 2020 21:46:14 +0100 Subject: [PATCH 2/5] Add FIXME --- src/librustc_mir/transform/validate.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index 9adb3d04480db..fe0243c4ae247 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -93,9 +93,11 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { if let ty::FnPtr(..) | ty::FnDef(..) = ty.kind { // We have a `FnPtr` or `FnDef` which is trivially safe to call. } else { - let fn_once_trait = self.tcx.require_lang_item(FnOnceTrait, None); + // FIXME(doctorn): call shims shouldn't reference unnormalized types, so + // this case should be removed. // We haven't got a `FnPtr` or `FnDef` but we are still safe to call it if it // implements `FnOnce` (as `Fn: FnMut` and `FnMut: FnOnce`). + let fn_once_trait = self.tcx.require_lang_item(FnOnceTrait, None); let item_def_id = self .tcx .associated_items(fn_once_trait) From 218eaeeb837a78519c9127f3a50c9ced8603ccc7 Mon Sep 17 00:00:00 2001 From: Nathan Corbyn Date: Wed, 10 Jun 2020 10:56:32 +0100 Subject: [PATCH 3/5] Use `require_lang_item` --- src/librustc_mir/transform/validate.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index fe0243c4ae247..f549245738e09 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -1,8 +1,8 @@ //! Validates the MIR to ensure that invariants are upheld. use super::{MirPass, MirSource}; +use rustc_hir::lang_items::FnOnceTraitLangItem; use rustc_hir::Constness; -use rustc_hir::lang_items::FnOnceTrait; use rustc_infer::infer::TyCtxtInferExt; use rustc_middle::mir::visit::Visitor; use rustc_middle::{ @@ -97,7 +97,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { // this case should be removed. // We haven't got a `FnPtr` or `FnDef` but we are still safe to call it if it // implements `FnOnce` (as `Fn: FnMut` and `FnMut: FnOnce`). - let fn_once_trait = self.tcx.require_lang_item(FnOnceTrait, None); + let fn_once_trait = self.tcx.require_lang_item(FnOnceTraitLangItem, None); let item_def_id = self .tcx .associated_items(fn_once_trait) From 9df2bc6bdba9b3cacf60eacf0622d1bdd715237a Mon Sep 17 00:00:00 2001 From: Nathan Corbyn Date: Sun, 14 Jun 2020 15:04:50 +0100 Subject: [PATCH 4/5] Gate paper to only be applied when validating shims --- src/librustc_mir/transform/validate.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index f549245738e09..f4e6fd76f69c6 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -31,12 +31,16 @@ 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 validating_shim = + if let ty::InstanceDef::Item(_) = source.instance { false } else { true }; + TypeChecker { when: &self.when, def_id, body, tcx, param_env, validating_shim } + .visit_body(body); } } struct TypeChecker<'a, 'tcx> { when: &'a str, + validating_shim: bool, def_id: DefId, body: &'a Body<'tcx>, tcx: TyCtxt<'tcx>, @@ -92,9 +96,9 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { fn check_ty_callable(&self, location: Location, ty: Ty<'tcx>) { if let ty::FnPtr(..) | ty::FnDef(..) = ty.kind { // We have a `FnPtr` or `FnDef` which is trivially safe to call. - } else { - // FIXME(doctorn): call shims shouldn't reference unnormalized types, so - // this case should be removed. + } else if self.validating_shim { + // FIXME(#69925): we shouldn't be special-casing for call-shims as we'd hope they + // have concrete substs by this point. // We haven't got a `FnPtr` or `FnDef` but we are still safe to call it if it // implements `FnOnce` (as `Fn: FnMut` and `FnMut: FnOnce`). let fn_once_trait = self.tcx.require_lang_item(FnOnceTraitLangItem, None); @@ -130,6 +134,11 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { ); } }); + } else { + self.fail( + location, + format!("encountered non-callable type {} in `Call` terminator", ty), + ); } } } From 82fa79a958b098aa7230838b49134c786b828c48 Mon Sep 17 00:00:00 2001 From: Nathan Corbyn Date: Mon, 15 Jun 2020 09:30:37 +0100 Subject: [PATCH 5/5] Update comments and add check for `Self` --- src/librustc_mir/transform/validate.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/transform/validate.rs b/src/librustc_mir/transform/validate.rs index f4e6fd76f69c6..0145d132ac79a 100644 --- a/src/librustc_mir/transform/validate.rs +++ b/src/librustc_mir/transform/validate.rs @@ -96,11 +96,18 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { fn check_ty_callable(&self, location: Location, ty: Ty<'tcx>) { if let ty::FnPtr(..) | ty::FnDef(..) = ty.kind { // We have a `FnPtr` or `FnDef` which is trivially safe to call. - } else if self.validating_shim { + // + // By this point, calls to closures should already have been lowered to calls to + // `Fn*::call*` so we do not consider them callable. + } else if self.validating_shim && ty == self.tcx.types.self_param { // FIXME(#69925): we shouldn't be special-casing for call-shims as we'd hope they // have concrete substs by this point. - // We haven't got a `FnPtr` or `FnDef` but we are still safe to call it if it - // implements `FnOnce` (as `Fn: FnMut` and `FnMut: FnOnce`). + // + // We haven't got a `FnPtr` or `FnDef` but if we're looking at a MIR shim, this could + // be due to a `Self` type still hanging about. To avoid rejecting these shims we + // any type in MIR shims as callable so long as: + // 1. it's `Self` + // 2. it implements `FnOnce` let fn_once_trait = self.tcx.require_lang_item(FnOnceTraitLangItem, None); let item_def_id = self .tcx @@ -130,7 +137,11 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { if !infcx.predicate_may_hold(&obligation) { self.fail( location, - format!("encountered non-callable type {} in `Call` terminator", ty), + format!( + "encountered {} in `Call` terminator of shim \ + which does not implement `FnOnce`", + ty, + ), ); } });