#include <stdatomic.h> int *_Atomic p; void f() { atomic_fetch_add(&p, 1); } gives pushq %rbp movq %rsp, %rbp lock addq $1, p(%rip) popq %rbp ret ... which is wrong; gcc should add 4 to p, not 1. C11's atomic_fetch_add seems very difficult to implement with GCC's current set of builtins (you could in principle use _Generic to detect whether you have an atomic integer type). To this end, Clang adds a __c11_atomic_fetch_add builtin which provides the correct C11 semantics (premultiply by sizeof(*x) for an atomic pointer type) of atomic_fetch_add_explicit. Clang's __c11_... builtins also enforce some of the other C11 rules; for instance, only pointers to _Atomic types are permitted, so they may be valuable to add to GCC independent of this bug.
The documentation indeed suggests this: "These built-in functions perform the operation suggested by the name, and return the value that had previously been in @code{*@var{ptr}}. That is, @smallexample @{ tmp = *ptr; *ptr @var{op}= val; return tmp; @} @end smallexample" thus here *(&p) += 1; Whether the bug is in documentation or code is another question. Confirmed that the testcase produces cited assembly on x86_64 and some older trunk.
libstdc++ uses these builtins in bits/atomic_base.h: __pointer_type fetch_add(ptrdiff_t __d, memory_order __m = memory_order_seq_cst) noexcept { return __atomic_fetch_add(&_M_p, _M_type_size(__d), __m); } Naturally, it's up to you guys how you handle this, but it would be problematic at least for Clang if the semantics of __atomic_fetch_add change to match the documentation: we'd somehow need to detect which flavor of libstdc++ is in use when compiling <atomic>. There may also be user code that depends on the current semantics that would be broken by changing the de facto semantics (that have been present for many GCC releases). Here's Clang's documentation for its __c11_* builtin set: http://clang.llvm.org/docs/LanguageExtensions.html#c11-atomic-builtins
The first question is whether this code is actually valid. C11 says "All of these operations are applicable to an object of any atomic integer type.", not mentioning pointer types as valid, but then refers to address types. Then, if it's valid to use pointer types here, something like the following (untested, and all four of the _add and _sub macros would need similar changes) should work without changing the built-in function semantics: /* EXPR1 if it has a pointer type, otherwise EXPR2. */ #define __atomic_ptr_choose(EXPR1, EXPR2) \ __builtin_choose_expr (__builtin_classify_type (EXPR1) == 5, \ (EXPR1), (EXPR2)) /* The size of *EXPR if EXPR has a pointer type, 1 otherwise. */ #define __atomic_ptr_size(EXPR) \ ((__PTRDIFF_TYPE__) \ sizeof (*(__typeof (__atomic_ptr_choose (EXPR, (char *) 0))) 0)) #define atomic_fetch_add(PTR, VAL) \ __extension__ \ ({ \ __auto_type __atomic_fetch_add_ptr = (PTR); \ __atomic_fetch_add (__atomic_fetch_add_ptr, \ (VAL) * __atomic_ptr_size (*__atomic_fetch_add_ptr), \ __ATOMIC_SEQ_CST); \ })
(And this shows an admission from the <stdatomic.h> tests - a new gcc.dg/atomic/stdatomic-op-*.c test should be added that tests _add and _sub for pointer types.)
(In reply to joseph@codesourcery.com from comment #3) > The first question is whether this code is actually valid. To my reading, the C11 standard says that these operations must work on all atomic integer types, must be disallowed on atomic_bool, and it's unspecified if they work on any other type. However, since GCC allows them for an atomic-qualified pointer, the other rules in 7.17.7.5 apply. I'm not at all confident that is the intended reading, though: in the C++ specification, from which the C specification was derived, there are overloads of atomic_fetch_add for each atomic-integral type, plus overloads for atomic<T*>, making it mandatory to handle the pointer case. I think this is merely a bug in C11's spec, and this wording should also allow pointers, but AFAICS it's not in C11's DR list.
(In reply to Richard Smith from comment #2) > libstdc++ uses these builtins in bits/atomic_base.h: > > __pointer_type > fetch_add(ptrdiff_t __d, > memory_order __m = memory_order_seq_cst) noexcept > { return __atomic_fetch_add(&_M_p, _M_type_size(__d), __m); } Ah yes, the _M_type_size function was added to fix PR 51811 so that std::atomic<T*> would work correctly.
See also bug 52291. This never worked correctly and the bug still exists on trunk (6.0). The C11 operations are expected to work on "address types" (which is a term inadvertently held over from the original proposal). There are other problems with the C11 specification and WG14 has started to clean this up. The DR that comes closest to addressing this is DR 486, though I expect it to be superseded by future, more focused papers. $ cat a.c && /build/gcc-trunk/gcc/xgcc -B /build/gcc-trunk/gcc -Wall -Wextra a.c && ./a.out #include <stdatomic.h> int main (void) { int a[2] = { 1, 2 }; { int *p = a; int *q = __sync_fetch_and_add (&p, 2); __builtin_printf ("%p => %p\n", a, p); } { int *p = a; int *q = __atomic_fetch_add (&p, 2, 0); __builtin_printf ("%p => %p\n", a, p); } { int* _Atomic p = a; int *q = atomic_fetch_add (&p, 2); __builtin_printf ("%p => %p\n", a, p); if (p != a + 2) __builtin_abort (); } } a.c: In function ‘main’: a.c:9:14: warning: unused variable ‘q’ [-Wunused-variable] int *q = __sync_fetch_and_add (&p, 2); ^ a.c:16:14: warning: unused variable ‘q’ [-Wunused-variable] int *q = __atomic_fetch_add (&p, 2, 0); ^ a.c:24:14: warning: unused variable ‘q’ [-Wunused-variable] int *q = atomic_fetch_add (&p, 2); ^ 0x3fffc5851208 => 0x3fffc585120a 0x3fffc5851208 => 0x3fffc585120a 0x3fffc5851208 => 0x3fffc585120a Aborted
I think we should keep the built-in function semantics as-is, and fix stdatomic.h along the lines I proposed in comment#3.