From 5415b34ca6077b45a241c51ef2a227993a644d26 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 24 Aug 2016 06:49:44 -0400 Subject: [PATCH 01/12] write to inherent_impls during the visitor The goal here is to avoid writing to the `inherent_impls` map from within the general `Coherence` task, and instead write to it as we visit. Writing to it from the Coherence task is actually an information leak; it happened to be safe because Coherence read from `DepNode::Krate`, but that was very coarse. I removed the `Rc` here because, upon manual inspection, nobody clones the data in this table, and it meant that we can accumulate the data in place. That said, the pattern that is used for the inherent impls map is *generally* an anti-pattern (that is, holding the borrow lock for the duration of using the contents), so it'd probably be better to clone (and I doubt that would be expensive -- how many inherent impls does a typical type have?). --- src/librustc/dep_graph/dep_tracking_map.rs | 11 +++++++ src/librustc/ty/maps.rs | 2 +- src/librustc/ty/mod.rs | 2 +- src/librustc_typeck/coherence/mod.rs | 24 ++------------- src/test/incremental/krate-inherent.rs | 34 ++++++++++++++++++++++ 5 files changed, 49 insertions(+), 24 deletions(-) create mode 100644 src/test/incremental/krate-inherent.rs diff --git a/src/librustc/dep_graph/dep_tracking_map.rs b/src/librustc/dep_graph/dep_tracking_map.rs index 88cd1efd3459a..51f7890c7a2f4 100644 --- a/src/librustc/dep_graph/dep_tracking_map.rs +++ b/src/librustc/dep_graph/dep_tracking_map.rs @@ -80,6 +80,17 @@ impl DepTrackingMap { pub fn keys(&self) -> Vec { self.map.keys().cloned().collect() } + + /// Append `elem` to the vector stored for `k`, creating a new vector if needed. + /// This is considered a write to `k`. + pub fn push(&mut self, k: M::Key, elem: E) + where M: DepTrackingMapConfig> + { + self.write(&k); + self.map.entry(k) + .or_insert(Vec::new()) + .push(elem); + } } impl MemoizationMap for RefCell> { diff --git a/src/librustc/ty/maps.rs b/src/librustc/ty/maps.rs index 0e8bea86178f3..5772d16c6d43d 100644 --- a/src/librustc/ty/maps.rs +++ b/src/librustc/ty/maps.rs @@ -39,7 +39,7 @@ dep_map_ty! { ImplTraitRefs: ItemSignature(DefId) -> Option> dep_map_ty! { TraitDefs: ItemSignature(DefId) -> &'tcx ty::TraitDef<'tcx> } dep_map_ty! { AdtDefs: ItemSignature(DefId) -> ty::AdtDefMaster<'tcx> } dep_map_ty! { ItemVariances: ItemSignature(DefId) -> Rc> } -dep_map_ty! { InherentImpls: InherentImpls(DefId) -> Rc> } +dep_map_ty! { InherentImpls: InherentImpls(DefId) -> Vec } dep_map_ty! { ImplItems: ImplItems(DefId) -> Vec } dep_map_ty! { TraitItems: TraitItems(DefId) -> Rc>> } dep_map_ty! { ReprHints: ReprHints(DefId) -> Rc> } diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index dfe24d5627bf1..78358ce534d99 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -2665,7 +2665,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { self.impl_items.borrow_mut().insert(impl_def_id, impl_items); } - self.inherent_impls.borrow_mut().insert(type_id, Rc::new(inherent_impls)); + self.inherent_impls.borrow_mut().insert(type_id, inherent_impls); self.populated_external_types.borrow_mut().insert(type_id); } diff --git a/src/librustc_typeck/coherence/mod.rs b/src/librustc_typeck/coherence/mod.rs index d2b7f07b9ce6c..57d3c46af9306 100644 --- a/src/librustc_typeck/coherence/mod.rs +++ b/src/librustc_typeck/coherence/mod.rs @@ -32,10 +32,7 @@ use rustc::ty::util::CopyImplementationError; use middle::free_region::FreeRegionMap; use CrateCtxt; use rustc::infer::{self, InferCtxt, TypeOrigin}; -use std::cell::RefCell; -use std::rc::Rc; use syntax_pos::Span; -use util::nodemap::{DefIdMap, FnvHashMap}; use rustc::dep_graph::DepNode; use rustc::hir::map as hir_map; use rustc::hir::intravisit; @@ -49,7 +46,6 @@ mod unsafety; struct CoherenceChecker<'a, 'gcx: 'a+'tcx, 'tcx: 'a> { crate_context: &'a CrateCtxt<'a, 'gcx>, inference_context: InferCtxt<'a, 'gcx, 'tcx>, - inherent_impls: RefCell>>>>, } struct CoherenceCheckVisitor<'a, 'gcx: 'a+'tcx, 'tcx: 'a> { @@ -109,15 +105,6 @@ impl<'a, 'gcx, 'tcx> CoherenceChecker<'a, 'gcx, 'tcx> { DepNode::CoherenceCheckImpl, &mut CoherenceCheckVisitor { cc: self }); - // Copy over the inherent impls we gathered up during the walk into - // the tcx. - let mut tcx_inherent_impls = - self.crate_context.tcx.inherent_impls.borrow_mut(); - for (k, v) in self.inherent_impls.borrow().iter() { - tcx_inherent_impls.insert((*k).clone(), - Rc::new((*v.borrow()).clone())); - } - // Populate the table of destructors. It might seem a bit strange to // do this here, but it's actually the most convenient place, since // the coherence tables contain the trait -> type mappings. @@ -175,14 +162,8 @@ impl<'a, 'gcx, 'tcx> CoherenceChecker<'a, 'gcx, 'tcx> { } fn add_inherent_impl(&self, base_def_id: DefId, impl_def_id: DefId) { - if let Some(implementation_list) = self.inherent_impls.borrow().get(&base_def_id) { - implementation_list.borrow_mut().push(impl_def_id); - return; - } - - self.inherent_impls.borrow_mut().insert( - base_def_id, - Rc::new(RefCell::new(vec!(impl_def_id)))); + let tcx = self.crate_context.tcx; + tcx.inherent_impls.borrow_mut().push(base_def_id, impl_def_id); } fn add_trait_impl(&self, impl_trait_ref: ty::TraitRef<'gcx>, impl_def_id: DefId) { @@ -556,7 +537,6 @@ pub fn check_coherence(ccx: &CrateCtxt) { CoherenceChecker { crate_context: ccx, inference_context: infcx, - inherent_impls: RefCell::new(FnvHashMap()), }.check(); }); unsafety::check(ccx.tcx); diff --git a/src/test/incremental/krate-inherent.rs b/src/test/incremental/krate-inherent.rs new file mode 100644 index 0000000000000..ac6cc3e9826f1 --- /dev/null +++ b/src/test/incremental/krate-inherent.rs @@ -0,0 +1,34 @@ +// Copyright 2014 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// revisions: rpass1 rpass2 +// compile-flags: -Z query-dep-graph + +#![allow(warnings)] +#![feature(rustc_attrs)] +#![rustc_partition_reused(module="krate_inherent-x", cfg="rpass2")] + +fn main() { } + +mod x { + struct Foo; + impl Foo { + fn foo(&self) { } + } + + fn method() { + let x: Foo = Foo; + x.foo(); // inherent methods used to add an edge from Krate + } +} + +#[cfg(rpass1)] +fn bar() { } // remove this unrelated fn in rpass2, which should not affect `x::method` + From c6363b801316c1d8279f55a6be44746d71018ceb Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 24 Aug 2016 07:35:49 -0400 Subject: [PATCH 02/12] ignore dep-graph when loading inlined HIR We touch the krate to adjust the maps, but we don't expose that data widely. --- src/librustc/hir/map/mod.rs | 2 ++ src/test/incremental/krate-inlined.rs | 29 +++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 src/test/incremental/krate-inlined.rs diff --git a/src/librustc/hir/map/mod.rs b/src/librustc/hir/map/mod.rs index 3ffc95e64f5a7..5c302d927a718 100644 --- a/src/librustc/hir/map/mod.rs +++ b/src/librustc/hir/map/mod.rs @@ -927,6 +927,8 @@ pub fn map_decoded_item<'ast, F: FoldOps>(map: &Map<'ast>, ii: InlinedItem, fold_ops: F) -> &'ast InlinedItem { + let _ignore = map.forest.dep_graph.in_ignore(); + let mut fld = IdAndSpanUpdater::new(fold_ops); let ii = match ii { II::Item(d, i) => II::Item(fld.fold_ops.new_def_id(d), diff --git a/src/test/incremental/krate-inlined.rs b/src/test/incremental/krate-inlined.rs new file mode 100644 index 0000000000000..3680658984792 --- /dev/null +++ b/src/test/incremental/krate-inlined.rs @@ -0,0 +1,29 @@ +// Copyright 2014 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// revisions: rpass1 rpass2 +// compile-flags: -Z query-dep-graph + +#![allow(warnings)] +#![feature(rustc_attrs)] +#![rustc_partition_reused(module="krate_inlined-x", cfg="rpass2")] + +fn main() { } + +mod x { + fn method() { + // use some methods that require inlining HIR from another crate: + let mut v = vec![]; + v.push(1); + } +} + +#[cfg(rpass1)] +fn bar() { } // remove this unrelated fn in rpass2, which should not affect `x::method` From 753590f0c52c0ca54e7b80c1cc90f72bb8bbc8fb Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 24 Aug 2016 07:36:54 -0400 Subject: [PATCH 03/12] add a debugging mechanism to forbid edges It is useful to track down an errant edge that is being added. This is not a perfect mechanism, since it doesn't consider (e.g.) if we are in an ignored task, but it's helpful enough for now. --- src/librustc/dep_graph/graph.rs | 18 +++++++++++++++++- src/librustc/dep_graph/raii.rs | 18 ++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/librustc/dep_graph/graph.rs b/src/librustc/dep_graph/graph.rs index bb027b11b45af..ca441c4bc3ff2 100644 --- a/src/librustc/dep_graph/graph.rs +++ b/src/librustc/dep_graph/graph.rs @@ -38,6 +38,10 @@ struct DepGraphData { /// Work-products that we generate in this run. work_products: RefCell, WorkProduct>>, + + /// Tracks nodes that are forbidden to read/write; see + /// `DepGraph::forbid`. Used for debugging only. + forbidden: RefCell>>, } impl DepGraph { @@ -46,7 +50,8 @@ impl DepGraph { data: Rc::new(DepGraphData { thread: DepGraphThreadData::new(enabled), previous_work_products: RefCell::new(FnvHashMap()), - work_products: RefCell::new(FnvHashMap()) + work_products: RefCell::new(FnvHashMap()), + forbidden: RefCell::new(Vec::new()), }) } } @@ -70,6 +75,15 @@ impl DepGraph { raii::DepTask::new(&self.data.thread, key) } + /// Debugging aid -- forbid reads/writes to `key` while the return + /// value is in scope. Note that this is only available when debug + /// assertions are enabled -- you should not check in code that + /// invokes this function. + #[cfg(debug_assertions)] + pub fn forbid<'graph>(&'graph self, key: DepNode) -> raii::Forbid<'graph> { + raii::Forbid::new(&self.data.forbidden, key) + } + pub fn with_ignore(&self, op: OP) -> R where OP: FnOnce() -> R { @@ -85,10 +99,12 @@ impl DepGraph { } pub fn read(&self, v: DepNode) { + debug_assert!(!self.data.forbidden.borrow().contains(&v)); self.data.thread.enqueue(DepMessage::Read(v)); } pub fn write(&self, v: DepNode) { + debug_assert!(!self.data.forbidden.borrow().contains(&v)); self.data.thread.enqueue(DepMessage::Write(v)); } diff --git a/src/librustc/dep_graph/raii.rs b/src/librustc/dep_graph/raii.rs index c43d493d17675..b344eb486240b 100644 --- a/src/librustc/dep_graph/raii.rs +++ b/src/librustc/dep_graph/raii.rs @@ -9,6 +9,7 @@ // except according to those terms. use hir::def_id::DefId; +use std::cell::RefCell; use super::DepNode; use super::thread::{DepGraphThreadData, DepMessage}; @@ -47,3 +48,20 @@ impl<'graph> Drop for IgnoreTask<'graph> { self.data.enqueue(DepMessage::PopIgnore); } } + +pub struct Forbid<'graph> { + forbidden: &'graph RefCell>> +} + +impl<'graph> Forbid<'graph> { + pub fn new(forbidden: &'graph RefCell>>, node: DepNode) -> Self { + forbidden.borrow_mut().push(node); + Forbid { forbidden: forbidden } + } +} + +impl<'graph> Drop for Forbid<'graph> { + fn drop(&mut self) { + self.forbidden.borrow_mut().pop(); + } +} From 2a84449169b3c882e101a68eb156800fe8ff24c3 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 24 Aug 2016 11:00:55 -0400 Subject: [PATCH 04/12] allow testing DepNode::Krate edges directly --- src/librustc_incremental/assert_dep_graph.rs | 158 +++++++++---------- src/test/incremental/krate-inlined.rs | 12 +- 2 files changed, 78 insertions(+), 92 deletions(-) diff --git a/src/librustc_incremental/assert_dep_graph.rs b/src/librustc_incremental/assert_dep_graph.rs index bd96ae69ffbc8..b28454cddb247 100644 --- a/src/librustc_incremental/assert_dep_graph.rs +++ b/src/librustc_incremental/assert_dep_graph.rs @@ -26,19 +26,20 @@ //! used to check when paths exist or do not. //! //! The full form of the `rustc_if_this_changed` annotation is -//! `#[rustc_if_this_changed(id)]`. The `"id"` is optional and -//! defaults to `"id"` if omitted. +//! `#[rustc_if_this_changed("foo")]`, which will report a +//! source node of `foo(def_id)`. The `"foo"` is optional and +//! defaults to `"Hir"` if omitted. //! //! Example: //! //! ``` -//! #[rustc_if_this_changed] +//! #[rustc_if_this_changed(Hir)] //! fn foo() { } //! -//! #[rustc_then_this_would_need("trans")] //~ ERROR no path from `foo` +//! #[rustc_then_this_would_need(trans)] //~ ERROR no path from `foo` //! fn bar() { } //! -//! #[rustc_then_this_would_need("trans")] //~ ERROR OK +//! #[rustc_then_this_would_need(trans)] //~ ERROR OK //! fn baz() { foo(); } //! ``` @@ -47,7 +48,7 @@ use rustc::dep_graph::{DepGraphQuery, DepNode}; use rustc::dep_graph::debug::{DepNodeFilter, EdgeFilter}; use rustc::hir::def_id::DefId; use rustc::ty::TyCtxt; -use rustc_data_structures::fnv::{FnvHashMap, FnvHashSet}; +use rustc_data_structures::fnv::FnvHashSet; use rustc_data_structures::graph::{Direction, INCOMING, OUTGOING, NodeIndex}; use rustc::hir; use rustc::hir::intravisit::Visitor; @@ -61,7 +62,6 @@ use syntax_pos::Span; const IF_THIS_CHANGED: &'static str = "rustc_if_this_changed"; const THEN_THIS_WOULD_NEED: &'static str = "rustc_then_this_would_need"; -const ID: &'static str = "id"; pub fn assert_dep_graph<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) { let _ignore = tcx.dep_graph.in_ignore(); @@ -80,8 +80,9 @@ pub fn assert_dep_graph<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) { // Find annotations supplied by user (if any). let (if_this_changed, then_this_would_need) = { let mut visitor = IfThisChanged { tcx: tcx, - if_this_changed: FnvHashMap(), - then_this_would_need: FnvHashMap() }; + if_this_changed: vec![], + then_this_would_need: vec![] }; + visitor.process_attrs(ast::CRATE_NODE_ID, &tcx.map.krate().attrs); tcx.map.krate().visit_all_items(&mut visitor); (visitor.if_this_changed, visitor.then_this_would_need) }; @@ -97,58 +98,51 @@ pub fn assert_dep_graph<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) { check_paths(tcx, &if_this_changed, &then_this_would_need); } -type SourceHashMap = - FnvHashMap)>>; -type TargetHashMap = - FnvHashMap)>>; +type Sources = Vec<(Span, DefId, DepNode)>; +type Targets = Vec<(Span, InternedString, ast::NodeId, DepNode)>; struct IfThisChanged<'a, 'tcx:'a> { tcx: TyCtxt<'a, 'tcx, 'tcx>, - if_this_changed: SourceHashMap, - then_this_would_need: TargetHashMap, + if_this_changed: Sources, + then_this_would_need: Targets, } impl<'a, 'tcx> IfThisChanged<'a, 'tcx> { - fn process_attrs(&mut self, node_id: ast::NodeId, def_id: DefId) { - for attr in self.tcx.get_attrs(def_id).iter() { + fn argument(&self, attr: &ast::Attribute) -> Option { + let mut value = None; + for list_item in attr.meta_item_list().unwrap_or_default() { + match list_item.word() { + Some(word) if value.is_none() => + value = Some(word.name().clone()), + _ => + // FIXME better-encapsulate meta_item (don't directly access `node`) + span_bug!(list_item.span(), "unexpected meta-item {:?}", list_item.node), + } + } + value + } + + fn process_attrs(&mut self, node_id: ast::NodeId, attrs: &[ast::Attribute]) { + let def_id = self.tcx.map.local_def_id(node_id); + for attr in attrs { if attr.check_name(IF_THIS_CHANGED) { - let mut id = None; - for list_item in attr.meta_item_list().unwrap_or_default() { - match list_item.word() { - Some(word) if id.is_none() => { - id = Some(word.name().clone()) - }, - _ => { - // FIXME better-encapsulate meta_item (don't directly access `node`) - span_bug!(list_item.span(), "unexpected list-item {:?}", list_item.node) + let dep_node_interned = self.argument(attr); + let dep_node = match dep_node_interned { + None => DepNode::Hir(def_id), + Some(ref n) => { + match DepNode::from_label_string(&n[..], def_id) { + Ok(n) => n, + Err(()) => { + self.tcx.sess.span_fatal( + attr.span, + &format!("unrecognized DepNode variant {:?}", n)); + } } } - } - - let id = id.unwrap_or(InternedString::new(ID)); - self.if_this_changed.entry(id) - .or_insert(FnvHashSet()) - .insert((attr.span, def_id, DepNode::Hir(def_id))); + }; + self.if_this_changed.push((attr.span, def_id, dep_node)); } else if attr.check_name(THEN_THIS_WOULD_NEED) { - let mut dep_node_interned = None; - let mut id = None; - for list_item in attr.meta_item_list().unwrap_or_default() { - match list_item.word() { - Some(word) if dep_node_interned.is_none() => { - dep_node_interned = Some(word.name().clone()); - }, - Some(word) if id.is_none() => { - id = Some(word.name().clone()) - }, - _ => { - // FIXME better-encapsulate meta_item (don't directly access `node`) - span_bug!(list_item.span(), "unexpected meta-item {:?}", list_item.node) - } - } - } - + let dep_node_interned = self.argument(attr); let dep_node = match dep_node_interned { Some(ref n) => { match DepNode::from_label_string(&n[..], def_id) { @@ -166,11 +160,10 @@ impl<'a, 'tcx> IfThisChanged<'a, 'tcx> { &format!("missing DepNode variant")); } }; - let id = id.unwrap_or(InternedString::new(ID)); - self.then_this_would_need - .entry(id) - .or_insert(FnvHashSet()) - .insert((attr.span, dep_node_interned.clone().unwrap(), node_id, dep_node)); + self.then_this_would_need.push((attr.span, + dep_node_interned.clone().unwrap(), + node_id, + dep_node)); } } } @@ -178,47 +171,38 @@ impl<'a, 'tcx> IfThisChanged<'a, 'tcx> { impl<'a, 'tcx> Visitor<'tcx> for IfThisChanged<'a, 'tcx> { fn visit_item(&mut self, item: &'tcx hir::Item) { - let def_id = self.tcx.map.local_def_id(item.id); - self.process_attrs(item.id, def_id); + self.process_attrs(item.id, &item.attrs); } } fn check_paths<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, - if_this_changed: &SourceHashMap, - then_this_would_need: &TargetHashMap) + if_this_changed: &Sources, + then_this_would_need: &Targets) { // Return early here so as not to construct the query, which is not cheap. if if_this_changed.is_empty() { + for &(target_span, _, _, _) in then_this_would_need { + tcx.sess.span_err( + target_span, + &format!("no #[rustc_if_this_changed] annotation detected")); + + } return; } let query = tcx.dep_graph.query(); - for (id, sources) in if_this_changed { - let targets = match then_this_would_need.get(id) { - Some(targets) => targets, - None => { - for &(source_span, ..) in sources.iter().take(1) { - tcx.sess.span_err( - source_span, - &format!("no targets for id `{}`", id)); - } - continue; - } - }; - - for &(_, source_def_id, ref source_dep_node) in sources { - let dependents = query.transitive_successors(source_dep_node); - for &(target_span, ref target_pass, _, ref target_dep_node) in targets { - if !dependents.contains(&target_dep_node) { - tcx.sess.span_err( - target_span, - &format!("no path from `{}` to `{}`", - tcx.item_path_str(source_def_id), - target_pass)); - } else { - tcx.sess.span_err( - target_span, - &format!("OK")); - } + for &(_, source_def_id, ref source_dep_node) in if_this_changed { + let dependents = query.transitive_successors(source_dep_node); + for &(target_span, ref target_pass, _, ref target_dep_node) in then_this_would_need { + if !dependents.contains(&target_dep_node) { + tcx.sess.span_err( + target_span, + &format!("no path from `{}` to `{}`", + tcx.item_path_str(source_def_id), + target_pass)); + } else { + tcx.sess.span_err( + target_span, + &format!("OK")); } } } diff --git a/src/test/incremental/krate-inlined.rs b/src/test/incremental/krate-inlined.rs index 3680658984792..ba32b41983fc2 100644 --- a/src/test/incremental/krate-inlined.rs +++ b/src/test/incremental/krate-inlined.rs @@ -8,22 +8,24 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// revisions: rpass1 rpass2 +// Regr. test that using HIR inlined from another krate does *not* add +// a dependency from the local Krate node. + +// revisions: cfail1 // compile-flags: -Z query-dep-graph #![allow(warnings)] #![feature(rustc_attrs)] -#![rustc_partition_reused(module="krate_inlined-x", cfg="rpass2")] + +#![rustc_if_this_changed(Krate)] fn main() { } mod x { + #[rustc_then_this_would_need(TransCrateItem)] //[cfail1]~ ERROR no path fn method() { // use some methods that require inlining HIR from another crate: let mut v = vec![]; v.push(1); } } - -#[cfg(rpass1)] -fn bar() { } // remove this unrelated fn in rpass2, which should not affect `x::method` From 4c2f3ff44263b813d4211150613edff0c5c92c30 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 24 Aug 2016 11:01:12 -0400 Subject: [PATCH 05/12] remove the "misc-items" from meta-data It was duplicating information available elsewhere. --- src/librustc_metadata/common.rs | 4 ++-- src/librustc_metadata/decoder.rs | 11 ++-------- src/librustc_metadata/encoder.rs | 35 +++++--------------------------- 3 files changed, 9 insertions(+), 41 deletions(-) diff --git a/src/librustc_metadata/common.rs b/src/librustc_metadata/common.rs index 1e6c74bef8da5..29b9cc0d1d923 100644 --- a/src/librustc_metadata/common.rs +++ b/src/librustc_metadata/common.rs @@ -149,9 +149,9 @@ pub const tag_items_data_item_visibility: usize = 0x78; pub const tag_items_data_item_inherent_impl: usize = 0x79; // GAP 0x7a pub const tag_mod_child: usize = 0x7b; -pub const tag_misc_info: usize = 0x108; // top-level only -pub const tag_misc_info_crate_items: usize = 0x7c; +// GAP 0x7c +// GAP 0x108 pub const tag_impls: usize = 0x109; // top-level only pub const tag_impls_trait: usize = 0x7d; pub const tag_impls_trait_impl: usize = 0x7e; diff --git a/src/librustc_metadata/decoder.rs b/src/librustc_metadata/decoder.rs index 6b48b4dfabcfd..d75d5a3b35443 100644 --- a/src/librustc_metadata/decoder.rs +++ b/src/librustc_metadata/decoder.rs @@ -23,6 +23,7 @@ use index; use tls_context; use tydecode::TyDecoder; +use rustc::hir::def_id::CRATE_DEF_INDEX; use rustc::hir::svh::Svh; use rustc::hir::map as hir_map; use rustc::hir::map::DefKey; @@ -732,15 +733,7 @@ pub fn each_top_level_item_of_crate(cdata: Cmd, get_crate_data: G, callbac where F: FnMut(DefLike, ast::Name, ty::Visibility), G: FnMut(ast::CrateNum) -> Rc, { - let root_doc = rbml::Doc::new(cdata.data()); - let misc_info_doc = reader::get_doc(root_doc, tag_misc_info); - let crate_items_doc = reader::get_doc(misc_info_doc, - tag_misc_info_crate_items); - - each_child_of_item_or_crate(cdata, - crate_items_doc, - get_crate_data, - callback) + each_child_of_item(cdata, CRATE_DEF_INDEX, get_crate_data, callback) } pub fn get_item_name(cdata: Cmd, id: DefIndex) -> ast::Name { diff --git a/src/librustc_metadata/encoder.rs b/src/librustc_metadata/encoder.rs index 23398a0400c51..603b7a483b90c 100644 --- a/src/librustc_metadata/encoder.rs +++ b/src/librustc_metadata/encoder.rs @@ -1693,30 +1693,6 @@ fn encode_impls<'a>(ecx: &'a EncodeContext, rbml_w.end_tag(); } -fn encode_misc_info(ecx: &EncodeContext, - krate: &hir::Crate, - rbml_w: &mut Encoder) { - rbml_w.start_tag(tag_misc_info); - rbml_w.start_tag(tag_misc_info_crate_items); - for item_id in &krate.module.item_ids { - rbml_w.wr_tagged_u64(tag_mod_child, - def_to_u64(ecx.tcx.map.local_def_id(item_id.id))); - - let item = ecx.tcx.map.expect_item(item_id.id); - each_auxiliary_node_id(item, |auxiliary_node_id| { - rbml_w.wr_tagged_u64(tag_mod_child, - def_to_u64(ecx.tcx.map.local_def_id(auxiliary_node_id))); - true - }); - } - - // Encode reexports for the root module. - encode_reexports(ecx, rbml_w, 0); - - rbml_w.end_tag(); - rbml_w.end_tag(); -} - // Encodes all reachable symbols in this crate into the metadata. // // This pass is seeded off the reachability list calculated in the @@ -1861,7 +1837,7 @@ fn encode_metadata_inner(rbml_w: &mut Encoder, codemap_bytes: u64, macro_defs_bytes: u64, impl_bytes: u64, - misc_bytes: u64, + reachable_bytes: u64, item_bytes: u64, index_bytes: u64, xref_bytes: u64, @@ -1877,7 +1853,7 @@ fn encode_metadata_inner(rbml_w: &mut Encoder, codemap_bytes: 0, macro_defs_bytes: 0, impl_bytes: 0, - misc_bytes: 0, + reachable_bytes: 0, item_bytes: 0, index_bytes: 0, xref_bytes: 0, @@ -1931,11 +1907,10 @@ fn encode_metadata_inner(rbml_w: &mut Encoder, encode_impls(&ecx, krate, rbml_w); stats.impl_bytes = rbml_w.writer.seek(SeekFrom::Current(0)).unwrap() - i; - // Encode miscellaneous info. + // Encode reachability info. i = rbml_w.writer.seek(SeekFrom::Current(0)).unwrap(); - encode_misc_info(&ecx, krate, rbml_w); encode_reachable(&ecx, rbml_w); - stats.misc_bytes = rbml_w.writer.seek(SeekFrom::Current(0)).unwrap() - i; + stats.reachable_bytes = rbml_w.writer.seek(SeekFrom::Current(0)).unwrap() - i; // Encode and index the items. rbml_w.start_tag(tag_items); @@ -1972,7 +1947,7 @@ fn encode_metadata_inner(rbml_w: &mut Encoder, println!(" codemap bytes: {}", stats.codemap_bytes); println!(" macro def bytes: {}", stats.macro_defs_bytes); println!(" impl bytes: {}", stats.impl_bytes); - println!(" misc bytes: {}", stats.misc_bytes); + println!(" reachable bytes: {}", stats.reachable_bytes); println!(" item bytes: {}", stats.item_bytes); println!(" index bytes: {}", stats.index_bytes); println!(" xref bytes: {}", stats.xref_bytes); From 2f91ba05fd930e003bb17b763a32382cfe4392a8 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 2 Sep 2016 07:50:18 -0400 Subject: [PATCH 06/12] implement a debugging "shadow graph" The shadow graph supercedes the existing code that checked for reads/writes without an active task and now adds the ability to filter for specific edges. --- src/librustc/dep_graph/README.md | 31 ++++++++ src/librustc/dep_graph/debug.rs | 7 ++ src/librustc/dep_graph/mod.rs | 1 + src/librustc/dep_graph/shadow.rs | 123 +++++++++++++++++++++++++++++++ src/librustc/dep_graph/thread.rs | 41 ++++------- src/librustc/lib.rs | 1 + 6 files changed, 176 insertions(+), 28 deletions(-) create mode 100644 src/librustc/dep_graph/shadow.rs diff --git a/src/librustc/dep_graph/README.md b/src/librustc/dep_graph/README.md index f16a9b386bb8a..48f5b7ea2595d 100644 --- a/src/librustc/dep_graph/README.md +++ b/src/librustc/dep_graph/README.md @@ -341,6 +341,8 @@ path is found (as demonstrated above). ### Debugging the dependency graph +#### Dumping the graph + The compiler is also capable of dumping the dependency graph for your debugging pleasure. To do so, pass the `-Z dump-dep-graph` flag. The graph will be dumped to `dep_graph.{txt,dot}` in the current @@ -392,6 +394,35 @@ This will dump out all the nodes that lead from `Hir(foo)` to `TypeckItemBody(bar)`, from which you can (hopefully) see the source of the erroneous edge. +#### Tracking down incorrect edges + +Sometimes, after you dump the dependency graph, you will find some +path that should not exist, but you will not be quite sure how it came +to be. **When the compiler is built with debug assertions,** it can +help you track that down. Simply set the `RUST_FORBID_DEP_GRAPH_EDGE` +environment variable to a filter. Every edge created in the dep-graph +will be tested against that filter -- if it matches, a `bug!` is +reported, so you can easily see the backtrace (`RUST_BACKTRACE=1`). + +The syntax for these filters is the same as described in the previous +section. However, note that this filter is applied to every **edge** +and doesn't handle longer paths in the graph, unlike the previous +section. + +Example: + +You find that there is a path from the `Hir` of `foo` to the type +check of `bar` and you don't think there should be. You dump the +dep-graph as described in the previous section and open `dep-graph.txt` +to see something like: + + Hir(foo) -> Collect(bar) + Collect(bar) -> TypeckItemBody(bar) + +That first edge looks suspicious to you. So you set +`RUST_FORBID_DEP_GRAPH_EDGE` to `Hir&foo -> Collect&bar`, re-run, and +then observe the backtrace. Voila, bug fixed! + ### Inlining of HIR nodes For the time being, at least, we still sometimes "inline" HIR nodes diff --git a/src/librustc/dep_graph/debug.rs b/src/librustc/dep_graph/debug.rs index 15b0380374c69..5b15c5e67174e 100644 --- a/src/librustc/dep_graph/debug.rs +++ b/src/librustc/dep_graph/debug.rs @@ -66,4 +66,11 @@ impl EdgeFilter { }) } } + + pub fn test(&self, + source: &DepNode, + target: &DepNode) + -> bool { + self.source.test(source) && self.target.test(target) + } } diff --git a/src/librustc/dep_graph/mod.rs b/src/librustc/dep_graph/mod.rs index a499cb10f2325..9c00e95c17e05 100644 --- a/src/librustc/dep_graph/mod.rs +++ b/src/librustc/dep_graph/mod.rs @@ -15,6 +15,7 @@ mod edges; mod graph; mod query; mod raii; +mod shadow; mod thread; mod visit; diff --git a/src/librustc/dep_graph/shadow.rs b/src/librustc/dep_graph/shadow.rs new file mode 100644 index 0000000000000..e038997e0ad42 --- /dev/null +++ b/src/librustc/dep_graph/shadow.rs @@ -0,0 +1,123 @@ +//! The "Shadow Graph" is maintained on the main thread and which +//! tracks each message relating to the dep-graph and applies some +//! sanity checks as they go by. If an error results, it means you get +//! a nice stack-trace telling you precisely what caused the error. +//! +//! NOTE: This is a debugging facility which can potentially have non-trivial +//! runtime impact. Therefore, it is largely compiled out if +//! debug-assertions are not enabled. +//! +//! The basic sanity check, always enabled, is that there is always a +//! task (or ignore) on the stack when you do read/write. +//! +//! Optionally, if you specify RUST_FORBID_DEP_GRAPH_EDGE, you can +//! specify an edge filter to be applied to each edge as it is +//! created. See `./README.md` for details. + +use hir::def_id::DefId; +use std::cell::{BorrowState, RefCell}; +use std::env; + +use super::DepNode; +use super::thread::DepMessage; +use super::debug::EdgeFilter; + +pub struct ShadowGraph { + // if you push None onto the stack, that corresponds to an Ignore + stack: RefCell>>>, + forbidden_edge: Option, +} + +const ENABLED: bool = cfg!(debug_assertions); + +impl ShadowGraph { + pub fn new() -> Self { + let forbidden_edge = if !ENABLED { + None + } else { + match env::var("RUST_FORBID_DEP_GRAPH_EDGE") { + Ok(s) => { + match EdgeFilter::new(&s) { + Ok(f) => Some(f), + Err(err) => bug!("RUST_FORBID_DEP_GRAPH_EDGE invalid: {}", err), + } + } + Err(_) => None, + } + }; + + ShadowGraph { + stack: RefCell::new(vec![]), + forbidden_edge: forbidden_edge, + } + } + + pub fn enqueue(&self, message: &DepMessage) { + if ENABLED { + match self.stack.borrow_state() { + BorrowState::Unused => {} + _ => { + // When we apply edge filters, that invokes the + // Debug trait on DefIds, which in turn reads from + // various bits of state and creates reads! Ignore + // those recursive reads. + return; + } + } + + let mut stack = self.stack.borrow_mut(); + match *message { + DepMessage::Read(ref n) => self.check_edge(Some(Some(n)), top(&stack)), + DepMessage::Write(ref n) => self.check_edge(top(&stack), Some(Some(n))), + DepMessage::PushTask(ref n) => stack.push(Some(n.clone())), + DepMessage::PushIgnore => stack.push(None), + DepMessage::PopTask(_) | + DepMessage::PopIgnore => { + // we could easily check that the stack is + // well-formed here, but since we use closures and + // RAII accessors, this bug basically never + // happens, so it seems not worth the overhead + stack.pop(); + } + DepMessage::Query => (), + } + } + } + + fn check_edge(&self, + source: Option>>, + target: Option>>) { + assert!(ENABLED); + match (source, target) { + // cannot happen, one side is always Some(Some(_)) + (None, None) => unreachable!(), + + // nothing on top of the stack + (None, Some(n)) | (Some(n), None) => bug!("read/write of {:?} but no current task", n), + + // this corresponds to an Ignore being top of the stack + (Some(None), _) | (_, Some(None)) => (), + + // a task is on top of the stack + (Some(Some(source)), Some(Some(target))) => { + if let Some(ref forbidden_edge) = self.forbidden_edge { + if forbidden_edge.test(source, target) { + bug!("forbidden edge {:?} -> {:?} created", source, target) + } + } + } + } + } +} + +// Do a little juggling: we get back a reference to an option at the +// top of the stack, convert it to an optional reference. +fn top<'s>(stack: &'s Vec>>) -> Option>> { + stack.last() + .map(|n: &'s Option>| -> Option<&'s DepNode> { + // (*) + // (*) type annotation just there to clarify what would + // otherwise be some *really* obscure code + n.as_ref() + }) +} diff --git a/src/librustc/dep_graph/thread.rs b/src/librustc/dep_graph/thread.rs index 4e16fae187070..90c42d66b7adf 100644 --- a/src/librustc/dep_graph/thread.rs +++ b/src/librustc/dep_graph/thread.rs @@ -20,13 +20,13 @@ use hir::def_id::DefId; use rustc_data_structures::veccell::VecCell; -use std::cell::Cell; use std::sync::mpsc::{self, Sender, Receiver}; use std::thread; use super::DepGraphQuery; use super::DepNode; use super::edges::DepGraphEdges; +use super::shadow::ShadowGraph; #[derive(Debug)] pub enum DepMessage { @@ -42,12 +42,16 @@ pub enum DepMessage { pub struct DepGraphThreadData { enabled: bool, - // Local counter that just tracks how many tasks are pushed onto the - // stack, so that we still get an error in the case where one is - // missing. If dep-graph construction is enabled, we'd get the same - // error when processing tasks later on, but that's annoying because - // it lacks precision about the source of the error. - tasks_pushed: Cell, + // The "shadow graph" is a debugging aid. We give it each message + // in real time as it arrives and it checks for various errors + // (for example, a read/write when there is no current task; it + // can also apply user-defined filters; see `shadow` module for + // details). This only occurs if debug-assertions are enabled. + // + // Note that in some cases the same errors will occur when the + // data is processed off the main thread, but that's annoying + // because it lacks precision about the source of the error. + shadow_graph: ShadowGraph, // current buffer, where we accumulate messages messages: VecCell, @@ -76,7 +80,7 @@ impl DepGraphThreadData { DepGraphThreadData { enabled: enabled, - tasks_pushed: Cell::new(0), + shadow_graph: ShadowGraph::new(), messages: VecCell::with_capacity(INITIAL_CAPACITY), swap_in: rx2, swap_out: tx1, @@ -118,21 +122,7 @@ impl DepGraphThreadData { /// the buffer is full, this may swap.) #[inline] pub fn enqueue(&self, message: DepMessage) { - // Regardless of whether dep graph construction is enabled, we - // still want to check that we always have a valid task on the - // stack when a read/write/etc event occurs. - match message { - DepMessage::Read(_) | DepMessage::Write(_) => - if self.tasks_pushed.get() == 0 { - self.invalid_message("read/write but no current task") - }, - DepMessage::PushTask(_) | DepMessage::PushIgnore => - self.tasks_pushed.set(self.tasks_pushed.get() + 1), - DepMessage::PopTask(_) | DepMessage::PopIgnore => - self.tasks_pushed.set(self.tasks_pushed.get() - 1), - DepMessage::Query => - (), - } + self.shadow_graph.enqueue(&message); if self.enabled { self.enqueue_enabled(message); @@ -147,11 +137,6 @@ impl DepGraphThreadData { self.swap(); } } - - // Outline this too. - fn invalid_message(&self, string: &str) { - bug!("{}; see src/librustc/dep_graph/README.md for more information", string) - } } /// Definition of the depgraph thread. diff --git a/src/librustc/lib.rs b/src/librustc/lib.rs index f70349d0ee08b..d2a2f8a972d96 100644 --- a/src/librustc/lib.rs +++ b/src/librustc/lib.rs @@ -24,6 +24,7 @@ #![cfg_attr(not(stage0), deny(warnings))] #![feature(associated_consts)] +#![feature(borrow_state)] #![feature(box_patterns)] #![feature(box_syntax)] #![feature(collections)] From fe6557eb62c1f8856ed95c6998083d09e6ec470b Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 2 Sep 2016 08:26:36 -0400 Subject: [PATCH 07/12] expanding a def-id is not a read Across crates only, converting a def-id into its def-key or def-path was considered a read. This caused spurious reads when computing the symbol name for some item. --- src/librustc_metadata/csearch.rs | 12 ++++++-- .../auxiliary/a.rs | 18 ++++++++++++ .../remove-private-item-cross-crate/main.rs | 28 +++++++++++++++++++ 3 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 src/test/incremental/remove-private-item-cross-crate/auxiliary/a.rs create mode 100644 src/test/incremental/remove-private-item-cross-crate/main.rs diff --git a/src/librustc_metadata/csearch.rs b/src/librustc_metadata/csearch.rs index 0fd7b683067b7..5e5cc7e1e6108 100644 --- a/src/librustc_metadata/csearch.rs +++ b/src/librustc_metadata/csearch.rs @@ -425,13 +425,21 @@ impl<'tcx> CrateStore<'tcx> for cstore::CStore { /// parent `DefId` as well as some idea of what kind of data the /// `DefId` refers to. fn def_key(&self, def: DefId) -> hir_map::DefKey { - self.dep_graph.read(DepNode::MetaData(def)); + // Note: loading the def-key (or def-path) for a def-id is not + // a *read* of its metadata. This is because the def-id is + // really just an interned shorthand for a def-path, which is the + // canonical name for an item. + // + // self.dep_graph.read(DepNode::MetaData(def)); let cdata = self.get_crate_data(def.krate); decoder::def_key(&cdata, def.index) } fn relative_def_path(&self, def: DefId) -> hir_map::DefPath { - self.dep_graph.read(DepNode::MetaData(def)); + // See `Note` above in `def_key()` for why this read is + // commented out: + // + // self.dep_graph.read(DepNode::MetaData(def)); let cdata = self.get_crate_data(def.krate); decoder::def_path(&cdata, def.index) } diff --git a/src/test/incremental/remove-private-item-cross-crate/auxiliary/a.rs b/src/test/incremental/remove-private-item-cross-crate/auxiliary/a.rs new file mode 100644 index 0000000000000..4d84e844dedbb --- /dev/null +++ b/src/test/incremental/remove-private-item-cross-crate/auxiliary/a.rs @@ -0,0 +1,18 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![allow(warnings)] +#![crate_name = "a"] +#![crate_type = "rlib"] + +pub fn foo(b: u8) -> u32 { b as u32 } + +#[cfg(rpass1)] +fn bar() { } diff --git a/src/test/incremental/remove-private-item-cross-crate/main.rs b/src/test/incremental/remove-private-item-cross-crate/main.rs new file mode 100644 index 0000000000000..582ee905d7ca4 --- /dev/null +++ b/src/test/incremental/remove-private-item-cross-crate/main.rs @@ -0,0 +1,28 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Test that we are able to reuse `main` even though a private +// item was removed from the root module of crate`a`. + +// revisions:rpass1 rpass2 +// aux-build:a.rs + +#![feature(rustc_attrs)] +#![crate_type = "bin"] +#![rustc_partition_reused(module="main", cfg="rpass2")] + +extern crate a; + +pub fn main() { + let vec: Vec = vec![0, 1, 2, 3]; + for &b in &vec { + println!("{}", a::foo(b)); + } +} From 07df8125e6a32796a14ac253aa0df2a0847098b6 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 2 Sep 2016 11:55:32 -0400 Subject: [PATCH 08/12] kill the forbidden code supplanted by RUST_FORBID_DEP_GRAPH_EDGE --- src/librustc/dep_graph/graph.rs | 16 ---------------- src/librustc/dep_graph/raii.rs | 16 ---------------- 2 files changed, 32 deletions(-) diff --git a/src/librustc/dep_graph/graph.rs b/src/librustc/dep_graph/graph.rs index ca441c4bc3ff2..c42eeead69ec1 100644 --- a/src/librustc/dep_graph/graph.rs +++ b/src/librustc/dep_graph/graph.rs @@ -38,10 +38,6 @@ struct DepGraphData { /// Work-products that we generate in this run. work_products: RefCell, WorkProduct>>, - - /// Tracks nodes that are forbidden to read/write; see - /// `DepGraph::forbid`. Used for debugging only. - forbidden: RefCell>>, } impl DepGraph { @@ -51,7 +47,6 @@ impl DepGraph { thread: DepGraphThreadData::new(enabled), previous_work_products: RefCell::new(FnvHashMap()), work_products: RefCell::new(FnvHashMap()), - forbidden: RefCell::new(Vec::new()), }) } } @@ -75,15 +70,6 @@ impl DepGraph { raii::DepTask::new(&self.data.thread, key) } - /// Debugging aid -- forbid reads/writes to `key` while the return - /// value is in scope. Note that this is only available when debug - /// assertions are enabled -- you should not check in code that - /// invokes this function. - #[cfg(debug_assertions)] - pub fn forbid<'graph>(&'graph self, key: DepNode) -> raii::Forbid<'graph> { - raii::Forbid::new(&self.data.forbidden, key) - } - pub fn with_ignore(&self, op: OP) -> R where OP: FnOnce() -> R { @@ -99,12 +85,10 @@ impl DepGraph { } pub fn read(&self, v: DepNode) { - debug_assert!(!self.data.forbidden.borrow().contains(&v)); self.data.thread.enqueue(DepMessage::Read(v)); } pub fn write(&self, v: DepNode) { - debug_assert!(!self.data.forbidden.borrow().contains(&v)); self.data.thread.enqueue(DepMessage::Write(v)); } diff --git a/src/librustc/dep_graph/raii.rs b/src/librustc/dep_graph/raii.rs index b344eb486240b..4445a02785b56 100644 --- a/src/librustc/dep_graph/raii.rs +++ b/src/librustc/dep_graph/raii.rs @@ -49,19 +49,3 @@ impl<'graph> Drop for IgnoreTask<'graph> { } } -pub struct Forbid<'graph> { - forbidden: &'graph RefCell>> -} - -impl<'graph> Forbid<'graph> { - pub fn new(forbidden: &'graph RefCell>>, node: DepNode) -> Self { - forbidden.borrow_mut().push(node); - Forbid { forbidden: forbidden } - } -} - -impl<'graph> Drop for Forbid<'graph> { - fn drop(&mut self) { - self.forbidden.borrow_mut().pop(); - } -} From 2446e258fcc0f5554f91e54d2f849da4dd69a026 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 2 Sep 2016 14:26:16 -0400 Subject: [PATCH 09/12] kill extra `use` --- src/librustc/dep_graph/raii.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/librustc/dep_graph/raii.rs b/src/librustc/dep_graph/raii.rs index 4445a02785b56..e4f572902f9e5 100644 --- a/src/librustc/dep_graph/raii.rs +++ b/src/librustc/dep_graph/raii.rs @@ -9,7 +9,6 @@ // except according to those terms. use hir::def_id::DefId; -use std::cell::RefCell; use super::DepNode; use super::thread::{DepGraphThreadData, DepMessage}; From dadce2521e332a6d8f8704ce440637b8a4df7c66 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 2 Sep 2016 18:38:54 -0400 Subject: [PATCH 10/12] always print def-path in Debug impl for DefId I also added an `opt_def_path` so that we can deal with DefIds that are missing a `DefPath` entry. --- src/librustc/hir/def_id.rs | 19 ++++++---------- src/librustc/middle/cstore.rs | 4 ++-- src/librustc/ty/mod.rs | 37 ++++++++++++++++++++++++++++---- src/librustc_metadata/csearch.rs | 2 +- src/librustc_metadata/decoder.rs | 13 ++++++++--- 5 files changed, 53 insertions(+), 22 deletions(-) diff --git a/src/librustc/hir/def_id.rs b/src/librustc/hir/def_id.rs index a3b83ec5be4bd..16afa705e3919 100644 --- a/src/librustc/hir/def_id.rs +++ b/src/librustc/hir/def_id.rs @@ -58,19 +58,14 @@ impl fmt::Debug for DefId { write!(f, "DefId {{ krate: {:?}, node: {:?}", self.krate, self.index)?; - // Unfortunately, there seems to be no way to attempt to print - // a path for a def-id, so I'll just make a best effort for now - // and otherwise fallback to just printing the crate/node pair - if self.is_local() { // (1) - // (1) side-step fact that not all external things have paths at - // the moment, such as type parameters - ty::tls::with_opt(|opt_tcx| { - if let Some(tcx) = opt_tcx { - write!(f, " => {}", tcx.item_path_str(*self))?; + ty::tls::with_opt(|opt_tcx| { + if let Some(tcx) = opt_tcx { + if let Some(def_path) = tcx.opt_def_path(*self) { + write!(f, " => {}", def_path.to_string(tcx))?; } - Ok(()) - })?; - } + } + Ok(()) + })?; write!(f, " }}") } diff --git a/src/librustc/middle/cstore.rs b/src/librustc/middle/cstore.rs index b33bc520fe216..52645883a8be9 100644 --- a/src/librustc/middle/cstore.rs +++ b/src/librustc/middle/cstore.rs @@ -233,7 +233,7 @@ pub trait CrateStore<'tcx> { def: DefKey) -> Option; fn def_key(&self, def: DefId) -> hir_map::DefKey; - fn relative_def_path(&self, def: DefId) -> hir_map::DefPath; + fn relative_def_path(&self, def: DefId) -> Option; fn variant_kind(&self, def_id: DefId) -> Option; fn struct_ctor_def_id(&self, struct_def_id: DefId) -> Option; fn tuple_struct_definition_if_ctor(&self, did: DefId) -> Option; @@ -430,7 +430,7 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore { // resolve fn def_key(&self, def: DefId) -> hir_map::DefKey { bug!("def_key") } - fn relative_def_path(&self, def: DefId) -> hir_map::DefPath { bug!("relative_def_path") } + fn relative_def_path(&self, def: DefId) -> Option { bug!("relative_def_path") } fn variant_kind(&self, def_id: DefId) -> Option { bug!("variant_kind") } fn struct_ctor_def_id(&self, struct_def_id: DefId) -> Option { bug!("struct_ctor_def_id") } diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index 78358ce534d99..53838b0760a79 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -2437,12 +2437,41 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { } } - /// Returns the `DefPath` of an item. Note that if `id` is not - /// local to this crate -- or is inlined into this crate -- the - /// result will be a non-local `DefPath`. + /// Convert a `DefId` into its fully expanded `DefPath` (every + /// `DefId` is really just an interned def-path). + /// + /// Note that if `id` is not local to this crate -- or is + /// inlined into this crate -- the result will be a non-local + /// `DefPath`. + /// + /// This function is only safe to use when you are sure that the + /// full def-path is accessible. Examples that are known to be + /// safe are local def-ids or items; see `opt_def_path` for more + /// details. pub fn def_path(self, id: DefId) -> ast_map::DefPath { + self.opt_def_path(id).unwrap_or_else(|| { + bug!("could not load def-path for {:?}", id) + }) + } + + /// Convert a `DefId` into its fully expanded `DefPath` (every + /// `DefId` is really just an interned def-path). + /// + /// When going across crates, we do not save the full info for + /// every cross-crate def-id, and hence we may not always be able + /// to create a def-path. Therefore, this returns + /// `Option` to cover that possibility. It will always + /// return `Some` for local def-ids, however, as well as for + /// items. The problems arise with "minor" def-ids like those + /// associated with a pattern, `impl Trait`, or other internal + /// detail to a fn. + /// + /// Note that if `id` is not local to this crate -- or is + /// inlined into this crate -- the result will be a non-local + /// `DefPath`. + pub fn opt_def_path(self, id: DefId) -> Option { if id.is_local() { - self.map.def_path(id) + Some(self.map.def_path(id)) } else { self.sess.cstore.relative_def_path(id) } diff --git a/src/librustc_metadata/csearch.rs b/src/librustc_metadata/csearch.rs index 5e5cc7e1e6108..d7ca93235fddb 100644 --- a/src/librustc_metadata/csearch.rs +++ b/src/librustc_metadata/csearch.rs @@ -435,7 +435,7 @@ impl<'tcx> CrateStore<'tcx> for cstore::CStore { decoder::def_key(&cdata, def.index) } - fn relative_def_path(&self, def: DefId) -> hir_map::DefPath { + fn relative_def_path(&self, def: DefId) -> Option { // See `Note` above in `def_key()` for why this read is // commented out: // diff --git a/src/librustc_metadata/decoder.rs b/src/librustc_metadata/decoder.rs index d75d5a3b35443..5bad89f1a593f 100644 --- a/src/librustc_metadata/decoder.rs +++ b/src/librustc_metadata/decoder.rs @@ -759,7 +759,7 @@ pub fn maybe_get_item_ast<'a, 'tcx>(cdata: Cmd, tcx: TyCtxt<'a, 'tcx, 'tcx>, id: krate: cdata.cnum, index: def_key(cdata, id).parent.unwrap() }; - let mut parent_def_path = def_path(cdata, id); + let mut parent_def_path = def_path(cdata, id).unwrap(); parent_def_path.data.pop(); if let Some(ast_doc) = reader::maybe_get_doc(item_doc, tag_ast as usize) { let ii = decode_inlined_item(cdata, @@ -1626,9 +1626,16 @@ fn item_def_key(item_doc: rbml::Doc) -> hir_map::DefKey { } } -pub fn def_path(cdata: Cmd, id: DefIndex) -> hir_map::DefPath { +// Returns the path leading to the thing with this `id`. Note that +// some def-ids don't wind up in the metadata, so `def_path` sometimes +// returns `None` +pub fn def_path(cdata: Cmd, id: DefIndex) -> Option { debug!("def_path(id={:?})", id); - hir_map::DefPath::make(cdata.cnum, id, |parent| def_key(cdata, parent)) + if cdata.get_item(id).is_some() { + Some(hir_map::DefPath::make(cdata.cnum, id, |parent| def_key(cdata, parent))) + } else { + None + } } pub fn get_panic_strategy(data: &[u8]) -> PanicStrategy { From c2ffa2f938ec3171180b8e0d4dc20bf92b3a8cfd Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sun, 4 Sep 2016 06:48:17 -0400 Subject: [PATCH 11/12] pacify the mercilous tidy add licenses to shadow.rs --- src/librustc/dep_graph/shadow.rs | 10 ++++++++++ src/librustc/middle/cstore.rs | 4 +++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/librustc/dep_graph/shadow.rs b/src/librustc/dep_graph/shadow.rs index e038997e0ad42..f9f75d006775f 100644 --- a/src/librustc/dep_graph/shadow.rs +++ b/src/librustc/dep_graph/shadow.rs @@ -1,3 +1,13 @@ +// Copyright 2012-2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + //! The "Shadow Graph" is maintained on the main thread and which //! tracks each message relating to the dep-graph and applies some //! sanity checks as they go by. If an error results, it means you get diff --git a/src/librustc/middle/cstore.rs b/src/librustc/middle/cstore.rs index 52645883a8be9..d1722ac6f2f7f 100644 --- a/src/librustc/middle/cstore.rs +++ b/src/librustc/middle/cstore.rs @@ -430,7 +430,9 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore { // resolve fn def_key(&self, def: DefId) -> hir_map::DefKey { bug!("def_key") } - fn relative_def_path(&self, def: DefId) -> Option { bug!("relative_def_path") } + fn relative_def_path(&self, def: DefId) -> Option { + bug!("relative_def_path") + } fn variant_kind(&self, def_id: DefId) -> Option { bug!("variant_kind") } fn struct_ctor_def_id(&self, struct_def_id: DefId) -> Option { bug!("struct_ctor_def_id") } From 9ca578687b88d2c7817d5709b2700fb6777348f2 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 12 Sep 2016 17:43:44 -0400 Subject: [PATCH 12/12] check stack discipline of tasks --- src/librustc/dep_graph/shadow.rs | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/librustc/dep_graph/shadow.rs b/src/librustc/dep_graph/shadow.rs index f9f75d006775f..72a321425ef06 100644 --- a/src/librustc/dep_graph/shadow.rs +++ b/src/librustc/dep_graph/shadow.rs @@ -17,8 +17,10 @@ //! runtime impact. Therefore, it is largely compiled out if //! debug-assertions are not enabled. //! -//! The basic sanity check, always enabled, is that there is always a -//! task (or ignore) on the stack when you do read/write. +//! The basic sanity check, enabled if you have debug assertions +//! enabled, is that there is always a task (or ignore) on the stack +//! when you do read/write, and that the tasks are pushed/popped +//! according to a proper stack discipline. //! //! Optionally, if you specify RUST_FORBID_DEP_GRAPH_EDGE, you can //! specify an edge filter to be applied to each edge as it is @@ -81,13 +83,23 @@ impl ShadowGraph { DepMessage::Write(ref n) => self.check_edge(top(&stack), Some(Some(n))), DepMessage::PushTask(ref n) => stack.push(Some(n.clone())), DepMessage::PushIgnore => stack.push(None), - DepMessage::PopTask(_) | + DepMessage::PopTask(ref n) => { + match stack.pop() { + Some(Some(m)) => { + if *n != m { + bug!("stack mismatch: found {:?} expected {:?}", m, n) + } + } + Some(None) => bug!("stack mismatch: found Ignore expected {:?}", n), + None => bug!("stack mismatch: found empty stack, expected {:?}", n), + } + } DepMessage::PopIgnore => { - // we could easily check that the stack is - // well-formed here, but since we use closures and - // RAII accessors, this bug basically never - // happens, so it seems not worth the overhead - stack.pop(); + match stack.pop() { + Some(Some(m)) => bug!("stack mismatch: found {:?} expected ignore", m), + Some(None) => (), + None => bug!("stack mismatch: found empty stack, expected ignore"), + } } DepMessage::Query => (), }