Skip to content

/rust

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.

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

Split dummy-idx node to fix expand_givens DFS #31442

Merged
merged 2 commits into from Feb 8, 2016

Conversation

Reviewers
No reviews
Assignees

@arielb1 arielb1

Labels
T-compiler beta-accepted
Projects
None yet
Milestone
No milestone
6 participants
@pnkfelix @rust-highfive @arielb1 @nikomatsakis @bors @alexcrichton
@pnkfelix
Copy link
Member

pnkfelix commented Feb 6, 2016

Split dummy-idx node to fix expand_givens DFS

(Much more detail in commit comments.)

Fix #30438.

@rust-highfive rust-highfive assigned arielb1 Feb 6, 2016

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Feb 6, 2016

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)
@arielb1

This comment has been minimized.

Sign in to view
Copy link
Contributor

arielb1 commented Feb 6, 2016

I would just split dummy_idx to a src and dst part. Either that or have the comments less uncertain.
@arielb1

This comment has been minimized.

Sign in to view
Copy link
Contributor

arielb1 commented Feb 6, 2016

I found that bug independently (that's what I deserve for going bug-hunting with such a huge notification backlog), and it was the same givens bug.
@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member Author

pnkfelix commented Feb 6, 2016

Ah yeah splitting dummy_idx sounds better in nearly every way. I'll do that tomorrow

@pnkfelix pnkfelix force-pushed the pnkfelix:issue-30438-sidestep-dummy-node-during-expand-givens-dfs branch from 544dafc to 77c8850 Feb 8, 2016

@pnkfelix pnkfelix changed the title Sidestep dummy-idx node during expand_givens DFS Split dummy-idx node to fix expand_givens DFS Feb 8, 2016

@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member Author

pnkfelix commented Feb 8, 2016

Also, this fix is so simple that I'm nominating it to be merged to beta as well.

@pnkfelix pnkfelix added beta-nominated T-compiler labels Feb 8, 2016

@pnkfelix
Split dummy in region inference graph into distinct source and sink n… 
…odes.

Why do this: The RegionGraph representation previously conflated all
of the non-variable regions (i.e. the concrete regions such as
lifetime parameters to the current function) into a single dummy node.

A single dummy node leads DFS on a graph `'a -> '_#1 -> '_#0 -> 'b` to
claim that `'_#1` is reachable from `'_#0` (due to `'a` and `'b` being
conflated in the graph representation), which is incorrect (and can
lead to soundness bugs later on in compilation, see #30438).

Splitting the dummy node ensures that DFS will never introduce new
ancestor relationships between nodes for variable regions in the
graph.
8801bdb
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Feb 8, 2016

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 8, 2016

📌 Commit 77c8850 has been approved by nikomatsakis
@pnkfelix
regression tests for issue #30438. 
Fix #30438.
Loading status checks…
77c8850
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 8, 2016

⌛️ Testing commit 77c8850 with merge 04f12ef...

bors added a commit that referenced this pull request Feb 8, 2016

@bors
Auto merge of #31442 - pnkfelix:issue-30438-sidestep-dummy-node-durin… 
…g-expand-givens-dfs, r=nikomatsakis

Split dummy-idx node to fix expand_givens DFS

(Much more detail in commit comments.)

Fix #30438.
04f12ef
@bors

@bors bors merged commit 77c8850 into rust-lang:master Feb 8, 2016
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details

@nikomatsakis nikomatsakis added the beta-accepted label Feb 11, 2016

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Feb 11, 2016

We don't normally backport random fixes, but this seems like both a severe problem (unsoundness and a kind of random one) and a simple fix, so go for it.

@llogiq llogiq referenced this pull request Feb 14, 2016

Closed

TWIR 118's COTW + interesting PRs #170

@pnkfelix pnkfelix referenced this pull request Feb 18, 2016

Merged

Backport pr 31442 to beta: Split dummy-idx node to fix expand_givens DFS #31768

@alexcrichton alexcrichton removed the beta-nominated label Feb 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.