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

cc-wrapper: broaden explicit libc++abi linking for LLVM stdenv #181485

Merged
merged 1 commit into from
Jul 22, 2022

Conversation

Itaros
Copy link
Contributor

@Itaros Itaros commented Jul 14, 2022

Description of changes

This change ensures that libc++abi is linked on all platforms explicitly by cc-wrapper if stdenv is LLVM.

This is designed to address #166205

Rationale:

Currently, attempt to build c++ software which requires abi-realted features like exceptions using LLVM >=12 breaks link phase.

For example, boost build(fails):

nix-build -E "with import {}; pkgs.boost-build.override {stdenv = pkgs.llvmPackages_14.stdenv;}"

> clang++ -x c++ -std=c++11 -O3 -s -DNDEBUG builtins.cpp class.cpp command.cpp compile.cpp constants.cpp cwd.cpp debug.cpp debugger.cpp execcmd.cpp execnt.cpp execunix.cpp filesys.cpp filent.cpp fileunix.cpp frames.cpp function.cpp glob.cpp hash.cpp hcache.cpp hdrmacro.cpp headers.cpp jam_strings.cpp jam.cpp jamgram.cpp lists.cpp make.cpp make1.cpp md5.cpp mem.cpp modules.cpp native.cpp object.cpp option.cpp output.cpp parse.cpp pathnt.cpp pathsys.cpp pathunix.cpp regexp.cpp rules.cpp scan.cpp search.cpp startup.cpp subst.cpp sysinfo.cpp timestamp.cpp variable.cpp w32_getreg.cpp modules/order.cpp modules/path.cpp modules/property-set.cpp modules/regex.cpp modules/sequence.cpp modules/set.cpp -o b2
ld: warning: option -s is obsolete and being ignored
Undefined symbols for architecture arm64:
  "operator delete(void*)", referenced from:
      builtin_normalize_path(frame*, int) in builtins-da63fb.o
      var_parse_group_compile(VAR_PARSE_GROUP const*, compiler*) in function-7967a9.o
      var_parse_to_string(VAR_PARSE_VAR const*, bool) in function-7967a9.o
      var_parse_to_string(VAR_PARSE_GROUP*, bool) in function-7967a9.o
      b2::paths::normalize(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in pathsys-eb8532.o
      b2::startup::builtin_boost_build(frame*, int) in startup-0cdd77.o
      b2::startup::bootstrap(frame*) in startup-0cdd77.o
      ...
  "operator new(unsigned long)", referenced from:
      var_parse_to_string(VAR_PARSE_VAR const*, bool) in function-7967a9.o
      b2::startup::builtin_boost_build(frame*, int) in startup-0cdd77.o
      b2::startup::bootstrap(frame*) in startup-0cdd77.o
      std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > std::__1::operator+<char, std::__1::char_traits<char>, std::__1::allocator<char> >(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in startup-0cdd77.o
  "___cxa_allocate_exception", referenced from:
      std::__1::__throw_length_error(char const*) in function-7967a9.o
      std::__1::__throw_length_error(char const*) in startup-0cdd77.o
  "___cxa_begin_catch", referenced from:
      ___clang_call_terminate in startup-0cdd77.o
  "___cxa_free_exception", referenced from:
      std::__1::__throw_length_error(char const*) in function-7967a9.o
      std::__1::__throw_length_error(char const*) in startup-0cdd77.o
  "___cxa_throw", referenced from:
      std::__1::__throw_length_error(char const*) in function-7967a9.o
      std::__1::__throw_length_error(char const*) in startup-0cdd77.o
  "___gxx_personality_v0", referenced from:
      builtin_normalize_path(frame*, int) in builtins-da63fb.o
      var_parse_group_compile(VAR_PARSE_GROUP const*, compiler*) in function-7967a9.o
      var_parse_to_string(VAR_PARSE_VAR const*, bool) in function-7967a9.o
      var_parse_to_string(VAR_PARSE_GROUP*, bool) in function-7967a9.o
      std::__1::__throw_length_error(char const*) in function-7967a9.o
      b2::paths::normalize(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in pathsys-eb8532.o
      b2::startup::builtin_boost_build(frame*, int) in startup-0cdd77.o
      ...
ld: symbol(s) not found for architecture arm64
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)

For example, cmake(succeeds):

nix-build -E "with import {}; pkgs.cmake.override {stdenv = pkgs.llvmPackages_14.stdenv;}"

There is an attempt to introduce the same change as in #181143 which has proposed commit as focused on fixing zig instead of enforcing libc++abi link as a separate concern.

In conclusion: why enforcing libc++abi link?

  • This is what is already done for linux.
  • There is no point in doing it differently and adding isDarwin duct tape to every libc++ ABI-consuming package considering the actual reason behind the requirement. See following point:
  • There is a discussion which I find interesting related to static linking shenanigans( https://reviews.llvm.org/D96070 ) which provides quite a few insights: Apple stock system libc++ is indeed already contains libc++abi inside which is reasonable considering this system is strictly single ABI, while LLVM libc++ does and will continue to require explicit ABI specification at link time because there are a few different ABIs available(which opens a huge can of worms related to cc-wrapper - it is quite unhealthy to do linking spec in there in a such blunt way. The proper way would be to expose ability to specify libc++ ABI for LLVM stdenvs and link accordingly(both for Linux and Darwin), but for now this is quite okay, I think).
  • Usage of reexport seems to be obsolete legacy or at least of libc++ internals support, but I can't find strong evidence about it as associated changes are quite contradictory(see linked nixpkgs issue for examples)

Some random test runs after the change:

nix-build -E "with import ./.{}; pkgs.boost-build.override {stdenv = pkgs.llvmPackages_14.stdenv;}"
otool -L result/bin/b2

result/bin/b2:
        /nix/store/j146zdsqp37rlidxarnj5zigh6ilqvrs-libcxxabi-14.0.6/lib/libc++abi.1.dylib (compatibility version 1.0.0, current version 1.0.0)
        /nix/store/jwk3wdc1fwbw7qvd2jjsgcnlwwy99z5d-libcxx-14.0.6/lib/libc++.1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.60.1)

nix-build -E "with import ./.{}; pkgs.boost-build.override {stdenv = pkgs.llvmPackages_11.stdenv;}"
otool -L result/bin/b2

result/bin/b2:
        /nix/store/vszv8b85jk3f5ad6bi2y4jcmrvrb6qjh-libcxxabi-11.1.0/lib/libc++abi.1.dylib (compatibility version 1.0.0, current version 1.0.0)
        /nix/store/a6hrx5qnk5sfjc45y3dzskdjb6y049nd-libcxx-11.1.0/lib/libc++.1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.60.1)

nix-build -E "with import ./.{}; pkgs.boost-build"
otool -L result/bin/b2

result/bin/b2:
        /nix/store/vszv8b85jk3f5ad6bi2y4jcmrvrb6qjh-libcxxabi-11.1.0/lib/libc++abi.1.dylib (compatibility version 1.0.0, current version 1.0.0)
        /nix/store/a6hrx5qnk5sfjc45y3dzskdjb6y049nd-libcxx-11.1.0/lib/libc++.1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.60.1)

nix-build --system x86_64-darwin -E "with import ./.{}; pkgs.boost-build.override {stdenv = pkgs.llvmPackages_14.stdenv;}"
otool -L result/bin/b2

result/bin/b2:
        /nix/store/h02cjzyvvcwggdha3nlzj3drlb7a3jp4-libcxxabi-14.0.6/lib/libc++abi.1.dylib (compatibility version 1.0.0, current version 1.0.0)
        /nix/store/fah6lawx65gr3dwkhzkd097igfq070qa-libcxx-14.0.6/lib/libc++.1.0.dylib (compatibility version 1.0.0, current version 1.0.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.60.2)
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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

Currently running these builds to verify this works on x86_64-darwin hardware.

Do we know what the rationale was to guard it behind isLinux in the first place?

Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

I can confirm the linking errors are fixed when always including libc++abi. I am still curious why it was guarded by isLinux in the first place though?

@bobby285271 bobby285271 added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Jul 19, 2022
@toonn toonn merged commit 946c3d8 into NixOS:staging Jul 22, 2022
@winterqt
Copy link
Member

@Itaros Do you happen to know why this issue crops up when using LLVM 11 (the default in our Darwin stdenv) to compile LLVM 13, as does in Zig? Since this issue only occurs with LLVM >= 12, and we're compiling LLVM 13 with LLVM 11, I'm a bit lost on how this issue would occur.

@vcunat
Copy link
Member

vcunat commented Aug 3, 2022

There are some breakages apparently caused by this:

(also the same packages on aarch64-darwin)

@winterqt
Copy link
Member

winterqt commented Aug 3, 2022

Something that may be worth nothing is that I cannot reproduce these failures (I tested nsis) with the Clang stdenv on x86_64-linux. Absolutely no clue what could be causing this.

@toonn
Copy link
Contributor

toonn commented Aug 3, 2022

I wonder if it's not finding libc++abi because it's one of the bootstrap stage clangs?

@ylh ylh mentioned this pull request Aug 6, 2022
13 tasks
@stephank
Copy link
Contributor

stephank commented Aug 9, 2022

This is causing breakage when using an alternate clang, because for example libcxxabi v13 now links against libcxxabi v11:

otool -L /nix/store/wk6crafpdldhpn0vlvm6gz31f93f6ds6-libcxxabi-13.0.1/lib/libc++abi.1.dylib
/nix/store/wk6crafpdldhpn0vlvm6gz31f93f6ds6-libcxxabi-13.0.1/lib/libc++abi.1.dylib:
	/nix/store/wk6crafpdldhpn0vlvm6gz31f93f6ds6-libcxxabi-13.0.1/lib/libc++abi.1.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.60.2)
	/nix/store/chgdwkvgghfr6jn0392l1sc9xyx2mca5-libcxxabi-11.1.0/lib/libc++abi.1.dylib (compatibility version 1.0.0, current version 1.0.0)

This is causing ld (from cctools) to hang for a while then assert:

0  0x1030a551c  __assert_rtn + 120
1  0x102f16cf8  ld::tool::InputFiles::findDylib(char const*, ld::dylib::File const*, bool) + 1160
2  0x102ff086c  generic::dylib::File<x86>::processIndirectLibraries(ld::dylib::File::DylibHandler*, bool) + 104
3  0x102f18514  ld::tool::InputFiles::createIndirectDylibs() + 696
4  0x102f1956c  ld::tool::InputFiles::forEachInitialAtom(ld::File::AtomHandler&, ld::Internal&) + 232
5  0x102fd22e8  ld::tool::Resolver::resolve() + 164
6  0x102f221e8  main + 828
A linker snapshot was created at:
        /tmp/BugpointPasses.dylib-2022-07-04-101821.ld-snapshot
ld: Assertion failed: (counter() < 0xffff), function nextCounter, file ./ld.hpp, line 314.	

When adding the -t flag, it shows it's stuck in an infinite loop parsing /nix/store/wk6crafpdldhpn0vlvm6gz31f93f6ds6-libcxxabi-13.0.1/lib/libc++abi.1.dylib over and over again. (This is InputFiles::createIndirectDylibs, where _allDylibs keeps being extended, so the while-loop never terminates.)

I couldn't checkout and fetch b060076^ without rebuilding stdenv, so I checked against the release-22.05 branch, and confirmed it doesn't show the issue in otool -L output.

I used install_name_tool to temporarily hack both references to point to v13, and that fixed the linker invocation I was testing with. I'm not sure if we can find an easy way to exclude libcxxabi from this wrapper option, but a fixup like that is also an option, if a little ugly.

@stephank
Copy link
Contributor

stephank commented Aug 9, 2022

Here is a potential fix with the install_name_tool workaround: #185766

Will be doing some more testing with this.

@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: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 11-100 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants