[libc++] Deprecate the C++20 synchronization library before C++20 #86410

ldionne · 2024-03-23T20:04:35Z

When we initially implemented the C++20 synchronization library, we reluctantly accepted for the implementation to be backported to C++03 upon request from the person who provided the patch. This was when we were only starting to have experience with the issues this can create, so we flinched. Nowadays, we have a much stricter stance about not backporting features to previous standards.

We have recently started fixing several bugs (and near bugs) in our implementation of the synchronization library. A recurring theme during these reviews has been how difficult to understand the current code is, and upon inspection it becomes clear that being able to use a few recent C++ features (in particular lambdas) would help a great deal. The code would still be pretty intricate, but it would be a lot easier to reason about the flow of callbacks through things like __thread_poll_with_backoff.

As a result, this patch deprecates support for the synchronization library before C++20. In the next release, we can remove that support entirely.

When we initially implemented the C++20 synchronization library, we reluctantly accepted for the implementation to be backported to C++03 upon request from the person who provided the patch. This was when we were only starting to have experience with the issues this can create, so we flinched. Nowadays, we have a much stricter stance about not backporting features to previous standards. We have recently started fixing several bugs (and near bugs) in our implementation of the synchronization library. A recurring theme during these reviews has been how difficult to understand the current code is, and upon inspection it becomes clear that being able to use a few recent C++ features (in particular lambdas) would help a great deal. The code would still be pretty intricate, but it would be a lot easier to reason about the flow of callbacks through things like __thread_poll_with_backoff. As a result, this patch deprecates support for the synchronization library before C++20. In the next release, we can remove that support entirely.

llvmbot · 2024-03-23T20:05:08Z

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

When we initially implemented the C++20 synchronization library, we reluctantly accepted for the implementation to be backported to C++03 upon request from the person who provided the patch. This was when we were only starting to have experience with the issues this can create, so we flinched. Nowadays, we have a much stricter stance about not backporting features to previous standards.

We have recently started fixing several bugs (and near bugs) in our implementation of the synchronization library. A recurring theme during these reviews has been how difficult to understand the current code is, and upon inspection it becomes clear that being able to use a few recent C++ features (in particular lambdas) would help a great deal. The code would still be pretty intricate, but it would be a lot easier to reason about the flow of callbacks through things like __thread_poll_with_backoff.

As a result, this patch deprecates support for the synchronization library before C++20. In the next release, we can remove that support entirely.

Full diff: https://github.com/llvm/llvm-project/pull/86410.diff

8 Files Affected:

(modified) libcxx/.clang-format (+1)
(modified) libcxx/docs/ReleaseNotes/19.rst (+4)
(modified) libcxx/include/__atomic/atomic.h (+8-4)
(modified) libcxx/include/__atomic/atomic_flag.h (+10-6)
(modified) libcxx/include/__config (+8)
(modified) libcxx/include/barrier (+1-1)
(modified) libcxx/include/latch (+1-1)
(modified) libcxx/include/semaphore (+1-1)

diff --git a/libcxx/.clang-format b/libcxx/.clang-format
index 39ae1322ffa8a6..c37ab817bca906 100644
--- a/libcxx/.clang-format
+++ b/libcxx/.clang-format
@@ -24,6 +24,7 @@ AttributeMacros: [
                   '_LIBCPP_CONSTEXPR_SINCE_CXX23',
                   '_LIBCPP_CONSTEXPR',
                   '_LIBCPP_CONSTINIT',
+                  '_LIBCPP_DEPRECATED_ATOMIC_SYNC',
                   '_LIBCPP_DEPRECATED_IN_CXX11',
                   '_LIBCPP_DEPRECATED_IN_CXX14',
                   '_LIBCPP_DEPRECATED_IN_CXX17',
diff --git a/libcxx/docs/ReleaseNotes/19.rst b/libcxx/docs/ReleaseNotes/19.rst
index cac42f9c3c3f79..1d9efa08550dc6 100644
--- a/libcxx/docs/ReleaseNotes/19.rst
+++ b/libcxx/docs/ReleaseNotes/19.rst
@@ -55,6 +55,10 @@ Improvements and New Features
 Deprecations and Removals
 -------------------------
 
+- The C++20 synchronization library (``<barrier>``, ``<latch>``, ``atomic::wait``, etc.) has been deprecated
+  in language modes prior to C++20. If you are using these features prior to C++20, please update to ``-std=c++20``.
+  In LLVM 20, the C++20 synchronization library will be removed entirely in language modes prior to C++20.
+
 - TODO: The ``LIBCXX_ENABLE_ASSERTIONS`` CMake variable that was used to enable the safe mode has been deprecated and setting
   it triggers an error; use the ``LIBCXX_HARDENING_MODE`` CMake variable with the value ``extensive`` instead. Similarly,
   the ``_LIBCPP_ENABLE_ASSERTIONS`` macro has been deprecated (setting it to ``1`` still enables the extensive mode in
diff --git a/libcxx/include/__atomic/atomic.h b/libcxx/include/__atomic/atomic.h
index 3dfb6937d0325e..bd3f659c22df01 100644
--- a/libcxx/include/__atomic/atomic.h
+++ b/libcxx/include/__atomic/atomic.h
@@ -462,22 +462,26 @@ atomic_wait_explicit(const atomic<_Tp>* __o, typename atomic<_Tp>::value_type __
 // atomic_notify_one
 
 template <class _Tp>
-_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void atomic_notify_one(volatile atomic<_Tp>* __o) _NOEXCEPT {
+_LIBCPP_DEPRECATED_ATOMIC_SYNC _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void
+atomic_notify_one(volatile atomic<_Tp>* __o) _NOEXCEPT {
   __o->notify_one();
 }
 template <class _Tp>
-_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void atomic_notify_one(atomic<_Tp>* __o) _NOEXCEPT {
+_LIBCPP_DEPRECATED_ATOMIC_SYNC _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void
+atomic_notify_one(atomic<_Tp>* __o) _NOEXCEPT {
   __o->notify_one();
 }
 
 // atomic_notify_all
 
 template <class _Tp>
-_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void atomic_notify_all(volatile atomic<_Tp>* __o) _NOEXCEPT {
+_LIBCPP_DEPRECATED_ATOMIC_SYNC _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void
+atomic_notify_all(volatile atomic<_Tp>* __o) _NOEXCEPT {
   __o->notify_all();
 }
 template <class _Tp>
-_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void atomic_notify_all(atomic<_Tp>* __o) _NOEXCEPT {
+_LIBCPP_DEPRECATED_ATOMIC_SYNC _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void
+atomic_notify_all(atomic<_Tp>* __o) _NOEXCEPT {
   __o->notify_all();
 }
 
diff --git a/libcxx/include/__atomic/atomic_flag.h b/libcxx/include/__atomic/atomic_flag.h
index 084366237c16eb..f063c56cf3051e 100644
--- a/libcxx/include/__atomic/atomic_flag.h
+++ b/libcxx/include/__atomic/atomic_flag.h
@@ -49,22 +49,26 @@ struct atomic_flag {
     __cxx_atomic_store(&__a_, _LIBCPP_ATOMIC_FLAG_TYPE(false), __m);
   }
 
-  _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void wait(bool __v, memory_order __m = memory_order_seq_cst) const
-      volatile _NOEXCEPT {
+  _LIBCPP_DEPRECATED_ATOMIC_SYNC _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void
+  wait(bool __v, memory_order __m = memory_order_seq_cst) const volatile _NOEXCEPT {
     std::__atomic_wait(*this, _LIBCPP_ATOMIC_FLAG_TYPE(__v), __m);
   }
-  _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void
+  _LIBCPP_DEPRECATED_ATOMIC_SYNC _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void
   wait(bool __v, memory_order __m = memory_order_seq_cst) const _NOEXCEPT {
     std::__atomic_wait(*this, _LIBCPP_ATOMIC_FLAG_TYPE(__v), __m);
   }
-  _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void notify_one() volatile _NOEXCEPT {
+  _LIBCPP_DEPRECATED_ATOMIC_SYNC _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void notify_one() volatile _NOEXCEPT {
+    std::__atomic_notify_one(*this);
+  }
+  _LIBCPP_DEPRECATED_ATOMIC_SYNC _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void notify_one() _NOEXCEPT {
     std::__atomic_notify_one(*this);
   }
-  _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void notify_one() _NOEXCEPT { std::__atomic_notify_one(*this); }
   _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void notify_all() volatile _NOEXCEPT {
     std::__atomic_notify_all(*this);
   }
-  _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void notify_all() _NOEXCEPT { std::__atomic_notify_all(*this); }
+  _LIBCPP_DEPRECATED_ATOMIC_SYNC _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void notify_all() _NOEXCEPT {
+    std::__atomic_notify_all(*this);
+  }
 
 #if _LIBCPP_STD_VER >= 20
   _LIBCPP_HIDE_FROM_ABI constexpr atomic_flag() _NOEXCEPT : __a_(false) {}
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 132cace3d5b2c4..27c653838ba63a 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -954,6 +954,14 @@ typedef __char32_t char32_t;
 #    define _LIBCPP_DEPRECATED_(m)
 #  endif
 
+#  if _LIBCPP_STD_VER < 20
+#    define _LIBCPP_DEPRECATED_ATOMIC_SYNC                                                                             \
+      _LIBCPP_DEPRECATED_("The C++20 synchronization library has been deprecated prior to C++20. Please update to "    \
+                          "using -std=c++20 if you need to use these facilities")
+#  else
+#    define _LIBCPP_DEPRECATED_ATOMIC_SYNC /* nothing */
+#  endif
+
 #  if !defined(_LIBCPP_CXX03_LANG)
 #    define _LIBCPP_DEPRECATED_IN_CXX11 _LIBCPP_DEPRECATED
 #  else
diff --git a/libcxx/include/barrier b/libcxx/include/barrier
index c5fd84b91925b1..d776078267625a 100644
--- a/libcxx/include/barrier
+++ b/libcxx/include/barrier
@@ -257,7 +257,7 @@ public:
 #  endif // !_LIBCPP_HAS_NO_TREE_BARRIER
 
 template <class _CompletionF = __empty_completion>
-class barrier {
+class _LIBCPP_DEPRECATED_ATOMIC_SYNC barrier {
   __barrier_base<_CompletionF> __b_;
 
 public:
diff --git a/libcxx/include/latch b/libcxx/include/latch
index 3cc72583811434..1937617f7dcc61 100644
--- a/libcxx/include/latch
+++ b/libcxx/include/latch
@@ -66,7 +66,7 @@ _LIBCPP_PUSH_MACROS
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-class latch {
+class _LIBCPP_DEPRECATED_ATOMIC_SYNC latch {
   __atomic_base<ptrdiff_t> __a_;
 
 public:
diff --git a/libcxx/include/semaphore b/libcxx/include/semaphore
index 1375ec3f7c04b1..0b1bb9525d5f0c 100644
--- a/libcxx/include/semaphore
+++ b/libcxx/include/semaphore
@@ -127,7 +127,7 @@ private:
 };
 
 template <ptrdiff_t __least_max_value = _LIBCPP_SEMAPHORE_MAX>
-class counting_semaphore {
+class _LIBCPP_DEPRECATED_ATOMIC_SYNC counting_semaphore {
   __atomic_semaphore_base __semaphore_;
 
 public:

ldionne · 2024-03-23T20:05:35Z

This is the deprecation step before #82008.

Note that this PR is incomplete, I'll have another pass at it.

github-actions · 2024-03-23T20:07:53Z

✅ With the latest revision this PR passed the C/C++ code formatter.

github-actions · 2024-03-23T20:07:53Z

✅ With the latest revision this PR passed the Python code formatter.

mordante

Thanks for the patch! As expected the CI fails; Some tests need to allow deprecated messages. Maybe add a TODO there so we remove it once we remove the deprecated feature.

mordante · 2024-03-25T17:42:53Z

libcxx/docs/ReleaseNotes/19.rst

@@ -55,6 +55,10 @@ Improvements and New Features
 Deprecations and Removals
 -------------------------

+- The C++20 synchronization library (``<barrier>``, ``<latch>``, ``atomic::wait``, etc.) has been deprecated
+  in language modes prior to C++20. If you are using these features prior to C++20, please update to ``-std=c++20``.
+  In LLVM 20, the C++20 synchronization library will be removed entirely in language modes prior to C++20.


Don't we typically deprecate for 2 releases?

I don't think we do? Or at least I don't think we have any sort of fixed policy for doing that?

I'm pretty sure we've deprecated stuff in one release and removed it in the next in some cases, and we might also have kept stuff deprecated for 2 release before removing it. Or is my memory playing tricks on me and we've actually always deprecated for 2 releases before removing? CC @philnik777

I checked and it indeed seems we use a shorter deprecation (nowadays?). So I'm oke to keep this as is.

libcxx/include/__config

EricWF · 2024-03-25T18:50:57Z

I would argue this is a unfortunate use for the deprecation warning, given that in the future the thing is not deprecated, we're just deprecating backwards support for it?

I would also be careful with this because synchronization is one of those things that we may want to use internally prior to C++20.

EricWF · 2024-03-25T18:52:16Z

I actually think this might be a case where providing the extension costs us less than removing it.

ldionne · 2024-04-02T19:02:44Z

I would argue this is a unfortunate use for the deprecation warning, given that in the future the thing is not deprecated, we're just deprecating backwards support for it?

Yes, but the deprecation warning includes a message explaining that it's only the use of it before C++20 that is deprecated, so I think this is not ambiguous.

I would also be careful with this because synchronization is one of those things that we may want to use internally prior to C++20.

In that case, we'd need to create an internal-only API like we often do in other cases. I don't see a problem with that if the need arises. I also think we're unlikely to want to use it for pre-C++20 stuff at this point since we implement almost everything pre-C++20 and we haven't had a need for it so far.

I actually think this might be a case where providing the extension costs us less than removing it.

I have to disagree. The synchronization library is currently really complex and has (or at least had) several subtle bugs. Some of those can be tracked down in part to the difficulty to reason about stuff like __libcpp_thread_poll_with_backoff's flow which would be a lot clearer if we localized e.g. __atomic_wait_poll_impl, but that requires lambdas. This desire to deprecate and remove didn't come out of nowhere, it came out of hours of @huixie90 and I plowing through the implementation trying to understand it and often thinking "wow this would have been a lot easier to understand if we had this or that feature".

ldionne · 2024-04-12T13:23:00Z

Pinging @llvm/libcxx-vendors for awareness.

mordante

Basically LGTM but I prefer to keep the tests working until we remove support.

...td/atomics/atomics.types.operations/atomics.types.operations.wait/atomic_notify_all.pass.cpp

mordante · 2024-04-12T15:43:51Z

libcxx/docs/ReleaseNotes/19.rst

@@ -55,6 +55,10 @@ Improvements and New Features
 Deprecations and Removals
 -------------------------

+- The C++20 synchronization library (``<barrier>``, ``<latch>``, ``atomic::wait``, etc.) has been deprecated
+  in language modes prior to C++20. If you are using these features prior to C++20, please update to ``-std=c++20``.
+  In LLVM 20, the C++20 synchronization library will be removed entirely in language modes prior to C++20.


I checked and it indeed seems we use a shorter deprecation (nowadays?). So I'm oke to keep this as is.

EricWF · 2024-04-12T18:00:57Z

I have to disagree. The synchronization library is currently really complex and has (or at least had) several subtle bugs. Some of those can be tracked down in part to the difficulty to reason about stuff like __libcpp_thread_poll_with_backoff's flow which would be a lot clearer if we localized e.g. __atomic_wait_poll_impl, but that requires lambdas. This desire to deprecate and remove didn't come out of nowhere, it came out of hours of @huixie90 and I plowing through the implementation trying to understand it and often thinking "wow this would have been a lot easier to understand if we had this or that feature".

I'll take your word on that. The surface level complexity of the functions we've applied the deprecation macro to don't immediately make that clear.

EricWF

I have no issue with this. It looks good to me, but I defer to the other reviewers.

ldionne · 2024-04-15T14:57:52Z

I'll take your word on that. The surface level complexity of the functions we've applied the deprecation macro to don't immediately make that clear.

That's right. The surface functions are really simple but the implementation is where it gets more complicated.

ldionne added this to the LLVM 19.X Release milestone Mar 23, 2024

ldionne requested a review from a team as a code owner March 23, 2024 20:04

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 23, 2024

mordante requested changes Mar 25, 2024

View reviewed changes

mordante self-assigned this Mar 25, 2024

ldionne added 2 commits April 2, 2024 15:26

Fix tests

e41c349

UNSUPPORT some tests

1dbea21

mordante approved these changes Apr 12, 2024

View reviewed changes

EricWF approved these changes Apr 12, 2024

View reviewed changes

ldionne added 2 commits April 15, 2024 10:58

Re-enable tests in C++11/14/17

d6f9eb0

Readjust UNSUPPORTED

3c4346d

ldionne merged commit 9ddedf0 into llvm:main Apr 16, 2024
53 checks passed

ldionne deleted the review/deprecate-sync-cxx20 branch April 16, 2024 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Deprecate the C++20 synchronization library before C++20 #86410

[libc++] Deprecate the C++20 synchronization library before C++20 #86410

ldionne commented Mar 23, 2024

llvmbot commented Mar 23, 2024

ldionne commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

mordante left a comment

mordante Mar 25, 2024

ldionne Apr 2, 2024

mordante Apr 12, 2024

EricWF commented Mar 25, 2024

EricWF commented Mar 25, 2024

ldionne commented Apr 2, 2024

ldionne commented Apr 12, 2024

mordante left a comment

mordante Apr 12, 2024

EricWF commented Apr 12, 2024

EricWF left a comment

ldionne commented Apr 15, 2024

[libc++] Deprecate the C++20 synchronization library before C++20 #86410

[libc++] Deprecate the C++20 synchronization library before C++20 #86410

Conversation

ldionne commented Mar 23, 2024

llvmbot commented Mar 23, 2024

ldionne commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

mordante left a comment

Choose a reason for hiding this comment

mordante Mar 25, 2024

Choose a reason for hiding this comment

ldionne Apr 2, 2024

Choose a reason for hiding this comment

mordante Apr 12, 2024

Choose a reason for hiding this comment

EricWF commented Mar 25, 2024

EricWF commented Mar 25, 2024

ldionne commented Apr 2, 2024

ldionne commented Apr 12, 2024

mordante left a comment

Choose a reason for hiding this comment

mordante Apr 12, 2024

Choose a reason for hiding this comment

EricWF commented Apr 12, 2024

EricWF left a comment

Choose a reason for hiding this comment

ldionne commented Apr 15, 2024