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 panic docs for atomic load and stores #97590

Conversation

Takashiidobe
Copy link
Contributor

Currently, the docs for loads and stores for Atomic types looks like this:

Load:

pub fn [load](https://doc.rust-lang.org/std/sync/atomic/struct.AtomicUsize.html#method.load)(&self, order: [Ordering](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html)) -> [usize](https://doc.rust-lang.org/std/primitive.usize.html)

Loads a value from the atomic integer.

load takes an [Ordering](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html) argument which describes the memory ordering of this operation. Possible values are [SeqCst](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.SeqCst), [Acquire](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.Acquire) and [Relaxed](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.Relaxed).
[Panics](https://doc.rust-lang.org/std/sync/atomic/struct.AtomicUsize.html#panics)

Panics if order is [Release](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.Release) or [AcqRel](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.AcqRel).

Store:

pub fn [store](https://doc.rust-lang.org/std/sync/atomic/struct.AtomicUsize.html#method.store)(&self, val: [usize](https://doc.rust-lang.org/std/primitive.usize.html), order: [Ordering](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html))

Stores a value into the atomic integer.

store takes an [Ordering](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html) argument which describes the memory ordering of this operation. Possible values are [SeqCst](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.SeqCst), [Release](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.Release) and [Relaxed](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.Relaxed).
[Panics](https://doc.rust-lang.org/std/sync/atomic/struct.AtomicUsize.html#panics-1)

Panics if order is [Acquire](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.Acquire) or [AcqRel](https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.AcqRel).

tl;dr, the docs indicate that storing with an ordering of Acquire or AcqRel panics at runtime, and loading with an ordering of Release or AcqRel panics at runtime.

This hasn't been true since 1.56 landed this patch, making it so incorrect orderings on loads and stores are compile time errors now.
#84039

A minimum example here:

$ cat src/main.rs
use std::sync::atomic::{AtomicBool, AtomicPtr, AtomicUsize, Ordering};

fn main() {
    let atomic_usize = AtomicUsize::default();

    atomic_usize.store(10, Ordering::Acquire);
    atomic_usize.store(20, Ordering::AcqRel);
    atomic_usize.load(Ordering::Release);
    atomic_usize.load(Ordering::AcqRel);

    let atomic_bool = AtomicBool::default();

    atomic_bool.store(true, Ordering::Acquire);
    atomic_bool.store(false, Ordering::AcqRel);
    atomic_bool.load(Ordering::Release);
    atomic_bool.load(Ordering::AcqRel);

    let atomic_ptr = AtomicPtr::default();
    let ten_ptr: *mut i32 = &mut 10;

    atomic_ptr.store(ten_ptr, Ordering::Acquire);
    atomic_ptr.store(ten_ptr, Ordering::AcqRel);
    atomic_ptr.load(Ordering::Release);
    atomic_ptr.load(Ordering::AcqRel);
}
rustc --version
rustc 1.61.0 (fe5b13d68 2022-05-18)
$ cargo run
   Compiling ordering v0.1.0 (/Users/takashi/Desktop/concurrency-problems/ordering)
error: atomic stores cannot have `Acquire` or `AcqRel` ordering
 --> src/main.rs:6:28
  |
6 |     atomic_usize.store(10, Ordering::Acquire);
  |                            ^^^^^^^^^^^^^^^^^
  |
  = note: `#[deny(invalid_atomic_ordering)]` on by default
  = help: consider using ordering modes `Release`, `SeqCst` or `Relaxed`

error: atomic stores cannot have `Acquire` or `AcqRel` ordering
 --> src/main.rs:7:28
  |
7 |     atomic_usize.store(20, Ordering::AcqRel);
  |                            ^^^^^^^^^^^^^^^^
  |
  = help: consider using ordering modes `Release`, `SeqCst` or `Relaxed`

error: atomic loads cannot have `Release` or `AcqRel` ordering
 --> src/main.rs:8:23
  |
8 |     atomic_usize.load(Ordering::Release);
  |                       ^^^^^^^^^^^^^^^^^
  |
  = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`

error: atomic loads cannot have `Release` or `AcqRel` ordering
 --> src/main.rs:9:23
  |
9 |     atomic_usize.load(Ordering::AcqRel);
  |                       ^^^^^^^^^^^^^^^^
  |
  = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`

error: atomic stores cannot have `Acquire` or `AcqRel` ordering
  --> src/main.rs:13:29
   |
13 |     atomic_bool.store(true, Ordering::Acquire);
   |                             ^^^^^^^^^^^^^^^^^
   |
   = help: consider using ordering modes `Release`, `SeqCst` or `Relaxed`

error: atomic stores cannot have `Acquire` or `AcqRel` ordering
  --> src/main.rs:14:30
   |
14 |     atomic_bool.store(false, Ordering::AcqRel);
   |                              ^^^^^^^^^^^^^^^^
   |
   = help: consider using ordering modes `Release`, `SeqCst` or `Relaxed`

error: atomic loads cannot have `Release` or `AcqRel` ordering
  --> src/main.rs:15:22
   |
15 |     atomic_bool.load(Ordering::Release);
   |                      ^^^^^^^^^^^^^^^^^
   |
   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`

error: atomic loads cannot have `Release` or `AcqRel` ordering
  --> src/main.rs:16:22
   |
16 |     atomic_bool.load(Ordering::AcqRel);
   |                      ^^^^^^^^^^^^^^^^
   |
   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`

error: atomic stores cannot have `Acquire` or `AcqRel` ordering
  --> src/main.rs:21:31
   |
21 |     atomic_ptr.store(ten_ptr, Ordering::Acquire);
   |                               ^^^^^^^^^^^^^^^^^
   |
   = help: consider using ordering modes `Release`, `SeqCst` or `Relaxed`

error: atomic stores cannot have `Acquire` or `AcqRel` ordering
  --> src/main.rs:22:31
   |
22 |     atomic_ptr.store(ten_ptr, Ordering::AcqRel);
   |                               ^^^^^^^^^^^^^^^^
   |
   = help: consider using ordering modes `Release`, `SeqCst` or `Relaxed`

error: atomic loads cannot have `Release` or `AcqRel` ordering
  --> src/main.rs:23:21
   |
23 |     atomic_ptr.load(Ordering::Release);
   |                     ^^^^^^^^^^^^^^^^^
   |
   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`

error: atomic loads cannot have `Release` or `AcqRel` ordering
  --> src/main.rs:24:21
   |
24 |     atomic_ptr.load(Ordering::AcqRel);
   |                     ^^^^^^^^^^^^^^^^
   |
   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`

error: could not compile `ordering` due to 12 previous errors

On 1.55, the same program panics, indicating that the docs were correct at that point in time.

$ rustup default 1.55
info: using existing install for '1.55-x86_64-apple-darwin'
info: default toolchain set to '1.55-x86_64-apple-darwin'

  1.55-x86_64-apple-darwin unchanged - rustc 1.55.0 (c8dfcfe04 2021-09-06)
$ rustc --version
rustc 1.55.0 (c8dfcfe04 2021-09-06)
$ cargo run
   Compiling ordering v0.1.0 (/Users/takashi/Desktop/concurrency-problems/ordering)
    Finished dev [unoptimized + debuginfo] target(s) in 1.58s
     Running `target/debug/ordering`
thread 'main' panicked at 'there is no such thing as an acquire store', /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/sync/atomic.rs:2342:24
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 31, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 May 31, 2022
@rust-log-analyzer

This comment has been minimized.

@luqmana
Copy link
Member

luqmana commented May 31, 2022

rustc will emit errors at compile time if possible but runtime panics are still possible:

Acquire => panic!("there is no such thing as an acquire store"),
AcqRel => panic!("there is no such thing as an acquire/release store"),

Release => panic!("there is no such thing as a release load"),
AcqRel => panic!("there is no such thing as an acquire/release load"),

@Takashiidobe
Copy link
Contributor Author

rustc will emit errors at compile time if possible but runtime panics are still possible:

Acquire => panic!("there is no such thing as an acquire store"),
AcqRel => panic!("there is no such thing as an acquire/release store"),

Release => panic!("there is no such thing as a release load"),
AcqRel => panic!("there is no such thing as an acquire/release load"),

Makes sense, that closes out this PR. I find it odd that the compiler gives you an error if you explicitly provide the wrong ordering as an Enum but not when the wrong ordering is provided as a variable.

If I replace o (which is Ordering::Release) with Ordering::Release the program fails to compile, instead of panicking at runtime: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8b8f74fa5554d2691d9d60bb67e9df37

It would be nice to have the compiler catch that case too, but I see that #[deny(invalid_atomic_ordering)] can be removed so stores and loads should always panic at runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

6 participants