gh-117657: Use an atomic store to set type flags.

[nascheme]

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.

• Issue: Make TSAN tests pass with the GIL disabled in free-threaded builds #117657

[colesbury]

Overall, looks good to me. I think type_set_flags can be simplified and I have a question below about locking.

[nascheme]

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();

[nascheme]

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.