[Bug c/64843] New: miscompilation of atomic_fetch_add on atomic pointer type

[Bug c/64843] New: miscompilation of atomic_fetch_add on atomic pointer type

[richard-gccbugzilla at metafoo dot co.uk]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64843

            Bug ID: 64843
           Summary: miscompilation of atomic_fetch_add on atomic pointer
                    type
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: richard-gccbugzilla at metafoo dot co.uk

#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.

[Bug c/64843] miscompilation of atomic_fetch_add on atomic pointer type

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64843

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |documentation, wrong-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-01-28
     Ever confirmed|0                           |1
      Known to fail|                            |5.0

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
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.

[Bug c/64843] miscompilation of atomic_fetch_add on atomic pointer type

[richard-gccbugzilla at metafoo dot co.uk]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64843

--- Comment #2 from Richard Smith <richard-gccbugzilla at metafoo dot co.uk> ---
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

[Bug c/64843] miscompilation of atomic_fetch_add on atomic pointer type

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64843

--- Comment #3 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
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); \
  })

[Bug c/64843] miscompilation of atomic_fetch_add on atomic pointer type

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64843

--- Comment #4 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
(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.)

[Bug c/64843] miscompilation of atomic_fetch_add on atomic pointer type

[richard-gccbugzilla at metafoo dot co.uk]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64843

--- Comment #5 from Richard Smith <richard-gccbugzilla at metafoo dot co.uk> ---
(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.

[Bug c/64843] miscompilation of atomic_fetch_add on atomic pointer type

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64843

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(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.