Skip to content

Commit fa9d8cc

Browse files
authored
Auto merge of #35960 - nikomatsakis:incr-comp-krate-edges, r=michaelwoerister
fix a few errant `Krate` edges Exploring the effect of small changes on `syntex` reuse, I discovered the following sources of unnecessary edges from `Krate` r? @michaelwoerister
2 parents 5531c31 + 9ca5786 commit fa9d8cc

File tree

24 files changed

+473
-205
lines changed

24 files changed

+473
-205
lines changed

src/librustc/dep_graph/README.md

+31
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,8 @@ path is found (as demonstrated above).
341341

342342
### Debugging the dependency graph
343343

344+
#### Dumping the graph
345+
344346
The compiler is also capable of dumping the dependency graph for your
345347
debugging pleasure. To do so, pass the `-Z dump-dep-graph` flag. The
346348
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
392394
`TypeckItemBody(bar)`, from which you can (hopefully) see the source
393395
of the erroneous edge.
394396

397+
#### Tracking down incorrect edges
398+
399+
Sometimes, after you dump the dependency graph, you will find some
400+
path that should not exist, but you will not be quite sure how it came
401+
to be. **When the compiler is built with debug assertions,** it can
402+
help you track that down. Simply set the `RUST_FORBID_DEP_GRAPH_EDGE`
403+
environment variable to a filter. Every edge created in the dep-graph
404+
will be tested against that filter -- if it matches, a `bug!` is
405+
reported, so you can easily see the backtrace (`RUST_BACKTRACE=1`).
406+
407+
The syntax for these filters is the same as described in the previous
408+
section. However, note that this filter is applied to every **edge**
409+
and doesn't handle longer paths in the graph, unlike the previous
410+
section.
411+
412+
Example:
413+
414+
You find that there is a path from the `Hir` of `foo` to the type
415+
check of `bar` and you don't think there should be. You dump the
416+
dep-graph as described in the previous section and open `dep-graph.txt`
417+
to see something like:
418+
419+
Hir(foo) -> Collect(bar)
420+
Collect(bar) -> TypeckItemBody(bar)
421+
422+
That first edge looks suspicious to you. So you set
423+
`RUST_FORBID_DEP_GRAPH_EDGE` to `Hir&foo -> Collect&bar`, re-run, and
424+
then observe the backtrace. Voila, bug fixed!
425+
395426
### Inlining of HIR nodes
396427

397428
For the time being, at least, we still sometimes "inline" HIR nodes

src/librustc/dep_graph/debug.rs

+7
Original file line numberDiff line numberDiff line change
@@ -66,4 +66,11 @@ impl EdgeFilter {
6666
})
6767
}
6868
}
69+
70+
pub fn test<D: Clone + Debug>(&self,
71+
source: &DepNode<D>,
72+
target: &DepNode<D>)
73+
-> bool {
74+
self.source.test(source) && self.target.test(target)
75+
}
6976
}

src/librustc/dep_graph/dep_tracking_map.rs

+11
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,17 @@ impl<M: DepTrackingMapConfig> DepTrackingMap<M> {
8080
pub fn keys(&self) -> Vec<M::Key> {
8181
self.map.keys().cloned().collect()
8282
}
83+
84+
/// Append `elem` to the vector stored for `k`, creating a new vector if needed.
85+
/// This is considered a write to `k`.
86+
pub fn push<E: Clone>(&mut self, k: M::Key, elem: E)
87+
where M: DepTrackingMapConfig<Value=Vec<E>>
88+
{
89+
self.write(&k);
90+
self.map.entry(k)
91+
.or_insert(Vec::new())
92+
.push(elem);
93+
}
8394
}
8495

8596
impl<M: DepTrackingMapConfig> MemoizationMap for RefCell<DepTrackingMap<M>> {

src/librustc/dep_graph/graph.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ impl DepGraph {
4646
data: Rc::new(DepGraphData {
4747
thread: DepGraphThreadData::new(enabled),
4848
previous_work_products: RefCell::new(FnvHashMap()),
49-
work_products: RefCell::new(FnvHashMap())
49+
work_products: RefCell::new(FnvHashMap()),
5050
})
5151
}
5252
}

src/librustc/dep_graph/mod.rs

+1
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ mod edges;
1515
mod graph;
1616
mod query;
1717
mod raii;
18+
mod shadow;
1819
mod thread;
1920
mod visit;
2021

src/librustc/dep_graph/raii.rs

+1
Original file line numberDiff line numberDiff line change
@@ -47,3 +47,4 @@ impl<'graph> Drop for IgnoreTask<'graph> {
4747
self.data.enqueue(DepMessage::PopIgnore);
4848
}
4949
}
50+

src/librustc/dep_graph/shadow.rs

+145
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
// Copyright 2012-2015 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
//! The "Shadow Graph" is maintained on the main thread and which
12+
//! tracks each message relating to the dep-graph and applies some
13+
//! sanity checks as they go by. If an error results, it means you get
14+
//! a nice stack-trace telling you precisely what caused the error.
15+
//!
16+
//! NOTE: This is a debugging facility which can potentially have non-trivial
17+
//! runtime impact. Therefore, it is largely compiled out if
18+
//! debug-assertions are not enabled.
19+
//!
20+
//! The basic sanity check, enabled if you have debug assertions
21+
//! enabled, is that there is always a task (or ignore) on the stack
22+
//! when you do read/write, and that the tasks are pushed/popped
23+
//! according to a proper stack discipline.
24+
//!
25+
//! Optionally, if you specify RUST_FORBID_DEP_GRAPH_EDGE, you can
26+
//! specify an edge filter to be applied to each edge as it is
27+
//! created. See `./README.md` for details.
28+
29+
use hir::def_id::DefId;
30+
use std::cell::{BorrowState, RefCell};
31+
use std::env;
32+
33+
use super::DepNode;
34+
use super::thread::DepMessage;
35+
use super::debug::EdgeFilter;
36+
37+
pub struct ShadowGraph {
38+
// if you push None onto the stack, that corresponds to an Ignore
39+
stack: RefCell<Vec<Option<DepNode<DefId>>>>,
40+
forbidden_edge: Option<EdgeFilter>,
41+
}
42+
43+
const ENABLED: bool = cfg!(debug_assertions);
44+
45+
impl ShadowGraph {
46+
pub fn new() -> Self {
47+
let forbidden_edge = if !ENABLED {
48+
None
49+
} else {
50+
match env::var("RUST_FORBID_DEP_GRAPH_EDGE") {
51+
Ok(s) => {
52+
match EdgeFilter::new(&s) {
53+
Ok(f) => Some(f),
54+
Err(err) => bug!("RUST_FORBID_DEP_GRAPH_EDGE invalid: {}", err),
55+
}
56+
}
57+
Err(_) => None,
58+
}
59+
};
60+
61+
ShadowGraph {
62+
stack: RefCell::new(vec![]),
63+
forbidden_edge: forbidden_edge,
64+
}
65+
}
66+
67+
pub fn enqueue(&self, message: &DepMessage) {
68+
if ENABLED {
69+
match self.stack.borrow_state() {
70+
BorrowState::Unused => {}
71+
_ => {
72+
// When we apply edge filters, that invokes the
73+
// Debug trait on DefIds, which in turn reads from
74+
// various bits of state and creates reads! Ignore
75+
// those recursive reads.
76+
return;
77+
}
78+
}
79+
80+
let mut stack = self.stack.borrow_mut();
81+
match *message {
82+
DepMessage::Read(ref n) => self.check_edge(Some(Some(n)), top(&stack)),
83+
DepMessage::Write(ref n) => self.check_edge(top(&stack), Some(Some(n))),
84+
DepMessage::PushTask(ref n) => stack.push(Some(n.clone())),
85+
DepMessage::PushIgnore => stack.push(None),
86+
DepMessage::PopTask(ref n) => {
87+
match stack.pop() {
88+
Some(Some(m)) => {
89+
if *n != m {
90+
bug!("stack mismatch: found {:?} expected {:?}", m, n)
91+
}
92+
}
93+
Some(None) => bug!("stack mismatch: found Ignore expected {:?}", n),
94+
None => bug!("stack mismatch: found empty stack, expected {:?}", n),
95+
}
96+
}
97+
DepMessage::PopIgnore => {
98+
match stack.pop() {
99+
Some(Some(m)) => bug!("stack mismatch: found {:?} expected ignore", m),
100+
Some(None) => (),
101+
None => bug!("stack mismatch: found empty stack, expected ignore"),
102+
}
103+
}
104+
DepMessage::Query => (),
105+
}
106+
}
107+
}
108+
109+
fn check_edge(&self,
110+
source: Option<Option<&DepNode<DefId>>>,
111+
target: Option<Option<&DepNode<DefId>>>) {
112+
assert!(ENABLED);
113+
match (source, target) {
114+
// cannot happen, one side is always Some(Some(_))
115+
(None, None) => unreachable!(),
116+
117+
// nothing on top of the stack
118+
(None, Some(n)) | (Some(n), None) => bug!("read/write of {:?} but no current task", n),
119+
120+
// this corresponds to an Ignore being top of the stack
121+
(Some(None), _) | (_, Some(None)) => (),
122+
123+
// a task is on top of the stack
124+
(Some(Some(source)), Some(Some(target))) => {
125+
if let Some(ref forbidden_edge) = self.forbidden_edge {
126+
if forbidden_edge.test(source, target) {
127+
bug!("forbidden edge {:?} -> {:?} created", source, target)
128+
}
129+
}
130+
}
131+
}
132+
}
133+
}
134+
135+
// Do a little juggling: we get back a reference to an option at the
136+
// top of the stack, convert it to an optional reference.
137+
fn top<'s>(stack: &'s Vec<Option<DepNode<DefId>>>) -> Option<Option<&'s DepNode<DefId>>> {
138+
stack.last()
139+
.map(|n: &'s Option<DepNode<DefId>>| -> Option<&'s DepNode<DefId>> {
140+
// (*)
141+
// (*) type annotation just there to clarify what would
142+
// otherwise be some *really* obscure code
143+
n.as_ref()
144+
})
145+
}

src/librustc/dep_graph/thread.rs

+13-28
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,13 @@
2020
2121
use hir::def_id::DefId;
2222
use rustc_data_structures::veccell::VecCell;
23-
use std::cell::Cell;
2423
use std::sync::mpsc::{self, Sender, Receiver};
2524
use std::thread;
2625

2726
use super::DepGraphQuery;
2827
use super::DepNode;
2928
use super::edges::DepGraphEdges;
29+
use super::shadow::ShadowGraph;
3030

3131
#[derive(Debug)]
3232
pub enum DepMessage {
@@ -42,12 +42,16 @@ pub enum DepMessage {
4242
pub struct DepGraphThreadData {
4343
enabled: bool,
4444

45-
// Local counter that just tracks how many tasks are pushed onto the
46-
// stack, so that we still get an error in the case where one is
47-
// missing. If dep-graph construction is enabled, we'd get the same
48-
// error when processing tasks later on, but that's annoying because
49-
// it lacks precision about the source of the error.
50-
tasks_pushed: Cell<usize>,
45+
// The "shadow graph" is a debugging aid. We give it each message
46+
// in real time as it arrives and it checks for various errors
47+
// (for example, a read/write when there is no current task; it
48+
// can also apply user-defined filters; see `shadow` module for
49+
// details). This only occurs if debug-assertions are enabled.
50+
//
51+
// Note that in some cases the same errors will occur when the
52+
// data is processed off the main thread, but that's annoying
53+
// because it lacks precision about the source of the error.
54+
shadow_graph: ShadowGraph,
5155

5256
// current buffer, where we accumulate messages
5357
messages: VecCell<DepMessage>,
@@ -76,7 +80,7 @@ impl DepGraphThreadData {
7680

7781
DepGraphThreadData {
7882
enabled: enabled,
79-
tasks_pushed: Cell::new(0),
83+
shadow_graph: ShadowGraph::new(),
8084
messages: VecCell::with_capacity(INITIAL_CAPACITY),
8185
swap_in: rx2,
8286
swap_out: tx1,
@@ -118,21 +122,7 @@ impl DepGraphThreadData {
118122
/// the buffer is full, this may swap.)
119123
#[inline]
120124
pub fn enqueue(&self, message: DepMessage) {
121-
// Regardless of whether dep graph construction is enabled, we
122-
// still want to check that we always have a valid task on the
123-
// stack when a read/write/etc event occurs.
124-
match message {
125-
DepMessage::Read(_) | DepMessage::Write(_) =>
126-
if self.tasks_pushed.get() == 0 {
127-
self.invalid_message("read/write but no current task")
128-
},
129-
DepMessage::PushTask(_) | DepMessage::PushIgnore =>
130-
self.tasks_pushed.set(self.tasks_pushed.get() + 1),
131-
DepMessage::PopTask(_) | DepMessage::PopIgnore =>
132-
self.tasks_pushed.set(self.tasks_pushed.get() - 1),
133-
DepMessage::Query =>
134-
(),
135-
}
125+
self.shadow_graph.enqueue(&message);
136126

137127
if self.enabled {
138128
self.enqueue_enabled(message);
@@ -147,11 +137,6 @@ impl DepGraphThreadData {
147137
self.swap();
148138
}
149139
}
150-
151-
// Outline this too.
152-
fn invalid_message(&self, string: &str) {
153-
bug!("{}; see src/librustc/dep_graph/README.md for more information", string)
154-
}
155140
}
156141

157142
/// Definition of the depgraph thread.

src/librustc/hir/def_id.rs

+7-12
Original file line numberDiff line numberDiff line change
@@ -58,19 +58,14 @@ impl fmt::Debug for DefId {
5858
write!(f, "DefId {{ krate: {:?}, node: {:?}",
5959
self.krate, self.index)?;
6060

61-
// Unfortunately, there seems to be no way to attempt to print
62-
// a path for a def-id, so I'll just make a best effort for now
63-
// and otherwise fallback to just printing the crate/node pair
64-
if self.is_local() { // (1)
65-
// (1) side-step fact that not all external things have paths at
66-
// the moment, such as type parameters
67-
ty::tls::with_opt(|opt_tcx| {
68-
if let Some(tcx) = opt_tcx {
69-
write!(f, " => {}", tcx.item_path_str(*self))?;
61+
ty::tls::with_opt(|opt_tcx| {
62+
if let Some(tcx) = opt_tcx {
63+
if let Some(def_path) = tcx.opt_def_path(*self) {
64+
write!(f, " => {}", def_path.to_string(tcx))?;
7065
}
71-
Ok(())
72-
})?;
73-
}
66+
}
67+
Ok(())
68+
})?;
7469

7570
write!(f, " }}")
7671
}

src/librustc/hir/map/mod.rs

+2
Original file line numberDiff line numberDiff line change
@@ -927,6 +927,8 @@ pub fn map_decoded_item<'ast, F: FoldOps>(map: &Map<'ast>,
927927
ii: InlinedItem,
928928
fold_ops: F)
929929
-> &'ast InlinedItem {
930+
let _ignore = map.forest.dep_graph.in_ignore();
931+
930932
let mut fld = IdAndSpanUpdater::new(fold_ops);
931933
let ii = match ii {
932934
II::Item(d, i) => II::Item(fld.fold_ops.new_def_id(d),

src/librustc/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#![cfg_attr(not(stage0), deny(warnings))]
2525

2626
#![feature(associated_consts)]
27+
#![feature(borrow_state)]
2728
#![feature(box_patterns)]
2829
#![feature(box_syntax)]
2930
#![feature(collections)]

src/librustc/middle/cstore.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ pub trait CrateStore<'tcx> {
233233
def: DefKey)
234234
-> Option<DefIndex>;
235235
fn def_key(&self, def: DefId) -> hir_map::DefKey;
236-
fn relative_def_path(&self, def: DefId) -> hir_map::DefPath;
236+
fn relative_def_path(&self, def: DefId) -> Option<hir_map::DefPath>;
237237
fn variant_kind(&self, def_id: DefId) -> Option<VariantKind>;
238238
fn struct_ctor_def_id(&self, struct_def_id: DefId) -> Option<DefId>;
239239
fn tuple_struct_definition_if_ctor(&self, did: DefId) -> Option<DefId>;
@@ -430,7 +430,9 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore {
430430

431431
// resolve
432432
fn def_key(&self, def: DefId) -> hir_map::DefKey { bug!("def_key") }
433-
fn relative_def_path(&self, def: DefId) -> hir_map::DefPath { bug!("relative_def_path") }
433+
fn relative_def_path(&self, def: DefId) -> Option<hir_map::DefPath> {
434+
bug!("relative_def_path")
435+
}
434436
fn variant_kind(&self, def_id: DefId) -> Option<VariantKind> { bug!("variant_kind") }
435437
fn struct_ctor_def_id(&self, struct_def_id: DefId) -> Option<DefId>
436438
{ bug!("struct_ctor_def_id") }

src/librustc/ty/maps.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ dep_map_ty! { ImplTraitRefs: ItemSignature(DefId) -> Option<ty::TraitRef<'tcx>>
3939
dep_map_ty! { TraitDefs: ItemSignature(DefId) -> &'tcx ty::TraitDef<'tcx> }
4040
dep_map_ty! { AdtDefs: ItemSignature(DefId) -> ty::AdtDefMaster<'tcx> }
4141
dep_map_ty! { ItemVariances: ItemSignature(DefId) -> Rc<Vec<ty::Variance>> }
42-
dep_map_ty! { InherentImpls: InherentImpls(DefId) -> Rc<Vec<DefId>> }
42+
dep_map_ty! { InherentImpls: InherentImpls(DefId) -> Vec<DefId> }
4343
dep_map_ty! { ImplItems: ImplItems(DefId) -> Vec<ty::ImplOrTraitItemId> }
4444
dep_map_ty! { TraitItems: TraitItems(DefId) -> Rc<Vec<ty::ImplOrTraitItem<'tcx>>> }
4545
dep_map_ty! { ReprHints: ReprHints(DefId) -> Rc<Vec<attr::ReprAttr>> }

0 commit comments

Comments
 (0)