gbitlock: Drop unnecessary volatile qualifiers

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 <pwithnall@endlessos.org>

Fixes: #2418

1 files 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));
     }
   }
 }