bpo-35257: Avoid leaking the linker flags into distutils. by stratakis · Pull Request #10900 · python/cpython (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation35 Commits4 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This is the first draft of the PR addressing the propagation of LDFLAGS to C extensions built by distutils.
The work is based on acb8c52 where CFLAGS_NODIST was added for compiler flags aimed to be used only by the interpreter and the stdlib modules but not by 3rd party C extensions. This PR expands that to the linker flags.

While LDFLAGS are not leaked into python-config as CFLAGS are, this can be checked by compiling a simple C extension (e.g. psycopg2).

https://bugs.python.org/issue35257

stratakis changed the title[WIP] bpo-35257: Add LDFLAGS_NODIST for stdlib c extensions [WIP] bpo-35257: Add LDFLAGS_NODIST for stdlib C extensions

Dec 4, 2018

stratakis changed the title[WIP] bpo-35257: Add LDFLAGS_NODIST for stdlib C extensions bpo-35257: Avoid leaking the linker flags into distutils.

Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is wrong in this change, but I don't know what: "git clean -fdx; ./configure CC=clang --with-lto && make" fails with this PR, whereas it pass in the master branch. Failure:

clang -pthread   -Xlinker -export-dynamic -o Programs/_testembed Programs/_testembed.o libpython3.8m.a -lpthread -ldl  -lutil -lm   -lm 
Programs/python.oPrograms/_testembed.o: file not recognized: : file not recognized: file format not recognizedfile format not recognized

clang-7: error: linker command failed with exit code 1 (use -v to see invocation)clang-7: 
error: linker command failed with exit code 1 (use -v to see invocation)

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Updates based on some discussion with Victor.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments. I didn't test your updated PR yet.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: "bpo-35257" syntax is now preferred.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted, will update.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would mind to add a comment explaining why PY_LDFLAGS (and not PY_LDFLAGS_NODIST) is used for LDSHARED?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to dig in a bit more into the buildsystem to figure out how they are used as my approach was a bit of trial and error for LDSHARED and BLDSHARED. Will try to offer a more comprehensive explanation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why $CFLAGS is passed as CFLAGS_NODIST :-(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the target for created the binary using profile guided optimizations, thus I followed the same pattern for ldflags as with the cflags.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it's a bug: https://bugs.python.org/issue35499 and that you should write: LDFLAGS_NODIST="$(LDFLAGS_NODIST) $(PGO_PROF_GEN_FLAG)".

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why $CFLAGS is passed as CFLAGS_NODIST :-(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should mention LTO compilation with clang. Maybe: "Avoid leaking Link Time Optimization (LTO) flags in linker flags into distutils when compiling C extensions."

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted, will do.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LTO is targeted now as the one that shouldn't propagate to C extensions, however that should target the all the flags that are not intended for propagation to C extensions, so shouldn't we have a more generic description of it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so shouldn't we have a more generic description of it?

Keep the generic description, but add a sentence to explain in which case the bug was the most visible. For example:

"Avoid leaking the linker flags into distutils when compiling C extensions, especially Link Time Optimization (LTO) flags."

From what I see in this PR, LDFLAGS="$LDFLAGS LTOFLAGS"replacedwithLDFLAGSNODIST="LTOFLAGS" replaced with LDFLAGS_NODIST="LTOFLAGS"replacedwithLDFLAGSNODIST="LDFLAGS_NODIST $LTOFLAGS" is the major change of this PR. I don't see other linker flags only passed to LDFLAGS_NODIST. Maybe during the temporary profiling step for PGO? But that's not visible for users.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike putting more and more code at the module level :-( Would you mind to move these code (from clfags to sysconfig.get_config_vars()['LDFLAGS']) into a new subfunction (ex: "set_compiler_flags()") which does update sysconfig?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why CFLAGS is used instead of PY_CFLAGS here? I would expect that setup.py would use the same CFLAGS than Makefile, something like $(PY_STDMODULE_CFLAGS).

Module/makesetup uses $(PY_BUILTIN_MODULE_CFLAGS).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_osx_support change LGTM

@vstinner I believe I addressed the current issues, didn't have time today to dig more to LDSHARED and BLDSHARED though. It works through all the configs I tested (--with-lto, --with-shared, using clang and more) and the flags do not propagate to distutils. I'd like to have it in 3.6 but maybe it can be added downstream only.

Also @skrah is the author of the original code that added cflags_nodist, maybe you could take a look?

By the way the issue first appeared due to https://bugs.python.org/issue31354 when 3.7 was the master branch. However I requested it to be backported to 3.6 which made this bug propagate to 3.6 as well. I realized it when working on backporting some lto+clang related fixes from master.

@vstinner tested locally and this now builds fine with

../configure '--with-lto' 'CC=clang' 'CXX=clang++'

and

% sh python-config --libs --ldflags
-lpython3.8m -lpthread -ldl  -lutil -lm -lm 
-L/home/sguelton/sources/cpython/lib/python3.8/config-3.8m-x86_64-linux-gnu -L/home/sguelton/sources/cpython/lib -lpython3.8m -lpthread -ldl  -lutil -lm -lm  -Xlinker -export-dynamic

which looks file to me

Can you please merge master into your PR, to retrieve PR #11164 changes? And can you modify build_all_generate_profile: and profile-opt: to use LDFLAGS_NODIST="$(LDFLAGS_NODIST) $(PGO_PROF_GEN_FLAG)"?

Rebased on top of master. Didn't have time yet to test the changes though.

When compiling 3rd party C extensions, the linker flags used by the compiler for the interpreter and the stdlib modules, will get leaked into distutils. In order to avoid that, the PY_CORE_LDFLAGS and PY_LDFLAGS_NODIST are introduced to keep those flags separated.

Thanks @stratakis for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

Sorry, @stratakis and @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker cf10a750f4b50b6775719cfb17bee00bc3a9c60b 3.7

vstinner added a commit that referenced this pull request

Dec 20, 2018

…H-11264)

When compiling 3rd party C extensions, the linker flags used by the compiler for the interpreter and the stdlib modules, will get leaked into distutils. In order to avoid that, the PY_CORE_LDFLAGS and PY_LDFLAGS_NODIST are introduced to keep those flags separated.

(cherry picked from commit cf10a75)

ned-deily pushed a commit that referenced this pull request

Dec 20, 2018

) (GH-11265)

When compiling 3rd party C extensions, the linker flags used by the compiler for the interpreter and the stdlib modules, will get leaked into distutils. In order to avoid that, the PY_CORE_LDFLAGS and PY_LDFLAGS_NODIST are introduced to keep those flags separated.

(cherry picked from commit cf10a75)

ned-deily pushed a commit to ned-deily/cpython that referenced this pull request

Dec 23, 2018

) (pythonGH-11264)

When compiling 3rd party C extensions, the linker flags used by the compiler for the interpreter and the stdlib modules, will get leaked into distutils. In order to avoid that, the PY_CORE_LDFLAGS and PY_LDFLAGS_NODIST are introduced to keep those flags separated.

(cherry picked from commit cf10a75)