gclosure: Allow full set of closure flags to be queried atomically

Currently, `GClosure` does atomic writes to the flags in its initial
bitfield, but it never does atomic reads from them. This is not safe —
even if the memory read for an int is atomic, the compiler might
duplicate or rearrange a non-atomic read and give an unexpected result.
Using an atomic fetch inserts the necessary compiler and hardware
barriers to ensure a single result is fetched.

(See !4575 of an example of where this kind of bug has caused
misbehaviour.)

So, introduce an atomic read helper for the `GClosure` bitfield. Rather
than reading a single member of the bitfield, it returns the entire
bitfield, as several `GClosure` methods need access to multiple members
of the bitfield, and atomically fetching them all separately would not
be atomic overall.

Fix various `GClosure` methods to use the new atomic read function.
There are more parts of the code still to fix — these were just the
straightforward ones.

Signed-off-by: Philip Withnall <pwithnall@gnome.org>

gobject/gclosure.c

@@ -101,7 +101,27 @@
                                          (cl)->n_fnotifiers + \
                                          (cl)->n_inotifiers)
 
+/* A copy of the flags bitfield from the beginning of `struct _GClosure`, which
+ * is in union with an int for atomic access to all fields at the same time.
+ *
+ * This must be kept in sync with `struct _GClosure`, but that’s easy because
+ * it’s (unfortunately) a public struct, so can never change for ABI
+ * compatibility reasons. */
 typedef union {
+  struct {
+    guint ref_count : 15;  /* (atomic) */
+    /* meta_marshal is not used anymore but must be zero for historical reasons
+       as it was exposed in the G_CLOSURE_N_NOTIFIERS macro */
+    guint meta_marshal_nouse : 1;  /* (atomic) */
+    guint n_guards : 1;  /* (atomic) */
+    guint n_fnotifiers : 2;  /* finalization notifiers (atomic) */
+    guint n_inotifiers : 8;  /* invalidation notifiers (atomic) */
+    guint in_inotify : 1;  /* (atomic) */
+    guint floating : 1;  /* (atomic) */
+    guint derivative_flag : 1;  /* (atomic) */
+    guint in_marshal : 1;  /* (atomic) */
+    guint is_invalid : 1;  /* (atomic) */
+  } flags;
   GClosure closure;
   int atomic_int;
 } GClosureFlags;
@@ -118,9 +138,9 @@ G_STMT_START {
     {                                                   		                \
       GClosureFlags tmp; \
       tmp.atomic_int = old_int; \
-      _SET_OLD tmp.closure._field; \
+      _SET_OLD tmp.flags._field; \
-      tmp.closure._field _OP _value; \
+      tmp.flags._field _OP _value; \
-      _SET_NEW tmp.closure._field; \
+      _SET_NEW tmp.flags._field; \
       new_int = tmp.atomic_int; \
       success = g_atomic_int_compare_and_exchange_full (&cunion->atomic_int, old_int, new_int, \
 							&old_int);			\
@@ -135,6 +155,14 @@ G_STMT_START {
 #define ATOMIC_DEC(_closure, _field)                   ATOMIC_CHANGE_FIELD (_closure, _field, -=,     1, TRUE,     (void),     (void) )
 #define ATOMIC_DEC_ASSIGN(_closure, _field, _newv)     ATOMIC_CHANGE_FIELD (_closure, _field, -=,     1, TRUE,     (void), *(_newv) = )
 
+static inline GClosureFlags
+closure_atomic_get_flags (GClosure *closure)
+{
+  GClosureFlags tmp;
+  tmp.atomic_int = g_atomic_int_get (&((GClosureFlags *) closure)->atomic_int);
+  return tmp;
+}
+
 enum {
   FNOTIFY,
   INOTIFY,
@@ -308,12 +336,15 @@ static void
 g_closure_set_meta_va_marshal (GClosure       *closure,
 			       GVaClosureMarshal va_meta_marshal)
 {
+  GClosureFlags old_flags;
   GRealClosure *real_closure;
 
   g_return_if_fail (closure != NULL);
   g_return_if_fail (va_meta_marshal != NULL);
-  g_return_if_fail (closure->is_invalid == FALSE);
+
-  g_return_if_fail (closure->in_marshal == FALSE);
+  old_flags = closure_atomic_get_flags (closure);
+  g_return_if_fail (!old_flags.flags.is_invalid);
+  g_return_if_fail (!old_flags.flags.in_marshal);
 
   real_closure = G_REAL_CLOSURE (closure);
 
@@ -352,12 +383,15 @@ g_closure_set_meta_marshal (GClosure       *closure,
 			    gpointer        marshal_data,
 			    GClosureMarshal meta_marshal)
 {
+  GClosureFlags old_flags;
   GRealClosure *real_closure;
 
   g_return_if_fail (closure != NULL);
   g_return_if_fail (meta_marshal != NULL);
-  g_return_if_fail (closure->is_invalid == FALSE);
+
-  g_return_if_fail (closure->in_marshal == FALSE);
+  old_flags = closure_atomic_get_flags (closure);
+  g_return_if_fail (!old_flags.flags.is_invalid);
+  g_return_if_fail (!old_flags.flags.in_marshal);
 
   real_closure = G_REAL_CLOSURE (closure);
 
@@ -391,14 +425,17 @@ g_closure_add_marshal_guards (GClosure      *closure,
 			      gpointer       post_marshal_data,
 			      GClosureNotify post_marshal_notify)
 {
+  GClosureFlags old_flags;
   guint i;
 
   g_return_if_fail (closure != NULL);
   g_return_if_fail (pre_marshal_notify != NULL);
   g_return_if_fail (post_marshal_notify != NULL);
-  g_return_if_fail (closure->is_invalid == FALSE);
+
-  g_return_if_fail (closure->in_marshal == FALSE);
+  old_flags = closure_atomic_get_flags (closure);
-  g_return_if_fail (closure->n_guards < CLOSURE_MAX_N_GUARDS);
+  g_return_if_fail (!old_flags.flags.is_invalid);
+  g_return_if_fail (!old_flags.flags.in_marshal);
+  g_return_if_fail (old_flags.flags.n_guards < CLOSURE_MAX_N_GUARDS);
 
   closure->notifiers = g_renew (GClosureNotifyData, closure->notifiers, CLOSURE_N_NOTIFIERS (closure) + 2);
   if (closure->n_inotifiers)
@@ -482,12 +519,15 @@ g_closure_add_invalidate_notifier (GClosure      *closure,
 				   gpointer       notify_data,
 				   GClosureNotify notify_func)
 {
+  GClosureFlags old_flags;
   guint i;
 
   g_return_if_fail (closure != NULL);
   g_return_if_fail (notify_func != NULL);
-  g_return_if_fail (closure->is_invalid == FALSE);
+
-  g_return_if_fail (closure->n_inotifiers < CLOSURE_MAX_N_INOTIFIERS);
+  old_flags = closure_atomic_get_flags (closure);
+  g_return_if_fail (!old_flags.flags.is_invalid);
+  g_return_if_fail (old_flags.flags.n_inotifiers < CLOSURE_MAX_N_INOTIFIERS);
 
   closure->notifiers = g_renew (GClosureNotifyData, closure->notifiers, CLOSURE_N_NOTIFIERS (closure) + 1);
   i = CLOSURE_N_MFUNCS (closure) + closure->n_fnotifiers + closure->n_inotifiers;
@@ -597,9 +637,12 @@ closure_invalidate_internal (GClosure *closure)
 void
 g_closure_invalidate (GClosure *closure)
 {
+  GClosureFlags old_flags;
+
   g_return_if_fail (closure != NULL);
 
-  if (!closure->is_invalid)
+  old_flags = closure_atomic_get_flags (closure);
+  if (!old_flags.flags.is_invalid)
     {
       g_closure_ref (closure);           /* preserve floating flag */
       closure_invalidate_internal (closure);
@@ -621,12 +664,15 @@ void
 g_closure_unref (GClosure *closure)
 {
   guint new_ref_count;
+  GClosureFlags old_flags;
 
   g_return_if_fail (closure != NULL);
-  g_return_if_fail (closure->ref_count > 0);
+
+  old_flags = closure_atomic_get_flags (closure);
+  g_return_if_fail (old_flags.flags.ref_count > 0);
 
   /* last unref, invalidate first */
-  if (closure->ref_count == 1 && !closure->is_invalid)
+  if (old_flags.flags.ref_count == 1 && !old_flags.flags.is_invalid)
     closure_invalidate_internal (closure);
 
   ATOMIC_DEC_ASSIGN (closure, ref_count, &new_ref_count);
@@ -712,15 +758,19 @@ g_closure_unref (GClosure *closure)
 void
 g_closure_sink (GClosure *closure)
 {
+  GClosureFlags old_flags;
+
   g_return_if_fail (closure != NULL);
-  g_return_if_fail (closure->ref_count > 0);
+
+  old_flags = closure_atomic_get_flags (closure);
+  g_return_if_fail (old_flags.flags.ref_count > 0);
 
   /* floating is basically a kludge to avoid creating closures
    * with a ref_count of 0. so the initial ref_count a closure has
    * is unowned. with invoking g_closure_sink() code may
    * indicate that it takes over that initial ref_count.
    */
-  if (closure->floating)
+  if (old_flags.flags.floating)
     {
       gboolean was_floating;
       ATOMIC_SWAP (closure, floating, FALSE, &was_floating);
@@ -746,10 +796,14 @@ g_closure_remove_invalidate_notifier (GClosure      *closure,
 				      gpointer       notify_data,
 				      GClosureNotify notify_func)
 {
+  GClosureFlags old_flags;
+
   g_return_if_fail (closure != NULL);
   g_return_if_fail (notify_func != NULL);
 
-  if (closure->is_invalid && closure->in_inotify && /* account removal of notify_func() while it's called */
+  old_flags = closure_atomic_get_flags (closure);
+
+  if (old_flags.flags.is_invalid && old_flags.flags.in_inotify && /* account removal of notify_func() while it's called */
       ((gpointer) closure->marshal) == ((gpointer) notify_func) &&
       closure->data == notify_data)
     closure->marshal = NULL;
@@ -774,10 +828,14 @@ g_closure_remove_finalize_notifier (GClosure      *closure,
 				    gpointer       notify_data,
 				    GClosureNotify notify_func)
 {
+  GClosureFlags old_flags;
+
   g_return_if_fail (closure != NULL);
   g_return_if_fail (notify_func != NULL);
 
-  if (closure->is_invalid && !closure->in_inotify && /* account removal of notify_func() while it's called */
+  old_flags = closure_atomic_get_flags (closure);
+
+  if (old_flags.flags.is_invalid && !old_flags.flags.in_inotify && /* account removal of notify_func() while it's called */
       ((gpointer) closure->marshal) == ((gpointer) notify_func) &&
       closure->data == notify_data)
     closure->marshal = NULL;
@@ -1098,13 +1156,16 @@ gboolean
 _g_closure_is_void (GClosure *closure,
 		    gpointer instance)
 {
+  GClosureFlags old_flags;
   GRealClosure *real_closure;
   GTypeClass *class;
   gpointer callback;
   GType itype;
   guint offset;
 
-  if (closure->is_invalid)
+  old_flags = closure_atomic_get_flags (closure);
+
+  if (old_flags.flags.is_invalid)
     return TRUE;
 
   real_closure = G_REAL_CLOSURE (closure);