atomic: Fix missing full memory barrier on GCC/Clang#15367
Open
cgutman wants to merge 1 commit intolibsdl-org:mainfrom
Open
atomic: Fix missing full memory barrier on GCC/Clang#15367cgutman wants to merge 1 commit intolibsdl-org:mainfrom
cgutman wants to merge 1 commit intolibsdl-org:mainfrom
Conversation
__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.
sezero
reviewed
Apr 12, 2026
| #ifndef SDL_PLATFORM_ANDROID | ||
| #define HAVE_ATOMIC_LOAD_N 1 | ||
| #endif | ||
| #if __has_builtin(__atomic_exchange_n) || defined(HAVE_GCC_ATOMICS) |
Contributor
There was a problem hiding this comment.
Maybe use SDL_HAS_BUILTIN to be safe? (_SDL_HAS_BUILTIN for SDL2.)
Collaborator
Author
There was a problem hiding this comment.
I pulled this change (and the change to use __atomic_load() on Android) into #15374
Contributor
|
If accepted, should probably be backported to SDL2. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
__sync_lock_test_and_set()is designed for creating locks, not as a general atomic exchange function like we use it for inSDL_SetAtomicInt(),SDL_SetAtomicU32(), andSDL_SetAtomicPointer(). 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.I also removed the ancient
#ifndef SDL_PLATFORM_ANDROIDcheck since this is hopefully fixed in modern NDKs.Existing Issue(s)