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

Filter and test predicates using normalize_and_test_predicates for const-prop #68297

Merged
merged 4 commits into from
Jan 21, 2020
Merged
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
5 changes: 1 addition & 4 deletions src/librustc/mir/mono.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use crate::dep_graph::{DepConstructor, DepNode, WorkProduct, WorkProductId};
use crate::ich::{Fingerprint, NodeIdHashingMode, StableHashingContext};
use crate::session::config::OptLevel;
use crate::traits::TraitQueryMode;
use crate::ty::print::obsolete::DefPathBasedNames;
use crate::ty::{subst::InternalSubsts, Instance, InstanceDef, SymbolName, TyCtxt};
use rustc_data_structures::base_n;
Expand Down Expand Up @@ -168,9 +167,7 @@ impl<'tcx> MonoItem<'tcx> {
MonoItem::GlobalAsm(..) => return true,
};

// We shouldn't encounter any overflow here, so we use TraitQueryMode::Standard\
// to report an error if overflow somehow occurs.
tcx.substitute_normalize_and_test_predicates((def_id, &substs, TraitQueryMode::Standard))
tcx.substitute_normalize_and_test_predicates((def_id, &substs))
}

pub fn to_string(&self, tcx: TyCtxt<'tcx>, debug: bool) -> String {
Expand Down
6 changes: 3 additions & 3 deletions src/librustc/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1148,11 +1148,11 @@ rustc_queries! {
desc { "normalizing `{:?}`", goal }
}

query substitute_normalize_and_test_predicates(key: (DefId, SubstsRef<'tcx>, traits::TraitQueryMode)) -> bool {
query substitute_normalize_and_test_predicates(key: (DefId, SubstsRef<'tcx>)) -> bool {
no_force
desc { |tcx|
"testing substituted normalized predicates in mode {:?}:`{}`",
key.2, tcx.def_path_str(key.0)
"testing substituted normalized predicates:`{}`",
tcx.def_path_str(key.0)
}
}

Expand Down
24 changes: 2 additions & 22 deletions src/librustc/traits/fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use super::CodeSelectionError;
use super::{ConstEvalFailure, Unimplemented};
use super::{FulfillmentError, FulfillmentErrorCode};
use super::{ObligationCause, PredicateObligation};
use crate::traits::TraitQueryMode;

impl<'tcx> ForestObligation for PendingPredicateObligation<'tcx> {
type Predicate = ty::Predicate<'tcx>;
Expand Down Expand Up @@ -63,9 +62,6 @@ pub struct FulfillmentContext<'tcx> {
// a snapshot (they don't *straddle* a snapshot, so there
// is no trouble there).
usable_in_snapshot: bool,

// The `TraitQueryMode` used when constructing a `SelectionContext`
query_mode: TraitQueryMode,
}

#[derive(Clone, Debug)]
Expand All @@ -79,26 +75,12 @@ pub struct PendingPredicateObligation<'tcx> {
static_assert_size!(PendingPredicateObligation<'_>, 136);

impl<'a, 'tcx> FulfillmentContext<'tcx> {
/// Creates a new fulfillment context with `TraitQueryMode::Standard`
/// You almost always want to use this instead of `with_query_mode`
/// Creates a new fulfillment context.
pub fn new() -> FulfillmentContext<'tcx> {
FulfillmentContext {
predicates: ObligationForest::new(),
register_region_obligations: true,
usable_in_snapshot: false,
query_mode: TraitQueryMode::Standard,
}
}

/// Creates a new fulfillment context with the specified query mode.
/// This should only be used when you want to ignore overflow,
/// rather than reporting it as an error.
pub fn with_query_mode(query_mode: TraitQueryMode) -> FulfillmentContext<'tcx> {
FulfillmentContext {
predicates: ObligationForest::new(),
register_region_obligations: true,
usable_in_snapshot: false,
query_mode,
}
}

Expand All @@ -107,7 +89,6 @@ impl<'a, 'tcx> FulfillmentContext<'tcx> {
predicates: ObligationForest::new(),
register_region_obligations: true,
usable_in_snapshot: true,
query_mode: TraitQueryMode::Standard,
}
}

Expand All @@ -116,7 +97,6 @@ impl<'a, 'tcx> FulfillmentContext<'tcx> {
predicates: ObligationForest::new(),
register_region_obligations: false,
usable_in_snapshot: false,
query_mode: TraitQueryMode::Standard,
}
}

Expand Down Expand Up @@ -237,7 +217,7 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> {
&mut self,
infcx: &InferCtxt<'_, 'tcx>,
) -> Result<(), Vec<FulfillmentError<'tcx>>> {
let mut selcx = SelectionContext::with_query_mode(infcx, self.query_mode);
let mut selcx = SelectionContext::new(infcx);
self.select(&mut selcx)
}

Expand Down
20 changes: 8 additions & 12 deletions src/librustc/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ pub enum IntercrateMode {
}

/// The mode that trait queries run in.
#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash, HashStable)]
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum TraitQueryMode {
// Standard/un-canonicalized queries get accurate
// spans etc. passed in and hence can do reasonable
Expand Down Expand Up @@ -1014,17 +1014,16 @@ where
/// environment. If this returns false, then either normalize
/// encountered an error or one of the predicates did not hold. Used
/// when creating vtables to check for unsatisfiable methods.
fn normalize_and_test_predicates<'tcx>(
pub fn normalize_and_test_predicates<'tcx>(
tcx: TyCtxt<'tcx>,
predicates: Vec<ty::Predicate<'tcx>>,
mode: TraitQueryMode,
) -> bool {
debug!("normalize_and_test_predicates(predicates={:?}, mode={:?})", predicates, mode);
debug!("normalize_and_test_predicates(predicates={:?})", predicates);

let result = tcx.infer_ctxt().enter(|infcx| {
let param_env = ty::ParamEnv::reveal_all();
let mut selcx = SelectionContext::with_query_mode(&infcx, mode);
let mut fulfill_cx = FulfillmentContext::with_query_mode(mode);
let mut selcx = SelectionContext::new(&infcx);
let mut fulfill_cx = FulfillmentContext::new();
let cause = ObligationCause::dummy();
let Normalized { value: predicates, obligations } =
normalize(&mut selcx, param_env, cause.clone(), &predicates);
Expand All @@ -1044,12 +1043,12 @@ fn normalize_and_test_predicates<'tcx>(

fn substitute_normalize_and_test_predicates<'tcx>(
tcx: TyCtxt<'tcx>,
key: (DefId, SubstsRef<'tcx>, TraitQueryMode),
key: (DefId, SubstsRef<'tcx>),
) -> bool {
debug!("substitute_normalize_and_test_predicates(key={:?})", key);

let predicates = tcx.predicates_of(key.0).instantiate(tcx, key.1).predicates;
let result = normalize_and_test_predicates(tcx, predicates, key.2);
let result = normalize_and_test_predicates(tcx, predicates);

debug!("substitute_normalize_and_test_predicates(key={:?}) = {:?}", key, result);
result
Expand Down Expand Up @@ -1102,10 +1101,7 @@ fn vtable_methods<'tcx>(
// Note that this method could then never be called, so we
// do not want to try and codegen it, in that case (see #23435).
let predicates = tcx.predicates_of(def_id).instantiate_own(tcx, substs);
// We don't expect overflow here, so report an error if it somehow ends
// up happening.
if !normalize_and_test_predicates(tcx, predicates.predicates, TraitQueryMode::Standard)
{
if !normalize_and_test_predicates(tcx, predicates.predicates) {
debug!("vtable_methods: predicates do not hold");
return None;
}
Expand Down
9 changes: 0 additions & 9 deletions src/librustc/ty/query/keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,6 @@ impl<'tcx> Key for (DefId, SubstsRef<'tcx>) {
}
}

impl<'tcx> Key for (DefId, SubstsRef<'tcx>, traits::TraitQueryMode) {
fn query_crate(&self) -> CrateNum {
self.0.krate
}
fn default_span(&self, tcx: TyCtxt<'_>) -> Span {
self.0.default_span(tcx)
}
}

impl<'tcx> Key for (ty::ParamEnv<'tcx>, ty::PolyTraitRef<'tcx>) {
fn query_crate(&self) -> CrateNum {
self.1.def_id().krate
Expand Down
44 changes: 22 additions & 22 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rustc::mir::{
SourceInfo, SourceScope, SourceScopeData, Statement, StatementKind, Terminator, TerminatorKind,
UnOp, RETURN_PLACE,
};
use rustc::traits::TraitQueryMode;
use rustc::traits;
use rustc::ty::layout::{
HasDataLayout, HasTyCtxt, LayoutError, LayoutOf, Size, TargetDataLayout, TyLayout,
};
Expand Down Expand Up @@ -90,28 +90,28 @@ impl<'tcx> MirPass<'tcx> for ConstProp {
// If there are unsatisfiable where clauses, then all bets are
// off, and we just give up.
//
// Note that we use TraitQueryMode::Canonical here, which causes
// us to treat overflow like any other error. This is because we
// are "speculatively" evaluating this item with the default substs.
// While this usually succeeds, it may fail with tricky impls
// (e.g. the typenum crate). Const-propagation is fundamentally
// "best-effort", and does not affect correctness in any way.
// Therefore, it's perfectly fine to just "give up" if we're
// unable to check the bounds with the default substs.
// We manually filter the predicates, skipping anything that's not
// "global". We are in a potentially generic context
// (e.g. we are evaluating a function without substituting generic
// parameters, so this filtering serves two purposes:
//
// False negatives (failing to run const-prop on something when we actually
// could) are fine. However, false positives (running const-prop on
// an item with unsatisfiable bounds) can lead to us generating invalid
// MIR.
if !tcx.substitute_normalize_and_test_predicates((
source.def_id(),
InternalSubsts::identity_for_item(tcx, source.def_id()),
TraitQueryMode::Canonical,
)) {
trace!(
"ConstProp skipped for item with unsatisfiable predicates: {:?}",
source.def_id()
);
// 1. We skip evaluating any predicates that we would
// never be able prove are unsatisfiable (e.g. `<T as Foo>`
// 2. We avoid trying to normalize predicates involving generic
// parameters (e.g. `<T as Foo>::MyItem`). This can confuse
// the normalization code (leading to cycle errors), since
// it's usually never invoked in this way.
let predicates = tcx
.predicates_of(source.def_id())
.predicates
.iter()
.filter_map(|(p, _)| if p.is_global() { Some(*p) } else { None })
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
.collect();
if !traits::normalize_and_test_predicates(
Copy link
Contributor

Choose a reason for hiding this comment

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

why call the function and not the query here?

Copy link
Member Author

@Aaron1011 Aaron1011 Jan 17, 2020

Choose a reason for hiding this comment

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

I didn't realize this was a query, lol.

EDIT: This isn't a query. Did you want me to query-ify this?

Copy link
Contributor

Choose a reason for hiding this comment

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

well there's substitute_normalize_and_test_predicates which you used before.

Copy link
Member Author

Choose a reason for hiding this comment

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

@oli-obk: That would use all of the predicates, which I needed to filter beforehand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh... sorry for not reading the docs properly. You're totally right. Let's merge this PR for now and experiment with making it a query separately. Such a query could share a lot of evaluation time between the predicate elaborations of completely independent definitions if they have the same predicate list.

tcx,
traits::elaborate_predicates(tcx, predicates).collect(),
) {
trace!("ConstProp skipped for {:?}: found unsatisfiable predicates", source.def_id());
return;
}

Expand Down
43 changes: 43 additions & 0 deletions src/test/ui/consts/issue-68264-overflow.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// check-pass
// compile-flags: --emit=mir,link
// Regression test for issue #68264
// Checks that we don't encounter overflow
// when running const-prop on functions with
// complicated bounds
pub trait Query {}

pub trait AsQuery {
type Query: Query;
}
pub trait Table: AsQuery + Sized {}

pub trait LimitDsl {
type Output;
}

pub(crate) trait LoadQuery<Conn, U>: RunQueryDsl<Conn> {}

impl<T: Query> AsQuery for T {
type Query = Self;
}

impl<T> LimitDsl for T
where
T: Table,
T::Query: LimitDsl,
{
type Output = <T::Query as LimitDsl>::Output;
}

pub(crate) trait RunQueryDsl<Conn>: Sized {
fn first<U>(self, _conn: &Conn) -> U
where
Self: LimitDsl,
Self::Output: LoadQuery<Conn, U>,
{
// Overflow is caused by this function body
unimplemented!()
}
}

fn main() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

The overflow only happens if the current crate is a library. It does not happen if it's a binary crate (containing a main function). That means this test tests something different.

Copy link
Member Author

Choose a reason for hiding this comment

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

Passing --emit=mir causes the issue to occur for a binary crate, since it forces optimized_mir (and therefore const-prop) to get run on the 'bad' function.

You can verify this locally with rustc +nightly src/test/ui/consts/issue-68264-overflow.rs --emit=mir