classification
Title: building extensions as builtins is broken in 3.7
Type: compile error Stage: resolved
Components: Build Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: doko, eric.snow, miss-islington, ncoghlan, ned.deily, pablogsal, vstinner, xdegaye
Priority: release blocker Keywords: patch

Created on 2017-12-06 13:03 by doko, last changed 2018-04-20 16:16 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
ext-as-builtins.diff doko, 2017-12-06 14:53 proposed patch
py_config_macros.py xdegaye, 2018-04-20 14:33
Pull Requests
URL Status Linked Edit
PR 5256 closed pablogsal, 2018-01-21 03:41
PR 6489 merged xdegaye, 2018-04-16 22:59
PR 6547 merged miss-islington, 2018-04-20 15:04
Messages (15)
msg307735 - (view) Author: Matthias Klose (doko) * (Python committer) Date: 2017-12-06 13:03
building extensions statically in linking these into the python binary is currently broken on 3.7.

I'm attaching the change that worked for me in 3.7alpha2, but doesn't work anymore with alpha3. Currently investigating.  When building extensions as builtins, these should benefit from the knowledge about the core interpreter.

x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -g -fdebug-prefix-map=/home/packages/python/3.7/python3.7-3.7.0~a3=. -fstack-protector-strong -Wformat -Werror=format-security    -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration  -IObjects -IInclude -IPython -I. -I../Include -Wdate-time -D_FORTIFY_SOURCE=2   -DPy_BUILD_CORE  -c ../Modules/_elementtree.c -o Modules/_elementtree.o
In file included from ../Include/pyatomic.h:10:0,
                 from ../Include/Python.h:53,
                 from ../Modules/_elementtree.c:16:
../Modules/_elementtree.c: In function 'element_dealloc':
../Include/pystate.h:314:34: error: '_PyRuntime' undeclared (first use in this function); did you mean 'PyRun_File'?
 #  define _PyThreadState_Current _PyRuntime.gilstate.tstate_current
                                  ^
../Include/pyatomic.h:533:5: note: in expansion of macro '_Py_atomic_load_explicit'
     _Py_atomic_load_explicit(ATOMIC_VAL, _Py_memory_order_relaxed)
     ^~~~~~~~~~~~~~~~~~~~~~~~
../Include/pystate.h:316:31: note: in expansion of macro '_Py_atomic_load_relaxed'
              ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current))
                               ^~~~~~~~~~~~~~~~~~~~~~~
../Include/pystate.h:316:56: note: in expansion of macro '_PyThreadState_Current'
              ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current))
                                                        ^~~~~~~~~~~~~~~~~~~~~~
../Include/object.h:1069:34: note: in expansion of macro 'PyThreadState_GET'
         PyThreadState *_tstate = PyThreadState_GET(); \
                                  ^~~~~~~~~~~~~~~~~
../Modules/_elementtree.c:634:5: note: in expansion of macro 'Py_TRASHCAN_SAFE_BEGIN'
     Py_TRASHCAN_SAFE_BEGIN(self)
     ^~~~~~~~~~~~~~~~~~~~~~
../Include/pystate.h:314:34: note: each undeclared identifier is reported only once for each function it appears in
 #  define _PyThreadState_Current _PyRuntime.gilstate.tstate_current
                                  ^
../Include/pyatomic.h:533:5: note: in expansion of macro '_Py_atomic_load_explicit'
     _Py_atomic_load_explicit(ATOMIC_VAL, _Py_memory_order_relaxed)
     ^~~~~~~~~~~~~~~~~~~~~~~~
../Include/pystate.h:316:31: note: in expansion of macro '_Py_atomic_load_relaxed'
              ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current))
                               ^~~~~~~~~~~~~~~~~~~~~~~
../Include/pystate.h:316:56: note: in expansion of macro '_PyThreadState_Current'
              ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current))
                                                        ^~~~~~~~~~~~~~~~~~~~~~
../Include/object.h:1069:34: note: in expansion of macro 'PyThreadState_GET'
         PyThreadState *_tstate = PyThreadState_GET(); \
                                  ^~~~~~~~~~~~~~~~~
../Modules/_elementtree.c:634:5: note: in expansion of macro 'Py_TRASHCAN_SAFE_BEGIN'
     Py_TRASHCAN_SAFE_BEGIN(self)
     ^~~~~~~~~~~~~~~~~~~~~~
../Include/pyatomic.h:56:5: error: '__atomic_load_ptr' undeclared (first use in this function); did you mean '__atomic_load_n'?
     atomic_load_explicit(&(ATOMIC_VAL)->_value, ORDER)
     ^
../Include/pyatomic.h:533:5: note: in expansion of macro '_Py_atomic_load_explicit'
     _Py_atomic_load_explicit(ATOMIC_VAL, _Py_memory_order_relaxed)
     ^~~~~~~~~~~~~~~~~~~~~~~~
../Include/pystate.h:316:31: note: in expansion of macro '_Py_atomic_load_relaxed'
              ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current))
                               ^~~~~~~~~~~~~~~~~~~~~~~
msg307741 - (view) Author: Matthias Klose (doko) * (Python committer) Date: 2017-12-06 14:53
that patch lets the build succeed. two additional includes and a little bit of shuffling
msg309682 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-01-09 02:19
Can someone provide a pull request and a test for this prior to beta 1?
msg310365 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2018-01-21 03:43
I have put together a PR with Matthias' patch and a test.
msg310366 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2018-01-21 04:26
It seems that Microsoft compiler does not handle well the circular dependencies between Include/pystate.h and Include/internal/pystate.h so I have included guards for Windows in the new include for Include/pystate.h and deactivated the test in that case. Some better ideas on how to handle that case?
msg311161 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-01-29 19:43
Thanks Pablo for taking up the challenge!  I just took a quick look at the current state of PR 5256 and I noted numerous issues that need to be addressed with the test.  I don't have time at the moment to review the non-test changes.  It would be good if another core developer could review it.  Since it's not likely this will get resolved prior to the imminent 3.7.0b1 code freeze, I'm going to reluctantly defer this to b2 (unless someone else gets to it first).
msg315368 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2018-04-16 23:04
The compilation failure is a consequence of the changes made in issue 30860. Simply adding '#include "internal/pystate.h"' in Modules/_elementtree.c fixes the compilation although this is not the correct fix.

The modules configured in Modules/Setup keep being built with -DPy_BUILD_CORE while the refactoring done in issue 30860 imposes new constraints on the way headers are handled for modules accessing the Py_BUILD_CORE API. Most modules configured in Modules/Setup do not use this API and none of the commented out modules in this file (normally built by setup.py [1]) does. PR 6489 fixes this by introducing yet another CFLAGS named PY_NO_CORE_CFLAGS to only use -DPy_BUILD_CORE with Setup modules that use the Py_BUILD_CORE API.

[1] the _xxsubinterpreters module is the only one that sets -DPy_BUILD_CORE explicitly in setup.py
msg315370 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-04-16 23:13
Thanks, Xavier, for your analysis and your PR!  We definitely meed to get this resolved before 3.7.0b4.  Given the complexity and potential impact on this area and that we are approaching the final beta, I think we need a few pairs of eyes to review it.  @doko, does the PR work for you?  @eric.snow and @ncoghlan, could you please take a look, too?  Thanks!
msg315391 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-04-17 10:55
Even when statically linked, extension modules should *NOT* be getting built with `-DPy_BUILD_CORE`. That preprocessor definition is for CPython core core only, and we moved the headers to help make that 100% crystal clear.
msg315392 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-04-17 10:56
Ah, oops - I commented before fully catching up on everything else, and now I see that Xavier already made exactly that point just above.

I'll go review the PR now :)
msg315515 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2018-04-20 14:33
To answer my own comment in PR 6489 about the interference of the pyconfig.h macros with the build of std lib extension modules, the py_config_macros.py script prints the list of pyconfig.h.in macros that are being used by the standard library extension modules that do not access the interpreter internals. The script prints:

errnomodule.c: const

So the conclusion is that this is not a problem.
msg315516 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-04-20 14:58
I also resolved my own puzzlement from the PR comments regarding why _weakrefs.c was fine with being built as a regular extension module: it's because the C level access API for __weakrefs__ is published through the regular C API, not as a core-only internal API.

So it wouldn't work if you tried to build it against the stable ABI (since it needs access to object internals), but it's fine without Py_BUILD_CORE.
msg315517 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-04-20 15:03
New changeset 063db62aab4041ac47798e7e48b27b2f2bef21c5 by Nick Coghlan (xdegaye) in branch 'master':
bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE (GH-6489)
https://github.com/python/cpython/commit/063db62aab4041ac47798e7e48b27b2f2bef21c5
msg315518 - (view) Author: miss-islington (miss-islington) Date: 2018-04-20 15:39
New changeset 18cd82815b9bdeabc5ac03aa7f58102109ccf368 by Miss Islington (bot) in branch '3.7':
bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE (GH-6489)
https://github.com/python/cpython/commit/18cd82815b9bdeabc5ac03aa7f58102109ccf368
msg315519 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-04-20 16:16
Xavier's changes should fix the reported compile error, while keeping the increased isolation of the interpreter core from the optional extension modules.

If the latter change causes a detectable performance regression, then I think that would make more sense as a separate performance issue.
History
Date User Action Args
2018-04-20 16:16:02ncoghlansetstatus: open -> closed
type: compile error
messages: + msg315519

resolution: fixed
stage: patch review -> resolved
2018-04-20 15:39:27miss-islingtonsetnosy: + miss-islington
messages: + msg315518
2018-04-20 15:04:18miss-islingtonsetpull_requests: + pull_request6244
2018-04-20 15:03:51ncoghlansetmessages: + msg315517
2018-04-20 14:58:54ncoghlansetmessages: + msg315516
2018-04-20 14:33:06xdegayesetfiles: + py_config_macros.py

messages: + msg315515
2018-04-17 10:56:52ncoghlansetmessages: + msg315392
2018-04-17 10:55:01ncoghlansetmessages: + msg315391
2018-04-16 23:13:54ned.deilysetpriority: deferred blocker -> release blocker
nosy: + vstinner
messages: + msg315370

2018-04-16 23:04:42xdegayesetnosy: + xdegaye
messages: + msg315368
2018-04-16 22:59:49xdegayesetpull_requests: + pull_request6188
2018-01-29 19:43:30ned.deilysetpriority: release blocker -> deferred blocker

messages: + msg311161
versions: + Python 3.8
2018-01-21 04:26:27pablogsalsetmessages: + msg310366
2018-01-21 03:43:32pablogsalsetnosy: + pablogsal
messages: + msg310365
2018-01-21 03:41:37pablogsalsetstage: patch review
pull_requests: + pull_request5103
2018-01-09 02:19:51ned.deilysetmessages: + msg309682
2017-12-06 16:21:18ned.deilysetpriority: normal -> release blocker
nosy: + ned.deily
2017-12-06 14:53:01dokosetfiles: + ext-as-builtins.diff
keywords: + patch
messages: + msg307741
2017-12-06 13:07:28vstinnersetnosy: + ncoghlan, eric.snow
2017-12-06 13:03:40dokocreate