Skip to content

Commit

Permalink
[libc++] Improve atomic_fetch_(add|sub).*.
Browse files Browse the repository at this point in the history
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

Reviewed By: ldionne, #libc

Differential Revision: https://reviews.llvm.org/D103983
  • Loading branch information
mordante committed Oct 8, 2021
1 parent b2ee408 commit aac5b84
Show file tree
Hide file tree
Showing 10 changed files with 409 additions and 182 deletions.
18 changes: 18 additions & 0 deletions libcxx/docs/ReleaseNotes.rst
Expand Up @@ -57,3 +57,21 @@ Build System Changes
- Building the libc++ shared or static library requires a C++ 20 capable compiler.
Use ``-DLLVM_ENABLE_PROJECTS='clang;compiler-rt' -DLLVM_ENABLE_RUNTIMES='libcxx;libcxxabi'``
to build libc++ using a fresh build of Clang.

- The functions ``std::atomic<T*>::fetch_(add|sub)`` and
``std::atomic_fetch_(add|sub)`` no longer accept a function pointer. While
this is technically an API break, the invalid syntax isn't supported by
libstc++ and MSVC STL. See https://godbolt.org/z/49fvzz98d.

This comment has been minimized.

Copy link
@tambry

tambry Oct 8, 2021

Contributor

Should be libstdc++?


- The call of the functions ``std::atomic_(add|sub)(std::atomic<T*>*, ...)``
with the explicit template argument ``T`` are now ill-formed. While this is
technically an API break, the invalid syntax isn't supported by libstc++ and
MSVC STL. See https://godbolt.org/z/v9959re3v.

Due to this change it's now possible to call these functions with the
explicit template argument ``T*``. This allows using the same syntax on the
major Standard library implementations.
See https://godbolt.org/z/oEfzPhTTb.

Calls to these functions where the template argument was deduced by the
compiler are unaffected by this change.
151 changes: 31 additions & 120 deletions libcxx/include/atomic
Expand Up @@ -1844,19 +1844,32 @@ struct atomic<_Tp*>
{__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);}
_Tp* fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) volatile _NOEXCEPT {
// __atomic_fetch_add accepts function pointers, guard against them.
static_assert(!is_function<typename remove_pointer<_Tp>::type>::value, "Pointer to function isn't allowed");
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);}
_Tp* fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) _NOEXCEPT {
// __atomic_fetch_add accepts function pointers, guard against them.
static_assert(!is_function<typename remove_pointer<_Tp>::type>::value, "Pointer to function isn't allowed");
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);}
_Tp* fetch_sub(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) volatile _NOEXCEPT {
// __atomic_fetch_add accepts function pointers, guard against them.
static_assert(!is_function<typename remove_pointer<_Tp>::type>::value, "Pointer to function isn't allowed");
return __cxx_atomic_fetch_sub(&this->__a_, __op, __m);
}

_LIBCPP_INLINE_VISIBILITY
_Tp* fetch_sub(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) _NOEXCEPT
{return __cxx_atomic_fetch_sub(&this->__a_, __op, __m);}
_Tp* fetch_sub(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) _NOEXCEPT {
// __atomic_fetch_add accepts function pointers, guard against them.
static_assert(!is_function<typename remove_pointer<_Tp>::type>::value, "Pointer to function isn't allowed");
return __cxx_atomic_fetch_sub(&this->__a_, __op, __m);
}

_LIBCPP_INLINE_VISIBILITY
_Tp* operator++(int) volatile _NOEXCEPT {return fetch_add(1);}
Expand Down Expand Up @@ -2192,82 +2205,32 @@ void atomic_notify_all(atomic<_Tp>* __o) _NOEXCEPT

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
typename enable_if
<
is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value,
_Tp
>::type
_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
typename enable_if
<
is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value,
_Tp
>::type
_Tp
atomic_fetch_add(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(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);
}

// 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,
_Tp
>::type
atomic_fetch_add_explicit(volatile atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op, memory_order __m) _NOEXCEPT
{
return __o->fetch_add(__op, __m);
}

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
typename enable_if
<
is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value,
_Tp
>::type
atomic_fetch_add_explicit(atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op, memory_order __m) _NOEXCEPT
_Tp atomic_fetch_add_explicit(volatile atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op, memory_order __m) _NOEXCEPT
{
return __o->fetch_add(__op, __m);
}

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

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
_Tp*
atomic_fetch_add_explicit(atomic<_Tp*>* __o, typename atomic<_Tp*>::difference_type __op, memory_order __m) _NOEXCEPT
_Tp atomic_fetch_add_explicit(atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op, memory_order __m) _NOEXCEPT
{
return __o->fetch_add(__op, __m);
}
Expand All @@ -2276,40 +2239,14 @@ atomic_fetch_add_explicit(atomic<_Tp*>* __o, typename atomic<_Tp*>::difference_t

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
typename enable_if
<
is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value,
_Tp
>::type
atomic_fetch_sub(volatile atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op) _NOEXCEPT
{
return __o->fetch_sub(__op);
}

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
typename enable_if
<
is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value,
_Tp
>::type
atomic_fetch_sub(atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op) _NOEXCEPT
{
return __o->fetch_sub(__op);
}

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

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
_Tp*
atomic_fetch_sub(atomic<_Tp*>* __o, typename atomic<_Tp*>::difference_type __op) _NOEXCEPT
_Tp atomic_fetch_sub(atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op) _NOEXCEPT
{
return __o->fetch_sub(__op);
}
Expand All @@ -2318,40 +2255,14 @@ atomic_fetch_sub(atomic<_Tp*>* __o, typename atomic<_Tp*>::difference_type __op)

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
typename enable_if
<
is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value,
_Tp
>::type
atomic_fetch_sub_explicit(volatile atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op, memory_order __m) _NOEXCEPT
{
return __o->fetch_sub(__op, __m);
}

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
typename enable_if
<
is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value,
_Tp
>::type
atomic_fetch_sub_explicit(atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op, memory_order __m) _NOEXCEPT
{
return __o->fetch_sub(__op, __m);
}

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
_Tp*
atomic_fetch_sub_explicit(volatile atomic<_Tp*>* __o, typename atomic<_Tp*>::difference_type __op, memory_order __m) _NOEXCEPT
_Tp atomic_fetch_sub_explicit(volatile atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op, memory_order __m) _NOEXCEPT
{
return __o->fetch_sub(__op, __m);
}

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
_Tp*
atomic_fetch_sub_explicit(atomic<_Tp*>* __o, typename atomic<_Tp*>::difference_type __op, memory_order __m) _NOEXCEPT
_Tp atomic_fetch_sub_explicit(atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op, memory_order __m) _NOEXCEPT
{
return __o->fetch_sub(__op, __m);
}
Expand Down
@@ -0,0 +1,76 @@
//===----------------------------------------------------------------------===//
//
// 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

// <atomic>

// template <class T>
// T* atomic_fetch_add(volatile atomic<T*>* obj, ptrdiff_t op)
// template <class T>
// T* atomic_fetch_add(atomic<T*>* obj, ptrdiff_t op);

#include <atomic>

void void_pointer() {
{
volatile std::atomic<void*> obj;
// expected-error@atomic:* {{incomplete type 'void' where a complete type is required}}
std::atomic_fetch_add(&obj, 0);
}
{
std::atomic<void*> obj;
// expected-error@atomic:* {{incomplete type 'void' where a complete type is required}}
std::atomic_fetch_add(&obj, 0);
}
}

struct Incomplete;

void pointer_to_incomplete_type() {
{
volatile std::atomic<Incomplete*> obj;
// expected-error@atomic:* {{incomplete type 'Incomplete' where a complete type is required}}
std::atomic_fetch_add(&obj, 0);
}
{
std::atomic<Incomplete*> obj;
// expected-error@atomic:* {{incomplete type 'Incomplete' where a complete type is required}}
std::atomic_fetch_add(&obj, 0);
}
}

void function_pointer() {
{
volatile std::atomic<void (*)(int)> fun;
// expected-error@atomic:* {{static_assert failed due to requirement '!is_function<void (int)>::value' "Pointer to function isn't allowed"}}
std::atomic_fetch_add(&fun, 0);
}
{
std::atomic<void (*)(int)> fun;
// expected-error@atomic:* {{static_assert failed due to requirement '!is_function<void (int)>::value' "Pointer to function isn't allowed"}}
std::atomic_fetch_add(&fun, 0);
}
}

struct S {
void fun(int);
};

void member_function_pointer() {
{
volatile std::atomic<void (S::*)(int)> fun;
// expected-error@atomic:* {{no member named 'fetch_add' in}}
std::atomic_fetch_add(&fun, 0);
}
{
std::atomic<void (S::*)(int)> fun;
// expected-error@atomic:* {{no member named 'fetch_add' in}}
std::atomic_fetch_add(&fun, 0);
}
}
@@ -0,0 +1,79 @@
//===----------------------------------------------------------------------===//
//
// 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

// <atomic>

// template <class T>
// T*
// atomic_fetch_add_explicit(volatile atomic<T*>* obj, ptrdiff_t op,
// memory_order m);
// template <class T>
// T*
// atomic_fetch_add_explicit(atomic<T*>* obj, ptrdiff_t op, memory_order m);

#include <atomic>

void void_pointer() {
{
volatile std::atomic<void*> obj;
// expected-error@atomic:* {{incomplete type 'void' where a complete type is required}}
std::atomic_fetch_add_explicit(&obj, 0, std::memory_order_relaxed);
}
{
std::atomic<void*> obj;
// expected-error@atomic:* {{incomplete type 'void' where a complete type is required}}
std::atomic_fetch_add_explicit(&obj, 0, std::memory_order_relaxed);
}
}

struct Incomplete;

void pointer_to_incomplete_type() {
{
volatile std::atomic<Incomplete*> obj;
// expected-error@atomic:* {{incomplete type 'Incomplete' where a complete type is required}}
std::atomic_fetch_add_explicit(&obj, 0, std::memory_order_relaxed);
}
{
std::atomic<Incomplete*> obj;
// expected-error@atomic:* {{incomplete type 'Incomplete' where a complete type is required}}
std::atomic_fetch_add_explicit(&obj, 0, std::memory_order_relaxed);
}
}

void function_pointer() {
{
volatile std::atomic<void (*)(int)> fun;
// expected-error@atomic:* {{static_assert failed due to requirement '!is_function<void (int)>::value' "Pointer to function isn't allowed"}}
std::atomic_fetch_add_explicit(&fun, 0, std::memory_order_relaxed);
}
{
std::atomic<void (*)(int)> fun;
// expected-error@atomic:* {{static_assert failed due to requirement '!is_function<void (int)>::value' "Pointer to function isn't allowed"}}
std::atomic_fetch_add_explicit(&fun, 0, std::memory_order_relaxed);
}
}

struct S {
void fun(int);
};

void member_function_pointer() {
{
volatile std::atomic<void (S::*)(int)> fun;
// expected-error@atomic:* {{no member named 'fetch_add' in}}
std::atomic_fetch_add_explicit(&fun, 0, std::memory_order_relaxed);
}
{
std::atomic<void (S::*)(int)> fun;
// expected-error@atomic:* {{no member named 'fetch_add' in}}
std::atomic_fetch_add_explicit(&fun, 0, std::memory_order_relaxed);
}
}

0 comments on commit aac5b84

Please sign in to comment.