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-111650: Ensure pyconfig.h includes Py_GIL_DISABLED on Windows #112778

Merged
merged 10 commits into from
Dec 13, 2023

Conversation

zooba
Copy link
Member

@zooba zooba commented Dec 5, 2023

@@ -739,4 +739,7 @@ Py_NO_ENABLE_SHARED to find out. Also support MS_NO_COREDLL for b/w compat */
/* Define if libssl has X509_VERIFY_PARAM_set1_host and related function */
#define HAVE_X509_VERIFY_PARAM_SET1_HOST 1

/* Define if you want to disable the GIL */
#undef Py_GIL_DISABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to rename the PC/pyconfig.h? It seems like some tools might treat the input template as if it's the true pyconfig.h file.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not actually a template, it's a totally valid and usable header file. In the default case (for now), Py_GIL_DISABLED is undefined, and we just change that on build if requested.

For editing in an IDE, we need some kind of actual header file available or else all the code assistance tools will fail. Rather than requiring the IDE understand the build directory (which VS can do, but others may not), this allows us to have a default GIL_DISABLED header before building and then the correct one when actually building.

PCbuild/pyproject.props Show resolved Hide resolved
@zooba
Copy link
Member Author

zooba commented Dec 7, 2023

The failure in the nogil build is an access violation/segfault (see the end of the build step where we get the real exit code - 0xC0000005), and it doesn't repro on my own machine. I've merged from master again in case it was unlucky and a re-run helps, but otherwise this could be a tricky one to diagnose.

@colesbury
Copy link
Contributor

@zooba, I'll see if I can reproduce it locally

@brettcannon brettcannon removed their request for review December 7, 2023 18:58
@colesbury
Copy link
Contributor

The files in PC/ are preferring PC/pyconfig.h over the generated one.

@zooba
Copy link
Member Author

zooba commented Dec 7, 2023

Ah, that makes sense. Easily fixed

@zooba
Copy link
Member Author

zooba commented Dec 7, 2023

Actually, I can't see how that happens unless your build differs from mine. (Side note - we should enable verbose output in CI so that we could see exactly what was happening there... the default output isn't detailed enough.)

What is happening is that the extension modules that are outside the core DLL are getting the unmodified header, because they have independent $(IntDir) directories that don't contain the updated one.

I'll add a new project just to do the header file update, so that way the dependencies/incremental build can work properly and we don't have to repeat the process for every module.

@colesbury
Copy link
Contributor

That's happening too for me (later on in the build), but files like PC\config.c, PC\dl_nt.c, PC\msvcrtmodule.c, PC\winreg.c are pulling the wrong pyconfig.h and they are part of pythoncore.vcxproj.

@zooba
Copy link
Member Author

zooba commented Dec 7, 2023

There shouldn't be a case where include "Python.h" -> include "pyconfig.h" searches the source file's directory. It'll look adjacent to Python.h and then start going through INCLUDE, which will have the generated directory earlier than PC/.

@zooba
Copy link
Member Author

zooba commented Dec 7, 2023

There shouldn't be a case where include "Python.h" -> include "pyconfig.h" searches the source file's directory.

And yet, /sourceDependencies says otherwise, so I guess the docs are wrong or incomplete here.

I'll hack things together a different way.

@zooba
Copy link
Member Author

zooba commented Dec 8, 2023

I fixed the test, and found that the crash is because operatormodule.m_index is being set to the same as m_name, which skips assigning the type in PyModuleDef_Init and then null refs during the typecheck in create_builtin (in import.c).

I don't see any obvious reason for the failure, and _operator isn't the first module listed in config.c, but it seems to reliably be that module. I won't get a chance to dig into this for a bit, you might want to look first @colesbury

@colesbury
Copy link
Contributor

@zooba - I haven't seen the operatormodule.m_index issue either locally or in the GH CI. However, there are a few more #ifndef _MSC_VER guards that need to be removed and an #include "pyconfig.h" needed: colesbury@6158d6e

With that change I'm able to build --disable-gil locally without any compilation warnings and the tests pass.

@colesbury
Copy link
Contributor

However, test_peg_generator still fails because setuptools needs updating. That's only run in buildbots (not GH) and was broken before this PR.

.\PCbuild\amd64\python_d.exe -m test -u all test_peg_generator -v
...
C:\Users\Administrator\cpython\include\Python.h(14): fatal error C1083: Cannot open include file: 'pyconfig.h': No such file or directory
ERROR
...

@zooba
Copy link
Member Author

zooba commented Dec 11, 2023

Yeah, apparently it works today? I see the last push I did also didn't crash. Hopefully it's not uninitialised memory, as that would be annoying to track down.

@colesbury
Copy link
Contributor

!buildbot windows

@bedevere-bot
Copy link

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

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

The builders matched are:

  • AMD64 Windows11 Bigmem PR
  • AMD64 Windows Server 2022 NoGIL PR
  • AMD64 Windows11 Refleaks PR
  • AMD64 Windows10 PR
  • ARM64 Windows Non-Debug PR
  • AMD64 Windows11 Non-Debug PR
  • ARM64 Windows PR

@colesbury
Copy link
Contributor

It looks like test_peg_generator is failing due to setuptools (and not just on the "nogil" builder as I originally thought). I'm not sure of the best way to fix it. Maybe we can add the intermediates directory to the include path?

include_dirs = [
str(MOD_DIR.parent.parent.parent / "Include" / "internal"),
str(MOD_DIR.parent.parent.parent / "Parser"),
str(MOD_DIR.parent.parent.parent / "Parser" / "lexer"),
str(MOD_DIR.parent.parent.parent / "Parser" / "tokenizer"),
]

@zooba
Copy link
Member Author

zooba commented Dec 11, 2023

Output directory is the one to add (dirname(sys.executable) or dirname(sys._base_executable) if there's a venv involved unless it's a symlinked venv in which case I think we just lose?)

I do intend to send a PR to setuptools for this, I just haven't yet, and I'd be more inclined to skip the test until that's in. Or if we've actually vendored setuptools, we can just patch our copy for now.1 I'm pretty sure setuptools trusts us enough to merge the change before we merge ours, but I wouldn't blame them if they didn't - it's not their circular dependency.

Footnotes

  1. FTR, this is the kind of scenario that has me -1 on using 3rd-party dependencies in our test suite.

@zooba
Copy link
Member Author

zooba commented Dec 11, 2023

unless it's a symlinked venv in which case I think we just lose?

My guess is that this is due to #86179 which I should probably just fix (pretty sure it only needs a realpath call in venv).

@zooba
Copy link
Member Author

zooba commented Dec 12, 2023

Issue and PR for setuptools are now submitted.

@zooba
Copy link
Member Author

zooba commented Dec 12, 2023

!buildbot windows

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @zooba for commit c74133b 🤖

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

The builders matched are:

  • AMD64 Windows11 Bigmem PR
  • AMD64 Windows Server 2022 NoGIL PR
  • AMD64 Windows11 Refleaks PR
  • AMD64 Windows10 PR
  • ARM64 Windows Non-Debug PR
  • AMD64 Windows11 Non-Debug PR
  • ARM64 Windows PR

@zooba
Copy link
Member Author

zooba commented Dec 13, 2023

@colesbury Anything else you think should be handled in this PR? Once this is in (and my other one to fix symlinks and simplify the new sysconfig code) I've got a broader range of build changes to make, so we likely won't have to live with this particular code for long.

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.

Looks great!

@@ -16,7 +16,7 @@ jobs:
steps:
- uses: actions/checkout@v4
- name: Build CPython
run: .\PCbuild\build.bat -e -d -p Win32 ${{ inputs.free-threaded && '--disable-gil' || '' }}
run: .\PCbuild\build.bat -e -d -v -p Win32 ${{ inputs.free-threaded && '--disable-gil' || '' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you want to keep this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it slows down CI enough to be a problem.

@zooba zooba merged commit 79dad03 into python:main Dec 13, 2023
46 checks passed
@zooba zooba deleted the gh-111650 branch December 13, 2023 15:38
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