fix a few errant `Krate` edges by nikomatsakis · Pull Request #35960 · rust-lang/rust

nikomatsakis · 2016-08-24T12:15:45Z

Exploring the effect of small changes on syntex reuse, I discovered the following sources of unnecessary edges from Krate

r? @michaelwoerister

nagisa · 2016-08-24T14:51:33Z

Should this field also be #[cfg(debug_assertions)]-ed?

michaelwoerister · 2016-08-24T21:30:30Z

Good catches both!
r=me with the nits and addressed.

arielb1 · 2016-08-25T19:39:35Z

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)

I remember a similar place in coherence where I intentionally held the lock because someone (i.e., metadata loading) was inserting stuff to the list behind my back and I wanted to prevent that problem from recurring.

If nobody is supposed to mutate a list behind your back, holding a lock is the "safe" option - it can lead to more ICEs, but it can make your assumptions more visible.

bors · 2016-08-27T13:05:08Z

☔ The latest upstream changes (presumably #36030) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2016-08-31T19:28:09Z

@bors r=mw

bors · 2016-08-31T19:28:10Z

📌 Commit 30c6f12 has been approved by mw

nikomatsakis · 2016-09-01T01:41:17Z

@bors r-

nikomatsakis · 2016-09-01T01:41:24Z

Warnings from travis look legit, even though I didn't see anything like that locally. Maybe I didn't run full tests on the latest commit or something.

nikomatsakis · 2016-09-01T21:56:25Z

@bors r=mw

bors · 2016-09-01T21:56:26Z

📌 Commit d287b7a has been approved by mw

nikomatsakis · 2016-09-02T08:45:02Z

@bors r- // again the travis issue looks legit, and again I wonder why I am not seeing this locally!

nikomatsakis · 2016-09-02T09:02:08Z

@nagisa @michaelwoerister

Should this field also be #[cfg(debug_assertions)]-ed?

The answer is sort of yes, but I wound up (see most recent commit) taking a different approach. In particular, the number of things that must have #[cfg] applied to them seemed to be growing at a kind of ridiculous rate, and moreover making a field #[cfg] is kind of annoying since you can't apply #[cfg] to the field in the struct literals. I really find code with #[cfg] sprinkled all about in random places quite hard to read -- I like it to be contained at one or two "pinch points" or not used at all, so in the most recent commit I removed the #[cfg(debug_assertions)] on the forbid method and replaced it with an assert!. But I'm not totally happy with that.

I'm not sure what's a better way to formulate the code here. It occurs to me now I could maybe make a mod forbid that is different if debug-assertions are disabled, so that at least all the cond code can be contained in one place, but it still feels like a lot for such a small thing.

nikomatsakis · 2016-09-02T09:04:09Z

again the travis issue looks legit, and again I wonder why I am not seeing this locally!

Oh, I am dumb, it's because I'm building with debug-assertions enabled, of course!

nikomatsakis · 2016-09-02T11:49:51Z

OK, so, I wound up implementing a better mechanism altogether. I'll push a few more commits shortly.

nikomatsakis · 2016-09-02T15:56:46Z

r? @michaelwoerister -- this fixes one add'l bug, and revamps the forbidden stuff into something more powerful.

bors · 2016-09-05T00:19:02Z

☔ The latest upstream changes (presumably #36203) made this pull request unmergeable. Please resolve the merge conflicts.

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?).

We touch the krate to adjust the maps, but we don't expose that data widely.

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.

It was duplicating information available elsewhere.

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.

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.

supplanted by RUST_FORBID_DEP_GRAPH_EDGE

I also added an `opt_def_path` so that we can deal with DefIds that are missing a `DefPath` entry.

add licenses to shadow.rs

michaelwoerister · 2016-09-06T15:39:35Z

+
+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!


michaelwoerister · 2016-09-12T22:26:42Z

@bors r+

bors · 2016-09-12T22:26:44Z

📌 Commit 9ca5786 has been approved by michaelwoerister

bors · 2016-09-13T00:15:26Z

⌛ Testing commit 9ca5786 with merge fa9d8cc...

@michaelwoerister

…oerister 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

bors · 2016-09-13T03:33:45Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: michaelwoerister
Pushing fa9d8cc to master...

rust-highfive assigned michaelwoerister Aug 24, 2016

nagisa reviewed Aug 24, 2016
View reviewed changes

Comment threadsrc/librustc/dep_graph/graph.rs Outdated
nagisaAug 24, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this field also be #[cfg(debug_assertions)]-ed?

nikomatsakis force-pushed the incr-comp-krate-edges branch from e08d4fb to 30c6f12Compare August 31, 2016 19:19

nikomatsakis force-pushed the incr-comp-krate-edges branch from 30c6f12 to 386abe1Compare September 1, 2016 10:08

nikomatsakis force-pushed the incr-comp-krate-edges branch from d287b7a to 3b4e587Compare September 2, 2016 08:55

nikomatsakis force-pushed the incr-comp-krate-edges branch 2 times, most recently from 54f8bdf to cf7fd6bCompare September 2, 2016 15:55

nikomatsakis force-pushed the incr-comp-krate-edges branch from 2d893b3 to bf27870Compare September 4, 2016 11:55

nikomatsakis added 4 commits September 6, 2016 11:17

ignore dep-graph when loading inlined HIR
c6363b8
We touch the krate to adjust the maps, but we don't expose that data widely.

add a debugging mechanism to forbid edges
753590f
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.

allow testing DepNode::Krate edges directly
2a84449

nikomatsakis added 7 commits September 6, 2016 11:18

remove the "misc-items" from meta-data
4c2f3ff
It was duplicating information available elsewhere.

implement a debugging "shadow graph"
2f91ba0
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.

expanding a def-id is not a read
fe6557e
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.

kill the forbidden code
07df812
supplanted by RUST_FORBID_DEP_GRAPH_EDGE

kill extra use
2446e25

always print def-path in Debug impl for DefId
dadce25
I also added an `opt_def_path` so that we can deal with DefIds that are missing a `DefPath` entry.

pacify the mercilous tidy
c2ffa2f
add licenses to shadow.rs

nikomatsakis force-pushed the incr-comp-krate-edges branch from bf27870 to c2ffa2fCompare September 6, 2016 15:28

michaelwoerister reviewed Sep 6, 2016
View reviewed changes

check stack discipline of tasks
9ca5786

bors merged commit 9ca5786 into rust-lang:masterSep 13, 2016

This was referenced Sep 13, 2016
ICH: Add ability to test the ICH of exported metadata items. #36370
Merged
Remove some obsolete code from the compiler #36393
Merged

nikomatsakis deleted the incr-comp-krate-edges branch October 3, 2016 14:55

Uh oh!

Uh oh!

Conversation

nikomatsakis commented Aug 24, 2016

Uh oh!

nagisaAug 24, 2016

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Aug 24, 2016

Uh oh!

arielb1 commented Aug 25, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Aug 27, 2016

Uh oh!

nikomatsakis commented Aug 31, 2016

Uh oh!

bors commented Aug 31, 2016

Uh oh!

nikomatsakis commented Sep 1, 2016

Uh oh!

nikomatsakis commented Sep 1, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Sep 1, 2016

Uh oh!

bors commented Sep 1, 2016

Uh oh!

nikomatsakis commented Sep 2, 2016

Uh oh!

nikomatsakis commented Sep 2, 2016

Uh oh!

nikomatsakis commented Sep 2, 2016

Uh oh!

nikomatsakis commented Sep 2, 2016

Uh oh!

nikomatsakis commented Sep 2, 2016

Uh oh!

bors commented Sep 5, 2016

Uh oh!

michaelwoeristerSep 6, 2016

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Sep 12, 2016

Uh oh!

bors commented Sep 12, 2016

Uh oh!

bors commented Sep 13, 2016

Uh oh!

bors commented Sep 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arielb1 commented Aug 25, 2016•
edited
Loading

nikomatsakis commented Sep 1, 2016•
edited
Loading