linux: allow building with ld.lld (for LTO) #344665
Merged
Conversation
This file contains 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
ofborg
bot
requested review from
alyssais,
K900,
NeQuissimus,
Ma27,
thoughtpolice and
TredwellGit
|
Full build failure log is here: I'm not quite sure what is failing. Will continue looking into it. |
blitz
force-pushed
the
kernel-lld
branch
2 times, most recently
from
a1800a7 to
4a1ff2b
Compare
|
|
Looks like that should be using the binutils wrapper and not the clang wrapper. |
|
Ah, |
|
There should be a strip in bintools: (I'm assuming llvm-objcopy must be multi-call or something for it to be the target of the link.) |
Only There is also a |
|
Want to fix it then? |
Yes, will open a different PR for this given that it's not a requirement for this one. The build goes further now: We're getting closer. |
|
I've opened #345691 for the strip issue. |
alyssais
reviewed
Oct 1, 2024
|
So I've narrowed down the build issue to the compiler/bintools wrappers. If I use the unwrapped toolchain for building (the last XXX commit), the build succeeds. I'm currently debugging what the wrappers do that causes this problem. I wish there were a flag to turn the wrapper magic off for cases like these... |
|
You have ran into #321667 if you're on x86_64-linux. |
Thanks for the pointer! But why would the kernel need patching, if it builds fine with the unwrapped toolchain? It seems like we do something weird in our wrapping logic. |
Our wrapper does invoke a behavior of LLVM which GNU ld doesn't have. GNU ld has some logic to not link with dynamic libraries when necessary. LLVM ld doesn't have this behavior. I did start some work in the wrapper to implement the same GNU ld behavior but I just haven't allocated enough time into this problem. I did however open an issue in LLVM itself to see about getting this behavior implemented. |
wegank
added
the
12.approvals: 1
|
I'm testing this locally now, but I don't understand why we need absolute paths for anything apart from the linker on x86_64. A comment mentions PATH clobbering issues, but what path clobbering issues? Have we ever seen any? |
|
I'm also wondering whether LD etc would be better set as environment variables in stdenv. Would you be interested in exploring that as followup? It would likely fix other packages with similar problems. |
The problem it refers to seems to be that |
I can look into this. I see a couple of packages that use |
But they're not going to be autodetected if we're explicitly setting them to the prefixed versions? |
For CC this is already done by stdenv. I was just talking about e.g. LD, for consistency. |
I looked into this and we don't have to set them at all! We only need to set I'm currently compiling some kernels, but so far it looks good. Testing is welcome! @alyssais If this approach works out, I'll squash the commits. |
|
@RossComputerGuy Can you also check how this version looks for you? If this works, it would remove quite a bit of clutter from the derivations. |
wegank
removed
the
12.approvals: 1
|
@alyssais @RossComputerGuy Do you have any input how to move this forward? |
|
Do you think it's ready? It looks fine to me with a squash. |
|
I think it's fine. |
I've rebased this and squashed the ld wrapper fixup. The other commits are reasonably self-sufficient. In case there are issues with 7ccb461, it could also be reverted individually without breaking functionality. I'm going to give this another nixpkgs-review round. |
ofborg
bot
added
10.rebuild-linux: 5001+
and removed
10.rebuild-linux: 2501-5000
labels
|
Result of 430 packages marked as broken and skipped:
175 packages failed to build:
2551 packages built:
|
|
The failed kernel modules seem unrelated. But maybe has a better overview of what's supposed to fail? What's our policy here? Does it make sense to mark them broken in another PR? |
|
Yeah. It's just that it needs investigation of what conditions cause them to fail to build, so having them marked broken properly requires somebody to put in the time to do that investigation. |
Even when building with pkgsLLVM.stdenv where ld is ld.lld and not binutils ld, the build picks up binutils ld for linking. This prevents features from working that require ld.lld. The reason is that when the LD environment variable is not set, Linux defaults to `ld` as a linker and ld is: /nix/store/zznja5f8v3jafffyah1rk46vpfcn38dv-gcc-wrapper-13.3.0/bin/ld GNU ld (GNU Binutils) 2.42 This ld comes from pkgsLLVM.buildPackages.stdenv.cc. Fix by being more specific about which tools we want to build with. This just extends what manual-config.nix has already done for quite a while to avoid similar problems for other tools. It's important to pass LD and other toolchain environment variables to `make config` in generate-config.pl, because otherwise `make config` will also make decisions based on the wrong toolchain.
When we set CROSS_COMPILE to set the toolchain prefix, everything works out of the box without using absolute paths everywhere. This removes a lot of duplication from the kernel derivations.
|
Result of 474 packages marked as broken and skipped:
228 packages failed to build:
2616 packages built:
|
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.
Even when building the Linux kernel with a
stdenvwhere the linker isld.lldinstead of binutilsld, the build picks up binutilsldfor linking. This prevents features that requireld.lldfrom working, such as LTO.Fix by being more specific about which tools we want to build with. This extends what manual-config.nix has already done for quite a while.
I've used the following to get
ld.lddas linker:Things left to be done
make configrecognizes the right linkernix-build clang-kernel.nix -A linux_llvmcurrently fails:Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.