llvmPackages_*.libcxx: include libcxxabi within libcxx

[pwaller]

Test case: nixpkgs#pkgsStatic.pkgsLLVM.ncurses

Prior to this patch, this fails with errors such as:

error: undefined symbol: __cxa_throw

I think this is a reasonable solution because in #292043, libcxxabi was
'merged into libcxx', however, the commit message suggests that only
dynamic linking was accounted for, because it says:

* linux/freebsd `libc++.so` is a linker script `LINK(libc++.so.1, -lc++abi)` making `-lc++` sufficient.

Whereas, I found that if I tried linking a "hello world" C++ program
with a static hostPlatform, it failed unless -lc++abi was passed.

Signed-off-by: Peter Waller [email protected]

Description of changes

Set -DLIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=On for hostPlatform.isStatic so that we get the equivalent behaviour is for dynamic linking that -lc++ is enough to pull in all the needed code.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• 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 rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 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.

---

Add a 👍 reaction to pull requests you find important.

[pwaller]

I sent this to master rather than staging on the understanding that it should be reasonable to do this for pkgsStatic.pkgsLLVM, which is likely broken without this change anyway.

One thing I've not considered in the above is if there is some other static platform where it is legitimate to combine libc++ with a different ABI other than libc++abi - I guess in that case you actually don't want c++abi included within libc++. If anyone has any thoughts as to whether that could be problematic.

[ghost]

seems reasonable. see also #305876 (that was my PR as was the libcxx / libcxxabi merge but i deleted my github account).

updated 305876.diff

diff --git a/pkgs/development/compilers/llvm/common/libcxx/default.nix b/pkgs/development/compilers/llvm/common/libcxx/default.nix
index 0e91f50551c5..11d9ced609cf 100644
--- a/pkgs/development/compilers/llvm/common/libcxx/default.nix
+++ b/pkgs/development/compilers/llvm/common/libcxx/default.nix
@@ -126,6 +126,31 @@ stdenv.mkDerivation (rec {
   postInstall = lib.optionalString (cxxabi != null) ''
     lndir ${lib.getDev cxxabi}/include $dev/include/c++/v1
     lndir ${lib.getLib cxxabi}/lib $out/lib
+    libcxxabi=$out/lib/lib${cxxabi.libName}.a
+  ''
+  # LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=ON doesn't work for LLVM < 16 or
+  # external cxxabi libraries so merge libc++abi.a into libc++.a ourselves.
+
+  # GNU binutils emits objects in LIFO order in MRI scripts so after the merge
+  # the objects are in reversed order so a second MRI script is required so the
+  # objects in the archive are listed in proper order (libc++.a, libc++abi.a)
+  + ''
+    libcxxabi=''${libcxxabi-$out/lib/libc++abi.a}
+    if [[ -f $out/lib/libc++.a && -e $libcxxabi ]]; then
+      $AR -M <<MRI
+        create $out/lib/libc++.a
+        addlib $out/lib/libc++.a
+        addlib $libcxxabi
+        save
+        end
+    MRI
+      $AR -M <<MRI
+        create $out/lib/libc++.a
+        addlib $out/lib/libc++.a
+        save
+        end
+    MRI
+    fi
   '';
 
   passthru = {
diff --git a/pkgs/test/cc-wrapper/default.nix b/pkgs/test/cc-wrapper/default.nix
index a0088751d4a2..f0da2dbddeb8 100644
--- a/pkgs/test/cc-wrapper/default.nix
+++ b/pkgs/test/cc-wrapper/default.nix
@@ -46,6 +46,16 @@ in stdenv.mkDerivation {
       $READELF -d ./atomics.so | grep libatomic.so && echo "ok" >&2 || echo "failed" >&2
     ''}
 
+    ${lib.optionalString isCxx ''
+      echo "checking whether can link with libc++... " >&2
+      $CXX ${./cxx-main.cc} -c -o cxx-main.o
+      $CC cxx-main.o -lc++ -o cxx-main
+      $CC cxx-main.o ${lib.getLib stdenv.cc.libcxx}/lib/libc++.a -o cxx-main-static
+      ${emulator} ./cxx-main
+      ${emulator} ./cxx-main-static
+      rm cxx-main{,-static,.o}
+    ''}
+
     ${lib.optionalString (stdenv.isDarwin && stdenv.cc.isClang) ''
       echo "checking whether compiler can build with CoreFoundation.framework... " >&2
       mkdir -p foo/lib

[pwaller]

Thanks for the reference to the closed PR, I had missed that and the additional context. I would like to see this fix landed so I'll leave this open for comments; I take it that this won't fix things for older LLVMs. Advice welcomed.

I'd be interested if the community might also accept this since it fixes things more recent toolchains at least, even if it doesn't fix all of them (which could be fixed at a later date).

I've pulled across the test you wrote for #305876.

[pwaller]

/cc potentially interested reviewers from the other thread:
@Ericson2314 @rrbutani @RossComputerGuy @Mindavi

[rrbutani]

This looks good, thank you!

Just one nit/question:

[ghost]

@ofborg build tests.cc-wrapper

[pwaller]

@ofborg build tests.cc-wrapper

[pwaller]

I think I've accounted for everything that was known (and mentioned in the other thread #305876).

Most recent update:

• For versions before 16, merge the two archive files with ar.
• Fix the cc-wrapper tests.
• Update PR title / commit message.
• Do the merging differently if cxxabi != null.
• Move the LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY flag from cxxabiCMakeFlags to cxxCMakeFlags. It relates to the ABI but it is a libcxx flag.

[ghost]

this will now need need to target staging.

to support cxxabi != null and all LLVM versions my preference is to use the diff embedded in #307184 (comment) since that is the minimal change. I don't see the value in using both LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY and ar given the extra complexity and the fact that the ar script is 4 lines long.

also note that using LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY prepends libc++abi.a to libc++.a which is the reverse order of what the shared libs do in the linker script. it probably doesn't really matter except if there are objects that in libc++abi and libc++ which contain the same symbol.

nonetheless, some minor comments inline.

[pwaller]

I've reinstated @annaleeleaves's diff for generating the AR and retested on x86_64-linux. I wasn't able to get a freebsd test to work.

[pwaller]

Also retargeted staging.

[ghost]

@pwaller thanks for working on this but unfortunately the change still is not correct.
nix-build -A libcxx && ar t result/lib/libc++.a shows duplicate objects in libc++.a

suggested changes

diff --git a/pkgs/development/compilers/llvm/common/libcxx/default.nix b/pkgs/development/compilers/llvm/common/libcxx/default.nix
index 8cad3067ca6d..5b4c2ca56ae1 100644
--- a/pkgs/development/compilers/llvm/common/libcxx/default.nix
+++ b/pkgs/development/compilers/llvm/common/libcxx/default.nix
@@ -67,12 +67,12 @@ let
 
   cxxCMakeFlags = [
     "-DLIBCXX_CXX_ABI=${cxxabiName}"
-  ] ++ lib.optionals (cxxabi == null && (lib.versionAtLeast release_version "16")) [
+  ] ++ lib.optionals (cxxabi == null && lib.versionAtLeast release_version "16") [
     # Note: llvm < 16 doesn't support this flag (or it's broken); handled in postInstall instead.
     # Include libc++abi symbols within libc++.a for static linking libc++;
     # dynamic linking includes them through libc++.so being a linker script
     # which includes both shared objects.
-    "-DLIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=On"
+    "-DLIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=ON"
   ] ++ lib.optionals (cxxabi != null) [
     "-DLIBCXX_CXX_ABI_INCLUDE_PATHS=${lib.getDev cxxabi}/include"
   ] ++ lib.optionals (stdenv.hostPlatform.isMusl || stdenv.hostPlatform.isWasi) [
@@ -129,9 +129,7 @@ stdenv.mkDerivation (rec {
   # libc++.so is a linker script which expands to multiple libraries,
   # libc++.so.1 and libc++abi.so or the external cxxabi. ld-wrapper doesn't
   # support linker scripts so the external cxxabi needs to be symlinked in
-  postInstall = ''
-    libcxxabi=$out/lib/libc++abi.a
-  '' + lib.optionalString (cxxabi != null) ''
+  postInstall = lib.optionalString (cxxabi != null) ''
     lndir ${lib.getDev cxxabi}/include $dev/include/c++/v1
     lndir ${lib.getLib cxxabi}/lib $out/lib
     libcxxabi=$out/lib/lib${cxxabi.libName}.a
@@ -142,7 +140,7 @@ stdenv.mkDerivation (rec {
   # GNU binutils emits objects in LIFO order in MRI scripts so after the merge
   # the objects are in reversed order so a second MRI script is required so the
   # objects in the archive are listed in proper order (libc++.a, libc++abi.a)
-  + ''
+  + lib.optionalString (cxxabi != null || lib.versionOlder release_version "16") ''
     libcxxabi=''${libcxxabi-$out/lib/libc++abi.a}
     if [[ -f $out/lib/libc++.a && -e $libcxxabi ]]; then
       $AR -M <<MRI

[pwaller]

Thanks @annaleeleaves - Note that I didn't get a notification for your edit which added suggested changes, so I was unaware of it until now.

Also ambiguous to me is whether the suggestions address your comment that the symbols are duplicated. I had a quick look for duplicated symbols with nm building pkgsStatic.pkgsLLVM.llvmPackages_17.libcxx, but I don't see them. If the problem is still present please would you mind sharing what test I can use as to whether it's right?

[pwaller]

Forgot to mention in the above comment: I believe I've applied your suggestions.

[ghost]

Also ambiguous to me is whether the suggestions address your comment that the symbols are duplicated. I had a quick look for duplicated symbols with nm building pkgsStatic.pkgsLLVM.llvmPackages_17.libcxx, but I don't see them. If the problem is still present please would you mind sharing what test I can use as to whether it's right?

it is fixed with the suggested changes. i listed a test in #307184 (comment) nix-build -A libcxx && ar t result/lib/libc++.a. ar t result/lib/libc++.a lists the objects in the archive. before this change the objects from libc++abi.a were added twice for llvm16+, with the change only once. nm also shows the duplicates. before this change there were 8024 lines listed for llvm17 libc++.a, after this change there are 6364.

[ghost]

change looks good. my preference is still the diff in #307184 (comment) but this works too.

tested linux x64 nix-build -A tests.cc-wrapper and also tested the changes work when libcxxrt is used via slight modifications to libcxxrt and libcxx.

also tested nix-build -A tests.cc-wrapper.llvmTests on darwin x64 with minor changes to skip llvm16 so to not require building stdenv.

[ghost]

@ofborg build tests.cc-wrapper.llvmTests
(failures are timeouts)

[rrbutani]

@annaleeleaves, @pwaller: Thanks for your work on this.

[ghost]

@wegank
would be good to get this in for 24.05 #303285 as there are only one or two staging cycles left. don't think this would be considered a breaking change as it fixes a bug introduced in #292043 (which removed -lc++abi from cc-wrapper for clang++ and thus changed the behaviour for linking static libs).

this change not only rectifies that bug but also fixes long standing bug clang -x c++ -lc++ hello.cxx for static linking ( #292043 fixed it for shared libs).

[pwaller]

I don't have commit rights, please can someone merge?

Thanks for the review input, all!

[kvtb]

Does it have sense to have two blocks with "create $out/lib/libc++.a" commands in sequence?
The second overwrites what did the first

[pwaller]

The second overwrites what did the first
This comment discusses the rationale; #307184 (comment)

AFAIU it has the effect of reversing the order of objects in the archive, so the second invocation sets the order straight.