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

Created on 2018-11-15 16:14 by cstratak, last changed 2018-12-14 20:41 by vstinner.

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 open cstratak, 2018-12-04 16:53
PR 11169 open vstinner, 2018-12-14 20:41
Messages (10)
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.
History
Date User Action Args
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