Skip to content
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

Fixes for pkgsLLVM with llvmPackages_{15,16,git} #246577

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

pwaller
Copy link
Contributor

@pwaller pwaller commented Aug 1, 2023

Description of changes

cc maintainers @dtzWill @Ericson2314 @lovek323 @primeos @qyliss @RaitoBezarius @rrbutani @sternenseemann

With the following overlay, I'm able to build and run pkgsLLVM.nix (i.e, nontrivial C++ software) with LLVM 16.

As others have observed, the runtimes build machinery changed quite a bit in recent LLVM versions, so it was a little tricky to make this work, and I've had to tweak the build a bit. I suspect my tweaks may be have the potential to break other platforms, so help wanted!

Overlay:

import /home/pwaller/.local/src/nixpkgs {
  overlays = [(self: super: {
    llvmPackages = self.llvmPackages_16;

    libseccomp = super.libseccomp.overrideAttrs (final: prev: {
      # Some failing tests, not investigated.
      doCheck = false;
    });

    lowdown = super.lowdown.overrideAttrs (final: prev: {
      # Breaks detection of strlcpy (it gets optimized away during conf test).
      hardeningDisable = [ "fortify" ];
    });

    boost = super.boost.overrideAttrs (final: prev: {
      # Required, otherwise clang can't find the linker.
      # Because boost sets --target=x86_64-pc-linux, and the linker is named
      # x86_64-unknown-linux-gnu-lld.
      NIX_CFLAGS_LINK = prev.NIX_CFLAGS_LINK + " --target=${self.stdenv.hostPlatform.config}";
    });

    db48 = super.db48.overrideAttrs (final: prev: {
      configureFlags = prev.configureFlags
        ++ self.lib.optional (super.db48.stdenv.hostPlatform.useLLVM or false)
          # See https://discourse.llvm.org/t/configure-script-breakage-with-the-new-werror-implicit-function-declaration/65213
          "CFLAGS=-Wno-error=implicit-function-declaration";
    });

    shadow = super.shadow.overrideAttrs (final: prev: {
      configureFlags = prev.configureFlags
        ++ self.lib.optional (super.shadow.stdenv.hostPlatform.useLLVM or false)
          # See https://discourse.llvm.org/t/configure-script-breakage-with-the-new-werror-implicit-function-declaration/65213
          "CFLAGS=-Wno-error=implicit-function-declaration";
    });

    nix = super.nix.overrideAttrs (final: prev: {
      # Some tests failing (possibly because they want libgcc_s for pthread cancel?).
      postPatch = (prev.postPatch or "") + ''
        #  rm -r $sourceRoot/tests/{build-remote-content-addressed-fixed.sh,build-remote-input-addressed.sh}
        sed -i '/build-remote-input-addressed.sh/d' tests/local.mk
        sed -i '/build-remote-content-addressed-fixed.sh/d' tests/local.mk
        sed -i '/build-remote-content-addressed-floating.sh/d' tests/local.mk
      '';
    });
  })];
}

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

pwaller added a commit to pwaller/nixpkgs that referenced this pull request Aug 1, 2023
What changed:

* Fixed crtbeginS.o and crtendS.o missing
  (they may or may not be called crt{begin_end},{,_shared}.

* Fixed implicit function declaration causing build errors for various
  builds by supplying -Wno-implicit-function-declaration.

* Fixed __cxxabi_config.h missing, by adding -I${cxxabi}/include/c++/v1
  in the wrapper.

* Fixed libcxx failing to build due to missing libunwind symbols by
  making libunwind a propagated build input from libcxxabi, and setting
  -DLIBCXX_ADDITIONAL_LIBRARIES=unwind for stdenv.hostPlatform.useLLVM == true.

* libcxxabi wants to find libunwind at libunwind_shared.so, so symlink
  it there in libunwind.

What was tested:

* x86_64-linux, aarch64-linux, the stdenv builds.
  * Additionally I was able to get nix to build, with an overlay to fix
    a couple of minor issues in downstream packages (overlay supplied in
    PR NixOS#246577.
* aarch64-darwin fails spuriously in a single LLVM test
  strip-preserve-atime.test checking atime timestamps.
@pwaller
Copy link
Contributor Author

pwaller commented Aug 1, 2023

Improved the commit message; building nix with pkgsLLVM also works on aarch64-linux. I've also got an aarch64-darwin build going but it's going to take a long time and the first time around failed in an LLVM test called strip-preserve-atime comparing timestamps; in the LLVM tests. It's the only failing LLVM test, I don't believe that's related. So I'm trying another build with doCheckByDefault = false to save some time.

pwaller added a commit to pwaller/nixpkgs that referenced this pull request Aug 2, 2023
What changed:

* Fixed crtbeginS.o and crtendS.o missing
  (they may or may not be called crt{begin_end},{,_shared}.

* Fixed implicit function declaration causing build errors for various
  builds by supplying -Wno-implicit-function-declaration.

* Fixed __cxxabi_config.h missing, by adding -I${cxxabi}/include/c++/v1
  in the wrapper.

* Fixed libcxx failing to build due to missing libunwind symbols by
  including libunwind as a buildInput, and setting
  -DLIBCXX_ADDITIONAL_LIBRARIES=unwind for stdenv.hostPlatform.useLLVM == true.

* libcxxabi wants to find libunwind at libunwind_shared.so, so symlink
  it there in libunwind.

* Drop unneeded musl hack patch from libcxx.

What was tested:

* x86_64-linux, aarch64-linux, the stdenv builds.
  * Additionally I was able to get nix to build, with an overlay to fix
    a couple of minor issues in downstream packages (overlay supplied in
    PR NixOS#246577.

* aarch64-darwin fails spuriously in a single LLVM test
  strip-preserve-atime.test checking atime timestamps.
@reckenrode
Copy link
Contributor

I cherry-picked this PR and #241692 onto staging to build and test the Darwin stdenv update.

@reckenrode
Copy link
Contributor

I've also got an aarch64-darwin build going but it's going to take a long time and the first time around failed in an LLVM test called strip-preserve-atime comparing timestamps; in the LLVM tests. It's the only failing LLVM test, I don't believe that's related. So I'm trying another build with doCheckByDefault = false to save some time.

That’s interesting. I’ve only encountered a few transient failures on x86_64-darwin while working on the stdenv update. Unfortunately, I can’t remember off hand what they were. In general, the tests should run successfully.

@reckenrode
Copy link
Contributor

The aarch64-darwin stdenv built after I made the cc-flags change conditional. I’m building x86_64-darwin now, which I’ll check in the morning.

I deleted my comments regarding -Wno-implicit-function-declaration because I missed that it was for a specific clang. I do wonder whether it’s really necessary considering #234710 will require fixing those issues anyway, and many have been fixed already, but since it doesn’t really affect Darwin, I’m less concerned about it.

Copy link
Contributor Author

@pwaller pwaller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for the speedy reviews.

pkgs/development/compilers/llvm/16/default.nix Outdated Show resolved Hide resolved
@pwaller

This comment was marked as resolved.

@pwaller

This comment was marked as resolved.

@pwaller

This comment was marked as resolved.

@pwaller
Copy link
Contributor Author

pwaller commented Aug 19, 2023

Rebased against current master and applied relevant fixes for llvmPackages_15, which allows them to build. pkgsLLVM.hello also builds. However I'm not able to build pkgsLLVM.nix (with the overlay) because I get:

       > x86_64-unknown-linux-gnu-ld: error: unable to find library -l

I haven't yet dug into this further, but I would prefer not to hold up landing this; those other issues can be found and fixed by others wanting to experiment with this toolchain.

I haven't yet looked at llvmPackages_git.

@pwaller

This comment was marked as resolved.

@pwaller
Copy link
Contributor Author

pwaller commented Aug 21, 2023

@RaitoBezarius please take another look, I've addressed your question.

@pwaller Are those changes unique to LLVM16 or do they make sense for:

(a) LLVM git (b) LLVM 15?

I've applied the changes to LLVM git and LLVM 15. pkgsLLVM.hello builds and runs for both of those now. Some downstream C++ stuff is broken which I have not investigated since it works for LLVM 16.

@pwaller
Copy link
Contributor Author

pwaller commented Aug 21, 2023

badd8c3 fixed the infinite recursion issue I hit earlier. The git log suggests that you added that fix to LLVM 16 when it landed.

@pwaller pwaller changed the title Fixes for pkgsLLVM with llvmPackages_16 Fixes for pkgsLLVM with llvmPackages_{15,16,git} Aug 22, 2023
@ofborg ofborg bot requested a review from RaitoBezarius August 22, 2023 16:27
@RaitoBezarius
Copy link
Member

OK, I checked a nixpkgs-review and this builds.
Please can you rebase? I will approve and we can move forward.

What changed:

* Fixed crtbeginS.o and crtendS.o missing
  (they may or may not be called crt{begin_end},{,_shared}.

* Fixed implicit function declaration causing build errors for various
  builds by supplying -Wno-implicit-function-declaration.

* Fixed __cxxabi_config.h missing, by adding -I${cxxabi}/include/c++/v1
  in the wrapper.

* Fixed libcxx failing to build due to missing libunwind symbols by
  including libunwind as a buildInput, and setting
  -DLIBCXX_ADDITIONAL_LIBRARIES=unwind for stdenv.hostPlatform.useLLVM == true.

* libcxxabi wants to find libunwind at libunwind_shared.so, so symlink
  it there in libunwind.

* llvmPackages_16.libcxxabi: Pass -nostdlib via CMAKE_*_LINKER_FLAGS

  Without this flag, the link of libcxxabi.so tries to pull in libgcc and
  friends, from the clang compiler driver.

* Drop unneeded musl hack patch from libcxx.

* Pass -Wno-error=implicit-function-declaration only to compiler-rt

  See LLVM forum discussion:

  https://discourse.llvm.org/t/configure-script-breakage-with-the-new-werror-implicit-function-declaration/65213

  In summary, LLVM 16 made implicit function declaration an error. This
  happens a lot in configure scripts which can break things.

* llvmPackages_16: !isDarwin: Supply -DLIBCXX_ABI_USE_LLVM_UNWINDER=On

  Otherwise it fails with various undefined references to _Unwind_*
  functions: (full list: _Unwind_DeleteException _Unwind_GetIP
  _Unwind_GetLanguageSpecificData _Unwind_GetRegionStart
  _Unwind_RaiseException _Unwind_Resume _Unwind_SetGR _Unwind_SetIP).

* 16.libcxxabi: Only pass -nostdlib for useLLVM and Darwin builds

What was tested:

* x86_64-linux, aarch64-linux, the stdenv builds.
  * Additionally I was able to get nix to build, with an overlay to fix
    a couple of minor issues in downstream packages (overlay supplied in
    PR NixOS#246577.

* aarch64-darwin fails spuriously in a single LLVM test
  strip-preserve-atime.test checking atime timestamps.

* The same for pkgsLLVM with llvmPackages = llvmPackages_15.

Signed-off-by: Peter Waller <p@pwaller.net>
Signed-off-by: Peter Waller <p@pwaller.net>
Comment on lines +73 to +77
# libcxxabi's CMake looks as though it treats -nostdlib++ as implying -nostdlib,
# but that does not appear to be the case for example when building
# pkgsLLVM.libcxxabi (which uses clangNoCompilerRtWithLibc).
"-DCMAKE_EXE_LINKER_FLAGS=-nostdlib"
"-DCMAKE_SHARED_LINKER_FLAGS=-nostdlib"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this? I would think we do wanna link libc but don't want to link (an already-existing) C++ standard library implementation. -nostdlib++ without -nostdlib therefore seems correct to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, this is coming back to it some time after I implemented it, so I have forgotten the detail here and I have limited cycles to spare to dive back in. I suspect it has to do with making cmake configure, or linking succeed. I presume if you take it out you'll find the same failure I experienced? If not, maybe there is something further downstream which is broken.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks @pwaller. So far, it actually seems to be working fine for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your comment is ambigous with two possible meanings:

  1. So far it works fine with the lines removed (as I suggested)
  2. So far it works fine as it currently is (with my lines present)

I read your intended response as (2).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pwaller: I think it's (1); see: c84a96b from #311836

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cross reference. I looked into it a little, I suspect the issue this was trying to fix went away when libcxxabi was merged into libcxx.

@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants