[CMake] Replace use of llvm-config with LLVM and Clang CMake packages

[xiaobai]

Summary

I did this for two reasons:

• Using the CMake packages simplifies building LLDB Standalone. This is for two reasons: 1) We were doing a decent amount of work that is already done in the LLVMConfig.cmake that we want to import, 2) We had to do some manual work to call llvm-config, parse its output, and populate variables that the build system uses.
• As far as I understand, using llvm-config makes it difficult if not impossible to cross-compile LLDB standalone.

[mgorny]

Please don't risk merging this before the branching.

[labath]

Seems like I nice cleanup. I just have a question about the search paths.

(Also, I agree with @mgorny about not doing this so close to the branch point.)

cmake/modules/LLDBStandalone.cmake

9 ↗

(On Diff #180981)

Why do you put NO_DEFAULT_PATH here? IIUC, the user will now have to specify LLDB_PATH_TO_LLVM_BUILD in order to build this, whereas previously llvm-config would be found on the path if it happened to be there (e.g. because it was already installed).

Would it not make sense to keep this behavior?

[compnerd]

cmake/modules/LLDBStandalone.cmake

7 ↗

(On Diff #180981)

Please add the following here:

file(TO_CMAKE_PATH LLDB_PATH_TO_LLVM_BUILD "${LLDB_PATH_TO_LLVM_BUILD}")
file(TO_CMAKE_PATH LLDB_PATH_TO_CLANG_BUILD" "${LLDB_PATH_TO_CLANG_BUILD}")

This ensures that you can use platform specific paths when they do not match CMake's view (i.e. Windows)

[xiaobai]

cmake/modules/LLDBStandalone.cmake

7 ↗

(On Diff #180981)

Will do, thanks!

9 ↗

(On Diff #180981)

In situations where you have multiple LLVM builds where each might be a different version of LLVM, I think it is better to force the user to specify which LLVM build they want to use instead of having them implicitly rely on whichever llvm-config happens to be in their path.

That being said, I would be willing to remove NO_DEFAULT_PATH here. I can understand if people find this behavior valuable or if the scenario I described is not very common.

[xiaobai]

compnerd's comment

[labath]

cmake/modules/LLDBStandalone.cmake

9 ↗

(On Diff #180981)

I don't actually use the standalone build, so I don't care about this too much. I just mentioned this because this is the default behavior when searching for packages (as well as the previous behavior when we searched for llvm-config). However, it is true that we are version-locked much more tightly with llvm than with any of the other packages we search for with find_package.

The other thing that bugs me about the LLDB_PATH_TO_(LLVM|CLANG)_BUILD variables is that they seem to imply that you should point them to the build tree of llvm/clang. However, it should be possible to build lldb against an already-installed llvm, right (in which case they will likely have the same value)? In either case, I think it would be nice to explicitly declare these as a cache variable, if only so we can provide a docstring for them.

[mgorny]

cmake/modules/LLDBStandalone.cmake

9 ↗

(On Diff #180981)

In situations when you have multiple versions of LLVM in PATH, you usually set PATH in the order you want it to be used. And you really don't like when projects try to second-guess you.

[xiaobai]

cmake/modules/LLDBStandalone.cmake

9 ↗

(On Diff #180981)

@labath: When I wrote this I thought that it is possible to build against an installed LLVM, but I don't think that's currently possible. For example, LLVM_MAIN_INCLUDE_DIR should be set to the include directory in the LLVM source tree. TableGen.cmake adds the path in this variable to the include path when it invokes llvm-tablegen. This is exposed in the LLVM CMake package as LLVM_BUILD_MAIN_INCLUDE_DIR but only when you're using an LLVM build tree. The reason you need this is tools/driver/Options.td includes "llvm/Option/OptParser.td", which does not get put into the include directory of your LLVM build/install.

Maybe I should rewrite part of this to make it clearer that you need a build tree and not an llvm install? Declaring the variables as cache variables is a good idea nonetheless.

@mgorny: It seems like you find the behavior valuable so I will remove NO_DEFAULT_PATH. CMake processes the HINTS before searching your PATH regardles, so if you set LLDB_PATH_TO_${PROJECT}_BUILD then it will use that instead of using whatever it finds in your PATH.

[xiaobai]

Removed use of NO_DEFAULT_PATH
Made LLDB_PATH_TO_{LLVM,CLANG}_BUILD cache variables

[sgraenitz]

Thanks for the initiative! Would be great to have this part cleaned up as well.

cmake/modules/LLDBStandalone.cmake

9 ↗

(On Diff #180981)

Is it that instead of -DLLVM_CONFIG=/path/to/llvm-build/bin/llvm-config we would now pass -DLLDB_PATH_TO_LLVM_BUILD=/path/to/llvm-build?

LLDB_PATH_TO_(LLVM|CLANG)_BUILD variables [...] they will likely have the same value?

In my understanding this was always required. Did you try pointing LLDB_PATH_TO_CLANG_BUILD to a standalone build of Clang? Is there a good use-case for it? How do we detect that this Clang builds against the same LLVM build-tree?

When using LLVM/Clang/etc. as a dependency in external projects I usually followed the advice in https://www.llvm.org/docs/CMake.html#embedding-llvm-in-your-project:

If LLVM is not installed or you wish to build directly against the LLVM build tree you can use LLVM_DIR as previously mentioned.

That would be quite simple and find_package(LLVM) accepts LLVM_DIR as an input. Did you check how the other subproject do that?

[xiaobai]

cmake/modules/LLDBStandalone.cmake

9 ↗

(On Diff #180981)

Is it that instead of -DLLVM_CONFIG=/path/to/llvm-build/bin/llvm-config we would now pass -DLLDB_PATH_TO_LLVM_BUILD=/path/to/llvm-build?

Correct.

In my understanding this was always required. Did you try pointing LLDB_PATH_TO_CLANG_BUILD to a standalone build of Clang? Is there a good use-case for it? How do we detect that this Clang builds against the same LLVM build-tree?

I did not try doing it with a standalone build of Clang. I'm not sure if there is a good use-case for it, but if somebody knows then I'd like to hear about it. I am aware that it is possible to do a standalone build of Clang but I usually do integrated builds.

As for detecting whether or not the Clang builds against the same LLVM tree, I'm not sure if that's possible currently with the way LLVM and Clang's CMake packages are set up.

That would be quite simple and find_package(LLVM) accepts LLVM_DIR as an input. Did you check how the other subproject do that?

The other subprojects that I have looked at (clang, lld) have the current behavior of using llvm-config, which I think repeats a lot of work. Ideally the other projects would use the LLVM CMake package as well imo.

As for assigning LLVM_DIR directly, that is something that you could do. Even if I submitted this patch today as-is, setting LLVM_DIR directly would ignore LLDB_PATH_TO_LLVM_BUILD entirely. I would prefer still having the HINTS mechanism because it can take a list of directories. This is especially useful on build machines where the LLVM build can be found in one of a few possible locations. I also believe that the variable LLDB_PATH_TO_LLVM_BUILD is a little easier to understand than LLVM_DIR for people trying to figure out how to build LLDB. I see these two solutions as the same in every other regard though.

[sgraenitz]

In D56531#1352298, @mgorny wrote:

Please don't risk merging this before the branching.

+1
The official branch goal is Wednesday this week.

cmake/modules/LLDBStandalone.cmake

9 ↗

(On Diff #180981)

Sounds good to me. It would be nice if LLDB_PATH_TO_LLVM_BUILD is sufficient for the regular case of having LLVM & Clang in one build tree.

[sgraenitz]

In D56531#inline-500764, @labath wrote:

it should be possible to build lldb against an already-installed llvm

In D56531#inline-501059, @xiaobai wrote:

When I wrote this I thought that it is possible to build against an installed LLVM, but I don't think that's currently possible. For example [...]

IIUC all LLVM subprojects are made for *testing* against the build-tree. LLDB tests depend on LLVMTestingSupport and others that are not part of the installed package (see LLVM_EXPORTS_BUILDTREE_ONLY).
While the ability to *build* against an installed LLVM sounds like a good goal in principle, it's questionable what benefit it has without the ability to *test* the result. Thus, I wouldn't consider it a high prio for now.

[labath]

In D56531#1355794, @sgraenitz wrote:

In D56531#inline-500764, @labath wrote:

it should be possible to build lldb against an already-installed llvm

In D56531#inline-501059, @xiaobai wrote:

When I wrote this I thought that it is possible to build against an installed LLVM, but I don't think that's currently possible. For example [...]

IIUC all LLVM subprojects are made for *testing* against the build-tree. LLDB tests depend on LLVMTestingSupport and others that are not part of the installed package (see LLVM_EXPORTS_BUILDTREE_ONLY).
While the ability to *build* against an installed LLVM sounds like a good goal in principle, it's questionable what benefit it has without the ability to *test* the result. Thus, I wouldn't consider it a high prio for now.

The fact that in is not possible to test lldb when using an installed llvm is unfortunate, but I think it would be nice to preserve the ability to build it at least, as this is what (some? i am not sure about other distros, but this is at least true for gentoo) linux distros use to provide modular llvm packages. If you still wanted to provide modular packages from a monolithic build you'd have to go through the install tree manually and sort the artefacts into respective packages, defeating the purpose of the install tree.

(This is also the use case for having separated llvm and clang and then building lldb against those.)

[xiaobai]

Thanks all for the feedback and discussion!

I won't commit this until after the branch cut. I'm thinking it should be fine to do early next week, but I'll wait if needed. Does anyone have any objects to this patch in its current form? I will rebase it later this week to handle any conflicts.

[sgraenitz]

FYI Fixed two small details with https://reviews.llvm.org/rL351879, hope that's ok:

• LLDB_PATH_TO_CLANG_BUILD defaults to LLDB_PATH_TO_LLVM_BUILD
• switch args for file(TO_CMAKE_PATH "<path>" <variable>)

[xiaobai]

@sgraenitz: I don't mind at all, I view those changes as improvements. Thanks for that!

[sgraenitz]

Thanks -- and one more :) https://reviews.llvm.org/rL352058

[mgorny]

This definitely broke stand-alone builds:

FAILED: tools/driver/Options.inc 
cd /var/tmp/portage/dev-util/lldb-9999/work/lldb-9999_build && /usr/lib/llvm/9/bin/llvm-tblgen -gen-opt-parser-defs -I /var/tmp/portage/dev-util/lldb-9999/work/lldb-9999/tools/driver /var/tmp/portage/dev-util/lldb-9999/work/lldb-9999/tools/driver/Options.td -o tools/driver/Options.inc -d tools/driver/Options.inc.d
/var/tmp/portage/dev-util/lldb-9999/work/lldb-9999/tools/driver/Options.td:1:9: error: Could not find include file 'llvm/Option/OptParser.td'
include "llvm/Option/OptParser.td"
        ^
/var/tmp/portage/dev-util/lldb-9999/work/lldb-9999/tools/driver/Options.td:1:9: error: Unexpected input at top level
include "llvm/Option/OptParser.td"
        ^

[mgorny]

lldb/trunk/cmake/modules/LLDBStandalone.cmake

20

Apparently the idea of using LLVM_BUILD_MAIN_INCLUDE_DIR is plain wrong here. The old code used the equivalent of LLVM_INCLUDE_DIR (i.e. the include dir of *installed* LLVM) and I think the new code should use the same. Given how long the old code was there, I suppose there shouldn't be any use case broken by using that directory.

[mgorny]

Submitted a fix as D57995.

[sgraenitz]

lldb/trunk/cmake/modules/LLDBStandalone.cmake

20

Please note that it works well when running against a LLVM/Clang build-tree, which is a very common (but indeed not the only) use case this code should support.

lldb/trunk/cmake/modules/LLDBStandalone.cmake

 # If we are not building as a part of LLVM, build LLDB as an
 # standalone project, using LLVM as an external library:
 if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   project(lldb)
 
   option(LLVM_INSTALL_TOOLCHAIN_ONLY "Only include toolchain files in the 'install' target." OFF)
 
-  # Rely on llvm-config.
-  set(CONFIG_OUTPUT)
-  find_program(LLVM_CONFIG "llvm-config")
-  if(LLVM_CONFIG)
-    message(STATUS "Found LLVM_CONFIG as ${LLVM_CONFIG}")
-    set(CONFIG_COMMAND ${LLVM_CONFIG}
-      "--assertion-mode"
-      "--bindir"
-      "--libdir"
-      "--includedir"
-      "--prefix"
-      "--src-root"
-      "--cmakedir")
-    execute_process(
-      COMMAND ${CONFIG_COMMAND}
-      RESULT_VARIABLE HAD_ERROR
-      OUTPUT_VARIABLE CONFIG_OUTPUT
-    )
-    if(NOT HAD_ERROR)
-      string(REGEX REPLACE
-        "[ \t]*[\r\n]+[ \t]*" ";"
-        CONFIG_OUTPUT ${CONFIG_OUTPUT})
+  set(LLDB_PATH_TO_LLVM_BUILD "" CACHE PATH "Path to LLVM build tree")
+  set(LLDB_PATH_TO_CLANG_BUILD "" CACHE PATH "Path to Clang build tree")
 
-    else()
-      string(REPLACE ";" " " CONFIG_COMMAND_STR "${CONFIG_COMMAND}")
-      message(STATUS "${CONFIG_COMMAND_STR}")
-      message(FATAL_ERROR "llvm-config failed with status ${HAD_ERROR}")
-    endif()
-  else()
-    message(FATAL_ERROR "llvm-config not found -- ${LLVM_CONFIG}")
-  endif()
+  file(TO_CMAKE_PATH LLDB_PATH_TO_LLVM_BUILD "${LLDB_PATH_TO_LLVM_BUILD}")
+  file(TO_CMAKE_PATH LLDB_PATH_TO_CLANG_BUILD "${LLDB_PATH_TO_CLANG_BUILD}")
 
-  list(GET CONFIG_OUTPUT 0 ENABLE_ASSERTIONS)
-  list(GET CONFIG_OUTPUT 1 TOOLS_BINARY_DIR)
-  list(GET CONFIG_OUTPUT 2 LIBRARY_DIR)
-  list(GET CONFIG_OUTPUT 3 INCLUDE_DIR)
-  list(GET CONFIG_OUTPUT 4 LLVM_OBJ_ROOT)
-  list(GET CONFIG_OUTPUT 5 MAIN_SRC_DIR)
-  list(GET CONFIG_OUTPUT 6 LLVM_CMAKE_PATH)
-
-  if(NOT MSVC_IDE)
-    set(LLVM_ENABLE_ASSERTIONS ${ENABLE_ASSERTIONS}
-      CACHE BOOL "Enable assertions")
-    # Assertions should follow llvm-config's.
-    mark_as_advanced(LLVM_ENABLE_ASSERTIONS)
-  endif()
+  find_package(LLVM REQUIRED CONFIG
+    HINTS "${LLDB_PATH_TO_LLVM_BUILD}" NO_CMAKE_FIND_ROOT_PATH)
+  find_package(Clang REQUIRED CONFIG
+    HINTS "${LLDB_PATH_TO_CLANG_BUILD}" NO_CMAKE_FIND_ROOT_PATH)
+
+  # We set LLVM_MAIN_INCLUDE_DIR so that it gets included in TableGen flags.
+  set(LLVM_MAIN_INCLUDE_DIR "${LLVM_BUILD_MAIN_INCLUDE_DIR}" CACHE PATH "Path to LLVM's source include dir")
+  # We set LLVM_CMAKE_PATH so that GetSVN.cmake is found correctly when building SVNVersion.inc
+  set(LLVM_CMAKE_PATH ${LLVM_CMAKE_DIR} CACHE PATH "Path to LLVM CMake modules")
 
-  set(LLVM_TOOLS_BINARY_DIR ${TOOLS_BINARY_DIR} CACHE PATH "Path to llvm/bin")
-  set(LLVM_LIBRARY_DIR ${LIBRARY_DIR} CACHE PATH "Path to llvm/lib")
-  set(LLVM_MAIN_INCLUDE_DIR ${INCLUDE_DIR} CACHE PATH "Path to llvm/include")
-  set(LLVM_DIR ${LLVM_OBJ_ROOT}/cmake/modules/CMakeFiles CACHE PATH "Path to LLVM build tree CMake files")
-  set(LLVM_BINARY_DIR ${LLVM_OBJ_ROOT} CACHE PATH "Path to LLVM build tree")
-  set(LLVM_MAIN_SRC_DIR ${MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
   set(LLVM_EXTERNAL_LIT ${LLVM_TOOLS_BINARY_DIR}/llvm-lit CACHE PATH "Path to llvm-lit")
-
   find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR}
     NO_DEFAULT_PATH)
 
-  set(LLVMCONFIG_FILE "${LLVM_CMAKE_PATH}/LLVMConfig.cmake")
-  if(EXISTS ${LLVMCONFIG_FILE})
-    file(TO_CMAKE_PATH "${LLVM_CMAKE_PATH}" LLVM_CMAKE_PATH)
-    list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_PATH}")
-    include(${LLVMCONFIG_FILE})
-  else()
-    message(FATAL_ERROR "Not found: ${LLVMCONFIG_FILE}")
-  endif()
-
   # They are used as destination of target generators.
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin)
   set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
   if(WIN32 OR CYGWIN)
     # DLL platform -- put DLLs into bin.
     set(LLVM_SHLIB_OUTPUT_INTDIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
   else()
     set(LLVM_SHLIB_OUTPUT_INTDIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
   endif()
 
+  # We append the directory in which LLVMConfig.cmake lives. We expect LLVM's
+  # CMake modules to be in that directory as well.
+  list(APPEND CMAKE_MODULE_PATH "${LLVM_DIR}")
   include(AddLLVM)
   include(TableGen)
   include(HandleLLVMOptions)
   include(CheckAtomic)
 
   if (PYTHON_EXECUTABLE STREQUAL "")
     set(Python_ADDITIONAL_VERSIONS 3.5 3.4 3.3 3.2 3.1 3.0 2.7 2.6 2.5)
     include(FindPythonInterp)
     if( NOT PYTHONINTERP_FOUND )
       message(FATAL_ERROR
               "Unable to find Python interpreter, required for builds and testing.
                Please install Python or specify the PYTHON_EXECUTABLE CMake variable.")
     endif()
   else()
     message("-- Found PythonInterp: ${PYTHON_EXECUTABLE}")
   endif()
 
-  # Import CMake library targets from LLVM and Clang.
-  include("${LLVM_OBJ_ROOT}/lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm/LLVMConfig.cmake")
-  if (EXISTS "${LLVM_OBJ_ROOT}/lib${LLVM_LIBDIR_SUFFIX}/cmake/clang/ClangConfig.cmake")
-    include("${LLVM_OBJ_ROOT}/lib${LLVM_LIBDIR_SUFFIX}/cmake/clang/ClangConfig.cmake")
-  endif()
-
   set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
   set(LLVM_INCLUDE_TESTS ON CACHE INTERNAL "")
 
   set(CMAKE_INCLUDE_CURRENT_DIR ON)
-  include_directories("${CMAKE_BINARY_DIR}/include" "${LLVM_MAIN_INCLUDE_DIR}")
-  # Next three include directories are needed when llvm-config is located in build directory.
-  # LLVM and Clang are assumed to be built together
-  if (EXISTS "${LLVM_OBJ_ROOT}/include")
-    include_directories("${LLVM_OBJ_ROOT}/include")
-  endif()
-  if (EXISTS "${LLVM_MAIN_SRC_DIR}/tools/clang/include")
-    include_directories("${LLVM_MAIN_SRC_DIR}/tools/clang/include")
-  endif()
-  if (EXISTS "${LLVM_OBJ_ROOT}/tools/clang/include")
-    include_directories("${LLVM_OBJ_ROOT}/tools/clang/include")
-  endif()
+  include_directories(
+    "${CMAKE_BINARY_DIR}/include"
+    "${LLVM_INCLUDE_DIRS}"
+    "${CLANG_INCLUDE_DIRS}")
+
   link_directories("${LLVM_LIBRARY_DIR}")
 
   set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
   set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
   set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
 
   set(LLDB_BUILT_STANDALONE 1)
 endif()