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

Some libc++abi symbols are missing with LLVM12+'s stdenv on Darwin #166205

Closed
rrbutani opened this issue Mar 29, 2022 · 7 comments · Fixed by #292043
Closed

Some libc++abi symbols are missing with LLVM12+'s stdenv on Darwin #166205

rrbutani opened this issue Mar 29, 2022 · 7 comments · Fixed by #292043
Labels
0.kind: bug Something is broken 6.topic: darwin Running or building packages on Darwin 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related

Comments

@rrbutani
Copy link
Contributor

rrbutani commented Mar 29, 2022

Problem

When using stdenvs (or even just libc++ versions, really) from llvmPackages corresponding to LLVM 12 and newer on macOS, symbols related to exceptions (such as __cxa_allocate_exception) are not found by the linker.

This can be reproduced by compiling and linking any C++ program that makes use of exceptions, i.e.:

clang++ -xc++ - <<<"int main() { throw 0; return 5; }" -o /tmp/foo

Results in:

Undefined symbols for architecture arm64:
  "___cxa_allocate_exception", referenced from:
      _main in --bbc70b.o
  "___cxa_throw", referenced from:
      _main in --bbc70b.o
ld: symbol(s) not found for architecture arm64
clang-13: error: linker command failed with exit code 1 (use -v to see invocation)

Comparing the libc++ dylibs used by the wrapped clang++ in the LLVM 12 and 13 stdenvs (i.e. llvmPackages_12.libcxxStdenv) to that used in the LLVM 11 stdenv reveals that while LLVM 11's libc++.dylib reexports such symbols, newer libc++.dylibs do not.

When run through llvm-nm -C and grepped for __cxa_allocate_exception, here is the output for LLVM 11's libc++ dylib:

                 U ___cxa_allocate_exception
                 I ___cxa_allocate_exception (indirect for ___cxa_allocate_exception)

And LLVM 13's:

                 U ___cxa_allocate_exception

Both of these libc++ dylibs have a dynamic dependency on libc++abi which actually provides a definition for this symbol (this can be observed with otool -L <dylib path> which shows the libc++abi.dylib dep and then llvm-nm -C <libc++abi.dylib path> which shows that the symbol is defined in the libc++abi dylib) however only libc++ versions prior to LLVM 12's reexport the symbol.

This is consistent with the libc++ source code. These symbols were initially removed from the main list of symbols reexported and gated on libc++ exception support. However later this logic and the special list of symbols was removed.

Open Questions

I am not certain I understand the changes in the commit linked above; it's not clear to me why libcxxabi was changed to reexport these symbols.

This seems like an ABI break but there is no mention of this being potentially problematic on the CL and the comments added to the changelog in this commit seem to insist that it only adds some reexports to libc++.

Potential Solutions

I am not sure what the "right" solution is but anecdotally it seems like it's common to pass in -lc++abi as a linkopt on macOS.

cc-wrapper already does this on Linux.

To Reproduce

Here's a flake:

{
  inputs = {
    # nixpkgs.url = github:nixos/nixpkgs?ref=1a8754caf8eb8831b904daaf34b2ee737053b383; # commit for the 13.0.1 LLVM bump
    nixpkgs.url = github:nixos/nixpkgs/nixpkgs-unstable; # LLVM 13 is broken on Darwin on 21.11; using < instead of ^ so we get caching
    flake-utils.url = github:numtide/flake-utils;
  };

  # The issue only affects Darwin. Both because symbol rexport is only a thing
  # on mach-o platforms
  # (https://github.com/llvm/llvm-project/blob/4f13b999297140486b2faa1b5d8d7c768fb40dfb/libcxx/src/CMakeLists.txt#L202-L215)
  # and because `-lc++abi` is always added to the cc wrapper for Linux:
  # https://github.com/NixOS/nixpkgs/blob/a785ec661fadba2ded58467f951b51c34631c269/pkgs/build-support/cc-wrapper/default.nix#L382
  outputs = { self, nixpkgs, flake-utils }: flake-utils.lib.eachSystem [ "aarch64-darwin" "x86_64-darwin" ] (system:
    let
      normal = import nixpkgs { inherit system; };
      imp = func: import nixpkgs { inherit system; config.replaceStdenv = { pkgs }: (func pkgs).libcxxStdenv; };
      l_11 = imp (n: n.llvmPackages_11);
      l_12 = imp (n: n.llvmPackages_12);
      l_13 = imp (n: n.llvmPackages_13);
      l_13' = import nixpkgs {
        inherit system;
        # config.replaceStdenv = { pkgs }: pkgs.llvmPackages_13.libcxxStdenv.overrideAttrs (old: {
        #   postFixup = old.postFixup + ''
        #     echo "-lc++abi" >> $out/nix-support/libcxx-ldflags
        #   '';
        # });
        config.replaceStdenv = { pkgs }:
          let
            clang' = pkgs.llvmPackages_13.libcxxClang.overrideAttrs (old: {
              # We _should_ use an overlay or something similar here but they allegedly don't interact well with `replaceStdenv`.
              #
              # We're essentially adding this: https://github.com/NixOS/nixpkgs/blob/3491c5ea290bca5437845b6348919fcb23950af9/pkgs/build-support/cc-wrapper/default.nix#L382
              postFixup = old.postFixup + ''
                echo "-lc++abi" >> $out/nix-support/libcxx-ldflags
              '';
            });

            # https://github.com/NixOS/nixpkgs/blob/904ed45698338365f086c22ab5af167adf8bee9a/pkgs/development/compilers/llvm/13/default.nix#L241
            stdenv' = pkgs.overrideCC pkgs.stdenv clang';
          in
            stdenv';
      };

      cmd = ''clang++ -xc++ - <<<"int main() { throw 4; return 0; }" -v'';
      test = np: np.runCommandCC "test-cxx" {} ''
        ${cmd} -o $out
      '';
      hook = ''
        cd "$(mktemp -d)"
        libcxx_dir="$(${cmd} -o out -Wl,-v |& tee /dev/stderr | grep -P '\t/nix/store/.*-libcxx-.*/lib' | tail -1)"
        libcxx=$(echo "$libcxx_dir/libc++.dylib" | xargs)

        echo -e "\nlibcxx: $libcxx"
        echo "contains:"
        ${normal.llvmPackages.llvm}/bin/llvm-nm -C "$libcxx" | grep __cxa_allocate_exception
      '';
    in {

    # `nix flake check` shows the error
    checks.l11 = test l_11; # Works fine
    checks.l12 = test l_12; # Fails
    checks.l13 = test l_13; # Fails
    checks.l13' = test l_13'; # A workaround

    devShells = {
      llvm11 = l_11.mkShell { shellHook = hook; };
      llvm12 = l_12.mkShell { shellHook = hook; };
      llvm13 = l_13.mkShell { shellHook = hook; };
    };
  });
}

Running nix flake check shows the error when compiling with the LLVM 12 and 13 stdenvs; entering the dev shells with nix develop prints the path of the libc++ dylib that's used and greps it for __cxa_allocate_exception (one of the missing symbols).

The above also contains an example of a workaround that adds -lc++abi to the wrapper used in the LLVM 13 stdenv.

I think this is reproducible with any nixpkgs version after the LLVM 12.0.0 package was added but just in case, here's my flake.lock file:

Click to expand
{
  "nodes": {
    "flake-utils": {
      "locked": {
        "lastModified": 1648297722,
        "narHash": "sha256-W+qlPsiZd8F3XkzXOzAoR+mpFqzm3ekQkJNa+PIh1BQ=",
        "owner": "numtide",
        "repo": "flake-utils",
        "rev": "0f8662f1319ad6abf89b3380dd2722369fc51ade",
        "type": "github"
      },
      "original": {
        "owner": "numtide",
        "repo": "flake-utils",
        "type": "github"
      }
    },
    "nixpkgs": {
      "locked": {
        "lastModified": 1648219316,
        "narHash": "sha256-Ctij+dOi0ZZIfX5eMhgwugfvB+WZSrvVNAyAuANOsnQ=",
        "owner": "nixos",
        "repo": "nixpkgs",
        "rev": "30d3d79b7d3607d56546dd2a6b49e156ba0ec634",
        "type": "github"
      },
      "original": {
        "owner": "nixos",
        "ref": "nixpkgs-unstable",
        "repo": "nixpkgs",
        "type": "github"
      }
    },
    "root": {
      "inputs": {
        "flake-utils": "flake-utils",
        "nixpkgs": "nixpkgs"
      }
    }
  },
  "root": "root",
  "version": 7
}
@rrbutani rrbutani added the 0.kind: bug Something is broken label Mar 29, 2022
@veprbl veprbl added the 6.topic: darwin Running or building packages on Darwin label Mar 30, 2022
@ck3d ck3d mentioned this issue Jul 14, 2022
13 tasks
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 1, 2022
@whitevegagabriel
Copy link

Any updates on this issue?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 30, 2023
reckenrode added a commit to reckenrode/nixpkgs that referenced this issue Oct 23, 2023
Crystal requires linking libc++abi when building with newer versions of
clang. See NixOS#166205.
reckenrode added a commit to reckenrode/nixpkgs that referenced this issue Nov 7, 2023
* Set deployment target based on stdenv; and
* Work around NixOS#166205.
reckenrode added a commit to reckenrode/nixpkgs that referenced this issue Nov 9, 2023
Work around clang’s not linking libc++abi when invoked as `clang` or
`cc` even when it’s needed to link a C++ library or C++ code.

See NixOS#166205.
reckenrode added a commit to reckenrode/nixpkgs that referenced this issue Nov 11, 2023
Work around clang’s not linking libc++abi when invoked as `clang` or
`cc` even when it’s needed to link a C++ library or C++ code.

See NixOS#166205.
nyabinary pushed a commit to nyabinary/nixpkgs that referenced this issue Nov 14, 2023
Crystal requires linking libc++abi when building with newer versions of
clang. See NixOS#166205.
reckenrode added a commit to reckenrode/nixpkgs that referenced this issue Nov 17, 2023
reckenrode added a commit to reckenrode/nixpkgs that referenced this issue Nov 19, 2023
zhaofengli added a commit to zhaofengli/attic that referenced this issue Dec 19, 2023
github-actions bot pushed a commit that referenced this issue Jan 2, 2024
Workaround for #166205 similar to
51451ac

And warning fix similar to `bazel_6` fix for
```
external/upb~0.0.0-20220923-a547704/upb/mini_table.c:634:14: error: defining a type within '__builtin_offsetof' is a Clang extension [-Werror,-Wgnu-offsetof-extensions]
  UPB_ASSERT(UPB_ALIGN_OF(upb_StringView) ==
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

(cherry picked from commit 306e8ea)
@ghost
Copy link

ghost commented Jan 20, 2024

NIX_LDFLAGS = "-L${lib.getLib json_c}/lib"
# Work around https://github.com/NixOS/nixpkgs/issues/166205.
+ lib.optionalString (stdenv.cc.isClang && stdenv.cc.libcxx != null) " -l${stdenv.cc.libcxx.cxxabi.libName}";

Hey it looks like we wound up with 33 copy-and-pastes of this code:

$ rg 'https://github.com/NixOS/nixpkgs/issues/166205' | wc -l
33

This means that when there's a bug in the copy-and-pasted code (which there is -- it breaks CI if stdenv.cc.libcxx exists but lacks a cxxabi subattribute) we need 33 separate fixes.

One of Nix's superpowers is that it provides abstraction, so we can refactor instead of copying and pasting. Please use those superpowers next time -- probably by changing ld-wrapper to add -l${stdenv.cc.libcxx.cxxabi.libName} automatically. That way, if there's a bug it can be fixed in one place instead of having to make thirty-three different changes, like these:

@kirillrdy
Copy link
Member

kirillrdy commented Jan 20, 2024

NIX_LDFLAGS = "-L${lib.getLib json_c}/lib"
# Work around https://github.com/NixOS/nixpkgs/issues/166205.
+ lib.optionalString (stdenv.cc.isClang && stdenv.cc.libcxx != null) " -l${stdenv.cc.libcxx.cxxabi.libName}";

Hey it looks like we wound up with 33 copy-and-pastes of this code:

$ rg 'https://github.com/NixOS/nixpkgs/issues/166205' | wc -l
33

This means that when there's a bug in the copy-and-pasted code (which there is -- it breaks CI if stdenv.cc.libcxx exists but lacks a cxxabi subattribute) we need 33 separate fixes.

One of Nix's superpowers is that it provides abstraction, so we can refactor instead of copying and pasting. Please use those superpowers next time -- probably by changing ld-wrapper to add -l${stdenv.cc.libcxx.cxxabi.libName} automatically. That way, if there's a bug it can be fixed in one place instead of having to make thirty-three different changes, like these:

* [34c3989](https://github.com/NixOS/nixpkgs/commit/34c3989a0e8ce395a6b28397848ee07eade3509e)

* [895dbe8](https://github.com/NixOS/nixpkgs/commit/895dbe8aa68b373c2c033a4c627d4f5b13da7e24)

* [9d96656](https://github.com/NixOS/nixpkgs/commit/9d966564329d31c23301d2517dc999858a704b04)

* [caab415](https://github.com/NixOS/nixpkgs/commit/caab4159953f2bb254602cca50fc89f28703fed1)

* [1bb69e8](https://github.com/NixOS/nixpkgs/commit/1bb69e8dfd520cf5b513699b5043dccc35ed09f4)

* [8670f50](https://github.com/NixOS/nixpkgs/commit/8670f502d0d19b68c617dd42539df1b9605e2486)

* [ad34020](https://github.com/NixOS/nixpkgs/commit/ad340203577eb36090b1afce7ec9c4fc15dff41d)

* [eca13b3](https://github.com/NixOS/nixpkgs/commit/eca13b37fd12f76a9f261427415fbc222423f4f5)

* [0917ca9](https://github.com/NixOS/nixpkgs/commit/0917ca9988103ef0abef67c1715360b1c368d4dd)

* [587ee6e](https://github.com/NixOS/nixpkgs/commit/587ee6e31b0a0c14d8da07d516f0bb9cfe792ddf)

* ... and on, and on ...

@amjoseph-nixpkgs thats a very good find ! we should fix it, is it darwin specific ?

Strum355 added a commit to sourcegraph/sourcegraph-public-snapshot that referenced this issue Jan 24, 2024
…59825)

Host clang for bazel reason:
```
ERROR: /Users/noah/Sourcegraph/sourcegraph/BUILD.bazel:305:5: GoLink sg_nogo_actual_/sg_nogo_actual [for tool] failed: (Exit 1): builder failed: error executing GoLink command (from target //:sg_nogo_actual) bazel-out/darwin_arm64-opt-exec-ST-1a88b5d644a4/bin/external/go_sdk/builder_reset/builder '-param=bazel-out/darwin_arm64-opt-exec-ST-1a88b5d644a4/bin/sg_nogo_actual_/sg_nogo_actual-0.params' -- -extld ... (remaining 6 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
external/go_sdk/pkg/tool/darwin_arm64/link: running external/local_config_cc/cc_wrapper.sh failed: exit status 1
ld: framework not found CoreFoundation
clang: error: linker command failed with exit code 1 (use -v to see invocation)
```

clang 11 for shell reason: NixOS/nixpkgs#166205


## Test plan

`./dev/scip-ctags-install.sh` and `bazel build //:gazelle`
@j-baker
Copy link
Contributor

j-baker commented Feb 20, 2024

Hi! This hits internal users where I work about once a week, as they add a native library and have to add this override. By and large, they are unfamiliar with the peculiarities of the linker, and the errors we get are strange as they usually reference other libraries, which is a red herring.

Is there some path to a default fix for this problem? From the above, there are a lot of workarounds being exported (which we apply internally), but there's no direct proposal for an override fix (adding the same override to ld-wrapper?).

Is it reasonable to (internally) automatically apply something which adds this linker flag to everything?

@ghost
Copy link

ghost commented Feb 28, 2024

Is there some path to a default fix for this problem? From the above, there are a lot of workarounds being exported (which we apply internally), but there's no direct proposal for an override fix (adding the same override to ld-wrapper?).

the change still needs to go through the review processes.

ghost pushed a commit that referenced this issue Mar 11, 2024
…abi into libcxx (#292043)

- merge libcxxabi into libcxx for LLVM 12, 13, 14, 15, 16, 17, and git.
- remove the link time workaround `-lc++ -lc++abi` from 58 packages as it is no longer required.
- fixes #166205
- provides alternative fixes for. #269548 NixOS/nix#9640
- pkgsCross.x86_64-freebsd builds work again

This change can be represented in 3 stages
1. merge libcxxabi into libcxx -- files: pkgs/development/compilers/llvm/[12, git]/{libcxx, libcxxabi}
2. update stdenv to account for merge -- files: stdenv.{adapters, cc.wrapper, darwin}
3. remove all references to libcxxabi outside of llvm (about 58 packages modified)

### merging libcxxabi into libcxx
- take the union of the libcxxabi and libcxx cmake flags
- eliminate the libcxx-headers-only package - it was only needed to break libcxx <-> libcxxabi circular dependency
- libcxx.cxxabi is removed. external cxxabi (freebsd) will symlink headers / libs into libcxx.
- darwin will re-export the libcxxabi symbols into libcxx so linking `-lc++` is sufficient.
- linux/freebsd `libc++.so` is a linker script `LINK(libc++.so.1, -lc++abi)` making `-lc++` sufficient.
- libcxx/default.nix [12, 17] are identical except for patches and `LIBCXX_ADDITIONAL_LIBRARIES` (only used in 16+)
- git/libcxx/defaul.nix  does not link with -nostdlib when useLLVM is true so flag is removed. this is not much different than before as libcxxabi used -nostdlib where libcxx did not, so libc was linked in anyway.

### stdenv changes
- darwin bootstrap, remove references to libcxxabi and cxxabi
- cc-wrapper: remove c++ link workaround when libcxx.cxxabi doesn't exist (still exists for LLVM pre 12)
- adapter: update overrideLibcxx to account for a pkgs.stdenv that only has libcxx

### 58 package updates
- remove `NIX_LDFLAGS = "-l${stdenv.cc.libcxx.cxxabi.libName}` as no longer needed
- swift, nodejs_v8 remove libcxxabi references in the clang override

#292043
@ghost ghost closed this as completed Mar 17, 2024
mweinelt pushed a commit that referenced this issue May 14, 2024
Builds were broken on Darwin with
#241692, so this applies the
workaround from #166205

(cherry picked from commit 904913e)
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug Something is broken 6.topic: darwin Running or building packages on Darwin 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants