[llvm-dev] D16945: LLVM overhaul to avoid linking LLVM component libraries with libLLVM

[Jack Howarth via llvm-dev]

    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
>    )