msg329951 - (view) |
Author: Charalampos Stratakis (cstratak) * |
Date: 2018-11-15 16:14 |
Through acb8c5234302f8057b331abaafb2cc8697daf58f the CFLAGS_NODIST variable was created, in order to place there compiler flags used by the interpreter, but not intended to be propagated to C extensions.
I saw a similar issue when working on backporting 67e997bcfdac55191033d57a16d1408aL1313 on python 3.6, where the -flto flag should be passed to CFLAGS_NODIST instead of BASECFLAGS, however even if that is fixed, the LDFLAGS will still be propagated to C extensions.
Thus in order to provide more flexibility in that regard, I propose to add the LDFLAGS_NODIST variable, which in a similar vein as CFLAGS_NODIST, will hold the LDFLAGS intended to be used only by the interpreter.
Thoughts or comments on this approach?
|
msg329952 - (view) |
Author: Charalampos Stratakis (cstratak) * |
Date: 2018-11-15 16:15 |
Correction: The second commit is referring to https://github.com/python/cpython/pull/9908/
|
msg331119 - (view) |
Author: Charalampos Stratakis (cstratak) * |
Date: 2018-12-05 14:43 |
So to better illustrate the actual issue I'll be using an example from the python documentation [0][1].
Get the demo.c and the setup.py. Compile cpython first with --with-lto and then compile the demo.c with ./python3 setup.py build.
You will notice that various link time optimization linker flags are passed to the extension.
[0] https://docs.python.org/3/extending/extending.html#keyword-parameters-for-extension-functions
[1] https://docs.python.org/3/extending/building.html#building-c-and-c-extensions-with-distutils
|
msg331122 - (view) |
Author: Charalampos Stratakis (cstratak) * |
Date: 2018-12-05 14:44 |
And here is the difference between compiling the extension with the current tip, comparing to applying my current draft PR:
Master branch with the linker flags propagated:
running build
running build_ext
building 'demo' extension
creating build
creating build/temp.linux-x86_64-3.8
gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/Harris/dev/cpython/_install/include/python3.8m -c demo.c -o build/temp.linux-x86_64-3.8/demo.o
creating build/lib.linux-x86_64-3.8
gcc -pthread -shared -flto -fuse-linker-plugin -ffat-lto-objects -flto-partition=none -g build/temp.linux-x86_64-3.8/demo.o -o build/lib.linux-x86_64-3.8/demo.cpython-38m-x86_64-linux-gnu.so
With introduction of the LDFLAGS_NODIST variable:
running build
running build_ext
building 'demo' extension
creating build
creating build/temp.linux-x86_64-3.8
gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/Harris/dev/cpython/_install/include/python3.8m -c demo.c -o build/temp.linux-x86_64-3.8/demo.o
creating build/lib.linux-x86_64-3.8
gcc -pthread -shared build/temp.linux-x86_64-3.8/demo.o -o build/lib.linux-x86_64-3.8/demo.cpython-38m-x86_64-linux-gnu.so
|
msg331177 - (view) |
Author: Charalampos Stratakis (cstratak) * |
Date: 2018-12-05 21:44 |
PR has been finalized.
|
msg331419 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2018-12-09 09:16 |
Unfortunately, it appears this won't be resolved in time for 3.7.2rc1 and 3.6.8rc1 and it would not be appropriate to merge something this potentially disruptive without prior exposure. Since 3.6.8 is planned to be the final 3.6.x bugfix release, unless some critical problem is discovered prior to their final releases it's likely this will not be fixed for 3.6.
|
msg331472 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2018-12-10 07:21 |
> Unfortunately, it appears this won't be resolved in time for 3.7.2rc1 and 3.6.8rc1
An update: the cutoff for these releases has been extended until about 30 hours from now so there is perhaps a small chance that the PR for this could still be updated, reviewed, and merged in time. If not, it can wait.
|
msg331540 - (view) |
Author: Charalampos Stratakis (cstratak) * |
Date: 2018-12-10 21:45 |
The PR is pending another round of review.
|
msg331845 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-12-14 18:07 |
There are multiple ways to configure and build Python, we should try most combinations:
* ./configure --enable-shared
* ./configure --with-lto
* ./configure --enable-optimizations
* make profile-opt
* make
* Maybe also: make install
Test:
* Build Python and make sure that python binary and C extensions (of the stdlib) are compiled with LTO
* python-config --cflags and python-config --ldflags don't leak LTO flags
* Build a C extension (Pillow) and check that there is no LTO flag in the command lines
I'm not how to test cross-compilation :-(
|
msg331852 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-12-14 18:55 |
See also:
* bpo-35499: "make profile-opt" overrides CFLAGS_NODIST
* bpo-35501: "make coverage" should use leak coverage flags to third party C extensions.
|
msg332150 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-12-19 16:38 |
TL; DR PR 10900 passed my manual tests ;-)
I made some tests on PR 10900, commit d1655f939d0eeeca24c2a4fee635da087e0d499b, using 3 shell scripts.
(1) step1.sh:
--
set -x -e
git clean -fdx
./configure CC=clang --with-lto --prefix /opt/py38
make 2>&1|tee log
grep -E -- '-o python|-o Python/bltinmodule.o|Modules/_asynciomodule.o' log|grep -c lto
# 4 expected: -flto passed to compiler *and* linker
--
(2) step2.sh:
--
set -e -x
rm -rf /opt/py38
mkdir /opt/py38
make install
/opt/py38/bin/python3.8-config --cflags --ldflags --libs|grep -c lto ||:
# "0" expected here: no LTO
LD_LIBRARY_PATH=/opt/py38/lib /opt/py38/bin/python3.8 -c 'import sysconfig; print("lto" in sysconfig.get_config_var("LDFLAGS"))'
# "False" expected: no LTO in LDFLAGS
--
(3) step3.sh:
--
set -e -x
tar -xf ../Pillow-5.3.0.tar.gz
cd Pillow-5.3.0/
LD_LIBRARY_PATH=/opt/py38/lib /opt/py38/bin/python3.8 setup.py install 2>&1|tee log
grep -c lto log
--
Get Pillow tarball using:
wget https://files.pythonhosted.org/packages/1b/e1/1118d60e9946e4e77872b69c58bc2f28448ec02c99a2ce456cd1a272c5fd/Pillow-5.3.0.tar.gz
== master ==
master branch, ./configure CC=clang --with-lto:
(1) 4
(2) 0, True => ERR
(3) 5 => ERR
master branch, ./configure CC=clang --with-lto --enable-shared:
(1) 4
(2) 0, True => ERR
(3) 5 => ERR
== PR ==
With PR 10900, ./configure CC=clang --with-lto:
(1) 4
(2) 0, False => OK!
(3) 0 => OK!
With PR 10900, ./configure CC=clang --with-lto --enable-shared:
(1) 4
(2) 0, False => OK!
(3) 0 => OK!
|
msg332153 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-12-19 17:05 |
PGO+LTO build with PR 10900: I see PGO options (-fprofile-instr-generate then -fprofile-instr-use=code.profclangd) and LTO option (-flto) passed to the compiler and to the linker, as expected:
$ git clean -fdx
$ ./configure CC=clang --with-lto --prefix /opt/py38 --enable-optimizations
$ sed -i -e 's/^PROFILE_TASK=.*/PROFILE_TASK=-c pass/' Makefile
$ make
...
# compile Python core
clang ... -flto ... -fprofile-instr-generate ... Modules/main.c
...
# link ./python program
clang -pthread -flto -g -fprofile-instr-generate -Xlinker -export-dynamic -o python Programs/python.o libpython3.8m.a -lpthread -ldl -lutil -lm -lm
...
# compile stdlib C extension
clang ... -flto ... -fprofile-instr-generate ... -c /home/vstinner/prog/python/master/Modules/_heapqmodule.c ...
...
rm -f profile-gen-stamp
...
# compile Python core
clang ... -flto ... -fprofile-instr-use=code.profclangd ... -o Programs/python.o ./Programs/python.c
...
# link ./python program
clang -pthread -flto -g -Xlinker -export-dynamic -o python Programs/python.o libpython3.8m.a -lpthread -ldl -lutil -lm -lm
...
# build struct extension
clang ... -flto ... -fprofile-instr-use=code.profclangd ... -c /home/vstinner/prog/python/master/Modules/_struct.c -o build/temp.linux-x86_64-3.8/home/vstinner/prog/python/master/Modules/_struct.o
warning: no profile data available for file "_struct.c" [-Wprofile-instr-unprofiled]
1 warning generated.
clang -pthread -shared -flto -g build/temp.linux-x86_64-3.8/home/vstinner/prog/python/master/Modules/_struct.o -L/opt/py38/lib -L/usr/local/lib -o build/lib.linux-x86_64-3.8/_struct.cpython-38m-x86_64-linux-gnu.so
...
|
msg332155 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-12-19 17:19 |
New changeset cf10a750f4b50b6775719cfb17bee00bc3a9c60b by Victor Stinner (stratakis) in branch 'master':
bpo-35257: Avoid leaking LTO linker flags into distutils (GH-10900)
https://github.com/python/cpython/commit/cf10a750f4b50b6775719cfb17bee00bc3a9c60b
|
msg332227 - (view) |
Author: Charalampos Stratakis (cstratak) * |
Date: 2018-12-20 14:12 |
This change fixes a regression introduced in 3.6.8rc1 with https://bugs.python.org/issue31354
|
msg332228 - (view) |
Author: Charalampos Stratakis (cstratak) * |
Date: 2018-12-20 14:13 |
And also 3.7.2rc1
|
msg332229 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-12-20 14:20 |
Ned: We have to do something with this issue before 3.6 and 3.7 releases. Either revert or backport fixes (merge PR 11264 and PR 11265).
|
msg332230 - (view) |
Author: Charalampos Stratakis (cstratak) * |
Date: 2018-12-20 14:30 |
Small correction. This regression has been on 3.7 for some time now (since it was the master branch then), but then I requested to have the buggy commit backported to 3.6 to fix the --with-lto flag there. Which unfortunately introduced the issue to 3.6 anyway. Master branch is fixed now and backports are open for 3.7 and 3.6 if those are to be accepted by the release manager.
|
msg332232 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-12-20 15:02 |
I repeated the same tests for Python 3.7 on PR 11264.
I replaced "38" with "37" and "3.8" and 3.7" and my 3 scripts :-) I modified step1.sh for PGO+LTO with -O0:
set -x -e
git clean -fdx
./configure CC=clang --with-lto --prefix /opt/py37 --enable-optimizations
sed -i -e 's/^PROFILE_TASK=.*/PROFILE_TASK=-c pass/' Makefile
sed -i -e 's/-O3/-O0/' Makefile
make 2>&1|tee log
grep -E -- '-o python|-o Python/bltinmodule.o|Modules/_asynciomodule.o' log|grep -c lto
# 4 expected: -flto passed to compiler *and* linker
Test results:
./configure CC=clang --with-lto --prefix /opt/py38
(1) 4
(2) 0, False => OK!
(3) 0 => OK!
./configure CC=clang --with-lto --enable-shared
(1) 4
(2) 0, False => OK!
(3) 0 => OK!
./configure CC=clang --with-lto --enable-optimizations
(1) 8 => OK!
(2) 0, False => OK!
(3) 0 => OK!
|
msg332233 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-12-20 15:03 |
New changeset 0198f52ea2328fd932622ad2299381f617a041f2 by Victor Stinner in branch '3.7':
bpo-35257: Avoid leaking LTO linker flags into distutils (GH-10900) (GH-11264)
https://github.com/python/cpython/commit/0198f52ea2328fd932622ad2299381f617a041f2
|
msg332234 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-12-20 15:24 |
I repeated the same tests for Python 3.6 on PR 11265: all tests are ok!
I replaced "38" with "36" and "3.8" and 3.6" and my 3 scripts :-) I modified step1.sh for PGO+LTO and to compile with -O0 (just to make tests faster): similar script than in my previous message.
./configure CC=clang --with-lto --prefix /opt/py36
(1) 4 => OK!
(2) 0, False => OK!
(3) 0 => OK!
./configure CC=clang --with-lto --prefix /opt/py36 --enable-shared
(1) 4 => OK!
(2) 0, False => OK!
(3) 0 => OK!
./configure CC=clang --with-lto --prefix /opt/py36 --enable-optimizations
(1) 8 => OK!
(2) 0, False => OK!
(3) 0 => OK!
|
msg332257 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2018-12-20 20:39 |
Also cherry-picked to 3.6 in commit a21bedf9edeacce6f0de3b2df0783e2ad6aa3b18
[3.6] bpo-35257: Avoid leaking LTO linker flags into distutils (GH-10900) (GH-11265)
|
msg332259 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-12-20 20:56 |
Just in case, I checked again 3.6 and 3.7 branches with "./configure CC=clang --with-lto --enable-shared": they still pass my manual tests ;-)
|
msg332466 - (view) |
Author: miss-islington (miss-islington) |
Date: 2018-12-24 15:36 |
New changeset 44a3ee07e30e18d83e2730c093d8b0e930f0a06c by Miss Islington (bot) (Ned Deily) in branch 'master':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297)
https://github.com/python/cpython/commit/44a3ee07e30e18d83e2730c093d8b0e930f0a06c
|
msg332467 - (view) |
Author: miss-islington (miss-islington) |
Date: 2018-12-24 15:36 |
New changeset 44a3ee07e30e18d83e2730c093d8b0e930f0a06c by Miss Islington (bot) (Ned Deily) in branch 'master':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297)
https://github.com/python/cpython/commit/44a3ee07e30e18d83e2730c093d8b0e930f0a06c
|
msg332469 - (view) |
Author: miss-islington (miss-islington) |
Date: 2018-12-24 15:40 |
New changeset 3d4b4b80f290e622b05ca219ad6dabc07b49421a by Miss Islington (bot) in branch '3.7':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297)
https://github.com/python/cpython/commit/3d4b4b80f290e622b05ca219ad6dabc07b49421a
|
msg332470 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2018-12-24 15:41 |
New changeset 68f5dfd955ee7a16fabf90d7c370159cc0ca9b75 by Ned Deily (Miss Islington (bot)) in branch '3.6':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297) (GH-11299)
https://github.com/python/cpython/commit/68f5dfd955ee7a16fabf90d7c370159cc0ca9b75
|
msg332490 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2018-12-24 16:32 |
New changeset f14087a4a96bb3f500e2d2bbc36c1124e90d5f73 by Ned Deily (Victor Stinner) in branch '3.7':
bpo-35257: Avoid leaking LTO linker flags into distutils (GH-10900) (GH-11264)
https://github.com/python/cpython/commit/f14087a4a96bb3f500e2d2bbc36c1124e90d5f73
New changeset 92f90242994652d6b7afe0a2c8a61cf2bccc8c32 by Ned Deily (Miss Islington (bot)) in branch '3.7':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297)
https://github.com/python/cpython/commit/92f90242994652d6b7afe0a2c8a61cf2bccc8c32
|
msg333145 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-01-07 11:54 |
New changeset 92f90242994652d6b7afe0a2c8a61cf2bccc8c32 by Ned Deily (Miss Islington (bot)) in branch '3.7':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297)
https://github.com/python/cpython/commit/92f90242994652d6b7afe0a2c8a61cf2bccc8c32
Since Ned pushed a new fix, I ran again my tests on the 3.7 branch with ./configure CC=clang --with-lto --prefix /opt/py37 --enable-shared:
(1) 4 => ok
(2) 0, False => ok
(3) 0 => ok
Good! It works (in my tests).
--
"make sharedmods" runs "LDSHARED=$BLDSHARED (...) ./python setup.py build"
"make libpython(...)" uses $(BLDSHARED) to link libpython.
Modules/makesetup uses BLDSHARED to build C extensions of the stdlib. Example from generated Makefile:
"Modules/posix$(EXT_SUFFIX): Modules/posixmodule.o; $(BLDSHARED) Modules/posixmodule.o -o Modules/posix$(EXT_SUFFIX) (...)"
distutils has a customize_compiler() function which uses LDSHARED from Makefile (can be overriden by environment variables): it doesn't use BLDSHARED.
It seems like BLDSHARED is only used to build Python itself and stdlib modules. So I agree that it's ok to add PY_LDFLAGS_NODIST flags to BLDSHARED.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:08 | admin | set | github: 79438 |
2019-01-07 11:54:37 | vstinner | set | messages:
+ msg333145 |
2018-12-24 16:32:58 | ned.deily | set | messages:
+ msg332490 |
2018-12-24 15:41:43 | ned.deily | set | messages:
+ msg332470 |
2018-12-24 15:40:51 | miss-islington | set | messages:
+ msg332469 |
2018-12-24 15:38:43 | miss-islington | set | pull_requests:
+ pull_request10534 |
2018-12-24 15:37:24 | miss-islington | set | pull_requests:
+ pull_request10533 |
2018-12-24 15:36:54 | miss-islington | set | messages:
+ msg332467 |
2018-12-24 15:36:18 | miss-islington | set | nosy:
+ miss-islington messages:
+ msg332466
|
2018-12-24 15:34:44 | ned.deily | set | pull_requests:
+ pull_request10532 |
2018-12-20 20:56:27 | vstinner | set | messages:
+ msg332259 |
2018-12-20 20:39:51 | ned.deily | set | status: open -> closed priority: release blocker -> messages:
+ msg332257
resolution: fixed stage: patch review -> resolved |
2018-12-20 15:24:26 | vstinner | set | messages:
+ msg332234 |
2018-12-20 15:03:04 | vstinner | set | messages:
+ msg332233 |
2018-12-20 15:02:36 | vstinner | set | messages:
+ msg332232 |
2018-12-20 14:30:59 | cstratak | set | messages:
+ msg332230 |
2018-12-20 14:20:15 | vstinner | set | priority: critical -> release blocker
messages:
+ msg332229 |
2018-12-20 14:18:34 | vstinner | set | pull_requests:
+ pull_request10496 |
2018-12-20 14:13:07 | cstratak | set | messages:
+ msg332228 |
2018-12-20 14:12:44 | cstratak | set | messages:
+ msg332227 |
2018-12-20 14:07:13 | vstinner | set | pull_requests:
+ pull_request10495 |
2018-12-19 17:19:05 | vstinner | set | messages:
+ msg332155 |
2018-12-19 17:05:29 | vstinner | set | messages:
+ msg332153 |
2018-12-19 16:38:36 | vstinner | set | messages:
+ msg332150 |
2018-12-14 20:41:59 | vstinner | set | pull_requests:
+ pull_request10406 |
2018-12-14 18:55:07 | vstinner | set | messages:
+ msg331852 |
2018-12-14 18:07:58 | vstinner | set | messages:
+ msg331845 |
2018-12-10 21:45:05 | cstratak | set | messages:
+ msg331540 |
2018-12-10 11:15:58 | vstinner | set | title: Avoid leaking linker flags into distutils. -> Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST |
2018-12-10 07:21:53 | ned.deily | set | messages:
+ msg331472 |
2018-12-09 09:16:15 | ned.deily | set | priority: normal -> critical nosy:
+ ned.deily messages:
+ msg331419
|
2018-12-05 21:44:57 | cstratak | set | title: Add LDFLAGS_NODIST for the LDFLAGS not intended for propagation to C extensions. -> Avoid leaking linker flags into distutils. |
2018-12-05 21:44:29 | cstratak | set | nosy:
+ vstinner messages:
+ msg331177
|
2018-12-05 14:50:33 | hroncok | set | nosy:
+ hroncok
|
2018-12-05 14:44:59 | cstratak | set | messages:
+ msg331122 |
2018-12-05 14:43:26 | cstratak | set | files:
+ setup.py |
2018-12-05 14:43:17 | cstratak | set | files:
+ demo.c
messages:
+ msg331119 |
2018-12-04 16:53:04 | cstratak | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request10140 |
2018-11-15 16:15:34 | cstratak | set | messages:
+ msg329952 |
2018-11-15 16:14:01 | cstratak | create | |