gh-117657: Use an atomic store to set type flags.#127588
gh-117657: Use an atomic store to set type flags.#127588nascheme merged 4 commits intopython:mainpython/cpython:mainfrom nascheme:gh-117657-tp-flags-set-atomicnascheme/cpython:gh-117657-tp-flags-set-atomicCopy head branch name to clipboard
Conversation
The `PyType_HasFeature()` function reads the flags with a relaxed atomic load and without holding the type lock. To avoid data races, use atomic stores if `PyType_Ready()` has already been called.
colesbury
left a comment
There was a problem hiding this comment.
Overall, looks good to me. I think type_set_flags can be simplified and I have a question below about locking.
Just use FT_ATOMIC_STORE_ULONG_RELAXED() always, not worth the extra code complexity.
This helps ensure that type flag setting is done in a free-threading safe way. Fix a few cases were flags were set without holding the type lock. Use `type_modified_unlocked()` in a couple places where the lock is already head.
|
If I add the assert, it fails like this: The code in When this runs, the type is already marked ready. The type lock is held inside Looking at The tp_version_tag is set with an atomic store (by A possible solution could be to change the |
|
Using |
The
PyType_HasFeature()function reads the flags with a relaxed atomic load and without holding the type lock. To avoid data races, use atomic stores to update the type flags.This change eliminates some warnings from TSAN. For non-free-threaded builds, there should be no performance impact.