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

Validate constants during const_eval_raw #74949

Merged
merged 18 commits into from
Sep 20, 2020
Merged

Validate constants during const_eval_raw #74949

merged 18 commits into from
Sep 20, 2020

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jul 30, 2020 •
edited
Loading

This PR implements the groundwork for #72396

  • constants are now validated during const_eval_raw
  • to prevent cycle errors, we do not validate references to statics anymore beyond the fact that they are not dangling
  • the const_eval query ICEs if used on static items
  • as a side effect promoteds are now evaluated to ConstValue::Scalar again (since they are just a reference to the actual promoted allocation in most cases).

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2020
@petrochenkov
Copy link
Contributor

r? @RalfJung

@RalfJung
Copy link
Member

I'm going to also change it in this PR so that the const_eval query may not be used for statics anymore

Are you also going to do the renames we talked about? Where the "raw query" returns a mir::Const, and then maybe that query is called const_eval and the one returning ty::Const is called const_eval_for_ty or so.

One problem I realized for this is that consumers that want to work with mir::Const still have to also be able to handle ty::Const embedded in MIR as operands -- it's just that if they encounter an Unevaluated they want to use mir::Const. Actually, does it really make sense for Operand to use ty::Const? This is not in a type after all, and all sorts of consts are supported! So mir::Const seems more appropriate. Except maybe for efficiency reasons we want to sometimes not generate a tcx Allocation backing this const (does this ever happen?), so the type we want would be more like "mir::Const or ty::Const". We probably also need that because MIR can refer to const generics.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 31, 2020

Are you also going to do the renames we talked about?

Idk, should I do them in the same PR? These are pretty invasive to a lot of code, so mixing them with this PR may make it hard to distinguish functional from non-functional changes.

Actually, does it really make sense for Operand to use ty::Const?

no, this would be exactly the site where we'd use mir::Const (or a multi-type as you suggested). What other sites did you have in mind for mir::Const?

Except maybe for efficiency reasons we want to sometimes not generate a tcx Allocation backing this const (does this ever happen?), so the type we want would be more like "mir::Const or ty::Const". We probably also need that because MIR can refer to const generics.

yes, I was not going to do this change in this PR, as it has its own complexity problems and I worry about the reviewability of such a huge change (I have a local branch working on this seperately).

@oli-obk oli-obk marked this pull request as ready for review July 31, 2020 11:34
RalfJung
RalfJung reviewed Aug 9, 2020
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Looks great overall. :) But I have a few comments and nits.

src/librustc_middle/mir/interpret/queries.rs Outdated Show resolved Hide resolved
src/librustc_mir/const_eval/eval_queries.rs Outdated Show resolved Hide resolved
src/librustc_mir/const_eval/eval_queries.rs Outdated Show resolved Hide resolved
src/librustc_mir/const_eval/eval_queries.rs Outdated Show resolved Hide resolved
src/librustc_mir/interpret/operand.rs Outdated Show resolved Hide resolved
src/librustc_mir/interpret/validity.rs Outdated Show resolved Hide resolved
src/test/ui/consts/const-eval/double_check2.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Aug 9, 2020

Actually, does it really make sense for Operand to use ty::Const?

no, this would be exactly the site where we'd use mir::Const (or a multi-type as you suggested). What other sites did you have in mind for mir::Const?

Mostly the return type of the const_eval_static query.

@RalfJung RalfJung mentioned this pull request Aug 10, 2020
Closed
@bors
Copy link
Contributor

bors commented Aug 11, 2020

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

@oli-obk oli-obk mentioned this pull request Aug 11, 2020
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 12, 2020
@JohnTitor
Rollup merge of rust-lang#75394 - oli-obk:get_static, r=RalfJung
5ef0a0a 
Add a function to `TyCtxt` for computing an `Allocation` for a `static` item's initializer

r? @RalfJung

miri-side: rust-lang/miri#1507

split out of rust-lang#74949 (comment) to make that PR leaner
@RalfJung
Copy link
Member

The Miri-affecting change landed so I think this is unblocked.

Btw I think the PR description needs updating.

@RalfJung RalfJung added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2020
@oli-obk oli-obk force-pushed the validate_const_eval_raw branch 2 times, most recently from f759b9c to f0978b3 Compare August 21, 2020 08:34
@bors
Copy link
Contributor

bors commented Aug 22, 2020

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

@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 Sep 19, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 20, 2020

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Sep 20, 2020

📌 Commit 34785fc has been approved by RalfJung

@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 Sep 20, 2020
@bors
Copy link
Contributor

bors commented Sep 20, 2020

⌛ Testing commit 34785fc with merge 5e449b9...

@bors
Copy link
Contributor

bors commented Sep 20, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: RalfJung
Pushing 5e449b9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 20, 2020
@bors bors mentioned this pull request Sep 20, 2020
Merged
@rust-highfive rust-highfive mentioned this pull request Sep 20, 2020
Closed
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #74949!

Tested on commit 5e449b9.
Direct link to PR: #74949

💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Sep 20, 2020
@rust-highfive
📣 Toolstate changed by rust-lang/rust#74949!
3f2157a 
Tested on commit rust-lang/rust@5e449b9.
Direct link to PR: <rust-lang/rust#74949>

💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
@bors bors merged commit 5e449b9 into rust-lang:master Sep 20, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 20, 2020
@bors bors mentioned this pull request Sep 20, 2020
Merged
@oli-obk oli-obk deleted the validate_const_eval_raw branch September 20, 2020 11:05
This was referenced Sep 20, 2020
Merged
Merged
@matthiaskrgr
Copy link
Member

Looks like this introduced some perf regressions https://perf.rust-lang.org/compare.html?start=10b3595ba6a4c658c9dea105488fc562c815e434&end=5e449b9adff463455743291b0c1f76feec092992&stat=instructions:u

@RalfJung
Copy link
Member

Uff, 500% on the stress test is heavy.^^ I guess we'll have to revert? :(

@Mark-Simulacrum
Copy link
Member

Note that it's just incremental getting worse on ~everything that I checked, so I suspect that probably we're invalidating caches more somewhere -- maybe fixable?

But FWIW I am not too worried about incremental perf when the trade-off is worse code or poor checking. Though of course if we can avoid regressing it that would be good :)

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 21, 2020

I'll investigate.

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 21, 2020

lol found it: eval_to_allocation_raw is not cached on disk at all.

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 21, 2020

Fix PR: #77006

@oli-obk oli-obk mentioned this pull request Sep 21, 2020
Merged
@ecstatic-morse
Copy link
Contributor

Hi! This PR showed up in the weekly perf triage report. It resulted in a very large regression in instruction counts of incremental builds (up to 515.8% on incr-unchanged builds of ctfe-stress-4-check).

Looks like this is already fixed. Nice work all!

@camelid camelid mentioned this pull request Sep 22, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 24, 2020
@bors
Auto merge of rust-lang#77006 - oli-obk:🐌_const_queries, r=Mark-Simul…
87d262a 
…acrum

Cache `eval_to_allocation_raw` on disk

rust-lang#74949 (comment) regressed the performance on these queries, this PR gets the perf back.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.
9 participants
@oli-obk @rust-highfive @petrochenkov @RalfJung @bors @matthiaskrgr @Mark-Simulacrum @ecstatic-morse @rustbot