Skip to content
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

Fix build of optimized functions with LTO #938

Merged
merged 1 commit into from
Apr 27, 2021
Merged

Fix build of optimized functions with LTO #938

merged 1 commit into from
Apr 27, 2021

Conversation

viccie30
Copy link
Contributor

When building zlib-ng with LTO enabled, at least GCC doesn't respect the flags of each source file and just throws all the flags together. See https://gcc.gnu.org/onlinedocs/gccint/LTO-object-file-layout.html.

Because of this, flags don't get applied correctly to the arch-specific files, leading to problems like #722.

CMake does not allow disabling LTO per source file, so in CMakeLists.txt I just added -fno-lto if the compiler is GCC-like and native mode is not enabled.
For the configure script I wrote the same logic, but using an extra variable in arch/*/Makefile.in.

@codecov
Copy link

codecov bot commented Apr 24, 2021

Codecov Report

Merging #938 (60b6d2d) into develop (f03918f) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #938      +/-   ##
===========================================
- Coverage    77.89%   77.88%   -0.02%     
===========================================
  Files           74       74              
  Lines         8307     8303       -4     
  Branches      1369     1374       +5     
===========================================
- Hits          6471     6467       -4     
  Misses        1307     1307              
  Partials       529      529              
Flag Coverage Δ
macos_clang 68.65% <ø> (ø)
macos_gcc 67.64% <ø> (ø)
ubuntu_clang 73.51% <ø> (-1.68%) ⬇️
ubuntu_clang_debug 73.81% <ø> (-1.62%) ⬇️
ubuntu_clang_inflate_allow_invalid_dist 73.24% <ø> (-1.68%) ⬇️
ubuntu_clang_inflate_strict 73.51% <ø> (-1.68%) ⬇️
ubuntu_clang_mmap 73.22% <ø> (-1.67%) ⬇️
ubuntu_clang_msan 73.51% <ø> (-1.68%) ⬇️
ubuntu_gcc 73.16% <ø> (-1.43%) ⬇️
ubuntu_gcc_aarch64 74.46% <ø> (+0.25%) ⬆️
ubuntu_gcc_aarch64_compat_no_opt 74.00% <ø> (+0.38%) ⬆️
ubuntu_gcc_aarch64_no_acle 74.28% <ø> (+0.36%) ⬆️
ubuntu_gcc_aarch64_no_neon 74.03% <ø> (+0.36%) ⬆️
ubuntu_gcc_armhf 75.15% <ø> (+0.25%) ⬆️
ubuntu_gcc_armhf_compat_no_opt 74.02% <ø> (+0.38%) ⬆️
ubuntu_gcc_armhf_no_acle 75.34% <ø> (+0.25%) ⬆️
ubuntu_gcc_armhf_no_neon 75.72% <ø> (+0.26%) ⬆️
ubuntu_gcc_armsf 75.16% <ø> (+0.25%) ⬆️
ubuntu_gcc_armsf_compat_no_opt 74.02% <ø> (+0.38%) ⬆️
ubuntu_gcc_compat_no_opt 75.10% <ø> (+0.26%) ⬆️
ubuntu_gcc_mingw_i686 0.00% <ø> (ø)
ubuntu_gcc_mingw_x86_64 0.00% <ø> (ø)
ubuntu_gcc_no_avx2 75.14% <ø> (+0.35%) ⬆️
ubuntu_gcc_no_pclmulqdq 71.61% <ø> (-1.35%) ⬇️
ubuntu_gcc_no_sse2 74.10% <ø> (+0.05%) ⬆️
ubuntu_gcc_no_sse4 71.58% <ø> (-1.36%) ⬇️
ubuntu_gcc_o3 73.05% <ø> (-1.45%) ⬇️
ubuntu_gcc_osb 75.99% <ø> (-1.31%) ⬇️
ubuntu_gcc_ppc 75.46% <ø> (+0.23%) ⬆️
ubuntu_gcc_ppc64 76.23% <ø> (+0.23%) ⬆️
ubuntu_gcc_ppc64le 74.98% <ø> (+0.26%) ⬆️
ubuntu_gcc_s390x 73.84% <ø> (+0.18%) ⬆️
ubuntu_gcc_sparc64 76.70% <ø> (+0.24%) ⬆️
win64_gcc 70.63% <ø> (ø)
win64_gcc_compat_no_opt 74.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
chunkset_tpl.h 95.14% <0.00%> (-3.89%) ⬇️
deflate.h 57.14% <0.00%> (+7.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f03918f...60b6d2d. Read the comment docs.

@viccie30
Copy link
Contributor Author

I tested this patch using sbuild on all supported Debian architectures (amd64, arm64, armel, armhf, i386, mips64el, mipsel, ppc64el, and s390x) on stable, testing, unstable, and experimental, both with and without LTO and for static and shared builds.

For all combinations, except mips64el and mipsel on experimental due to dependency breakage, the builds succeed and all tests pass (using qemu-user-static for the non-x86 architectures).

@Dead2
Copy link
Member

Dead2 commented Apr 26, 2021

I think this override should perhaps not be active if the user enables Native mode, native is obviously useless for redistribution such as in distros, but it can be useful for those who compile locally and want the (possibly) fastest end result.

When native mode is enabled, all files are compiled with the same flags, essentially enabling all instruction sets the local cpu supports, so that should allow it to work with LTO.

@viccie30
Copy link
Contributor Author

I agree, that's why both in configure and CMakeLists.txt the no lto flag is only defined if native mode is disabled

@Dead2
Copy link
Member

Dead2 commented Apr 26, 2021

Hah, indeed it is, I didn't notice that. 👍

@viccie30
Copy link
Contributor Author

It took me a few tries to get that right as well, but I think I did the right way round

@Dead2 Dead2 merged commit 461c279 into zlib-ng:develop Apr 27, 2021
@viccie30 viccie30 deleted the lto-fix branch April 27, 2021 10:41
Dead2 added a commit that referenced this pull request May 8, 2021
- Include porting guide in release packages #917
- Documentation improvements #913 #949
- Added Windows ARM binaries in release packages #916
- Fix crash on ARMv7 #927
- Fix building on FreeBSD #921
- Fix building with musl on aarch64 #936
- Fix ARM float-abi detection #918
- Fix cmake detection of risc-v architectures #942
- Minor buildsystem fixes #922 #924 #933 #938 #950
- Improve zlib-compat build #915 #944
- CI/Test improvements #926 #929 #925 #937 #939 #940
Dead2 added a commit that referenced this pull request May 8, 2021
- Include porting guide in release packages #917
- Documentation improvements #913 #949
- Added Windows ARM binaries in release packages #916
- Fix crash on ARMv7 #927
- Fix building on FreeBSD #921
- Fix building with musl on aarch64 #936
- Fix ARM float-abi detection #918
- Fix cmake detection of risc-v architectures #942
- Minor buildsystem fixes #922 #924 #933 #938 #950
- Improve zlib-compat build #915 #944
- CI/Test improvements #926 #929 #927 #937 #939 #940
@Dead2 Dead2 mentioned this pull request May 8, 2021
Dead2 added a commit that referenced this pull request May 8, 2021
- Include porting guide in release packages #917
- Documentation improvements #913 #949
- Added Windows ARM binaries in release packages #916
- Fix crash on ARMv7 #927
- Fix building on FreeBSD #921
- Fix building with musl on aarch64 #936
- Fix ARM float-abi detection #918
- Fix cmake detection of risc-v architectures #942
- Minor buildsystem fixes #922 #924 #933 #938 #950
- Improve zlib-compat build #915 #944
- CI/Test improvements #926 #929 #927 #937 #939 #940
Dead2 added a commit that referenced this pull request May 9, 2021
- Include porting guide in release packages #917
- Documentation improvements #913 #949
- Added Windows ARM binaries in release packages #916
- Fix crash on ARMv7 #927
- Fix building on FreeBSD #921
- Fix building with musl on aarch64 #936 #952
- Fix ARM float-abi detection #918
- Fix cmake detection of risc-v architectures #942
- Minor buildsystem fixes #922 #924 #933 #938 #950
- Improve zlib-compat build #915 #944
- CI/Test improvements #926 #929 #927 #937 #939 #940
Dead2 added a commit that referenced this pull request May 13, 2021
- Include porting guide in release packages #917
- Documentation improvements #913 #949
- Added Windows ARM binaries in release packages #916
- Fix crash on ARMv7 #927
- Fix building on FreeBSD #921
- Fix building with musl on aarch64 #936 #952
- Fix ARM float-abi detection #918
- Fix cmake detection of risc-v architectures #942
- Minor buildsystem fixes #922 #924 #933 #938 #950
- Improve zlib-compat build #915 #944
- CI/Test improvements #926 #929 #927 #937 #939 #940
gsjaardema pushed a commit to gsjaardema/zlib-ng that referenced this pull request May 13, 2021
- Include porting guide in release packages zlib-ng#917
- Documentation improvements zlib-ng#913 zlib-ng#949
- Added Windows ARM binaries in release packages zlib-ng#916
- Fix crash on ARMv7 zlib-ng#927
- Fix building on FreeBSD zlib-ng#921
- Fix building with musl on aarch64 zlib-ng#936 zlib-ng#952
- Fix ARM float-abi detection zlib-ng#918
- Fix cmake detection of risc-v architectures zlib-ng#942
- Minor buildsystem fixes zlib-ng#922 zlib-ng#924 zlib-ng#933 zlib-ng#938 zlib-ng#950
- Improve zlib-compat build zlib-ng#915 zlib-ng#944
- CI/Test improvements zlib-ng#926 zlib-ng#929 zlib-ng#927 zlib-ng#937 zlib-ng#939 zlib-ng#940
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants