classification
Title: Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST
Type: Stage: resolved
Components: Build, Distutils, Extension Modules Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: cstratak, dstufft, eric.araujo, hroncok, miss-islington, ned.deily, vstinner
Priority: Keywords: patch

Created on 2018-11-15 16:14 by cstratak, last changed 2019-01-07 11:54 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
demo.c cstratak, 2018-12-05 14:43
setup.py cstratak, 2018-12-05 14:43
Pull Requests
URL Status Linked Edit
PR 10900 merged cstratak, 2018-12-04 16:53
PR 11169 closed vstinner, 2018-12-14 20:41
PR 11264 merged vstinner, 2018-12-20 14:07
PR 11265 merged vstinner, 2018-12-20 14:18
PR 11297 merged ned.deily, 2018-12-24 15:34
PR 11298 merged miss-islington, 2018-12-24 15:37
PR 11299 merged miss-islington, 2018-12-24 15:38
Messages (28)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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.
History
Date User Action Args
2019-01-07 11:54:37vstinnersetmessages: + msg333145
2018-12-24 16:32:58ned.deilysetmessages: + msg332490
2018-12-24 15:41:43ned.deilysetmessages: + msg332470
2018-12-24 15:40:51miss-islingtonsetmessages: + msg332469
2018-12-24 15:38:43miss-islingtonsetpull_requests: + pull_request10534
2018-12-24 15:37:24miss-islingtonsetpull_requests: + pull_request10533
2018-12-24 15:36:54miss-islingtonsetmessages: + msg332467
2018-12-24 15:36:18miss-islingtonsetnosy: + miss-islington
messages: + msg332466
2018-12-24 15:34:44ned.deilysetpull_requests: + pull_request10532
2018-12-20 20:56:27vstinnersetmessages: + msg332259
2018-12-20 20:39:51ned.deilysetstatus: open -> closed
priority: release blocker ->
messages: + msg332257

resolution: fixed
stage: patch review -> resolved
2018-12-20 15:24:26vstinnersetmessages: + msg332234
2018-12-20 15:03:04vstinnersetmessages: + msg332233
2018-12-20 15:02:36vstinnersetmessages: + msg332232
2018-12-20 14:30:59cstrataksetmessages: + msg332230
2018-12-20 14:20:15vstinnersetpriority: critical -> release blocker

messages: + msg332229
2018-12-20 14:18:34vstinnersetpull_requests: + pull_request10496
2018-12-20 14:13:07cstrataksetmessages: + msg332228
2018-12-20 14:12:44cstrataksetmessages: + msg332227
2018-12-20 14:07:13vstinnersetpull_requests: + pull_request10495
2018-12-19 17:19:05vstinnersetmessages: + msg332155
2018-12-19 17:05:29vstinnersetmessages: + msg332153
2018-12-19 16:38:36vstinnersetmessages: + msg332150
2018-12-14 20:41:59vstinnersetpull_requests: + pull_request10406
2018-12-14 18:55:07vstinnersetmessages: + msg331852
2018-12-14 18:07:58vstinnersetmessages: + msg331845
2018-12-10 21:45:05cstrataksetmessages: + msg331540
2018-12-10 11:15:58vstinnersettitle: Avoid leaking linker flags into distutils. -> Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST
2018-12-10 07:21:53ned.deilysetmessages: + msg331472
2018-12-09 09:16:15ned.deilysetpriority: normal -> critical
nosy: + ned.deily
messages: + msg331419

2018-12-05 21:44:57cstrataksettitle: Add LDFLAGS_NODIST for the LDFLAGS not intended for propagation to C extensions. -> Avoid leaking linker flags into distutils.
2018-12-05 21:44:29cstrataksetnosy: + vstinner
messages: + msg331177
2018-12-05 14:50:33hroncoksetnosy: + hroncok
2018-12-05 14:44:59cstrataksetmessages: + msg331122
2018-12-05 14:43:26cstrataksetfiles: + setup.py
2018-12-05 14:43:17cstrataksetfiles: + demo.c

messages: + msg331119
2018-12-04 16:53:04cstrataksetkeywords: + patch
stage: patch review
pull_requests: + pull_request10140
2018-11-15 16:15:34cstrataksetmessages: + msg329952
2018-11-15 16:14:01cstratakcreate