Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST

[stratakis]

BPO	35257
Nosy	@vstinner, @ned-deily, @merwok, @dstufft, @stratakis, @hroncok, @miss-islington
PRs	bpo-35257: Avoid leaking the linker flags into distutils. #10900 [WIP] bpo-35502: Fix memory leak in xml.etree iterparse() #11169 [3.7] bpo-35257: Avoid leaking LTO linker flags into distutils (GH-10900) #11264 [3.6] bpo-35257: Avoid leaking LTO linker flags into distutils (GH-10900) #11265 bpo-35257: fix broken BLDSHARED - needs LDFLAGS too #11297 [3.7] bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297) #11298 [3.6] bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297) #11299
Files	demo.c setup.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-12-20.20:39:51.124>
created_at = <Date 2018-11-15.16:14:01.289>
labels = ['extension-modules', '3.8', 'build', 'library', '3.7']
title = 'Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST'
updated_at = <Date 2019-01-07.11:54:37.112>
user = 'https://github.com/stratakis'

bugs.python.org fields:

activity = <Date 2019-01-07.11:54:37.112>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-12-20.20:39:51.124>
closer = 'ned.deily'
components = ['Build', 'Distutils', 'Extension Modules']
creation = <Date 2018-11-15.16:14:01.289>
creator = 'cstratak'
dependencies = []
files = ['47976', '47977']
hgrepos = []
issue_num = 35257
keywords = ['patch']
message_count = 28.0
messages = ['329951', '329952', '331119', '331122', '331177', '331419', '331472', '331540', '331845', '331852', '332150', '332153', '332155', '332227', '332228', '332229', '332230', '332232', '332233', '332234', '332257', '332259', '332466', '332467', '332469', '332470', '332490', '333145']
nosy_count = 7.0
nosy_names = ['vstinner', 'ned.deily', 'eric.araujo', 'dstufft', 'cstratak', 'hroncok', 'miss-islington']
pr_nums = ['10900', '11169', '11264', '11265', '11297', '11298', '11299']
priority = None
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue35257'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[stratakis]

Through acb8c52 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?

[stratakis]

Correction: The second commit is referring to #9908

[stratakis]

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

[stratakis]

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

[stratakis]

PR has been finalized.

[ned-deily]

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.

[ned-deily]

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.

[stratakis]

The PR is pending another round of review.

[vstinner]

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

[vstinner]

See also:

• bpo-35499: "make profile-opt" overrides CFLAGS_NODIST
• bpo-35501: "make coverage" should use leak coverage flags to third party C extensions.

[vstinner]

TL; DR PR 10900 passed my manual tests ;-)

I made some tests on PR 10900, commit d1655f9, 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!

[vstinner]

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

[vstinner]

New changeset cf10a75 by Victor Stinner (stratakis) in branch 'master':
bpo-35257: Avoid leaking LTO linker flags into distutils (GH-10900)
cf10a75

[stratakis]

This change fixes a regression introduced in 3.6.8rc1 with https://bugs.python.org/issue31354

[stratakis]

And also 3.7.2rc1

[vstinner]

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

[stratakis]

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.

[vstinner]

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!

[vstinner]

New changeset 0198f52 by Victor Stinner in branch '3.7':
bpo-35257: Avoid leaking LTO linker flags into distutils (GH-10900) (GH-11264)
0198f52

[vstinner]

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!

[ned-deily]

Also cherry-picked to 3.6 in commit a21bedf
[3.6] bpo-35257: Avoid leaking LTO linker flags into distutils (GH-10900) (GH-11265)

[vstinner]

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

[miss-islington]

New changeset 44a3ee0 by Miss Islington (bot) (Ned Deily) in branch 'master':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297)
44a3ee0

[miss-islington]

New changeset 44a3ee0 by Miss Islington (bot) (Ned Deily) in branch 'master':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297)
44a3ee0

[miss-islington]

New changeset 3d4b4b8 by Miss Islington (bot) in branch '3.7':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297)
3d4b4b8

[ned-deily]

New changeset 68f5dfd by Ned Deily (Miss Islington (bot)) in branch '3.6':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297) (GH-11299)
68f5dfd

[ned-deily]

New changeset f14087a by Ned Deily (Victor Stinner) in branch '3.7':
bpo-35257: Avoid leaking LTO linker flags into distutils (GH-10900) (GH-11264)
f14087a

New changeset 92f9024 by Ned Deily (Miss Islington (bot)) in branch '3.7':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297)
92f9024

[vstinner]

New changeset 92f9024 by Ned Deily (Miss Islington (bot)) in branch '3.7':
bpo-35257: fix broken BLDSHARED - needs LDFLAGS too (GH-11297)
92f9024

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.