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

building extensions as builtins is broken in 3.7 #76413

Closed
doko42 opened this issue Dec 6, 2017 · 15 comments
Closed

building extensions as builtins is broken in 3.7 #76413

doko42 opened this issue Dec 6, 2017 · 15 comments
Labels
3.7 (EOL) end of life 3.8 (EOL) end of life build The build process and cross-build release-blocker

Comments

@doko42
Copy link
Member

doko42 commented Dec 6, 2017

BPO 32232
Nosy @doko42, @ncoghlan, @vstinner, @ned-deily, @xdegaye, @ericsnowcurrently, @pablogsal, @miss-islington
PRs
  • bpo-32232: Fix "building extensions as builtins is broken in 3.7" #5256
  • bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE #6489
  • [3.7] bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE (GH-6489) #6547
  • Files
  • ext-as-builtins.diff: proposed patch
  • py_config_macros.py
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-04-20.16:16:02.391>
    created_at = <Date 2017-12-06.13:03:40.132>
    labels = ['3.8', 'build', '3.7', 'release-blocker']
    title = 'building extensions as builtins is broken in 3.7'
    updated_at = <Date 2018-04-20.16:16:02.389>
    user = 'https://github.com/doko42'

    bugs.python.org fields:

    activity = <Date 2018-04-20.16:16:02.389>
    actor = 'ncoghlan'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-04-20.16:16:02.391>
    closer = 'ncoghlan'
    components = ['Build']
    creation = <Date 2017-12-06.13:03:40.132>
    creator = 'doko'
    dependencies = []
    files = ['47325', '47544']
    hgrepos = []
    issue_num = 32232
    keywords = ['patch']
    message_count = 15.0
    messages = ['307735', '307741', '309682', '310365', '310366', '311161', '315368', '315370', '315391', '315392', '315515', '315516', '315517', '315518', '315519']
    nosy_count = 8.0
    nosy_names = ['doko', 'ncoghlan', 'vstinner', 'ned.deily', 'xdegaye', 'eric.snow', 'pablogsal', 'miss-islington']
    pr_nums = ['5256', '6489', '6547']
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'compile error'
    url = 'https://bugs.python.org/issue32232'
    versions = ['Python 3.7', 'Python 3.8']

    @doko42
    Copy link
    Member Author

    doko42 commented Dec 6, 2017

    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))
    ^~~~~~~~~~~~~~~~~~~~~~~

    @doko42 doko42 added 3.7 (EOL) end of life build The build process and cross-build labels Dec 6, 2017
    @doko42
    Copy link
    Member Author

    doko42 commented Dec 6, 2017

    that patch lets the build succeed. two additional includes and a little bit of shuffling

    @ned-deily
    Copy link
    Member

    Can someone provide a pull request and a test for this prior to beta 1?

    @pablogsal
    Copy link
    Member

    I have put together a PR with Matthias' patch and a test.

    @pablogsal
    Copy link
    Member

    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?

    @ned-deily
    Copy link
    Member

    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).

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Apr 16, 2018

    The compilation failure is a consequence of the changes made in bpo-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 bpo-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

    @ned-deily
    Copy link
    Member

    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!

    @ncoghlan
    Copy link
    Contributor

    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.

    @ncoghlan
    Copy link
    Contributor

    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 :)

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Apr 20, 2018

    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.

    @ncoghlan
    Copy link
    Contributor

    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.

    @ncoghlan
    Copy link
    Contributor

    New changeset 063db62 by Nick Coghlan (xdegaye) in branch 'master':
    bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE (GH-6489)
    063db62

    @miss-islington
    Copy link
    Contributor

    New changeset 18cd828 by Miss Islington (bot) in branch '3.7':
    bpo-32232: by default, Setup modules are no longer built with -DPy_BUILD_CORE (GH-6489)
    18cd828

    @ncoghlan
    Copy link
    Contributor

    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.

    @ncoghlan ncoghlan added the build The build process and cross-build label Apr 20, 2018
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 (EOL) end of life build The build process and cross-build release-blocker
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants