Conversation
The Clang-only parts stay Clang-only: --icf=safe needs .llvm_addrsig, which GCC does not emit, and separate-loadable-segments only pays on the patchelf-ed AppImage path.
Without it the lld selection in CMakeLists is inert on every ubuntu row -- lld-21 only exists in the rockylinux8-vcpkg image.
lld-$CLANG_VER is already installed by scripts/mrbind/install_deps_ubuntu.sh; only the PATH symlink was missing, and hardcoding a version could dangle.
Contributor
Author
|
Closing as measured-marginal: linking gets 57% faster (3.46s -> 1.48s, ~2s of a 20-40 min build), stripped size is neutral (+10.6 KB), but unstripped grows +446 KB and the .deb ships unstripped, so the artifact would get slightly bigger. Worth noting for the record: the ubuntu Clang rows already had lld (installed by scripts/mrbind/install_deps_ubuntu.sh) and were already folding -- only GCC failed the check, because it resolves -fuse-ld=lld through PATH while Clang finds ld.lld in its own llvm-*/bin. Stripping the .deb is the better lever and goes in its own PR. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GCC rows on Linux still link with
ld.bfd, so they get none of the folding from #6547 / #6550. This relaxes the compiler-ID gate on the lld selection while leaving the ICF and padding flags Clang-only:--icf=safereads Clang's.llvm_addrsig, which GCC does not emit, and-z separate-loadable-segmentsonly pays on the patchelf-ed AppImage path.Measured: a no-op, because the ubuntu images have no lld
One
ubuntu24/ GCC 13 / Release leg, three variants on a throwaway branch:ld.bfd)--icf=allfor GCCByte-identical, all three. The configure log says it plainly:
Performing Test MESHLIB_HAVE_LLD - Failed. Neither ubuntu image installs lld —ubuntu22Dockerfilehasg++-12 clang-14,ubuntu24Dockerfilehasg++-13 gcc-14 g++-14 clang-18, andlld-21*appears only inrockylinux8-vcpkgDockerfile. So every flag in the block is inert on ubuntu today, for the Clang rows as much as the GCC ones, and the whole ICF series only ever applied to therockylinux8-vcpkgbuilds (thelinux-vcpkgpackage and the manylinux wheels).Making this actually do something needs
lldadded to both ubuntu Dockerfiles plus an image rebuild — a heavier change than a CMake gate. And it would still not shrink the.debs:--icf=safefolds nothing on GCC output (no address-significance table), which the--icf=allrow above cannot demonstrate here but Fold identical functions at link time with --icf=safe #6547's measurements did.--icf=allwould fold, but it folds functions whose addresses are taken, so&f == &gcan become true — the semantic we deliberately rejected for a public library. Shipping.debs with different folding rules from the wheels would be worse than shipping them unfolded.So the honest options for the
.debs are: install lld and take the link-time win only, or build them with Clang. Neither is this PR.Recommendation
Close, unless we want the link-time angle — in which case this should be reopened together with the Dockerfile change, and justified on build time rather than size.