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

do not reference LLVM for our concurrency memory model #65339

Merged
merged 3 commits into from
Oct 13, 2019
Merged
Changes from 1 commit
Commits
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
52 changes: 27 additions & 25 deletions src/libcore/sync/atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@
//!
//! Each method takes an [`Ordering`] which represents the strength of
//! the memory barrier for that operation. These orderings are the
//! same as [LLVM atomic orderings][1]. For more information see the [nomicon][2].
//! same as the [C++ atomic orderings][1]. For more information see the [nomicon][2].
Copy link
Contributor

Choose a reason for hiding this comment

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

not C11?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK C just copies C++ here. And there were some fixes since C++11.

But indeed the Nomicon says "C11"...

Copy link
Contributor

@Centril Centril Oct 12, 2019

Choose a reason for hiding this comment

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

Yea; I mostly want to have a consistent saying one way or the other. (I don't have a preference re. which place should be updated and you are the field expert. Tho cc @nikomatsakis )

Copy link
Member Author

Choose a reason for hiding this comment

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

(Adding Cc in an edit does not trigger email notifications.)

The C page on memory_order is much less informative, so I'd prefer to link to the C++ page. And that page currently reflects C++20. I am not sure if it is worth trying to pin a page from some older C++ version; AFAIK all the changes have been bugfixes that we certainly want.

I assume @Gankra wrote "C11" as most people just say "C11 concurrency". It's shorter to write than "C/C++ from 2011 and later".

Cc @nikomatsakis @Gankra

Copy link
Member Author

Choose a reason for hiding this comment

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

I could replace "C++" by "C/C++" everywhere, and we could do the same in the Nomicon?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, also see rust-lang/nomicon#168.

Copy link
Contributor

@petrochenkov petrochenkov Oct 12, 2019

Choose a reason for hiding this comment

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

This is a minor thing, but I'm glad to see this fixed, I've been correcting Gankro on this since at least three years ago.

(I suspect the people using "C11 concurrency" are exactly those who read it from Nomicon, because I very rarely see it used outside of Rust community.)

Copy link
Member Author

Choose a reason for hiding this comment

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

@petrochenkov so do you think we should further clarify the comment I added on this in the Nomicon?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not just Gankra though; here's a talk by some of my colleagues also using "C11". I think it is generally understood as a short-hand for "C/C++11" -- but it is indeed somewhat confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, looks ok as is.

//!
//! [`Ordering`]: enum.Ordering.html
//!
//! [1]: https://llvm.org/docs/LangRef.html#memory-model-for-concurrent-operations
//! [1]: https://en.cppreference.com/w/cpp/atomic/memory_order
//! [2]: ../../../nomicon/atomics.html
//!
//! Atomic variables are safe to share between threads (they implement [`Sync`])
Expand Down Expand Up @@ -217,8 +217,8 @@ unsafe impl<T> Sync for AtomicPtr<T> {}
/// operations synchronize other memory while additionally preserving a total order of such
/// operations across all threads.
///
/// Rust's memory orderings are [the same as
/// LLVM's](https://llvm.org/docs/LangRef.html#memory-model-for-concurrent-operations).
/// Rust's memory orderings are [the same as those of
/// C++](https://en.cppreference.com/w/cpp/atomic/memory_order).
///
/// For more information see the [nomicon].
///
Expand All @@ -231,9 +231,9 @@ unsafe impl<T> Sync for AtomicPtr<T> {}
pub enum Ordering {
/// No ordering constraints, only atomic operations.
///
/// Corresponds to LLVM's [`Monotonic`] ordering.
/// Corresponds to [`memory_order_relaxed`] in C++.
///
/// [`Monotonic`]: https://llvm.org/docs/Atomics.html#monotonic
/// [`memory_order_relaxed`]: https://en.cppreference.com/w/cpp/atomic/memory_order#Relaxed_ordering
#[stable(feature = "rust1", since = "1.0.0")]
Relaxed,
/// When coupled with a store, all previous operations become ordered
Expand All @@ -246,11 +246,12 @@ pub enum Ordering {
///
/// This ordering is only applicable for operations that can perform a store.
///
/// Corresponds to LLVM's [`Release`] ordering.
/// Corresponds to [`memory_order_release`] in C++.
///
/// [`Release`]: https://llvm.org/docs/Atomics.html#release
/// [`Acquire`]: https://llvm.org/docs/Atomics.html#acquire
/// [`Relaxed`]: https://llvm.org/docs/Atomics.html#monotonic
/// [`Release`]: #Release
/// [`Acquire`]: #Acquire
/// [`Relaxed`]: #Relaxed
/// [`memory_order_release`]: https://en.cppreference.com/w/cpp/atomic/memory_order#Release-Acquire_ordering
#[stable(feature = "rust1", since = "1.0.0")]
Release,
/// When coupled with a load, if the loaded value was written by a store operation with
Expand All @@ -263,40 +264,41 @@ pub enum Ordering {
///
/// This ordering is only applicable for operations that can perform a load.
///
/// Corresponds to LLVM's [`Acquire`] ordering.
/// Corresponds to [`memory_order_acquire`] in C++.
///
/// [`Acquire`]: https://llvm.org/docs/Atomics.html#acquire
/// [`Release`]: https://llvm.org/docs/Atomics.html#release
/// [`Relaxed`]: https://llvm.org/docs/Atomics.html#monotonic
/// [`Acquire`]: #Acquire
/// [`Release`]: #Release
/// [`Relaxed`]: #Relaxed
/// [`memory_order_acquire`]: https://en.cppreference.com/w/cpp/atomic/memory_order#Release-Acquire_ordering
#[stable(feature = "rust1", since = "1.0.0")]
Acquire,
/// Has the effects of both [`Acquire`] and [`Release`] together:
/// For loads it uses [`Acquire`] ordering. For stores it uses the [`Release`] ordering.
///
/// Notice that in the case of `compare_and_swap`, it is possible that the operation ends up
/// not performing any store and hence it has just [`Acquire`] ordering. However,
/// [`AcqRel`][`AcquireRelease`] will never perform [`Relaxed`] accesses.
/// `AcqRel` will never perform [`Relaxed`] accesses.
///
/// This ordering is only applicable for operations that combine both loads and stores.
///
/// Corresponds to LLVM's [`AcquireRelease`] ordering.
/// Corresponds to [`memory_order_acq_rel`] in C++.
///
/// [`AcquireRelease`]: https://llvm.org/docs/Atomics.html#acquirerelease
/// [`Acquire`]: https://llvm.org/docs/Atomics.html#acquire
/// [`Release`]: https://llvm.org/docs/Atomics.html#release
/// [`Relaxed`]: https://llvm.org/docs/Atomics.html#monotonic
/// [`memory_order_acq_rel`]: https://en.cppreference.com/w/cpp/atomic/memory_order#Release-Acquire_ordering
/// [`Acquire`]: #Acquire
/// [`Release`]: #Release
/// [`Relaxed`]: #Relaxed
#[stable(feature = "rust1", since = "1.0.0")]
AcqRel,
/// Like [`Acquire`]/[`Release`]/[`AcqRel`] (for load, store, and load-with-store
/// operations, respectively) with the additional guarantee that all threads see all
/// sequentially consistent operations in the same order.
///
/// Corresponds to LLVM's [`SequentiallyConsistent`] ordering.
/// Corresponds to [`memory_order_seq_cst`] in C++.
///
/// [`SequentiallyConsistent`]: https://llvm.org/docs/Atomics.html#sequentiallyconsistent
/// [`Acquire`]: https://llvm.org/docs/Atomics.html#acquire
/// [`Release`]: https://llvm.org/docs/Atomics.html#release
/// [`AcqRel`]: https://llvm.org/docs/Atomics.html#acquirerelease
/// [`memory_order_seq_cst`]: https://en.cppreference.com/w/cpp/atomic/memory_order#Sequentially-consistent_ordering
/// [`Acquire`]: #Acquire
/// [`Release`]: #Release
/// [`AcqRel`]: #AcqRel
#[stable(feature = "rust1", since = "1.0.0")]
SeqCst,
}
Expand Down