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

Std/thread: deny unsafe op in unsafe fn #74225

Merged
merged 5 commits into from
Sep 26, 2020
Merged

Std/thread: deny unsafe op in unsafe fn #74225

merged 5 commits into from
Sep 26, 2020

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented Jul 10, 2020 •
edited
Loading

Partial fix of #73904.

This encloses unsafe operations in unsafe fn in libstd/thread.
@rustbot modify labels: F-unsafe-block-in-unsafe-fn

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 Jul 10, 2020
@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Jul 11, 2020
Closed
22 tasks
@bors
Copy link
Contributor

bors commented Jul 28, 2020

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

@poliorcetics poliorcetics force-pushed the std-thread-unsafe-op-in-unsafe-fn branch from dfa69f8 to 58c7e35 Compare July 28, 2020 16:55
@bors
Copy link
Contributor

bors commented Aug 10, 2020

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

@poliorcetics poliorcetics force-pushed the std-thread-unsafe-op-in-unsafe-fn branch from 58c7e35 to 3a22b21 Compare August 10, 2020 17:00
@crlf0710 crlf0710 added T-libs Relevant to the library team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2020
LeSeulArtichaut
LeSeulArtichaut reviewed Aug 28, 2020
View reviewed changes
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut left a comment

Choose a reason for hiding this comment

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

Here is a quick high-level review (I can't go any deeper because I know pretty much nothing about threads and TLS and OSes). Thanks again for helping with this @poliorcetics!

library/std/src/thread/local.rs Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
@poliorcetics
Copy link
Contributor Author

In case GitHub makes it non-obvious: I've left answers to some of your suggestions and modified the other comments in response.

Mark-Simulacrum
Mark-Simulacrum reviewed Sep 15, 2020
View reviewed changes
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
library/std/src/thread/local.rs Outdated Show resolved Hide resolved
@poliorcetics
Copy link
Contributor Author

I modified all the comments that were noted. There are some part of those that could maybe go as documentation for their enclosing method and not inner SAFETY comments but, apart from those I did, I'm not sure how to write them.

@joshtriplett
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 20, 2020

📌 Commit 0e56b52 has been approved by joshtriplett

@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 Sep 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 21, 2020
@Dylan-DPC
Rollup merge of rust-lang#74225 - poliorcetics:std-thread-unsafe-op-i…
4cb73e3 
…n-unsafe-fn, r=joshtriplett

Std/thread: deny unsafe op in unsafe fn

Partial fix of rust-lang#73904.

This encloses `unsafe` operations in `unsafe fn` in `libstd/thread`.
@rustbot modify labels: F-unsafe-block-in-unsafe-fn
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Sep 21, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 21, 2020
@Dylan-DPC
Rollup merge of rust-lang#74225 - poliorcetics:std-thread-unsafe-op-i…
7e86f01 
…n-unsafe-fn, r=joshtriplett

Std/thread: deny unsafe op in unsafe fn

Partial fix of rust-lang#73904.

This encloses `unsafe` operations in `unsafe fn` in `libstd/thread`.
@rustbot modify labels: F-unsafe-block-in-unsafe-fn
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Sep 21, 2020
Closed
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 21, 2020
@RalfJung
Rollup merge of rust-lang#74225 - poliorcetics:std-thread-unsafe-op-i…
be347bc 
…n-unsafe-fn, r=joshtriplett

Std/thread: deny unsafe op in unsafe fn

Partial fix of rust-lang#73904.

This encloses `unsafe` operations in `unsafe fn` in `libstd/thread`.
@rustbot modify labels: F-unsafe-block-in-unsafe-fn
@RalfJung RalfJung mentioned this pull request Sep 21, 2020
Closed
@RalfJung
Copy link
Member

Failed in #77001 (comment)
@bors r- rollup=iffy

@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 Sep 21, 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 Sep 21, 2020
@poliorcetics poliorcetics force-pushed the std-thread-unsafe-op-in-unsafe-fn branch from 0e56b52 to 3afadaa Compare September 21, 2020 20:38
@poliorcetics
Copy link
Contributor Author

I fixed the bug for wasm32.

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

@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 Sep 21, 2020
@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 24, 2020

📌 Commit d01bd19 has been approved by joshtriplett

@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 Sep 24, 2020
@bors
Copy link
Contributor

bors commented Sep 26, 2020

⌛ Testing commit d01bd19 with merge 9e1c436...

@bors
Copy link
Contributor

bors commented Sep 26, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: joshtriplett
Pushing 9e1c436 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 26, 2020
@bors bors merged commit 9e1c436 into rust-lang:master Sep 26, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 26, 2020
@poliorcetics poliorcetics deleted the std-thread-unsafe-op-in-unsafe-fn branch September 27, 2020 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeSeulArtichaut LeSeulArtichaut LeSeulArtichaut left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@joshtriplett joshtriplett

Labels
F-unsafe-block-in-unsafe-fn RFC #2585 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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.
9 participants
@poliorcetics @rust-highfive @bors @joshtriplett @RalfJung @Mark-Simulacrum @LeSeulArtichaut @crlf0710 @rustbot