Skip to content
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.

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

Don't call lookdict_index in delitemif_lock_held #117826

Closed
colesbury opened this issue Apr 12, 2024 · 2 comments
Closed

Don't call lookdict_index in delitemif_lock_held #117826

colesbury opened this issue Apr 12, 2024 · 2 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@colesbury
Copy link
Contributor

colesbury commented Apr 12, 2024

The delitemif_lock_held function calls lookdict_index and passes the returned hashpos to delitem_common. We should just pass the actual hash to delitem_common.

The existing code is confusing and unnecessary, but it's not exactly a bug. The behavior is fine because it's safe in this case to pretend the hashtable index (hashpos) is a hash value. The resulting size_t i = (size_t)hash & mask to compute the index from the hash is effectively a no-op.

cpython/Objects/dictobject.c

Lines 2635 to 2645 in 069de14

hashpos = lookdict_index(mp->ma_keys, hash, ix);
assert(hashpos >= 0);
if (res > 0) {
PyInterpreterState *interp = _PyInterpreterState_GET();
uint64_t new_version = _PyDict_NotifyEvent(
interp, PyDict_EVENT_DELETED, mp, key, NULL);
return delitem_common(mp, hashpos, ix, old_value, new_version);
} else {
return 0;
}

cpython/Objects/dictobject.c

Lines 2502 to 2504 in 069de14

static int
delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix,
PyObject *old_value, uint64_t new_version)

cpython/Objects/dictobject.c

Lines 995 to 1000 in 069de14

static Py_ssize_t
lookdict_index(PyDictKeysObject *k, Py_hash_t hash, Py_ssize_t index)
{
size_t mask = DK_MASK(k);
size_t perturb = (size_t)hash;
size_t i = (size_t)hash & mask;

Linked PRs

@corona10
Copy link
Member

@colesbury Now we can close the issue?

@colesbury
Copy link
Contributor Author

Yes, thanks for fixing this @corona10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Development

No branches or pull requests

2 participants