gh-109693: Update _gil_runtime_state.locked to use pyatomic.h #110836

corona10 · 2023-10-13T15:06:03Z

Issue: Clean-up pyatomic headers #109693

vstinner

LGTM.

@colesbury: Do you want to review pyatomic.h changes?

Include/cpython/pyatomic_msc.h

vstinner

Windows logs many compiler warnings, see: https://github.com/python/cpython/pull/110836/files

colesbury

Can you also add a test to Modules/_testcapi/pyatomic.c for the new functions?

Include/cpython/pyatomic_msc.h

Include/cpython/pyatomic_std.h

Python/ceval_gil.c

colesbury

Two more comments I missed in the first pass. When you get everything working to your satisfaction, I'd suggest testing the PR on all the buildbots as there is some platforms specific code.

Include/cpython/pyatomic_msc.h

corona10 · 2023-10-13T15:39:12Z

Two more comments I missed in the first pass. When you get everything working to your satisfaction, I'd suggest testing the PR on all the buildbots as there is some platforms specific code.

Got it

corona10 · 2023-10-13T17:15:02Z

Convert to the draft PR since I need to check many things.

bedevere-bot · 2023-10-13T17:59:57Z

🤖 New build scheduled with the buildbot fleet by @corona10 for commit b5d0539 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

vstinner · 2023-10-13T23:15:38Z

You can use !buildbot PATTERN to only run tests on a specific CI rather pulling the huge number of CI, it may be faster as well. Tell me of you need help about that.

corona10 · 2023-10-15T11:18:51Z

@colesbury @vstinner
Okay, the failed test(test_exception) on build bots are not directly related to this PR. (I monitored another build, and it happens)

So can you please take a look?

corona10 · 2023-10-15T11:21:34Z

Include/cpython/pyatomic_msc.h

+_Py_atomic_store_int_release(int *obj, int value)
+{
+#if defined(_M_X64) || defined(_M_IX86)
+    *(int volatile *)obj = value;


@colesbury
Basically, int volatile * and volatile int* have the same meaning.
So I just follow the convention from _Py_atomic_load_ptr_acquire

Modules/_testcapi/pyatomic.c

Python/ceval_gil.c

Include/cpython/pyatomic_msc.h

vstinner

LGTM

corona10 · 2023-10-16T13:44:13Z

Thanks @vstinner , let's wait @colesbury too :)

…ythongh-110836)

corona10 added 2 commits October 14, 2023 00:03

pythongh-109693: Update _gil_runtime_state.locked to use pyatomic.h

1eb7914

nit

6ad8d1b

corona10 requested review from vstinner and colesbury October 13, 2023 15:06

bedevere-app bot mentioned this pull request Oct 13, 2023

Clean-up pyatomic headers #109693

Closed

bedevere-app bot added the awaiting core review label Oct 13, 2023

corona10 added the skip news label Oct 13, 2023

vstinner approved these changes Oct 13, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Oct 13, 2023

fix

7624da8

vstinner reviewed Oct 13, 2023

View reviewed changes

Include/cpython/pyatomic_msc.h Outdated Show resolved Hide resolved

corona10 added 2 commits October 14, 2023 00:14

fix

efe778d

fix

869a350

vstinner reviewed Oct 13, 2023

View reviewed changes

corona10 mentioned this pull request Oct 13, 2023

gh-109693: Update pyruntimestate._finalizing to use pyatomic.h #110837

Merged

fix

64196a8

colesbury reviewed Oct 13, 2023

View reviewed changes

corona10 added 3 commits October 14, 2023 00:33

Address code review

bd7d3b1

Address code review

06847b6

Address code review

39989fc

colesbury reviewed Oct 13, 2023

View reviewed changes

Include/cpython/pyatomic_msc.h Outdated Show resolved Hide resolved

Include/cpython/pyatomic_msc.h Outdated Show resolved Hide resolved

corona10 added 2 commits October 14, 2023 00:40

Address code review

48cb663

Add test code

89aa5b7

corona10 changed the title ~~gh-109693: Update _gil_runtime_state.locked to use pyatomic.h~~ [WIP] gh-109693: Update _gil_runtime_state.locked to use pyatomic.h Oct 13, 2023

nit

c0bdbc6

corona10 changed the title ~~[WIP] gh-109693: Update _gil_runtime_state.locked to use pyatomic.h~~ gh-109693: Update _gil_runtime_state.locked to use pyatomic.h Oct 13, 2023

fix

fffdcff

corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 13, 2023

corona10 marked this pull request as draft October 13, 2023 17:15

bedevere-app bot removed the awaiting merge label Oct 13, 2023

corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 13, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 13, 2023

corona10 marked this pull request as ready for review October 15, 2023 11:17

bedevere-app bot added the awaiting core review label Oct 15, 2023

corona10 changed the title ~~[WIP] gh-109693: Update _gil_runtime_state.locked to use pyatomic.h~~ gh-109693: Update _gil_runtime_state.locked to use pyatomic.h Oct 15, 2023

corona10 commented Oct 15, 2023

View reviewed changes

corona10 requested review from colesbury and vstinner October 15, 2023 11:32

vstinner reviewed Oct 16, 2023

View reviewed changes

Modules/_testcapi/pyatomic.c Outdated Show resolved Hide resolved

Python/ceval_gil.c Outdated Show resolved Hide resolved

corona10 added 2 commits October 16, 2023 22:23

fix test

e37e80d

Address code review

044570e

vstinner reviewed Oct 16, 2023

View reviewed changes

Include/cpython/pyatomic_msc.h Show resolved Hide resolved

corona10 added 2 commits October 16, 2023 22:35

Address code review

b067e13

Update

c58dfc8

vstinner approved these changes Oct 16, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Oct 16, 2023

colesbury approved these changes Oct 16, 2023

View reviewed changes

corona10 merged commit 86559dd into python:main Oct 16, 2023

bedevere-app bot removed the awaiting merge label Oct 16, 2023

corona10 deleted the gh-109693-gil-api branch October 16, 2023 22:32

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-109693: Update _gil_runtime_state.locked to use pyatomic.h (p…

7c29d25

…ythongh-110836)

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-109693: Update _gil_runtime_state.locked to use pyatomic.h (p…

8bd0584

…ythongh-110836)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-109693: Update _gil_runtime_state.locked to use pyatomic.h #110836

gh-109693: Update _gil_runtime_state.locked to use pyatomic.h #110836

corona10 commented Oct 13, 2023 •

edited by bedevere-app bot

Loading

vstinner left a comment

vstinner left a comment

colesbury left a comment

colesbury left a comment

corona10 commented Oct 13, 2023

corona10 commented Oct 13, 2023 •

edited

Loading

bedevere-bot commented Oct 13, 2023

vstinner commented Oct 13, 2023

corona10 commented Oct 15, 2023 •

edited

Loading

corona10 Oct 15, 2023 •

edited

Loading

vstinner left a comment

corona10 commented Oct 16, 2023

gh-109693: Update _gil_runtime_state.locked to use pyatomic.h #110836

gh-109693: Update _gil_runtime_state.locked to use pyatomic.h #110836

Conversation

corona10 commented Oct 13, 2023 • edited by bedevere-app bot Loading

vstinner left a comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

colesbury left a comment

Choose a reason for hiding this comment

colesbury left a comment

Choose a reason for hiding this comment

corona10 commented Oct 13, 2023

corona10 commented Oct 13, 2023 • edited Loading

bedevere-bot commented Oct 13, 2023

vstinner commented Oct 13, 2023

corona10 commented Oct 15, 2023 • edited Loading

corona10 Oct 15, 2023 • edited Loading

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

corona10 commented Oct 16, 2023

corona10 commented Oct 13, 2023 •

edited by bedevere-app bot

Loading

corona10 commented Oct 13, 2023 •

edited

Loading

corona10 commented Oct 15, 2023 •

edited

Loading

corona10 Oct 15, 2023 •

edited

Loading