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

[zhuhan0]

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 only	libclang
STATIC only	libclang
SHARED and STATIC	libclang 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_STATIC	LLVM_ENABLE_PIC	Libraries built and installed
OFF	ON	libclang.so
OFF	OFF	libclang.a
ON	ON	libclang.a and libclang.so
ON	OFF	libclang.a

[zhuhan0]

Spacing.

[zhuhan0]

Rebase

[smeenai]

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.

[zhuhan0]

Address comment

[smeenai]

Do you need me to commit this for you?

[zhuhan0]

@smeenai Yes please. Thanks!

[smeenai]

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

clang/cmake/modules/AddClang.cmake

 endmacro()
 
 macro(add_clang_subdirectory name)
   add_llvm_subdirectory(CLANG TOOL ${name})
 endmacro()
 
 macro(add_clang_library name)
   cmake_parse_arguments(ARG
-    "SHARED;INSTALL_WITH_TOOLCHAIN"
+    "SHARED;STATIC;INSTALL_WITH_TOOLCHAIN"
     ""
     "ADDITIONAL_HEADERS"
     ${ARGN})
   set(srcs)
   if(MSVC_IDE OR XCODE)
     # Add public headers
     file(RELATIVE_PATH lib_path
       ${CLANG_SOURCE_DIR}/lib/
macro(add_clang_library name)
   endif(MSVC_IDE OR XCODE)
   if(srcs OR ARG_ADDITIONAL_HEADERS)
     set(srcs
       ADDITIONAL_HEADERS
       ${srcs}
       ${ARG_ADDITIONAL_HEADERS} # It may contain unparsed unknown args.
       )
   endif()
-  if(ARG_SHARED)
+
+  if(ARG_SHARED AND ARG_STATIC)
+    set(LIBTYPE SHARED STATIC)
+  elseif(ARG_SHARED)
     set(LIBTYPE SHARED)
   else()
     # llvm_add_library ignores BUILD_SHARED_LIBS if STATIC is explicitly set,
     # so we need to handle it here.
     if(BUILD_SHARED_LIBS)
       set(LIBTYPE SHARED)
     else()
       set(LIBTYPE STATIC)
     endif()
     if(NOT XCODE)
       # The Xcode generator doesn't handle object libraries correctly.
       list(APPEND LIBTYPE OBJECT)
     endif()
     set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
   llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
-  if(TARGET ${name})
-    target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})
+  set(libs ${name})
+  if(ARG_SHARED AND ARG_STATIC)
+    list(APPEND libs ${name}_static)
+  endif()
+
+  foreach(lib ${libs})
+    if(TARGET ${lib})
+      target_link_libraries(${lib} INTERFACE ${LLVM_COMMON_LIBS})
 
       if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY OR ARG_INSTALL_WITH_TOOLCHAIN)
         set(export_to_clangtargets)
-      if(${name} IN_LIST LLVM_DISTRIBUTION_COMPONENTS OR
+        if(${lib} IN_LIST LLVM_DISTRIBUTION_COMPONENTS OR
             "clang-libraries" IN_LIST LLVM_DISTRIBUTION_COMPONENTS OR
             NOT LLVM_DISTRIBUTION_COMPONENTS)
           set(export_to_clangtargets EXPORT ClangTargets)
           set_property(GLOBAL PROPERTY CLANG_HAS_EXPORTS True)
         endif()
 
-      install(TARGETS ${name}
-        COMPONENT ${name}
+        install(TARGETS ${lib}
+          COMPONENT ${lib}
           ${export_to_clangtargets}
           LIBRARY DESTINATION lib${LLVM_LIBDIR_SUFFIX}
           ARCHIVE DESTINATION lib${LLVM_LIBDIR_SUFFIX}
           RUNTIME DESTINATION bin)
 
         if (NOT LLVM_ENABLE_IDE)
-        add_llvm_install_targets(install-${name}
-                                 DEPENDS ${name}
-                                 COMPONENT ${name})
+          add_llvm_install_targets(install-${lib}
+                                   DEPENDS ${lib}
+                                   COMPONENT ${lib})
         endif()
 
-      set_property(GLOBAL APPEND PROPERTY CLANG_LIBS ${name})
+        set_property(GLOBAL APPEND PROPERTY CLANG_LIBS ${lib})
       endif()
-    set_property(GLOBAL APPEND PROPERTY CLANG_EXPORTS ${name})
+      set_property(GLOBAL APPEND PROPERTY CLANG_EXPORTS ${lib})
     else()
       # Add empty "phony" target
-    add_custom_target(${name})
+      add_custom_target(${lib})
     endif()
+  endforeach()
 
   set_target_properties(${name} PROPERTIES FOLDER "Clang libraries")
   set_clang_windows_version_resource_properties(${name})
 endmacro(add_clang_library)
 
 macro(add_clang_executable name)
   add_llvm_executable( ${name} ${ARGN} )
   set_target_properties(${name} PROPERTIES FOLDER "Clang executables")
   set_clang_windows_version_resource_properties(${name})