<atomic>, <memory>, <execution>: make sure acquire and release are safe to use and start using them
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 11.1k
- Forks
- 1.7k
- Avg merge
- 4d 15h
- Merged PRs (30d)
- 22
Description
Currently memory_order_acquire and memory_order_release are considered unsafe:
The problem is critical sections overlap in the following situation with mutexes or other synch object:
T1: a.acquire(); a.release(); b.acquire(); b.release(),
T2: b.acquire(); b.release(); a.acquire(); a.release();
Release reorders past subsequent unrelated acquire, so sections overlap and deadlock occurs.
The current resolution is believed to be the following:
- Acquire should observe the release result in a finite time, so release operations cannot be reordered past infinite amount of acquire attempts
- In hardware, memory changes take time to propagate, but relatively a very small time, definitely not infinite time
- In software, the compiler either does not reorder operations at all, or does not reorder them past potentially infinite amount of other operations
Unfortunately, 1 is not what Standard currently says, and 2 and 3 has to be confirmed with compiler vendors
Before the status of acquire / release is clarified, currently seq_cst is used in some places, specifically:
atomic_shared_ptrinternal spinlock:
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/inc/memory#L3130
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/inc/memory#L3150- Non-lock-free atomic
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/inc/atomic#L394-L407 - Parallel algorithms in
<execution>(more than just this occurrence):
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/inc/execution#L3624 memory_resource.cpp
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/memory_resource.cpp#L24
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/memory_resource.cpp#L33
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/memory_resource.cpp#L43
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/memory_resource.cpp#L53filesystem.cpp
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/filesystem.cpp#L36-L50
Some places believed to be not affected by the issue still use acquire / release, specifically:
shared_ptrexternal spinlock:
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/atomic.cpp#L16-L34<system_error>
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/inc/system_error#L590-L597atomic_wait.cpp
https://github.com/microsoft/STL/blob/12c684bba78f9b032050526abdebf14f58ca26a3/stl/src/atomic_wait.cpp#L147-L152
The task is to confirm the situation with compiler team and decide on using memory_order_acquire / memory_order_release in mentioned and possibly unmentioned preexisting code and new code
Note also that memory model implementation on arm may change in the future, see #83 , see also ##488 , #775 , #1082
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reviewing the acquire/release uses in stl/inc/memory, stl/inc/atomic, stl/inc/execution, stl/inc/system_error, and the atomic.cpp, atomic_wait.cpp, memory_resource.cpp, and filesystem.cpp sources. Confirm the C++ memory-model and compiler behavior with the compiler team, then audit the listed and related synchronization sites. Done means the safety question is resolved and the appropriate memory orders are consistently chosen.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers, operating-systems
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100