atomics documentation for conflicting access uses undefined term "write" leading to ambiguity about data races

[briansmith]

Location

https://doc.rust-lang.org/nightly/core/sync/atomic/index.html

Summary

At https://doc.rust-lang.org/nightly/core/sync/atomic/index.html we have:

A data race is defined as conflicting non-synchronized accesses where at least one of the accesses is non-atomic. Here, accesses are conflicting if they affect overlapping regions of memory and at least one of them is a write. They are non-synchronized if neither of them happens-before the other, according to the happens-before order of the memory model.

I propose we change this to:

A data race is defined as conflicting non-synchronized accesses where at least one of the accesses is non-atomic. Here, accesses are conflicting if they affect overlapping regions of memory and at least one modifies part of the overlapping region. (A compare_exchange or compare_exchange_weak that doesn't succeed does not modify a part of the overlapping region.) They are non-synchronized if neither of them happens-before the other, according to the happens-before order of the memory model.

With the existing wording, the term "write" is not defined and isn't used in the reference C++ spec.

For motivation, consider these two operations happening concurrently in separate threads:

thread 1:

let _ = my_atomic_usize.load(Ordering::Acquire); // Synchronize [1]
// ...
let p = my_atomic_usize.as_ptr(); 
let value = unsafe { p.read() };  // [2]

thread 2:

let _ = my_atomic_usize.compare_exchange(0, 1, Ordering::AcqRel, Ordering::Acquire);

Obviously, if thread 2 succeeds in modifying my_atomic_usize (its value was zero) then its operation must be considered a "write". But, what if it doesn't succeed (the value was non-zero)? Is that compare_exchange still considered a "write"?

The real-world use case is once_cell::OnceNonZeroUsize, a one-time initialize type where a nonzero value indicates that initialization has already been done. When the application has already read a non-zero value ([1] above), it would like to use non-synchronized reads ([2] above) for subsequent loads, because the optimizer can then eliminate redundant loads. We think this is safe but only this presumes that compare_exchange is only considered a write if it succeeds, for the purpose of the quoted statement.

I understand compare_exchange that fails is still considered an "attempt to write" write when it is done on read-only memory, and thus leads to UB, according to a later section in same documentation.

But, "write" and "attempt to write" are not necessarily the same thing.

[briansmith]

The language being discussed here was added in PR #128778 by @RalfJung. Again, I think the main issues are:

• More urgent: clarify whether a compare_exchange that fails should be considered a "write", or is it only considered a "write" if it succeeds, or is it platform-dependent? If there is some reason it could be platform-dependent, then maybe we could investigate documenting the behavior on each platform (incrementally).
• Less urgent: Use defined terminology; avoid "write".

[RalfJung]

I think "write" is fine terminology, we just have to specify whether a failing RMW is a write or not. In Miri, we answered this with "no, not a write". So I think the answer is indeed that a failing compare_exchange is not a write. This is all happening in the C++ memory model; there can't be anything platform-dependent here. Cc @gnzlbg @chorman0773 to be sure.

With the existing wording, the term "write" is not defined and isn't used in the reference C++ spec.

With the newly proposed wording, the term "modifies" is not defined either... (EDIT: it does seem to be the term used in the C++ spec, but I couldn't find a definition there either)

I think "write" is a strictly better term than "modifies". Saying "modifies" introduces extra ambiguity since it may sound like writing the value that is already stored in memory might not "modify" memory, but it unambiguously is a "write".