Page MenuHomePhabricator

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

Authored by aaronpuchert on Sep 7 2019, 10:08 AM.



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.

Event Timeline

aaronpuchert created this revision.Sep 7 2019, 10:08 AM
aaronpuchert added inline comments.Sep 7 2019, 10:11 AM

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

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)
 function(clang_target_link_libraries target type)
+  get_target_property(TARGET_TYPE ${target} TYPE)
     target_link_libraries(${target} ${type} clang-cpp)
+    target_link_libraries(${target} INTERFACE ${ARGN})
     target_link_libraries(${target} ${type} ${ARGN})
tstellar added inline comments.Sep 9 2019, 7:00 AM

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

beanz requested changes to this revision.Sep 9 2019, 12:32 PM
beanz added inline comments.

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.

This revision now requires changes to proceed.Sep 9 2019, 12:32 PM
aaronpuchert added inline comments.Sep 9 2019, 5:25 PM

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):

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

target_link_libraries(${name} ${libtype}

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 added a comment.Sep 9 2019, 5:45 PM

@aaronpuchert sounds like a reasonable approach

aaronpuchert marked 4 inline comments as done.

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

@beanz Could you have a look again?