user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Auto merge of #45867 - michaelwoerister:check-ich-stability, r=nikomatsakis

Browse source 
incr.comp.: Verify stability of incr. comp. hashes and clean up various other things.

The main contribution of this PR is that it adds the `-Z incremental-verify-ich` functionality. Normally, when the red-green tracking system determines that a certain query result has not changed, it does not re-compute the incr. comp. hash (ICH) for that query result because that hash is already known. `-Z incremental-verify-ich` tells the compiler to re-hash the query result and compare the new hash against the cached hash. This is a rather thorough way of
- testing hashing implementation stability,
- finding missing `[input]` annotations on `DepNodes`, and
- finding missing read-edges,

since both a missed read and a missing `[input]` annotation can lead to something being marked as green instead of red and thus will have a different hash than it should have.

Case in point, implementing this verification logic and activating it for all `src/test/incremental` tests has revealed several such oversights, all of which are fixed in this PR.

r? @nikomatsakis
This commit is contained in:
bors 2017-11-08 22:27:06 +00:00
commit da3fbe750f
21 changed files with 329 additions and 246 deletions
Download patch file Download diff file Expand all files Collapse all files

80
src/librustc/dep_graph/dep_node.rs
View file

@ -459,10 +459,6 @@ define_dep_nodes!( <'tcx>
// Represents metadata from an extern crate.
[input] CrateMetadata(CrateNum),
// Represents some artifact that we save to disk. Note that these
// do not have a def-id as part of their identifier.
[] WorkProduct(WorkProductId),
// Represents different phases in the compiler.
[] RegionScopeTree(DefId),
[eval_always] Coherence,
@ -537,38 +533,19 @@ define_dep_nodes!( <'tcx>
// The set of impls for a given trait.
[] TraitImpls(DefId),
[] AllLocalTraitImpls,
[input] AllLocalTraitImpls,
// Trait selection cache is a little funny. Given a trait
// reference like `Foo: SomeTrait<Bar>`, there could be
// arbitrarily many def-ids to map on in there (e.g., `Foo`,
// `SomeTrait`, `Bar`). We could have a vector of them, but it
// requires heap-allocation, and trait sel in general can be a
// surprisingly hot path. So instead we pick two def-ids: the
// trait def-id, and the first def-id in the input types. If there
// is no def-id in the input types, then we use the trait def-id
// again. So for example:
//
// - `i32: Clone` -> `TraitSelect { trait_def_id: Clone, self_def_id: Clone }`
// - `u32: Clone` -> `TraitSelect { trait_def_id: Clone, self_def_id: Clone }`
// - `Clone: Clone` -> `TraitSelect { trait_def_id: Clone, self_def_id: Clone }`
// - `Vec<i32>: Clone` -> `TraitSelect { trait_def_id: Clone, self_def_id: Vec }`
// - `String: Clone` -> `TraitSelect { trait_def_id: Clone, self_def_id: String }`
// - `Foo: Trait<Bar>` -> `TraitSelect { trait_def_id: Trait, self_def_id: Foo }`
// - `Foo: Trait<i32>` -> `TraitSelect { trait_def_id: Trait, self_def_id: Foo }`
// - `(Foo, Bar): Trait` -> `TraitSelect { trait_def_id: Trait, self_def_id: Foo }`
// - `i32: Trait<Foo>` -> `TraitSelect { trait_def_id: Trait, self_def_id: Foo }`
//
// You can see that we map many trait refs to the same
// trait-select node. This is not a problem, it just means
// imprecision in our dep-graph tracking. The important thing is
// that for any given trait-ref, we always map to the **same**
// trait-select node.
[anon] TraitSelect,
[] ParamEnv(DefId),
[] DescribeDef(DefId),
[] DefSpan(DefId),
// FIXME(mw): DefSpans are not really inputs since they are derived from
// HIR. But at the moment HIR hashing still contains some hacks that allow
// to make type debuginfo to be source location independent. Declaring
// DefSpan an input makes sure that changes to these are always detected
// regardless of HIR hashing.
[input] DefSpan(DefId),
[] LookupStability(DefId),
[] LookupDeprecationEntry(DefId),
[] ItemBodyNestedBodies(DefId),
@ -588,7 +565,7 @@ define_dep_nodes!( <'tcx>
[eval_always] LintLevels,
[] Specializes { impl1: DefId, impl2: DefId },
[input] InScopeTraits(DefIndex),
[] ModuleExports(DefId),
[input] ModuleExports(DefId),
[] IsSanitizerRuntime(CrateNum),
[] IsProfilerRuntime(CrateNum),
[] GetPanicStrategy(CrateNum),
@ -598,9 +575,9 @@ define_dep_nodes!( <'tcx>
[] NativeLibraries(CrateNum),
[] PluginRegistrarFn(CrateNum),
[] DeriveRegistrarFn(CrateNum),
[] CrateDisambiguator(CrateNum),
[] CrateHash(CrateNum),
[] OriginalCrateName(CrateNum),
[input] CrateDisambiguator(CrateNum),
[input] CrateHash(CrateNum),
[input] OriginalCrateName(CrateNum),
[] ImplementationsOfTrait { krate: CrateNum, trait_id: DefId },
[] AllTraitImplementations(CrateNum),
@ -608,27 +585,27 @@ define_dep_nodes!( <'tcx>
[] IsDllimportForeignItem(DefId),
[] IsStaticallyIncludedForeignItem(DefId),
[] NativeLibraryKind(DefId),
[] LinkArgs,
[input] LinkArgs,
[] NamedRegion(DefIndex),
[] IsLateBound(DefIndex),
[] ObjectLifetimeDefaults(DefIndex),
[input] NamedRegion(DefIndex),
[input] IsLateBound(DefIndex),
[input] ObjectLifetimeDefaults(DefIndex),
[] Visibility(DefId),
[] DepKind(CrateNum),
[] CrateName(CrateNum),
[input] CrateName(CrateNum),
[] ItemChildren(DefId),
[] ExternModStmtCnum(DefId),
[] GetLangItems,
[input] GetLangItems,
[] DefinedLangItems(CrateNum),
[] MissingLangItems(CrateNum),
[] ExternConstBody(DefId),
[] VisibleParentMap,
[] MissingExternCrateItem(CrateNum),
[] UsedCrateSource(CrateNum),
[] PostorderCnums,
[] HasCloneClosures(CrateNum),
[] HasCopyClosures(CrateNum),
[input] PostorderCnums,
[input] HasCloneClosures(CrateNum),
[input] HasCopyClosures(CrateNum),
// This query is not expected to have inputs -- as a result, it's
// not a good candidate for "replay" because it's essentially a
@ -638,11 +615,11 @@ define_dep_nodes!( <'tcx>
// may save a bit of time.
[anon] EraseRegionsTy { ty: Ty<'tcx> },
[] Freevars(DefId),
[] MaybeUnusedTraitImport(DefId),
[input] Freevars(DefId),
[input] MaybeUnusedTraitImport(DefId),
[] MaybeUnusedExternCrates,
[] StabilityIndex,
[] AllCrateNums,
[input] AllCrateNums,
[] ExportedSymbols(CrateNum),
[eval_always] CollectAndPartitionTranslationItems,
[] ExportName(DefId),
@ -650,7 +627,7 @@ define_dep_nodes!( <'tcx>
[] IsTranslatedFunction(DefId),
[] CodegenUnit(InternedString),
[] CompileCodegenUnit(InternedString),
[] OutputFilenames,
[input] OutputFilenames,
[anon] NormalizeTy,
// We use this for most things when incr. comp. is turned off.
[] Null,
@ -800,13 +777,6 @@ impl WorkProductId {
hash: fingerprint
}
}
pub fn to_dep_node(self) -> DepNode {
DepNode {
kind: DepKind::WorkProduct,
hash: self.hash,
}
}
}
impl_stable_hash_for!(struct ::dep_graph::WorkProductId {

134
src/librustc/dep_graph/graph.rs
View file

@ -511,60 +511,67 @@ impl DepGraph {
return None
}
None => {
if dep_dep_node.kind.is_input() {
// This input does not exist anymore.
debug_assert!(dep_dep_node.extract_def_id(tcx).is_none(),
"Encountered input {:?} without color",
dep_dep_node);
debug!("try_mark_green({:?}) - END - dependency {:?} \
was deleted input", dep_node, dep_dep_node);
return None;
// We don't know the state of this dependency. If it isn't
// an input node, let's try to mark it green recursively.
if !dep_dep_node.kind.is_input() {
debug!("try_mark_green({:?}) --- state of dependency {:?} \
is unknown, trying to mark it green", dep_node,
dep_dep_node);
if let Some(node_index) = self.try_mark_green(tcx, dep_dep_node) {
debug!("try_mark_green({:?}) --- managed to MARK \
dependency {:?} as green", dep_node, dep_dep_node);
current_deps.push(node_index);
continue;
}
} else if cfg!(debug_assertions) {
match dep_dep_node.kind {
DepKind::Hir |
DepKind::HirBody |
DepKind::CrateMetadata => {
assert!(dep_dep_node.extract_def_id(tcx).is_none(),
"Input {:?} should have been pre-allocated but wasn't.",
dep_dep_node);
}
_ => {
// For other kinds of inputs it's OK to be
// forced.
}
}
}
debug!("try_mark_green({:?}) --- state of dependency {:?} \
is unknown, trying to mark it green", dep_node,
dep_dep_node);
// We don't know the state of this dependency. Let's try to
// mark it green.
if let Some(node_index) = self.try_mark_green(tcx, dep_dep_node) {
debug!("try_mark_green({:?}) --- managed to MARK \
dependency {:?} as green", dep_node, dep_dep_node);
current_deps.push(node_index);
} else {
// We failed to mark it green, so we try to force the query.
debug!("try_mark_green({:?}) --- trying to force \
dependency {:?}", dep_node, dep_dep_node);
if ::ty::maps::force_from_dep_node(tcx, dep_dep_node) {
let dep_dep_node_color = data.colors
.borrow()
.get(dep_dep_node)
.cloned();
match dep_dep_node_color {
Some(DepNodeColor::Green(node_index)) => {
debug!("try_mark_green({:?}) --- managed to \
FORCE dependency {:?} to green",
dep_node, dep_dep_node);
current_deps.push(node_index);
}
Some(DepNodeColor::Red) => {
debug!("try_mark_green({:?}) - END - \
dependency {:?} was red after forcing",
dep_node,
dep_dep_node);
return None
}
None => {
bug!("try_mark_green() - Forcing the DepNode \
should have set its color")
}
// We failed to mark it green, so we try to force the query.
debug!("try_mark_green({:?}) --- trying to force \
dependency {:?}", dep_node, dep_dep_node);
if ::ty::maps::force_from_dep_node(tcx, dep_dep_node) {
let dep_dep_node_color = data.colors
.borrow()
.get(dep_dep_node)
.cloned();
match dep_dep_node_color {
Some(DepNodeColor::Green(node_index)) => {
debug!("try_mark_green({:?}) --- managed to \
FORCE dependency {:?} to green",
dep_node, dep_dep_node);
current_deps.push(node_index);
}
Some(DepNodeColor::Red) => {
debug!("try_mark_green({:?}) - END - \
dependency {:?} was red after forcing",
dep_node,
dep_dep_node);
return None
}
None => {
bug!("try_mark_green() - Forcing the DepNode \
should have set its color")
}
} else {
// The DepNode could not be forced.
debug!("try_mark_green({:?}) - END - dependency {:?} \
could not be forced", dep_node, dep_dep_node);
return None
}
} else {
// The DepNode could not be forced.
debug!("try_mark_green({:?}) - END - dependency {:?} \
could not be forced", dep_node, dep_dep_node);
return None
}
}
}
@ -777,7 +784,30 @@ impl CurrentDepGraph {
read_set: _,
reads
} = popped_node {
debug_assert_eq!(node, key);
assert_eq!(node, key);
// If this is an input node, we expect that it either has no
// dependencies, or that it just depends on DepKind::CrateMetadata
// or DepKind::Krate. This happens for some "thin wrapper queries"
// like `crate_disambiguator` which sometimes have zero deps (for
// when called for LOCAL_CRATE) or they depend on a CrateMetadata
// node.
if cfg!(debug_assertions) {
if node.kind.is_input() && reads.len() > 0 &&
// FIXME(mw): Special case for DefSpan until Spans are handled
// better in general.
node.kind != DepKind::DefSpan &&
reads.iter().any(|&i| {
!(self.nodes[i].kind == DepKind::CrateMetadata ||
self.nodes[i].kind == DepKind::Krate)
})
{
bug!("Input node {:?} with unexpected reads: {:?}",
node,
reads.iter().map(|&i| self.nodes[i]).collect::<Vec<_>>())
}
}
self.alloc_node(node, reads)
} else {
bug!("pop_task() - Expected regular task to be popped")
@ -798,6 +828,8 @@ impl CurrentDepGraph {
read_set: _,
reads
} = popped_node {
debug_assert!(!kind.is_input());
let mut fingerprint = self.anon_id_seed;
let mut hasher = StableHasher::new();

5
src/librustc/hir/lowering.rs
View file

@ -838,7 +838,10 @@ impl<'a> LoweringContext<'a> {
return n;
}
assert!(!def_id.is_local());
let n = self.cstore.item_generics_cloned_untracked(def_id).regions.len();
let n = self.cstore
.item_generics_cloned_untracked(def_id, self.sess)
.regions
.len();
self.type_def_lifetime_params.insert(def_id, n);
n
});

12
src/librustc/hir/map/mod.rs
View file

@ -416,6 +416,12 @@ impl<'hir> Map<'hir> {
/// if the node is a body owner, otherwise returns `None`.
pub fn maybe_body_owned_by(&self, id: NodeId) -> Option<BodyId> {
if let Some(entry) = self.find_entry(id) {
if self.dep_graph.is_fully_enabled() {
let hir_id_owner = self.node_to_hir_id(id).owner;
let def_path_hash = self.definitions.def_path_hash(hir_id_owner);
self.dep_graph.read(def_path_hash.to_dep_node(DepKind::HirBody));
}
if let Some(body_id) = entry.associated_body() {
// For item-like things and closures, the associated
// body has its own distinct id, and that is returned
@ -530,6 +536,12 @@ impl<'hir> Map<'hir> {
/// from a node to the root of the ast (unless you get the same id back here
/// that can happen if the id is not in the map itself or is just weird).
pub fn get_parent_node(&self, id: NodeId) -> NodeId {
if self.dep_graph.is_fully_enabled() {
let hir_id_owner = self.node_to_hir_id(id).owner;
let def_path_hash = self.definitions.def_path_hash(hir_id_owner);
self.dep_graph.read(def_path_hash.to_dep_node(DepKind::HirBody));
}
self.find_entry(id).and_then(|x| x.parent_node()).unwrap_or(id)
}

4
src/librustc/ich/hcx.rs
View file

@ -227,6 +227,8 @@ impl<'gcx> StableHashingContext<'gcx> {
match binop {
hir::BiAdd |
hir::BiSub |
hir::BiShl |
hir::BiShr |
hir::BiMul => self.overflow_checks_enabled,
hir::BiDiv |
@ -237,8 +239,6 @@ impl<'gcx> StableHashingContext<'gcx> {
hir::BiBitXor |
hir::BiBitAnd |
hir::BiBitOr |
hir::BiShl |
hir::BiShr |
hir::BiEq |
hir::BiLt |
hir::BiLe |

28
src/librustc/ich/impls_hir.rs
View file

@ -356,33 +356,7 @@ impl<'gcx> HashStable<StableHashingContext<'gcx>> for hir::Block {
targeted_by_break,
} = *self;
let non_item_stmts = || stmts.iter().filter(|stmt| {
match stmt.node {
hir::StmtDecl(ref decl, _) => {
match decl.node {
// If this is a declaration of a nested item, we don't
// want to leave any trace of it in the hash value, not
// even that it exists. Otherwise changing the position
// of nested items would invalidate the containing item
// even though that does not constitute a semantic
// change.
hir::DeclItem(_) => false,
hir::DeclLocal(_) => true
}
}
hir::StmtExpr(..) |
hir::StmtSemi(..) => true
}
});
let count = non_item_stmts().count();
count.hash_stable(hcx, hasher);
for stmt in non_item_stmts() {
stmt.hash_stable(hcx, hasher);
}
stmts.hash_stable(hcx, hasher);
expr.hash_stable(hcx, hasher);
rules.hash_stable(hcx, hasher);
span.hash_stable(hcx, hasher);

4
src/librustc/middle/cstore.rs
View file

@ -273,7 +273,7 @@ pub trait CrateStore {
fn item_children_untracked(&self, did: DefId, sess: &Session) -> Vec<def::Export>;
fn load_macro_untracked(&self, did: DefId, sess: &Session) -> LoadedMacro;
fn extern_mod_stmt_cnum_untracked(&self, emod_id: ast::NodeId) -> Option<CrateNum>;
fn item_generics_cloned_untracked(&self, def: DefId) -> ty::Generics;
fn item_generics_cloned_untracked(&self, def: DefId, sess: &Session) -> ty::Generics;
fn associated_item_cloned_untracked(&self, def: DefId) -> ty::AssociatedItem;
fn postorder_cnums_untracked(&self) -> Vec<CrateNum>;
@ -327,7 +327,7 @@ impl CrateStore for DummyCrateStore {
{ bug!("crate_data_as_rc_any") }
// item info
fn visibility_untracked(&self, def: DefId) -> ty::Visibility { bug!("visibility") }
fn item_generics_cloned_untracked(&self, def: DefId) -> ty::Generics
fn item_generics_cloned_untracked(&self, def: DefId, sess: &Session) -> ty::Generics
{ bug!("item_generics_cloned") }
// trait/impl-item info

6
src/librustc/middle/resolve_lifetime.rs
View file

@ -1001,8 +1001,12 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
&map.object_lifetime_defaults[&id]
} else {
let cstore = self.cstore;
let sess = self.sess;
self.xcrate_object_lifetime_defaults.entry(def_id).or_insert_with(|| {
cstore.item_generics_cloned_untracked(def_id).types.into_iter().map(|def| {
cstore.item_generics_cloned_untracked(def_id, sess)
.types
.into_iter()
.map(|def| {
def.object_lifetime_default
}).collect()
})

2
src/librustc/session/config.rs
View file

@ -1046,6 +1046,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"print high-level information about incremental reuse (or the lack thereof)"),
incremental_dump_hash: bool = (false, parse_bool, [UNTRACKED],
"dump hash information in textual format to stdout"),
incremental_verify_ich: bool = (false, parse_bool, [UNTRACKED],
"verify incr. comp. hashes of green query instances"),
dump_dep_graph: bool = (false, parse_bool, [UNTRACKED],
"dump the dependency graph to $RUST_DEP_GRAPH (default: /tmp/dep_graph.gv)"),
query_dep_graph: bool = (false, parse_bool, [UNTRACKED],

37
src/librustc/ty/maps/plumbing.rs
View file

@ -327,7 +327,8 @@ macro_rules! define_maps {
return Self::load_from_disk_and_cache_in_memory(tcx,
key,
span,
dep_node_index)
dep_node_index,
&dep_node)
}
}
@ -372,7 +373,8 @@ macro_rules! define_maps {
fn load_from_disk_and_cache_in_memory(tcx: TyCtxt<'a, $tcx, 'lcx>,
key: $K,
span: Span,
dep_node_index: DepNodeIndex)
dep_node_index: DepNodeIndex,
dep_node: &DepNode)
-> Result<$V, CycleError<'a, $tcx>>
{
debug_assert!(tcx.dep_graph.is_green(dep_node_index));
@ -390,6 +392,32 @@ macro_rules! define_maps {
})
})?;
// If -Zincremental-verify-ich is specified, re-hash results from
// the cache and make sure that they have the expected fingerprint.
if tcx.sess.opts.debugging_opts.incremental_verify_ich {
use rustc_data_structures::stable_hasher::{StableHasher, HashStable};
use ich::Fingerprint;
assert!(Some(tcx.dep_graph.fingerprint_of(dep_node)) ==
tcx.dep_graph.prev_fingerprint_of(dep_node),
"Fingerprint for green query instance not loaded \
from cache: {:?}", dep_node);
debug!("BEGIN verify_ich({:?})", dep_node);
let mut hcx = tcx.create_stable_hashing_context();
let mut hasher = StableHasher::new();
result.hash_stable(&mut hcx, &mut hasher);
let new_hash: Fingerprint = hasher.finish();
debug!("END verify_ich({:?})", dep_node);
let old_hash = tcx.dep_graph.fingerprint_of(dep_node);
assert!(new_hash == old_hash, "Found unstable fingerprints \
for {:?}", dep_node);
}
if tcx.sess.opts.debugging_opts.query_dep_graph {
tcx.dep_graph.mark_loaded_from_cache(dep_node_index, true);
}
@ -693,9 +721,8 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
DepKind::EraseRegionsTy |
DepKind::NormalizeTy |
// These are just odd
DepKind::Null |
DepKind::WorkProduct => {
// This one should never occur in this context
DepKind::Null => {
bug!("force_from_dep_node() - Encountered {:?}", dep_node.kind)
}