[cmake] Fix build with BUILD_SHARED_LIBS=ON

[cristian.adam]

Summary

On Windows a build with BUILD_SHARED_LIBS=ON needs to store the dependencies between libraries, due to the import libraries that (.a/.lib) that Windows needs to have for DLL files.

The dependencies will also be available in the cmake files.

llvm\unittests\Passes needs "Core" as LLVM_LINK_COMPONENTS, otherwise it won't link.

[compnerd]

I don't understand the change for the PRIVATE to PUBLIC. That doesn't make a different to the import library handling, which is baked right into CMake, so that will always be handled properly irrespective of the link dependency's visibility. I think the only thing that may be needed is the Core dependency on the TestPlugin.

[cristian.adam]

In D63881#1561072, @compnerd wrote:

I don't understand the change for the PRIVATE to PUBLIC. That doesn't make a different to the import library handling, which is baked right into CMake, so that will always be handled properly irrespective of the link dependency's visibility. I think the only thing that may be needed is the Core dependency on the TestPlugin.

TestPlugin depends on Core. Core depends on BinaryFormat, Remarks, and Support. If we keep PRIVATE then TestPlugin will also need the dependencies from Core.

By having PUBLIC TestPlugin will get the dependencies from Core, and <INSTALL_DIR>\lib\cmake\llvm\LLVMExports.cmake will contain:

# Create imported target LLVMCore
add_library(LLVMCore SHARED IMPORTED)

set_target_properties(LLVMCore PROPERTIES
  INTERFACE_LINK_LIBRARIES "LLVMBinaryFormat;LLVMRemarks;LLVMSupport"
)

At the moment LLVM/Clang has no dependencies in the CMake exports when BUILD_SHARED_LIBS is set to ON.

While building Qt Creator we had to add all the little libraries that libclang or libTooling depends because on Linux LLVM is being built with BUILD_SHARED_LIBS=ON :(

[beanz]

This is definitely not the right fix. We don’t want to set everything as Public dependencies. Our build system’s model for dependencies is that each component specified what it directly uses which is its Private dependencies. If you are having issues that probably means missing dependencies.

[cristian.adam]

In D63881#1571286, @beanz wrote:

This is definitely not the right fix. We don’t want to set everything as Public dependencies. Our build system’s model for dependencies is that each component specified what it directly uses which is its Private dependencies. If you are having issues that probably means missing dependencies.

Well, at the moment the CMake build system is broken, llvm/unittests/Passes won't build when BUILD_SHARED_LIBS is set to ON.

The projects that consume the LLVM/Clang CMake exports need to explicitly link to all dependencies when Clang/LLVM is build with BUILD_SHARED_LIBS set to ON.

Qt Creator for example will not have the convenience of just saying target_link_libraries(qtcreator PRIVATE libclang), which works fine with a static build of Clang/LLVM. Qt Creator will have to explicitly name the targets, in the worst case like target_link_libraries(qtcreator PRIVATE libclang clangAST clangBasic clangFrontend clangIndex clangLex clangSema ...).

This is at least on Windows, where you need to use the DLL import libraries. On Linux you'll have problems because when I say "libclang" it will use the imported target "libclang" and not "-lclang", which might get the needed dependencies.

[beanz]

Reading through the old comments on this thread I think there are a few things going on here, but the core of the issue is you're using BUILD_SHARED_LIBS *outside* LLVM's build system. That is explicitly not supported (https://www.llvm.org/docs/CMake.html). We do not support BUILD_SHARED_LIBS for distributing LLVM in any form.

In D63881#1561325, @cristian.adam wrote:

TestPlugin depends on Core. Core depends on BinaryFormat, Remarks, and Support. If we keep PRIVATE then TestPlugin will also need the dependencies from Core.

For legacy reasons, we actually have a different system for how we manage transitive library dependencies for the shared library build (llvm-config & LLVMBuild). Making this change would result in over-specifying the dependency graph inside the LLVM build tree, which could have adverse impact on our development build times, and the BUILD_SHARED_LIBS option exists solely to speed up slow builds. Seems counter-productive.

At the moment LLVM/Clang has no dependencies in the CMake exports when BUILD_SHARED_LIBS is set to ON.

While building Qt Creator we had to add all the little libraries that libclang or libTooling depends because on Linux LLVM is being built with BUILD_SHARED_LIBS=ON :(

If you insist on building this way, which you shouldn't, you will need to use llvm-config to get the transitive library dependencies setup correctly, the CMake exports will not work.

Additionally the change you are making to TestPlugin will almost certainly break some of our more common configurations (BUILD_SHARED_LIBS=Off comes to mind). The problem is that LLVM can't have more than one copy of itself in the same process memory space because it relies on global data in silly ways.

If you build your patch with BUILD_SHARED_LIBS=Off (the default configuration), this should result in LLVMSupport being linked into the TestPlugin library as well as into the PluginTests tool that eventually loads it. When the test runs and it loads the TestPlugin... kaboom (llvm should hit an assert).

We should add PLUGIN_TOOL PluginTest to the add_llvm_library call for TestPlugin, which will tell TestPlugin to link against PluginTest's LLVM. That will fix some build configurations (and not break any others), but may not resolve this particular issue.

[cristian.adam]

BUILD_SHARED_LIBS does create lots of shared objects, and LLVM_BUILD_LLVM_DYLIB should be the right solution for Qt Creator where we have a bit of big binaries.

[mati865]

MSYS2 provides LLVM built with LLVM_BUILD_LLVM_DYLIB=TRUE and hits the same issue. So far it was using this hack: https://github.com/msys2/MINGW-packages/blob/f31bc199ade4d1025cda9bb5aed4427f80d3c584/mingw-w64-clang/0012-fix-testplugin-linking.patch
Applying set(LLVM_LINK_COMPONENTS Core) from this diff also fixes the build.

What would be the proper solution for it?

llvm/cmake/modules/AddLLVM.cmake

function(llvm_add_library name)
   endif()
 
   if(ARG_MODULE OR ARG_SHARED)
     # Do not add -Dname_EXPORTS to the command-line when building files in this
     # target. Doing so is actively harmful for the modules build because it
     # creates extra module variants, and not useful because we don't use these
     # macros.
     set_target_properties( ${name} PROPERTIES DEFINE_SYMBOL "" )
-
-    if (LLVM_EXPORTED_SYMBOL_FILE)
+
+    if (LLVM_EXPORTED_SYMBOL_FILE)
       add_llvm_symbol_exports( ${name} ${LLVM_EXPORTED_SYMBOL_FILE} )
     endif()
   endif()
 
   if(ARG_SHARED AND UNIX)
     if(NOT APPLE AND ARG_SONAME)
       get_target_property(output_name ${name} OUTPUT_NAME)
       if(${output_name} STREQUAL "output_name-NOTFOUND")
else()
     # 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(ARG_STATIC)
     set(libtype INTERFACE)
   else()
-    # We can use PRIVATE since SO knows its dependent libs.
-    set(libtype PRIVATE)
+    # Use public to have the dependencies in the exported targets
+    set(libtype PUBLIC)
   endif()
 
   target_link_libraries(${name} ${libtype}
       ${ARG_LINK_LIBS}
       ${lib_deps}
       ${llvm_libs}
       )
 

llvm/unittests/Passes/CMakeLists.txt

 # Needed by LLVM's CMake checks because this file defines multiple targets.
 set(LLVM_OPTIONAL_SOURCES PluginsTest.cpp TestPlugin.cpp)
 
 # If plugins are disabled, this test will disable itself at runtime. Otherwise,
 # reconfiguring with plugins disabled will leave behind a stale executable.
 if (LLVM_ENABLE_PLUGINS)
   add_definitions(-DLLVM_ENABLE_PLUGINS)
 endif()
 
 set(LLVM_LINK_COMPONENTS Support Passes Core)
 add_llvm_unittest(PluginsTests
   PluginsTest.cpp
   )
-export_executable_symbols(PluginsTests)
+export_executable_symbols(PluginsTests)
 target_link_libraries(PluginsTests PRIVATE LLVMTestingSupport)
 
-set(LLVM_LINK_COMPONENTS)
+set(LLVM_LINK_COMPONENTS Core)
 add_llvm_library(TestPlugin MODULE BUILDTREE_ONLY
   TestPlugin.cpp
   )
 
 # Put plugin next to the unit test executable.
 set_output_directory(TestPlugin
   BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
   LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
   )
 set_target_properties(TestPlugin PROPERTIES FOLDER "Tests")
 
 add_dependencies(TestPlugin intrinsics_gen)
 add_dependencies(PluginsTests TestPlugin)