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

miri: Detect uninitialized integers and floats #88670

Merged
merged 1 commit into from
Nov 10, 2021
Merged

miri: Detect uninitialized integers and floats #88670

merged 1 commit into from
Nov 10, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Sep 5, 2021 •
edited
Loading

Part of rust-lang/miri#1340.

Companion Miri PR: rust-lang/miri#1904

r? @RalfJung

@camelid camelid added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-miri Area: The miri tool labels Sep 5, 2021
@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@camelid camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 9, 2021
@camelid
Copy link
Member Author

camelid commented Sep 23, 2021

Rebased to fix merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid camelid mentioned this pull request Oct 26, 2021
Merged
@camelid camelid changed the title [WIP] miri: Detect uninitialized integers and floats miri: Detect uninitialized integers and floats Oct 26, 2021
@camelid camelid marked this pull request as ready for review October 26, 2021 23:15
@camelid
Copy link
Member Author

camelid commented Oct 26, 2021

I can squash, too, but I'll wait for you to review first :)

@camelid camelid 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 Oct 27, 2021
@camelid camelid added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2021
camelid
camelid commented Nov 7, 2021
View reviewed changes
compiler/rustc_const_eval/src/interpret/validity.rs Outdated
@@ -212,6 +212,10 @@ struct ValidityVisitor<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> {
}

impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M> {
fn allow_uninit_and_ptr_numbers(&self) -> bool {
!M::enforce_number_initialization(self.ecx)
Copy link
Member Author

Choose a reason for hiding this comment

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

One thing that might be confusing about this is that enforce_number_initialization also controls whether pointer numbers are allowed. Should I rename enforce_number_initialization to something like enforce_number_validity (and maybe rename the Miri-the-tool flag too)?

Copy link
Member

Choose a reason for hiding this comment

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

I like enforce_number_validity. :)

Also, what is the point of introducing fn allow_uninit_and_ptr_numbers here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like enforce_number_validity. :)

Ok, I'll rename to that :)

One thing though: will it be confusing because enforce_validity also exists?

Also, what is the point of introducing fn allow_uninit_and_ptr_numbers here?

It's not really necessary anymore; I'll remove it.

Copy link
Member

Choose a reason for hiding this comment

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

One thing though: will it be confusing because enforce_validity also exists?

On a broad level it seems consistent. It gets a bit subtle but I think we can handle this with comments/documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, and if we have to, we can rename the flag later too. Anyway, I updated this PR and the miri PR with the rename :)

@RalfJung
Copy link
Member

Looking good, apart from a final comment nit. :)

@camelid
miri: Detect uninitialized integers and floats
d8a1454 
Change the Miri engine to allow configuring whether to check
initialization of integers and floats. This allows the Miri tool to
optionally check for initialization if requested by the user.
@camelid
Copy link
Member Author

camelid commented Nov 10, 2021

Done! Thanks for your help with this PR :)

@RalfJung
Copy link
Member

Sure, thanks for your patience. :)
@bors r+

@bors
Copy link
Contributor

bors commented Nov 10, 2021

📌 Commit d8a1454 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 Nov 10, 2021
@bors
Copy link
Contributor

bors commented Nov 10, 2021

⌛ Testing commit d8a1454 with merge 800a156...

@bors
Copy link
Contributor

bors commented Nov 10, 2021

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 800a156 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 10, 2021
@bors bors merged commit 800a156 into rust-lang:master Nov 10, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 10, 2021
@rust-highfive rust-highfive mentioned this pull request Nov 10, 2021
Closed
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #88670!

Tested on commit 800a156.
Direct link to PR: #88670

💔 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 Nov 10, 2021
@rust-highfive
📣 Toolstate changed by rust-lang/rust#88670!
ab8ba75 
Tested on commit rust-lang/rust@800a156.
Direct link to PR: <rust-lang/rust#88670>

💔 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-timer
Copy link
Collaborator

Finished benchmarking commit (800a156): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

  • Large regression in instruction counts (up to 1.0% on incr-unchanged builds of wg-grammar)

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

@rustbot rustbot added the perf-regression Performance regression. label Nov 10, 2021
@RalfJung
Copy link
Member

RalfJung commented Nov 10, 2021 •
edited
Loading

I cannot quite explain why there would be any perf change here... if anything this should improve perf since two conditionals got replaced by a constant true. Strange.

@camelid camelid deleted the miri-uninit-num branch November 10, 2021 19:29
@camelid
Copy link
Member Author

camelid commented Nov 10, 2021

I cannot quite explain why there would be any perf change here... if anything this should improve perf since two conditionals got replaced by a constant true. Strange.

Maybe there's a mismatch between ctfe_mode and compile_time_machine!, so this check is being run more than before? From what I can see, the perf regressions seem to be in unrelated queries though.

@RalfJung
Copy link
Member

No, in rustc the check is only ever invoked by the CTFE query code after obtaining the final result. enforce_validity is false so the check is never run during evaluation.

camelid added a commit to camelid/miri that referenced this pull request Nov 10, 2021
@camelid
rustup
6dd1082 
So that we get rust-lang/rust#88670.
bors added a commit to rust-lang/miri that referenced this pull request Nov 10, 2021
@bors
Auto merge of #1904 - camelid:uninit-num, r=RalfJung
a8b976e 
Add flag to check for uninitialized numbers

Closes #1340.

Companion rustc PR that implements this in the Miri engine: rust-lang/rust#88670

r? `@RalfJung`
@Mark-Simulacrum Mark-Simulacrum removed the perf-regression Performance regression. label Nov 16, 2021
@Mark-Simulacrum
Copy link
Member

I'm pretty sure this is noise, likely due to the introduction of randomized hashing as part of incr-comp verification that hasn't lasted long enough yet to bump noise levels up as part of perf collection.

Unmarking as a regression.

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
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-miri Area: The miri tool 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.
9 participants
@camelid @rust-highfive @rust-log-analyzer @bors @RalfJung @rust-timer @Mark-Simulacrum @apiraino @rustbot