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

gh-120326: Include intrin.h on Windows #120329

Merged
merged 5 commits into from
Jun 11, 2024
Merged

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
Include/object.h Outdated Show resolved Hide resolved
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
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
36 checks passed
@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
@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
@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 <[email protected]>
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants