Skip to content

gh-90108: Disable LTO on _freeze_module and _testembed #109581

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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Sep 19, 2023

LTO optimization is nice to make Python faster, but _freeze_module and _testembed performance is not important. Using LTO to build these two programs make a whole Python build way slower, especially combined with a sanitizer (like ASAN).

LTO optimization is nice to make Python faster, but _freeze_module
and _testembed performance is not important. Using LTO to build these
two programs make a whole Python build way slower, especially
combined with a sanitizer (like ASAN).
@vstinner
Copy link
Member Author

@vstinner
Copy link
Member Author

vstinner added the skip news label

I don't think that this change is important to document in the Changelog, since it has no impact on users, it only makes the build a little bit faster when LTO is used.

@vstinner
Copy link
Member Author

I noticed this issue when working on PR #109576.

@corona10
Copy link
Member

corona10 commented Sep 19, 2023

Did you check that build is passed with the clang(thinLTO)?

@corona10
Copy link
Member

corona10 commented Sep 19, 2023

PTAL: #96761 (comment)

I am not sure that it will happen again, but you need check.

@vstinner
Copy link
Member Author

Before, I only tested my PR with GCC.

It seems like in practice, this change has no effect on clang with Python is built with ./configure --with-lto=<any value>. clang always use LTO in this case.

Did you check that build is passed with the clang(thinLTO)?

Sure.

git clean -fdx
./configure --with-lto=thin CC=clang LD=clang 
make

It just works, but there is a trick: CONFIGURE_LDFLAGS_NOLTO=-flto=thin in Makefile, because of this line in configure.ac:

      dnl Clang linker requires -flto in order to link objects with LTO information.
      dnl Thin LTO is faster and works for object files with full LTO information, too.
      AX_CHECK_COMPILE_FLAG([-flto=thin],[LDFLAGS_NOLTO="-flto=thin"],[LDFLAGS_NOLTO="-flto"])

Code added by you in commit 83d84e6 :-)

In short, "NOLTO" does not disable LTO but enable LTO instead :-)


I also tried:

  • ./configure --with-lto=full CC=clang LD=clang.
  • ./configure --with-lto CC=clang LD=clang: alias to --with-lto=thin in practice.

Both commands produce CONFIGURE_LDFLAGS_NOLTO=-flto=thin as well in the Makefile.

Python build works (with my PR) in both cases.


If I modify manually Makefile to use CONFIGURE_LDFLAGS_NOLTO=-fno-lto, Python build fails with my change.

clang   -fno-lto  -o Programs/_freeze_module Programs/_freeze_module.o  (...)

Programs/_freeze_module.o: file not recognized: file format not recognized

But... Python also fails on the main branch (without this PR) if manually edit Makefile to use CONFIGURE_LDFLAGS_NOLTO=-fno-lto. Build fails somewhere else:

clang   -fno-lto  -o _bootstrap_python Modules/getbuildinfo.o (...)

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

Well:

$ file Modules/getbuildinfo.o
Modules/getbuildinfo.o: LLVM IR bitcode

@vstinner
Copy link
Member Author

I discovered "NOLTO" variables when I worked on PR #109576: "Don't export -fsanitize compiler options".

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

LGTM

@vstinner vstinner merged commit 3e3a7da into python:main Sep 20, 2023
@vstinner vstinner deleted the no_lto branch September 20, 2023 01:40
@vstinner
Copy link
Member Author

Thanks for reviews @corona10 and @gpshead.

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x Debian 3.x has failed when building commit 3e3a7da.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/49/builds/6610) and take a look at the build logs.
  4. Check if the failure is related to this commit (3e3a7da) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/49/builds/6610

Failed tests:

  • test_tools

Failed subtests:

  • test_freeze_simple_script - test.test_tools.test_freeze.TestFreeze.test_freeze_simple_script

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/test/test_tools/test_freeze.py", line 28, in test_freeze_simple_script
    outdir, scriptfile, python = helper.prepare(script, outdir)
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Tools/freeze/test/freeze.py", line 146, in prepare
    copy_source_tree(srcdir, SRCDIR)
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Tools/freeze/test/freeze.py", line 95, in copy_source_tree
    shutil.copytree(oldroot, newroot, ignore=ignore_non_src)
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/shutil.py", line 588, in copytree
    return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/shutil.py", line 542, in _copytree
    raise Error(errors)
shutil.Error: [('/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/build/test_python_2763169æ/test_python_t7c7j57j.sock', '/tmp/test_python_a4o1bjnm/tmphlyck6y3/cpython/build/test_python_2763169æ/test_python_t7c7j57j.sock', "[Errno 6] No such device or address: '/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/build/test_python_2763169æ/test_python_t7c7j57j.sock'")]

@erlend-aasland
Copy link
Contributor

Nice, thanks Victor!

csm10495 pushed a commit to csm10495/cpython that referenced this pull request Sep 28, 2023
…109581)

LTO optimization is nice to make Python faster, but _freeze_module
and _testembed performance is not important. Using LTO to build these
two programs make a whole Python build way slower, especially
combined with a sanitizer (like ASAN).
vstinner added a commit to vstinner/cpython that referenced this pull request Oct 11, 2023
vstinner added a commit that referenced this pull request Oct 11, 2023
#110720)

gh-110313: Revert "gh-90108: Disable LTO on _freeze_module and _testembed (#109581)"

This reverts commit 3e3a7da.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
…109581)

LTO optimization is nice to make Python faster, but _freeze_module
and _testembed performance is not important. Using LTO to build these
two programs make a whole Python build way slower, especially
combined with a sanitizer (like ASAN).
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
…e and _teste… (python#110720)

pythongh-110313: Revert "pythongh-90108: Disable LTO on _freeze_module and _testembed (python#109581)"

This reverts commit 3e3a7da.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants