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

Fixing a bug related to LTO only build #75535

Closed
octaviansoldea mannequin opened this issue Sep 5, 2017 · 11 comments
Closed

Fixing a bug related to LTO only build #75535

octaviansoldea mannequin opened this issue Sep 5, 2017 · 11 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes build The build process and cross-build type-feature A feature request or enhancement

Comments

@octaviansoldea
Copy link
Mannequin

octaviansoldea mannequin commented Sep 5, 2017

BPO 31354
Nosy @gpshead, @vstinner, @ned-deily, @stratakis, @octaviansoldea
PRs
  • bpo-31354: Fixing a bug related to LTO only build #3110
  • [3.6] bpo-31354: Let configure --with-lto work on all builds #10261
  • 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 = 'https://github.com/gpshead'
    closed_at = <Date 2018-12-09.09:08:30.867>
    created_at = <Date 2017-09-05.21:54:13.298>
    labels = ['3.8', 'type-feature', '3.7', 'build']
    title = 'Fixing a bug related to LTO only build'
    updated_at = <Date 2018-12-09.09:08:30.865>
    user = 'https://github.com/octaviansoldea'

    bugs.python.org fields:

    activity = <Date 2018-12-09.09:08:30.865>
    actor = 'ned.deily'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2018-12-09.09:08:30.867>
    closer = 'ned.deily'
    components = ['Build']
    creation = <Date 2017-09-05.21:54:13.298>
    creator = 'octavian.soldea'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31354
    keywords = []
    message_count = 11.0
    messages = ['301381', '301385', '301722', '329444', '329844', '329876', '329897', '329925', '331346', '331351', '331418']
    nosy_count = 5.0
    nosy_names = ['gregory.p.smith', 'vstinner', 'ned.deily', 'cstratak', 'octavian.soldea']
    pr_nums = ['3110', '10261']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue31354'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    @octaviansoldea
    Copy link
    Mannequin Author

    octaviansoldea mannequin commented Sep 5, 2017

    Hi All,

    This is Octavian Soldea from the Scripting Languages Optimization team at Intel Corporation. I would like to submit a patch to fix an LTO build issue. With "./configure --with-lto" followed by "make", CPython should be built with LTO or Link-Time-Optimization. However, currently, no impact is made to the build process, meaning the flag is not effective. This patch has been created out of a python-dev discussion thread: http://www.mail-archive.com/python-dev@python.org/msg96469.html. The LTO flag was originally introduced by my team member Alecsandru Patrascu back in 2015: https://bugs.python.org/issue25702.

    Before the change:

    ./configure --with-lto
    make
    gcc command line output example:
    gcc -pthread -c -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -I. -I./Include -DPy_BUILD_CORE -o Python/ceval.o Python/ceval.c
    Note no "lto" flag was present. This is the same result as the default build without the LTO flag.

    After changes:

    ./configure --with-lto
    make
    expected gcc command line output example =>
    gcc -pthread -c -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -flto -fuse-linker-plugin -ffat-lto-objects -flto-partition=none -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -I. -I./Include -DPy_BUILD_CORE -o Python/ceval.o Python/ceval.c

    Thanks,
    Octavian

    @octaviansoldea octaviansoldea mannequin added 3.7 (EOL) end of life build The build process and cross-build type-feature A feature request or enhancement labels Sep 5, 2017
    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2017

    Would you mind to create a pull request?

    @gpshead
    Copy link
    Member

    gpshead commented Sep 8, 2017

    New changeset 4c81401 by Gregory P. Smith (octaviansoldea) in branch 'master':
    bpo-31354: Let configure --with-lto work on all builds
    4c81401

    @gpshead gpshead closed this as completed Sep 8, 2017
    @gpshead gpshead self-assigned this Sep 8, 2017
    @vstinner
    Copy link
    Member

    vstinner commented Nov 7, 2018

    New changeset f6b9459 by Victor Stinner (stratakis) in branch '3.6':
    [3.6] bpo-31354: Let configure --with-lto work on all builds (GH-10261)
    f6b9459

    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented Nov 13, 2018

    This change exports -flto in cflags.

    You can check it with python3-config --cflags after this commit.

    Which means that every c extension which will use those cflags will also utilize -flto which wasn't happening before, thus I'd say it's a regression.

    @gpshead
    Copy link
    Member

    gpshead commented Nov 14, 2018

    Is it an actual problem to compile extension modules with -flto? (I realize as an extension module there isn't a huge benefit to the concept unless it happens to have multiple source files or link against a non-shared library)

    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented Nov 14, 2018

    I'd say yes. In general python may have been compiled with -flto, but it's still a bit buggy from the compilers' side. It doesn't work well always or at all depending on the toolchain, and even if python was compiled with -flto, that doesn't mean that all c extensions compiled using that python build, would work properly with -flto.

    In my opinion that should be a flag reserved for python only, if used for its compilation, and not having it propagated to c extensions, similarly to the PGO related flags.

    @ned-deily
    Copy link
    Member

    This does sound like a regression bug that should be fixed in all branches. Another issue is that, if it is propagated to third-party extension module builds through distutils, there is no guarantee that the extension module is being built with the same compiler as Python itself was.

    @ned-deily ned-deily reopened this Nov 14, 2018
    @gpshead gpshead added release-blocker 3.8 only security fixes labels Nov 14, 2018
    @ned-deily
    Copy link
    Member

    This issue is still open and blocking the upcoming 3.7.2rc1 and 3.6.8rc1 releases.

    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented Dec 7, 2018

    Hi Ned,

    I recently pushed a fix on the master and 3.7 for this exact issue: https://bugs.python.org/issue35351 but it builds on top of #10922 which is not yet in 3.6.

    Thus 3.7 is fine for the rc cutoff.

    However there is another side effect of that change with the linker flags also leaking: https://bugs.python.org/issue35257 but it proved to be more complicated than what I initially thought to fix it.

    PR here: #10900

    The issue is that I figured it wouldn't make it to the 3.6 branch as it would require extensive review due to the complexity of the build system and the requirement for extensive review to catch any possible regressions, thus it would be late for the rc cutoff.

    It would take some days to figure out everything I believe. Would it be possible to push that after rc1? Or maybe not fixing that would be better, deadline wise? I consider this an important issue but some more feedback is always welcome.

    Side note on your comment: The compiler used (CC) also seems to leak into distutils instead of the system default CC, on linux with 'python3 setup.py build'.

    @ned-deily
    Copy link
    Member

    OK, I think we are in better shape for 3.6.8 now. I've merged the backports to 3.6 of bpo-28015 / PR 9908 (LTO with clang) and bpo-35351 / PR 10797 (prevent LTO CFLAGS leakage into distutils). So, at the moment, master, 3.7, and 3.6 should behave the same in this area, although I'm a little bit leery of having all this backporting just before 3.6 stops accepting bugfixes. As far as I can tell, what remains is resolving bpo-35257 (prevent LTO LDFLAGS leakage into distutils).

    Side note on your comment: The compiler used (CC) also seems to leak
    into distutils instead of the system default CC, on linux with
    'python3 setup.py build'.

    Yes, it does by design as do many other of the common build Makefile / environment variables. The point I was trying to make applies more to binary distributions of Python, like with the python.org macOS installer. In cases like that, the binary distribution is designed to install and run on multiple OS versions and that means that the compiler, and OS version for that matter, used to build the python executable and standard library extension modules are often very different from those on the end user's system when building third-party extension modules.

    @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 only security fixes build The build process and cross-build type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants