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

libcxxabi: remove link with build libcxxabi #185766

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

stephank
Copy link
Contributor

@stephank stephank commented Aug 9, 2022

Description of changes

This is a fix for the issue seen in #181485 (comment).

I updated the snippet across all LLVM versions, but that probably implies a Darwin stdenv rebuild (via v11). I've not yet tested this, just the library build itself on v13 (and verified otool -L output).

The previous snippet did a for-loop over lib/*.dylib, but all but one of those are symlinks. This also meant the final install name was whatever it processed last. It now preserves the exact basename.

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.

@stephank
Copy link
Contributor Author

stephank commented Aug 9, 2022

Removing the loop was apparently a mistake, because sometimes it's more than one dylib? Looks like just another copy of the same. Any way, I restored the loop.

I did a successful stdenv rebuild on aarch64-darwin and confirmed my problem went away in the build I was trying.

@toonn
Copy link
Contributor

toonn commented Aug 26, 2022

I'm trying to test this but usingllvmPackages_13.stdenv to build hello doesn't surface the original error. I assume I should build a C++ package to experience the problem, any nice and small ones that are nice for testing? I tried nix but I'm getting a ca hash mismatch:

error: ca hash mismatch importing path '/nix/store/rsqipm4dcmxwg0045fms97qkn1z20l87-source';
         specified: sha256:0px2dfq1wky75hgif998cqpcz5yzjqv8gp0k47w4632l8mjjpahc
         got:       sha256:1f0hx96zaknhh7fwanycz2012j2v6jj8ifhsgn7axq96vv1zgd2g

@stephank
Copy link
Contributor Author

Rebased to fix conflicts.

I've only seen this cause ld to hang/crash in my Swift build. I've been looking for a repro outside of it, but no luck. Tried some builds for shared libraries, applications, and applications depending on C++ shared libraries, but they all succeed. Not sure what is special about Swift in this case.

@stephank
Copy link
Contributor Author

stephank commented Oct 2, 2022

Ping! What can we do to move this forward?

@toonn
Copy link
Contributor

toonn commented Oct 3, 2022

I was intending to review this as part of the work on Swift, which IIUC is ready for review? I don't think it's blocking anything else? Will try to review both PRs this week. Others should not be held back from reviewing by this, of course.

@stephank
Copy link
Contributor Author

stephank commented Oct 3, 2022

Yes, the Swift PR is ready. I don't have anything to add to it. 👍

And yes, afaik this PR doesn't block anything else. Should be fine to merge it together with the Swift PR or by itself, whichever we prefer.

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.

Does what it says on the tin.

Before:

> otool -L /nix/store/srj4s0xsrz6fdhc4zip4ah5b8lhqvdhr-libcxxabi-13.0.1/lib/libc++abi.1.dylib
/nix/store/srj4s0xsrz6fdhc4zip4ah5b8lhqvdhr-libcxxabi-13.0.1/lib/libc++abi.1.dylib:
        /nix/store/srj4s0xsrz6fdhc4zip4ah5b8lhqvdhr-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/36grdzp9lha3isiwiagnj1vyr4y10qvd-libcxxabi-11.1.0/lib/libc++abi.1.dylib (compatibility version 1.0.0, current version 1.0.0)

After:

> otool -L /nix/store/l34pp467vl2xnccqrllnxvigvsmdgjqk-libcxxabi-13.0.1/lib/libc++abi.1.dylib
/nix/store/l34pp467vl2xnccqrllnxvigvsmdgjqk-libcxxabi-13.0.1/lib/libc++abi.1.dylib:
       /nix/store/l34pp467vl2xnccqrllnxvigvsmdgjqk-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/l34pp467vl2xnccqrllnxvigvsmdgjqk-libcxxabi-13.0.1/lib/libc++abi.1.dylib (compatibility version 1.0.0, current version 1.0.0)

I assume the repetition of the dylib is harmless?

@stephank
Copy link
Contributor Author

stephank commented Oct 5, 2022

It appears to be harmless in all my testing. I looked for a way to remove the entry completely, but there doesn't appear to be a tool for that?

@toonn toonn merged commit 6a132cf into NixOS:staging Oct 5, 2022
@stephank stephank deleted the fix/libcxxabi branch October 5, 2022 16:01
@rrbutani rrbutani mentioned this pull request Oct 10, 2022
92 tasks
@joshchngs
Copy link
Contributor

Looks like something similar needs to be done for libcxx as well.

$ otool -L /nix/store/hr1i08bb3qjsqcklgap1s17d199587rj-libcxx-14.0.6/lib/libc++.1.0.dylib
/nix/store/hr1i08bb3qjsqcklgap1s17d199587rj-libcxx-14.0.6/lib/libc++.1.0.dylib:
        /nix/store/hr1i08bb3qjsqcklgap1s17d199587rj-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/store/5jl0l6bk1zad13mr5vw30675f2zwyvaz-libcxxabi-11.1.0/lib/libc++abi.1.dylib (compatibility version 1.0.0, current version 1.0.0)

libc++ should link against its own version of libc++abi, but current links the version from the previous stage stdenv which built libc++.
Outputs built using the llvmPackages_*.libcxxStdenv links correctly to the selected version of both, but also transitively link to the wrong version.

joshchngs added a commit to music-tribe/nixpkgs that referenced this pull request Nov 15, 2022
Same adjustment as made for libc++abi in NixOS#185766, for the same reason:
the unamended dylib links to the libc++abi in the build stdenv, which
is the wrong version.

Tested on Darwin with LLVM 14 stdenv, but the phase is added to all
versions, including 11 - so this will cause a mass rebuild.

See: NixOS#185766
joshchngs added a commit to music-tribe/nixpkgs that referenced this pull request Nov 22, 2022
Same adjustment as made for libc++abi in NixOS#185766, for the same reason:
the unamended dylib links to the libc++abi in the build stdenv, which
is the wrong version.

Tested on Darwin with LLVM 14 stdenv, but the phase is added to all
versions, including 11 - so this will cause a mass rebuild.

See: NixOS#185766
github-actions bot pushed a commit that referenced this pull request Nov 28, 2022
Same adjustment as made for libc++abi in #185766, for the same reason:
the unamended dylib links to the libc++abi in the build stdenv, which
is the wrong version.

Tested on Darwin with LLVM 14 stdenv, but the phase is added to all
versions, including 11 - so this will cause a mass rebuild.

See: #185766
(cherry picked from commit 67f11a2)
@Artturin Artturin mentioned this pull request Nov 25, 2023
13 tasks
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants