[staging-next] firefox: use rustc.llvmPackages.stdenv with LLVM bintools, fix deadlock

[oxalica]

Motivation for this change

Fix #144670
ZHF: #144627

Follows #144670 (comment)

Update: Currently we switch to llvmPackages*.stdenv with LLVM bintools to build firefox. LLVM and cc-wrapper are not touched.

Outdated:

In clangUseLLVM we explicit use FULL LLVM toolchain, we should not set --gcc-toolchain anymore.

Well, it seems cannot explain why LLVM 12 works when this option is provided. According Clang 13.0.0 Release Notes, there are some internal changes occurs on -B and --gcc-toolchain. In our case and some experiments, in LLVM 13, when --gcc-toolchain is specified, its include path will take precedence over libc++ and mess up everything, no matter whether this option is before -isystem or after. I guess that we are just not expected to set gcc-toolchain in full LLVM toolchain usage but it used to work due to some magic.

Currently I just remove --gcc-toolchain from llvmPackages_13.clangUseLLVM, not any older version.
I checked that LLVM 12 with that option removed doesn't affect a simple hello-world build, but not sure if downstream packages still build. So I'm just not touching them.

After this PR, firefox builds and runs flawless for me.
Note that clangUseLLVM is currently only used by firefox and cross compilation, I think no other package is affected.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 21.11 Release Notes (or backporting 21.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[sternenseemann]

I can appreciate why we need a new argument to cc-wrapper here, but I kind of dislike it. I think this is partially due to the awkward place LLVM is in with some many different consumers in very different bootstrapping scenarios (see also #142901 for example). I'm not sure if there's a nicer alternative in this case that is also simple to implement as a staging-next fix.

I think in this case I'd just suggest to use a different name for the thing at hand. useLLVM is already vague enough in its current form and here it is just supposed to do a single specific thing: How about allowGccLibs or addGccToolchain or something along those lines? It's a bit tricky because other factors play a role.

[oxalica]

@sternenseemann

I think in this case I'd just suggest to use a different name for the thing at hand. useLLVM is already vague enough in its current form and here it is just supposed to do a single specific thing: How about allowGccLibs or addGccToolchain or something along those lines? It's a bit tricky because other factors play a role.

I'd prefer useGccLibs and useGccToolchain.

Also worth mentioning that the original issue is only about --gcc-toolchain, not about useGccForLibs (-L, -B and etc). It's just my change on useLLVM that turns off both of them. Because I personally think in pure LLVM toolchain, there should be no gcc-related things at all in stdenv, and if pkgs requires both gcc and LLVM, they should explicit introduce the other one. That actually happens since some rust crates in firefox explicitly emit -lgcc and/or -lstdc++, so I need to add them back in nativeBuildInputs.

What's you opinion on this? Should we keep gccLibs for clangUseLLVM, or drop it and let packages themselves to introduce if needed (like what I did currently)?

[sternenseemann]

What's you opinion on this? Should we keep gccLibs for clangUseLLVM, or drop it and let packages themselves to introduce if needed (like what I did currently)?

It makes more sense to drop the gcc stuff, because an useLLVM stdenv doesn't have them. If we are to expose clangUseLLVM it should behave like pkgsLLVM.stdenv as much as possible.

This whole annoyance brings me to consider whether we should be exposing clangUseLLVM at all: There is no way for packages to distinguish between clangUseLLVM (with compiler-rt) and clangStdenv (with libgcc), since it is only exposed via the platform at the moment (I guess we could expose some kind of runtime lib flag from the compiler in the future).

I think there may be a way to sidestep the whole issue for now: Do you know why clangUseLLVM was used in the first place? Does firefox need compiler-rt or is libgcc as rtlib also fine? Using an LLVM stdenv with libgcc and adding lld in may be easier than this dance here…

[oxalica]

I think there may be a way to sidestep the whole issue for now: Do you know why clangUseLLVM was used in the first place?

It was introduced for LTO support and used to be lldClang in #99922, and changed to clangUseLLVM later.

Does firefox need compiler-rt or is libgcc as rtlib also fine? Using an LLVM stdenv with libgcc and adding lld in may be easier than this dance here…

I'm not sure if compiler-rt is requied. But note that llvmPackages*.stdenv uses non-LLVM bintools, (Yeah, another hidden difference 😞 ) but firefox expecting llvm-ar and friends when LTO is enabled.
I tried to build with llvmPackages_13.stdenv and llvmPackages_13.{lld,bintools} in nativeBuildInputs but with no luck. It somehow tries to use AR=/path/to/non-llvm-bintools/llvm-ar and of course it doesn't exist.

[sternenseemann]

Right, my plan was to reintroduce lldClang in some shape or form anyways, so this may very well be a good opportunity, see #142901. If you're interested, I can try making a PR for this in a bit?

[sternenseemann]

Current draft for a quick and simpler workaround:

diff --git a/pkgs/applications/networking/browsers/firefox/common.nix b/pkgs/applications/networking/browsers/firefox/common.nix
index c99d1c10a12..e1ad8e030fc 100644
--- a/pkgs/applications/networking/browsers/firefox/common.nix
+++ b/pkgs/applications/networking/browsers/firefox/common.nix
@@ -109,9 +109,11 @@ let
 
   # When LTO for Darwin is fixed, the following will need updating as lld
   # doesn't work on it. For now it is fine since ltoSupport implies no Darwin.
+  # TODO(@sternenseemann): reintroduce lldClang and replace hack (see #142901)
   buildStdenv = if ltoSupport
-                then overrideCC stdenv llvmPackages.clangUseLLVM
-                else stdenv;
+                then overrideCC stdenv (llvmPackages.clang.override {
+                  inherit (llvmPackages) bintools;
+                }) else stdenv;
 
   # --enable-release adds -ffunction-sections & LTO that require a big amount of
   # RAM and the 32-bit memory space cannot handle that linking

Just started a build, fingers crossed…

[oxalica]

It's built successfully. I also included a patch from Arch fixing a deadlock. I just ran into it when playing the previous build.
Seems we don't need explicit gccLib now.

This comment is sent from the built result.