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

Report monomorphization time errors in dead code, too #112879

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
8 changes: 5 additions & 3 deletions compiler/rustc_codegen_cranelift/src/constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,11 @@ impl ConstantCx {

pub(crate) fn check_constants(fx: &mut FunctionCx<'_, '_, '_>) -> bool {
let mut all_constants_ok = true;
for constant in &fx.mir.required_consts {
if eval_mir_constant(fx, constant).is_none() {
all_constants_ok = false;
for (item, _) in &fx.mir.required_items {
if let rustc_middle::mir::MonoItem::Const(constant) = item {
Copy link
Member

Choose a reason for hiding this comment

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

We have this logic a couple of times. Might be worth it to have a required_consts method on mir::Body?

if eval_mir_constant(fx, constant).is_none() {
all_constants_ok = false;
}
}
}
all_constants_ok
Expand Down
23 changes: 15 additions & 8 deletions compiler/rustc_codegen_ssa/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::base;
use crate::traits::*;
use rustc_middle::mir;
use rustc_middle::mir::interpret::ErrorHandled;
use rustc_middle::mir::MonoItem;
use rustc_middle::ty::layout::{FnAbiOf, HasTyCtxt, TyAndLayout};
use rustc_middle::ty::{self, Instance, Ty, TyCtxt, TypeFoldable, TypeVisitableExt};
use rustc_target::abi::call::{FnAbi, PassMode};
Expand Down Expand Up @@ -202,14 +203,20 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(

// Evaluate all required consts; codegen later assumes that CTFE will never fail.
let mut all_consts_ok = true;
for const_ in &mir.required_consts {
if let Err(err) = fx.eval_mir_constant(const_) {
all_consts_ok = false;
match err {
// errored or at least linted
ErrorHandled::Reported(_) => {}
ErrorHandled::TooGeneric => {
span_bug!(const_.span, "codegen encountered polymorphic constant: {:?}", err)
for (const_, _) in &mir.required_items {
if let MonoItem::Const(const_) = const_ {
if let Err(err) = fx.eval_mir_constant(const_) {
all_consts_ok = false;
match err {
// errored or at least linted
ErrorHandled::Reported(_) => {}
ErrorHandled::TooGeneric => {
span_bug!(
const_.span,
"codegen encountered polymorphic constant: {:?}",
err
)
}
}
}
}
Expand Down
17 changes: 13 additions & 4 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -703,10 +703,19 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.stack_mut().push(frame);

// Make sure all the constants required by this frame evaluate successfully (post-monomorphization check).
for ct in &body.required_consts {
let span = ct.span;
let ct = self.subst_from_current_frame_and_normalize_erasing_regions(ct.literal)?;
self.eval_mir_constant(&ct, Some(span), None)?;
for (ct, span) in &body.required_items {
if let mir::MonoItem::Const(ct) = ct {
match ct.literal {
mir::ConstantKind::Ty(c) => match c.kind() {
ty::ConstKind::Unevaluated(_) => {}
_ => continue,
},
mir::ConstantKind::Unevaluated(_, _) => {}
mir::ConstantKind::Val(_, _) => continue,
}
Comment on lines +708 to +715
Copy link
Member

Choose a reason for hiding this comment

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

What's this about? The codegen backends don't do this. At least this needs a comment explaining why we are skipping some consts.

let ct = self.subst_from_current_frame_and_normalize_erasing_regions(ct.literal)?;
self.eval_mir_constant(&ct, Some(*span), None)?;
}
}

// Most locals are initially dead.
Expand Down
29 changes: 24 additions & 5 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,23 @@ pub struct GeneratorInfo<'tcx> {
pub generator_kind: GeneratorKind,
}

#[derive(Clone, TyEncodable, TyDecodable, Debug, HashStable, TypeFoldable, TypeVisitable)]
/// Items that the monomorphization collector needs to walk into.
pub enum MonoItem<'tcx> {
/// The `bool` states whether this is a direct call or not.
Fn(DefId, SubstsRef<'tcx>, bool),
Static(DefId),
Vtable {
source_ty: Ty<'tcx>,
target_ty: Ty<'tcx>,
},
Const(Constant<'tcx>),
Drop(Ty<'tcx>),
Closure(DefId, SubstsRef<'tcx>),
}

pub type MonoItems<'tcx> = Vec<(MonoItem<'tcx>, Span)>;

/// The lowered representation of a single function.
#[derive(Clone, TyEncodable, TyDecodable, Debug, HashStable, TypeFoldable, TypeVisitable)]
pub struct Body<'tcx> {
Expand Down Expand Up @@ -275,9 +292,11 @@ pub struct Body<'tcx> {
/// A span representing this MIR, for error reporting.
pub span: Span,

/// Constants that are required to evaluate successfully for this MIR to be well-formed.
/// We hold in this field all the constants we are not able to evaluate yet.
pub required_consts: Vec<Constant<'tcx>>,
/// Items that this body needs to monomorphize if monomorphized itself.
/// This field avoids the issue that optimizations can remove function calls in dead code
/// causing differences in monomorphization time errors depending on the optimizations
/// enabled.
pub required_items: MonoItems<'tcx>,

/// Does this body use generic parameters. This is used for the `ConstEvaluatable` check.
///
Expand Down Expand Up @@ -347,7 +366,7 @@ impl<'tcx> Body<'tcx> {
spread_arg: None,
var_debug_info,
span,
required_consts: Vec::new(),
required_items: Vec::new(),
is_polymorphic: false,
injection_phase: None,
tainted_by_errors,
Expand All @@ -374,7 +393,7 @@ impl<'tcx> Body<'tcx> {
arg_count: 0,
spread_arg: None,
span: DUMMY_SP,
required_consts: Vec::new(),
required_items: Vec::new(),
var_debug_info: Vec::new(),
is_polymorphic: false,
injection_phase: None,
Expand Down
9 changes: 7 additions & 2 deletions compiler/rustc_middle/src/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1039,9 +1039,14 @@ macro_rules! super_body {

$self.visit_span($(& $mutability)? $body.span);

for const_ in &$($mutability)? $body.required_consts {
for (item, _) in &$($mutability)? $body.required_items {
let location = Location::START;
$self.visit_constant(const_, location);
match item {
MonoItem::Const(const_) => if let ConstantKind::Unevaluated(..) = const_.literal {
$self.visit_constant(const_, location)
},
_ => {}
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/build/custom/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ pub(super) fn build_custom_mir<'tcx>(
spread_arg: None,
var_debug_info: Vec::new(),
span,
required_consts: Vec::new(),
required_items: Vec::new(),
is_polymorphic: false,
tainted_by_errors: None,
injection_phase: None,
Expand Down
13 changes: 1 addition & 12 deletions compiler/rustc_mir_transform/src/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -625,18 +625,7 @@ impl<'tcx> Inliner<'tcx> {
kind: TerminatorKind::Goto { target: integrator.map_block(START_BLOCK) },
});

// Copy only unevaluated constants from the callee_body into the caller_body.
// Although we are only pushing `ConstKind::Unevaluated` consts to
// `required_consts`, here we may not only have `ConstKind::Unevaluated`
// because we are calling `subst_and_normalize_erasing_regions`.
caller_body.required_consts.extend(
callee_body.required_consts.iter().copied().filter(|&ct| match ct.literal {
ConstantKind::Ty(_) => {
bug!("should never encounter ty::UnevaluatedConst in `required_consts`")
}
ConstantKind::Val(..) | ConstantKind::Unevaluated(..) => true,
}),
);
caller_body.required_items.extend(callee_body.required_items.iter().cloned());
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't required_items already reference the callee one way or another, making this redundant?

}
kind => bug!("unexpected terminator kind {:?}", kind),
}
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ extern crate tracing;
#[macro_use]
extern crate rustc_middle;

use required_consts::RequiredConstsVisitor;
use rustc_const_eval::util;
use rustc_data_structures::fx::FxIndexSet;
use rustc_data_structures::steal::Steal;
Expand Down Expand Up @@ -303,12 +302,13 @@ fn mir_promoted(
body.tainted_by_errors = Some(error_reported);
}

let mut required_consts = Vec::new();
let mut required_consts_visitor = RequiredConstsVisitor::new(&mut required_consts);
let param_env = tcx.param_env(def);
let mut required_consts_visitor =
required_consts::MirNeighborCollector { tcx, body: &body, output: vec![], param_env };
for (bb, bb_data) in traversal::reverse_postorder(&body) {
required_consts_visitor.visit_basic_block_data(bb, bb_data);
}
body.required_consts = required_consts;
body.required_items = required_consts_visitor.output;

// What we need to run borrowck etc.
let promote_pass = promote_consts::PromoteTemps::default();
Expand Down
Loading