Bug report

Bug description:

First of all, I'm not a threading expert and my understanding of the memory-ordering model may be wrong. So, if I'm wrong I will be happy to fix my knowledge lacoons.

I saw some inconsistency (from my sight) of loading and writing of managed dict pointer.
Non-atomic loads:

1. PyObject_VisitManagedDict
   

   cpython/Objects/dictobject.c

   Line 7202 in 9ad0c7b

   Py_VISIT(_PyObject_ManagedDictPointer(obj)->dict);

2. PyObject_ClearManagedDict
   

   cpython/Objects/dictobject.c

   Line 7462 in 9ad0c7b

   Py_CLEAR(_PyObject_ManagedDictPointer(obj)->dict);

3. _PyObject_GetDictPtr
   

   cpython/Objects/object.c

   Line 1541 in 9ad0c7b

   return (PyObject **)&_PyObject_ManagedDictPointer(obj)->dict;

Non-atomic stores:

1. _PyObject_InitInlineValues
   

   cpython/Objects/dictobject.c

   Lines 6787 to 6791 in 9ad0c7b

   	for (size_t i = 0; i < size; i++) {
   	values->values[i] = NULL;
   	}
   	_PyObject_ManagedDictPointer(obj)->dict = NULL;
   	}

IIUC mixing of non-atomic loads/stores with atomic ones may lead to data races.

memory_order_acquire loads:

1. _PyObject_GetManagedDict
   

   cpython/Include/internal/pycore_object.h

   Lines 932 to 936 in 9ad0c7b

   	_PyObject_GetManagedDict(PyObject *obj)
   	{
   	PyManagedDictPointer *dorv = _PyObject_ManagedDictPointer(obj);
   	return (PyDictObject *)FT_ATOMIC_LOAD_PTR_ACQUIRE(dorv->dict);
   	}

memory_order_release stores:

1. _PyObject_MaterializeManagedDict_LockHeld
   

   cpython/Objects/dictobject.c

   Lines 6827 to 6829 in 9ad0c7b

   	FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict,
   	dict);
   	return dict;

2. store_instance_attr_lock_held
   

   cpython/Objects/dictobject.c

   Lines 6925 to 6927 in 9ad0c7b

   	FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict,
   	(PyDictObject *)dict);
   	return 0;

3. ensure_managed_dict
   

   cpython/Objects/dictobject.c

   Lines 7494 to 7495 in 9ad0c7b

   	FT_ATOMIC_STORE_PTR_RELEASE(_PyObject_ManagedDictPointer(obj)->dict,
   	(PyDictObject *)dict);

memory_order_seq_cst stores:

1. set_dict_inline_values
   

   cpython/Objects/dictobject.c

   Line 7225 in 9ad0c7b

   FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict, new_dict);

2. try_set_dict_inline_only_or_other_dict
   

   cpython/Objects/dictobject.c

   Lines 7252 to 7253 in 9ad0c7b

   	FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
   	(PyDictObject *)Py_XNewRef(new_dict));

3. replace_dict_probably_inline_materialized
   

   cpython/Objects/dictobject.c

   Lines 7287 to 7289 in 9ad0c7b

   	FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
   	(PyDictObject *)Py_XNewRef(new_dict));
   	return 0;

IIUC mixing acquire/release with seq_cst may break total order of seq_cst operations.

Mixing with memory_order_seq_cst stores

1. _PyObject_SetManagedDict
   

   cpython/Objects/dictobject.c

   Lines 7356 to 7365 in 9ad0c7b

   	PyDictObject *dict = _PyObject_GetManagedDict(obj);
   	if (dict == NULL) {
   	set_dict_inline_values(obj, (PyDictObject *)new_dict);
   	return 0;
   	}
   	if (_PyDict_DetachFromObject(dict, obj) == 0) {
   	_PyObject_ManagedDictPointer(obj)->dict = (PyDictObject *)Py_XNewRef(new_dict);
   	Py_DECREF(dict);
   	return 0;
   	}

_PyObject_SetManagedDict uses non-atomic load but stores with seq_cst mode so it is OK (IIUC), but following store without fence may lead to data race.

Are my findings valid or am I completely wrong?

cc @colesbury @kumaraditya303

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

Linked PRs

• gh-133980: use atomic store in PyObject_GenericSetDict #133988
• [3.14] gh-133980: use atomic store in PyObject_GenericSetDict (GH-133988) #134354