Respect CLANG_LINK_CLANG_DYLIB=ON in libclang and c-index-test

[aaronpuchert]

Summary

Without these changes an application can't link both libclang and
clang-shlib ("clang-cpp") at the same time, because otherwise some flags
are registered twice. This was observable in c-index-test with the flag
"-limited-coverage-experimental" from lib/CodeGen/CodeGenModule.cpp.

This also further reduces the installation size of Clang.

[aaronpuchert]

clang/tools/libclang/CMakeLists.txt

116

This might not be correct for static builds, I think we need INTERFACE here.

[aaronpuchert]

I'm thinking about the following, but I'm not sure if that's the proper way to do it.

--- a/clang/cmake/modules/AddClang.cmake
+++ b/clang/cmake/modules/AddClang.cmake
@@ -174,8 +174,11 @@ macro(add_clang_symlink name dest)
 endmacro()
 
 function(clang_target_link_libraries target type)
+  get_target_property(TARGET_TYPE ${target} TYPE)
   if (CLANG_LINK_CLANG_DYLIB)
     target_link_libraries(${target} ${type} clang-cpp)
+  elseif(${TARGET_TYPE} EQUAL "STATIC_LIBRARY")
+    target_link_libraries(${target} INTERFACE ${ARGN})
   else()
     target_link_libraries(${target} ${type} ${ARGN})
   endif()

[tstellar]

clang/tools/libclang/CMakeLists.txt

116

This patch looks OK to me, but you should find someone with more CMake knowledge to answer this question.

[beanz]

clang/tools/libclang/CMakeLists.txt

116

This part of the patch is a bit tricky.

As implemented it is fine for the most common build configurations, but will be a problem if LIBCLANG_BUILD_STATIC=On or if LLVM_ENABLE_PIC=Off.

The correct solution is probably to wrap this code in if (ENABLE_SHARED), and to have another code block that handles if (ENABLE_STATIC). In that block you need to call this with INTERFACE as the linkage type, and you'll need to handle the case where both ENABLE_SHARED and ENABLE_STATIC is set. In that case the static library target is named libclang_static.

[aaronpuchert]

clang/tools/libclang/CMakeLists.txt

116

I agree with your analysis. What do you think about modifying clang_target_link_libraries instead? I thought we could make it do what llvm_add_library does with its LINK_LIBS argument (code which we are basically replacing here):

if(ARG_STATIC)
  set(libtype INTERFACE)
else()
  # We can use PRIVATE since SO knows its dependent libs.
  set(libtype PRIVATE)
endif()

target_link_libraries(${name} ${libtype}
    ${ARG_LINK_LIBS}
    ${lib_deps}
    ${llvm_libs}
    )

We could query get_target_property(TARGET_TYPE ${target} TYPE) and use that to determine the correct dependency type.

You're also right that it's possible to build both static and dynamic libraries. We could check for the existence of ${target}_static and add the dependencies there as well.

If we handle it there, we'll also solve it for other libraries that depend on Clang components. (I'm thinking of libraries in clang-tools-extra and lldb, where I'd like to propose similar changes to this one.)

[beanz]

@aaronpuchert sounds like a reasonable approach

[aaronpuchert]

Handle static libraries correctly.

Dependencies of executables are obviously always PRIVATE, but for library targets we have to turn PRIVATE or PUBLIC dependencies into INTERFACE dependencies. This is basically also what add_llvm_library's LINK_LIBS does.

I tested this by configuring a couple of build profiles and inspecting build.ninja.

[aaronpuchert]

Fix typo.

[aaronpuchert]

@beanz Could you have a look again?

[mgorny]

Ping. The current state is that c-index-test crashes for us, and I'd like to see it fixed for 10.0.1. I have a patch of my own but it doesn't handle all that _static magic, so I guess this one's better.

[aaronpuchert]

Yup, c-index-test crashing was one of the motivators behind this.

I think this should handle all cases. I tried it with CLANG_LINK_CLANG_DYLIB on/off combined with shared/static/shared+static libraries and inspected the generated build.ninja, which looked about right in all cases. (I didn't take the time to run those 6 builds though.)

Since we're just imitating llvm_add_library I think it's even theoretically right.

[smeenai]

Super late to the party here, sorry.

I don't understand why the type of linking needs to be changed depending on if the library is static or shared. If a static library A depends on a static library B as PRIVATE, and then executable C links to A, CMake will make sure the link line for A also includes C; you don't need INTERFACE or PUBLIC for that. https://cmake.org/pipermail/cmake/2016-May/063400.html has a good explanation of what the different link types mean for static libraries.

clang/cmake/modules/AddClang.cmake

182

Note that D74106 changes the corresponding logic in llvm_add_library to PUBLIC for this case.

clang/cmake/modules/AddClang.cmake

 endmacro()
 
 macro(add_clang_symlink name dest)
   add_llvm_tool_symlink(${name} ${dest} ALWAYS_GENERATE)
   # Always generate install targets
   llvm_install_symlink(${name} ${dest} ALWAYS_GENERATE)
 endmacro()
 
+# Link with a Clang component library, or clang-cpp if CLANG_LINK_CLANG_DYLIB
+# is set. If target is a static library, PUBLIC and PRIVATE dependencies are
+# treated as INTERFACE dependencies, so this can replace LINK_LIBS.
 function(clang_target_link_libraries target type)
+  get_target_property(TARGET_TYPE ${target} TYPE)
+  if(${TARGET_TYPE} EQUAL "STATIC_LIBRARY")
+    set(type INTERFACE)
+  endif()
   if (CLANG_LINK_CLANG_DYLIB)
     target_link_libraries(${target} ${type} clang-cpp)
   else()
     target_link_libraries(${target} ${type} ${ARGN})
   endif()
 
+  if (TARGET "${target}_static")
+    if (CLANG_LINK_CLANG_DYLIB)
+      target_link_libraries("${target}_static" INTERFACE clang-cpp)
+    else()
+      target_link_libraries("${target}_static" INTERFACE ${ARGN})
+    endif()
+  endif()
 endfunction()

clang/tools/c-index-test/CMakeLists.txt

target_link_libraries(c-index-test
     libclang_static
     clangCodeGen
     clangIndex
   )
 else()
   target_link_libraries(c-index-test
     PRIVATE
     libclang
+  )
+  clang_target_link_libraries(c-index-test
+    PRIVATE
     clangAST
     clangBasic
     clangCodeGen
     clangFrontend
     clangIndex
     clangSerialization
   )
 endif()

clang/tools/libclang/CMakeLists.txt

set(SOURCES
   CXSourceLocation.h
   CXString.h
   CXTranslationUnit.h
   CXType.h
   Index_Internal.h
   ../../include/clang-c/Index.h
   )
 
-set(LIBS
+set(CLANG_LIB_DEPS
   clangAST
   clangBasic
   clangDriver
   clangFrontend
   clangIndex
   clangLex
   clangSema
   clangSerialization
   clangTooling
-  LLVMSupport
 )
 
 if (CLANG_ENABLE_ARCMT)
-  list(APPEND LIBS clangARCMigrate)
+  list(APPEND CLANG_LIB_DEPS clangARCMigrate)
 endif ()
 
 if (TARGET clangTidyPlugin)
   add_definitions(-DCLANG_TOOL_EXTRA_BUILD)
   list(APPEND LIBS clangTidyPlugin)
   list(APPEND LIBS clangIncludeFixerPlugin)
   if(LLVM_ENABLE_MODULES)
     list(APPEND LLVM_COMPILE_FLAGS "-fmodules-ignore-macro=CLANG_TOOL_EXTRA_BUILD")
add_clang_library(libclang ${ENABLE_SHARED} ${ENABLE_STATIC} INSTALL_WITH_TOOLCHAIN
   ${LIBS}
 
   LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   Core
   Support
   )
 
+clang_target_link_libraries(libclang
+  PRIVATE
+  ${CLANG_LIB_DEPS}
+  )
+
 if(ENABLE_SHARED)
   if(WIN32)
     set_target_properties(libclang
       PROPERTIES
       VERSION ${LIBCLANG_LIBRARY_VERSION}
       DEFINE_SYMBOL _CINDEX_LIB_)
   elseif(APPLE)
     set(LIBCLANG_LINK_FLAGS " -Wl,-compatibility_version -Wl,1")