Hello,
interesting idea!

I am not at all sure how far we should go with such a support, because
if the compiler is claiming to be C11 (which much of them do with
-std=c11) and we don't provide incomplete atomics support, the
application programmer will have difficulties dealing with the cases
and have to do feature tests which are not trivial at all.

The __STD_NO_ATOMICS__ macro is the only feature test macro that is
foreseen by the standard and it leaves no choice: either you have
*all* atomics support (_Atomic qualifier, _Atomic type specifier,
operations on atomic types, atomic type generic functions) or *none*.

I would propose some rules that should be respected by such a header
file:

 - all syntax that this supports should be correct C11 syntax, but it
   may be a restricted version, that is not all C11 might be
   supported. In such a case the compilation should fail.

 - all C11 syntax that was not in C99 and that compiles with this
   header should compile to correct executables with provable atomic
   sematic as defined by C11

 - all produced object code must be ABI compatible with the newer
   versions of the same compiler

Your file doesn't provide support for atomics for the non-basic
types, but which the C11 standard requests? How to deal with them? Do
you plan to implement libatomic.a ? I think that would be a good idea,
because the gcc implementation is bogus and relies on
pthread_mutex_t instead of atomic_flag.

Would we'd have to mimic the different interfaces on a per compiler
base?

Generally, at least for a start, I think it would be a good idea to
separate the support for different compilers, and in particular don't
claim support for a compiler which you didn't even test :)

Having something for earlier versions of clang would already be a good
start.

Some more comments in the text, but I didn't do a complete review,
yet, less did I do tests.

Jens

Am Sonntag, den 09.11.2014, 13:53 +0100 schrieb Joakim Sindholt:
> GCC and Clang ship their own stdatomic.h headers but not for all the
> versions mentioned above. They ship many other standard headers too that
> musl also ships (stdbool/def/int/...).
> 
> In the end musl should be a complete libc with all standard headers and
> this is one of them. It is up to the individual programmer to ensure
> that his or her compiler is properly supported by for example checking
> __STD_NO_ATOMICS__ and __STDC_VERSION__ >= 201112L.

As said above checking these would have to give the assertion that the
platform supports all atomics, library and compiler, which we
shouldn't give lighthearted

> For clang the support is complete for versions >= 3.1. GCC added the
> __atomic functions in 4.7 but not _Atomic until 4.9 so from 4.1 it will
> work correctly so long as you don't use VLAs together with things like
> the ++ operator in arguments and is subject to the same limitations as
> 4.7.

I don't follow here. Could you explain a bit more?

I have the impression that your patch can't work at all with gcc,
since it is missing _Atomic and you need the typedef's with _Atomic in
them. Then, what is the connection with VLA? and the ++ operator?

> From 4.7 it will work as inteded so long as you only use the
> function-like-macros and from 4.9 it will have full support.

I don't think the functionlike macros will work for arbitrary types

> This patch has only been tested with clang. The __typeof__ magic is to
> avoid changes to alltypes. It should be changed before committing this
> to musl. This patch is only for people who want to test themselves.

I would be easier to read if you would provide the changes in
alltypes, as well. Since we are using git, it should not be a problem
to keep all these changes to different files together in one patch.

> This work is released to the Public Domain.
> ---
>  include/stdatomic.h | 287 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 287 insertions(+)
>  create mode 100644 include/stdatomic.h
> 
> diff --git a/include/stdatomic.h b/include/stdatomic.h
> new file mode 100644
> index 0000000..3a61e47
> --- /dev/null
> +++ b/include/stdatomic.h
> @@ -0,0 +1,287 @@
> +#ifndef _STDATOMIC_H
> +#define _STDATOMIC_H
> +
> +#ifndef __has_extension
> +#define __has_extension(x) 0
> +#define __musl_has_extension 1
> +#endif
> +
> +#if defined(__clang__) && __has_extension(c_atomic)
> +
> +#define kill_dependency(y) (y)
> +
> +#define ATOMIC_VAR_INIT(value) (value)
> +#define atomic_init(obj, value) __c11_atomic_init(obj, value)
> +
> +#define atomic_is_lock_free(obj) __c11_atomic_is_lock_free(sizeof(obj))
> +#define __atomic_type_is_lock_free(type) \
> +    __c11_atomic_is_lock_free(sizeof(type))
> +
> +#define atomic_thread_fence(order) __c11_atomic_thread_fence(order)
> +#define atomic_signal_fence(order) __c11_atomic_signal_fence(order)
> +
> +#define atomic_store_explicit(object, desired, order) \
> +    __c11_atomic_store(object, desired, order)
> +#define atomic_load_explicit(object, order) \
> +    __c11_atomic_load(object, order)
> +
> +#define atomic_exchange_explicit(object, value, order) \
> +    __c11_atomic_exchange(object, value, order)
> +#define atomic_compare_exchange_strong_explicit(object, expected, desired, \
> +                                                success, failure) \
> +    __c11_atomic_compare_exchange_strong(object, expected, desired, \
> +                                         success, failure)
> +#define atomic_compare_exchange_weak_explicit(object, expected, desired, \
> +                                              success, failure) \
> +    __c11_atomic_compare_exchange_weak(object, expected, desired, \
> +                                       success, failure)
> +
> +#define atomic_fetch_add_explicit(object, operand, order) \
> +    __c11_atomic_fetch_add(object, operand, order)
> +#define atomic_fetch_sub_explicit(object, operand, order) \
> +    __c11_atomic_fetch_sub(object, operand, order)
> +#define atomic_fetch_or_explicit(object, operand, order) \
> +    __c11_atomic_fetch_or(object, operand, order)
> +#define atomic_fetch_xor_explicit(object, operand, order) \
> +    __c11_atomic_fetch_xor(object, operand, order)
> +#define atomic_fetch_and_explicit(object, operand, order) \
> +    __c11_atomic_fetch_and(object, operand, order)
> +
> +#elif (__GNUC__ == 4 && __GNUC_MINOR__ >= 7) || __GNUC__ > 4
> +
> +#define ATOMIC_VAR_INIT(value) (value)
> +#define atomic_init(obj, value) do { *(obj) = (value); } while (0)
> +
> +#if (__GNUC__ == 4 && __GNUC_MINOR__ >= 9) || __GNUC__ > 4
> +#define kill_dependency(y) __extension__({__auto_type __y = (y); __y;})
> +
> +#define atomic_is_lock_free(obj) \
> +    __extension__({ \
> +        __auto_type __obj = (obj); \
> +        __atomic_is_lock_free(sizeof(*__obj), __obj); \
> +    })
> +#else
> +#define __NEED__Atomic
> +
> +#define kill_dependency(y) (y)
> +
> +#define atomic_is_lock_free(obj) \
> +    __atomic_is_lock_free(sizeof(*(obj)), (void *)0)
> +#endif
> +
> +#define __atomic_type_is_lock_free(type) \
> +    __atomic_always_lock_free(sizeof(type), (void *)0)
> +
> +#define atomic_thread_fence(order) __atomic_thread_fence(order)
> +#define atomic_signal_fence(order) __atomic_signal_fence(order)
> +
> +#define atomic_store_explicit(object, value, order) \
> +    __atomic_store_n(object, value, order)
> +#define atomic_load_explicit(object, order) \
> +    __atomic_load_n(object, order)
> +
> +#define atomic_exchange_explicit(object, desired, order) \
> +    __atomic_exchange_n(object, desired, order)
> +#define atomic_compare_exchange_strong_explicit(object, expected, desired, \
> +                                                success, failure) \
> +    __atomic_compare_exchange_n(object, expected, desired, 0, success, failure)
> +#define atomic_compare_exchange_weak_explicit(object, expected, desired, \
> +                                              success, failure) \
> +    __atomic_compare_exchange_n(object, expected, desired, 1, success, failure)
> +
> +#define atomic_fetch_add_explicit(object, operand, order) \
> +    __atomic_fetch_add(object, operand, order)
> +#define atomic_fetch_sub_explicit(object, operand, order) \
> +    __atomic_fetch_sub(object, operand, order)
> +#define atomic_fetch_or_explicit(object, operand, order) \
> +    __atomic_fetch_or(object, operand, order)
> +#define atomic_fetch_xor_explicit(object, operand, order) \
> +    __atomic_fetch_xor(object, operand, order)
> +#define atomic_fetch_and_explicit(object, operand, order) \
> +    __atomic_fetch_and(object, operand, order)
> +
> +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 1
> +
> +#define __NEED__Atomic
> +
> +#define kill_dependency(y) (y)
> +
> +#define ATOMIC_VAR_INIT(value) (value)
> +#define atomic_init(obj, value) do { *(obj) = (value); } while (0)
> +
> +#define atomic_is_lock_free(obj) (sizeof(obj) <= sizeof(void *))
> +#define __atomic_type_is_lock_free(type) (sizeof(type) <= sizeof(void *))
> +
> +#define atomic_thread_fence(order) __sync_synchronize()
> +#define atomic_signal_fence(order) __asm__ volatile ("" : : : "memory")
> +
> +/* for GCC < 4.7 some concessions are made. First off, ordering is ignored and
> + * always treated as a full seq_cst barrier.

ok, this is consistent with the standard

> Second, although these are
> + * described as "generic functions" GCC < 4.7 cannot support uses such as:
> + * int n = 4;
> + * atomic_int arr[n];
> + * atomic_int *i = arr;
> + * if (atomic_compare_exchange_strong(i++, ...)) {...
> + * because of the liberal use of the __typeof__ extension.
> + * Furthermore, GCC specified that test_and_set doesn't necessarily support
> + * arbitrary values. It's used as both exchange and store here. Be careful. */

lock_test_and_set is simply not the right tool for these operations,
this should only be used for atomic_flag

> +#define atomic_store_explicit(object, value, order) \
> +    do { \
> +        __sync_lock_test_and_set(object, value); \
> +        __sync_synchronize(); \
> +    while (0)

A missing "}" ?

> +#define atomic_load_explicit(object, order) \
> +    __atomic_fetch_and_add(object, 0)
> +
> +#define atomic_exchange_explicit(object, desired, order) \
> +    __extension__({ \
> +        __typeof__(*(object)) __ret; \
> +        __ret = __sync_lock_test_and_set(object, desired); \
> +        __sync_synchronize(); \
> +        __ret; \
> +    })
> +#define atomic_compare_exchange_strong_explicit(object, expected, desired, \
> +                                                success, failure) \
> +    __extension__({ \
> +        __typeof__(object) __expected = (expected); \
> +        __typeof__(*__expected) __prev; \
> +        _Bool __ret; \
> +        __prev = __sync_val_compare_and_swap(object, *__expected, desired); \
> +        __ret = (__prev == *__expected); \
> +        *__expected = __prev; \
> +        __ret; \
> +    })
> +#define atomic_compare_exchange_weak_explicit(object, expected, desired, \
> +                                              success, failure) \
> +    atomic_compare_exchange_strong_explicit(object, expected, desired, \
> +                                            success, failure)
> +
> +#define atomic_fetch_add_explicit(object, operand, order) \
> +    __sync_fetch_and_add(object, operand)
> +#define atomic_fetch_sub_explicit(object, operand, order) \
> +    __sync_fetch_and_sub(object, operand)
> +#define atomic_fetch_or_explicit(object, operand, order) \
> +    __sync_fetch_and_or(object, operand)
> +#define atomic_fetch_xor_explicit(object, operand, order) \
> +    __sync_fetch_and_xor(object, operand)
> +#define atomic_fetch_and_explicit(object, operand, order) \
> +    __sync_fetch_and_and(object, operand)
> +
> +#else
> +
> +#error "Musl's stdatomic.h does not support your compiler"
> +
> +#endif
> +
> +#ifdef __musl_has_extension
> +#undef __musl_has_extension
> +#undef __has_extension
> +#endif
> +
> +#ifdef __NEED__Atomic
> +#undef __NEED__Atomic
> +#define _Atomic volatile
> +#define volatile(x) x volatile
> +#endif

argh, redefining volatile, no way

this messes the syntax completely think of contexts where () can
appear after volatile. E.g

int volatile (*toto)[5];

is a valid array declaration. Probably there are other such uses of
volatile that would mess up completely.

I suppose that you want to ensure that your _Atomic macro works with
both, the qualifier version and the type specifier. I don't think that
any of this is possible. This is a language feature, and nothing the
library can deal with.

In P99 I made the choice to just support the specifier variant,
definining _Atomic to be a function like macro. First, this is then
still valid C99.

Then, this avoids to give the false impression to the application
programmer that _Atomic qualifiers with all the operations would be
supported.

E.g with your patch something like

_Atomic unsigned counter = ATOMIC_VAR_INIT(0);

...

if (counter++) {
  ... so something ..
} else {
  ... we are the first ...
}

would compile and pass completely unnoticed, but wouldn't implement
atomic semantics at all.

With P99, the eqivalent line with type specifiers

 Atomic(unsigned) counter = ATOMIC_VAR_INIT(0);

would compile, but the line

if (counter++) {

would fail.

In contrast, with the line

if (atomic_fetch_add(&counter, 1)) {

which also is correct C11 would work.

> +typedef enum {
> +    memory_order_relaxed = 0,
> +    memory_order_consume = 1,
> +    memory_order_acquire = 2,
> +    memory_order_release = 3,
> +    memory_order_acq_rel = 4,
> +    memory_order_seq_cst = 5
> +} memory_order;

I'd prefer to provide the corresponding macros as gcc defines them
with #ifdef and then just use these macros here

> +typedef _Atomic _Bool atomic_bool;
> +typedef _Atomic char atomic_char;
> +typedef _Atomic signed char atomic_schar;
> +typedef _Atomic short atomic_short;
> +typedef _Atomic int atomic_int;
> +typedef _Atomic long atomic_long;
> +typedef _Atomic long long atomic_llong;
> +typedef _Atomic unsigned char atomic_uchar;
> +typedef _Atomic unsigned short atomic_ushort;
> +typedef _Atomic unsigned int atomic_uint;
> +typedef _Atomic unsigned long atomic_ulong;
> +typedef _Atomic unsigned long long atomic_ullong;
> +typedef _Atomic unsigned short atomic_char16_t;
> +typedef _Atomic unsigned atomic_char32_t;
> +typedef _Atomic __typeof__(L'\0') atomic_wchar_t;
> +typedef _Atomic signed char atomic_int_least8_t;
> +typedef _Atomic short atomic_int_least16_t;
> +typedef _Atomic int atomic_int_least32_t;
> +typedef _Atomic __typeof__(0x100000000) atomic_int_least64_t;
> +typedef _Atomic signed char atomic_int_fast8_t;
> +typedef _Atomic int atomic_int_fast16_t;
> +typedef _Atomic int atomic_int_fast32_t;
> +typedef _Atomic __typeof__(0x100000000) atomic_int_fast64_t;
> +typedef _Atomic unsigned char atomic_uint_least8_t;
> +typedef _Atomic unsigned short atomic_uint_least16_t;
> +typedef _Atomic unsigned int atomic_uint_least32_t;
> +typedef _Atomic __typeof__(0x100000000U) atomic_uint_least64_t;
> +typedef _Atomic unsigned char atomic_uint_fast8_t;
> +typedef _Atomic unsigned atomic_uint_fast16_t;
> +typedef _Atomic unsigned atomic_uint_fast32_t;
> +typedef _Atomic __typeof__(0x100000000U) atomic_uint_fast64_t;
> +typedef _Atomic __typeof__((char *)0 - (char *)0) atomic_intptr_t;
> +typedef _Atomic __typeof__(sizeof(0)) atomic_uintptr_t;
> +typedef _Atomic __typeof__(sizeof(0)) atomic_size_t_t;
> +typedef _Atomic __typeof__((char *)0 - (char *)0) atomic_ptrdiff_t;
> +typedef _Atomic __typeof__((char *)0 - (char *)0) atomic_intmax_t;
> +typedef _Atomic __typeof__(sizeof(0)) atomic_uintmax_t;
> +
> +#define ATOMIC_BOOL_LOCK_FREE __atomic_type_is_lock_free(_Bool)
> +#define ATOMIC_CHAR_LOCK_FREE __atomic_type_is_lock_free(char)
> +#define ATOMIC_CHAR16_T_LOCK_FREE __atomic_type_is_lock_free(unsigned short)
> +#define ATOMIC_CHAR32_T_LOCK_FREE __atomic_type_is_lock_free(unsigned)
> +#define ATOMIC_WCHAR_T_LOCK_FREE __atomic_type_is_lock_free(__typeof__(L'\0'))
> +#define ATOMIC_SHORT_LOCK_FREE __atomic_type_is_lock_free(short)
> +#define ATOMIC_INT_LOCK_FREE __atomic_type_is_lock_free(int)
> +#define ATOMIC_LONG_LOCK_FREE __atomic_type_is_lock_free(long)
> +#define ATOMIC_LLONG_LOCK_FREE __atomic_type_is_lock_free(long long)
> +#define ATOMIC_POINTER_LOCK_FREE __atomic_type_is_lock_free(void *)
> +
> +#define atomic_store(object, desired) \
> +    atomic_store_explicit(object, desired, memory_order_seq_cst)
> +#define atomic_load(object) \
> +    atomic_load_explicit(object, memory_order_seq_cst)
> +
> +#define atomic_exchange(object, desired) \
> +    atomic_exchange_explicit(object, desired, memory_order_seq_cst)
> +#define atomic_compare_exchange_strong(object, expected, desired) \
> +    atomic_compare_exchange_strong_explicit(object, expected, desired, \
> +                                            memory_order_seq_cst, \
> +                                            memory_order_seq_cst)
> +#define atomic_compare_exchange_weak(object, expected, desired) \
> +    atomic_compare_exchange_weak_explicit(object, expected, desired, \
> +                                          memory_order_seq_cst, \
> +                                          memory_order_seq_cst)
> +
> +#define atomic_fetch_add(object, operand) \
> +    atomic_fetch_add_explicit(object, operand, memory_order_seq_cst)
> +#define atomic_fetch_sub(object, operand) \
> +    atomic_fetch_sub_explicit(object, operand, memory_order_seq_cst)
> +#define atomic_fetch_or(object, operand) \
> +    atomic_fetch_or_explicit(object, operand, memory_order_seq_cst)
> +#define atomic_fetch_xor(object, operand) \
> +    atomic_fetch_xor_explicit(object, operand, memory_order_seq_cst)
> +#define atomic_fetch_and(object, operand) \
> +    atomic_fetch_and_explicit(object, operand, memory_order_seq_cst)
> +
> +typedef _Atomic _Bool atomic_flag;

that has chances to be wrong base type, probably this would be better

typedef _Atomic int atomic_flag;

at least you'd really have to be sure about the ABI that later
versions of the corresponding compiler suppose

> +#define ATOMIC_FLAG_INIT ATOMIC_VAR_INIT(0)
> +
> +#define atomic_flag_test_and_set(object) \
> +    atomic_exchange(object, 1)
> +#define atomic_flag_test_and_set_explicit(object, order) \
> +    atomic_exchange_explicit(object, 1, order)

here also this should definitively chose the ABI that is provided,
namely the __sync_lock_test_and_set builtins.

Operations on atomic_flag must be guaranteed to be lockfree, and the
__sync_lock builtins are the only ones where gcc seem to give such a
guarantee.

> +#define atomic_flag_clear(object) \
> +    atomic_store(object, 0)
> +#define atomic_flag_clear_explicit(object, order) \
> +    atomic_store_explicit(object, 0, order)
> +
> +#endif


-- 
:: INRIA Nancy Grand Est ::: AlGorille ::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::