Skip to content

Conversation

@Eclips4
Copy link
Member

@Eclips4 Eclips4 commented Jun 10, 2024

@Eclips4
Copy link
Member Author

Eclips4 commented Jun 10, 2024

@cdgriffith can you confirm that this resolves the issue?

@AlexWaygood AlexWaygood changed the title gh-120326: Include intrih.h on Windows gh-120326: Include intrin.h on Windows Jun 10, 2024
@cdgriffith
Copy link
Contributor

@Eclips4 The build completes, and produces a 3.14 executable (assuming this will also be merged into 3.13).

Thank you for fast fix!

@Eclips4 Eclips4 added the needs backport to 3.13 bugs and security fixes label Jun 10, 2024
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM, I just left minor suggestions.

Eclips4 and others added 2 commits June 11, 2024 16:12
…DF1.rst Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
@Eclips4
Copy link
Member Author

Eclips4 commented Jun 11, 2024

Thanks for the review, Victor.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Oh wait, now I recall vaguely a C++20 module issue if the include is done inside a extern "C" { ... }. Would it be possible to move the include to Python.h, and add a comment explaining why it's needed, mention at least one function which is used?

Also, is this include only needed for Free Threading, or for JIT compiler, or both?

@vstinner vstinner enabled auto-merge (squash) June 11, 2024 15:43
@vstinner
Copy link
Member

Thanks, I enabled auto-merge.

For example, I see that _Py_ThreadId() uses:

#if defined(_MSC_VER) && defined(_M_X64) tid = __readgsqword(48); 
@Eclips4 Eclips4 disabled auto-merge June 11, 2024 16:18
@Eclips4
Copy link
Member Author

Eclips4 commented Jun 11, 2024

@vstinner I've disabled automerge due to failure of "Tests / Windows (free-threading) / build and test (x86) (pull_request)" job (see https://github.com/python/cpython/actions/runs/9468440435/attempts/1?pr=120329) . I've re-run it and seems it will pass (UPD: Yes, it is).

I don't know if it is related to this pull request, but it is definitely something strange, and I am not sure we can merge it now. However, I cannot reproduce the failure from the job locally on my Windows setup :(

@vstinner vstinner merged commit 939c201 into python:main Jun 11, 2024
@miss-islington-app
Copy link

Thanks @Eclips4 for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 11, 2024
…ythonGH-120329) (cherry picked from commit 939c201) Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
@bedevere-app
Copy link

bedevere-app bot commented Jun 11, 2024

GH-120363 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 11, 2024
@vstinner
Copy link
Member

@vstinner I've disabled automerge due to failure of "Tests / Windows (free-threading) / build and test (x86) (pull_request)" job (see https://github.com/python/cpython/actions/runs/9468440435/attempts/1?pr=120329) . I've re-run it and seems it will pass (UPD: Yes, it is).

I don't see how an include can trigger this issue. It's strange since the job passed when re-run.

@vstinner
Copy link
Member

Oh, the x86 job failed again on the commit checks: https://github.com/python/cpython/actions/runs/9470417218/job/26091279722

Run .\python.bat -m test.pythoninfo Running Debug|Win32 interpreter... Error: Process completed with exit code 1. 
@vstinner
Copy link
Member

I built Python with Free Threading on Windows for x86: I failed to reproduce the issue.

I ran PCbuild\build.bat -e -d -p x86 --disable-gil to build Python, and then I ran python.bat -m test.pythoninfo.

@vstinner
Copy link
Member

Windows x86 Free Threading failed 2x in a row, and then succeeded 5x in a row. This bug is strange. I don't know how to debug it since I cannot reproduce it locally.

@vstinner vstinner added the needs backport to 3.13 bugs and security fixes label Jun 12, 2024
@miss-islington-app
Copy link

Thanks @Eclips4 for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 12, 2024
…ythonGH-120329) (cherry picked from commit 939c201) Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
@bedevere-app
Copy link

bedevere-app bot commented Jun 12, 2024

GH-120414 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 12, 2024
vstinner pushed a commit that referenced this pull request Jun 12, 2024
…H-120329) (#120414) gh-120326: Include <intrin.h> on Windows with Free Threading (GH-120329) (cherry picked from commit 939c201) Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
@Eclips4 Eclips4 deleted the issue-120326 branch June 15, 2024 16:13
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants