Document unsafe in libcore #66506
Document unsafe in libcore #66506
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @shepmaster (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
General style nit: would make things more readable if backticks were consistently used when talking about code (expressions, types, etc.). Reassigning to r? @RalfJung but they may not have the time; cc also @rust-lang/wg-unsafe-code-guidelines |
| @@ -405,6 +404,7 @@ impl AtomicBool { | |||
| #[inline] | |||
| #[stable(feature = "rust1", since = "1.0.0")] | |||
| pub fn load(&self, order: Ordering) -> bool { | |||
| // SAFETY: data races are prevented by atomic intrinsics | |||
| unsafe { atomic_load(self.v.get(), order) != 0 } | |||
There was a problem hiding this comment.
We could change the atomic_* intrinsics to take references instead of pointers and make them safe. Afaict the only reason they are unsafe is because they take raw pointers
There was a problem hiding this comment.
There are other reasons. Atomic operation require natural alignment (align=size) which is sometimes more than the "ABI alignment" required by references (e.g., for u64 on some 32 bit targets). There may be more caveats I'm forgetting right now.
There was a problem hiding this comment.
There's also #55005, which basically requires the use of raw pointers for some kinds of concurrent code (the kind that synchronizes to handle deallocation).
There was a problem hiding this comment.
Heh, ok. so alignment and dangliness are what should be mentioned here, not data races
There was a problem hiding this comment.
Data races are also relevant: this must not race with a non-atomic access to the same memory.
There was a problem hiding this comment.
So what would be the best thing to say here? I'm not sure what kind of guarantees we can make about UnsafeCell.
There was a problem hiding this comment.
I created another pull request with a smaller number of changes (including core::sync::atomic). I'm going to close this request, so it might be better to finish this discussion there: #66564
| @@ -405,6 +404,7 @@ impl AtomicBool { | |||
| #[inline] | |||
| #[stable(feature = "rust1", since = "1.0.0")] | |||
| pub fn load(&self, order: Ordering) -> bool { | |||
| // SAFETY: data races are prevented by atomic intrinsics | |||
There was a problem hiding this comment.
I believe the reason this needs an unsafe block is the passing of the raw pointer, and we know that raw pointer is valid because we got it from a reference
There was a problem hiding this comment.
Just noticed this. Would saying something like
SAFETY: any data races are prevented by atomic intrinsics, and the raw pointer passed in is valid because we got it from a reference
be better? Or should we just leave out the first bit about atomics preventing data races?
There was a problem hiding this comment.
Atomic and non-atomic accesses still race with each other, right?
There was a problem hiding this comment.
That's true, but I think in the context of AtomicBool, any accesses are going to be atomic or unique because of mut access, which is why it's safe.
| @@ -2518,6 +2547,7 @@ pub fn fence(order: Ordering) { | |||
| #[inline] | |||
| #[stable(feature = "compiler_fences", since = "1.21.0")] | |||
| pub fn compiler_fence(order: Ordering) { | |||
| // SAFETY: doesn't compile to machine code | |||
| unsafe { | |||
| match order { | |||
| Acquire => intrinsics::atomic_singlethreadfence_acq(), | |||
There was a problem hiding this comment.
are any of these intrinsics unsafe for a reason? Invoking the compiler_fence function is safe, so the intrinsics could be safe
There was a problem hiding this comment.
Found this:
As with any other FFI functions, these are always unsafe to call.
https://doc.rust-lang.org/1.1.0/book/intrinsics.html
The docs are old but I think they're still true. Meaning, I don't think there's a way around it unless you add a wrapper function but I'm not sure that adds anything.
There was a problem hiding this comment.
We have safe intrinsics these days, like size_of.
|
Woah, this is a lot of stuff. To simplify reviewing it would help to split this PR into 3 or 4 independent PRs that cover different parts of libcore -- no need to do it all at once and have all of these separate discussions about unsafe blocks in the same PR. I can't promise when I will get around to look at this in detail, I am afraid. But I saw others already started. <3 |
|
No worries, I'll try breaking this into pieces tonight so it's more manageable. |
…l-str, r=Amanieu
Document unsafe blocks in core::{cell, str, sync}
Split from rust-lang#66506 (issue rust-lang#66219). Hopefully doing a chunk at a time is more manageable!
r? @RalfJung
…l-str, r=Amanieu
Document unsafe blocks in core::{cell, str, sync}
Split from rust-lang#66506 (issue rust-lang#66219). Hopefully doing a chunk at a time is more manageable!
r? @RalfJung
#66219
@oli-obk
@Centril
I finished documenting all of the unsafe blocks in libcore! I'd really like to have this be reviewed for correctness/style before I start documenting libstd. Let me know if I should elaborate on anything, too.