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

Various const eval and pattern matching ICE fixes #67192

Merged
merged 22 commits into from
Dec 27, 2019

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 10, 2019

r? @RalfJung
cc @spastorino

This PR does not change existing behaviour anymore and just fixes a bunch of ICEs reachable from user code (sometimes even on stable via obscure union transmutes).

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 10, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 10, 2019

cc @varkor for the match code changes

Comment on lines 768 to 774
// FIXME(oli-obk): make this check an assertion that it's not a static here
// FIXME(RalfJ, oli-obk): document that `Place::Static` can never be anything but a static
// and `ConstValue::Unevaluated` can never be a static
let param_env = if self.tcx.is_static(gid.instance.def_id()) {
// FIXME(oli-obk, spastorino): the above FIXME is not true anymore, PlaceBase::Static does
// not exist anymore (except for promoteds but it's going away soon).
Copy link
Member

Choose a reason for hiding this comment

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

This chain of FIXMEs is getting some weird kind of joke.^^

I think it referred to the "lazy" vs "evaluated" AllocId for statics?

Copy link
Member

Choose a reason for hiding this comment

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

The last two are gone in my PR 😄

@RalfJung
Copy link
Member

I don't understand the motivation for this. This changes one of many allocation methods to avoid ZST, but we may still get empty allocations elsewhere. (At least, there's no assertion that I could see in Memory or so that would prevent this.) You are linking to codegen but I have no idea what that code is telling me... something is special-casing ZST. Okay, so... what does that have to do with this PR?^^ The code in codegen creates an "undef immediate", while CTFE uses an integer ptr, so if this PR is about making codegen and CTFE more consistent then I don't see it.

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 10, 2019

I don't understand the motivation for this. This changes one of many allocation methods to avoid ZST, but we may still get empty allocations elsewhere.

I could set it up so ZST Stack allocations can never actually have alloc ids. That would fold all of this logic down to the allocate function instead of the piecemeal spread around the interpreter.

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 13, 2019

I tried to keep every change as independent of all the others as possible and make a commit for each step. Some of these could be pulled out into a separate PR without changing behaviour if you want.

@bors
Copy link
Contributor

bors commented Dec 13, 2019

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

@oli-obk oli-obk changed the title Don't generate AllocIds for ZST allocations Various const eval and pattern matching ICE fixes Dec 13, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 13, 2019

This PR has now turned around 100% and is not changing behaviour wrt zst addresses anymore

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 20, 2019

I believe I have addressed all review comments

bors added a commit that referenced this pull request Dec 20, 2019
Promote references to constants instead of statics

This sits on top of @oli-obk's #67192. When that's merged I can rebase and make it a final PR.

r? @oli-obk
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 21, 2019

@bors try

@bors
Copy link
Contributor

bors commented Dec 21, 2019

⌛ Trying commit d2ad810 with merge e45d134659ec3c8628d74e3ddb5c9f0d1df24223...

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 26, 2019

rebased over rustfmt and const_eval.rs split PRs

@bors r=RalfJung,varkor

@bors
Copy link
Contributor

bors commented Dec 26, 2019

📌 Commit f65a91e has been approved by RalfJung,varkor

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 26, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Dec 26, 2019
…varkor

Various const eval and pattern matching ICE fixes

r? @RalfJung
cc @spastorino

This PR does not change existing behaviour anymore and just fixes a bunch of ICEs reachable from user code (sometimes even on stable via obscure union transmutes).
bors added a commit that referenced this pull request Dec 26, 2019
Rollup of 12 pull requests

Successful merges:

 - #67112 (Refactor expression parsing thoroughly)
 - #67192 (Various const eval and pattern matching ICE fixes)
 - #67287 (typeck: note other end-point when checking range pats)
 - #67459 (prune ill-conceived BTreeMap iter_mut assertion and test its mutability)
 - #67576 (reuse `capacity` variable in slice::repeat)
 - #67602 (Use issue = "none" instead of "0" in intrinsics)
 - #67614 (Set callbacks globally)
 - #67617 (Remove `compiler_builtins_lib` documentation)
 - #67629 (Remove redundant link texts)
 - #67632 (Convert collapsed to shortcut reference links)
 - #67633 (Update .mailmap)
 - #67635 (Document safety of Path casting)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Dec 27, 2019

⌛ Testing commit f65a91e with merge 8f5f8f9...

bors added a commit that referenced this pull request Dec 27, 2019
Various const eval and pattern matching ICE fixes

r? @RalfJung
cc @spastorino

This PR does not change existing behaviour anymore and just fixes a bunch of ICEs reachable from user code (sometimes even on stable via obscure union transmutes).
@bors
Copy link
Contributor

bors commented Dec 27, 2019

☀️ Test successful - checks-azure
Approved by: RalfJung,varkor
Pushing 8f5f8f9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 27, 2019
@bors bors merged commit f65a91e into rust-lang:master Dec 27, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #67192!

Tested on commit 8f5f8f9.
Direct link to PR: #67192

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

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Dec 27, 2019
Tested on commit rust-lang/rust@8f5f8f9.
Direct link to PR: <rust-lang/rust#67192>

💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).
bors added a commit to rust-lang/miri that referenced this pull request Dec 27, 2019
Remove all uses of `to_ptr`

Also only works on top of rust-lang/rust#67192
@oli-obk oli-obk deleted the const_zst_addr branch March 16, 2021 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

10 participants