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.

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

Don't promote to 'static the result of dereferences. #47408

Merged
merged 1 commit into from
Feb 17, 2018

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jan 13, 2018

This is a breaking change, removing copies out of dereferences from rvalue-to-'static promotion.

With miri we won't easily know whether the dereference itself would see the same value at runtime as miri (e.g. after mutating a static) or even if it can be interpreted (e.g. integer pointers).
One alternative to this ban is defining at least some of those situations as UB, i.e. you shouldn't have a reference in the first place, and you should work through raw pointers instead, to avoid promotion.

EDIT: The other may seem to be to add some analysis which whitelists references-to-constant-values and assume any values produced by arbitrary computation to not be safe to promote dereferences thereof - but that means producing a reference from an associated constant or const fn would necessarily obscure it, and in the former case, this could still impact code that runs on stable today. What we do today to track "references to statics" only works because we restrict taking a reference to a static at all to other statics (which, again, are currently limited in that they can't be read at compile-time) and to runtime-only fns (not const fns).

I'm primarily opening this PR with a conservative first approximation (e.g. &(*r).a is not allowed, only reborrows are, and in the old borrow only implicit ones from adjustments, at that) for cratering.

r? @nikomatsakis

@eddyb
Copy link
Member Author

eddyb commented Jan 13, 2018

cc @rust-lang/lang @Kimundi - we didn't catch this before stabilization, will have to make a decision.

@bors try

@bors
Copy link
Contributor

bors commented Jan 13, 2018

⌛ Trying commit 121499a with merge e15dd17...

bors added a commit that referenced this pull request Jan 13, 2018
Don't promote to 'static the result of dereferences.

**DO NOT MERGE** *(yet)*

This is a **breaking change**, removing copies out of dereferences from rvalue-to-`'static` promotion.

With miri we won't easily know whether the dereference itself would see the same value at runtime as miri (e.g. after mutating a `static`) or even if it can be interpreted (e.g. integer pointers).
The alternative to this ban is defining at least *some* of those situations as UB, i.e. you shouldn't have a reference in the first place, and you should work through raw pointers instead, to avoid promotion.

I'm primarily opening this PR with a conservative first approximation (e.g. `&(*r).a` is not allowed, only reborrows are, and in the old borrow only implicit ones from adjustments, at that) for cratering.

r? @nikomatsakis
@kennytm kennytm added T-lang Relevant to the language team, which will review and decide on the PR/issue. S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 13, 2018
@bors
Copy link
Contributor

bors commented Jan 13, 2018

☀️ Test successful - status-travis
State: approved= try=True

@eddyb
Copy link
Member Author

eddyb commented Jan 13, 2018

@rust-lang/infra Can I get a crater run on this?

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jan 17, 2018

Crater run started (3ish hours ago, actually).

@nikomatsakis
Copy link
Contributor

@Mark-Simulacrum results available?

@Mark-Simulacrum
Copy link
Member

Probably Tuesday mid-day or Wednesday unfortunately; we have problems with crater downloading the wrong CI artifacts that took a couple days to resolve. Crater claims 22 hours left, but I don't really believe it.

@alexreg
Copy link
Contributor

alexreg commented Jan 26, 2018

@Mark-Simulacrum Update, please?

@Mark-Simulacrum
Copy link
Member

We have not yet resolved some problem in crater that uses us all available disk space. The problem hasn't been identified, so until it is, we can't move forward unfortunately.

@aidanhs
Copy link
Member

aidanhs commented Jan 31, 2018

We've bitten the bullet and have got a disk with much more space - I anticipate the run succeeding now (just started).

@aidanhs
Copy link
Member

aidanhs commented Feb 4, 2018

At long last!

Crater results: http://cargobomb-reports.s3.amazonaws.com/pr-47408/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

@kennytm
Copy link
Member

kennytm commented Feb 4, 2018

Looks like all regressions are spurious

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Feb 4, 2018
@eddyb
Copy link
Member Author

eddyb commented Feb 4, 2018

@aidanhs @kennytm Thanks! I agree that there appear to only be spurious regressions.
This PR can only cause less code to compile, but there's no regressions due to compile errors.

@eddyb
Copy link
Member Author

eddyb commented Feb 4, 2018

@rfcbot pr merge

I'm proposing to merge this PR, retroactively restricting rvalue-to-'static promotion to not promote any copy through a dereference (for which we'd need intrusive points-to analyses post-miri).

@rfcbot
Copy link

rfcbot commented Feb 4, 2018

Team member @eddyb has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 4, 2018
@kennytm kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 4, 2018
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 4, 2018
@nikomatsakis
Copy link
Contributor

Nominating so that I can badger @rust-lang/lang members into checking their boxes.

@nikomatsakis
Copy link
Contributor

I checked for @nrc -- on the premise that he is not objecting but just busy.

@aidanhs
Copy link
Member

aidanhs commented Feb 8, 2018

Out of curiosity, can you give a small self-contained example of something that gets broken by this?

@bors
Copy link
Contributor

bors commented Feb 16, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 16, 2018
@kennytm
Copy link
Member

kennytm commented Feb 16, 2018

@bors retry #46903

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2018
@bors
Copy link
Contributor

bors commented Feb 16, 2018

⌛ Testing commit 121499a with merge 24a82a987f9f716fad83088470137c38badfc4c1...

@bors
Copy link
Contributor

bors commented Feb 16, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 16, 2018
@kennytm
Copy link
Member

kennytm commented Feb 16, 2018

@bors retry #46903

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2018
@bors
Copy link
Contributor

bors commented Feb 16, 2018

⌛ Testing commit 121499a with merge b6c10e42efef10877fc43c26a5712cd7f78d9533...

@bors
Copy link
Contributor

bors commented Feb 16, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 16, 2018
@kennytm
Copy link
Member

kennytm commented Feb 17, 2018

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 17, 2018
@bors
Copy link
Contributor

bors commented Feb 17, 2018

⌛ Testing commit 121499a with merge 5313e87...

bors added a commit that referenced this pull request Feb 17, 2018
Don't promote to 'static the result of dereferences.

This is a **breaking change**, removing copies out of dereferences from rvalue-to-`'static` promotion.

With miri we won't easily know whether the dereference itself would see the same value at runtime as miri (e.g. after mutating a `static`) or even if it can be interpreted (e.g. integer pointers).
One alternative to this ban is defining at least *some* of those situations as UB, i.e. you shouldn't have a reference in the first place, and you should work through raw pointers instead, to avoid promotion.

**EDIT**: The other *may seem* to be to add some analysis which whitelists references-to-constant-values and assume any values produced by arbitrary computation to not be safe to promote dereferences thereof - but that means producing a reference from an associated constant or `const fn` would necessarily obscure it, and in the former case, this could still impact code that runs on stable today. What we do today to track "references to statics" only works because we restrict taking a reference to a `static` at all to other `static`s (which, again, are currently limited in that they can't be read at compile-time) and to runtime-only `fn`s (*not* `const fn`s).

I'm primarily opening this PR with a conservative first approximation (e.g. `&(*r).a` is not allowed, only reborrows are, and in the old borrow only implicit ones from adjustments, at that) for cratering.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Feb 17, 2018

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

@bors bors merged commit 121499a into rust-lang:master Feb 17, 2018
@alexreg
Copy link
Contributor

alexreg commented Feb 19, 2018

@nikomatsakis Maybe worth taking off the tags (especially FCP) now that this is closed/merged... unless there's a reason it should still be open? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants