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

Introduce stricter checks for might_permit_raw_init under a debug flag #97323

Merged
merged 1 commit into from
May 25, 2022

Conversation

5225225
Copy link
Contributor

@5225225 5225225 commented May 23, 2022

This is intended to be a version of the strict checks tried out in #79296, but also checking number validity (under the assumption that let _ = std::mem::uninitialized::<u32>() is UB, which seems to be what rust-lang/unsafe-code-guidelines#71 is leaning towards.)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 23, 2022
@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 May 23, 2022
@rust-log-analyzer

This comment has been minimized.

@5225225

This comment was marked as outdated.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 23, 2022
@5225225

This comment was marked as outdated.

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2022
@5225225
Copy link
Contributor Author

5225225 commented May 23, 2022

I'll leave the variant checks for future work.

Was initially going to try and get them in now (so it's not as much breakage to add them later), but I figure adding more panics on UB when you're using a debugging flag specifically to panic on UB is pretty justifiable.

That, and I want to get the infrastructure in here sooner than later. Will clean up this PR to just do the array and primitive checks.

@5225225 5225225 marked this pull request as ready for review May 23, 2022 22:13
@5225225
Copy link
Contributor Author

5225225 commented May 23, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2022
&& !layout.might_permit_raw_init(
self,
/*zero:*/ false,
self.tcx.sess.opts.debugging_opts.strict_init_checks,
Copy link
Contributor

Choose a reason for hiding this comment

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

somewhat annoying to pass this everywhere, when self is also passed. Though HasDataLayout doesn't have a useful way to get at this.

maybe add a field to TargetDataLayout for that bool?

Idk, doesn't seem great either. I guess let's roll with the PR as it is.

@oli-obk
Copy link
Contributor

oli-obk commented May 24, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 24, 2022

📌 Commit d59cc92d7a30f0b4b7d9e192ca0e165a306f61ef has been approved by oli-obk

@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 May 24, 2022
@oli-obk
Copy link
Contributor

oli-obk commented May 24, 2022

davidtwco approved these changes now

oh lol, apologies, that's funny timing

@RalfJung
Copy link
Member

Cc #66151

@RalfJung
Copy link
Member

@bors r-

We're not in a rush for this feature I think, so let's fix that nit before landing. :)

@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 May 24, 2022
@RalfJung
Copy link
Member

RalfJung commented May 24, 2022

And also thanks a lot @5225225 for doing this, I quite like the idea of opting-in to the more strict checks. :D

FWIW, as @oli-obk proposed once, there is an elegant way to get the "fully strict" check but it requires more work and no code sharing with the non-strict case: fire up an InterpCx and create an actual zeroed/uninit allocation, and then ask the validity checker in the interpreter whether this is valid for the given type. That will handle all types properly; it is the same logic that Miri uses to detect incorrect values. So this might be better than trying something ad-hoc for multi-variant types.
If that peeks your interest, feel free to ask about details on Zulip. :D

@5225225 5225225 force-pushed the strict_init_checks branch 2 times, most recently from 690792e to 3b7dc2d Compare May 24, 2022 11:37
@5225225
Copy link
Contributor Author

5225225 commented May 24, 2022

Okay, I've fixed the formatting in the cranelift code, moved the zero: bool to a valid_as: ValidAs enum (hope you don't mind, but I was touching this code anyways :) ), and removed the redundant parameter on is_uninit_valid.

And yeah, the "actually make an allocation and then ask the interpreter if it's valid" seems like the cleanest solution.

Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

cg_clif changes LGTM

@oli-obk
Copy link
Contributor

oli-obk commented May 24, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 24, 2022

📌 Commit dd9f31d has been approved by oli-obk

@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 May 24, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request May 25, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#93604 (Make llvm-libunwind a per-target option)
 - rust-lang#97026 (Change orderings of `Debug` for the Atomic types to `Relaxed`.)
 - rust-lang#97105 (Add tests for lint on type dependent on consts)
 - rust-lang#97323 (Introduce stricter checks for might_permit_raw_init under a debug flag )
 - rust-lang#97379 (Add aliases for `current_dir`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 02c0c76 into rust-lang:master May 25, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 25, 2022
@5225225 5225225 deleted the strict_init_checks branch June 2, 2022 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

9 participants