From 91759dbd638ddca9317950ba5b0a9fd6451cc28b Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 3 Jun 2021 13:44:15 +0100 Subject: [PATCH 1/3] tests: Ignore warning about use of volatile from a test for exactly that Signed-off-by: Philip Withnall Helps: #2418 --- glib/tests/atomic.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/glib/tests/atomic.c b/glib/tests/atomic.c index fb8cdcd46..8bc100f4e 100644 --- a/glib/tests/atomic.c +++ b/glib/tests/atomic.c @@ -97,9 +97,12 @@ test_types (void) /* Note that atomic variables should almost certainly not be marked as * `volatile` — see http://isvolatileusefulwiththreads.in/c/. This test exists * to make sure that we don’t warn when built against older third party code. */ +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wincompatible-pointer-types" g_atomic_pointer_set (&vp_str_vol, NULL); res = g_atomic_pointer_compare_and_exchange (&vp_str_vol, NULL, str); g_assert_true (res); +#pragma GCC diagnostic pop g_atomic_pointer_set (&ip, 0); ip2 = g_atomic_pointer_get (&ip); From 11cd751203f32dfdb2a2bc8fa690cbc8bcd99272 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 3 Jun 2021 13:45:43 +0100 Subject: [PATCH 2/3] gbitlock: Drop unnecessary volatile qualifiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This follows on from !1719. It drops volatile qualifiers on internal functions in `gbitlock.c`, and casts volatile public arguments to non-volatile versions to avoid the `g_atomic_*()` macros from propagating the volatile qualifier. We can’t drop the `volatile` qualifier from the public API in `gbitlock.h`, as that would unfortunately be an API break. Update the documentation in `gbitlock` to mention that users of the API should not use `volatile`. See http://c.isvolatileusefulwiththreads.com/ Signed-off-by: Philip Withnall Fixes: #2418 --- glib/gbitlock.c | 96 ++++++++++++++++++++++++++++++------------------- 1 file changed, 59 insertions(+), 37 deletions(-) diff --git a/glib/gbitlock.c b/glib/gbitlock.c index 3aa4b4a43..9e17bde52 100644 --- a/glib/gbitlock.c +++ b/glib/gbitlock.c @@ -76,8 +76,8 @@ static GSList *g_futex_address_list = NULL; * separate process. */ static void -g_futex_wait (const volatile gint *address, - gint value) +g_futex_wait (const gint *address, + gint value) { syscall (__NR_futex, address, (gsize) FUTEX_WAIT_PRIVATE, (gsize) value, NULL); } @@ -94,7 +94,7 @@ g_futex_wait (const volatile gint *address, * thread being woken up. */ static void -g_futex_wake (const volatile gint *address) +g_futex_wake (const gint *address) { syscall (__NR_futex, address, (gsize) FUTEX_WAKE_PRIVATE, (gsize) 1, NULL); } @@ -104,13 +104,13 @@ g_futex_wake (const volatile gint *address) /* emulate futex(2) */ typedef struct { - const volatile gint *address; - gint ref_count; - GCond wait_queue; + const gint *address; + gint ref_count; + GCond wait_queue; } WaitAddress; static WaitAddress * -g_futex_find_address (const volatile gint *address) +g_futex_find_address (const gint *address) { GSList *node; @@ -126,8 +126,8 @@ g_futex_find_address (const volatile gint *address) } static void -g_futex_wait (const volatile gint *address, - gint value) +g_futex_wait (const gint *address, + gint value) { g_mutex_lock (&g_futex_mutex); if G_LIKELY (g_atomic_int_get (address) == value) @@ -159,7 +159,7 @@ g_futex_wait (const volatile gint *address, } static void -g_futex_wake (const volatile gint *address) +g_futex_wake (const gint *address) { WaitAddress *waiter; @@ -177,7 +177,7 @@ g_futex_wake (const volatile gint *address) #endif #define CONTENTION_CLASSES 11 -static volatile gint g_bit_lock_contended[CONTENTION_CLASSES]; +static gint g_bit_lock_contended[CONTENTION_CLASSES]; /* (atomic) */ #if (defined (i386) || defined (__amd64__)) #if G_GNUC_CHECK_VERSION(4, 5) @@ -202,7 +202,8 @@ static volatile gint g_bit_lock_contended[CONTENTION_CLASSES]; * * This function accesses @address atomically. All other accesses to * @address must be atomic in order for this function to work - * reliably. + * reliably. While @address has a `volatile` qualifier, this is a historical + * artifact and the argument passed to it should not be `volatile`. * * Since: 2.24 **/ @@ -210,6 +211,8 @@ void g_bit_lock (volatile gint *address, gint lock_bit) { + gint *address_nonvolatile = (gint *) address; + #ifdef USE_ASM_GOTO retry: __asm__ volatile goto ("lock bts %1, (%0)\n" @@ -225,13 +228,13 @@ g_bit_lock (volatile gint *address, guint mask = 1u << lock_bit; guint v; - v = (guint) g_atomic_int_get (address); + v = (guint) g_atomic_int_get (address_nonvolatile); if (v & mask) { - guint class = ((gsize) address) % G_N_ELEMENTS (g_bit_lock_contended); + guint class = ((gsize) address_nonvolatile) % G_N_ELEMENTS (g_bit_lock_contended); g_atomic_int_add (&g_bit_lock_contended[class], +1); - g_futex_wait (address, v); + g_futex_wait (address_nonvolatile, v); g_atomic_int_add (&g_bit_lock_contended[class], -1); } } @@ -241,14 +244,14 @@ g_bit_lock (volatile gint *address, guint v; retry: - v = g_atomic_int_or (address, mask); + v = g_atomic_int_or (address_nonvolatile, mask); if (v & mask) /* already locked */ { - guint class = ((gsize) address) % G_N_ELEMENTS (g_bit_lock_contended); + guint class = ((gsize) address_nonvolatile) % G_N_ELEMENTS (g_bit_lock_contended); g_atomic_int_add (&g_bit_lock_contended[class], +1); - g_futex_wait (address, v); + g_futex_wait (address_nonvolatile, v); g_atomic_int_add (&g_bit_lock_contended[class], -1); goto retry; @@ -272,7 +275,8 @@ g_bit_lock (volatile gint *address, * * This function accesses @address atomically. All other accesses to * @address must be atomic in order for this function to work - * reliably. + * reliably. While @address has a `volatile` qualifier, this is a historical + * artifact and the argument passed to it should not be `volatile`. * * Returns: %TRUE if the lock was acquired * @@ -294,10 +298,11 @@ g_bit_trylock (volatile gint *address, return result; #else + gint *address_nonvolatile = (gint *) address; guint mask = 1u << lock_bit; guint v; - v = g_atomic_int_or (address, mask); + v = g_atomic_int_or (address_nonvolatile, mask); return ~v & mask; #endif @@ -314,7 +319,8 @@ g_bit_trylock (volatile gint *address, * * This function accesses @address atomically. All other accesses to * @address must be atomic in order for this function to work - * reliably. + * reliably. While @address has a `volatile` qualifier, this is a historical + * artifact and the argument passed to it should not be `volatile`. * * Since: 2.24 **/ @@ -322,6 +328,8 @@ void g_bit_unlock (volatile gint *address, gint lock_bit) { + gint *address_nonvolatile = (gint *) address; + #ifdef USE_ASM_GOTO __asm__ volatile ("lock btr %1, (%0)" : /* no output */ @@ -330,14 +338,14 @@ g_bit_unlock (volatile gint *address, #else guint mask = 1u << lock_bit; - g_atomic_int_and (address, ~mask); + g_atomic_int_and (address_nonvolatile, ~mask); #endif { - guint class = ((gsize) address) % G_N_ELEMENTS (g_bit_lock_contended); + guint class = ((gsize) address_nonvolatile) % G_N_ELEMENTS (g_bit_lock_contended); if (g_atomic_int_get (&g_bit_lock_contended[class])) - g_futex_wake (address); + g_futex_wake (address_nonvolatile); } } @@ -366,10 +374,10 @@ g_bit_unlock (volatile gint *address, * * g_futex_wake (g_futex_int_address (int_address)); */ -static const volatile gint * -g_futex_int_address (const volatile void *address) +static const gint * +g_futex_int_address (const void *address) { - const volatile gint *int_address = address; + const gint *int_address = address; /* this implementation makes these (reasonable) assumptions: */ G_STATIC_ASSERT (G_BYTE_ORDER == G_LITTLE_ENDIAN || @@ -395,12 +403,17 @@ g_futex_int_address (const volatile void *address) * For portability reasons, you may only lock on the bottom 32 bits of * the pointer. * + * While @address has a `volatile` qualifier, this is a historical + * artifact and the argument passed to it should not be `volatile`. + * * Since: 2.30 **/ void (g_pointer_bit_lock) (volatile void *address, gint lock_bit) { + void *address_nonvolatile = (void *) address; + g_return_if_fail (lock_bit < 32); { @@ -416,23 +429,23 @@ void contended: { - volatile gsize *pointer_address = address; + gsize *pointer_address = address_nonvolatile; gsize mask = 1u << lock_bit; gsize v; v = (gsize) g_atomic_pointer_get (pointer_address); if (v & mask) { - guint class = ((gsize) address) % G_N_ELEMENTS (g_bit_lock_contended); + guint class = ((gsize) address_nonvolatile) % G_N_ELEMENTS (g_bit_lock_contended); g_atomic_int_add (&g_bit_lock_contended[class], +1); - g_futex_wait (g_futex_int_address (address), v); + g_futex_wait (g_futex_int_address (address_nonvolatile), v); g_atomic_int_add (&g_bit_lock_contended[class], -1); } } goto retry; #else - volatile gsize *pointer_address = address; + gsize *pointer_address = address_nonvolatile; gsize mask = 1u << lock_bit; gsize v; @@ -441,10 +454,10 @@ void if (v & mask) /* already locked */ { - guint class = ((gsize) address) % G_N_ELEMENTS (g_bit_lock_contended); + guint class = ((gsize) address_nonvolatile) % G_N_ELEMENTS (g_bit_lock_contended); g_atomic_int_add (&g_bit_lock_contended[class], +1); - g_futex_wait (g_futex_int_address (address), (guint) v); + g_futex_wait (g_futex_int_address (address_nonvolatile), (guint) v); g_atomic_int_add (&g_bit_lock_contended[class], -1); goto retry; @@ -464,6 +477,9 @@ void * For portability reasons, you may only lock on the bottom 32 bits of * the pointer. * + * While @address has a `volatile` qualifier, this is a historical + * artifact and the argument passed to it should not be `volatile`. + * * Returns: %TRUE if the lock was acquired * * Since: 2.30 @@ -487,7 +503,8 @@ gboolean return result; #else - volatile gsize *pointer_address = address; + void *address_nonvolatile = (void *) address; + gsize *pointer_address = address_nonvolatile; gsize mask = 1u << lock_bit; gsize v; @@ -511,12 +528,17 @@ gboolean * For portability reasons, you may only lock on the bottom 32 bits of * the pointer. * + * While @address has a `volatile` qualifier, this is a historical + * artifact and the argument passed to it should not be `volatile`. + * * Since: 2.30 **/ void (g_pointer_bit_unlock) (volatile void *address, gint lock_bit) { + void *address_nonvolatile = (void *) address; + g_return_if_fail (lock_bit < 32); { @@ -526,16 +548,16 @@ void : "r" (address), "r" ((gsize) lock_bit) : "cc", "memory"); #else - volatile gsize *pointer_address = address; + gsize *pointer_address = address_nonvolatile; gsize mask = 1u << lock_bit; g_atomic_pointer_and (pointer_address, ~mask); #endif { - guint class = ((gsize) address) % G_N_ELEMENTS (g_bit_lock_contended); + guint class = ((gsize) address_nonvolatile) % G_N_ELEMENTS (g_bit_lock_contended); if (g_atomic_int_get (&g_bit_lock_contended[class])) - g_futex_wake (g_futex_int_address (address)); + g_futex_wake (g_futex_int_address (address_nonvolatile)); } } } From 942501bec7197cd5c5e7a3ceda73025aca0cad81 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 3 Jun 2021 13:49:00 +0100 Subject: [PATCH 3/3] gbitlock: Minor improvement to documentation formatting Signed-off-by: Philip Withnall --- glib/gbitlock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib/gbitlock.c b/glib/gbitlock.c index 9e17bde52..9384d1a44 100644 --- a/glib/gbitlock.c +++ b/glib/gbitlock.c @@ -471,7 +471,7 @@ void * @address: (not nullable): a pointer to a #gpointer-sized value * @lock_bit: a bit value between 0 and 31 * - * This is equivalent to g_bit_trylock, but working on pointers (or + * This is equivalent to g_bit_trylock(), but working on pointers (or * other pointer-sized values). * * For portability reasons, you may only lock on the bottom 32 bits of