-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fixup Atomic*::from_ptr
safety docs
#116762
Conversation
r? @cuviper (rustbot has picked a reviewer for you, use r? to override) |
Thanks! Reading the text, I am also confused by this:
What does it mean to have a happens-before relationship "via the return value"? Happens-before is a partial order on memory operations, it does not go "via values". |
Thanks for doing this directly Waffle, easier than me guessing around |
This is referring to the return value of the function, i.e. always-atomic accesses via the returned |
Oh, it's "happens before with (atomic access via return value)", I see. Is that part necessary? Why can't we just say, any non-atomic and atomic accesses to the memory |
I adapted it from #115719 (comment), but maybe that statement could just be combined with the first point under it? Maybe these docs should just have suggest the default "don't use nonatomically for |
Yeah I agree. Module-level docs seem better.
|
Hm, so the module docs solution would be something like this? //! ## Overlapping accesses
//!
//! blah blah blah bad
//!
//! ## Mixing atomic and non-atomic operations
//!
//! blah blah blah, be careful
/// ## Safety
///
/// * ...
/// * See also documentation on [overlapping accesses][self#Overlapping-accesses] and [mixing atomic and non-atomic operations][self#Mixing-atomic-and-non-atomic-operations], this method does not allow you to violate the rules described there, it's still UB
fn from_pt() |
We should probably have something like ## Memory model for atomic accesses
Rust atomics currently follow the same rules as [C++20 atomics](https://en.cppreference.com/w/cpp/atomic),
specifically atomic_ref. Basically, creating a shared reference to one of the Rust atomic types corresponds to
creating an atomic_ref in C++; the atomic_ref is destroyed when the lifetime of the shared reference ends.
(A Rust atomic type that is exclusively owned or behind a mutable reference does not correspond to an
“atomic object” in C++, since it can be accessed via non-atomic operations.)
(I think all the text that used to follow the above paragraph should probably go above this new
subsection? Except for the part about `Ordering`, that one should go into the model section.)
Since C++ does not support mixing atomic and non-atomic accesses, or non-synchronized
different-sized accesses to the same data, Rust does not support those operations either. (more text here) Turns out "imperfectly overlapping" and "different size" are equivalent due to the strong alignment requirements, so it's probably easier to just talk about mixed-size accesses than trying to talk about imperfect overlap more general. |
@rustbot author |
9ab403c
to
5e4fb31
Compare
This comment has been minimized.
This comment has been minimized.
5e4fb31
to
5956520
Compare
I don't particularly like my wording or the examples, but I also sleepy, so can't come up with anything better. @rustbot review |
library/core/src/sync/atomic.rs
Outdated
/// duration of lifetime `'a`. Most use cases should be able to follow this guideline. | ||
/// * This requirement is also trivially satisfied if all accesses (atomic or not) are done | ||
/// from the same thread. | ||
/// * You must adhere to the [Memory model for atomic accesses]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// * You must adhere to the [Memory model for atomic accesses]. | |
/// * You must adhere to the [Memory model for atomic accesses]. In particular, it is not allowed to | |
/// mix atomic and non-atomic accesses, or atomic accesses of different sizes, without synchronization. |
That's great, thanks! I think I'd leave a very short one-sentence summary of the surprising parts of the memory model at each of these functions, but otherwise LGTM. @rustbot author |
5956520
to
1023845
Compare
Looks good, thanks. :) |
…=RalfJung Fixup `Atomic*::from_ptr` safety docs See rust-lang#115719 (comment) cc `@RalfJung`
Rollup of 6 pull requests Successful merges: - rust-lang#116762 (Fixup `Atomic*::from_ptr` safety docs) - rust-lang#117645 (Extend builtin/auto trait args with error when they have >1 argument) - rust-lang#117694 (Move `BorrowedBuf` and `BorrowedCursor` from `std:io` to `core::io`) - rust-lang#117705 (triagebot.toml: use inclusive language) - rust-lang#117723 (speed up `x clean`) - rust-lang#117724 (Restore rustc shim error message) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#116762 - WaffleLapkin:fixup_fromptr_docs, r=RalfJung Fixup `Atomic*::from_ptr` safety docs See rust-lang#115719 (comment) cc ``@RalfJung``
Update Rust toolchain from nightly-2023-11-09 to nightly-2023-11-10 without any other source changes. This is an automatically generated pull request. If any of the CI checks fail, manual intervention is required. In such a case, review the changes at https://github.com/rust-lang/rust from rust-lang@fdaaaf9 up to rust-lang@0f44eb3. The log for this commit range is: rust-lang@0f44eb32f1 Auto merge of rust-lang#117727 - saethlin:inline-derived-fmt, r=nnethercote rust-lang@eae4135939 Auto merge of rust-lang#117708 - onur-ozkan:x-setup, r=clubby789 rust-lang@4c8862b263 Auto merge of rust-lang#117122 - ferrocene:pa-configure-git-diff, r=albertlarsan68 rust-lang@d32d9238cf Emit #[inline] on derive(Debug) rust-lang@b7583d38b7 Auto merge of rust-lang#117712 - lcnr:expand-coroutine, r=jackh726 rust-lang@488dd9bc73 fmt rust-lang@e7998aa21f Auto merge of rust-lang#117734 - nnethercote:rm-Zstrip, r=davidtwco rust-lang@287ae4db75 Auto merge of rust-lang#117632 - Nilstrieb:icup, r=davidtwco rust-lang@492e57c6ad Auto merge of rust-lang#117736 - TaKO8Ki:rollup-fjrtmlb, r=TaKO8Ki rust-lang@d1e26401bc chore(bootstrap): capitalize {error, warning, info, note} tags rust-lang@42fbf3ebf5 allow users to override the existing configuration during x setup rust-lang@3d6417fc7a check config file before prompts on x setup rust-lang@f5195c52bb Rollup merge of rust-lang#117724 - Kobzol:shim-error-message, r=onur-ozkan rust-lang@e603f4491c Rollup merge of rust-lang#117723 - onur-ozkan:keep-bootstrap-on-x-clean, r=albertlarsan68 rust-lang@6533c62ce7 Rollup merge of rust-lang#117705 - tshepang:patch-2, r=Nilstrieb rust-lang@b4fa5b7004 Rollup merge of rust-lang#117694 - jmillikin:core-io-borrowed-buf, r=m-ou-se rust-lang@4cc549811f Rollup merge of rust-lang#117645 - compiler-errors:auto-trait-subst, r=petrochenkov rust-lang@a1a8d6fe9c Rollup merge of rust-lang#116762 - WaffleLapkin:fixup_fromptr_docs, r=RalfJung rust-lang@d8dbf7ca0e Auto merge of rust-lang#117557 - Zoxc:panic-prio, r=petrochenkov rust-lang@ecc936b155 Remove `-Z strip`. rust-lang@57fb1e643a Auto merge of rust-lang#117454 - shepmaster:github-actions-m1-tests, r=GuillaumeGomez,onur-ozkan rust-lang@341c85648c Move `BorrowedBuf` and `BorrowedCursor` from `std:io` to `core::io` rust-lang@92267c9794 update mir-opt tests rust-lang@992d93f687 rename `BorrowKind::Shallow` to `Fake` rust-lang@a42eca42df generator layout: ignore fake borrows rust-lang@622be2d138 Restore rustc shim error message rust-lang@de0458af97 speed up `x clean` rust-lang@6909992501 Run tests in CI for aarch64-apple-darwin rust-lang@64090536d4 Install tidy for aarch64-apple-darwin rust-lang@469d34b39b Mark Rustdoc test as Linux-only rust-lang@bf360d407e instrument constituent types computation rust-lang@03435e6fdd accept review suggestion rust-lang@769ad29c3e triagebot.toml: use inclusive language rust-lang@102384523a Document how rust atomics work wrt mixed-sized and non-atomic accesses rust-lang@c17d33f1df Extend builtin/auto trait args with error when they have >1 argument rust-lang@580fa0c1a9 rename github_repository to git_repository rust-lang@ffffc2038f Update ICU4X rust-lang@ff1858e2aa Make `FatalErrorMarker` lower priority than other panics rust-lang@4a0873533f update suggest-tests rust-lang@5a562d962e pass the correct args to compiletest rust-lang@545cc830e1 allow configuring the parent GitHub repository Co-authored-by: celinval <[email protected]>
See #115719 (comment)
cc @RalfJung