Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST #79438

stratakis · 2018-11-15T16:14:01Z

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 · 2018-11-15T16:14:01Z

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 · 2018-11-15T16:15:35Z

Correction: The second commit is referring to #9908

stratakis · 2018-12-05T14:43:17Z

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 · 2018-12-05T14:44:59Z

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 · 2018-12-05T21:44:29Z

PR has been finalized.

ned-deily · 2018-12-09T09:16:15Z

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 · 2018-12-10T07:21:53Z

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 · 2018-12-10T21:45:05Z

The PR is pending another round of review.

vstinner · 2018-12-14T18:07:58Z

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 · 2018-12-14T18:55:08Z

(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 · 2018-12-19T17:05:29Z

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 · 2018-12-19T17:19:06Z

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

stratakis · 2018-12-20T14:12:44Z

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

stratakis · 2018-12-20T14:13:07Z

And also 3.7.2rc1

vstinner · 2018-12-20T14:20:15Z

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 · 2018-12-20T14:30:59Z

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 · 2018-12-20T15:02:36Z

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 · 2018-12-20T15:03:05Z

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 · 2018-12-20T15:24:26Z

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 · 2018-12-20T20:39:51Z

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 · 2018-12-20T20:56:27Z

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 · 2018-12-24T15:36:19Z

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 · 2018-12-24T15:36:54Z

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 · 2018-12-24T15:40:51Z

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

ned-deily · 2018-12-24T15:41:43Z

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 · 2018-12-24T16:32:59Z

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 · 2019-01-07T11:54:37Z

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.

stratakis mannequin added 3.7 (EOL) end of life 3.8 only security fixes build The build process and cross-build stdlib Python modules in the Lib dir extension-modules C modules in the Modules dir labels Nov 15, 2018

stratakis mannequin changed the title ~~Add LDFLAGS_NODIST for the LDFLAGS not intended for propagation to C extensions.~~ Avoid leaking linker flags into distutils. Dec 5, 2018

vstinner changed the title ~~Avoid leaking linker flags into distutils.~~ Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST Dec 10, 2018

vstinner added the release-blocker label Dec 20, 2018

ned-deily closed this as completed Dec 20, 2018

ned-deily removed the release-blocker label Dec 20, 2018

ezio-melotti transferred this issue from another repository Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST #79438

Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST #79438

stratakis mannequin commented Nov 15, 2018

stratakis mannequin commented Nov 15, 2018

stratakis mannequin commented Nov 15, 2018

stratakis mannequin commented Dec 5, 2018

stratakis mannequin commented Dec 5, 2018

stratakis mannequin commented Dec 5, 2018

ned-deily commented Dec 9, 2018

ned-deily commented Dec 10, 2018

stratakis mannequin commented Dec 10, 2018

vstinner commented Dec 14, 2018

vstinner commented Dec 14, 2018

vstinner commented Dec 19, 2018

vstinner commented Dec 19, 2018

vstinner commented Dec 19, 2018

stratakis mannequin commented Dec 20, 2018

stratakis mannequin commented Dec 20, 2018

vstinner commented Dec 20, 2018

stratakis mannequin commented Dec 20, 2018

vstinner commented Dec 20, 2018

vstinner commented Dec 20, 2018

vstinner commented Dec 20, 2018

ned-deily commented Dec 20, 2018

vstinner commented Dec 20, 2018

miss-islington commented Dec 24, 2018

miss-islington commented Dec 24, 2018

miss-islington commented Dec 24, 2018

ned-deily commented Dec 24, 2018

ned-deily commented Dec 24, 2018

vstinner commented Jan 7, 2019

Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST #79438

Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST #79438

Comments

stratakis mannequin commented Nov 15, 2018

stratakis mannequin commented Nov 15, 2018

stratakis mannequin commented Nov 15, 2018

stratakis mannequin commented Dec 5, 2018

stratakis mannequin commented Dec 5, 2018

stratakis mannequin commented Dec 5, 2018

ned-deily commented Dec 9, 2018

ned-deily commented Dec 10, 2018

stratakis mannequin commented Dec 10, 2018

vstinner commented Dec 14, 2018

vstinner commented Dec 14, 2018

vstinner commented Dec 19, 2018

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

(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

vstinner commented Dec 19, 2018

vstinner commented Dec 19, 2018

stratakis mannequin commented Dec 20, 2018

stratakis mannequin commented Dec 20, 2018

vstinner commented Dec 20, 2018

stratakis mannequin commented Dec 20, 2018

vstinner commented Dec 20, 2018

vstinner commented Dec 20, 2018

vstinner commented Dec 20, 2018

ned-deily commented Dec 20, 2018

vstinner commented Dec 20, 2018

miss-islington commented Dec 24, 2018

miss-islington commented Dec 24, 2018

miss-islington commented Dec 24, 2018

ned-deily commented Dec 24, 2018

ned-deily commented Dec 24, 2018

vstinner commented Jan 7, 2019

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

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