bpo-35257: Avoid leaking the linker flags into distutils. #10900

stratakis · 2018-12-04T16:53:03Z

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

vstinner

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)

bedevere-bot · 2018-12-06T10:53:08Z

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.

stratakis · 2018-12-06T16:40:05Z

Updates based on some discussion with Victor.

vstinner

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

Lib/test/pythoninfo.py

vstinner · 2018-12-07T09:57:21Z

Makefile.pre.in

@@ -85,6 +85,10 @@ CONFIGURE_CFLAGS=	@CFLAGS@
 # Use it when a compiler flag should _not_ be part of the distutils CFLAGS
 # once Python is installed (Issue #21121).
 CONFIGURE_CFLAGS_NODIST=@CFLAGS_NODIST@
+# LDFLAGS_NODIST is used in the same manner as CFLAGS_NODIST.
+# Use it when a linker flag should _not_ be part of the distutils LDFLAGS
+# once Python is installed (Issue #35257)


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

Noted, will update.

vstinner · 2018-12-07T09:58:15Z

Makefile.pre.in

@@ -147,7 +154,7 @@ CONFINCLUDEPY=	$(CONFINCLUDEDIR)/python$(LDVERSION)
 SHLIB_SUFFIX=	@SHLIB_SUFFIX@
 EXT_SUFFIX=	@EXT_SUFFIX@
 LDSHARED=	@LDSHARED@ $(PY_LDFLAGS)


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

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.

vstinner · 2018-12-07T09:59:00Z

Makefile.pre.in

@@ -497,7 +504,7 @@ profile-run-stamp:
 	touch $@

 build_all_generate_profile:
-	$(MAKE) @DEF_MAKE_RULE@ CFLAGS_NODIST="$(CFLAGS) $(PGO_PROF_GEN_FLAG)" LDFLAGS="$(LDFLAGS) $(PGO_PROF_GEN_FLAG)" LIBS="$(LIBS)"
+	$(MAKE) @DEF_MAKE_RULE@ CFLAGS_NODIST="$(CFLAGS) $(PGO_PROF_GEN_FLAG)" LDFLAGS_NODIST="$(LDFLAGS) $(PGO_PROF_GEN_FLAG)" LIBS="$(LIBS)"


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

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

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

I also wrote PR #11219 to avoid modifying LDFLAGS here: https://bugs.python.org/issue35499#msg332079

vstinner · 2018-12-07T09:59:09Z

Makefile.pre.in

@@ -511,7 +518,7 @@ build_all_merge_profile:
 profile-opt: profile-run-stamp
 	@echo "Rebuilding with profile guided optimizations:"
 	-rm -f profile-clean-stamp
-	$(MAKE) @DEF_MAKE_RULE@ CFLAGS_NODIST="$(CFLAGS) $(PGO_PROF_USE_FLAG)" LDFLAGS="$(LDFLAGS)"
+	$(MAKE) @DEF_MAKE_RULE@ CFLAGS_NODIST="$(CFLAGS) $(PGO_PROF_USE_FLAG)" LDFLAGS_NODIST="$(LDFLAGS)"


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

Ditto: https://bugs.python.org/issue35499 write: LDFLAGS_NODIST="$(LDFLAGS_NODIST) $(PGO_PROF_GEN_FLAG)".

vstinner · 2018-12-07T10:00:50Z

Misc/NEWS.d/next/Build/2018-12-05-22-28-40.bpo-35257.dmcd_s.rst

@@ -0,0 +1,2 @@
+Avoid leaking the linker flags into distutils when compiling
+C extensions.


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

Noted, will do.

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?

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" replaced with LDFLAGS_NODIST="$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.

vstinner · 2018-12-07T10:03:53Z

setup.py

+# modules (Issue #35257).
+ldflags = sysconfig.get_config_var('LDFLAGS')
+py_ldflags_nodist = sysconfig.get_config_var('PY_LDFLAGS_NODIST')
+sysconfig.get_config_vars()['LDFLAGS'] = ldflags + ' ' + py_ldflags_nodist


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?

vstinner · 2018-12-07T10:04:41Z

setup.py

@@ -24,6 +24,12 @@
 py_cflags_nodist = sysconfig.get_config_var('PY_CFLAGS_NODIST')
 sysconfig.get_config_vars()['CFLAGS'] = cflags + ' ' + py_cflags_nodist


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

vstinner · 2018-12-07T10:08:32Z

setup.py

@@ -24,6 +24,12 @@
 py_cflags_nodist = sysconfig.get_config_var('PY_CFLAGS_NODIST')
 sysconfig.get_config_vars()['CFLAGS'] = cflags + ' ' + py_cflags_nodist

+# Add special LDFLAGS reserved for building the interpreter and the stdlib
+# modules (Issue #35257).


ditto: bpo-35257

ned-deily

_osx_support change LGTM

stratakis · 2018-12-10T21:44:08Z

@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?

vstinner · 2018-12-12T15:15:57Z

cc @serge-sans-paille

stratakis · 2018-12-12T15:34:52Z

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.

serge-sans-paille · 2018-12-13T09:48:59Z

@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

vstinner · 2018-12-17T21:41:47Z

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)"?

stratakis · 2018-12-18T22:25:17Z

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.

pythoninfo.py

miss-islington · 2018-12-19T17:19:05Z

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

miss-islington · 2018-12-19T17:19:09Z

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

bedevere-bot · 2018-12-20T14:07:15Z

GH-11264 is a backport of this pull request to the 3.7 branch.

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

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

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

the-knights-who-say-ni added the CLA signed label Dec 4, 2018

bedevere-bot added the awaiting review label Dec 4, 2018

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 force-pushed the ldflags_nodist branch from 8da1451 to 6669fe8 Compare December 5, 2018 21:41

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

vstinner requested changes Dec 6, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Dec 6, 2018

stratakis force-pushed the ldflags_nodist branch from 6669fe8 to dc46afc Compare December 6, 2018 16:39

stratakis requested a review from a team as a code owner December 6, 2018 16:39

vstinner reviewed Dec 7, 2018

View reviewed changes

ned-deily reviewed Dec 9, 2018

View reviewed changes

vstinner requested a review from Yhg1s December 12, 2018 15:15

stratakis force-pushed the ldflags_nodist branch from 699ee3d to 25ac429 Compare December 18, 2018 22:24

stratakis added 4 commits December 19, 2018 15:10

Log PY_CORE_LDFLAGS and PY_STDMODULE_CFLAGS at

1819b28

pythoninfo.py

Refer to the issue number in the bpo-* format in comments.

d7ebc21

Move setting of the compiler and linker flags to a function.

d1655f9

stratakis force-pushed the ldflags_nodist branch from 25ac429 to d1655f9 Compare December 19, 2018 14:10

vstinner approved these changes Dec 19, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Dec 19, 2018

vstinner added the needs backport to 3.7 label Dec 19, 2018

vstinner merged commit cf10a75 into python:master Dec 19, 2018

bedevere-bot removed the awaiting merge label Dec 19, 2018

miss-islington assigned vstinner Dec 19, 2018

bedevere-bot removed the needs backport to 3.7 label Dec 20, 2018

vstinner mentioned this pull request Dec 20, 2018

[3.6] bpo-35257: Avoid leaking LTO linker flags into distutils (GH-10900) #11265

Merged

stratakis deleted the ldflags_nodist branch January 3, 2020 21:28

		@@ -0,0 +1,2 @@
		Avoid leaking the linker flags into distutils when compiling
		C extensions.

		@@ -24,6 +24,12 @@
		py_cflags_nodist = sysconfig.get_config_var('PY_CFLAGS_NODIST')
		sysconfig.get_config_vars()['CFLAGS'] = cflags + ' ' + py_cflags_nodist

Uh oh!

bpo-35257: Avoid leaking the linker flags into distutils. #10900

bpo-35257: Avoid leaking the linker flags into distutils. #10900

Uh oh!

Conversation

stratakis commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 6, 2018

Uh oh!

stratakis commented Dec 6, 2018

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ned-deily left a comment

Choose a reason for hiding this comment

Uh oh!

stratakis commented Dec 10, 2018

Uh oh!

vstinner commented Dec 12, 2018

Uh oh!

stratakis commented Dec 12, 2018

Uh oh!

serge-sans-paille commented Dec 13, 2018

Uh oh!

vstinner commented Dec 17, 2018

Uh oh!

stratakis commented Dec 18, 2018

Uh oh!

miss-islington commented Dec 19, 2018

Uh oh!

miss-islington commented Dec 19, 2018

Uh oh!

bedevere-bot commented Dec 20, 2018

Uh oh!

Uh oh!

stratakis commented Dec 4, 2018 •

edited

Loading