[libc++] Fix broken configuration system-libcxxabi on Apple #110920
Conversation
On Apple platforms, using system-libcxxabi as an ABI library wouldn't work because we'd try to re-export symbols from libc++abi that the system libc++abi.dylib might not have. Instead, only re-export those symbols when we're using the in-tree libc++abi. This does mean that libc++.dylib won't re-export any libc++abi symbols when building against the system libc++abi, which could be fixed in various ways. However, the best solution really depends on the intended use case, so this patch doesn't try to solve that problem. As a drive-by, also improve the diagnostic message when the user forgets to set the LIBCXX_CXX_ABI_INCLUDE_PATHS variable, which would previously lead to a confusing error. Closes llvm#104672
llvmbot
added
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesOn Apple platforms, using system-libcxxabi as an ABI library wouldn't work because we'd try to re-export symbols from libc++abi that the system libc++abi.dylib might not have. Instead, only re-export those symbols when we're using the in-tree libc++abi. This does mean that libc++.dylib won't re-export any libc++abi symbols when building against the system libc++abi, which could be fixed in various ways. However, the best solution really depends on the intended use case, so this patch doesn't try to solve that problem. As a drive-by, also improve the diagnostic message when the user forgets to set the LIBCXX_CXX_ABI_INCLUDE_PATHS variable, which would previously lead to a confusing error. Closes #104672 Full diff: https://github.com/llvm/llvm-project/pull/110920.diff 3 Files Affected:
diff --git a/libcxx/cmake/Modules/HandleLibCXXABI.cmake b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
index 34e9a672a960f9..e7caf3044b8bcf 100644
--- a/libcxx/cmake/Modules/HandleLibCXXABI.cmake
+++ b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
@@ -83,6 +83,10 @@ endfunction()
# Link against a system-provided libstdc++
if ("${LIBCXX_CXX_ABI}" STREQUAL "libstdc++")
+ if(NOT LIBCXX_CXX_ABI_INCLUDE_PATHS)
+ message(FATAL_ERROR "LIBCXX_CXX_ABI_INCLUDE_PATHS must be set when selecting libstdc++ as an ABI library")
+ endif()
+
add_library(libcxx-abi-headers INTERFACE)
import_private_headers(libcxx-abi-headers "${LIBCXX_CXX_ABI_INCLUDE_PATHS}"
"cxxabi.h;bits/c++config.h;bits/os_defines.h;bits/cpu_defines.h;bits/cxxabi_tweaks.h;bits/cxxabi_forced.h")
@@ -96,6 +100,10 @@ if ("${LIBCXX_CXX_ABI}" STREQUAL "libstdc++")
# Link against a system-provided libsupc++
elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libsupc++")
+ if(NOT LIBCXX_CXX_ABI_INCLUDE_PATHS)
+ message(FATAL_ERROR "LIBCXX_CXX_ABI_INCLUDE_PATHS must be set when selecting libsupc++ as an ABI library")
+ endif()
+
add_library(libcxx-abi-headers INTERFACE)
import_private_headers(libcxx-abi-headers "${LIBCXX_CXX_ABI_INCLUDE_PATHS}"
"cxxabi.h;bits/c++config.h;bits/os_defines.h;bits/cpu_defines.h;bits/cxxabi_tweaks.h;bits/cxxabi_forced.h")
@@ -114,7 +122,13 @@ elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
target_compile_definitions(libcxx-abi-headers INTERFACE "-DLIBCXX_BUILDING_LIBCXXABI")
if (TARGET cxxabi_shared)
- add_library(libcxx-abi-shared ALIAS cxxabi_shared)
+ add_library(libcxx-abi-shared INTERFACE)
+ target_link_libraries(libcxx-abi-shared INTERFACE cxxabi_shared)
+
+ # When using the in-tree libc++abi as an ABI library, libc++ re-exports the
+ # libc++abi symbols (on platforms where it can) because libc++abi is only an
+ # implementation detail of libc++.
+ target_link_libraries(libcxx-abi-shared INTERFACE cxxabi-reexports)
endif()
if (TARGET cxxabi_static)
@@ -131,6 +145,10 @@ elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
# Link against a system-provided libc++abi
elseif ("${LIBCXX_CXX_ABI}" STREQUAL "system-libcxxabi")
+ if(NOT LIBCXX_CXX_ABI_INCLUDE_PATHS)
+ message(FATAL_ERROR "LIBCXX_CXX_ABI_INCLUDE_PATHS must be set when selecting system-libcxxabi as an ABI library")
+ endif()
+
add_library(libcxx-abi-headers INTERFACE)
import_private_headers(libcxx-abi-headers "${LIBCXX_CXX_ABI_INCLUDE_PATHS}" "cxxabi.h;__cxxabi_config.h")
target_compile_definitions(libcxx-abi-headers INTERFACE "-DLIBCXX_BUILDING_LIBCXXABI")
diff --git a/libcxx/src/CMakeLists.txt b/libcxx/src/CMakeLists.txt
index 48c5111a0acbf6..6b928ebeeeb221 100644
--- a/libcxx/src/CMakeLists.txt
+++ b/libcxx/src/CMakeLists.txt
@@ -210,14 +210,10 @@ if (LIBCXX_ENABLE_SHARED)
target_link_libraries(cxx_shared PUBLIC libcxx-abi-shared)
endif()
- # 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.
+ # Maybe force some symbols to be weak, not weak or not exported.
+ # TODO: This shouldn't depend on the platform, and ideally it should be done in the sources.
if (APPLE AND LIBCXX_CXX_ABI MATCHES "libcxxabi$"
AND NOT LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
- target_link_libraries(cxx_shared PRIVATE cxxabi-reexports)
-
- # TODO: These exports controls should not be tied to whether we re-export libc++abi symbols
target_link_libraries(cxx_shared PRIVATE
"-Wl,-unexported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/../lib/libc++unexp.exp"
"-Wl,-force_symbols_not_weak_list,${CMAKE_CURRENT_SOURCE_DIR}/../lib/notweak.exp"
diff --git a/libcxxabi/src/CMakeLists.txt b/libcxxabi/src/CMakeLists.txt
index 6f16c614212ef1..480e528b819bb9 100644
--- a/libcxxabi/src/CMakeLists.txt
+++ b/libcxxabi/src/CMakeLists.txt
@@ -213,6 +213,8 @@ if (LIBCXXABI_ENABLE_SHARED)
list(APPEND LIBCXXABI_INSTALL_TARGETS "cxxabi_shared")
endif()
+ # TODO: Move this to libc++'s HandleLibCXXABI.cmake since this is effectively trying to control
+ # what libc++ re-exports.
add_library(cxxabi-reexports INTERFACE)
function(export_symbols file)
# -exported_symbols_list is only available on Apple platforms
|
|
@llvm/pr-subscribers-libcxxabi Author: Louis Dionne (ldionne) ChangesOn Apple platforms, using system-libcxxabi as an ABI library wouldn't work because we'd try to re-export symbols from libc++abi that the system libc++abi.dylib might not have. Instead, only re-export those symbols when we're using the in-tree libc++abi. This does mean that libc++.dylib won't re-export any libc++abi symbols when building against the system libc++abi, which could be fixed in various ways. However, the best solution really depends on the intended use case, so this patch doesn't try to solve that problem. As a drive-by, also improve the diagnostic message when the user forgets to set the LIBCXX_CXX_ABI_INCLUDE_PATHS variable, which would previously lead to a confusing error. Closes #104672 Full diff: https://github.com/llvm/llvm-project/pull/110920.diff 3 Files Affected:
diff --git a/libcxx/cmake/Modules/HandleLibCXXABI.cmake b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
index 34e9a672a960f9..e7caf3044b8bcf 100644
--- a/libcxx/cmake/Modules/HandleLibCXXABI.cmake
+++ b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
@@ -83,6 +83,10 @@ endfunction()
# Link against a system-provided libstdc++
if ("${LIBCXX_CXX_ABI}" STREQUAL "libstdc++")
+ if(NOT LIBCXX_CXX_ABI_INCLUDE_PATHS)
+ message(FATAL_ERROR "LIBCXX_CXX_ABI_INCLUDE_PATHS must be set when selecting libstdc++ as an ABI library")
+ endif()
+
add_library(libcxx-abi-headers INTERFACE)
import_private_headers(libcxx-abi-headers "${LIBCXX_CXX_ABI_INCLUDE_PATHS}"
"cxxabi.h;bits/c++config.h;bits/os_defines.h;bits/cpu_defines.h;bits/cxxabi_tweaks.h;bits/cxxabi_forced.h")
@@ -96,6 +100,10 @@ if ("${LIBCXX_CXX_ABI}" STREQUAL "libstdc++")
# Link against a system-provided libsupc++
elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libsupc++")
+ if(NOT LIBCXX_CXX_ABI_INCLUDE_PATHS)
+ message(FATAL_ERROR "LIBCXX_CXX_ABI_INCLUDE_PATHS must be set when selecting libsupc++ as an ABI library")
+ endif()
+
add_library(libcxx-abi-headers INTERFACE)
import_private_headers(libcxx-abi-headers "${LIBCXX_CXX_ABI_INCLUDE_PATHS}"
"cxxabi.h;bits/c++config.h;bits/os_defines.h;bits/cpu_defines.h;bits/cxxabi_tweaks.h;bits/cxxabi_forced.h")
@@ -114,7 +122,13 @@ elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
target_compile_definitions(libcxx-abi-headers INTERFACE "-DLIBCXX_BUILDING_LIBCXXABI")
if (TARGET cxxabi_shared)
- add_library(libcxx-abi-shared ALIAS cxxabi_shared)
+ add_library(libcxx-abi-shared INTERFACE)
+ target_link_libraries(libcxx-abi-shared INTERFACE cxxabi_shared)
+
+ # When using the in-tree libc++abi as an ABI library, libc++ re-exports the
+ # libc++abi symbols (on platforms where it can) because libc++abi is only an
+ # implementation detail of libc++.
+ target_link_libraries(libcxx-abi-shared INTERFACE cxxabi-reexports)
endif()
if (TARGET cxxabi_static)
@@ -131,6 +145,10 @@ elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
# Link against a system-provided libc++abi
elseif ("${LIBCXX_CXX_ABI}" STREQUAL "system-libcxxabi")
+ if(NOT LIBCXX_CXX_ABI_INCLUDE_PATHS)
+ message(FATAL_ERROR "LIBCXX_CXX_ABI_INCLUDE_PATHS must be set when selecting system-libcxxabi as an ABI library")
+ endif()
+
add_library(libcxx-abi-headers INTERFACE)
import_private_headers(libcxx-abi-headers "${LIBCXX_CXX_ABI_INCLUDE_PATHS}" "cxxabi.h;__cxxabi_config.h")
target_compile_definitions(libcxx-abi-headers INTERFACE "-DLIBCXX_BUILDING_LIBCXXABI")
diff --git a/libcxx/src/CMakeLists.txt b/libcxx/src/CMakeLists.txt
index 48c5111a0acbf6..6b928ebeeeb221 100644
--- a/libcxx/src/CMakeLists.txt
+++ b/libcxx/src/CMakeLists.txt
@@ -210,14 +210,10 @@ if (LIBCXX_ENABLE_SHARED)
target_link_libraries(cxx_shared PUBLIC libcxx-abi-shared)
endif()
- # 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.
+ # Maybe force some symbols to be weak, not weak or not exported.
+ # TODO: This shouldn't depend on the platform, and ideally it should be done in the sources.
if (APPLE AND LIBCXX_CXX_ABI MATCHES "libcxxabi$"
AND NOT LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
- target_link_libraries(cxx_shared PRIVATE cxxabi-reexports)
-
- # TODO: These exports controls should not be tied to whether we re-export libc++abi symbols
target_link_libraries(cxx_shared PRIVATE
"-Wl,-unexported_symbols_list,${CMAKE_CURRENT_SOURCE_DIR}/../lib/libc++unexp.exp"
"-Wl,-force_symbols_not_weak_list,${CMAKE_CURRENT_SOURCE_DIR}/../lib/notweak.exp"
diff --git a/libcxxabi/src/CMakeLists.txt b/libcxxabi/src/CMakeLists.txt
index 6f16c614212ef1..480e528b819bb9 100644
--- a/libcxxabi/src/CMakeLists.txt
+++ b/libcxxabi/src/CMakeLists.txt
@@ -213,6 +213,8 @@ if (LIBCXXABI_ENABLE_SHARED)
list(APPEND LIBCXXABI_INSTALL_TARGETS "cxxabi_shared")
endif()
+ # TODO: Move this to libc++'s HandleLibCXXABI.cmake since this is effectively trying to control
+ # what libc++ re-exports.
add_library(cxxabi-reexports INTERFACE)
function(export_symbols file)
# -exported_symbols_list is only available on Apple platforms
|
|
@h-vetinari It would be great if you can let me know whether this solves your problem |
Thanks a lot for this, I'll test this and get back to you! 🙏 |
|
So I'm trying this in conda-forge/libcxx-feedstock#177, and I'm getting It's plausible/possible that this is a behavioural difference between our current workaround to depend on the system libcxxabi and doing it "properly". In particular, the ninja -C build cxx cxxabi unwind
ninja -C build install-cxx install-cxxabi install-unwind
$INSTALL_NAME_TOOL -change "@rpath/libc++abi.1.dylib" "/usr/lib/libc++abi.dylib" $PREFIX/lib/libc++.1.0.dylibI'm aware that this is fragile (and we do carry a patch for systems before macOS 12 due to ABI differences), but it's what we had to move to, because the system libcxxabi gets picked up in some situations (e.g. from CPython, see README here). IOW, it's conceivable to me that (re)building everything in a CC @isuruf Footnotes
|
|
OK, rereading your comment from #104672
now makes more sense. The thing I don't yet get is how things are working for us in the current setup; presumably the fact that the libcxxabi symbols don't get re-exported through our workaround helps. |
How was Another thing you could try on the side is to re-export |
|
I'm going to ship this since it does improve some of the story for this configuration, even though the question of how to properly re-export symbols from the system libc++abi isn't resolved yet. Shipping this will unblock my work on another patch which cleans up the ABI selection mechanism a lot more. |
|
Sorry for the delayed response! I was planning to get to this but not fast enough apparently 😅
Not sure I understand 100%, but in our case the system ABI has a clear lower bound and we know it at build-time (currently 10.13 on osx-64, 11.0 on osx-arm64).
This might be naïve, but can't we just start by relying on the availability of symbols from the lower-bound system ABI? This is effectively what we're doing already. If we're building on 10.13, we require (through our package metadata that drives whether an artefact can be installed) that the resulting package can only be installed on macOS>=10.13. That has worked for a long time already, and didn't need a full mapping.
I had pasted the essence of the build script above; here's the link to the full thing again.
That sounds very plausible, but it needs to be happening essentially by default, because I'm not actively choosing to re-export the libcxxabi symbols (I would have wanted to avoid that, had I known). |
|
Do you think it would be possible to backport this fix into 19.x? |
|
@mgorny I don't think there are any releases planned for LLVM 19 anymore |
|
Well, my point was for 19.1.4, but I guess that ship just sailed. |
) On Apple platforms, using system-libcxxabi as an ABI library wouldn't work because we'd try to re-export symbols from libc++abi that the system libc++abi.dylib might not have. Instead, only re-export those symbols when we're using the in-tree libc++abi. This does mean that libc++.dylib won't re-export any libc++abi symbols when building against the system libc++abi, which could be fixed in various ways. However, the best solution really depends on the intended use case, so this patch doesn't try to solve that problem. As a drive-by, also improve the diagnostic message when the user forgets to set the LIBCXX_CXX_ABI_INCLUDE_PATHS variable, which would previously lead to a confusing error. Closes llvm#104672 (cherry picked from commit 21da4e7)
) On Apple platforms, using system-libcxxabi as an ABI library wouldn't work because we'd try to re-export symbols from libc++abi that the system libc++abi.dylib might not have. Instead, only re-export those symbols when we're using the in-tree libc++abi. This does mean that libc++.dylib won't re-export any libc++abi symbols when building against the system libc++abi, which could be fixed in various ways. However, the best solution really depends on the intended use case, so this patch doesn't try to solve that problem. As a drive-by, also improve the diagnostic message when the user forgets to set the LIBCXX_CXX_ABI_INCLUDE_PATHS variable, which would previously lead to a confusing error. Closes llvm#104672 (cherry picked from commit 21da4e7)
|
FWIW, there are still releases happening for 19.x, so this could still be backported. |
|
I think it were: |
|
Yeah, I noticed when rebasing on 19.1.6! Just didn't see an xref when I looked at the PR again (well, except the little ones for commits that often happen spuriously, so I didn't look closely enough). Sorry for the noise. |
|
No problem. Just were worried the fix was insufficient for you. |
On Apple platforms, using system-libcxxabi as an ABI library wouldn't work because we'd try to re-export symbols from libc++abi that the system libc++abi.dylib might not have. Instead, only re-export those symbols when we're using the in-tree libc++abi.
This does mean that libc++.dylib won't re-export any libc++abi symbols when building against the system libc++abi, which could be fixed in various ways. However, the best solution really depends on the intended use case, so this patch doesn't try to solve that problem.
As a drive-by, also improve the diagnostic message when the user forgets to set the LIBCXX_CXX_ABI_INCLUDE_PATHS variable, which would previously lead to a confusing error.
Closes #104672