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

Remove unsafety from sys/unsupported and add deny(unsafe_op_in_unsafe_fn). #77722

Merged

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Oct 8, 2020

Replacing UnsafeCells by a Cells simplifies things and makes the mutex and rwlock implementations safe. Other than that, only unsafety in strlen() contained unsafe code.

@rustbot modify labels: +F-unsafe-block-in-unsafe-fn +C-cleanup

Replacing the UnsafeCell by a Cell simplifies things and makes it all
safe.
Replacing the UnsafeCell by a Cell makes it all safe.
@rustbot rustbot added C-cleanup Category: PRs that clean code up or issues documenting cleanup. F-unsafe-block-in-unsafe-fn RFC #2585 labels Oct 8, 2020
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Oct 8, 2020
@m-ou-se m-ou-se changed the title Remove unsafety from unsupported/{mutex,rwlock}.rs by using a Cell. Remove unsafety from sys/unsupported and add deny(unsafe_op_in_unsafe_fn). Oct 8, 2020
@jyn514 jyn514 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 8, 2020
@Mark-Simulacrum
Copy link
Member

Could you add a comment on both introduced Cells that they're for platforms without threads so there's no need for synchronization? The UnsafeCell would've wanted that comment too, I feel.

It seems a bit unfortunate that we implement Sync for the Mutex/RwLock in sys/unsupported -- do we really need to do that? It's obviously untrue... it seems like maybe ideally Rust would provide a cfg(no_threads) or whatever for us.

@Mark-Simulacrum Mark-Simulacrum 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 Oct 13, 2020
@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 13, 2020

Sure. Will add a comment.

It's obviously untrue...

Well they're safe to share between all the threads your program can have on those platforms: just the one main thread. ^^'

Without Sync, you couldn't use one in a static like on other platforms.

@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 13, 2020

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@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 Oct 13, 2020
@Mark-Simulacrum
Copy link
Member

Yes, I mean that it would be better if (for example) the compiler removed Sync bounds on statics, for example, on single-threaded platforms. But that would require lang approval and design :)

@bors r+

@bors
Copy link
Contributor

bors commented Oct 13, 2020

📌 Commit b26aa5d has been approved by Mark-Simulacrum

@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 Oct 13, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 13, 2020
…ported-locks, r=Mark-Simulacrum

Remove unsafety from sys/unsupported and add deny(unsafe_op_in_unsafe_fn).

Replacing `UnsafeCell`s by a `Cell`s simplifies things and makes the mutex and rwlock implementations safe. Other than that, only unsafety in strlen() contained unsafe code.

@rustbot modify labels: +F-unsafe-block-in-unsafe-fn +C-cleanup
@JohnTitor
Copy link
Member

Failed in https://github.com/rust-lang-ci/rust/runs/1248784774:

warning: dropping unsupported crate type `dylib` for target `wasm32-unknown-unknown`

error: unnecessary `unsafe` block
  --> library/std/src/sys/wasm/../unsupported/common.rs:43:5
   |
43 |     unsafe {
   |     ^^^^^^ unnecessary `unsafe` block
   |
   = note: `-D unused-unsafe` implied by `-D warnings`

error: aborting due to previous error; 1 warning emitted

[RUSTC-TIMING] std test:false 2.295
error: could not compile `std`

@bors r-

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 13, 2020
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 13, 2020
@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 13, 2020

Ah, apparently sys/wasm takes some stuff directly from sys/unsupported. Then sys/unsupported/mod.rs gets bypassed and the deny(unsafe_op_in_unsafe_fn) attribute does not apply.

@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 13, 2020

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Checked locally that std now compiles for wasm32-unknown-unknown. I only applied the attribute to the one mondule where it was necessary. Looks like sys/wasm will be applying the attribute to everything soon as well: #74477

@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 Oct 13, 2020
@m-ou-se m-ou-se force-pushed the safe-unsupported-locks branch from f92b36a to af414dc Compare October 13, 2020 16:56
@jyn514
Copy link
Member

jyn514 commented Oct 13, 2020

@bors r=Mark-Simulacrum rollup=iffy

@bors
Copy link
Contributor

bors commented Oct 13, 2020

📌 Commit af414dc has been approved by Mark-Simulacrum

@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 Oct 13, 2020
@bors
Copy link
Contributor

bors commented Oct 13, 2020

⌛ Testing commit af414dc with merge 772eaeb38d5203c0efb95201b52b2bbaa780c734...

@bors
Copy link
Contributor

bors commented Oct 13, 2020

💔 Test failed - checks-actions

@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 Oct 13, 2020
@JohnTitor
Copy link
Member

@bors retry
Seems the failure is due to GitHub outage (it's resolved now): https://www.githubstatus.com/incidents/nq2s4gt7xs1w

@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 Oct 13, 2020
@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 13, 2020

Ah, thanks! Was just about to post a comment because I didn't understand what failed.

By the way, looks like all the relevant platforms for this PR passed their tests, so rollup should be fine now. ^^

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2020
Rollup of 14 pull requests

Successful merges:

 - rust-lang#77239 (Enable building Cargo for aarch64-apple-darwin)
 - rust-lang#77569 (BTreeMap: type-specific variants of node_as_mut and cast_unchecked)
 - rust-lang#77719 (Remove unnecessary rustc_const_stable attributes.)
 - rust-lang#77722 (Remove unsafety from sys/unsupported and add deny(unsafe_op_in_unsafe_fn).)
 - rust-lang#77725 (Add regression issue template)
 - rust-lang#77776 ( Give an error when running `x.py test --stage 0 src/test/ui`)
 - rust-lang#77786 (Mention rustdoc in `x.py setup`)
 - rust-lang#77825 (`min_const_generics` diagnostics improvements)
 - rust-lang#77868 (Include `llvm-dis`, `llc` and `opt` in `llvm-tools-preview` component)
 - rust-lang#77884 (Use Option::unwrap_or instead of open-coding it)
 - rust-lang#77886 (Replace trivial bool matches with the `matches!` macro)
 - rust-lang#77892 (Replace absolute paths with relative ones)
 - rust-lang#77895 (Include aarch64-apple-darwin in the dist manifests)
 - rust-lang#77909 (bootstrap: set correct path for the build-manifest binary)

Failed merges:

 - rust-lang#77902 (Include aarch64-pc-windows-msvc in the dist manifests)

r? `@ghost`
@bors bors merged commit cc5a1aa into rust-lang:master Oct 14, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. F-unsafe-block-in-unsafe-fn RFC #2585 S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants