gh-117657: TSAN Fix races in `PyMember_Get` and `PyMember_Set`, for C extensions #123211

dpdani · 2024-08-21T21:09:46Z

Fix data races that would only be visible when using C extensions.

This is a follow-up on #119368.

I'm intentionally not testing:

_Py_T_NONE (deprecated)
_Py_T_OBJECT (deprecated)
Py_T_STRING (immutable)
Py_T_STRING_INPLACE (immutable)

Py_T_CHAR

For some reason Py_T_CHAR is untested also in test_capi.test_structmembers.
In fact, it's not even in the supporting C types, which I'm using for the TSAN tests as well: old api, new api.
I'm wondering if there's a specific reason for this, or if it should be in the test suite instead.

I'm guessing that the TSAN suite should cover the thread-safety of Py_T_CHAR regardless?
Or should we dismiss it for TSAN tests as well?

I've added a new member to the T_CHAR member to the _testcapi module, and the rest of the tests don't seem to break.
I can revert this change if needed.

Issue: Make TSAN tests pass with the GIL disabled in free-threaded builds #117657

… for C extensions

picnixz

Quite hard to review since everything looks the same and the functions are not ordered by pairs apparently. I didn't go through everything but I had a question and an observation.

Include/cpython/pyatomic.h

Include/internal/pycore_pyatomic_ft_wrappers.h

dpdani · 2024-08-22T17:23:03Z

I'm not exactly sure why test_importlib fails, but I don't want to push a dummy commit just to retry it.

Anyways, the TSAN checks for test_free_threading.test_slots are 🟢

picnixz · 2024-08-22T17:24:12Z

I'm not exactly sure why test_importlib fails, but I don't want to push a dummy commit just to retry it.

I've relaunched the test manually (I can't relaunch the SSL test though, I don't know why)

colesbury

Thanks. I left a few comments below.

How long do the added tests takes to run?

Python/structmember.c

dpdani · 2024-09-16T11:56:50Z

@colesbury can you take another look?

colesbury

This mostly looks good. Please add a char test to test_structmembers.py. I think the implementation in the PR is broken (see inline comment) and it's not caught by any unit test.

It's fine to ping me on PRs like you did here, but please also use GitHub's "re-request review" button (top-right corner, in the reviewers section). It means that the PR shows up in my list of "awaiting reviews".

Python/structmember.c

Include/cpython/pyatomic_msc.h

dpdani · 2024-09-16T18:25:19Z

Sorry, will do.

Co-authored-by: Sam Gross <[email protected]>

dpdani · 2024-11-02T17:53:55Z

I finally had some time to come back to this.

I guess that moving the stores after error checking does slightly change the externally-visible behavior.
Should we write a news entry?

colesbury

LGTM

colesbury · 2024-12-03T01:17:31Z

!buildbot nogil

bedevere-bot · 2024-12-03T01:17:35Z

🤖 New build scheduled with the buildbot fleet by @colesbury for commit 6c5cec5 🤖

The command will test the builders whose names match following regular expression: nogil

The builders matched are:

AMD64 Windows Server 2022 NoGIL PR
AMD64 Fedora Rawhide NoGIL refleaks PR
AMD64 CentOS9 NoGIL Refleaks PR
x86-64 MacOS Intel ASAN NoGIL PR
AMD64 Windows PGO NoGIL PR
aarch64 Fedora Rawhide NoGIL PR
PPC64LE Fedora Rawhide NoGIL refleaks PR
aarch64 Fedora Rawhide NoGIL refleaks PR
ARM64 MacOS M1 NoGIL PR
AMD64 CentOS9 NoGIL PR
ARM64 MacOS M1 Refleaks NoGIL PR
AMD64 Fedora Rawhide NoGIL PR
PPC64LE Fedora Rawhide NoGIL PR
x86-64 MacOS Intel NoGIL PR

colesbury · 2024-12-03T14:42:04Z

Thanks @dpdani!

bedevere-bot · 2024-12-03T15:02:29Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL9 3.x has failed when building commit 979bf24.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1565/builds/859) and take a look at the build logs.
Check if the failure is related to this commit (979bf24) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1565/builds/859

Failed tests:

test_capi

Failed subtests:

test_char - test.test_capi.test_structmembers.ReadWriteTests_NewAPI.test_char
test_char - test.test_capi.test_structmembers.ReadWriteTests_OldAPI.test_char

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x/build/Lib/test/test_capi/test_structmembers.py", line 170, in test_char
    self.assertEqual(ts.T_CHAR, "c")
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
AssertionError: '\x00' != 'c'
- �
+ c

bedevere-bot · 2024-12-03T15:16:51Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL9 LTO + PGO 3.x has failed when building commit 979bf24.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1578/builds/862) and take a look at the build logs.
Check if the failure is related to this commit (979bf24) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1578/builds/862

Failed tests:

test_capi

Failed subtests:

test_char - test.test_capi.test_structmembers.ReadWriteTests_NewAPI.test_char
test_char - test.test_capi.test_structmembers.ReadWriteTests_OldAPI.test_char

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.lto-pgo/build/Lib/test/test_capi/test_structmembers.py", line 170, in test_char
    self.assertEqual(ts.T_CHAR, "c")
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
AssertionError: '\x00' != 'c'
- �
+ c

dpdani · 2024-12-03T15:20:10Z

that sounds like my fault, I'll take a look later today

bedevere-bot · 2024-12-03T15:23:52Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL9 LTO 3.x has failed when building commit 979bf24.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1587/builds/864) and take a look at the build logs.
Check if the failure is related to this commit (979bf24) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1587/builds/864

Failed tests:

test_capi

Failed subtests:

test_char - test.test_capi.test_structmembers.ReadWriteTests_NewAPI.test_char
test_char - test.test_capi.test_structmembers.ReadWriteTests_OldAPI.test_char

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.lto/build/Lib/test/test_capi/test_structmembers.py", line 170, in test_char
    self.assertEqual(ts.T_CHAR, "c")
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
AssertionError: '\x00' != 'c'
- �
+ c

bedevere-bot · 2024-12-03T15:35:23Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL8 3.x has failed when building commit 979bf24.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/509/builds/7885) and take a look at the build logs.
Check if the failure is related to this commit (979bf24) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/509/builds/7885

Failed tests:

test_capi

Failed subtests:

test_char - test.test_capi.test_structmembers.ReadWriteTests_NewAPI.test_char
test_char - test.test_capi.test_structmembers.ReadWriteTests_OldAPI.test_char

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel8-s390x/build/Lib/test/test_capi/test_structmembers.py", line 170, in test_char
    self.assertEqual(ts.T_CHAR, "c")
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
AssertionError: '\x00' != 'c'
- �
+ c

bedevere-bot · 2024-12-03T15:48:37Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL8 LTO + PGO 3.x has failed when building commit 979bf24.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/442/builds/7968) and take a look at the build logs.
Check if the failure is related to this commit (979bf24) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/442/builds/7968

Failed tests:

test_capi

Failed subtests:

test_char - test.test_capi.test_structmembers.ReadWriteTests_NewAPI.test_char
test_char - test.test_capi.test_structmembers.ReadWriteTests_OldAPI.test_char

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel8-s390x.lto-pgo/build/Lib/test/test_capi/test_structmembers.py", line 170, in test_char
    self.assertEqual(ts.T_CHAR, "c")
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
AssertionError: '\x00' != 'c'
- �
+ c

bedevere-bot · 2024-12-03T15:56:05Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL8 LTO 3.x has failed when building commit 979bf24.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/567/builds/7887) and take a look at the build logs.
Check if the failure is related to this commit (979bf24) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/567/builds/7887

Failed tests:

test_capi

Failed subtests:

test_char - test.test_capi.test_structmembers.ReadWriteTests_NewAPI.test_char
test_char - test.test_capi.test_structmembers.ReadWriteTests_OldAPI.test_char

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel8-s390x.lto/build/Lib/test/test_capi/test_structmembers.py", line 170, in test_char
    self.assertEqual(ts.T_CHAR, "c")
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
AssertionError: '\x00' != 'c'
- �
+ c

vstinner · 2024-12-03T17:00:00Z

@dpdani @colesbury: As you can see in previous comments, test_capi fails on multiple buildbots. I'm not sure why.

dpdani · 2024-12-03T17:08:43Z

mm, I can't seem to reproduce it locally. maybe some platform compatibility issue?
bedevere reported here failures only for s390x, but I don't have easy access to that.

what do you usually do in these situations?

colesbury · 2024-12-03T17:16:56Z

Let's skip the test on s390x for now while we investigate.

I'm not sure what's causing the failure. The only thing I can think of is that char is unsigned on s390x (vs. signed on GCC x86-64 and arm64).

colesbury · 2024-12-03T19:18:09Z

#127577 should fix it. The problem was that the 'C' format code expects an int as the destination (not a char):

https://docs.python.org/3/c-api/arg.html#c.Py_BuildValue

This happens to work out okay on little endian systems because the non-zero part of the value is at the same place (the first byte), and it just writes some zeros to the struct's padding bytes. On big endian systems, like s390x, it would write zero to char_member and some non-zero value to parts of the padding bytes.

I followed the instructions on https://docs.gitlab.com/omnibus/development/s390x.html to debug s390x using Docker and QEMU. I had to slightly modify their commands: in particular, I had to specify --platform linux/s390x:

docker run --rm --privileged multiarch/qemu-user-static --reset -p yes
docker run --platform linux/s390x -v $(pwd):/cpython --rm -it s390x/ubuntu

dpdani · 2024-12-04T09:54:35Z

oops, my bad

thank you for all the details 🙏

dpdani added 2 commits August 21, 2024 18:08

pythongh-117657: TSAN Fix races in PyMember_Get and PyMember_Set,…

d63eccb

… for C extensions

exercise race in T_CHAR as well

c2d3d41

bedevere-app bot added the awaiting review label Aug 21, 2024

bedevere-app bot mentioned this pull request Aug 21, 2024

Make TSAN tests pass with the GIL disabled in free-threaded builds #117657

Open

dpdani added 21 commits August 22, 2024 15:13

signed char races

f6ba4a3

unsigned char race

d9614b7

unsigned char race 👀

1659849

unsigned char race 👀

4cfa619

signed short race

107a9f5

unsigned short race

e20b9d1

signed int race

86a0945

who uses MSC anyways?

1a07031

unsigned int races

16f61d8

👀

cf25726

signed long race

d41132c

unsigned long race

2212605

ssizet race

74616d6

float race

23d10c6

double race

ae37a8e

T_CHAR

bce02d0

signed long long race

afe2504

unsigned long long race

3dfaab8

fix return type

50a7bb6

arm64 does not have 128-bit integers, I suppose

416e0bb

at this point I'm just making guesses

4bd6927

picnixz reviewed Aug 22, 2024

View reviewed changes

Include/cpython/pyatomic.h Show resolved Hide resolved

Include/internal/pycore_pyatomic_ft_wrappers.h Outdated Show resolved Hide resolved

colesbury added the skip news label Aug 23, 2024

colesbury reviewed Aug 23, 2024

View reviewed changes

Python/structmember.c Outdated Show resolved Hide resolved

Python/structmember.c Outdated Show resolved Hide resolved

Python/structmember.c Outdated Show resolved Hide resolved

colesbury reviewed Sep 16, 2024

View reviewed changes

dpdani and others added 3 commits September 22, 2024 12:02

Apply suggestions from code review

761257e

Co-authored-by: Sam Gross <[email protected]>

missing load

d045747

fixes

c1b4c35

dpdani requested a review from colesbury November 2, 2024 17:54

colesbury approved these changes Nov 4, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Nov 4, 2024

Merge branch 'main' into pythongh-117657-tsan-pymember-c-ext

6c5cec5

colesbury merged commit 979bf24 into python:main Dec 3, 2024
57 of 61 checks passed

bedevere-app bot removed the awaiting merge label Dec 3, 2024

colesbury mentioned this pull request Dec 3, 2024

test_char in test_structmembers fails on s390x #127572

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-117657: TSAN Fix races in `PyMember_Get` and `PyMember_Set`, for C extensions #123211

gh-117657: TSAN Fix races in `PyMember_Get` and `PyMember_Set`, for C extensions #123211

dpdani commented Aug 21, 2024 •

edited by bedevere-app bot

Loading

picnixz left a comment

dpdani commented Aug 22, 2024

picnixz commented Aug 22, 2024 •

edited

Loading

colesbury left a comment

dpdani commented Sep 16, 2024

colesbury left a comment

dpdani commented Sep 16, 2024

dpdani commented Nov 2, 2024

colesbury left a comment

colesbury commented Dec 3, 2024

bedevere-bot commented Dec 3, 2024

colesbury commented Dec 3, 2024

bedevere-bot commented Dec 3, 2024

bedevere-bot commented Dec 3, 2024

dpdani commented Dec 3, 2024

bedevere-bot commented Dec 3, 2024

bedevere-bot commented Dec 3, 2024

bedevere-bot commented Dec 3, 2024

bedevere-bot commented Dec 3, 2024

vstinner commented Dec 3, 2024

dpdani commented Dec 3, 2024

colesbury commented Dec 3, 2024

colesbury commented Dec 3, 2024

dpdani commented Dec 4, 2024

gh-117657: TSAN Fix races in PyMember_Get and PyMember_Set, for C extensions #123211

gh-117657: TSAN Fix races in PyMember_Get and PyMember_Set, for C extensions #123211

Conversation

dpdani commented Aug 21, 2024 • edited by bedevere-app bot Loading

Py_T_CHAR

picnixz left a comment

Choose a reason for hiding this comment

dpdani commented Aug 22, 2024

picnixz commented Aug 22, 2024 • edited Loading

colesbury left a comment

Choose a reason for hiding this comment

dpdani commented Sep 16, 2024

colesbury left a comment

Choose a reason for hiding this comment

dpdani commented Sep 16, 2024

dpdani commented Nov 2, 2024

colesbury left a comment

Choose a reason for hiding this comment

colesbury commented Dec 3, 2024

bedevere-bot commented Dec 3, 2024

colesbury commented Dec 3, 2024

bedevere-bot commented Dec 3, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

bedevere-bot commented Dec 3, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

dpdani commented Dec 3, 2024

bedevere-bot commented Dec 3, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

bedevere-bot commented Dec 3, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

bedevere-bot commented Dec 3, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

bedevere-bot commented Dec 3, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

vstinner commented Dec 3, 2024

dpdani commented Dec 3, 2024

colesbury commented Dec 3, 2024

colesbury commented Dec 3, 2024

dpdani commented Dec 4, 2024

gh-117657: TSAN Fix races in `PyMember_Get` and `PyMember_Set`, for C extensions #123211

gh-117657: TSAN Fix races in `PyMember_Get` and `PyMember_Set`, for C extensions #123211

dpdani commented Aug 21, 2024 •

edited by bedevere-app bot

Loading

picnixz commented Aug 22, 2024 •

edited

Loading