Преглед изворни кода

atomic: Fix missing full memory barrier on GCC/Clang

__sync_lock_test_and_set() is designed for creating locks, not as
a general atomic exchange function. As a result, it only provides
an acquire memory barrier and isn't guaranteed to actually store
the provided value (though it does on architectures we care about).

__atomic_exchange_n() is supported on GCC/Clang for the last ~10
years, so let's use that instead if available. We will keep the
__sync_lock_test_and_set() fallback around for ancient platforms,
but add a full memory barrier to match the documented behavior.
Cameron Gutman пре 1 месец
родитељ
комит
509a36db16
2 измењених фајлова са 31 додато и 3 уклоњено
  1. 1 0
      CMakeLists.txt
  2. 30 3
      src/atomic/SDL_atomic.c

+ 1 - 0
CMakeLists.txt

@@ -3251,6 +3251,7 @@ elseif(PS2)
       gskit
       dmakit
       ps2_drivers
+      atomic
   )
 elseif(N3DS)
   sdl_glob_sources("${SDL3_SOURCE_DIR}/src/main/n3ds/*.c")

+ 30 - 3
src/atomic/SDL_atomic.c

@@ -33,12 +33,18 @@
 #include <atomic.h>
 #endif
 
-// The __atomic_load_n() intrinsic showed up in different times for different compilers.
-#if defined(__GNUC__) && (__GNUC__ >= 5)
+// The __atomic intrinsics showed up in different times for different compilers.
+#if (defined(__GNUC__) && (__GNUC__ >= 5)) || (defined(__clang__) && defined(HAVE_GCC_ATOMICS))
 #define HAVE_ATOMIC_LOAD_N 1
-#elif SDL_HAS_BUILTIN(__atomic_load_n) || (defined(__clang__) && defined(HAVE_GCC_ATOMICS))
+#define HAVE_ATOMIC_EXCHANGE_N 1
+#else
+#if SDL_HAS_BUILTIN(__atomic_load_n)
 #define HAVE_ATOMIC_LOAD_N 1
 #endif
+#if SDL_HAS_BUILTIN(__atomic_exchange_n)
+#define HAVE_ATOMIC_EXCHANGE_N 1
+#endif
+#endif
 
 /*
   If any of the operations are not provided then we must emulate some
@@ -174,7 +180,14 @@ int SDL_SetAtomicInt(SDL_AtomicInt *a, int v)
 #ifdef HAVE_MSC_ATOMICS
     SDL_COMPILE_TIME_ASSERT(atomic_set, sizeof(long) == sizeof(a->value));
     return _InterlockedExchange((long *)&a->value, v);
+#elif defined(HAVE_ATOMIC_EXCHANGE_N)
+    return __atomic_exchange_n(&a->value, v, __ATOMIC_SEQ_CST);
 #elif defined(HAVE_GCC_ATOMICS)
+    // __sync_lock_test_and_set() is designed for locking rather than a
+    // generic atomic exchange, so it only provides an acquire barrier
+    // and may not store the exact value on all architectures. We prefer
+    // __atomic_exchange_n() instead on all modern compilers.
+    __sync_synchronize();
     return __sync_lock_test_and_set(&a->value, v);
 #elif defined(SDL_PLATFORM_SOLARIS)
     SDL_COMPILE_TIME_ASSERT(atomic_set, sizeof(uint_t) == sizeof(a->value));
@@ -193,7 +206,14 @@ Uint32 SDL_SetAtomicU32(SDL_AtomicU32 *a, Uint32 v)
 #ifdef HAVE_MSC_ATOMICS
     SDL_COMPILE_TIME_ASSERT(atomic_set, sizeof(long) == sizeof(a->value));
     return _InterlockedExchange((long *)&a->value, v);
+#elif defined(HAVE_ATOMIC_EXCHANGE_N)
+    return __atomic_exchange_n(&a->value, v, __ATOMIC_SEQ_CST);
 #elif defined(HAVE_GCC_ATOMICS)
+    // __sync_lock_test_and_set() is designed for locking rather than a
+    // generic atomic exchange, so it only provides an acquire barrier
+    // and may not store the exact value on all architectures. We prefer
+    // __atomic_exchange_n() instead on all modern compilers.
+    __sync_synchronize();
     return __sync_lock_test_and_set(&a->value, v);
 #elif defined(SDL_PLATFORM_SOLARIS)
     SDL_COMPILE_TIME_ASSERT(atomic_set, sizeof(uint_t) == sizeof(a->value));
@@ -211,7 +231,14 @@ void *SDL_SetAtomicPointer(void **a, void *v)
 {
 #ifdef HAVE_MSC_ATOMICS
     return _InterlockedExchangePointer(a, v);
+#elif defined(HAVE_ATOMIC_EXCHANGE_N)
+    return __atomic_exchange_n(a, v, __ATOMIC_SEQ_CST);
 #elif defined(HAVE_GCC_ATOMICS)
+    // __sync_lock_test_and_set() is designed for locking rather than a
+    // generic atomic exchange, so it only provides an acquire barrier
+    // and may not store the exact value on all architectures. We prefer
+    // __atomic_exchange_n() instead on all modern compilers.
+    __sync_synchronize();
     return __sync_lock_test_and_set(a, v);
 #elif defined(SDL_PLATFORM_SOLARIS)
     return atomic_swap_ptr(a, v);