[llvm-dev] D16945: LLVM overhaul to avoid linking LLVM component libraries with libLLVM
Jack Howarth via llvm-dev
llvm-dev at lists.llvm.org
Tue Feb 9 12:41:00 PST 2016
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
More information about the llvm-dev
mailing list