LLVM overhaul to avoid linking LLVM component libraries with libLLVM

[jwhowarth]

Summary

Current trunk suffers several thousand llvm test suite failures for the LLVM_LINK_LLVM_DYLIB build due to incorrect linkages of both libLLVM and the individual LLVM component static libraries. The proposed changes to the llvm_config() macro in cmake/modules/LLVM-Config.cmake and the llvm_add_library() function in cmake/modules/AddLLVM.cmake solves this issue by stripping the individual LLVM component libraries from LLVM_LINK_COMPONENTS and LINK_COMPONENTS for the LLVM_LINK_LLVM_DYLIB build. To address the remaining instances where the unittests executables are incorrectly linked against both libLLVM and libLLVMSupport in the LLVM_LINK_LLVM_DYLIB build, the explicit linkage of LLVMSupport is removed from utils/unittest/CMakeLists.txt. However, the explicit linkage on LLVMSupport in the llvm_add_library name function of cmake/modules/AddLLVM.cmake and utils/unittest/UnitTestMain/CMakeLists.txt are retained and conditionalized on LLVM_LINK_LLVM_DYLIB. These are required to prevent the BUILD_SHARED_LIBS build from missing required the LLVMSupport linkages on unittests executables. Finally a new --enable-llvm-link-llvm-dylib is introduced in utils/llvm-build/llvmbuild/main.py and utilized in the top-level CMakeLists.txt file for the LLVM_LINK_LLVM_DYLIB build. In utils/llvm-build/llvmbuild/main.py, this new option sets a use_llvm_link_llvm_dylib global which is then tested in the write_library_table routine to control pruning 'Support' from the required dependencies of the gtest library. This produces the desired result for the DLLVM_LINK_LLVM_DYLIB build of the producing a tools/llvm-config/LibraryDependencies.inc file containing...

{ "gtest", "libgtest.a", false, { } },

instead of

{ "gtest", "libgtest.a", false, { "support" } },

The command...

find . -name link.txt -print0 | xargs -0 cat | grep libgtest | grep -v -c libLLVMSupport

was used to verify that none of the linkages on libgtest were missing the required linkages of libLLVMSupport for the stock and BUILD_SHARED_LIBS builds.

find . -name link.txt -print0 | xargs -0 cat | grep libgtest | grep libLLVM | grep -c libLLVMSupport

was used to verify that none of the unitests linkages on the LLVM_LINK_LLVM_DYLIB build had both libLLVM.dylib and libLLVMSupport.

Confirmed bootstraps on x86_64 darwin for the stock (static), BUILD_SHARED_LIBS and LLVM_LINK_LLVM_DYLIB builds of current trunk with this patch.

[jwhowarth]

Confirmed for 3-stage bootstraps (with stage-2/stage-3 file comparison) on x86-64 darwin of llvm/clang/clang-extras/compiler-rt/polly/openmp/libc++ svn trunk with diff 47085 applied that no regressions on the stock (static), BUILD_SHARED_LIBS and LLVM_LINK_LLVM_DYLIB builds are produced.

[jwhowarth]

The original bug report this bug address is [[ URL | https://llvm.org/bugs/show_bug.cgi?id=26393 ]].

[jwhowarth]

Unfortunately, I haven't caught every single hard-coding of LLVMSupport linkages. We still have a libLLVMSupport.a linkage with libLLVM and libgtest for FileCheck which seems to be hardcoded in cmake/modules/TableGen.cmake. So we will have to conditionalize that on. We are also getting those for the linkage of not, llvm-tblgen and yams-bench as well.

[jwhowarth]

Eliminated linkages against both libLLVM and libLLVMSupport for FileCheck, KillTheDoctor, not and yams-bench with addition of a conditional on LLVM_LINK_LLVM_DYLIB in their CMakeLists.txt. Also, restored LIBS assignment using a conditional in unittest/CMakeLists.txt. I suspect the cmake files in the utils subdirectories are configured differently because TableGen build needs to link against selective static LLVM components as it is used in the later LLVM component builds.

[jwhowarth]

Tested Diff 47114 with 3-stage bootstraps and stage2/stage3 file comparison for llvm/clang/clang-tools-extra/polly/openmp/libc++ on x86_64 darwin. The stock (static), BUILD_SHARED_LIBS and LLVM_LINK_LLVM_DYLIB builds all show no regressions in the test suite. Also confirmed on the LLVM_LINK_LLVM_DYLIB build using...

find . -name link.txt -print0 | xargs -0 cat | grep libLLVM | grep libLLVMSupport | grep -v "/usr/bin/ar"

that the only executable files linked against libLLVMSupport.a are llvm-tblgen and clang-tblgen and neither (correctly) are linked to libLLVM.dylib. Also repeated the same testing regime with the patch applied to current 3.8 branch without issues. So the patch now looks complete to me.

[jwhowarth]

Confirmed that the instance of 'LLVM' appended on target_link_libraries for LLVM_LINK_LLVM_DYLIB is essential in utils/FileCheck/CMakeLists.txt, utils/KillTheDoctor/CMakeLists.txt, utils/not/CMakeLists.txt and utils/yaml-bench/CMakeLists.txt. Removal of 'LLVM" from those instances of target_link_libraries() for LLVM_LINK_LLVM_DYLIB in Diff ID 47114 prunes it from the generated link.txt.

Likewise, also confirmed that the instance of the appended 'LLVM' on add_llvm_library() in utils/unittest/UnitTestMain/CMakeLists.txt is essential. Removal of 'LLVM" from those instances of target_link_libraries() for LLVM_LINK_LLVM_DYLIB in Diff ID 47114 prunes it from the generated link.txt.

[jwhowarth]

With regard to the changes in utils/unittest/CMakeLists.txt, these were introduced with the commit of http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20140224/206085.html and then refactored in https://github.com/llvm-mirror/llvm/commit/e4f11175cb95cd8609c0a81c798a550bd9add685#diff-740fc9f6ddb86b18e2f51ef242f40b8a. Since it is unclear exactly where utils/unittest/CMakeLists.txt is used, I think we should treat it very conservatively as I do in Diff ID 47114.

[jwhowarth]

While a change to utils/unittest/CMakeLists.txt is required to avoid these failures for the LLVM_LINK_LLVM_DYLIB build...

Failing Tests (4):

Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.AddsTargetAndMode
Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.IgnoresExistingMode
Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.IgnoresExistingTarget
Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.PathIgnored

the fix can be reduced to just placing a 'if (NOT LLVM_LINK_LLVM_DYLIB)' wrapper around the setting of LIBS to LLVMSupport. The linkage for libLLVM is already being added elsewhere for these test cases.

[jwhowarth]

Simplify fix for utils/unittest/CMakeLists.txt to just adding 'if (NOT LLVM_LINK_LLVM_DYLIB)' wrapper.

[jwhowarth]

Confirmed for Diff ID 47155 that a 3-stage bootstrap with stage2/stage3 file comparison for llvm/clang/clang-tools-extra/polly/openmp/libc++ on x86_64 darwin built with LLVM_LINK_LLVM_DYLIB produces no test suite regressions on both trunk and 3.8 branch. All other sections of the proposed patch have now been verified as essential.

[axw]

I hope what I've said is clear - please ask if not. I can see if I can find some time to propose an alternative, but also very busy with day job.

cmake/modules/AddLLVM.cmake

890–901 ↗

(On Diff #47155)

Just remove the LLVMSupport. CMake will link the program with LLVM or LLVMSupport because gtest depends on it.

utils/FileCheck/CMakeLists.txt

5–9 ↗

(On Diff #47155)

A couple of issues:

• this is subverting the explicit "DISABLE_LLVM_LINK_LLVM_DYLIB" in add_llvm_utility
• if that weren't there, you could/should just do:

set(LLVM_LINK_COMPONENTS Support)
add_llvm_utility(FileCheck FileCheck.cpp)

(llvm_config will decide which libraries to link with depending on LLVM_LINK_LLVM_DYLIB)

Is there any harm in the utilities not using libLLVM? When I made the changes, only the packaged/distributed tools were made to link to libLLVM. I don't think the internal utilities really need to.

Ditto for the other utilities.

utils/KillTheDoctor/CMakeLists.txt

5 ↗

(On Diff #47155)

as above

utils/llvm-build/llvmbuild/main.py

353 ↗

(On Diff #47155)

This is a bit of a dirty hack (hard-coding component names and such), and I'm not convinced modifying LLVMBuild is the right layer to be changing anyway. Doing this means that "llvm-config --libs --link-static gtest" won't do the right thing anymore.

I really think you should be doing this at the CMake layer. I think what you want to do, instead, is to have LLVMBuild output CMake fragments with the component names rather than library names, and then include them in the LLVM_LINK_COMPONENTS handling.

That is:

• don't remove the dependency from gtest to the Support component
• in CMake, interpret the "required_libraries: Support" as "add Support to LLVM_LINK_COMPONENTS"; we'll then either link with libLLVM or libLLVMSupport as we do already

utils/unittest/CMakeLists.txt

35–39 ↗

(On Diff #47155)

AFAICT, the explicit dependency is not even needed, as long as you don't remove the dependency of gtest->{LLVM,LLVMSupport}.

utils/unittest/UnitTestMain/CMakeLists.txt

1–17 ↗

(On Diff #47155)

You should just be able to remove the LLVMSupport line, and otherwise stick with the original.
gtest_main depends on gtest, and gtest depends on LLVM or LLVMSupport. CMake will link the transitive closure.

[jwhowarth]

Any suggestions on how to adjust cmake to interpret the "required_libraries: Support" as "add Support to LLVM_LINK_COMPONENTS"? I am lost on that one.

cmake/modules/AddLLVM.cmake

890–901 ↗

(On Diff #47155)

This works as long as the associated unittests explicitly add Support to LLVM_LINK_COMPONENTS, Currently I have found five CMakeLists.txt across the llvm, cfe and clang-tools-extra sources that need this fix.

utils/FileCheck/CMakeLists.txt

5–9 ↗

(On Diff #47155)

Without a linkage on libLLVM, the LLVM_LINK_LLVM_DYLIB build of FileCheck fails on numerous unresolved symbols like llvm::cl::Option::anchor().

utils/llvm-build/llvmbuild/main.py

353 ↗

(On Diff #47155)

Any suggestions in how to adjust cmake to interpret the "required_libraries: Support" as "add Support to LLVM_LINK_COMPONENTS"?

utils/unittest/UnitTestMain/CMakeLists.txt

1–17 ↗

(On Diff #47155)

Removal of the LLVMSupport from the add_llvm_library arguments here will require all unittests to explicitly define...

set(LLVM_LINK_COMPONENTS
  Support
  )

in their local CMakeLists.txt file. If that is acceptable to force on the maintainers for code with unittests then we can go that route. Currently llvm/unittests/DebugInfo/DWARF/CMakeLists.txt, llvm/unittests/DebugInfo/PDB/CMakeLists.txt, llvm/unittests/Linker/CMakeLists.txt, cfe/unittests/libclang/CMakeLists.txt and clang-tools-extra/unittests/clang-apply-replacements/CMakeLists.txt need those fixes.

[jwhowarth]

Doesn't the correct handling of the 'support' library dependency of 'gtest' have to be special cased for LLVM_LINK_LLVM_DYLIB in expand_topologically() of cmake/modules/LLVM-Config.cmake? It preceded by the comment...

# Perform a post-order traversal of the dependency graph.
# This duplicates the algorithm used by llvm-config, originally
# in tools/llvm-config/llvm-config.cpp, function ComputeLibsForComponents.

[jwhowarth]

This version still requires the pruning of 'support' from the required_libraries of the 'gtest' library at the cmake level.

$ find . -name link.txt -print0 | xargs -0 cat | grep libgtest | grep libLLVM.dylib | grep -c libLLVMSupport
37

[jwhowarth]

Using message statements to print out link_components in explicit_llvm_config() of cmake/modules/LLVM-Config.cmake shows that the 'gtest' linkage never passes through there.

[jwhowarth]

I suspect the problem *might* be at...

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

in the llvm_add_library() function of cmake/modules/AddLLVM.cmake. I see it is passed 'gtest' during the cmake run on lib_deps, however I don't understand how to deconstruction the LLVMBUILD_LIB_DEPS_gtest that is being invoked.

[jwhowarth]

It does look like the 'get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_${name})' call for name as gtest is the problem. The build directory has...

set_property(GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_gtest LLVMSupport)

in the generated LLVMBuild.cmake.

[jwhowarth]

For LLVM_LINK_LLVM_DYLIB builds, set the library dependencies of the gtest library to LLVM within llvm_add_library(), using a set_property() on LLVMBUILD_LIB_DEPS_gtest, rather than reading them from tools/llvm-config/LibraryDependencies.inc using get_property(). The patch avoids any attempt to refactor the unitests build machinery to keep these changes suitable for back porting to 3.8 branch. Confirmed that bootstraps of llvm/clang/clang-tools-extra/compiler-rt/openmp/polly/libc++ on x86_64 darwin for the stock (static), BUILD_SHARED_LIBS and LLVM_LINK_LLVM_DYLIB builds of current trunk produce no test suite regressions. Refactoring the unitests build machinery should be a separate follow-on patch to trunk.

[jwhowarth]

utils/FileCheck/CMakeLists.txt

5–9 ↗

(On Diff #47276)

Reconfirmed that changing...

Index: utils/FileCheck/CMakeLists.txt
===================================================================
--- utils/FileCheck/CMakeLists.txt      (revision 260055)
+++ utils/FileCheck/CMakeLists.txt      (working copy)
@@ -1,5 +1,5 @@
+set(LLVM_LINK_COMPONENTS Support)
+
 add_llvm_utility(FileCheck
   FileCheck.cpp
   )
-
-target_link_libraries(FileCheck LLVMSupport)

for the LLVM_LINK_LLVM_DYLIB build with Diff ID 47276 ends up linking against libLLVMSupport.a rather than libLLVM.dylib. So the full conditional with explicit passing of LLVM on target_link_libraries() for LLVM_LINK_LLVM_DYLIB in Diff ID 47276 is required.

[jwhowarth]

Drop changes to the linkage of the utils programs from patch. Successsfully regression tested on x86_64 darwin for a llvm/clang/clang-tools-extra/compiler-rt/polly/openmp/libc++ build using LLVM_LINK_LLVM_DYLIB.

[jwhowarth]

Diff 6 ID 47297 also tested for the same llvm/clang/clang-tools-extra/compiler-rt/openmp/polly/libc++ build on x86_64 darwin for the stock (static) and BUILD_SHARED_LIBS builds without regression. Likewise tested the patch back ported to 3.8 branch for the LLVM_LINK_LLVM_DYLIB build without regressions.

[beanz]

Sorry I haven't been engaged on this thread. I'll try to do better.

Comments inline.

cmake/modules/AddLLVM.cmake

477 ↗

(On Diff #47297)

I'm really not sure I like this approach, but if you're going to do it, rather than doing this here, why not do it in the root CMakeLists.txt?

I think after line 491 you could add:

if(LLVM_LINK_LLVM_DYLIB)
  set_property(GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_gtest LLVM)
endif()

If you do it there, then I don't think any of the property reading code needs to change here.

The reason I don't like this approach is that the reason llvm-build exists is to supply dependency information the same way to projects using multiple build systems. That said, in this case it is probably fine because the gtest stuff should only be used by LLVM projects. So I think this is safe.

484 ↗

(On Diff #47297)

I don't believe this is correct. What happens if LLVM_LINK_COMPONENTS contains things not in LLVM_DYLIB_COMPNENTS?

This is why I had suggested on an earlier thread that we are doing the component filtering at the wrong layer. We probably should do it inside llvm_map_components_to_libnames.

894 ↗

(On Diff #47297)

Playing devil's advocate, what if LLVM_DYLIB_COMPONENTS didn't include LLVMSupport?

[axw]

cmake/modules/AddLLVM.cmake

484 ↗

(On Diff #47297)

How about just using llvm_config here instead, passing USE_SHARED as appropriate? That macro has already been fixed so that it links with libLLVM if USE_SHARED, but also with the component libraries not included in libLLVM.

[jwhowarth]

Use refactored patch from Andrew Wilkins instead. Tested stock (static), BUILD_SHARED_LIBS and LLVM_LINK_LLVM_DYLIB builds of current trunk with this patch on x86_64 darwin for llvm/clang/clang-tools-extra/compiler-rt/polly/openmp/libc++ with no regressions introduced and proper linkages verified. Okay for trunk and back port to 3.8 branch?

[axw]

IMO this is good to go, but I'm biased as I was involved in writing it. I'll resign as reviewer for that reason. @beanz, PTAL.

You're quite right that there's an issue if LLVM_DYLIB_COMPONENTS does not include all components, but I'm not sure that changing llvm_map_components_to_libnames is the right thing to do. For one thing, that's what libLLVM uses to determine the library names; that could be changed, of course. For another, there's llvm_config which essentially does what we want: conditionally link with libLLVM, and with any libraries that are not included in that.

The immediate issue is resolved, and LLVM_LINK_LLVM_DYLIB is opt-in and still fairly new. I'd like to defer the missing-component fix to a follow-up if you don't mind.

[beanz]

LGTM. Sorry this took so much back and forth.

Thank you both of you for all your work on this patch.

-Chris

llvm/trunk/cmake/modules/AddLLVM.cmake

if(NOT APPLE AND ARG_SONAME)
         COMPONENT ${name}
         ALWAYS_GENERATE)
       llvm_install_library_symlink(${output_name} ${library_name} SHARED
         COMPONENT ${name}
         ALWAYS_GENERATE)
     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)
+  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()
+    # 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)
     # Link libs w/o keywords, assuming PUBLIC.
     target_link_libraries(${name}
       ${ARG_LINK_LIBS}
       ${lib_deps}
       ${llvm_libs}
       )
function(add_unittest test_suite test_name)
   endif ()
 
   if (SUPPORTS_NO_VARIADIC_MACROS_FLAG)
     list(APPEND LLVM_COMPILE_FLAGS "-Wno-variadic-macros")
   endif ()
 
   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)
   if (NOT ${test_suite_folder} STREQUAL "NOTFOUND")
     set_property(TARGET ${test_name} PROPERTY FOLDER "${test_suite_folder}")
   endif ()
 endfunction()
 

llvm/trunk/cmake/modules/LLVM-Config.cmake

macro(llvm_config executable)
   set(link_components ${ARG_UNPARSED_ARGUMENTS})
 
   if(USE_SHARED)
     # If USE_SHARED is specified, then we link against libLLVM,
     # but also against the component libraries below. This is
     # 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()
 
   explicit_llvm_config(${executable} ${link_components})
 endmacro(llvm_config)
 
 
 function(explicit_llvm_config executable)

llvm/trunk/utils/unittest/CMakeLists.txt

 
 set(LLVM_REQUIRES_RTTI 1)
 add_definitions( -DGTEST_HAS_RTTI=0 )
 
 if (NOT LLVM_ENABLE_THREADS)
   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)
 endif()
 
 add_llvm_library(gtest
   googletest/src/gtest-all.cc
 
   LINK_LIBS
   ${LIBS}
+
+  LINK_COMPONENTS
+  Support # Depends on llvm::raw_ostream
 )
 
 add_subdirectory(UnitTestMain)

llvm/trunk/utils/unittest/UnitTestMain/CMakeLists.txt

 add_llvm_library(gtest_main
   TestMain.cpp
 
   LINK_LIBS
   gtest
-  LLVMSupport # Depends on llvm::cl
+
+  LINK_COMPONENTS
+  Support # Depends on llvm::cl
   )