-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
llvmPackages_*.clang: bind linker on compiling clang #220595
base: staging
Are you sure you want to change the base?
Conversation
seems like a loop occurs, let me find out how to fix |
18dd5e1
to
442b21a
Compare
The infinite recursion is caused by wasi libc introduced by firefox. But I find it hard to figure out how the recursion occurs. Is there someone familier with bootstrapping to offer a help? |
It's a recursion in |
It has been a while. Now I finally figured out how the circular dependency occurs. Recall that in our original implementation, we imitate gcc
and add the following flags to clang diff --git a/pkgs/development/compilers/llvm/12/clang/default.nix b/pkgs/development/compilers/llvm/12/clang/default.nix
index 7ecd4efc0837..d8f6e50b348c 100644
--- a/pkgs/development/compilers/llvm/12/clang/default.nix
+++ b/pkgs/development/compilers/llvm/12/clang/default.nix
@@ -2,6 +2,7 @@
, buildLlvmTools
, fixDarwinDylibNames
, enableManpages ? false
+, targetPackages
}:
let
@@ -40,6 +41,8 @@ let
] ++ lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform) [
"-DLLVM_TABLEGEN_EXE=${buildLlvmTools.llvm}/bin/llvm-tblgen"
"-DCLANG_TABLEGEN=${buildLlvmTools.libclang.dev}/bin/clang-tblgen"
+ ] ++ lib.optionals (stdenv.hostPlatform != stdenv.targetPlatform) [
+ "-DCLANG_DEFAULT_LINKER=${targetPackages.stdenv.cc.bintools}/bin/${stdenv.targetPlatform.config}-ld"
];
patches = [ This change introduced two circles: Circle 1
How does gcc avoid this circle? The magic occurs in the last dependency step. For gcc-based pkgsCross, Now we do the same thing to diff --git a/pkgs/development/compilers/llvm/12/default.nix b/pkgs/development/compilers/llvm/12/default.nix
index b976dd2ee67a..81bed7d9db96 100644
--- a/pkgs/development/compilers/llvm/12/default.nix
+++ b/pkgs/development/compilers/llvm/12/default.nix
@@ -210,7 +210,9 @@ let
};
clangNoLibc = wrapCCWith rec {
- cc = tools.clang-unwrapped;
+ cc = tools.clang-unwrapped.override {
+ targetPackages.stdenv.cc.bintools = bintoolsNoLibc';
+ };
libcxx = null;
bintools = bintoolsNoLibc';
extraPackages = [
@@ -223,7 +225,9 @@ let
}; Circle 2But the story is not coming to the end. There is another circle:
Luckily similar solution works: diff --git a/pkgs/development/compilers/llvm/12/default.nix b/pkgs/development/compilers/llvm/12/default.nix
index b976dd2ee67a..81bed7d9db96 100644
--- a/pkgs/development/compilers/llvm/12/default.nix
+++ b/pkgs/development/compilers/llvm/12/default.nix
@@ -223,7 +225,9 @@ let
};
clangNoCompilerRt = wrapCCWith rec {
- cc = tools.clang-unwrapped;
+ cc = tools.clang-unwrapped.override {
+ targetPackages.stdenv.cc.bintools = bintoolsNoLibc';
+ };
libcxx = null;
bintools = bintoolsNoLibc';
extraPackages = [ ]; |
442b21a
to
261b67c
Compare
261b67c
to
d847d68
Compare
Now the circle occurs in darwin. Yet another odyssey to figure out why. |
Thank you for this amazing PR, I was going to embark on the journey too. |
To reproduce the error locally:
|
You may want to look at #256590. Darwin cross is messy because the source-based SDK is prone infinite recursions if you’re not careful. The stdenv bootstrap is able to do that, but cross-compilation doesn’t allow that level of control. #260543 is another PR that fixes cross-compilation of LLVM itself, which I found was necessary for building some things after I got x86_64-darwin cross-compilation working. The merge conflict is probably due to #260963, which made curl build like a normal package on Darwin, but it required stdenv bootstrap changes to break more infinite recursions. I was planning to fix #256590 after this staging-next (where my focus is on getting the default LLVM update through), but I can fix the merge conflict and re-push the PR if that would be helpful or there’s interest in getting it into staging for the next staging-next cycle. |
] ++ lib.optionals (stdenv.hostPlatform != stdenv.targetPlatform) [ | ||
"-DCLANG_DEFAULT_LINKER=${targetPackages.stdenv.cc.bintools}/bin/${stdenv.targetPlatform.config}-ld" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure targetPackages
is the right thing to use here?
targetPackages
means pkgsTargetTarget
, which is the packages which run on the targetPlatform
and emit binaries for the targetPlatform
.
When you're building a (build==host)!=target
compiler, its linker will run on the hostPlatform
But I don't know clang very well so maybe I am misreading the meaning of -DCLANG_DEFAULT_LINKER
. @Ericson2314 would know for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I agree with @amjoseph-nixpkgs here.
I would expect rather pkgsBuildTarget
to be used here, but maybe there's some splicing surprises… ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might look suprising. Yes targetPackages
is the packages that run on targetPlatform and emit binaries for the targetPlatform, but targetPackages.stdenv
is something else. It is the environment that produces targetPackages
. Now that targetPackages
is the cross packages, targetPackages.stdenv.cc.bintools
is the bintools that runs on host platform and emit target platform binaries.
Holy smokes these treewide operations on |
@rrbutani tried… It was hard because we need to unify all changes first :( |
When using clang for cross compilation, clang cannot find the linker. Because, for consistency with gcc, given a linker name, clang finds the linker in the following order: 1. in prefix dirs, which contains toolchain specific paths and paths specified by `-B` 2. in ProgramPaths 3. in PATH environment (refer to `Driver::GetProgramPath` in `clang/lib/Driver/Driver.cpp`) (also refer to https://gcc.gnu.org/onlinedocs/gccint/Collect2.html) The step 2 and 3 considers all possible target triple prefixes, but the step 1 does not. In nixpkgs, all binaries in cross toolchains are prefixed by the target triple, thus clang cannot find them. For gcc in nixpkgs, the linker path is hardcoded in configuration phase. This commit copies this behavior to clang. But the added cmakeFlags also introduce circular dependency. The circle comes from the fact that clangNoLibc and clangNoCompilerRT introduce dependency of libc unexpectedly via cmakeFlags of their underlying clang. We need to override bintools for them. Refer to NixOS#220595 (comment) for details.
d847d68
to
b89ce8b
Compare
Added fixes for darwin packages. Let's see if ofborg likes it. |
Sorry I'm late for the party, but I want to advocate for rejecting this PR. Rationale:
Therefore, it's so much better to handle this in a wrapper, we should do so even if it means a little bit of pain for some use cases. Note that I'm not saying other use cases should be harder: it's just that they probably need to be enabled in a different way. One observation is that developing the alternative way should be much easier, since it will only involve a rebuild of the compiler wrapper, and not the compiler. Another observation is that if you do this right, you never have to rebuild clang, because you can simply use one which is already a cache hit. The effect is that you can get to cross compiling your desired workload much faster. See for example a case where I eliminated a rebuild of clang, enabling reuse of the one in cache.nixos.org: #351685 There has also been some discussion about tweaking |
I agree that the method in this PR maybe came to a deadend. I found it even more difficult to eliminate the cyclic dependency in the darwin toolchain. That is also why this PR has been stagnating for so long. The initial idea is to make clang work the same way as gcc, which is exactly how this PR (not) works. Modifying cc-wrapper might be more feasible. But the idea of modifying cc-wrapper also urges us to rethink whether gcc is doing in the right way. Should we also change the way gcc works? Anyway I do not think that rebuilding clang is a major problem because both modifying clang and modifying cc-wrapper will trigger a mass rebuild in nixpkgs. |
gcc, unlike clang, is not a cross compiler by default. So it's already rebuilding if you need a different target. I don't see the sense of trying to make these have an equivalent implementation. By "Avoiding clang rebuilds", I don't mean over time, I mean, between targets. So, if I want to cross compile to an exotic target in an exotic configuration, if you have things like this in the description of the clang build, it is necessary to rebuild clang before you even start. If you just have to rebuild a clang wrapper, it is instant. So the user experience for building for their exotic target is substantially better, and it means they can use an existing compiler build from cache.nixos.org. And it's not just the rebuild cost, but it's also the storage cost. If I'm building for multiple targets, I'd much rather have one clang build than many lying around. |
Darwin should be a lot easier to handle since #346043 was merged, but I would also advocate for rejecting this PR. As part of that work, I made Darwin mostly build LLVM using the LLVM bootstrap. Once @emilazy’s and @paparodeo’s work to increase the minimum SDK and update Darwin to clang 19 lands, I plan to finish that work on the LLVM bootstrap. Darwin will build LLVM using LLVM bintools including the linker (LLD). Having to build clang again to use ld64 for the rest of the Darwin bootstrap doesn’t make sense. One change I would support (because it affects Darwin cross-compilation) is making clang try |
When using clang for cross compilation, clang cannot find the linker.
Example (works for clang 12 and later):
Let me explain why this happens. For consistency with gcc, given a linker name (
ld
), clang finds the linker in the following order:-B
command line args.The step 2 and 3 considers all possible target triple prefixes, but the step 1 does not. In nixpkgs, all binaries in cross toolchains are prefixed by the target triple, thus clang cannot find them.
Related LLVM source: https://github.com/llvm/llvm-project/blob/8dfdcc7b7bf66834a761bd8de445840ef68e4d1a/clang/lib/Driver/Driver.cpp#L5857-L5897
Related LLVM commit: llvm/llvm-project@3452a0d
Related gcc doc: https://gcc.gnu.org/onlinedocs/gccint/Collect2.html
For gcc in nixpkgs, the linker path is hardcoded in configuration phase (as is shown below). This commit copies this behavior to clang.
nixpkgs/pkgs/development/compilers/gcc/common/configure-flags.nix
Line 38 in 5c5ca01
Description of changes
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)