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

document when atomic loads are guaranteed read-only #115577

Merged
merged 9 commits into from
Oct 17, 2023
Merged

document when atomic loads are guaranteed read-only #115577

Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b5e67a0
document when atomic loads are guaranteed read-only
RalfJung Sep 5, 2023
7453235
add ARM and RISC-V values
RalfJung Sep 5, 2023
07b8c10
add general powerpc64le bound
RalfJung Sep 8, 2023
69b62ec
define 'read-only memory'
RalfJung Sep 8, 2023
275d5c8
wording
RalfJung Sep 9, 2023
9b8686d
only guarantee for Relaxed; add ptr-size fallback
RalfJung Oct 14, 2023
9d8506d
acquire loads can be done as relaxed load; acquire fence
RalfJung Oct 15, 2023
6605116
use target-arch based table
RalfJung Oct 16, 2023
e494df4
remove 128bit atomics, they are anyway not exposed on those targets
RalfJung Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions library/core/src/sync/atomic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,42 @@
//!
//! [lock-free]: https://en.wikipedia.org/wiki/Non-blocking_algorithm
//!
//! # Atomic accesses to read-only memory
//!
//! In general, *all* atomic accesses on read-only memory are Undefined Behavior. For instance, attempting
//! to do a `compare_exchange` that will definitely fail (making it conceptually a read-only
//! operation) can still cause a page fault if the underlying memory page is mapped read-only. Since
//! atomic `load`s might be implemented using compare-exchange operations, even a `load` can fault
//! on read-only memory.
//!
//! For the purpose of this section, "read-only memory" is defined as memory that is read-only in
//! the underlying target, i.e., the pages are mapped with a read-only flag and any attempt to write
//! will cause a page fault. In particular, an `&u128` reference that points to memory that is
//! read-write mapped is *not* considered to point to "read-only memory". In Rust, almost all memory
//! is read-write; the only exceptions are memory created by `const` items or `static` items without
//! interior mutability, and memory that was specifically marked as read-only by the operating
//! system via platform-specific APIs.
//!
//! As an exception from the general rule stated above, "sufficiently small" atomic loads with
//! `Ordering::Relaxed` are implemented in a way that works on read-only memory, and are hence not
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
//! Undefined Behavior. The exact size limit for what makes a load "sufficiently small" varies
//! depending on the target:
//!
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
//! | Target triple prefix (regular expression) | Size limit |
//! |---------------|---------|
//! | `i(3|5|6)86-`, `arm`, `thumb`, `mips(|el)-`, `powerpc-`, `riscv32`, `sparc-` | 4 bytes |
//! | `x86_64-`, `aarch64-`, `loongarch64-`, `mips64(|el)-`, `powerpc64-`, `riscv64` | 8 bytes |
//! | `powerpc64le-` | 16 bytes |
//! | `s390x-` | 16 bytes |
Copy link
Member

@taiki-e taiki-e Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
//! | `i(3|5|6)86-`, `arm`, `thumb`, `mips(|el)-`, `powerpc-`, `riscv32`, `sparc-` | 4 bytes |
//! | `x86_64-`, `aarch64-`, `loongarch64-`, `mips64(|el)-`, `powerpc64-`, `riscv64` | 8 bytes |
//! | `powerpc64le-` | 16 bytes |
//! | `s390x-` | 16 bytes |
//! | `i(3|5|6)86-`, `arm`, `thumb`, `mips(|el)-`, `mipsisa32r6(|el)-`, `powerpc-`, `riscv32`, `sparc-` | 4 bytes |
//! | `x86_64`, `aarch64`, `arm64`, `loongarch64-`, `mips64(|el)-`, `mipsisa64r6(|el)-`, `powerpc64-`, `riscv64`, `sparc(64|v9)-` | 8 bytes |
//! | `powerpc64le-`, `s390x-` | 16 bytes |

Changes in the above suggestion:

  • x86_64- -> x86_64: x86_64h(-apple-darwin) is not x86_64- but is x86_64
  • aarch64- -> aarch64: aarch64_be-(-unknown-linux-gnu(|_ilp32)|-unknown-linux-netbsd) are not aarch64- but is aarch64
  • add arm64: arm64_32(-apple-watchos) is not aarch64- but is aarch64
  • add sparc(64|v9)- (target_arch: sparc64), mipsisa32r6(|el)- (target_arch: mips32r6), mipsisa64r6(|el)- (target_arch: mips64r6)
  • merge powerpc64le- and s390x- into the same row

Since target triples are complex and cannot be handled correctly on the user side without a build script, I recommend using target_arch based table (like #115577 (comment)) if possible.

Copy link
Member Author

@RalfJung RalfJung Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I recommend using target_arch based table (like #115577 (comment)) if possible.

Yeah that's what we had earlier, but powerpc64le- has target_arch = powerpc64 so then we can't tell it apart from powerpc64-.

Would it make sense to use target_cpu? That's a field I can see in the JSON target spec, anyway. But it doesn't seem exposed as a cfg so I guess that's not helpful either...

Copy link
Member

Choose a reason for hiding this comment

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

My recommendation is to guarantee nothing about 128-bit atomics as said in #115577 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

Well I guess we're back to a table that just says "pointer size" is a convoluted way then. 🤷

//!
//! Atomics loads that are larger than this limit as well as atomic loads with ordering other
//! than `Relaxed`, as well as *all* atomic loads on targets not listed in the table, might still be
//! read-only under certain conditions, but that is not a stable guarantee and should not be relied
//! upon.
//!
//! If you need to do an acquire load on read-only memory, you can do a relaxed load followed by an
//! acquire fence instead.
//!
//! # Examples
//!
//! A simple spinlock:
Expand Down
Loading