From ee922d47f2e6386be4f69346ab554c97db502cc0 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 2 Jan 2020 00:42:31 -0500 Subject: [PATCH 1/7] Fix ICE involving calling `Instance.ty` during const evaluation Fixes #67639 `Instance.ty` assumes that we are in a fully monomorphic context (e.g. codegen), and can therefore use an empty `ParamEnv` when performing normalization. Howver, the MIR constant evaluator code ends up calling `Instance.ty` as a result of us attemptign to 'speculatively' const-evaluate generic functions during const propagation. As a result, we may end up with projections involving type parameters (e.g. ::Bar>) in the type we are trying to normalize. Normalization expects us to have proper predicates in the `ParamEnv` for such projections, and will ICE if we don't. This commit adds a new method `Instance.ty_env`, which takes a `ParamEnv` for use during normalization. The MIR const-evaluator code is changed to use this method, passing in the proper `ParamEnv` for the context at hand. --- src/librustc/ty/instance.rs | 28 +++++++++++++++ src/librustc_mir/const_eval/eval_queries.rs | 2 +- src/librustc_mir/interpret/terminator.rs | 2 +- .../ui/mir/issue-67639-normalization-ice.rs | 34 +++++++++++++++++++ 4 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 src/test/ui/mir/issue-67639-normalization-ice.rs diff --git a/src/librustc/ty/instance.rs b/src/librustc/ty/instance.rs index a7e716ad7b730..8d4717384f754 100644 --- a/src/librustc/ty/instance.rs +++ b/src/librustc/ty/instance.rs @@ -62,10 +62,38 @@ pub enum InstanceDef<'tcx> { } impl<'tcx> Instance<'tcx> { + /// Returns the `Ty` corresponding to this `Instance`, + /// with generic substitutions applied and lifetimes erased. + /// + /// This method can only be called when the 'substs' for this Instance + /// are fully monomorphic (no `ty::Param`'s are present). + /// This is usually the case (e.g. during codegen). + /// However, during constant evaluation, we may want + /// to try to resolve a `Instance` using generic parameters + /// (e.g. when we are attempting to to do const-propagation). + /// In this case, `Instace.ty_env` should be used to provide + /// the `ParamEnv` for our generic context. pub fn ty(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx> { let ty = tcx.type_of(self.def.def_id()); + // There shouldn't be any params - if there are, then + // Instance.ty_env should have been used to provide the proper + // ParamEnv + if self.substs.has_param_types() { + panic!( + "Instance.ty called for type {:?} with projections in substs: {:?}", + ty, self.substs + ); + } tcx.subst_and_normalize_erasing_regions(self.substs, ty::ParamEnv::reveal_all(), &ty) } + + /// Like `Instance.ty`, but allows a `ParamEnv` to be specified for use during + /// normalization. This method is only really useful during constant evaluation, + /// where we are dealing with potentially generic types. + pub fn ty_env(&self, tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>) -> Ty<'tcx> { + let ty = tcx.type_of(self.def.def_id()); + tcx.subst_and_normalize_erasing_regions(self.substs, param_env, &ty) + } } impl<'tcx> InstanceDef<'tcx> { diff --git a/src/librustc_mir/const_eval/eval_queries.rs b/src/librustc_mir/const_eval/eval_queries.rs index d55620f657ba9..46e76512d35fc 100644 --- a/src/librustc_mir/const_eval/eval_queries.rs +++ b/src/librustc_mir/const_eval/eval_queries.rs @@ -221,7 +221,7 @@ pub fn const_eval_validated_provider<'tcx>( // We call `const_eval` for zero arg intrinsics, too, in order to cache their value. // Catch such calls and evaluate them instead of trying to load a constant's MIR. if let ty::InstanceDef::Intrinsic(def_id) = key.value.instance.def { - let ty = key.value.instance.ty(tcx); + let ty = key.value.instance.ty_env(tcx, key.param_env); let substs = match ty.kind { ty::FnDef(_, substs) => substs, _ => bug!("intrinsic with type {:?}", ty), diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index 032062d636040..a28bb539fd070 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -204,7 +204,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // ABI check { let callee_abi = { - let instance_ty = instance.ty(*self.tcx); + let instance_ty = instance.ty_env(*self.tcx, self.param_env); match instance_ty.kind { ty::FnDef(..) => instance_ty.fn_sig(*self.tcx).abi(), ty::Closure(..) => Abi::RustCall, diff --git a/src/test/ui/mir/issue-67639-normalization-ice.rs b/src/test/ui/mir/issue-67639-normalization-ice.rs new file mode 100644 index 0000000000000..21851a725254f --- /dev/null +++ b/src/test/ui/mir/issue-67639-normalization-ice.rs @@ -0,0 +1,34 @@ +// compile-flags: -Z mir-opt-level=3 +// build-pass + +// This used to ICE in const-prop due +// to an empty ParamEnv being used during normalization +// of a generic type + + +fn main() { + join_all::(); +} + +trait Foo { + type Item; +} + +impl Foo for u32 { + type Item = u8; +} + +trait Bar { + type Item2; +} + +impl Bar for u8 { + type Item2 = u64; +} + +fn join_all() +where I: Foo, + I::Item: Bar +{ + Vec::<::Item2>::new(); // ICE occurs processing this line +} From d41f9dd5dd364c96745b4633a497d3697b9792f4 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 2 Jan 2020 20:38:54 -0500 Subject: [PATCH 2/7] Change 'panic!' to 'bug!' Co-Authored-By: Wesley Wiser --- src/librustc/ty/instance.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc/ty/instance.rs b/src/librustc/ty/instance.rs index 8d4717384f754..dbbd798538ec8 100644 --- a/src/librustc/ty/instance.rs +++ b/src/librustc/ty/instance.rs @@ -79,7 +79,7 @@ impl<'tcx> Instance<'tcx> { // Instance.ty_env should have been used to provide the proper // ParamEnv if self.substs.has_param_types() { - panic!( + bug!( "Instance.ty called for type {:?} with projections in substs: {:?}", ty, self.substs ); From 71d163b53c1a02759ff1765ad6fc9a8e8a9363c7 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 2 Jan 2020 22:27:26 -0500 Subject: [PATCH 3/7] Run rustfmt --- src/librustc/ty/instance.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc/ty/instance.rs b/src/librustc/ty/instance.rs index dbbd798538ec8..009b2af206715 100644 --- a/src/librustc/ty/instance.rs +++ b/src/librustc/ty/instance.rs @@ -81,7 +81,8 @@ impl<'tcx> Instance<'tcx> { if self.substs.has_param_types() { bug!( "Instance.ty called for type {:?} with projections in substs: {:?}", - ty, self.substs + ty, + self.substs ); } tcx.subst_and_normalize_erasing_regions(self.substs, ty::ParamEnv::reveal_all(), &ty) From 464b58ca11d33e742f5d86c8bdad185e7b273e0d Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 2 Jan 2020 23:03:05 -0500 Subject: [PATCH 4/7] s/projections/params/ --- src/librustc/ty/instance.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc/ty/instance.rs b/src/librustc/ty/instance.rs index 009b2af206715..e92ee2f8de5a9 100644 --- a/src/librustc/ty/instance.rs +++ b/src/librustc/ty/instance.rs @@ -80,7 +80,7 @@ impl<'tcx> Instance<'tcx> { // ParamEnv if self.substs.has_param_types() { bug!( - "Instance.ty called for type {:?} with projections in substs: {:?}", + "Instance.ty called for type {:?} with params in substs: {:?}", ty, self.substs ); From 33caf0b61fe241b6c0e24240ab243e1240b0e04f Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 5 Jan 2020 14:59:40 -0500 Subject: [PATCH 5/7] Rename Instance.ty to Instance.monomorphic_ty --- src/librustc/ty/instance.rs | 8 ++------ src/librustc/ty/layout.rs | 2 +- src/librustc_codegen_llvm/callee.rs | 2 +- src/librustc_codegen_llvm/consts.rs | 4 ++-- src/librustc_codegen_llvm/debuginfo/metadata.rs | 2 +- src/librustc_codegen_llvm/intrinsic.rs | 2 +- src/librustc_codegen_llvm/mono_item.rs | 2 +- src/librustc_mir/interpret/traits.rs | 2 +- src/librustc_mir/monomorphize/collector.rs | 5 +++-- 9 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/librustc/ty/instance.rs b/src/librustc/ty/instance.rs index e92ee2f8de5a9..f2813f0403577 100644 --- a/src/librustc/ty/instance.rs +++ b/src/librustc/ty/instance.rs @@ -73,17 +73,13 @@ impl<'tcx> Instance<'tcx> { /// (e.g. when we are attempting to to do const-propagation). /// In this case, `Instace.ty_env` should be used to provide /// the `ParamEnv` for our generic context. - pub fn ty(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx> { + pub fn monomorphic_ty(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx> { let ty = tcx.type_of(self.def.def_id()); // There shouldn't be any params - if there are, then // Instance.ty_env should have been used to provide the proper // ParamEnv if self.substs.has_param_types() { - bug!( - "Instance.ty called for type {:?} with params in substs: {:?}", - ty, - self.substs - ); + bug!("Instance.ty called for type {:?} with params in substs: {:?}", ty, self.substs); } tcx.subst_and_normalize_erasing_regions(self.substs, ty::ParamEnv::reveal_all(), &ty) } diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 63345f828e09c..5f599034e7d4d 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -2301,7 +2301,7 @@ impl<'tcx> ty::Instance<'tcx> { // or should go through `FnAbi` instead, to avoid losing any // adjustments `FnAbi::of_instance` might be performing. fn fn_sig_for_fn_abi(&self, tcx: TyCtxt<'tcx>) -> ty::PolyFnSig<'tcx> { - let ty = self.ty(tcx); + let ty = self.monomorphic_ty(tcx); match ty.kind { ty::FnDef(..) | // Shims currently have type FnPtr. Not sure this should remain. diff --git a/src/librustc_codegen_llvm/callee.rs b/src/librustc_codegen_llvm/callee.rs index 7be179a2098c1..78dd6fc8ffe75 100644 --- a/src/librustc_codegen_llvm/callee.rs +++ b/src/librustc_codegen_llvm/callee.rs @@ -36,7 +36,7 @@ pub fn get_fn(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<'tcx>) -> &'ll Value } let sym = tcx.symbol_name(instance).name.as_str(); - debug!("get_fn({:?}: {:?}) => {}", instance, instance.ty(cx.tcx()), sym); + debug!("get_fn({:?}: {:?}) => {}", instance, instance.monomorphic_ty(cx.tcx()), sym); let fn_abi = FnAbi::of_instance(cx, instance, &[]); diff --git a/src/librustc_codegen_llvm/consts.rs b/src/librustc_codegen_llvm/consts.rs index 4b4fbd0e1ad53..bccf3f5735cc8 100644 --- a/src/librustc_codegen_llvm/consts.rs +++ b/src/librustc_codegen_llvm/consts.rs @@ -204,7 +204,7 @@ impl CodegenCx<'ll, 'tcx> { def_id ); - let ty = instance.ty(self.tcx); + let ty = instance.monomorphic_ty(self.tcx); let sym = self.tcx.symbol_name(instance).name; debug!("get_static: sym={} instance={:?}", sym, instance); @@ -361,7 +361,7 @@ impl StaticMethods for CodegenCx<'ll, 'tcx> { }; let instance = Instance::mono(self.tcx, def_id); - let ty = instance.ty(self.tcx); + let ty = instance.monomorphic_ty(self.tcx); let llty = self.layout_of(ty).llvm_type(self); let g = if val_llty == llty { g diff --git a/src/librustc_codegen_llvm/debuginfo/metadata.rs b/src/librustc_codegen_llvm/debuginfo/metadata.rs index 88e692cb11747..a1e81c10b415e 100644 --- a/src/librustc_codegen_llvm/debuginfo/metadata.rs +++ b/src/librustc_codegen_llvm/debuginfo/metadata.rs @@ -2287,7 +2287,7 @@ pub fn create_global_var_metadata(cx: &CodegenCx<'ll, '_>, def_id: DefId, global }; let is_local_to_unit = is_node_local_to_unit(cx, def_id); - let variable_type = Instance::mono(cx.tcx, def_id).ty(cx.tcx); + let variable_type = Instance::mono(cx.tcx, def_id).monomorphic_ty(cx.tcx); let type_metadata = type_metadata(cx, variable_type, span); let var_name = SmallCStr::new(&tcx.item_name(def_id).as_str()); let linkage_name = if no_mangle { diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index 0e121df1d257e..1c7146308389a 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -89,7 +89,7 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { span: Span, ) { let tcx = self.tcx; - let callee_ty = instance.ty(tcx); + let callee_ty = instance.monomorphic_ty(tcx); let (def_id, substs) = match callee_ty.kind { ty::FnDef(def_id, substs) => (def_id, substs), diff --git a/src/librustc_codegen_llvm/mono_item.rs b/src/librustc_codegen_llvm/mono_item.rs index e0a946a38e4d0..681bc1f2dcb77 100644 --- a/src/librustc_codegen_llvm/mono_item.rs +++ b/src/librustc_codegen_llvm/mono_item.rs @@ -22,7 +22,7 @@ impl PreDefineMethods<'tcx> for CodegenCx<'ll, 'tcx> { symbol_name: &str, ) { let instance = Instance::mono(self.tcx, def_id); - let ty = instance.ty(self.tcx); + let ty = instance.monomorphic_ty(self.tcx); let llty = self.layout_of(ty).llvm_type(self); let g = self.define_global(symbol_name, llty).unwrap_or_else(|| { diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index d6fd48cc89fa0..f660a4a72a189 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -140,7 +140,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // to determine the type. let drop_instance = self.memory.get_fn(drop_fn)?.as_instance()?; trace!("Found drop fn: {:?}", drop_instance); - let fn_sig = drop_instance.ty(*self.tcx).fn_sig(*self.tcx); + let fn_sig = drop_instance.monomorphic_ty(*self.tcx).fn_sig(*self.tcx); let fn_sig = self.tcx.normalize_erasing_late_bound_regions(self.param_env, &fn_sig); // The drop function takes `*mut T` where `T` is the type being dropped, so get that. let args = fn_sig.inputs(); diff --git a/src/librustc_mir/monomorphize/collector.rs b/src/librustc_mir/monomorphize/collector.rs index 231ee792918b3..dca850462ce2e 100644 --- a/src/librustc_mir/monomorphize/collector.rs +++ b/src/librustc_mir/monomorphize/collector.rs @@ -358,7 +358,7 @@ fn collect_items_rec<'tcx>( // Sanity check whether this ended up being collected accidentally debug_assert!(should_monomorphize_locally(tcx, &instance)); - let ty = instance.ty(tcx); + let ty = instance.monomorphic_ty(tcx); visit_drop_use(tcx, ty, true, &mut neighbors); recursion_depth_reset = None; @@ -1002,7 +1002,8 @@ impl ItemLikeVisitor<'v> for RootCollector<'_, 'v> { def_id_to_string(self.tcx, def_id) ); - let ty = Instance::new(def_id, InternalSubsts::empty()).ty(self.tcx); + let ty = + Instance::new(def_id, InternalSubsts::empty()).monomorphic_ty(self.tcx); visit_drop_use(self.tcx, ty, true, self.output); } } From db2c4f214f9bb638261cb0e1e0d6d66847d122e1 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 5 Jan 2020 15:00:55 -0500 Subject: [PATCH 6/7] Fix typo --- src/librustc/ty/instance.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc/ty/instance.rs b/src/librustc/ty/instance.rs index f2813f0403577..e315de115688b 100644 --- a/src/librustc/ty/instance.rs +++ b/src/librustc/ty/instance.rs @@ -71,7 +71,7 @@ impl<'tcx> Instance<'tcx> { /// However, during constant evaluation, we may want /// to try to resolve a `Instance` using generic parameters /// (e.g. when we are attempting to to do const-propagation). - /// In this case, `Instace.ty_env` should be used to provide + /// In this case, `Instance.ty_env` should be used to provide /// the `ParamEnv` for our generic context. pub fn monomorphic_ty(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx> { let ty = tcx.type_of(self.def.def_id()); From 336b902f669167d09782702dffcc12513ebc8950 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 5 Jan 2020 18:09:57 -0500 Subject: [PATCH 7/7] Use Instance.ty_env instead of Instance.monomorphic_ty in interpreter --- src/librustc_mir/interpret/traits.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index f660a4a72a189..efbbca534856a 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -140,7 +140,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // to determine the type. let drop_instance = self.memory.get_fn(drop_fn)?.as_instance()?; trace!("Found drop fn: {:?}", drop_instance); - let fn_sig = drop_instance.monomorphic_ty(*self.tcx).fn_sig(*self.tcx); + let fn_sig = drop_instance.ty_env(*self.tcx, self.param_env).fn_sig(*self.tcx); let fn_sig = self.tcx.normalize_erasing_late_bound_regions(self.param_env, &fn_sig); // The drop function takes `*mut T` where `T` is the type being dropped, so get that. let args = fn_sig.inputs();