This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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.

Event Timeline

aaronpuchert created this revision.Sep 7 2019, 10:08 AM
aaronpuchert added inline comments.Sep 7 2019, 10:11 AM
clang/tools/libclang/CMakeLists.txt
115

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)
 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 added inline comments.Sep 9 2019, 7:00 AM
clang/tools/libclang/CMakeLists.txt
115

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.
clang/tools/libclang/CMakeLists.txt
115

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
clang/tools/libclang/CMakeLists.txt
115

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 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 build.ninja.

@beanz Could you have a look again?

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.

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.

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 ↗(On Diff #222303)

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

jkorous resigned from this revision.Jan 21 2022, 3:56 PM
smeenai resigned from this revision.Apr 28 2022, 2:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 2:06 PM