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

[atomics.order] Memory operations should be definitions #6567

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Eisenwave
Copy link
Contributor

Related: Stack Overflow / Where is the definition of the acquire operation in the C++ 20?

OP in this Q&A was confused by operations involving memory_order::acquire referring to "acquire operation", but this term isn't defined anywhere. It is indeed confusing because "acquire operation" is colloquially used throughout the standard (e.g. [intro.races] p3).

I believe the proper resolution is to tie the definition to the listing of memory orders.

@languagelawyer
Copy link
Contributor

Maybe they should be \defnadj{X}{operation}

@jwakely
Copy link
Member

jwakely commented Sep 19, 2023

I don't think [atomics.order] p1 provides definitions, it says that a store operation with one of those memory orders performs a release operation. That's not the same as saying a release operation is a store operation with one of those memory orders. Unlocking a mutex is a release operation, but it's not a store to an atomic variable using one of those memory orders. [intro.races] specifically says that the library defines atomic operations and operations on mutexes which are synchronization operations, and then says that every synchronization operation is either a consume operation, acquire operation, release operation, or acquire+release operation. If we use [atomics.order] as the definitions, that means that every synchronization operation (including those on mutexes) must be either a store or load of an atomic variable using an appropriate memory_order constant, but that's not how mutexes are defined in the standard.

I agree that we don't have a precise definition of the term "release operation" etc. but I don't think we can just add italics to turn that text into the desired definition.

@Eisenwave
Copy link
Contributor Author

Eisenwave commented Sep 19, 2023

@jwakely [thread.mutex.requirements] doesn't actually mention "release operation" or "acquire operation", but it does say:

The implementation provides lock and unlock operations, as described below.
For purposes of determining the existence of a data race, these behave as atomic operations.

So there isn't any issue with saying that both atomic objects and mutexes use the definition in [atomics.order]. Maybe the wording in [intro.races] needs to be changed as well so it no longer mentions mutexes, or the mention of mutexes needs to be a note. In any case, there doesn't seem to be any problem with turning [atomics.order] p1 into definitions because all uses of the terms "acquire/release operation" can be seen as leading back to it.

@Eisenwave
Copy link
Contributor Author

Furthermore, [intro.races] p3 doesn't look normative to me. It's just an introductory paragraph that lists the contents of various library parts. The entire thing could probably be a note, so it shouldn't make or break whether we can turn something into a definition.

@jwakely
Copy link
Member

jwakely commented Sep 19, 2023

[atomics.order] still talks about load and store operations, which aren't defined for mutexes.

@Eisenwave
Copy link
Contributor Author

Eisenwave commented Sep 19, 2023

[atomics.order] still talks about load and store operations, which aren't defined for mutexes.

I don't think it's a problem with [atomics.order]; I think it's a problem with the wishy washy statement

For purposes of determining the existence of a data race, these behave as atomic operations.

... without any details as to what mutex operation maps onto what sort of atomic operation with what memory order. The solution is probably to elaborate on this and to say that:

  • std::mutex::unlock() behaves like a release operation where the mutex is considered to be an atomic object.
  • std::mutex::lock() behaves like an acquire operation where the mutex is considered to be an atomic object.

If the mutex is considered to be an atomic object, then .store() and .load() are defined within this consideration. This is still a bit vague because it's unclear what value is being stored and loaded, but it's certainly more clear than the wishy washy text we have now.

We're entering CWG territory here though.


On another note, I think the fences in [atomics.fences] p5 need to be given the same \defn treatment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants