pythongh-142534: Avoid TSan warnings in dictobject.c (pythongh-142544)

colesbury · miss-islington · commit 59c612ff2253 · 2025-12-11T21:23:25.000Z
There are places we use "relaxed" loads where C11 requires "consume" or stronger. Unfortunately, compilers don't really implement "consume" so fake it for our use in a way that avoids upsetting TSan. (cherry picked from commit 0a62f82) Co-authored-by: Sam Gross <colesbury@gmail.com>
diff --git a/Include/cpython/pyatomic.h b/Include/cpython/pyatomic.h
@@ -591,6 +591,17 @@ static inline void _Py_atomic_fence_release(void);
 
 // --- aliases ---------------------------------------------------------------
 
+// Compilers don't really support "consume" semantics, so we fake it. Use
+// "acquire" with TSan to support false positives. Use "relaxed" otherwise,
+// because CPUs on all platforms we support respect address dependencies without
+// extra barriers.
+// See 2.6.7 in https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2055r0.pdf
+#if defined(_Py_THREAD_SANITIZER)
+# define _Py_atomic_load_ptr_consume _Py_atomic_load_ptr_acquire
+#else
+# define _Py_atomic_load_ptr_consume _Py_atomic_load_ptr_relaxed
+#endif
+
 #if SIZEOF_LONG == 8
 # define _Py_atomic_load_ulong(p) \
     _Py_atomic_load_uint64((uint64_t *)p)
diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h
@@ -31,6 +31,8 @@ extern "C" {
     _Py_atomic_store_ptr(&value, new_value)
 #define FT_ATOMIC_LOAD_PTR_ACQUIRE(value) \
     _Py_atomic_load_ptr_acquire(&value)
+#define FT_ATOMIC_LOAD_PTR_CONSUME(value) \
+    _Py_atomic_load_ptr_consume(&value)
 #define FT_ATOMIC_LOAD_UINTPTR_ACQUIRE(value) \
     _Py_atomic_load_uintptr_acquire(&value)
 #define FT_ATOMIC_LOAD_PTR_RELAXED(value) \
@@ -123,6 +125,7 @@ extern "C" {
 #define FT_ATOMIC_LOAD_SSIZE_ACQUIRE(value) value
 #define FT_ATOMIC_LOAD_SSIZE_RELAXED(value) value
 #define FT_ATOMIC_LOAD_PTR_ACQUIRE(value) value
+#define FT_ATOMIC_LOAD_PTR_CONSUME(value) value
 #define FT_ATOMIC_LOAD_UINTPTR_ACQUIRE(value) value
 #define FT_ATOMIC_LOAD_PTR_RELAXED(value) value
 #define FT_ATOMIC_LOAD_UINT8(value) value
diff --git a/Objects/dictobject.c b/Objects/dictobject.c
@@ -1082,7 +1082,7 @@ compare_unicode_unicode(PyDictObject *mp, PyDictKeysObject *dk,
                         void *ep0, Py_ssize_t ix, PyObject *key, Py_hash_t hash)
 {
     PyDictUnicodeEntry *ep = &((PyDictUnicodeEntry *)ep0)[ix];
-    PyObject *ep_key = FT_ATOMIC_LOAD_PTR_RELAXED(ep->me_key);
+    PyObject *ep_key = FT_ATOMIC_LOAD_PTR_CONSUME(ep->me_key);
     assert(ep_key != NULL);
     assert(PyUnicode_CheckExact(ep_key));
     if (ep_key == key ||
@@ -1375,7 +1375,7 @@ compare_unicode_generic_threadsafe(PyDictObject *mp, PyDictKeysObject *dk,
                                    void *ep0, Py_ssize_t ix, PyObject *key, Py_hash_t hash)
 {
     PyDictUnicodeEntry *ep = &((PyDictUnicodeEntry *)ep0)[ix];
-    PyObject *startkey = _Py_atomic_load_ptr_relaxed(&ep->me_key);
+    PyObject *startkey = _Py_atomic_load_ptr_consume(&ep->me_key);
     assert(startkey == NULL || PyUnicode_CheckExact(ep->me_key));
     assert(!PyUnicode_CheckExact(key));
 
@@ -1418,7 +1418,7 @@ compare_unicode_unicode_threadsafe(PyDictObject *mp, PyDictKeysObject *dk,
                                    void *ep0, Py_ssize_t ix, PyObject *key, Py_hash_t hash)
 {
     PyDictUnicodeEntry *ep = &((PyDictUnicodeEntry *)ep0)[ix];
-    PyObject *startkey = _Py_atomic_load_ptr_relaxed(&ep->me_key);
+    PyObject *startkey = _Py_atomic_load_ptr_consume(&ep->me_key);
     if (startkey == key) {
         assert(PyUnicode_CheckExact(startkey));
         return 1;
@@ -1454,7 +1454,7 @@ compare_generic_threadsafe(PyDictObject *mp, PyDictKeysObject *dk,
                            void *ep0, Py_ssize_t ix, PyObject *key, Py_hash_t hash)
 {
     PyDictKeyEntry *ep = &((PyDictKeyEntry *)ep0)[ix];
-    PyObject *startkey = _Py_atomic_load_ptr_relaxed(&ep->me_key);
+    PyObject *startkey = _Py_atomic_load_ptr_consume(&ep->me_key);
     if (startkey == key) {
         return 1;
     }
@@ -5519,7 +5519,7 @@ dictiter_iternext_threadsafe(PyDictObject *d, PyObject *self,
     k = _Py_atomic_load_ptr_acquire(&d->ma_keys);
     assert(i >= 0);
     if (_PyDict_HasSplitTable(d)) {
-        PyDictValues *values = _Py_atomic_load_ptr_relaxed(&d->ma_values);
+        PyDictValues *values = _Py_atomic_load_ptr_consume(&d->ma_values);
         if (values == NULL) {
             goto concurrent_modification;
         }
@@ -7111,7 +7111,7 @@ _PyObject_TryGetInstanceAttribute(PyObject *obj, PyObject *name, PyObject **attr
     Py_BEGIN_CRITICAL_SECTION(dict);
 
     if (dict->ma_values == values && FT_ATOMIC_LOAD_UINT8(values->valid)) {
-        value = _Py_atomic_load_ptr_relaxed(&values->values[ix]);
+        value = _Py_atomic_load_ptr_consume(&values->values[ix]);
         *attr = _Py_XNewRefWithLock(value);
         success = true;
     } else {
