[libc++] Improve atomic_fetch_(add|sub).*.

[Mordante]

Summary

While looking at the review comments in D103765 there was an oddity in
the tests for the following functions:

• atomic_fetch_add
• atomic_fetch_add_explicit
• atomic_fetch_sub
• atomic_fetch_sub_explicit

Libc++ allows usage of
atomic_fetch_add<int>(atomic<int*>*, atomic<int*>::difference_type);
MSVC and GCC reject this code: https://godbolt.org/z/9d8WzohbE

This makes the atomic fetch(add|sub).* Standard conforming and removes the non-conforming extensions.

Fixes PR47908

[• Quuxplusone]

libcxx/include/atomic

2172–2177

IMO, libc++ is wrong to constrain these functions at all.
https://eel.is/c++draft/atomics.nonmembers
does not mention any constraints. The only requirement on _Tp here is that typename atomic<_Tp>::difference_type must exist.

So, I would simply eliminate the enable_ifs here. Write

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
_Tp atomic_fetch_add(volatile atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op) _NOEXCEPT
{
    return __o->fetch_add(__op);
}

to match the Standard specification exactly.

Compare to atomic_store on line 1908, for example.

[Mordante]

I'll look at implementing the proper constrains for the final patch.
For now I would like to determine whether we want to keep or remove the non-standard extension.
(Not sure whether they're extensions or bugs.)

libcxx/include/atomic

2172–2177

Agreed it would be better to add the constrains in the class.
This seems to be required already. In libc++ you can add to an atomic function pointer.
https://godbolt.org/z/effMvEr8j
This isn't allowed http://eel.is/c++draft/atomics.types.pointer#5

[• Quuxplusone]

libcxx/include/atomic

2196–2212

This is not a compiler extension; I mean,

atomic<int*> a;
atomic_fetch_add(&a, 1);

must keep working no matter what. https://godbolt.org/z/ajdEYPhfK
I suggest that you keep it working — and simplify the implementation — by removing the bogus constraints on line 2187, and then delete lines 2196-2212.
(I'm willing to commandeer, if what I'm saying isn't making sense to you.)

[Mordante]

libcxx/include/atomic

2196–2212

This is not a compiler extension; I mean,

atomic<int*> a;
atomic_fetch_add(&a, 1);

must keep working no matter what. https://godbolt.org/z/ajdEYPhfK

Correct, but that's not an extension. These two are https://godbolt.org/z/rs1oxYEMv

	atomic<int*> a;
	atomic_fetch_add<int>(&a, 1);

	atomic<int(*)()> f;
	atomic_fetch_add(&f, 1);

I suggest that you keep it working — and simplify the implementation — by removing the bogus constraints on line 2187, and then delete lines 2196-2212.
(I'm willing to commandeer, if what I'm saying isn't making sense to you.)

What you say makes sense, but I just want to know whether we want to keep these 2 extensions (bugs?).
There's no need to commandeer, I just want to decide which direction to take before implementing the changes and the tests.

[• Quuxplusone]

libcxx/include/atomic

2196–2212

Ah. Yeah, "obviously" it's OK to remove those usages.

• Using atomic_fetch_add<int> with an explicit template argument violates the general SD-8-esque rules about explicit template arguments, e.g. make_pair<int, int>.

• Incrementing a function pointer doesn't make sense semantically; currently we codegen it to a no-op; it was never supposed to be supported.

Lines 2196-2212 were clearly just trying to implement atomic_fetch_add for pointers (after accidentally/bogusly failing to support pointers in the primary template, due to that bogus constraint that needs removing).

[ldionne]

Just to record this publicly:

The question is do we want to keep the current non-conforming code or do we want to remove it?

Remove them. Almost every time we've implemented an extension, it has ended up biting us. It just keeps biting us time after time. This is not a blanket statement to remove all extensions, but in this case, let's aim to be as close to the spec as possible, period.

Thanks for looking at this.

[Mordante]

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_add.pass.cpp

56

Use an array as @Quuxplusone suggested in D103769.

[Mordante]

Rebased.
Remove the invalid overloads.
Validates the requirement the pointer points to a complete object type.
Use array's in the existing unit tests to avoid UB, as suggested by @Quuxplusone.
Test whether the functions are noexcept, as a small drive-by improvement.
Document the changes in behavior in the release notes.
Updated the patch to fix the issue for all add and sub functions.

[Mordante]

libcxx/include/atomic

1825

I'm not too happy with this way to constrain the function. I'm open to better suggestions.

[• Quuxplusone]

libcxx/include/atomic

1825

I recommend:

_LIBCPP_INLINE_VISIBILITY
_Tp* fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) volatile _NOEXCEPT
    {return __cxx_atomic_fetch_add(&this->__a_, __op, __m);}

This follows the Standard's specification exactly, and also is simple (no weird metaprogramming involved). Also, this is what libstdc++ does. (MSVC essentially handles this via a partial specialization: they conditional_t between _Atomic_pointer and _Atomic_storage depending on whether the pointer is an object pointer type. But we don't need to do that, the standard doesn't do that, and libstdc++ doesn't do that. Let's Keep It Simple.)

If you want to explicitly nod to the "Mandates:" in http://eel.is/c++draft/atomics.types.pointer#5 , then I recommend

_LIBCPP_INLINE_VISIBILITY
_Tp* fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) volatile _NOEXCEPT {
    static_assert(is_object<typename remove_pointer<_Tp>::type>::value, "Must be an object pointer type");
    return __cxx_atomic_fetch_add(&this->__a_, __op, __m);
}

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_sub_explicit.verify.cpp

26 ↗

(On Diff #357800)

.verify.cpp tests are run only on libc++? This test wouldn't pass against libstdc++, for example, because libstdc++ doesn't SFINAE away fetch_sub in these cases. (There'd still be an error; it'd just be a different error, because subtracting from a void* is ill-formed.)

[Mordante]

Thanks for the review!

libcxx/include/atomic

1825

I'll have a look at the other two implementations. However we need to do something else we do arithmetic on a function pointer. Or do you mean that Clang should protect us against that behavior? When I don't disable the function pointer here it allows function pointers.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_sub_explicit.verify.cpp

26 ↗

(On Diff #357800)

Correct these tests are only executed with Clang and libc++. So here it's valid to validate against Clang specific diagnostics.

[Mordante]

libcxx/include/atomic

1825

I did some testing. Clang accepts pointers to functions, so we need an static_assert to guard against these. The other invalid pointers generate a compiler diagnostic.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_sub_explicit.verify.cpp

26 ↗

(On Diff #357800)

I recently learned these still should be in the libcxx instead of the std part of the tests.

[Mordante]

Addresses review comments.

[ldionne]

libcxx/docs/ReleaseNotes.rst

74 ↗

(On Diff #377918)

[• Quuxplusone]

Other than the troublesome release note, this LGTM (AFAIrecall and from a quick refresher).

libcxx/docs/ReleaseNotes.rst

62–63 ↗

(On Diff #377918)

Here and on line 68, I strongly recommend removing the sentence This isn't allowed by the Standard.. I mean, we wouldn't make the change if it weren't allowed, right?
I think what you meant is The old behavior wasn't conforming; the new behavior is (more) conforming. But we don't need to say that either; those who trust their library vendor will assume that the change moves in the right direction (not the wrong direction), and those who don't trust, can just go read the Standard themselves if they want some kind of rationale for why we're suddenly implementing the Standard behavior.

[Mordante]

libcxx/docs/ReleaseNotes.rst

62–63 ↗

(On Diff #377918)

I don't feel it's wrong to mention it. However I'm not strongly attached to the sentence, so I've removed it.

libcxx/include/atomic

struct atomic<_Tp*>
     _Tp* operator=(_Tp* __d) volatile _NOEXCEPT
         {__base::store(__d); return __d;}
     _LIBCPP_INLINE_VISIBILITY
     _Tp* operator=(_Tp* __d) _NOEXCEPT
         {__base::store(__d); return __d;}
 
     _LIBCPP_INLINE_VISIBILITY
     _Tp* fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst)
                                                                         volatile _NOEXCEPT
         {return __cxx_atomic_fetch_add(&this->__a_, __op, __m);}
     _LIBCPP_INLINE_VISIBILITY
     _Tp* fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) _NOEXCEPT
         {return __cxx_atomic_fetch_add(&this->__a_, __op, __m);}
     _LIBCPP_INLINE_VISIBILITY
     _Tp* fetch_sub(ptrdiff_t __op, memory_order __m = memory_order_seq_cst)
                                                                         volatile _NOEXCEPT
         {return __cxx_atomic_fetch_sub(&this->__a_, __op, __m);}

 {
     __o->notify_all();
 }
 
 // atomic_fetch_add
 
 template <class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
-    is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value,
+    (is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value) ||
+  (is_pointer<_Tp>::value && is_object<typename remove_pointer<_Tp>::type>::value && !is_const<_Tp>::value),
     _Tp
 >::type
 atomic_fetch_add(volatile atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op) _NOEXCEPT
 {
     return __o->fetch_add(__op);
 }
 
 template <class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
-    is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value,
+    (is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value) ||
+  (is_pointer<_Tp>::value && is_object<typename remove_pointer<_Tp>::type>::value && !is_const<_Tp>::value),
     _Tp
 >::type
 atomic_fetch_add(atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op) _NOEXCEPT
 {
     return __o->fetch_add(__op);
 }
 
+#ifdef DO_WE_WANT_TO_KEEP_THIS_COMPILER_EXTENSION
 template <class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 _Tp*
 atomic_fetch_add(volatile atomic<_Tp*>* __o, typename atomic<_Tp*>::difference_type __op) _NOEXCEPT
 {
     return __o->fetch_add(__op);
 }
 
 template <class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 _Tp*
 atomic_fetch_add(atomic<_Tp*>* __o, typename atomic<_Tp*>::difference_type __op) _NOEXCEPT
 {
     return __o->fetch_add(__op);
 }
+#endif
 
 // atomic_fetch_add_explicit
 
 template <class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
     is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value,

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_add.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // UNSUPPORTED: libcpp-has-no-threads
 //  ... test crashes clang
 
 // <atomic>
 
-// template <class Integral>
-//     Integral
-//     atomic_fetch_add(volatile atomic<Integral>* obj, Integral op);
-//
-// template <class Integral>
-//     Integral
-//     atomic_fetch_add(atomic<Integral>* obj, Integral op);
-//
 // template <class T>
 //     T*
-//     atomic_fetch_add(volatile atomic<T*>* obj, ptrdiff_t op);
+//     atomic_fetch_add(volatile atomic<T>* obj, typename atomic<T>::difference_type) noexcept;
 //
 // template <class T>
 //     T*
-//     atomic_fetch_add(atomic<T*>* obj, ptrdiff_t op);
+//     atomic_fetch_add(atomic<T*>* obj, typename atomic<T>::difference_type) noexcept;
 
 #include <atomic>
 #include <type_traits>
 #include <cassert>
 
 #include "test_macros.h"
 #include "atomic_helpers.h"
 

 
 template <class T>
 void testp()
 {
     {
         typedef std::atomic<T> A;
         typedef typename std::remove_pointer<T>::type X;
         A t;
         std::atomic_init(&t, T(1*sizeof(X)));
         assert(std::atomic_fetch_add(&t, 2) == T(1*sizeof(X)));
-#ifdef _LIBCPP_VERSION // libc++ is nonconforming
+    /*
+     * This doesn't compile with MSVC or GCC, but works with current Clang.
+     * Do we want to keep this?
+     */
         std::atomic_fetch_add<X>(&t, 0);
-#else
+        assert(t == T(3*sizeof(X)));
+    }
+    {
+        typedef std::atomic<T> A;
+        typedef typename std::remove_pointer<T>::type X;
+        A t;
+        std::atomic_init(&t, T(1*sizeof(X)));
+        assert(std::atomic_fetch_add(&t, 2) == T(1*sizeof(X)));
         std::atomic_fetch_add<T>(&t, 0);
-#endif // _LIBCPP_VERSION
         assert(t == T(3*sizeof(X)));
+    ASSERT_NOEXCEPT(std::atomic_fetch_add(&t, 0));
     }
     {
         typedef std::atomic<T> A;
         typedef typename std::remove_pointer<T>::type X;
         volatile A t;
         std::atomic_init(&t, T(1*sizeof(X)));
         assert(std::atomic_fetch_add(&t, 2) == T(1*sizeof(X)));
-#ifdef _LIBCPP_VERSION // libc++ is nonconforming
-        std::atomic_fetch_add<X>(&t, 0);
-#else
         std::atomic_fetch_add<T>(&t, 0);
-#endif // _LIBCPP_VERSION
         assert(t == T(3*sizeof(X)));
+    ASSERT_NOEXCEPT(std::atomic_fetch_add(&t, 0));
     }
 }
 
 int main(int, char**)
 {
     TestEachIntegralType<TestFn>()();
     testp<int*>();
     testp<const int*>();
 
   return 0;
 }