[clang-shlib] Fix clang-shlib for PRIVATE dependencies

[smeenai]

Summary

Any static library with a PRIVATE dependency ends up with a
$<LINK_ONLY:...> generator expression in its INTERFACE_LINK_LIBRARIES,
which won't be evaluated by the $<TARGET_PROPERTY:...>, so we end up
with an unevaluated generator expression in the generated build file and
Ninja chokes on the dollar sign. Just use the static library directly
for its dependencies instead of trying to propagate dependencies
manually.

[smeenai]

@beanz, what do you think of this? I think it's kinda awful, but I can't think of a better alternative short of upgrading to CMake 3.12 (as detailed in my comment).

[phosek]

While upgrading to newer CMake would be nice, I think it's unlikely that we could move all the way to 3.12 since that version was only released a year ago and still isn't available in most distributions, so this LGTM. Maybe leave a TODO that this should be revisited after we bump minimum CMake requirement to 3.12 or newer?

[smeenai]

In D64579#1581172, @phosek wrote:

While upgrading to newer CMake would be nice, I think it's unlikely that we could move all the way to 3.12 since that version was only released a year ago and still isn't available in most distributions, so this LGTM. Maybe leave a TODO that this should be revisited after we bump minimum CMake requirement to 3.12 or newer?

FWIW, CMake 3.4.3 was released Jan 25 2016 (https://cmake.org/pipermail/cmake/2016-January/062616.html) and we started requiring it on May 31 2016 (https://reviews.llvm.org/D20822), so we do have precedence for requiring a pretty recent CMake version. I guess we should figure out what other compelling reasons we'd have for moving to that version though.

[smeenai]

In D64579#1581194, @smeenai wrote:

In D64579#1581172, @phosek wrote:

While upgrading to newer CMake would be nice, I think it's unlikely that we could move all the way to 3.12 since that version was only released a year ago and still isn't available in most distributions, so this LGTM. Maybe leave a TODO that this should be revisited after we bump minimum CMake requirement to 3.12 or newer?

FWIW, CMake 3.4.3 was released Jan 25 2016 (https://cmake.org/pipermail/cmake/2016-January/062616.html) and we started requiring it on May 31 2016 (https://reviews.llvm.org/D20822), so we do have precedence for requiring a pretty recent CMake version. I guess we should figure out what other compelling reasons we'd have for moving to that version though.

I'll wait a bit in case @beanz has any thoughts and then commit this with the added TODO.

[beanz]

Yea, this makes sense even if it is a bit sad. We do use the --whole-archive approach for libLLVM, and it works. I was hoping to avoid it here in part to free up the ability to link libclang_shared before or in parallel to archiving the libclang*.a archives.

[smeenai]

In D64579#1581557, @beanz wrote:

Yea, this makes sense even if it is a bit sad. We do use the --whole-archive approach for libLLVM, and it works. I was hoping to avoid it here in part to free up the ability to link libclang_shared before or in parallel to archiving the libclang*.a archives.

Yeah, makes sense. I've had issues with static libraries getting reordered out of the --whole-archive with other projects in the past, which makes me wary of it.

[brzycki]

Hello @smeenai , this commit causes LLVM to fail to build when set set -D BUILD_SHARED_LIBS=ON. Here is the failing CMake and Ninja invocation:

/tools/build/cmake-3.14.5/bin/cmake \
    -G Ninja  \
    -D CMAKE_MAKE_PROGRAM=/tools/build/ninja-1.9.0/ninja \
    -D CMAKE_C_COMPILER=/usr/lib/llvm-7/bin/clang \
    -D CMAKE_CXX_COMPILER=/usr/lib/llvm-7/bin/clang++ \
    -D CMAKE_BUILD_TYPE=Release \
    -D CMAKE_INSTALL_PREFIX=/work/upstream/install \
    -D LLVM_TOOL_CLANG_BUILD=ON \
    -D LIBCXX_INCLUDE_BENCHMARKS=ON \
    -D BUILD_SHARED_LIBS=ON \
    -D LLVM_ENABLE_ASSERTIONS=ON \
    -D LLVM_TARGETS_TO_BUILD=X86 \
    /work/upstream/llvm-project/llvm

/tools/build/ninja-1.9.0/ninja -v install

I am able to successfully build LLVM when I change to -D BUILD_SHARED_LIBS=OFF.

The outputted errors happen during link time of lib/libclang_shared.so.9svn:

FAILED: lib/libclang_shared.so.9svn

...

tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/CodeGenModule.cpp.o: In function `llvm::cl::opt<bool, false, llvm::cl::parser<bool> >::~opt()':
CodeGenModule.cpp:(.text._ZN4llvm2cl3optIbLb0ENS0_6parserIbEEED2Ev[_ZN4llvm2cl3optIbLb0ENS0_6parserIbEEED2Ev]+0x7): undefined reference to `vtable for llvm::cl::Option'
tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/CodeGenModule.cpp.o: In function `clang::CodeGen::CodeGenModule::CodeGenModule(clang::ASTContext&, clang::HeaderSearchOptions const&, clang::PreprocessorOptions const&, clang::CodeGenOptions const&, llvm::Module&, clang::DiagnosticsEngine&, clang::CoverageSourceInfo*)':
CodeGenModule.cpp:(.text._ZN5clang7CodeGen13CodeGenModuleC2ERNS_10ASTContextERKNS_19HeaderSearchOptionsERKNS_19PreprocessorOptionsERKNS_14CodeGenOptionsERN4llvm6ModuleERNS_17DiagnosticsEngineEPNS_18CoverageSourceInfoE+0x57a): undefined reference to `llvm::FoldingSetBase::FoldingSetBase(unsigned int)'
CodeGenModule.cpp:(.text._ZN5clang7CodeGen13CodeGenModuleC2ERNS_10ASTContextERKNS_19HeaderSearchOptionsERKNS_19PreprocessorOptionsERKNS_14CodeGenOptionsERN4llvm6ModuleERNS_17DiagnosticsEngineEPNS_18CoverageSourceInfoE+0x5cc): undefined reference to `llvm::Type::getVoidTy(llvm::LLVMContext&)'
CodeGenModule.cpp:(.text._ZN5clang7CodeGen13CodeGenModuleC2ERNS_10ASTContextERKNS_19HeaderSearchOptionsERKNS_19PreprocessorOptionsERKNS_14CodeGenOptionsERN4llvm6ModuleERNS_17DiagnosticsEngineEPNS_18CoverageSourceInfoE+0x5d7): undefined reference to `llvm::Type::getInt8Ty(llvm::LLVMContext&)'
CodeGenModule.cpp:(.text._ZN5clang7CodeGen13CodeGenModuleC2ERNS_10ASTContextERKNS_19HeaderSearchOptionsERKNS_19PreprocessorOptionsERKNS_14CodeGenOptionsERN4llvm6ModuleERNS_17DiagnosticsEngineEPNS_18CoverageSourceInfoE+0x5e3): undefined reference to `llvm::Type::getInt16Ty(llvm::LLVMContext&)'

... (clipped thousands of similar errors)

WhitespaceManager.cpp:(.text._ZN5clang6formatL18AlignTokenSequenceIRZNS0_17WhitespaceManager28alignConsecutiveDeclarationsEvE3$_2EEvjjjOT_RN4llvm11SmallVectorINS2_6ChangeELj16EEE+0x129): undefined reference to `llvm::SmallVectorBase::grow_pod(void*, unsigned long, unsigned long)'
tools/clang/tools/clang-fuzzer/handle-cxx/CMakeFiles/obj.clangHandleCXX.dir/handle_cxx.cpp.o: In function `clang_fuzzer::HandleCXX(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&)':
handle_cxx.cpp:(.text._ZN12clang_fuzzer9HandleCXXERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKSt6vectorIPKcSaISA_EE+0x67): undefined reference to `llvm::SmallVectorBase::grow_pod(void*, unsigned long, unsigned long)'
handle_cxx.cpp:(.text._ZN12clang_fuzzer9HandleCXXERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKSt6vectorIPKcSaISA_EE+0x32b): undefined reference to `llvm::MemoryBuffer::getMemBuffer(llvm::StringRef, llvm::StringRef, bool)'
handle_cxx.cpp:(.text._ZN12clang_fuzzer9HandleCXXERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKSt6vectorIPKcSaISA_EE+0x669): undefined reference to `llvm::SmallVectorBase::grow_pod(void*, unsigned long, unsigned long)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

clang/tools/clang-shlib/CMakeLists.txt

 # Building libclang_shared.so fails if LLVM_ENABLE_PIC=Off
 if (NOT LLVM_ENABLE_PIC)
   return()
 endif()
 
 get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
 
 foreach (lib ${clang_libs})
   list(APPEND _OBJECTS $<TARGET_OBJECTS:obj.${lib}>)
-  list(APPEND _DEPS $<TARGET_PROPERTY:${lib},INTERFACE_LINK_LIBRARIES>)
-  list(APPEND _DEPS $<TARGET_PROPERTY:${lib},LINK_LIBRARIES>)
+  # Use the static library for its dependencies. The objects that constitute the
+  # static library will appear on the link line before the library, so it'll
+  # just be ignored, but the dependencies of the library will still be linked
+  # correctly.
+  #
+  # We could propagate the dependencies manually using the library's
+  # INTERFACE_LINK_LIBRARIES property, but that will contain $<LINK_ONLY:...>
+  # generator expressions if a static library has a private dependency, so we
+  # can't use a $<TARGET_PROPERTY:...> generator expression to get the property
+  # (since it wouldn't evaluate any generator expressions inside the property).
+  # We could use get_property and do string manipulation to manually evaluate
+  # the $<LINK_ONLY:...>, but that would miss any dependencies added after we
+  # evaluate the get_property. We could also use the LINK_LIBRARIES property
+  # instead, which should be free of any generator expressions, but that's
+  # technically incorrect (it'd most likely work fine in practice, but still).
+  #
+  # Another alternative would be to use --whole-archive or equivalent instead of
+  # using the object library at all. However, CMake reorders static libraries on
+  # the link line so that a library appears after all its dependents, which can
+  # reorder static libraries out of their --whole-archive --no-whole-archive
+  # sandwich. It's really hard to avoid that reordering while still propagating
+  # dependencies, which defeats the whole point.
+  #
+  # The ideal solution here is to bump our minimum CMake requirement to 3.12,
+  # which adds support for dependencies on object libraries. Until then, linking
+  # both the object and the static libraries seems like the least bad solution.
+  list(APPEND _DEPS ${lib})
 endforeach ()
 
 add_clang_library(clang_shared
                   SHARED
                   clang-shlib.cpp
                   ${_OBJECTS}
                   LINK_LIBS
                   ${_DEPS})