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 in9ad0c7b

   Py_VISIT(_PyObject_ManagedDictPointer(obj)->dict);

2. PyObject_ClearManagedDict
   

   cpython/Objects/dictobject.c

   Line 7462 in9ad0c7b

   Py_CLEAR(_PyObject_ManagedDictPointer(obj)->dict);

3. _PyObject_GetDictPtr
   

   cpython/Objects/object.c

   Line 1541 in9ad0c7b

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

Non-atomic stores:

1. _PyObject_InitInlineValues
   

   cpython/Objects/dictobject.c

   Lines 6787 to 6791 in9ad0c7b

   	for (size_ti=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 in9ad0c7b

   	_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 in9ad0c7b

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

2. store_instance_attr_lock_held
   

   cpython/Objects/dictobject.c

   Lines 6925 to 6927 in9ad0c7b

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

3. ensure_managed_dict
   

   cpython/Objects/dictobject.c

   Lines 7494 to 7495 in9ad0c7b

   	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 in9ad0c7b

   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 in9ad0c7b

   	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 in9ad0c7b

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

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

Mixing withmemory_order_seq_cst stores

1. _PyObject_SetManagedDict
   

   cpython/Objects/dictobject.c

   Lines 7356 to 7365 in9ad0c7b

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

_PyObject_SetManagedDict uses non-atomic load but stores withseq_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 inPyObject_GenericSetDict #133988
• [3.14] gh-133980: use atomic store inPyObject_GenericSetDict (GH-133988) #134354