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

llvmPackages_{12,13,14,15,16,17,18,git}.libcxx: merge static libc++ab… #305876

Closed

Conversation

ghost
Copy link

@ghost ghost commented Apr 22, 2024

…i.a into libc++.a

https://discourse.nixos.org/t/tracking-down-what-broke-boehm-gc-static-aarch64-apple-darwin/42234/1

merge libc++abi.a, if it exists, into libc++.a so static builds behave similar to shared object builds and only require using -lc++

add a test in tests.cc-wrapper to test that can link (and run) hello world linking via $CC hello.o -lc++ and $CC hello.o -l:libc++.a

https://reviews.llvm.org/D96070#2552492

testing:

nix-build -A pkgsStatic.pkgsLLVM.boehmgc
nix-build -A tests.cc-wrapper

works on linux.

Description of changes

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:
  • 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.

@ghost
Copy link
Author

ghost commented Apr 22, 2024

just simplified the ar command. validated that libc++.a hash is identical to the previous one.

@ofborg ofborg bot requested a review from RossComputerGuy April 22, 2024 08:54
Copy link
Contributor

@Mindavi Mindavi left a comment

Choose a reason for hiding this comment

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

This seems fine to me.

@Ericson2314
Copy link
Member

It would be nice to have a stdenv / cc-wrapper regression this for static linking.

Copy link
Contributor

@rrbutani rrbutani left a comment

Choose a reason for hiding this comment

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

@ghostofannalee Thank you for your work on this.

Apologies if you've already addressed this elsewhere: do you know if LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY (available in LLVM 7+) is sufficient for this?

I haven't tested ^ yet but it seems to do what we want.

@ghost ghost force-pushed the merge-static-libcxx-libcxxabi branch from da6f11e to 86687b0 Compare April 22, 2024 14:34
@ghost
Copy link
Author

ghost commented Apr 22, 2024

updated to use LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=ON. verified that it adds libc++abi.a -- it adds to the start of libc++.a where i was appending. works fine for freebsd too (eg: doesn't break the build) pkgsCross.x86_64-freebsd.libcxx (with unrelated fix) and pkgsCross.x86_64-freebsd.llvmPackages_16.libcxx even though there is no libcxxrt.a.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tracking-down-what-broke-boehm-gc-static-aarch64-apple-darwin/42234/4

@RossComputerGuy
Copy link
Contributor

Looks like there's eval errors, idk what the errors are since I'm on mobile but they need to be fixed.

@ghost
Copy link
Author

ghost commented Apr 22, 2024

Looks like there's eval errors, idk what the errors are since I'm on mobile but they need to be fixed.

eval errors due to a bad merge in staging but had to wait till fix merged #306036.

@ofborg eval

@ghost
Copy link
Author

ghost commented Apr 23, 2024

updated to use LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=ON.

blurg. LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY doesn't seem to work on LLVM < 16 so going to revert to the MRI script which works for LLVM 12+. added some tests but going to mark as draft until i have tests and update ready. missed this staging cycle anyway.

@ghost ghost marked this pull request as draft April 23, 2024 06:58
annalee added 2 commits April 23, 2024 08:07
…o libc++.a

merge libc++abi.a, if it exists, into libc++.a so static builds behave
similar to shared object builds and only require using -lc++
test that shared and static libc++ can be linked without needing to
specify libc++abi
@ghost ghost force-pushed the merge-static-libcxx-libcxxabi branch from 86687b0 to 188c149 Compare April 23, 2024 08:11
@ghost
Copy link
Author

ghost commented Apr 23, 2024

ok. moved back to the ar MRI script as that is supported from LLVM12+, made freebsd work if libcxxrt.a exists (currently it doesn't exist but tested with #306238). when #306027 merges will need to resolve the merge conflict.

unfortunately, the MRI script is kind of gross but can't just do $AR -qL libc++.a libc++abi.a because ar doesn't support the -L flag in older LLVMs (at least 12) (-L allows merging archives).

aarch64-linux tests.cc-wrapper fails due to bug #285016

@ghost ghost marked this pull request as ready for review April 23, 2024 08:17
@ofborg ofborg bot requested a review from rrbutani April 23, 2024 08:38
@ghost ghost mentioned this pull request Apr 23, 2024
13 tasks
@Ericson2314
Copy link
Member

I am wondering whether we should still use LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY where possible, and also get upstream to fix it.

@rrbutani
Copy link
Contributor

@Ericson2314 by "fix it" do you mean backporting libcxx build changes to LLVM 12 through 15 to get that build option to work as intended or are you referring to that option's behavior when used with other cxxabi sources (i.e. libcxxrt)?

@ghost
Copy link
Author

ghost commented Apr 23, 2024

this is more than i have time to deal with.

@ghost ghost closed this Apr 23, 2024
@ghost ghost deleted the merge-static-libcxx-libcxxabi branch April 23, 2024 15:32
@Ericson2314
Copy link
Member

Ericson2314 commented Apr 23, 2024

I certainly didn't mean for upstream changes to be a blocker for this PR. I meant

  • Should we use LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY when version >= 16 and cxxabi == null? A question, not a rhetorical one.

  • Should we let them know that LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY doesn't work with a different c++ ABI library?

This pull request was closed.
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.

None yet

6 participants