gh-117657: Use an atomic store to set type flags. - #127588
Conversation
Sorry, something went wrong.
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.
Sorry, something went wrong.
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: _PyCriticalSection_AssertHeld (mutex=<optimized out>) at ./Include/internal/pycore_critical_section.h:222 type_set_flags (tp=tp@entry=0x200006adc10, flags=22016) at Objects/typeobject.c:355 type_clear_flags (tp=tp@entry=0x200006adc10, flag=flag@entry=1048576) at Objects/typeobject.c:379 type_set_abstractmethods (type=0x200006adc10, value=0x20000342010, context=<optimized out>) at Objects/typeobject.c:1570 getset_set (self=0x20000010790, obj=0x200006adc10, value=0x20000342010) at Objects/descrobject.c:249 type_setattro (self=0x200006adc10, name=<optimized out>, value=0x20000342010) at Objects/typeobject.c:5967 PyObject_SetAttr (v=v@entry=0x200006adc10, name=<optimized out>, value=value@entry=0x20000342010) at Objects/object.c:1409 The code in type_set_abstractmethods is: PyType_Modified(type);
if (abstract)
type_add_flags(type, Py_TPFLAGS_IS_ABSTRACT);
else
type_clear_flags(type, Py_TPFLAGS_IS_ABSTRACT);
When this runs, the type is already marked ready. The type lock is held inside PyType_Modified(). To me this looks like a potential data race. Another thread could read the flags after PyType_Modified releases the lock and before the next lines of code are executed. Looking at PyType_Modified(), I wonder if there is another data race there. The code is: // Quick check without the lock held
if (type->tp_version_tag == 0) {
return;
}
BEGIN_TYPE_LOCK();
type_modified_unlocked(type);
END_TYPE_LOCK();
The tp_version_tag is set with an atomic store (by set_version_unlocked()). I would think when checking if it's 0, we need an atomic load for it. A possible solution could be to change the type_set_abstractmethods code to be: BEGIN_TYPE_LOCK();
type_modified_unlocked(type);
if (abstract)
type_add_flags(type, Py_TPFLAGS_IS_ABSTRACT);
else
type_clear_flags(type, Py_TPFLAGS_IS_ABSTRACT);
END_TYPE_LOCK();
|
Sorry, something went wrong.
|
Using type_modified_unlocked() in couple of places and putting the flag updates inside the critical section seems to have worked well. I only had to modify a couple places and the unit tests now all pass without that lock held assertion failing. |
Sorry, something went wrong.
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.