Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

incr.comp.: Assert that no DepNode is re-opened (see issue #42298). #43590

Merged
merged 2 commits into from
Aug 4, 2017
Merged

incr.comp.: Assert that no DepNode is re-opened (see issue #42298). #43590

merged 2 commits into from
Aug 4, 2017

Conversation

michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented Aug 1, 2017 •
edited
Loading

This PR removes the last occurrence of DepNode re-opening and adds an assertion that prevents our doing so in the future too. The DepGraph should now be guaranteed to be cycle free.

r? @nikomatsakis

EDIT: Closes #42298

nikomatsakis
nikomatsakis reviewed Aug 2, 2017
View reviewed changes
src/librustc/dep_graph/edges.rs
// A set to help assert that no two tasks use the same DepNode. This is a
// temporary measure. Once we load the previous dep-graph as readonly, this
// check will fall out of the graph implementation naturally.
opened_once: FxHashSet<DepNode>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we want to make #[cfg(debug_assertions)]?

@nikomatsakis
Copy link
Contributor

r=me -- but decide if you think it should be #[cfg] or not

@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

Since this should only ever be hit in incremental mode and that is almost exclusively used with debug-assertions-enabled nightlies, I think it's OK to leave it the way it is.

@bors
Copy link
Contributor

bors commented Aug 3, 2017

📌 Commit 6bb0693 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 3, 2017

⌛ Testing commit 6bb0693 with merge 189ca70d2b10391ebdcea00135b4cb76579bb3a0...

@bors
Copy link
Contributor

bors commented Aug 3, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Aug 3, 2017

gnu-incremental failed to compile rustc_data_structures. Legit.

[00:19:50] error: internal compiler error: /checkout/src/librustc/dep_graph/edges.rs:107: Re-opened node ConstEval(Fingerprint(12533344459010390675, 9240382652965425921))
[00:19:50] 
[00:19:50] note: the compiler unexpectedly panicked. this is a bug.
[00:19:50] 
[00:19:50] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:19:50] 
[00:19:50] note: rustc 1.21.0-dev (189ca70d2 2017-08-03) running on x86_64-unknown-linux-gnu
[00:19:50] 
[00:19:50] thread 'rustc' panicked at 'Box<Any>', /checkout/src/librustc_errors/lib.rs:486:8
[00:19:50] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:19:50] 
[00:19:50] error: Could not compile `rustc_data_structures`.

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 3, 2017
@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

Passes travis now after the fixes. Let's give it another try.

@bors
Copy link
Contributor

bors commented Aug 4, 2017

📌 Commit b34c5a2 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 4, 2017

⌛ Testing commit b34c5a2 with merge 6ae9c4835c091a7692fdd70376845e5c9f26df52...

@bors
Copy link
Contributor

bors commented Aug 4, 2017

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Aug 4, 2017

@bors retry #40474 — x86_64-pc-windows-gnu failed to download LLVM

Attempting with retry: sh -c rm -f d9e7d2696e41983b6b5a0b4fac604b4e548a84d3.tar.gz &&             curl -sSL -O https://github.com/rust-lang/llvm/archive/d9e7d2696e41983b6b5a0b4fac604b4e548a84d3.tar.gz
gzip: stdin: not in gzip format

@bors
Copy link
Contributor

bors commented Aug 4, 2017

⌛ Testing commit b34c5a2 with merge d692a91...

bors added a commit that referenced this pull request Aug 4, 2017
@bors
Auto merge of #43590 - michaelwoerister:no-reopening-1, r=nikomatsakis
d692a91 
incr.comp.: Assert that no DepNode is re-opened (see issue #42298).

This PR removes the last occurrence of DepNode re-opening and adds an assertion that prevents our doing so in the future too. The DepGraph should no be guaranteed to be cycle free.

r? @nikomatsakis

EDIT: Closes #42298
@bors
Copy link
Contributor

bors commented Aug 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing d692a91 to master...

@bors bors merged commit b34c5a2 into rust-lang:master Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@michaelwoerister @nikomatsakis @bors @kennytm @aidanhs