gh-96761: Fix lto-build for clang #96762

matthiasgoergens · 2022-09-12T07:53:58Z

When enabling link time optimization for clang, you eventually get the following error:

Modules/getbuildinfo.o: file not recognized: file format not recognized

This patch fixes that problem.

Issue: Link-time-optimization with clang is broken #96761

When enabling link time optimization for clang, you eventually get the following error: `Modules/getbuildinfo.o: file not recognized: file format not recognized` This patch fixes that problem.

corona10

Please re-generate configure file by using the following command.

docker run --rm --pull=always -v$(pwd):/src quay.io/tiran/cpython_autoconf:269

bedevere-bot · 2022-09-12T08:13:59Z

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.

tiran · 2022-09-12T08:20:59Z

configure.ac

@@ -1829,7 +1829,7 @@ if test "$Py_LTO" = 'true' ; then
  case $CC in
    *clang*)
      dnl flag to disable lto during linking
-      LDFLAGS_NOLTO="-fno-lto"
+      LDFLAGS_NOLTO="-flto=${Py_LTO_POLICY}"


LDFLAGS_NOLTO should be linker flags without LTO, hence the name no lto.

I guess I'll need to make a bigger change, that changes the name to something more descriptive.

# LDFLAGS_NOLTO is an extra flag to disable lto. It is used to speed up building # of _bootstrap_python and _freeze_module tools, which don't need LTO.

For clang that seems to work better when they are the same as for the rest of the build. The thin-lto build is already plenty fast (and even then, a slow build is better than a broken one).

clang does not support linking with objects that have LTO information? It works fine for GCC. Could you please update the comment on line 1831?

I am not quite sure what the exact rootcause is. I just noticed that when I tried to build some things with LTO and some things (like _bootstrap_python) without LTO, I get an error from the linker via clang.

I updated the comment.

Does thin LTO linking work with object files that are built with full LTO? This would speed up LTO builds a bit.

IIUC, at this moment -flto=${Py_LTO_POLICY} could be -flto=default so it's wrong.

Hmm, let me think about how to fix this. Or do you have a suggestion?

matthiasgoergens · 2022-09-12T08:40:57Z

Please re-generate configure file by using the following command.
docker run --rm --pull=always -v$(pwd):/src quay.io/tiran/cpython_autoconf:269

Done!

matthiasgoergens · 2022-09-12T09:02:58Z

I have made the requested changes; please review again.

bedevere-bot · 2022-09-12T09:03:02Z

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.

corona10

see: #96762 (comment)

bedevere-bot · 2022-09-12T10:09:16Z

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.

configure.ac

Co-authored-by: Christian Heimes <[email protected]>

matthiasgoergens · 2022-09-14T09:52:49Z

@corona10 Just to be sure: does your PR at #96766 obsolete this one?

If yes, I'll close mine.

corona10 · 2022-09-14T10:02:09Z

@matthiasgoergens cc @tiran
There are several consensuses on this issue.

A. Using ThinLTO as the default LTO policy for clang is not decided yet, #96766 will cover the issue.
B. If #96766 is merged we can use ThinLTO on clang for bootstrap_python if needed.
C. But A and B do not mean that bootstrap_python should be built with clang by using LTO. (If needed we should use it, but if it is not a root cause we don't need to use it) since I can not meet the same issue on macOS with clang so I need to reproduce the issue on Linux first. I need to know the root cause.

corona10 · 2022-09-14T10:06:19Z

@corona10 Just to be sure: does your PR at #96766 obsolete this one?

So in conclusion, it's a slightly different issue.

dwvshell · 2022-09-14T10:17:09Z

How do I put this to work

configure.ac

corona10

@matthiasgoergens

Please rebase the PR and regen configure with the suggested changes.

Please update the following comment also (for clang case).

cpython/Makefile.pre.in

Lines 94 to 95 in 1fc8bd3

    
           # LDFLAGS_NOLTO is an extra flag to disable lto. It is used to speed up building 
        
           # of _bootstrap_python and _freeze_module tools, which don't need LTO.

Co-authored-by: Dong-hee Na <[email protected]>

corona10

Would you like to regenerate the configure by using docker run --rm --pull=always -v$(pwd):/src quay.io/tiran/cpython_autoconf:269?

matthiasgoergens · 2022-09-21T02:01:58Z

Closed in favour of #96945

Fix lto-build for clang

aee5500

When enabling link time optimization for clang, you eventually get the following error: `Modules/getbuildinfo.o: file not recognized: file format not recognized` This patch fixes that problem.

bedevere-bot added the awaiting review label Sep 12, 2022

corona10 requested changes Sep 12, 2022

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Sep 12, 2022

corona10 requested a review from tiran September 12, 2022 08:17

tiran reviewed Sep 12, 2022

View reviewed changes

Run with approved autoconf

9bd6dc3

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 12, 2022

bedevere-bot requested a review from corona10 September 12, 2022 09:03

Update comment

c6129de

corona10 requested changes Sep 12, 2022

View reviewed changes

bedevere-bot removed the awaiting change review label Sep 12, 2022

bedevere-bot added the awaiting changes label Sep 12, 2022

tiran reviewed Sep 12, 2022

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

Update configure.ac

2861b12

Co-authored-by: Christian Heimes <[email protected]>

corona10 reviewed Sep 16, 2022

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

corona10 requested changes Sep 16, 2022

View reviewed changes

Update configure.ac

8953bf7

Co-authored-by: Dong-hee Na <[email protected]>

corona10 added the needs backport to 3.11 only security fixes label Sep 18, 2022

corona10 mentioned this pull request Sep 18, 2022

Link-time-optimization with clang is broken #96761

Closed

corona10 reviewed Sep 19, 2022

View reviewed changes

matthiasgoergens closed this Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-96761: Fix lto-build for clang #96762

gh-96761: Fix lto-build for clang #96762

matthiasgoergens commented Sep 12, 2022 •

edited by bedevere-bot

Loading

corona10 left a comment

bedevere-bot commented Sep 12, 2022

tiran Sep 12, 2022

matthiasgoergens Sep 12, 2022 •

edited

Loading

tiran Sep 12, 2022

matthiasgoergens Sep 12, 2022

matthiasgoergens Sep 12, 2022

tiran Sep 12, 2022

corona10 Sep 12, 2022

matthiasgoergens Sep 12, 2022

matthiasgoergens commented Sep 12, 2022

matthiasgoergens commented Sep 12, 2022

bedevere-bot commented Sep 12, 2022

corona10 left a comment

bedevere-bot commented Sep 12, 2022

matthiasgoergens commented Sep 14, 2022

corona10 commented Sep 14, 2022 •

edited

Loading

corona10 commented Sep 14, 2022

dwvshell commented Sep 14, 2022

corona10 left a comment •

edited

Loading

corona10 left a comment

matthiasgoergens commented Sep 21, 2022

	# LDFLAGS_NOLTO is an extra flag to disable lto. It is used to speed up building
	# of _bootstrap_python and _freeze_module tools, which don't need LTO.

gh-96761: Fix lto-build for clang #96762

gh-96761: Fix lto-build for clang #96762

Conversation

matthiasgoergens commented Sep 12, 2022 • edited by bedevere-bot Loading

corona10 left a comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 12, 2022

tiran Sep 12, 2022

Choose a reason for hiding this comment

matthiasgoergens Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

tiran Sep 12, 2022

Choose a reason for hiding this comment

matthiasgoergens Sep 12, 2022

Choose a reason for hiding this comment

matthiasgoergens Sep 12, 2022

Choose a reason for hiding this comment

tiran Sep 12, 2022

Choose a reason for hiding this comment

corona10 Sep 12, 2022

Choose a reason for hiding this comment

matthiasgoergens Sep 12, 2022

Choose a reason for hiding this comment

matthiasgoergens commented Sep 12, 2022

matthiasgoergens commented Sep 12, 2022

bedevere-bot commented Sep 12, 2022

corona10 left a comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 12, 2022

matthiasgoergens commented Sep 14, 2022

corona10 commented Sep 14, 2022 • edited Loading

corona10 commented Sep 14, 2022

dwvshell commented Sep 14, 2022

corona10 left a comment • edited Loading

Choose a reason for hiding this comment

corona10 left a comment

Choose a reason for hiding this comment

matthiasgoergens commented Sep 21, 2022

matthiasgoergens commented Sep 12, 2022 •

edited by bedevere-bot

Loading

matthiasgoergens Sep 12, 2022 •

edited

Loading

corona10 commented Sep 14, 2022 •

edited

Loading

corona10 left a comment •

edited

Loading