Jack Howarth via llvm-dev
2016-Feb-09 20:41 UTC
[llvm-dev] D16945: LLVM overhaul to avoid linking LLVM component libraries with libLLVM
On Mon, Feb 8, 2016 at 12:45 PM, Hans Wennborg <hans at chromium.org> wrote:> Chris Bieneman is probably your best bet, and maybe also Dan Liew. >Hans, My current, and hopefully final, revision of the proposed patch is simplified and reworked to solve the problem entirely from cmake without touching the the llvm-build python scripts. Basically, the new fix for avoiding the Support library dependency set from the 'required_libraries = Support' in llvm/utils/unittest/LLVMBuild.txt is not read it for LLVM_LINK_LLVM_DYLIB. The new patch modifies llvm_add_library() in cmake/modules/AddLLVM.cmake to manually set the LLVMBUILD_LIB_DEPS_gtest property to LLVM when LLVM_LINK_LLVM_DYLIB is set and the library name is gtest rather than read from the tools/llvm-config/LibraryDependencies.inc generated by llvm-build. Jack ps As soon as this patch lands, I'll look at trying to rework the unittests handling of the LLVMSupport dependency and see if setting that explicitly can be dropped (as it should be pulled in from the LLVMBUILD_LIB_DEPS_gtest property setting of Support). However that really isn't suitable for back porting to 3.8 so I want to avoid conflating those changes with those required to solve the test suite failures in bug 26393. Index: cmake/modules/AddLLVM.cmake ==================================================================--- cmake/modules/AddLLVM.cmake (revision 260200) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -473,17 +473,23 @@ # It would be nice to verify that we have the dependencies for this library # name, but using get_property(... SET) doesn't suffice to determine if a # property has been set to an empty value. - get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_${name}) - - if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_STATIC AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB) - set(llvm_libs LLVM) + if (LLVM_LINK_LLVM_DYLIB AND ${name} STREQUAL gtest) + set_property(GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_gtest LLVM) else() - llvm_map_components_to_libnames(llvm_libs - ${ARG_LINK_COMPONENTS} - ${LLVM_LINK_COMPONENTS} - ) + get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_${name}) endif() + if (DEFINED LLVM_LINK_COMPONENTS OR DEFINED ARG_LINK_COMPONENTS) + if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB) + set(llvm_libs LLVM) + else() + llvm_map_components_to_libnames(llvm_libs + ${ARG_LINK_COMPONENTS} + ${LLVM_LINK_COMPONENTS} + ) + endif() + endif() + if(CMAKE_VERSION VERSION_LESS 2.8.12) # Link libs w/o keywords, assuming PUBLIC. target_link_libraries(${name} @@ -885,11 +891,18 @@ add_llvm_executable(${test_name} IGNORE_EXTERNALIZE_DEBUGINFO ${ARGN}) set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}) set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir}) + if (LLVM_LINK_LLVM_DYLIB) + target_link_libraries(${test_name} + gtest + gtest_main + ) + else() target_link_libraries(${test_name} - gtest - gtest_main - LLVMSupport # gtest needs it for raw_ostream. - ) + gtest + gtest_main + LLVMSupport # Depends on llvm::cl + ) + endif() add_dependencies(${test_suite} ${test_name}) get_target_property(test_suite_folder ${test_suite} FOLDER) Index: cmake/modules/LLVM-Config.cmake ==================================================================--- cmake/modules/LLVM-Config.cmake (revision 260200) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@ # done in case libLLVM does not contain all of the components # the target requires. # - # TODO strip LLVM_DYLIB_COMPONENTS out of link_components. + # Strip LLVM_DYLIB_COMPONENTS out of link_components. # To do this, we need special handling for "all", since that # may imply linking to libraries that are not included in # libLLVM. + if (DEFINED link_components AND DEFINED LLVM_DYLIB_COMPONENTS) + if("${LLVM_DYLIB_COMPONENTS}" STREQUAL "all") + set(link_components "") + else() + list(REMOVE_ITEM link_components ${LLVM_DYLIB_COMPONENTS}) + endif() + endif() target_link_libraries(${executable} LLVM) endif() Index: utils/unittest/CMakeLists.txt ==================================================================--- utils/unittest/CMakeLists.txt (revision 260200) +++ utils/unittest/CMakeLists.txt (working copy) @@ -32,9 +32,15 @@ add_definitions( -DGTEST_HAS_PTHREAD=0 ) endif() -set(LIBS - LLVMSupport # Depends on llvm::raw_ostream -) +if (LLVM_LINK_LLVM_DYLIB) + set(LIBS + LLVM # Depends on llvm::raw_ostream + ) +else() + set(LIBS + LLVMSupport # Depends on llvm::raw_ostream + ) +endif() find_library(PTHREAD_LIBRARY_PATH pthread) if (PTHREAD_LIBRARY_PATH) Index: utils/unittest/UnitTestMain/CMakeLists.txt ==================================================================--- utils/unittest/UnitTestMain/CMakeLists.txt (revision 260200) +++ utils/unittest/UnitTestMain/CMakeLists.txt (working copy) @@ -1,7 +1,17 @@ -add_llvm_library(gtest_main - TestMain.cpp +if (LLVM_LINK_LLVM_DYLIB) + add_llvm_library(gtest_main + TestMain.cpp - LINK_LIBS - gtest - LLVMSupport # Depends on llvm::cl - ) + LINK_LIBS + gtest + LLVM # Depends on llvm::cl + ) +else() + add_llvm_library(gtest_main + TestMain.cpp + + LINK_LIBS + gtest + LLVMSupport # Depends on llvm::cl + ) +endif()> On Sat, Feb 6, 2016 at 10:55 AM, Jack Howarth > <howarth.mailing.lists at gmail.com> wrote: >> Hans, >> I have posted a complete patch for solving the linkage issues >> with LLVM_LINK_LLVM_DYLIB on Phabricator at >> http://reviews.llvm.org/D16945. The bulk of the fix the simple >> changes of... >> >> Index: cmake/modules/AddLLVM.cmake >> ==================================================================>> --- cmake/modules/AddLLVM.cmake (revision 259743) >> +++ cmake/modules/AddLLVM.cmake (working copy) >> @@ -475,13 +475,15 @@ >> # property has been set to an empty value. >> get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_${name}) >> >> - if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_STATIC AND NOT >> ARG_DISABLE_LLVM_LINK_LLVM_DYLIB) >> - set(llvm_libs LLVM) >> - else() >> - llvm_map_components_to_libnames(llvm_libs >> - ${ARG_LINK_COMPONENTS} >> - ${LLVM_LINK_COMPONENTS} >> - ) >> + if (DEFINED LLVM_LINK_COMPONENTS OR DEFINED ARG_LINK_COMPONENTS) >> + if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB) >> + set(llvm_libs LLVM) >> + else() >> + llvm_map_components_to_libnames(llvm_libs >> + ${ARG_LINK_COMPONENTS} >> + ${LLVM_LINK_COMPONENTS} >> + ) >> + endif() >> endif() >> >> if(CMAKE_VERSION VERSION_LESS 2.8.12) >> Index: cmake/modules/LLVM-Config.cmake >> ==================================================================>> --- cmake/modules/LLVM-Config.cmake (revision 259743) >> +++ cmake/modules/LLVM-Config.cmake (working copy) >> @@ -40,10 +40,17 @@ >> # done in case libLLVM does not contain all of the components >> # the target requires. >> # >> - # TODO strip LLVM_DYLIB_COMPONENTS out of link_components. >> + # Strip LLVM_DYLIB_COMPONENTS out of link_components. >> # To do this, we need special handling for "all", since that >> # may imply linking to libraries that are not included in >> # libLLVM. >> + if (DEFINED link_components AND DEFINED LLVM_DYLIB_COMPONENTS) >> + if("${LLVM_DYLIB_COMPONENTS}" STREQUAL "all") >> + set(link_components "") >> + else() >> + list(REMOVE_ITEM link_components ${LLVM_DYLIB_COMPONENTS}) >> + endif() >> + endif() >> target_link_libraries(${executable} LLVM) >> endif() >> >> However the avoiding the accidental linkage of libLLVMSupport with >> libLLVM and libgtest for the unittests was really tricky as two >> different mechanisms to pass LLVMSupport are in play. The underlying >> problem was that the python based llvm-build tool was forcing a >> dependency on LLVMSupport for libgtest according to the >> required_libraries entry for the gtest library in >> utils/unittest/LLVMBuild.txt. Since the llvm-build tool has no access >> to the cmake defines, I had to solve this problem by adding a new >> --enable-llvm-link-llvm-dylib option in >> utils/llvm-build/llvmbuild/main.py to allow the LLVM_LINK_LLVM_DYLIB >> build to prune 'Support' from the the required library dependencies of >> the gtest library before the creation of >> tools/llvm-config/LibraryDependencies.inc. Now the >> LLVM_LINK_LLVM_DYLIB build contains... >> >> { "gtest", "libgtest.a", false, { } }, >> >> rather than >> >> { "gtest", "libgtest.a", false, { "support" } }, >> >> in that file. If there are specific maintainers for the llvm-build >> code, please add them to the review. >> Thanks in advance. >> Jack
Jack Howarth via llvm-dev
2016-Feb-10 13:57 UTC
[llvm-dev] D16945: LLVM overhaul to avoid linking LLVM component libraries with libLLVM
Hans and Chris,
Andrew Wilkins refactored my last patch for
http://reviews.llvm.org/D16945 into a version that properly handles
the Support library dependency of the gtest library without requiring
conditionals on LLVM_LINK_LLVM_DYLIB in
cmake/modules/LLVM-Config.cmake and utils/unittest/CMakeLists.txt. He
is awaiting a review from Chris before committing it to trunk. I have
successfully regression tested this version on the stock (static),
BUILD_SHARED_LIBS and LLVM_LINK_LLVM_DYLIB builds of
llvm/clang/clang-tools-extra/compiler-rt/openmp/polly/libc++ on x86_64
darwin, The patch also successfully applies and regression tests on
3.8 branch so hopefully we can squeeze this in for the 3.8.0 releases
since currently the LLVM_LINK_LLVM_DYLIB build produces broken
binaries on both trees.
Jack
Index: cmake/modules/AddLLVM.cmake
==================================================================---
cmake/modules/AddLLVM.cmake (revision 259475)
+++ cmake/modules/AddLLVM.cmake (working copy)
@@ -468,20 +468,23 @@
endif()
endif()
- # Add the explicit dependency information for this library.
- #
- # It would be nice to verify that we have the dependencies for this library
- # name, but using get_property(... SET) doesn't suffice to determine if a
- # property has been set to an empty value.
- get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_${name})
-
- if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_STATIC AND NOT
ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
- set(llvm_libs LLVM)
+ if (DEFINED LLVM_LINK_COMPONENTS OR DEFINED ARG_LINK_COMPONENTS)
+ if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
+ set(llvm_libs LLVM)
+ else()
+ llvm_map_components_to_libnames(llvm_libs
+ ${ARG_LINK_COMPONENTS}
+ ${LLVM_LINK_COMPONENTS}
+ )
+ endif()
else()
- llvm_map_components_to_libnames(llvm_libs
- ${ARG_LINK_COMPONENTS}
- ${LLVM_LINK_COMPONENTS}
- )
+ # Components have not been defined explicitly in CMake, so add the
+ # dependency information for this library as defined by LLVMBuild.
+ #
+ # It would be nice to verify that we have the dependencies for this library
+ # name, but using get_property(... SET) doesn't suffice to determine if
a
+ # property has been set to an empty value.
+ get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_${name})
endif()
if(CMAKE_VERSION VERSION_LESS 2.8.12)
@@ -882,14 +885,11 @@
set(LLVM_REQUIRES_RTTI OFF)
+ list(APPEND LLVM_LINK_COMPONENTS Support) # gtest needs it for raw_ostream
add_llvm_executable(${test_name} IGNORE_EXTERNALIZE_DEBUGINFO ${ARGN})
set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
- target_link_libraries(${test_name}
- gtest
- gtest_main
- LLVMSupport # gtest needs it for raw_ostream.
- )
+ target_link_libraries(${test_name} gtest_main gtest)
add_dependencies(${test_suite} ${test_name})
get_target_property(test_suite_folder ${test_suite} FOLDER)
Index: cmake/modules/LLVM-Config.cmake
==================================================================---
cmake/modules/LLVM-Config.cmake (revision 259475)
+++ cmake/modules/LLVM-Config.cmake (working copy)
@@ -40,10 +40,19 @@
# done in case libLLVM does not contain all of the components
# the target requires.
#
- # TODO strip LLVM_DYLIB_COMPONENTS out of link_components.
+ # Strip LLVM_DYLIB_COMPONENTS out of link_components.
# To do this, we need special handling for "all", since that
# may imply linking to libraries that are not included in
# libLLVM.
+
+ if (DEFINED link_components AND DEFINED LLVM_DYLIB_COMPONENTS)
+ if("${LLVM_DYLIB_COMPONENTS}" STREQUAL "all")
+ set(link_components "")
+ else()
+ list(REMOVE_ITEM link_components ${LLVM_DYLIB_COMPONENTS})
+ endif()
+ endif()
+
target_link_libraries(${executable} LLVM)
endif()
Index: utils/unittest/CMakeLists.txt
==================================================================---
utils/unittest/CMakeLists.txt (revision 259475)
+++ utils/unittest/CMakeLists.txt (working copy)
@@ -32,10 +32,6 @@
add_definitions( -DGTEST_HAS_PTHREAD=0 )
endif()
-set(LIBS
- LLVMSupport # Depends on llvm::raw_ostream
-)
-
find_library(PTHREAD_LIBRARY_PATH pthread)
if (PTHREAD_LIBRARY_PATH)
list(APPEND LIBS pthread)
@@ -46,6 +42,9 @@
LINK_LIBS
${LIBS}
+
+ LINK_COMPONENTS
+ Support # Depends on llvm::raw_ostream
)
add_subdirectory(UnitTestMain)
Index: utils/unittest/UnitTestMain/CMakeLists.txt
==================================================================---
utils/unittest/UnitTestMain/CMakeLists.txt (revision 259475)
+++ utils/unittest/UnitTestMain/CMakeLists.txt (working copy)
@@ -3,5 +3,7 @@
LINK_LIBS
gtest
- LLVMSupport # Depends on llvm::cl
+
+ LINK_COMPONENTS
+ Support # Depends on llvm::cl
)
Jack Howarth via llvm-dev
2016-Feb-12 14:18 UTC
[llvm-dev] D16945: LLVM overhaul to avoid linking LLVM component libraries with libLLVM
Confirmed on current trunk that Bug 26393,
-DLLVM_LINK_LLVM_DYLIB:BOOL=ON produces many llvm test suite
regressions, is now fixed at r260641 on x86_64 darwin for the stock
(static), BUILD_SHARED_LIBS and LLVM_LINK_LLVM_DYLIB builds which all
produce clean test suite results for
llvm/clang/clang-tools/compiler-rt/polly/openmp/libc++. Since no build
bot regressions appear at http://lab.llvm.org:8011/console, the
changes in r260641 appear safe for a back port to 3.8 to fix the
LLVM_LINK_LLVM_DYLIB feature there.
Jack
On Wed, Feb 10, 2016 at 8:57 AM, Jack Howarth
<howarth.mailing.lists at gmail.com> wrote:> Hans and Chris,
> Andrew Wilkins refactored my last patch for
> http://reviews.llvm.org/D16945 into a version that properly handles
> the Support library dependency of the gtest library without requiring
> conditionals on LLVM_LINK_LLVM_DYLIB in
> cmake/modules/LLVM-Config.cmake and utils/unittest/CMakeLists.txt. He
> is awaiting a review from Chris before committing it to trunk. I have
> successfully regression tested this version on the stock (static),
> BUILD_SHARED_LIBS and LLVM_LINK_LLVM_DYLIB builds of
> llvm/clang/clang-tools-extra/compiler-rt/openmp/polly/libc++ on x86_64
> darwin, The patch also successfully applies and regression tests on
> 3.8 branch so hopefully we can squeeze this in for the 3.8.0 releases
> since currently the LLVM_LINK_LLVM_DYLIB build produces broken
> binaries on both trees.
> Jack
>
> Index: cmake/modules/AddLLVM.cmake
> ==================================================================> ---
cmake/modules/AddLLVM.cmake (revision 259475)
> +++ cmake/modules/AddLLVM.cmake (working copy)
> @@ -468,20 +468,23 @@
> endif()
> endif()
>
> - # Add the explicit dependency information for this library.
> - #
> - # It would be nice to verify that we have the dependencies for this
library
> - # name, but using get_property(... SET) doesn't suffice to determine
if a
> - # property has been set to an empty value.
> - get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_${name})
> -
> - if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_STATIC AND NOT
> ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
> - set(llvm_libs LLVM)
> + if (DEFINED LLVM_LINK_COMPONENTS OR DEFINED ARG_LINK_COMPONENTS)
> + if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
> + set(llvm_libs LLVM)
> + else()
> + llvm_map_components_to_libnames(llvm_libs
> + ${ARG_LINK_COMPONENTS}
> + ${LLVM_LINK_COMPONENTS}
> + )
> + endif()
> else()
> - llvm_map_components_to_libnames(llvm_libs
> - ${ARG_LINK_COMPONENTS}
> - ${LLVM_LINK_COMPONENTS}
> - )
> + # Components have not been defined explicitly in CMake, so add the
> + # dependency information for this library as defined by LLVMBuild.
> + #
> + # It would be nice to verify that we have the dependencies for this
library
> + # name, but using get_property(... SET) doesn't suffice to
determine if a
> + # property has been set to an empty value.
> + get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_${name})
> endif()
>
> if(CMAKE_VERSION VERSION_LESS 2.8.12)
> @@ -882,14 +885,11 @@
>
> set(LLVM_REQUIRES_RTTI OFF)
>
> + list(APPEND LLVM_LINK_COMPONENTS Support) # gtest needs it for
raw_ostream
> add_llvm_executable(${test_name} IGNORE_EXTERNALIZE_DEBUGINFO ${ARGN})
> set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
> set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR
${outdir})
> - target_link_libraries(${test_name}
> - gtest
> - gtest_main
> - LLVMSupport # gtest needs it for raw_ostream.
> - )
> + target_link_libraries(${test_name} gtest_main gtest)
>
> add_dependencies(${test_suite} ${test_name})
> get_target_property(test_suite_folder ${test_suite} FOLDER)
> Index: cmake/modules/LLVM-Config.cmake
> ==================================================================> ---
cmake/modules/LLVM-Config.cmake (revision 259475)
> +++ cmake/modules/LLVM-Config.cmake (working copy)
> @@ -40,10 +40,19 @@
> # done in case libLLVM does not contain all of the components
> # the target requires.
> #
> - # TODO strip LLVM_DYLIB_COMPONENTS out of link_components.
> + # Strip LLVM_DYLIB_COMPONENTS out of link_components.
> # To do this, we need special handling for "all", since that
> # may imply linking to libraries that are not included in
> # libLLVM.
> +
> + if (DEFINED link_components AND DEFINED LLVM_DYLIB_COMPONENTS)
> + if("${LLVM_DYLIB_COMPONENTS}" STREQUAL "all")
> + set(link_components "")
> + else()
> + list(REMOVE_ITEM link_components ${LLVM_DYLIB_COMPONENTS})
> + endif()
> + endif()
> +
> target_link_libraries(${executable} LLVM)
> endif()
>
> Index: utils/unittest/CMakeLists.txt
> ==================================================================> ---
utils/unittest/CMakeLists.txt (revision 259475)
> +++ utils/unittest/CMakeLists.txt (working copy)
> @@ -32,10 +32,6 @@
> add_definitions( -DGTEST_HAS_PTHREAD=0 )
> endif()
>
> -set(LIBS
> - LLVMSupport # Depends on llvm::raw_ostream
> -)
> -
> find_library(PTHREAD_LIBRARY_PATH pthread)
> if (PTHREAD_LIBRARY_PATH)
> list(APPEND LIBS pthread)
> @@ -46,6 +42,9 @@
>
> LINK_LIBS
> ${LIBS}
> +
> + LINK_COMPONENTS
> + Support # Depends on llvm::raw_ostream
> )
>
> add_subdirectory(UnitTestMain)
> Index: utils/unittest/UnitTestMain/CMakeLists.txt
> ==================================================================> ---
utils/unittest/UnitTestMain/CMakeLists.txt (revision 259475)
> +++ utils/unittest/UnitTestMain/CMakeLists.txt (working copy)
> @@ -3,5 +3,7 @@
>
> LINK_LIBS
> gtest
> - LLVMSupport # Depends on llvm::cl
> +
> + LINK_COMPONENTS
> + Support # Depends on llvm::cl
> )