Date: Mon, 10 Sep 2012 20:41:55 +0200 From: Tijl Coosemans <tijl@coosemans.org> To: freebsd-threads@freebsd.org Subject: review stdatomic.h fixes Message-ID: <504E3473.6010107@coosemans.org>
next in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
--------------enig83B97E350A1AAB484E25EDE0
Content-Type: multipart/mixed; boundary="------------060506060407090809070302"
This is a multi-part message in MIME format.
--------------060506060407090809070302
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: quoted-printable
Hi,
Below is a patch+descriptions for stdatomic.h that I was hoping somebody =
could
review.
> Index: stdatomic.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- stdatomic.h (revision 240316)
> +++ stdatomic.h (working copy)
> @@ -54,9 +54,7 @@
> #define atomic_init(obj, value) __c11_atomic_init(obj, value)
> #else
> #define ATOMIC_VAR_INIT(value) { .__val =3D (value) }
> -#define atomic_init(obj, value) do { \
> - (obj)->__val =3D (value); \
> -} while (0)
> +#define atomic_init(obj, value) ((void)((obj)->__val =3D (value)))
atomic_init() is defined as a (generic) function returning void, so make =
this
a void expression instead of using do-while.
> #endif
> =20
> /*
> @@ -111,8 +109,9 @@
> #define atomic_thread_fence(order) __atomic_thread_fence(order)
> #define atomic_signal_fence(order) __atomic_signal_fence(order)
> #else
> -#define atomic_thread_fence(order) __sync_synchronize()
> -#define atomic_signal_fence(order) __asm volatile ("" : : : "memory")
> +#define atomic_thread_fence(order) ((void)(order), __sync_synchronize(=
))
> +#define atomic_signal_fence(order) \
> + __extension__ ({ (void)(order); __asm volatile ("" ::: "memory"); (vo=
id)0; })
These are defined as ordinary functions returning void so turn these into=
void expressions and evaluate the argument.
Because these are ordinary functions they should also be added to libc,
but that's not part of this patch.
> #endif
> =20
> /*
> @@ -121,13 +120,13 @@
> =20
> #if defined(__CLANG_ATOMICS)
> #define atomic_is_lock_free(obj) \
> - __c11_atomic_is_lock_free(sizeof(obj))
> + ((void)(obj), __c11_atomic_is_lock_free(sizeof(*(obj))))
Evaluate the argument and add a * because obj is the address of an atomic=
variable.
> #elif defined(__GNUC_ATOMICS)
> #define atomic_is_lock_free(obj) \
> - __atomic_is_lock_free(sizeof((obj)->__val))
> + __atomic_is_lock_free(sizeof((obj)->__val), &(obj)->val)
Add a second argument following GCC documentation.
> #else
> #define atomic_is_lock_free(obj) \
> - (sizeof((obj)->__val) <=3D sizeof(void *))
> + ((void)(obj), sizeof((obj)->__val) <=3D sizeof(void *))
Evaluate the argument.
> #endif
> =20
> /*
> @@ -234,13 +233,17 @@
> __atomic_store_n(&(object)->__val, desired, order)
> #else
> #define atomic_compare_exchange_strong_explicit(object, expected, \
> - desired, success, failure) ({ \
> + desired, success, failure) __extension__ ({ \
> __typeof__((object)->__val) __v; \
> + __typeof__(expected) __e; \
> _Bool __r; \
> + __e =3D (expected); \
> + (void)(success); \
> + (void)(failure); \
> __v =3D __sync_val_compare_and_swap(&(object)->__val, \
> - *(expected), desired); \
> - __r =3D *(expected) =3D=3D __v; \
> - *(expected) =3D __v; \
> + *__e, desired); \
> + __r =3D (*__e =3D=3D __v); \
> + *__e =3D __v; \
> __r; \
> })
Evaluate "success" and "failure" and evaluate "expected" only once.
> =20
> @@ -251,18 +254,20 @@
> #if __has_builtin(__sync_swap)
> /* Clang provides a full-barrier atomic exchange - use it if available=
=2E */
> #define atomic_exchange_explicit(object, desired, order) \
> - __sync_swap(&(object)->__val, desired)
> + ((void)(order), __sync_swap(&(object)->__val, desired))
Evaluate argument.
> #else
> /*
> * __sync_lock_test_and_set() is only an acquire barrier in theory (al=
though in
> - * practice it is usually a full barrier) so we need an explicit barri=
er after
> + * practice it is usually a full barrier) so we need an explicit barri=
er before
It's missing a release barrier so I think the extra barrier has to be put=
before not after. A store with a release barrier for instance guarantees
that all previous loads and stores are satisfied before doing the store.
That should give the implementation below acquire-release semantics but
I'm not sure it's sequentially consistent.
> * it.
> */
> -#define atomic_exchange_explicit(object, desired, order) ({ \
> - __typeof__((object)->__val) __v; \
> - __v =3D __sync_lock_test_and_set(&(object)->__val, desired); \
> +#define atomic_exchange_explicit(object, desired, order) \
> +__extension__ ({ \
> + __typeof__(object) __o =3D (object); \
> + __typeof__(desired) __d =3D (desired); \
> + (void)(order); \
> __sync_synchronize(); \
> - __v; \
> + __sync_lock_test_and_set(&(__o)->__val, __d); \
> })
The standard doesn't define what a generic function is, but I assume that=
like an ordinary function it has a sequence point between the evaluation
of the arguments and the function body. So evaluate all arguments before
the barrier.
Also, __sync_lock_test_and_set doesn't perform an exchange on all
architectures so I think this macro should only be defined for
architectures where it's guaranteed to work and undefined otherwise.
> #endif
> #define atomic_fetch_add_explicit(object, operand, order) \
> @@ -277,11 +282,14 @@
> __sync_fetch_and_xor(&(object)->__val, operand)
> #define atomic_load_explicit(object, order) \
> __sync_fetch_and_add(&(object)->__val, 0)
> -#define atomic_store_explicit(object, desired, order) do { \
> +#define atomic_store_explicit(object, desired, order) __extension__ ({=
\
> + __typeof__(object) __o =3D (object); \
> + __typeof__(desired) __d =3D (desired); \
> + (void)(order); \
> __sync_synchronize(); \
> - (object)->__val =3D (desired); \
> + __o->__val =3D __d; \
> __sync_synchronize(); \
> -} while (0)
> +})
> #endif
> =20
> /*
Evaluate all arguments before the barrier and turn this into a void
expression. The assignment isn't guaranteed to be atomic (e.g. 64 bit
values on 32 bit architectures). So as with the exchange operation I
think this macro should only be defined if it's guaranteed to work in
all cases. Atomics are hard enough to get right that we shouldn't have
implementations that can fail sporadically. In my opinion if a
programmer wants to use c11 atomics he should use a c11 compiler, so
there's nothing wrong with leaving some operations undefined for older
compilers.
The implementation of the atomic_flag type and functions needs some work
too. The type is currently defined as atomic_bool, but it should be a
plain structure type. The difference is that atomic_bool can be used in
expressions and atomic_flag cannot.
--------------060506060407090809070302
Content-Type: text/plain; charset=ISO-8859-15;
name="stdatomic.h.patch"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
filename="stdatomic.h.patch"
Index: stdatomic.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- stdatomic.h (revision 240316)
+++ stdatomic.h (working copy)
@@ -54,9 +54,7 @@
#define atomic_init(obj, value) __c11_atomic_init(obj, value)
#else
#define ATOMIC_VAR_INIT(value) { .__val =3D (value) }
-#define atomic_init(obj, value) do { \
- (obj)->__val =3D (value); \
-} while (0)
+#define atomic_init(obj, value) ((void)((obj)->__val =3D (value)))
#endif
=20
/*
@@ -111,8 +109,9 @@
#define atomic_thread_fence(order) __atomic_thread_fence(order)
#define atomic_signal_fence(order) __atomic_signal_fence(order)
#else
-#define atomic_thread_fence(order) __sync_synchronize()
-#define atomic_signal_fence(order) __asm volatile ("" : : : "memory")
+#define atomic_thread_fence(order) ((void)(order), __sync_synchronize())=
+#define atomic_signal_fence(order) \
+ __extension__ ({ (void)(order); __asm volatile ("" ::: "memory"); (void=
)0; })
#endif
=20
/*
@@ -121,13 +120,13 @@
=20
#if defined(__CLANG_ATOMICS)
#define atomic_is_lock_free(obj) \
- __c11_atomic_is_lock_free(sizeof(obj))
+ ((void)(obj), __c11_atomic_is_lock_free(sizeof(*(obj))))
#elif defined(__GNUC_ATOMICS)
#define atomic_is_lock_free(obj) \
- __atomic_is_lock_free(sizeof((obj)->__val))
+ __atomic_is_lock_free(sizeof((obj)->__val), &(obj)->val)
#else
#define atomic_is_lock_free(obj) \
- (sizeof((obj)->__val) <=3D sizeof(void *))
+ ((void)(obj), sizeof((obj)->__val) <=3D sizeof(void *))
#endif
=20
/*
@@ -234,13 +233,17 @@
__atomic_store_n(&(object)->__val, desired, order)
#else
#define atomic_compare_exchange_strong_explicit(object, expected, \
- desired, success, failure) ({ \
+ desired, success, failure) __extension__ ({ \
__typeof__((object)->__val) __v; \
+ __typeof__(expected) __e; \
_Bool __r; \
+ __e =3D (expected); \
+ (void)(success); \
+ (void)(failure); \
__v =3D __sync_val_compare_and_swap(&(object)->__val, \
- *(expected), desired); \
- __r =3D *(expected) =3D=3D __v; \
- *(expected) =3D __v; \
+ *__e, desired); \
+ __r =3D (*__e =3D=3D __v); \
+ *__e =3D __v; \
__r; \
})
=20
@@ -251,18 +254,20 @@
#if __has_builtin(__sync_swap)
/* Clang provides a full-barrier atomic exchange - use it if available. =
*/
#define atomic_exchange_explicit(object, desired, order) \
- __sync_swap(&(object)->__val, desired)
+ ((void)(order), __sync_swap(&(object)->__val, desired))
#else
/*
* __sync_lock_test_and_set() is only an acquire barrier in theory (alth=
ough in
- * practice it is usually a full barrier) so we need an explicit barrier=
after
+ * practice it is usually a full barrier) so we need an explicit barrier=
before
* it.
*/
-#define atomic_exchange_explicit(object, desired, order) ({ \
- __typeof__((object)->__val) __v; \
- __v =3D __sync_lock_test_and_set(&(object)->__val, desired); \
+#define atomic_exchange_explicit(object, desired, order) \
+__extension__ ({ \
+ __typeof__(object) __o =3D (object); \
+ __typeof__(desired) __d =3D (desired); \
+ (void)(order); \
__sync_synchronize(); \
- __v; \
+ __sync_lock_test_and_set(&(__o)->__val, __d); \
})
#endif
#define atomic_fetch_add_explicit(object, operand, order) \
@@ -277,11 +282,14 @@
__sync_fetch_and_xor(&(object)->__val, operand)
#define atomic_load_explicit(object, order) \
__sync_fetch_and_add(&(object)->__val, 0)
-#define atomic_store_explicit(object, desired, order) do { \
+#define atomic_store_explicit(object, desired, order) __extension__ ({ \=
+ __typeof__(object) __o =3D (object); \
+ __typeof__(desired) __d =3D (desired); \
+ (void)(order); \
__sync_synchronize(); \
- (object)->__val =3D (desired); \
+ __o->__val =3D __d; \
__sync_synchronize(); \
-} while (0)
+})
#endif
=20
/*
--------------060506060407090809070302--
--------------enig83B97E350A1AAB484E25EDE0
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iF4EAREIAAYFAlBONHgACgkQfoCS2CCgtitnoAD/UCPkEEiwf5oGO9042MXEIy3s
lfQbpzuCwyyvTXJRUWMA/jm2qIo81f+Rb94ALoJsgHRk/KqSAb3fcpbVC8VbbPWG
=4Yq1
-----END PGP SIGNATURE-----
--------------enig83B97E350A1AAB484E25EDE0--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?504E3473.6010107>