Page MenuHomePhabricator

Fix and re-submit D78534 - [libclang] Install both libclang.a and libclang.so when LIBCLANG_BUILD_STATIC=ON
ClosedPublic

Authored by zhuhan0 on Apr 28 2020, 6:33 PM.

Details

Summary

D78354 enabled installing PIC version of both libclang.a and libclang.so when LIBCLANG_BUILD_STATIC is ON. But it broke the no-PIC build when LLVM_ENABLE_PIC=OFF with the following error:

CMake Error at                                                             
/b/s/w/ir/cache/builder/src/third_party/llvm/clang/tools/libclang/CMakeLists.txt:123
(target_compile_definitions):                                              
    target_compile_definitions called with non-compilable target type

This is because as the code loops through ${name} and ${name}_static, it introduced a side effect, which is adding an empty libclang_static to targets. Later target_compile_definitions is called on libclang_static. That function requires that target must have been created by a command such as add_executable() or add_library(), so it crashed.

The solution is to not naively loop through both libclang and libclang_static, but only the ones that are actually added by llvm_add_library(). Here's the library build type to library target name mapping:

SHARED onlylibclang
STATIC onlylibclang
SHARED and STATIClibclang and libclang_static

So only when SHARED and STATIC are both set should we loop through two targets. Explicitly parse the STATIC argument and set the list accordingly.

Test plan:
Try all combinations of LIBCLANG_BUILD_STATIC and LLVM_ENABLE_PIC and make sure results are expected:

LIBCLANG_BUILD_STATICLLVM_ENABLE_PICLibraries built and installed
OFFONlibclang.so
OFFOFFlibclang.a
ONONlibclang.a and libclang.so
ONOFFlibclang.a

Diff Detail

Event Timeline

zhuhan0 created this revision.Apr 28 2020, 6:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 6:33 PM
zhuhan0 edited the summary of this revision. (Show Details)Apr 28 2020, 6:50 PM
zhuhan0 edited the summary of this revision. (Show Details)Apr 28 2020, 6:58 PM
zhuhan0 edited the summary of this revision. (Show Details)
zhuhan0 edited the summary of this revision. (Show Details)
zhuhan0 updated this revision to Diff 260818.Apr 28 2020, 7:03 PM
zhuhan0 edited the summary of this revision. (Show Details)

Spacing.

zhuhan0 edited the summary of this revision. (Show Details)Apr 28 2020, 7:06 PM
smith added a subscriber: smith.May 1 2020, 3:33 AM
smeenai accepted this revision.Tue, May 5, 10:14 PM

Sorry for the delayed review here.

I'm not the biggest fan of this cos it's encoding knowledge of llvm_add_library's internal behavior (that it creates a *_static target if both STATIC and SHARED are passed) inside add_clang_library, so that if someone changes llvm_add_library, they might break add_clang_library without realizing it. However, it looks like a bunch of other places in the build also assume the _static suffix, so we already have that problem today, and this is fine.

clang/cmake/modules/AddClang.cmake
144

Super nit: you can do list(APPEND libs ${name}_static) instead.

This revision is now accepted and ready to land.Tue, May 5, 10:14 PM
zhuhan0 updated this revision to Diff 262506.Wed, May 6, 5:09 PM

Address comment

zhuhan0 marked an inline comment as done.Wed, May 6, 5:10 PM

Do you need me to commit this for you?

@smeenai Yes please. Thanks!

Committed. Please let me know of any buildbot failure emails you receive.

This revision was automatically updated to reflect the committed changes.