From 5896998e7659bf22ffb804956a3680c028ede45f Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 5 Jan 2020 22:32:53 -0500 Subject: [PATCH 1/6] Don't run const propagation on items with inconsistent bounds Using `#![feature(trivial_bounds)]`, it's possible to write functions with unsatisfiable 'where' clauses, making them uncallable. However, the user can act as if these 'where' clauses are true inside the body of the function, leading to code that would normally be impossible to write. Since const propgation can run even without any user-written calls to a function, we need to explcitly check for these uncallable functions. --- src/librustc_mir/transform/const_prop.rs | 25 +++++++++++++++++++ ...ounds-inconsistent-associated-functions.rs | 4 +++ ...s-inconsistent-associated-functions.stderr | 2 ++ 3 files changed, 31 insertions(+) create mode 100644 src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.stderr diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 1d5a643484a..48ab24a14c7 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -74,6 +74,31 @@ impl<'tcx> MirPass<'tcx> for ConstProp { return; } + // Check if it's even possible to satisy the 'where' clauses + // for this item. + // This branch will never be taken for any normal function. + // However, it's possible to `#!feature(trivial_bounds)]` to write + // a function with impossible to satisfy clauses, e.g.: + // `fn foo() where String: Copy {}` + // + // We don't usually need to worry about this kind of case, + // since we would get a compilation error if the user tried + // to call it. However, since we can do const propagation + // even without any calls to the function, we need to make + // sure that it even makes sense to try to evaluate the body. + // If there are unsatisfiable where clauses, then all bets are + // off, and we just give up. + if !tcx.substitute_normalize_and_test_predicates(( + source.def_id(), + InternalSubsts::identity_for_item(tcx, source.def_id()), + )) { + trace!( + "ConstProp skipped for item with unsatisfiable predicates: {:?}", + source.def_id() + ); + return; + } + trace!("ConstProp starting for {:?}", source.def_id()); let dummy_body = &Body::new( diff --git a/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs b/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs index 6450ddd1b67..818be105547 100644 --- a/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs +++ b/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs @@ -1,4 +1,8 @@ // run-pass +// Force mir to be emitted, to ensure that const +// propagation doesn't ICE on a function +// with an 'impossible' body. See issue #67696 +// compile-flags: --emit=mir,link // Inconsistent bounds with trait implementations #![feature(trivial_bounds)] diff --git a/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.stderr b/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.stderr new file mode 100644 index 00000000000..0ee1e2e0ba5 --- /dev/null +++ b/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.stderr @@ -0,0 +1,2 @@ +warning: due to multiple output types requested, the explicitly specified output file name will be adapted for each output type + From e1fc22c4eba6e07a5549c74b197fbaa53f7b29e1 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 7 Jan 2020 10:01:52 -0500 Subject: [PATCH 2/6] Add additional regression test --- .../ui/consts/issue-67696-const-prop-ice.rs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 src/test/ui/consts/issue-67696-const-prop-ice.rs diff --git a/src/test/ui/consts/issue-67696-const-prop-ice.rs b/src/test/ui/consts/issue-67696-const-prop-ice.rs new file mode 100644 index 00000000000..ad52608b3f4 --- /dev/null +++ b/src/test/ui/consts/issue-67696-const-prop-ice.rs @@ -0,0 +1,20 @@ +// check-pass +// compile-flags: --emit=mir,link +// Checks that we don't ICE due to attempting to run const prop +// on a function with unsatisifable 'where' clauses + +#![allow(unused)] + +trait A { + fn foo(&self) -> Self where Self: Copy; +} + +impl A for [fn(&())] { + fn foo(&self) -> Self where Self: Copy { *(&[] as &[_]) } +} + +impl A for i32 { + fn foo(&self) -> Self { 3 } +} + +fn main() {} From 92d86cc9b8bbd744c5eebf3718a9dc87576b786c Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 7 Jan 2020 10:53:04 -0500 Subject: [PATCH 3/6] Fix typo --- src/librustc_mir/transform/const_prop.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 48ab24a14c7..35dbf2f7eaa 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -74,7 +74,7 @@ impl<'tcx> MirPass<'tcx> for ConstProp { return; } - // Check if it's even possible to satisy the 'where' clauses + // Check if it's even possible to satisfy the 'where' clauses // for this item. // This branch will never be taken for any normal function. // However, it's possible to `#!feature(trivial_bounds)]` to write From 7df8ca29545301c1d3020bf8fc9aa8f9fd8759ea Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 11 Jan 2020 13:20:09 -0500 Subject: [PATCH 4/6] Convert test to check-pass --- .../trivial-bounds-inconsistent-associated-functions.rs | 4 ++-- .../trivial-bounds-inconsistent-associated-functions.stderr | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) delete mode 100644 src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.stderr diff --git a/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs b/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs index 818be105547..e0d4d538b40 100644 --- a/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs +++ b/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs @@ -1,8 +1,8 @@ -// run-pass +// check-pass +// compile-flags: --emit=mir // Force mir to be emitted, to ensure that const // propagation doesn't ICE on a function // with an 'impossible' body. See issue #67696 -// compile-flags: --emit=mir,link // Inconsistent bounds with trait implementations #![feature(trivial_bounds)] diff --git a/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.stderr b/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.stderr deleted file mode 100644 index 0ee1e2e0ba5..00000000000 --- a/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.stderr +++ /dev/null @@ -1,2 +0,0 @@ -warning: due to multiple output types requested, the explicitly specified output file name will be adapted for each output type - From 6a0bb1867b2026e05982f865bb48857d7e492cd7 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 11 Jan 2020 14:17:42 -0500 Subject: [PATCH 5/6] Add "--emit=link" This avoids a strange linker error that we get with only "--emit=mir" and "check-pass" --- .../trivial-bounds-inconsistent-associated-functions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs b/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs index e0d4d538b40..69eee66e64d 100644 --- a/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs +++ b/src/test/ui/trivial-bounds/trivial-bounds-inconsistent-associated-functions.rs @@ -1,5 +1,5 @@ // check-pass -// compile-flags: --emit=mir +// compile-flags: --emit=mir,link // Force mir to be emitted, to ensure that const // propagation doesn't ICE on a function // with an 'impossible' body. See issue #67696 From e390b91e5717a338db20360a1ddffa23ba66701d Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 13 Jan 2020 06:06:42 -0500 Subject: [PATCH 6/6] Use TraitQueryMode::Canonical when testing predicates in const prop --- src/librustc/mir/mono.rs | 5 ++++- src/librustc/query/mod.rs | 6 +++--- src/librustc/traits/fulfill.rs | 24 ++++++++++++++++++++++-- src/librustc/traits/mod.rs | 18 +++++++++++------- src/librustc/ty/query/keys.rs | 9 +++++++++ src/librustc_mir/transform/const_prop.rs | 16 ++++++++++++++++ 6 files changed, 65 insertions(+), 13 deletions(-) diff --git a/src/librustc/mir/mono.rs b/src/librustc/mir/mono.rs index 51ce575e51f..e7a4c5b5921 100644 --- a/src/librustc/mir/mono.rs +++ b/src/librustc/mir/mono.rs @@ -1,6 +1,7 @@ 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; @@ -167,7 +168,9 @@ impl<'tcx> MonoItem<'tcx> { MonoItem::GlobalAsm(..) => return true, }; - tcx.substitute_normalize_and_test_predicates((def_id, &substs)) + // 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)) } pub fn to_string(&self, tcx: TyCtxt<'tcx>, debug: bool) -> String { diff --git a/src/librustc/query/mod.rs b/src/librustc/query/mod.rs index 50ef115da2c..669ba4abfad 100644 --- a/src/librustc/query/mod.rs +++ b/src/librustc/query/mod.rs @@ -1141,11 +1141,11 @@ rustc_queries! { desc { "normalizing `{:?}`", goal } } - query substitute_normalize_and_test_predicates(key: (DefId, SubstsRef<'tcx>)) -> bool { + query substitute_normalize_and_test_predicates(key: (DefId, SubstsRef<'tcx>, traits::TraitQueryMode)) -> bool { no_force desc { |tcx| - "testing substituted normalized predicates:`{}`", - tcx.def_path_str(key.0) + "testing substituted normalized predicates in mode {:?}:`{}`", + key.2, tcx.def_path_str(key.0) } } diff --git a/src/librustc/traits/fulfill.rs b/src/librustc/traits/fulfill.rs index 46ece6fc405..9e5abc80822 100644 --- a/src/librustc/traits/fulfill.rs +++ b/src/librustc/traits/fulfill.rs @@ -16,6 +16,7 @@ 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>; @@ -62,6 +63,9 @@ 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)] @@ -75,12 +79,26 @@ pub struct PendingPredicateObligation<'tcx> { static_assert_size!(PendingPredicateObligation<'_>, 136); impl<'a, 'tcx> FulfillmentContext<'tcx> { - /// Creates a new fulfillment context. + /// Creates a new fulfillment context with `TraitQueryMode::Standard` + /// You almost always want to use this instead of `with_query_mode` 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, } } @@ -89,6 +107,7 @@ impl<'a, 'tcx> FulfillmentContext<'tcx> { predicates: ObligationForest::new(), register_region_obligations: true, usable_in_snapshot: true, + query_mode: TraitQueryMode::Standard, } } @@ -97,6 +116,7 @@ impl<'a, 'tcx> FulfillmentContext<'tcx> { predicates: ObligationForest::new(), register_region_obligations: false, usable_in_snapshot: false, + query_mode: TraitQueryMode::Standard, } } @@ -217,7 +237,7 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> { &mut self, infcx: &InferCtxt<'_, 'tcx>, ) -> Result<(), Vec>> { - let mut selcx = SelectionContext::new(infcx); + let mut selcx = SelectionContext::with_query_mode(infcx, self.query_mode); self.select(&mut selcx) } diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs index 2d3160dc3e5..31de5409fc8 100644 --- a/src/librustc/traits/mod.rs +++ b/src/librustc/traits/mod.rs @@ -95,7 +95,7 @@ pub enum IntercrateMode { } /// The mode that trait queries run in. -#[derive(Copy, Clone, PartialEq, Eq, Debug)] +#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash, HashStable)] pub enum TraitQueryMode { // Standard/un-canonicalized queries get accurate // spans etc. passed in and hence can do reasonable @@ -1017,13 +1017,14 @@ where fn normalize_and_test_predicates<'tcx>( tcx: TyCtxt<'tcx>, predicates: Vec>, + mode: TraitQueryMode, ) -> bool { - debug!("normalize_and_test_predicates(predicates={:?})", predicates); + debug!("normalize_and_test_predicates(predicates={:?}, mode={:?})", predicates, mode); let result = tcx.infer_ctxt().enter(|infcx| { let param_env = ty::ParamEnv::reveal_all(); - let mut selcx = SelectionContext::new(&infcx); - let mut fulfill_cx = FulfillmentContext::new(); + let mut selcx = SelectionContext::with_query_mode(&infcx, mode); + let mut fulfill_cx = FulfillmentContext::with_query_mode(mode); let cause = ObligationCause::dummy(); let Normalized { value: predicates, obligations } = normalize(&mut selcx, param_env, cause.clone(), &predicates); @@ -1043,12 +1044,12 @@ fn normalize_and_test_predicates<'tcx>( fn substitute_normalize_and_test_predicates<'tcx>( tcx: TyCtxt<'tcx>, - key: (DefId, SubstsRef<'tcx>), + key: (DefId, SubstsRef<'tcx>, TraitQueryMode), ) -> 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); + let result = normalize_and_test_predicates(tcx, predicates, key.2); debug!("substitute_normalize_and_test_predicates(key={:?}) = {:?}", key, result); result @@ -1101,7 +1102,10 @@ 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); - if !normalize_and_test_predicates(tcx, predicates.predicates) { + // 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) + { debug!("vtable_methods: predicates do not hold"); return None; } diff --git a/src/librustc/ty/query/keys.rs b/src/librustc/ty/query/keys.rs index d64f27d9cc2..20f8e7f564a 100644 --- a/src/librustc/ty/query/keys.rs +++ b/src/librustc/ty/query/keys.rs @@ -115,6 +115,15 @@ 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 diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 35dbf2f7eaa..90c97480c75 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -14,6 +14,7 @@ use rustc::mir::{ SourceInfo, SourceScope, SourceScopeData, Statement, StatementKind, Terminator, TerminatorKind, UnOp, RETURN_PLACE, }; +use rustc::traits::TraitQueryMode; use rustc::ty::layout::{ HasDataLayout, HasTyCtxt, LayoutError, LayoutOf, Size, TargetDataLayout, TyLayout, }; @@ -88,9 +89,24 @@ impl<'tcx> MirPass<'tcx> for ConstProp { // sure that it even makes sense to try to evaluate the body. // 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. + // + // 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: {:?}",