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

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

Author: bors

src/librustc/dep_graph/dep_node.rs

@@ -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,
-
-    // 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.
+    [input] AllLocalTraitImpls,
+
     [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,37 +575,37 @@ 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),
 
     [] 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,19 +615,19 @@ 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),
     [] ContainsExternIndicator(DefId),
     [] 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 {

src/librustc/dep_graph/graph.rs

@@ -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();
 

src/librustc/hir/lowering.rs

@@ -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
                 });

src/librustc/hir/map/mod.rs

@@ -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)
     }
 

src/librustc/ich/hcx.rs

@@ -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 |