glib fallback atomic int/ptr get/set have memory barrier in wrong place
For reference: https://en.cppreference.com/w/c/atomic/memory_order
The latest gcc implementation of the atomic ops use the __ATOMIC_SEQ_CST memory ordering. Which is good. The fallback uses memory barrier to implement those instead; in particular:
#define g_atomic_int_get(atomic) \
(G_GNUC_EXTENSION ({ \
G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gint)); \
(void) (0 ? *(atomic) ^ *(atomic) : 0); \
__sync_synchronize (); \
(gint) *(atomic); \
}))
#define g_atomic_int_set(atomic, newval) \
(G_GNUC_EXTENSION ({ \
G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gint)); \
(void) (0 ? *(atomic) ^ (newval) : 0); \
*(atomic) = (newval); \
__sync_synchronize (); \
}))
#define g_atomic_pointer_get(atomic) \
(G_GNUC_EXTENSION ({ \
G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gpointer)); \
__sync_synchronize (); \
(gpointer) *(atomic); \
}))
#define g_atomic_pointer_set(atomic, newval) \
(G_GNUC_EXTENSION ({ \
G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gpointer)); \
(void) (0 ? (gpointer) *(atomic) : 0); \
*(atomic) = (__typeof__ (*(atomic))) (gsize) (newval); \
__sync_synchronize (); \
}))
Ie. a get is memory-barrier followed by unprotected read; a set is unprotected write followed by memory-barrier.
This, however, as was pointed out to me by some Google thread-sanitizer engineers, is problematic. Indeed, there never even was a description of what memory ordering model / guarantees glib's atomic provides. But the common way to use this operations shows a valid bug, specially with the pointer versions.
Typically one would create an object, then atomically sets the a pointer to point to the object. Other threads that read the pointer need to be sure that the data pointed to is correctly visible. To make sure, we want to ensure no write operations before the pointer set is reordered by the compiler to after setting the pointer. Ie. the memory barrier on set operation should be before the write, not after it. Indeed, that's part of the release semantics:
""" memory_order_release A store operation with this memory order performs the release operation: no reads or writes in the current thread can be reordered after this store. All writes in the current thread are visible in other threads that acquire the same atomic variable (see Release-Acquire ordering below) and writes that carry a dependency into the atomic variable become visible in other threads that consume the same atomic (see Release-Consume ordering below). """
The get operation also needs a memory barrier after it, such that dependent reads cannot be reordered to before it. To quote an example from https://en.wikipedia.org/wiki/Memory_barrier
Processor #1:
while (f == 0);
// Memory fence required here
print x;
Processor #2:
x = 42;
// Memory fence required here
f = 1;
I suggest reversing the orders.
I have copied this ordering in cairo, fontconfig, and harfbuzz over the years. Need to go fix them.