Page MenuHomePhabricator

[cmake] Fix build with BUILD_SHARED_LIBS=ON
AbandonedPublic

Authored by cristian.adam on Jun 27 2019, 8:29 AM.

Details

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.

Diff Detail

Event Timeline

cristian.adam created this revision.Jun 27 2019, 8:29 AM

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 added a comment.EditedJun 27 2019, 1:33 PM

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 added a comment.Jul 5 2019, 4:39 AM

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.

beanz requested changes to this revision.Jul 5 2019, 4:43 AM
This revision now requires changes to proceed.Jul 5 2019, 4:43 AM

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 added a comment.Jul 5 2019, 5:19 AM

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.

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 abandoned this revision.Jul 5 2019, 5:26 AM

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.

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?