Bug 64843 - miscompilation of atomic_fetch_add on atomic pointer type
Summary: miscompilation of atomic_fetch_add on atomic pointer type
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 4.9.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: documentation, wrong-code
Depends on:
Blocks:
 
Reported: 2015-01-28 19:38 UTC by Richard Smith
Modified: 2021-11-29 06:19 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.9.3, 5.0, 6.0
Last reconfirmed: 2015-01-28 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Smith 2015-01-28 19:38:40 UTC
#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.
Comment 1 Richard Biener 2015-01-28 20:38:11 UTC
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.
Comment 2 Richard Smith 2015-01-28 21:44:25 UTC
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
Comment 3 joseph@codesourcery.com 2015-01-28 23:20:31 UTC
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); \
  })
Comment 4 joseph@codesourcery.com 2015-01-28 23:24:04 UTC
(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.)
Comment 5 Richard Smith 2015-01-29 00:04:49 UTC
(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.
Comment 6 Jonathan Wakely 2015-01-29 01:04:33 UTC
(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.
Comment 7 Martin Sebor 2016-01-19 23:56:51 UTC
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
Comment 8 joseph@codesourcery.com 2016-01-20 01:08:39 UTC
I think we should keep the built-in function semantics as-is, and fix 
stdatomic.h along the lines I proposed in comment#3.