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

[WIP] llvmPackages_{[12, 17] + git}.libcxx: re-export libcxxabi new, delete, exceptions #282624

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 21, 2024

Fixes #166205

if patching to master need to apply #278945 first.

This change installs the list of new / delete / exception symbols from libcxxabi into $dev/share/libcxxabi/ and then imports them into the libcxx build where libcxx will re-export them.

on darwin this should fix having to link with libcxxabi when libcxx is already specified.

Note: if libcxxabi and libcxx were built together and not as separate components this shouldn't be necessary.

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.

Comment on lines +120 to +121
"-DLIBCXXABI_REEXPORT_NEW_DELETE=${lib.getDev cxxabi}/share/${cxxabi.pname}/new-delete.exp"
"-DLIBCXXABI_REEXPORT_EXCEPTIONS=${lib.getDev cxxabi}/share/${cxxabi.pname}/exceptions.exp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does only Darwin need this, or is it also needed by other platforms using libc++ (such as pkgsLLVM or FreeBSD)?

Copy link
Author

@ghost ghost Jan 21, 2024

Choose a reason for hiding this comment

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

just darwin. APPLE must be defined for it to be used. the patch uses it in https://github.com/llvm/llvm-project/blob/bc82cfb38d83f1afeb2c290aa472c2e2e88919cb/libcxx/src/CMakeLists.txt#L233-L247 and the patched code looks like.

  # Maybe re-export symbols from libc++abi
  # In particular, we don't re-export the symbols if libc++abi is merged statically
  # into libc++ because in that case there's no dylib to re-export from.
  if (APPLE AND LIBCXX_CXX_ABI MATCHES "libcxxabi$"
            AND NOT DEFINED LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS
            AND NOT LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
    set(LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS ON)
  endif()

  if (LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS)
    target_link_libraries(cxx_shared PRIVATE
      "-Wl,-unexported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/../lib/libc++unexp.exp"
      "-Wl,-reexported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/../lib/libc++abi.exp"
      "-Wl,-force_symbols_not_weak_list,${CMAKE_CURRENT_SOURCE_DIR}/../lib/notweak.exp"
      "-Wl,-force_symbols_weak_list,${CMAKE_CURRENT_SOURCE_DIR}/../lib/weak.exp")

+    if (NOT LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS
+        AND DEFINED LIBCXXABI_REEXPORT_NEW_DELETE)
+      target_link_libraries(cxx_shared PRIVATE
+              "-Wl,-reexported_symbols_list,${LIBCXXABI_REEXPORT_NEW_DELETE}")
+    endif()
+    if (DEFINED LIBCXXABI_REEXPORT_EXCEPTIONS)
+      target_link_libraries(cxx_shared PRIVATE
+              "-Wl,-reexported_symbols_list,${LIBCXXABI_REEXPORT_EXCEPTIONS}")
+    endif()

    target_link_libraries(cxx_shared PRIVATE $<TARGET_NAME_IF_EXISTS:cxxabi-reexports>)
  endif()

Copy link
Author

Choose a reason for hiding this comment

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

Does only Darwin need this, or is it also needed by other platforms using libc++ (such as pkgsLLVM or FreeBSD)?

I haven't tried pkgsLLVM or FreeBSD but we should provide whatever experience the provided system tools do. i've read that they still require libc++abi but random internet sources are not reliable. however, if they are true just fixing this for darwin seems sufficient as that will provide the same development / build experience using nixpkgs tools or the system tools.

Copy link
Contributor

@reckenrode reckenrode Jan 22, 2024

Choose a reason for hiding this comment

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

My understanding is FreeBSD doesn’t use libc++abi for its ABI library. I don’t know what FreeBSD in nixpkgs does, but I assume it intends to use the same one (libcxxrt).

https://wiki.freebsd.org/NewC%2B%2BStack

Copy link
Contributor

Choose a reason for hiding this comment

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

As for Linux (pkgsLLVM), I assume it uses libc++abi. I’ll kick off a build on a Linux system with my branch (featuring this PR with the workarounds for #166205 removed) to see whether it needs any of its own patches.

@ghost ghost changed the title [WIP] llvmPackages_{15,16}.libcxxi: re-export libcxxabi new, delete, exceptions [WIP] llvmPackages_{15,16}.libcxx: re-export libcxxabi new, delete, exceptions Jan 21, 2024
@ghost ghost linked an issue Jan 22, 2024 that may be closed by this pull request
@reckenrode
Copy link
Contributor

reckenrode commented Jan 22, 2024

As discussed on Matrix, I applied this PR to staging and reverted all workarounds for #166205. After fixing a few unrelated issues (see #282764 and #282777), I was able to build the following flake successfully on Darwin (see below gist).

@ghost
Copy link
Author

ghost commented Jan 23, 2024

I am likely going to implement this change differently -- eg: build libcxx and libcxxabi together -- but since the changes to llvm 12-17 + git are tested and verified to work (simple smoke test of compiling + linking a hello-world.cxx for all except for llvm 16 which was tested more thoroughly by @reckenrode) i figure they can be posted.

@ghost ghost changed the title [WIP] llvmPackages_{15,16}.libcxx: re-export libcxxabi new, delete, exceptions [WIP] llvmPackages_{[11-17] + git}.libcxx: re-export libcxxabi new, delete, exceptions Jan 23, 2024
@ghost ghost changed the title [WIP] llvmPackages_{[11-17] + git}.libcxx: re-export libcxxabi new, delete, exceptions [WIP] llvmPackages_{[11, 17] + git}.libcxx: re-export libcxxabi new, delete, exceptions Jan 23, 2024
@ghost ghost changed the title [WIP] llvmPackages_{[11, 17] + git}.libcxx: re-export libcxxabi new, delete, exceptions [WIP] llvmPackages_{[12, 17] + git}.libcxx: re-export libcxxabi new, delete, exceptions Jan 23, 2024
@ghost ghost closed this Mar 10, 2024
@ghost ghost deleted the reexport-libcxxabi branch March 10, 2024 01:45
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
This pull request was closed.
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: 101-500
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants