[glib: 1/2] gatomic: Fix false positive with Clang+TSAN
- From: Philip Withnall <pwithnall src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [glib: 1/2] gatomic: Fix false positive with Clang+TSAN
- Date: Sat, 21 Sep 2019 07:56:48 +0000 (UTC)
commit a3d90c0726fad30979225f166935a6982e8a4d94
Author: Peter Wu <peter lekensteyn nl>
Date: Fri Jul 26 00:55:52 2019 +0100
gatomic: Fix false positive with Clang+TSAN
__atomic_load_8 and friends do not exist under clang. Use the generic
__atomic_load variant instead that are documented here:
https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
These have the additional benefit that the exact size of gint (4 bytes)
or gpointer (4 or 8 bytes) no longer have to be checked.
I initially tried `__typeof__(*(atomic)) val;`, but that caused warnings
in Clang (-Wincompatible-pointer-types-discards-qualifiers) when
"atomic" points to a volatile variable. Aside from that, it is
apparently not supported everywhere, see the g_has_typeof macro.
Another reason not to use it are new warnings under Clang, including:
glib/deprecated/gthread-deprecated.c:683:11: warning: incompatible pointer types initializing 'typeof
(*(&mutex->mutex.mutex))' (aka 'union _GMutex *') with an expression of type 'GRecMutex *' (aka 'struct
_GRecMutex *') [-Wincompatible-pointer-types]
g_atomic_pointer_set (&mutex->mutex.mutex, result);
Hence, cast the atomic variable to gint/gpointer pointers, the size was
already statically asserted so the cast should be safe.
The macros use a (hopefully) rare "gaps_temp" name instead of something
like "val" to avoid an issue with GCC builds:
glib/tests/once.c:123:test_once4: assertion failed (val == "foo"): (NULL == "foo")
Closes #1843
glib/gatomic.c | 9 ---------
glib/gatomic.h | 44 +++++++++++---------------------------------
2 files changed, 11 insertions(+), 42 deletions(-)
---
diff --git a/glib/gatomic.c b/glib/gatomic.c
index c52aaad91..8b8c6453d 100644
--- a/glib/gatomic.c
+++ b/glib/gatomic.c
@@ -96,15 +96,6 @@
#if defined (__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)
-#if defined(__ATOMIC_SEQ_CST) && !defined(__clang__)
-/* The implementation used in this code path in gatomic.h assumes
- * 4-byte int */
-G_STATIC_ASSERT (sizeof (gint) == 4);
-
-/* The implementations in gatomic.h assume 4- or 8-byte pointers */
-G_STATIC_ASSERT (sizeof (void *) == 4 || sizeof (void *) == 8);
-#endif
-
/**
* g_atomic_int_get:
* @atomic: a pointer to a #gint or #guint
diff --git a/glib/gatomic.h b/glib/gatomic.h
index 971176eb9..8af201814 100644
--- a/glib/gatomic.h
+++ b/glib/gatomic.h
@@ -85,61 +85,39 @@ G_END_DECLS
#if defined(G_ATOMIC_LOCK_FREE) && defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)
/* We prefer the new C11-style atomic extension of GCC if available */
-#if defined(__ATOMIC_SEQ_CST) && !defined(__clang__)
-
-/* This assumes sizeof(int) is 4: gatomic.c statically
- * asserts that (using G_STATIC_ASSERT at top-level in a header was
- * problematic, see #730932) */
+#if defined(__ATOMIC_SEQ_CST)
#define g_atomic_int_get(atomic) \
(G_GNUC_EXTENSION ({ \
G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gint)); \
+ gint gaig_temp; \
(void) (0 ? *(atomic) ^ *(atomic) : 1); \
- (gint) __atomic_load_4 ((atomic), __ATOMIC_SEQ_CST); \
+ __atomic_load ((gint *)(atomic), &gaig_temp, __ATOMIC_SEQ_CST); \
+ (gint) gaig_temp; \
}))
#define g_atomic_int_set(atomic, newval) \
(G_GNUC_EXTENSION ({ \
G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gint)); \
+ gint gais_temp = (gint) (newval); \
(void) (0 ? *(atomic) ^ (newval) : 1); \
- __atomic_store_4 ((atomic), (newval), __ATOMIC_SEQ_CST); \
+ __atomic_store ((gint *)(atomic), &gais_temp, __ATOMIC_SEQ_CST); \
}))
-#if GLIB_SIZEOF_VOID_P == 8
-
#define g_atomic_pointer_get(atomic) \
(G_GNUC_EXTENSION ({ \
G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gpointer)); \
- guint64 gapg_temp = __atomic_load_8 ((atomic), __ATOMIC_SEQ_CST); \
- (gpointer) gapg_temp; \
+ gpointer gapg_temp; \
+ __atomic_load ((gpointer *)(atomic), &gapg_temp, __ATOMIC_SEQ_CST); \
+ gapg_temp; \
}))
#define g_atomic_pointer_set(atomic, newval) \
(G_GNUC_EXTENSION ({ \
G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gpointer)); \
+ gpointer gaps_temp = (gpointer)(newval); \
(void) (0 ? (gpointer) *(atomic) : NULL); \
- __atomic_store_8 ((atomic), (gsize) (newval), __ATOMIC_SEQ_CST); \
+ __atomic_store ((gpointer *)(atomic), &gaps_temp, __ATOMIC_SEQ_CST); \
}))
-#else /* GLIB_SIZEOF_VOID_P == 8 */
-
-/* This assumes that if sizeof(void *) is not 8, then it is 4:
- * gatomic.c statically asserts that (using G_STATIC_ASSERT
- * at top-level in a header was problematic, see #730932) */
-
-#define g_atomic_pointer_get(atomic) \
- (G_GNUC_EXTENSION ({ \
- G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gpointer)); \
- guint32 gapg_temp = __atomic_load_4 ((atomic), __ATOMIC_SEQ_CST); \
- (gpointer) gapg_temp; \
- }))
-#define g_atomic_pointer_set(atomic, newval) \
- (G_GNUC_EXTENSION ({ \
- G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gpointer)); \
- (void) (0 ? (gpointer) *(atomic) : NULL); \
- __atomic_store_4 ((atomic), (gsize) (newval), __ATOMIC_SEQ_CST); \
- }))
-
-#endif /* GLIB_SIZEOF_VOID_P == 8 */
-
#else /* defined(__ATOMIC_SEQ_CST) */
#define g_atomic_int_get(atomic) \
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]