[CMake] Fix `is_llvm_target_library` and support out-of-order components

[beanz]

Summary

This patch is required by D28855, and enables us to rely on CMake's ability to handle out of order target dependencies.

[bryant]

Just a quick reminder.

cmake/modules/LLVM-Config.cmake

210 ↗

(On Diff #84877)

list(APPEND expanded_components "$<LINK_ONLY:LLVM${c}>")

[bryant]

cmake/modules/LLVM-Config.cmake

33 ↗

(On Diff #84877)

(See the inline comment after this one for justication.)

@@ -32,6 +32,28 @@ function(is_llvm_target_library library return_var)
   endforeach()
 endfunction(is_llvm_target_library)

+function(is_omitted_target_lib library return_var)
+  set(${return_var} OFF PARENT_SCOPE)
+  string(TOUPPER "${library}" capitalized_lib)
+  set(omitted_targets ${LLVM_ALL_TARGETS})
+  list(REMOVE_ITEM omitted_targets ${LLVM_TARGETS_TO_BUILD})
+  string(TOUPPER "${omitted_targets}" targets)
+  foreach(t ${targets})
+    if( capitalized_lib STREQUAL t OR
+        capitalized_lib STREQUAL "${t}" OR
+        capitalized_lib STREQUAL "${t}DESC" OR
+        capitalized_lib STREQUAL "${t}CODEGEN" OR
+        capitalized_lib STREQUAL "${t}ASMPARSER" OR
+        capitalized_lib STREQUAL "${t}ASMPRINTER" OR
+        capitalized_lib STREQUAL "${t}DISASSEMBLER" OR
+        capitalized_lib STREQUAL "${t}INFO" OR
+        capitalized_lib STREQUAL "${t}UTILS" OR
+        capitalized_lib STREQUAL "${t}INSTPRINTER" )
+      set(${return_var} ON PARENT_SCOPE)
+      break()
+    endif()
+  endforeach()
+endfunction(is_omitted_target_lib)

 macro(llvm_config executable)
   cmake_parse_arguments(ARG "USE_SHARED" "" "" ${ARGN})

211 ↗

(On Diff #84877)

I might be reading this wrong, but the logic here seems to be:

• Check if $c is a target lib (i.e., LLVMAArch64Utils) regardless of whether its corresponding target will be built;
  • If it is, then ignore it.
  • Otherwise, assume that it's a yet-to-be-scanned library.

This seems a bit off, since I recall you mentioning that "adding target libraries that are configured out is a bad idea." I think what we need is:

• Check if $c is a target lib that will not be built.
  • If it is, ignore it.
  • Otherwise, it's either a yet-to-be-scanned {normal,target} lib.

@@ -201,12 +223,12 @@ function(llvm_map_components_to_libnames out_libs)
       list(FIND capitalized_libs LLVM${capitalized} lib_idx)
       if( lib_idx LESS 0 )
         # The component is unknown. Maybe is an omitted target?
-        is_llvm_target_library(${c} iltl_result)
-        if( NOT iltl_result )
-          # If it is not an omitted target we should assume it is a component
-          # that hasn't yet been processed by CMake. Missing components will
-          # cause errors later in the configuration, so we can safely assume
-          # that this is valid here.
+        is_omitted_target_lib(${c} iltl_result)    # see above inline comment
+        if(iltl_result)
+          message(FATAL_ERROR "Depended on ${c}, whose target isn't builts.")
+        else()
+          # either target lib or a normal lib that will be built but has yet to
+          # be scanned.
           list(APPEND expanded_components "$<LINK_ONLY:LLVM${c}>")
         endif()
       else( lib_idx LESS 0 )

This seems to generate the right build.ninja modulo reordered dependencies.

[beanz]

Updating patch with a few new things.

• Added an enforcement mechanism to ensure that target libraries aren't specified before all target libraries are created. We have this by convention today, the enforcment just creates an error.
• Also took inspiration from bryant in D28855, to further clean up and improve the logic around is_llvm_target_library.

[bryant]

cmake/modules/LLVM-Config.cmake

12 ↗

(On Diff #85607)

Could there be some documentation for this function? Something like:

# Checks if ${library} is a member component of some target in a list of
# targets. This target list is one of:
#     ALL_TARGETS (the default, if no target list specifier is given);
#     INCLUDED_TARGETS;
#     OMITTED_TARGETS.

Feel free to word-smith that.

121 ↗

(On Diff #85607)

Just a small nit, not mission-critical: Could LLVM_TARGET_REGISTRATION_COMPLETE (both the local var and global prop) be shortened to, say, LLVM_TARGETS_REGISTERED, LLVM_SCANNED_TARGETS, LLVM_GOT_ALL_TARGETS? It's kind of a finger-ful to type.

128 ↗

(On Diff #85607)

If we know that all targets have been scanned, can we bypass this check entirely?

if(NOT LLVM_TARGET_REGIS)  # sorry, that name is just too long
  foreach(c ${link_components})
    is_llvm_target_specifier(..)
      if(iltl_result)
        message(FATAL_ERROR ...)
      endif()
    endforeach()
endif(NOT LLVM_TARGET_REG)

242 ↗

(On Diff #85607)

If we arrive here, it's precisely because one of the dependencies is a sub-component of an omitted target, right? Can you make the error message more specific to reflect this?

248 ↗

(On Diff #85607)

list(APPEND expanded_components "$<LINK_ONLY:LLVM${c}>") ? Then we won't have to re-process the resulting list in the caller.

lib/Target/CMakeLists.txt

25 ↗

(On Diff #85607)

CC from IRC:

9:12:17 < zq> beanz: beanz[m] crazy idea: if we're gonna require that target-depending components be scanned _after_ lib/Targets
19:12:29 < zq> then why even bother with those synonyms?
19:12:52 < zq> beanz: beanz[m] like ALLTARGETS and so forth
19:13:21 < zq> beanz: beanz[m] we could set those as special vars in lib/TArget/CMakeLists.txt right around where LLVM_TARGET_REGISTRATION_COMPLETE is set

[beanz]

cmake/modules/LLVM-Config.cmake

12 ↗

(On Diff #85607)

Will do.

121 ↗

(On Diff #85607)

LLVM_HAZ_ALL_TARGETS has a nice ring to it :-).

You're right that is a finger-full. I'll shorten it.

128 ↗

(On Diff #85607)

Yes, that is a better expression of the logic.

242 ↗

(On Diff #85607)

Will do.

248 ↗

(On Diff #85607)

No, we can't do that here. There are places where this function is called where that generator expression is inappropriate (see: tools/llvm-shlib/CMakeLists.txt).

lib/Target/CMakeLists.txt

25 ↗

(On Diff #85607)

There is value to the synonyms, but I have some ideas for how we can improve them. I think that shouldn't be part of this patch though.

[beanz]

Updates based on review by bryant

[bryant]

cmake/modules/LLVM-Config.cmake

192 ↗

(On Diff #85957)

Were you also planning to add the missing target sub-components in this patch? I think LLVM${c}Utils is missing from here.

[beanz]

Adding Info and Utils handling.

This function needs a massive cleanup, but that should be a separate patch.

[bryant]

LGTM

[pelikan]

FYI this probably broke a clang-9999 build on Gentoo.

• Clang version: 5.0.0
• Performing Test CXX_SUPPORTS_NO_NESTED_ANON_TYPES_FLAG
• Performing Test CXX_SUPPORTS_NO_NESTED_ANON_TYPES_FLAG - Failed

CMake Error at /usr/lib64/cmake/llvm/LLVM-Config.cmake:140 (message):

Specified target library before target registration is complete.

Call Stack (most recent call first):

/usr/lib64/cmake/llvm/LLVM-Config.cmake:100 (llvm_map_components_to_libnames)
/usr/lib64/cmake/llvm/LLVM-Config.cmake:93 (explicit_llvm_config)
/usr/lib64/cmake/llvm/AddLLVM.cmake:713 (llvm_config)
cmake/modules/AddClang.cmake:125 (add_llvm_executable)
examples/clang-interpreter/CMakeLists.txt:11 (add_clang_executable)

The CMake invocation was as follows:

Working in BUILD_DIR: "/var/tmp/portage/sys-devel/clang-9999/work/clang-9999-abi_x86_64.amd64"

cmake -C /var/tmp/portage/sys-devel/clang-9999/work/clang-9999-abi_x86_64.amd64/gentoo_common_config.cmake -G Ninja -DCMAKE_INSTALL_PREFIX=/usr -DLLVM_CONFIG=/usr/bin/x86_64-pc-linux-gnu-llvm-config -DCLANG_RESOURCE_DIR=../lib/clang/5.0.0 -DBUILD_SHARED_LIBS=ON -DLLVM_TARGETS_TO_BUILD=AMDGPU;AArch64;ARM;BPF;Hexagon;Lanai;Mips;MSP430;NVPTX;PowerPC;RISCV;Sparc;SystemZ;X86;XCore -DLLVM_BUILD_TESTS=no -DLLVM_ENABLE_EH=ON -DLLVM_ENABLE_RTTI=ON -DCMAKE_DISABLE_FIND_PACKAGE_LibXml2=yes -DCLANG_DEFAULT_OPENMP_RUNTIME=libomp -DCLANG_DEFAULT_CXX_STDLIB=libc++ -DCLANG_DEFAULT_RTLIB=compiler-rt -DCLANG_ENABLE_ARCMT=yes -DCLANG_ENABLE_STATIC_ANALYZER=yes -DLLVM_BUILD_DOCS=no -DLLVM_ENABLE_SPHINX=no -DLLVM_ENABLE_DOXYGEN=OFF -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_USER_MAKE_RULES_OVERRIDE=/var/tmp/portage/sys-devel/clang-9999/work/clang-9999-abi_x86_64.amd64/gentoo_rules.cmake -DCMAKE_TOOLCHAIN_FILE=/var/tmp/portage/sys-devel/clang-9999/work/clang-9999-abi_x86_64.amd64/gentoo_toolchain.cmake /var/tmp/portage/sys-devel/clang-9999/work/clang-9999

llvm/trunk/cmake/modules/LLVM-Config.cmake

 function(get_system_libs return_var)
   message(AUTHOR_WARNING "get_system_libs no longer needed")
   set(${return_var} "" PARENT_SCOPE)
 endfunction()
 
 
 function(link_system_libs target)
   message(AUTHOR_WARNING "link_system_libs no longer needed")
 endfunction()
 
-
+# is_llvm_target_library(
+#   library
+#     Name of the LLVM library to check
+#   return_var
+#     Output variable name
+#   ALL_TARGETS;INCLUDED_TARGETS;OMITTED_TARGETS
+#     ALL_TARGETS - default looks at the full list of known targets
+#     INCLUDED_TARGETS - looks only at targets being configured
+#     OMITTED_TARGETS - looks only at targets that are not being configured
+# )
 function(is_llvm_target_library library return_var)
+  cmake_parse_arguments(ARG "ALL_TARGETS;INCLUDED_TARGETS;OMITTED_TARGETS" "" "" ${ARGN})
   # Sets variable `return_var' to ON if `library' corresponds to a
   # LLVM supported target. To OFF if it doesn't.
   set(${return_var} OFF PARENT_SCOPE)
   string(TOUPPER "${library}" capitalized_lib)
+  if(ARG_INCLUDED_TARGETS)
+    string(TOUPPER "${LLVM_TARGETS_TO_BUILD}" targets)
+  elseif(ARG_OMITTED_TARGETS)
+    set(omitted_targets ${LLVM_ALL_TARGETS})
+    list(REMOVE_ITEM omitted_targets ${LLVM_TARGETS_TO_BUILD})
+    string(TOUPPER "${omitted_targets}" targets)
+  else()
     string(TOUPPER "${LLVM_ALL_TARGETS}" targets)
+  endif()
   foreach(t ${targets})
     if( capitalized_lib STREQUAL t OR
-        capitalized_lib STREQUAL "LLVM${t}" OR
-        capitalized_lib STREQUAL "LLVM${t}CODEGEN" OR
-        capitalized_lib STREQUAL "LLVM${t}ASMPARSER" OR
-        capitalized_lib STREQUAL "LLVM${t}ASMPRINTER" OR
-        capitalized_lib STREQUAL "LLVM${t}DISASSEMBLER" OR
-        capitalized_lib STREQUAL "LLVM${t}INFO" )
+        capitalized_lib STREQUAL "${t}" OR
+        capitalized_lib STREQUAL "${t}DESC" OR
+        capitalized_lib STREQUAL "${t}CODEGEN" OR
+        capitalized_lib STREQUAL "${t}ASMPARSER" OR
+        capitalized_lib STREQUAL "${t}ASMPRINTER" OR
+        capitalized_lib STREQUAL "${t}DISASSEMBLER" OR
+        capitalized_lib STREQUAL "${t}INFO" OR
+        capitalized_lib STREQUAL "${t}UTILS" )
       set(${return_var} ON PARENT_SCOPE)
       break()
     endif()
   endforeach()
 endfunction(is_llvm_target_library)
 
+function(is_llvm_target_specifier library return_var)
+  is_llvm_target_library(${library} ${return_var} ${ARGN})
+  string(TOUPPER "${library}" capitalized_lib)
+  if(NOT ${return_var})
+    if( capitalized_lib STREQUAL "ALLTARGETSASMPARSERS" OR
+        capitalized_lib STREQUAL "ALLTARGETSDESCS" OR
+        capitalized_lib STREQUAL "ALLTARGETSDISASSEMBLERS" OR
+        capitalized_lib STREQUAL "ALLTARGETSINFOS" OR
+        capitalized_lib STREQUAL "NATIVE" OR
+        capitalized_lib STREQUAL "NATIVECODEGEN" )
+      set(${return_var} ON PARENT_SCOPE)
+    endif()
+  endif()
+endfunction()
 
 macro(llvm_config executable)
   cmake_parse_arguments(ARG "USE_SHARED" "" "" ${ARGN})
   set(link_components ${ARG_UNPARSED_ARGUMENTS})
 
   if(USE_SHARED)
     # If USE_SHARED is specified, then we link against libLLVM,
     # but also against the component libraries below. This is

 function(llvm_map_components_to_libnames out_libs)
   set( link_components ${ARGN} )
   if(NOT LLVM_AVAILABLE_LIBS)
     # Inside LLVM itself available libs are in a global property.
     get_property(LLVM_AVAILABLE_LIBS GLOBAL PROPERTY LLVM_LIBS)
   endif()
   string(TOUPPER "${LLVM_AVAILABLE_LIBS}" capitalized_libs)
 
+  get_property(LLVM_TARGETS_CONFIGURED GLOBAL PROPERTY LLVM_TARGETS_CONFIGURED)
+
+  # Generally in our build system we avoid order-dependence. Unfortunately since
+  # not all targets create the same set of libraries we actually need to ensure
+  # that all build targets associated with a target are added before we can
+  # process target dependencies.
+  if(NOT LLVM_TARGETS_CONFIGURED)
+    foreach(c ${link_components})
+      is_llvm_target_specifier(${c} iltl_result ALL_TARGETS)
+      if(iltl_result)
+        message(FATAL_ERROR "Specified target library before target registration is complete.")
+      endif()
+    endforeach()
+  endif()
+
   # Expand some keywords:
   list(FIND LLVM_TARGETS_TO_BUILD "${LLVM_NATIVE_ARCH}" have_native_backend)
   list(FIND link_components "engine" engine_required)
   if( NOT engine_required EQUAL -1 )
     list(FIND LLVM_TARGETS_WITH_JIT "${LLVM_NATIVE_ARCH}" have_jit)
     if( NOT have_native_backend EQUAL -1 AND NOT have_jit EQUAL -1 )
       list(APPEND link_components "jit")
       list(APPEND link_components "native")
if( NOT idx LESS 0 )
         list(APPEND expanded_components "LLVM${c}Desc")
       endif()
       if( TARGET LLVM${c}Info )
         list(APPEND expanded_components "LLVM${c}Info")
       endif()
       if( TARGET LLVM${c}Disassembler )
         list(APPEND expanded_components "LLVM${c}Disassembler")
       endif()
+      if( TARGET LLVM${c}Info )
+        list(APPEND expanded_components "LLVM${c}Info")
+      endif()
+      if( TARGET LLVM${c}Utils )
+        list(APPEND expanded_components "LLVM${c}Utils")
+      endif()
     elseif( c STREQUAL "native" )
       # already processed
     elseif( c STREQUAL "nativecodegen" )
       list(APPEND expanded_components "LLVM${LLVM_NATIVE_ARCH}CodeGen")
       if( TARGET LLVM${LLVM_NATIVE_ARCH}Desc )
         list(APPEND expanded_components "LLVM${LLVM_NATIVE_ARCH}Desc")
       endif()
       if( TARGET LLVM${LLVM_NATIVE_ARCH}Info )
elseif( c STREQUAL "AllTargetsInfos" )
         endif()
       endforeach(t)
     else( NOT idx LESS 0 )
       # Canonize the component name:
       string(TOUPPER "${c}" capitalized)
       list(FIND capitalized_libs LLVM${capitalized} lib_idx)
       if( lib_idx LESS 0 )
         # The component is unknown. Maybe is an omitted target?
-        is_llvm_target_library(${c} iltl_result)
-        if( NOT iltl_result )
-          message(FATAL_ERROR "Library `${c}' not found in list of llvm libraries.")
+        is_llvm_target_library(${c} iltl_result OMITTED_TARGETS)
+        if(iltl_result)
+          # A missing library to a directly referenced omitted target would be bad.
+          message(FATAL_ERROR "Library '${c}' is a direct reference to a target library for an omitted target.")
+        else()
+          # If it is not an omitted target we should assume it is a component
+          # that hasn't yet been processed by CMake. Missing components will
+          # cause errors later in the configuration, so we can safely assume
+          # that this is valid here.
+          list(APPEND expanded_components LLVM${c})
         endif()
       else( lib_idx LESS 0 )
         list(GET LLVM_AVAILABLE_LIBS ${lib_idx} canonical_lib)
         list(APPEND expanded_components ${canonical_lib})
       endif( lib_idx LESS 0 )
     endif( NOT idx LESS 0 )
   endforeach(c)
 

llvm/trunk/lib/Target/CMakeLists.txt

add_llvm_library(LLVMTarget
   ADDITIONAL_HEADER_DIRS
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Target
   )
 
 foreach(t ${LLVM_TARGETS_TO_BUILD})
   message(STATUS "Targeting ${t}")
   add_subdirectory(${t})
 endforeach()
+
+# Currently we do not allow libraries from lib to reference targets directly.
+# This property is used to enforce that convention. It is important because the
+# logic in llvm_map_components_to_libnames is order dependent on the target
+# libraries being created.
+set_property(GLOBAL PROPERTY LLVM_TARGETS_CONFIGURED On)