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

Let CTFE to handle partially uninitialized unions without marking the entire value as uninitialized. #94527

Merged
merged 3 commits into from
Apr 5, 2022

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 2, 2022

follow up to #94411

To fix #69488 and by extension fix #94371, we should stop treating types like MaybeUninit<usize> as something that the Scalar type in the interpreter engine can represent. So we add a new field to abi::Primitive that records whether the primitive is nested in a union

cc @RalfJung

r? @ghost

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 2, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 2, 2022

@bors r+ @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Mar 2, 2022

📌 Commit 66a1d3d02ea0c3473163301fcd1d31b2fbd0fba6 has been approved by oli-obk

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 2, 2022
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 2, 2022
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 2, 2022

@bors r- oops

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 2, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 2, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Mar 2, 2022

⌛ Trying commit efa7539866423e0fd1ca420730ece30204dd8b74 with merge d8d9e85406dc30f48aa6825ae48eb8f7c366efea...

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 3, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@RalfJung
Copy link
Member

RalfJung commented Mar 3, 2022

Bors didn't seem to react?
@bors try

@bors
Copy link
Contributor

bors commented Mar 3, 2022

⌛ Trying commit 837ac8d340aa2ce177277ddd8e0c1e2f7ffa1f5c with merge 124a62c1537d6f500769f854b7e6d193479561a9...

@RalfJung
Copy link
Member

RalfJung commented Mar 3, 2022

So from what I can tell this is entirely unconnected to the logic in #93670? We might not want to emit noundef everywhere quite yet as this PR would, but not relating these two at all also seems odd.

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 3, 2022

First I want to see if it is feasible to go this route, then we can talk to the llvm people. I do believe we should be able to use it at some point, but it may very well be badly handled by llvm for now.

The linked PR works on types, where this PR encodes the information in the layout. So there may be unintended consequences. For now we should probably only use it in miri.

Maybe we should also flip the logic of the Boolean and search for a better name (!noundef checks are the worst, triple negation)

@bors
Copy link
Contributor

bors commented Mar 3, 2022

☀️ Try build successful - checks-actions
Build commit: 124a62c1537d6f500769f854b7e6d193479561a9 (124a62c1537d6f500769f854b7e6d193479561a9)

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 5, 2022

@bors r=nagisa,erikdesjardin

@bors
Copy link
Contributor

bors commented Apr 5, 2022

📌 Commit d57b755 has been approved by nagisa,erikdesjardin

@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. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 5, 2022
@bors
Copy link
Contributor

bors commented Apr 5, 2022

⌛ Testing commit d57b755 with merge f262ca1...

@bors
Copy link
Contributor

bors commented Apr 5, 2022

☀️ Test successful - checks-actions
Approved by: nagisa,erikdesjardin
Pushing f262ca1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 5, 2022
@bors bors merged commit f262ca1 into rust-lang:master Apr 5, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 5, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f262ca1): comparison url.

Summary:

  • Primary benchmarks: changes not relevant. 3 results were found to be statistically significant but the changes were too small to be relevant.
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 1 6 2 6 3
mean2 0.5% 0.8% -0.4% -1.1% -0.1%
max 0.5% 1.1% -0.4% -1.3% 0.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

@rustbot rustbot added the perf-regression Performance regression. label Apr 5, 2022
bors added a commit to rust-lang/miri that referenced this pull request Apr 5, 2022
test that partially uninit MaybeUninit works correctly

This got finally fixed by rust-lang/rust#94527 :)
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2022
interp/validity: enforce Scalar::Initialized

This is a follow-up to rust-lang#94527, to also account for the new kind of `Scalar` layout inside the validity checker.

r? `@oli-obk`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 12, 2022
`UnsafeCell` blocks niches inside its nested type from being available outside

fixes rust-lang#87341

This implements the plan by `@eddyb` in rust-lang#87341 (comment)

Somewhat related PR (not strictly necessary, but that cleanup made this PR simpler): rust-lang#94527
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 13, 2022
`UnsafeCell` blocks niches inside its nested type from being available outside

fixes rust-lang#87341

This implements the plan by ``@eddyb`` in rust-lang#87341 (comment)

Somewhat related PR (not strictly necessary, but that cleanup made this PR simpler): rust-lang#94527
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 13, 2022
`UnsafeCell` blocks niches inside its nested type from being available outside

fixes rust-lang#87341

This implements the plan by `@eddyb` in rust-lang#87341 (comment)

Somewhat related PR (not strictly necessary, but that cleanup made this PR simpler): rust-lang#94527
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 18, 2022
`UnsafeCell` blocks niches inside its nested type from being available outside

fixes rust-lang#87341

This implements the plan by `@eddyb` in rust-lang#87341 (comment)

Somewhat related PR (not strictly necessary, but that cleanup made this PR simpler): rust-lang#94527
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. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
10 participants