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

Fix ICE involving calling Instance.ty during const evaluation #67800

Merged
merged 7 commits into from
Jan 6, 2020
Merged
Show file tree
Hide file tree
Changes from 6 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
27 changes: 26 additions & 1 deletion src/librustc/ty/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,35 @@ pub enum InstanceDef<'tcx> {
}

impl<'tcx> Instance<'tcx> {
pub fn ty(&self, tcx: TyCtxt<'tcx>) -> Ty<'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, `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());
// 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() {
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
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)
}

/// 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)
}
Comment on lines -65 to +93
Copy link
Member

Choose a reason for hiding this comment

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

I would keep a single ty method and force callers to pass in ty::ParamEnv::reveal_all() (eventually all of codegen should have to keep track of a ParamEnv anyway, because of @davidtwco's work on partial monomorphization).

}

impl<'tcx> InstanceDef<'tcx> {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not obvious to me that this is correct. Ca you go up the call chain and see if there are any noncodegen uses?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only caller is FnAbi:of_instance, which is only called from codegen.

Copy link
Member

Choose a reason for hiding this comment

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

We need to stop assuming that codegen = monomorphic.

match ty.kind {
ty::FnDef(..) |
// Shims currently have type FnPtr. Not sure this should remain.
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_codegen_llvm/callee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, &[]);

Expand Down
4 changes: 2 additions & 2 deletions src/librustc_codegen_llvm/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_codegen_llvm/debuginfo/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_codegen_llvm/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_codegen_llvm/mono_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(|| {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Aaron1011 marked this conversation as resolved.
Show resolved Hide resolved
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();
Expand Down
5 changes: 3 additions & 2 deletions src/librustc_mir/monomorphize/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
Expand Down
34 changes: 34 additions & 0 deletions src/test/ui/mir/issue-67639-normalization-ice.rs
Original file line number Diff line number Diff line change
@@ -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::<u32>();
}

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<I>()
where I: Foo,
I::Item: Bar
{
Vec::<<I::Item as Bar>::Item2>::new(); // ICE occurs processing this line
}