[libcxx] Use interface library for libcxx-abi-shared

[nikic]

Summary

The libc++.so linker script generation uses the IMPORTED_LIBNAME
target property on libcxx-abi-shared. However, libcxx-abi-shared
is not an interface library and as such cannot have an
IMPORTED_LIBNAME target property.

Convert libcxx-abi-shared into an imported interface library
and use IMPORTED_LIBNAME in place of IMPORTED_LOCATION. This makes
linker script generation work correctly with system-libcxxabi.

I believe this fixes the issue that D131037 was intended to fix.

[ldionne]

libcxx/cmake/Modules/HandleLibCXXABI.cmake

69

Instead, why don't we make this:

add_library(${target} ${kind} INTERFACE IMPORTED GLOBAL)
# current stuff
set_target_properties(${target} PROPERTIES IMPORTED_LIBNAME ${name})

And then we could keep using imported_library below? Or is setting IMPORTED_LOCATION not valid on an INTERFACE IMPORTED library?

[nikic]

For the record, here is the reproducer:

cmake -G Ninja -S runtimes -B build-libcxx \
    -DLLVM_ENABLE_RUNTIMES="libcxx" \
    -DLIBCXX_CXX_ABI=system-libcxxabi \
    -DLIBCXX_CXX_ABI_INCLUDE_PATHS=/usr/include
ninja -Cbuild-libcxx

I think the linker script is currently corrupted for any value of LIBCXX_CXX_ABI other than libcxxabi.

libcxx/cmake/Modules/HandleLibCXXABI.cmake

69

From a quick test, setting IMPORTED_LOCATION on an interface library seems to be allowed, while setting IMPORTED_LIBNAME on a non-interface library is forbidden. However, I'm not sure why we would want to set IMPORTED_LOCATION, if we already set and use IMPORTED_LIBNAME.

[ldionne]

libcxx/cmake/Modules/HandleLibCXXABI.cmake

69

It's just that if we set both, we can use the same function to declare both our static and our shared library targets. The only thing I don't like with the patch as-is is that it introduces a second function that differs from the existing imported_library function only in a really subtle way. Someone adding a new choice of LIBCXX_CXX_ABI would have a hard time understanding why to use one over the other, I think.

Instead, I'd add

# Necessary for linker script generation
set_target_properties(${target} PROPERTIES IMPORTED_LIBNAME ${name})

to imported_library and use that for both our static and shared libraries below.

[nikic]

Use import_static_library() and import_shared_library() functions for clearer semantics.

libcxx/cmake/Modules/HandleLibCXXABI.cmake

69

What do you think about this variant? The updated names should make it clear when to use each function, and allow us to drop some more code that is no longer necessary.

libcxx/cmake/Modules/HandleLibCXXABI.cmake

function(import_private_headers target include_dirs headers)
 
   target_link_libraries(${target} INTERFACE ${target}-generate-private-headers)
   target_include_directories(${target} INTERFACE "${LIBCXX_BINARY_DIR}/private-abi-headers")
 endfunction()
 
 # This function creates an imported library named <target> of the given <kind> (SHARED|STATIC).
 # It imports a library named <name> searched at the given <path>.
 function(imported_library target kind path name)
   add_library(${target} ${kind} IMPORTED GLOBAL)
   set(libnames "${CMAKE_${kind}_LIBRARY_PREFIX}${name}${CMAKE_${kind}_LIBRARY_SUFFIX}")
   # Make sure we find .tbd files on macOS
   if (kind STREQUAL "SHARED")
     list(APPEND libnames "${CMAKE_${kind}_LIBRARY_PREFIX}${name}.tbd")
   endif()
   find_library(file
     NAMES ${libnames}
     PATHS "${path}"
     NO_CACHE)
   set_target_properties(${target} PROPERTIES IMPORTED_LOCATION "${file}")
 endfunction()
 
+function(interface_library target name)
+  add_library(${target} INTERFACE IMPORTED GLOBAL)
+  set_target_properties(${target} PROPERTIES IMPORTED_LIBNAME "${name}")
+endfunction()
+
 # Link against a system-provided libstdc++
 if ("${LIBCXX_CXX_ABI}" STREQUAL "libstdc++")
   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")
   target_compile_definitions(libcxx-abi-headers INTERFACE "-DLIBSTDCXX" "-D__GLIBCXX__")
 
-  imported_library(libcxx-abi-shared SHARED "${LIBCXX_CXX_ABI_LIBRARY_PATH}" stdc++)
+  interface_library(libcxx-abi-shared stdc++)
   target_link_libraries(libcxx-abi-shared INTERFACE libcxx-abi-headers)
 
   imported_library(libcxx-abi-static STATIC "${LIBCXX_CXX_ABI_LIBRARY_PATH}" stdc++)
   target_link_libraries(libcxx-abi-static INTERFACE libcxx-abi-headers)
 
 # Link against a system-provided libsupc++
 elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libsupc++")
   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")
   target_compile_definitions(libcxx-abi-headers INTERFACE "-D__GLIBCXX__")
 
-  imported_library(libcxx-abi-shared SHARED "${LIBCXX_CXX_ABI_LIBRARY_PATH}" supc++)
+  interface_library(libcxx-abi-shared supc++)
   target_link_libraries(libcxx-abi-shared INTERFACE libcxx-abi-headers)
 
   imported_library(libcxx-abi-static STATIC "${LIBCXX_CXX_ABI_LIBRARY_PATH}" supc++)
   target_link_libraries(libcxx-abi-static INTERFACE libcxx-abi-headers)
 
 # Link against the in-tree libc++abi
 elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
   add_library(libcxx-abi-headers INTERFACE)
elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
   endif()
 
 # Link against a system-provided libc++abi
 elseif ("${LIBCXX_CXX_ABI}" STREQUAL "system-libcxxabi")
   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")
 
-  imported_library(libcxx-abi-shared SHARED "${LIBCXX_CXX_ABI_LIBRARY_PATH}" c++abi)
+  interface_library(libcxx-abi-shared c++abi)
   target_link_libraries(libcxx-abi-shared INTERFACE libcxx-abi-headers)
 
   imported_library(libcxx-abi-static STATIC "${LIBCXX_CXX_ABI_LIBRARY_PATH}" c++abi)
   target_link_libraries(libcxx-abi-static INTERFACE libcxx-abi-headers)
 
 # Link against a system-provided libcxxrt
 elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxrt")
   # libcxxrt does not provide aligned new and delete operators
   # TODO: We're keeping this for backwards compatibility, but this doesn't belong here.
   set(LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS ON)
 
   if(NOT LIBCXX_CXX_ABI_INCLUDE_PATHS)
     message(STATUS "LIBCXX_CXX_ABI_INCLUDE_PATHS not set, using /usr/include/c++/v1")
     set(LIBCXX_CXX_ABI_INCLUDE_PATHS "/usr/include/c++/v1")
   endif()
   add_library(libcxx-abi-headers INTERFACE)
   import_private_headers(libcxx-abi-headers "${LIBCXX_CXX_ABI_INCLUDE_PATHS}"
     "cxxabi.h;unwind.h;unwind-arm.h;unwind-itanium.h")
   target_compile_definitions(libcxx-abi-headers INTERFACE "-DLIBCXXRT")
 
-  imported_library(libcxx-abi-shared SHARED "${LIBCXX_CXX_ABI_LIBRARY_PATH}" cxxrt)
+  interface_library(libcxx-abi-shared cxxrt)
   target_link_libraries(libcxx-abi-shared INTERFACE libcxx-abi-headers)
 
   imported_library(libcxx-abi-static STATIC "${LIBCXX_CXX_ABI_LIBRARY_PATH}" cxxrt)
   target_link_libraries(libcxx-abi-static INTERFACE libcxx-abi-headers)
 
 # Link against a system-provided vcruntime
 # FIXME: Figure out how to configure the ABI library on Windows.
 elseif ("${LIBCXX_CXX_ABI}" STREQUAL "vcruntime")