Skip to content

gh-117657: Fix data races report by TSAN unicode-hash#119907

Merged
corona10 merged 2 commits intopython:mainfrom
corona10:gh-117657-unicode-hash
Jun 3, 2024
Merged

gh-117657: Fix data races report by TSAN unicode-hash#119907
corona10 merged 2 commits intopython:mainfrom
corona10:gh-117657-unicode-hash

Conversation

@corona10
Copy link
Member

@corona10 corona10 commented Jun 1, 2024 •
edited by bedevere-app bot
Loading

@corona10 corona10 changed the title gh-117657: Fix data races report by TSAN unicode-hash [WIP] gh-117657: Fix data races report by TSAN unicode-hash Jun 1, 2024
@bedevere-app bedevere-app bot mentioned this pull request Jun 1, 2024
Closed
@corona10 corona10 changed the title [WIP] gh-117657: Fix data races report by TSAN unicode-hash gh-117657: Remove unicode-hash from TSAN suppressions Jun 1, 2024
@corona10 corona10 requested review from colesbury and mpage June 1, 2024 14:40
@corona10
Copy link
Member Author

corona10 commented Jun 1, 2024

@colesbury @mpage
From my local clang-18 and GHA do not report race condition issues for unicode-hash, so IMO, we can remove the function from the list until it is detected in the future.

@corona10 corona10 added the needs backport to 3.13 bugs and security fixes label Jun 1, 2024
@corona10 corona10 marked this pull request as ready for review June 1, 2024 14:42
@colesbury
Copy link
Contributor

colesbury commented Jun 1, 2024

I think there is still a data race, even if it's only reported occasionally. We read and write to _PyUnicode_HASH(self) without atomics -- I think we should use relaxed atomic load/stores.

cpython/Objects/unicodeobject.c

Lines 11391 to 11396 in 90ec19f

if (_PyUnicode_HASH(self) != -1)
return _PyUnicode_HASH(self);
x = _Py_HashBytes(PyUnicode_DATA(self),
PyUnicode_GET_LENGTH(self) * PyUnicode_KIND(self));
_PyUnicode_HASH(self) = x;

@corona10
Copy link
Member Author

corona10 commented Jun 1, 2024

I think there is still a data race, even if it's only reported occasionally. We read and write to _PyUnicode_HASH(self) without atomics -- I think we should use relaxed atomic load/stores.

Thank you for the feedback, I will take a look.

@corona10 corona10 changed the title gh-117657: Remove unicode-hash from TSAN suppressions [WIP] gh-117657: Remove unicode-hash from TSAN suppressions Jun 1, 2024
@corona10 corona10 changed the title [WIP] gh-117657: Remove unicode-hash from TSAN suppressions gh-117657: Fix data races report by TSAN unicode-hash g Jun 2, 2024
@corona10 corona10 changed the title gh-117657: Fix data races report by TSAN unicode-hash g gh-117657: Fix data races report by TSAN unicode-hash Jun 2, 2024
colesbury
colesbury approved these changes Jun 2, 2024
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@corona10 corona10 merged commit 0594a27 into python:main Jun 3, 2024
@miss-islington-app
Copy link

miss-islington-app bot commented Jun 3, 2024

Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@corona10 corona10 deleted the gh-117657-unicode-hash branch June 3, 2024 03:22
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 3, 2024
@corona10 @miss-islington
pythongh-117657: Fix data races report by TSAN unicode-hash (pythongh…
ca55fa2 
…-119907)

(cherry picked from commit 0594a27)

Co-authored-by: Donghee Na <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jun 3, 2024

GH-119963 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 3, 2024
corona10 added a commit that referenced this pull request Jun 3, 2024
@miss-islington @corona10
[3.13] gh-117657: Fix data races report by TSAN unicode-hash (gh-119907…
e5dfcea 
…) (gh-119963)

gh-117657: Fix data races report by TSAN unicode-hash (gh-119907)
(cherry picked from commit 0594a27)

Co-authored-by: Donghee Na <[email protected]>
mliezun pushed a commit to mliezun/cpython that referenced this pull request Jun 3, 2024
@corona10 @mliezun
pythongh-117657: Fix data races report by TSAN unicode-hash (pythongh…
7c006a3 
…-119907)
barneygale pushed a commit to barneygale/cpython that referenced this pull request Jun 5, 2024
@corona10 @barneygale
pythongh-117657: Fix data races report by TSAN unicode-hash (pythongh…
ad9cff9 
…-119907)
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
@corona10 @noahbkim
pythongh-117657: Fix data races report by TSAN unicode-hash (pythongh…
719c07f 
…-119907)
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@corona10 @estyxx
pythongh-117657: Fix data races report by TSAN unicode-hash (pythongh…
f1a1699 
…-119907)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury approved these changes

@mpage mpage Awaiting requested review from mpage

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@corona10 @colesbury