Skip to content

[3.13] gh-117657: Fix data races reported by TSAN in some set methods (GH-120914) #121240

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 1, 2024
Merged

[3.13] gh-117657: Fix data races reported by TSAN in some set methods (GH-120914) #121240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions Include/internal/pycore_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,20 @@ _PyObject_IS_GC(PyObject *obj)
&& (type->tp_is_gc == NULL || type->tp_is_gc(obj)));
}

// Fast inlined version of PyObject_Hash()
static inline Py_hash_t
_PyObject_HashFast(PyObject *op)
{
if (PyUnicode_CheckExact(op)) {
Py_hash_t hash = FT_ATOMIC_LOAD_SSIZE_RELAXED(
_PyASCIIObject_CAST(op)->hash);
if (hash != -1) {
return hash;
}
}
return PyObject_Hash(op);
}

// Fast inlined version of PyType_IS_GC()
#define _PyType_IS_GC(t) _PyType_HasFeature((t), Py_TPFLAGS_HAVE_GC)

Expand Down
9 changes: 3 additions & 6 deletions Modules/_collectionsmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2537,12 +2537,9 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping,
if (key == NULL)
break;

if (!PyUnicode_CheckExact(key) ||
(hash = _PyASCIIObject_CAST(key)->hash) == -1)
{
hash = PyObject_Hash(key);
if (hash == -1)
goto done;
hash = _PyObject_HashFast(key);
if (hash == -1) {
goto done;
}

oldval = _PyDict_GetItem_KnownHash(mapping, key, hash);
Expand Down
135 changes: 51 additions & 84 deletions Objects/dictobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ static inline Py_hash_t
unicode_get_hash(PyObject *o)
{
assert(PyUnicode_CheckExact(o));
return _PyASCIIObject_CAST(o)->hash;
return FT_ATOMIC_LOAD_SSIZE_RELAXED(_PyASCIIObject_CAST(o)->hash);
}

/* Print summary info about the state of the optimized allocator */
Expand Down Expand Up @@ -2170,13 +2170,10 @@ dict_getitem(PyObject *op, PyObject *key, const char *warnmsg)
}
PyDictObject *mp = (PyDictObject *)op;

Py_hash_t hash;
if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) {
hash = PyObject_Hash(key);
if (hash == -1) {
PyErr_FormatUnraisable(warnmsg);
return NULL;
}
Py_hash_t hash = _PyObject_HashFast(key);
if (hash == -1) {
PyErr_FormatUnraisable(warnmsg);
return NULL;
}

PyThreadState *tstate = _PyThreadState_GET();
Expand Down Expand Up @@ -2225,12 +2222,9 @@ _PyDict_LookupIndex(PyDictObject *mp, PyObject *key)
assert(PyDict_CheckExact((PyObject*)mp));
assert(PyUnicode_CheckExact(key));

Py_hash_t hash = unicode_get_hash(key);
Py_hash_t hash = _PyObject_HashFast(key);
if (hash == -1) {
hash = PyObject_Hash(key);
if (hash == -1) {
return -1;
}
return -1;
}

return _Py_dict_lookup(mp, key, hash, &value);
Expand Down Expand Up @@ -2301,14 +2295,10 @@ PyDict_GetItemRef(PyObject *op, PyObject *key, PyObject **result)
return -1;
}

Py_hash_t hash;
if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1)
{
hash = PyObject_Hash(key);
if (hash == -1) {
*result = NULL;
return -1;
}
Py_hash_t hash = _PyObject_HashFast(key);
if (hash == -1) {
*result = NULL;
return -1;
}

return _PyDict_GetItemRef_KnownHash((PyDictObject *)op, key, hash, result);
Expand All @@ -2320,13 +2310,10 @@ _PyDict_GetItemRef_Unicode_LockHeld(PyDictObject *op, PyObject *key, PyObject **
ASSERT_DICT_LOCKED(op);
assert(PyUnicode_CheckExact(key));

Py_hash_t hash;
if ((hash = unicode_get_hash(key)) == -1) {
hash = PyObject_Hash(key);
if (hash == -1) {
*result = NULL;
return -1;
}
Py_hash_t hash = _PyObject_HashFast(key);
if (hash == -1) {
*result = NULL;
return -1;
}

PyObject *value;
Expand Down Expand Up @@ -2360,12 +2347,9 @@ PyDict_GetItemWithError(PyObject *op, PyObject *key)
PyErr_BadInternalCall();
return NULL;
}
if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1)
{
hash = PyObject_Hash(key);
if (hash == -1) {
return NULL;
}
hash = _PyObject_HashFast(key);
if (hash == -1) {
return NULL;
}

#ifdef Py_GIL_DISABLED
Expand Down Expand Up @@ -2433,10 +2417,9 @@ _PyDict_LoadGlobal(PyDictObject *globals, PyDictObject *builtins, PyObject *key)
Py_hash_t hash;
PyObject *value;

if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) {
hash = PyObject_Hash(key);
if (hash == -1)
return NULL;
hash = _PyObject_HashFast(key);
if (hash == -1) {
return NULL;
}

/* namespace 1: globals */
Expand All @@ -2461,14 +2444,11 @@ setitem_take2_lock_held(PyDictObject *mp, PyObject *key, PyObject *value)
assert(key);
assert(value);
assert(PyDict_Check(mp));
Py_hash_t hash;
if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) {
hash = PyObject_Hash(key);
if (hash == -1) {
Py_DECREF(key);
Py_DECREF(value);
return -1;
}
Py_hash_t hash = _PyObject_HashFast(key);
if (hash == -1) {
Py_DECREF(key);
Py_DECREF(value);
return -1;
}

PyInterpreterState *interp = _PyInterpreterState_GET();
Expand Down Expand Up @@ -2617,12 +2597,10 @@ delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix,
int
PyDict_DelItem(PyObject *op, PyObject *key)
{
Py_hash_t hash;
assert(key);
if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) {
hash = PyObject_Hash(key);
if (hash == -1)
return -1;
Py_hash_t hash = _PyObject_HashFast(key);
if (hash == -1) {
return -1;
}

return _PyDict_DelItem_KnownHash(op, key, hash);
Expand Down Expand Up @@ -2946,15 +2924,12 @@ pop_lock_held(PyObject *op, PyObject *key, PyObject **result)
return 0;
}

Py_hash_t hash;
if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) {
hash = PyObject_Hash(key);
if (hash == -1) {
if (result) {
*result = NULL;
}
return -1;
Py_hash_t hash = _PyObject_HashFast(key);
if (hash == -1) {
if (result) {
*result = NULL;
}
return -1;
}
return _PyDict_Pop_KnownHash(dict, key, hash, result);
}
Expand Down Expand Up @@ -3285,10 +3260,9 @@ dict_subscript(PyObject *self, PyObject *key)
Py_hash_t hash;
PyObject *value;

if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) {
hash = PyObject_Hash(key);
if (hash == -1)
return NULL;
hash = _PyObject_HashFast(key);
if (hash == -1) {
return NULL;
}
ix = _Py_dict_lookup_threadsafe(mp, key, hash, &value);
if (ix == DKIX_ERROR)
Expand Down Expand Up @@ -4172,10 +4146,9 @@ dict_get_impl(PyDictObject *self, PyObject *key, PyObject *default_value)
Py_hash_t hash;
Py_ssize_t ix;

if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) {
hash = PyObject_Hash(key);
if (hash == -1)
return NULL;
hash = _PyObject_HashFast(key);
if (hash == -1) {
return NULL;
}
ix = _Py_dict_lookup_threadsafe(self, key, hash, &val);
if (ix == DKIX_ERROR)
Expand Down Expand Up @@ -4205,14 +4178,12 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu
return -1;
}

if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) {
hash = PyObject_Hash(key);
if (hash == -1) {
if (result) {
*result = NULL;
}
return -1;
hash = _PyObject_HashFast(key);
if (hash == -1) {
if (result) {
*result = NULL;
}
return -1;
}

if (mp->ma_keys == Py_EMPTY_KEYS) {
Expand Down Expand Up @@ -4642,12 +4613,10 @@ static PyMethodDef mapp_methods[] = {
int
PyDict_Contains(PyObject *op, PyObject *key)
{
Py_hash_t hash;
Py_hash_t hash = _PyObject_HashFast(key);

if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) {
hash = PyObject_Hash(key);
if (hash == -1)
return -1;
if (hash == -1) {
return -1;
}

return _PyDict_Contains_KnownHash(op, key, hash);
Expand Down Expand Up @@ -6727,11 +6696,9 @@ int
_PyDict_SetItem_LockHeld(PyDictObject *dict, PyObject *name, PyObject *value)
{
if (value == NULL) {
Py_hash_t hash;
if (!PyUnicode_CheckExact(name) || (hash = unicode_get_hash(name)) == -1) {
hash = PyObject_Hash(name);
if (hash == -1)
return -1;
Py_hash_t hash = _PyObject_HashFast(name);
if (hash == -1) {
return -1;
}
return delitem_knownhash_lock_held((PyObject *)dict, name, hash);
} else {
Expand Down
30 changes: 9 additions & 21 deletions Objects/setobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,41 +365,29 @@ set_discard_entry(PySetObject *so, PyObject *key, Py_hash_t hash)
static int
set_add_key(PySetObject *so, PyObject *key)
{
Py_hash_t hash;

if (!PyUnicode_CheckExact(key) ||
(hash = _PyASCIIObject_CAST(key)->hash) == -1) {
hash = PyObject_Hash(key);
if (hash == -1)
return -1;
Py_hash_t hash = _PyObject_HashFast(key);
if (hash == -1) {
return -1;
}
return set_add_entry(so, key, hash);
}

static int
set_contains_key(PySetObject *so, PyObject *key)
{
Py_hash_t hash;

if (!PyUnicode_CheckExact(key) ||
(hash = _PyASCIIObject_CAST(key)->hash) == -1) {
hash = PyObject_Hash(key);
if (hash == -1)
return -1;
Py_hash_t hash = _PyObject_HashFast(key);
if (hash == -1) {
return -1;
}
return set_contains_entry(so, key, hash);
}

static int
set_discard_key(PySetObject *so, PyObject *key)
{
Py_hash_t hash;

if (!PyUnicode_CheckExact(key) ||
(hash = _PyASCIIObject_CAST(key)->hash) == -1) {
hash = PyObject_Hash(key);
if (hash == -1)
return -1;
Py_hash_t hash = _PyObject_HashFast(key);
if (hash == -1) {
return -1;
}
return set_discard_entry(so, key, hash);
}
Expand Down
13 changes: 4 additions & 9 deletions Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5069,15 +5069,10 @@ find_name_in_mro(PyTypeObject *type, PyObject *name, int *error)
{
ASSERT_TYPE_LOCK_HELD();

Py_hash_t hash;
if (!PyUnicode_CheckExact(name) ||
(hash = _PyASCIIObject_CAST(name)->hash) == -1)
{
hash = PyObject_Hash(name);
if (hash == -1) {
*error = -1;
return NULL;
}
Py_hash_t hash = _PyObject_HashFast(name);
if (hash == -1) {
*error = -1;
return NULL;
}

/* Look in tp_dict of types in MRO */
Expand Down
1 change: 0 additions & 1 deletion Tools/tsan/suppressions_free_threading.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ race_top:assign_version_tag
race_top:insertdict
race_top:lookup_tp_dict
race_top:new_reference
race_top:set_contains_key
# https://gist.github.com/colesbury/d13d033f413b4ad07929d044bed86c35
race_top:set_discard_entry
race_top:_PyDict_CheckConsistency
Expand Down
Loading