build: remove use of llvm-config

[compnerd]

Summary

Update the build to use the standard CMake package search mechanism (-DLLVM_DIR=). This reduces the use of llvm-config which will eventually allow us to remove the tool. It also enhances the support to cross-compile lld in standalone mode.

[ruiu]

I'm not CMake expert so I'm probably not the right person to review this, but if you are making the same change to other tools under the LLVM umbrella, I'm fine with this change.

[tstellar]

I made a similar change to clang: https://reviews.llvm.org/rL346732, and I think this is the right direction to go.

[ruiu]

Tom,

Feel free to approve this patch once your patch to clang has approved.

[tstellar]

The clang patch was merged a few months ago. LGTM.

[mgorny]

This change breaks building against installed LLVM with externally provided unconfigured sources. LLVM_MAIN_SRC_DIR was made a cache variable originally for this purpose.

[compnerd]

This doesn't break the build, we check that the path exists, and find_program will not error out. You can still specify LLVM_LIT_EXE to specify the lit.py to use. Is that not sufficient for your needs?

[tstellar]

In D57406#1377708, @compnerd wrote:

This doesn't break the build, we check that the path exists, and find_program will not error out. You can still specify LLVM_LIT_EXE to specify the lit.py to use. Is that not sufficient for your needs?

The unittests won't be enabled unless ${LLVM_BUILD_MAIN_SRC_DIR} exists and points to an LLVM source tree, which is one reason to have it as a cache variable.

[xiaobai]

I wrote a patch similar to this in LLDB, so I'll chime in a bit.

@mgorny: This doesn't actually *break* the build per se. This change just means you need to do a bit more work to be able to test the lld you just built. Some things that could be done to improve the situation:

• Users who want to test lld should specify LLVM_BUILD_MAIN_SRC_DIR (and possibly LLVM_LIT_EXE) manually.
• Introduce some new variable like PATH_TO_LLVM_SRC and replace LLVM_BUILD_MAIN_SRC_DIR. This could be optional variable which would need to be set if you want to test.

I don't see this as a hard blocker for putting this patch in. It simplifies the build logic because we don't have to call out to some other process and parse its output, and it de-duplicates work because the LLVM CMake package provides a lot of the variables that we were filling out.

[tstellar]

In D57406#1377725, @xiaobai wrote:

I wrote a patch similar to this in LLDB, so I'll chime in a bit.

@mgorny: This doesn't actually *break* the build per se. This change just means you need to do a bit more work to be able to test the lld you just built. Some things that could be done to improve the situation:

• Users who want to test lld should specify LLVM_BUILD_MAIN_SRC_DIR (and possibly LLVM_LIT_EXE) manually.

This isn't actually different from what you had to do before this patch. The only difference is that LLVM_MAIN_SRC_DIR was a CACHE variable, but now LLVM_BUILD_MAIN_SRC_DIR is not.
The request was to have a CACHE variable.

• Introduce some new variable like PATH_TO_LLVM_SRC and replace LLVM_BUILD_MAIN_SRC_DIR. This could be optional variable which would need to be set if you want to test.

I don't see this as a hard blocker for putting this patch in. It simplifies the build logic because we don't have to call out to some other process and parse its output, and it de-duplicates work because the LLVM CMake package provides a lot of the variables that we were filling out.

[xiaobai]

In D57406#1377799, @tstellar wrote:

In D57406#1377725, @xiaobai wrote:

I wrote a patch similar to this in LLDB, so I'll chime in a bit.

@mgorny: This doesn't actually *break* the build per se. This change just means you need to do a bit more work to be able to test the lld you just built. Some things that could be done to improve the situation:

• Users who want to test lld should specify LLVM_BUILD_MAIN_SRC_DIR (and possibly LLVM_LIT_EXE) manually.

This isn't actually different from what you had to do before this patch. The only difference is that LLVM_MAIN_SRC_DIR was a CACHE variable, but now LLVM_BUILD_MAIN_SRC_DIR is not.
The request was to have a CACHE variable

• Introduce some new variable like PATH_TO_LLVM_SRC and replace LLVM_BUILD_MAIN_SRC_DIR. This could be optional variable which would need to be set if you want to test.

I don't see this as a hard blocker for putting this patch in. It simplifies the build logic because we don't have to call out to some other process and parse its output, and it de-duplicates work because the LLVM CMake package provides a lot of the variables that we were filling out.

The issue here is that the old variable LLVM_MAIN_SRC_DIR was a CACHE variable and LLVM_

Ah, I misunderstood what was being said. I should have read more carefully. :)
Adding back LLVM_MAIN_SRC_DIR as a cache var and having it default to LLVM_BUILD_MAIN_SRC_DIR should be easy enough.

[compnerd]

Restore LLVM_MAIN_SRC_DIR cache variable

[xiaobai]

After taking the time to go through this more thoroughly I realized there were several issues that need to be addressed before this can go in.

I also found that testing is broken with LLD standalone builds. Specifically, I ran ninja check-lld and that doesn't work because it tries to use llvm-lit from the lld build tree. This patch is not at fault, as it fails the same way with or without this patch. I think this could be addressed in a later patch.

CMakeLists.txt

10

This should be changed to LLVM_CMAKE_DIR or LLVM_DIR (Both of these directories will have the LLVM CMake modules). As-is, LLVM_CMAKE_PATH is not set.

11

You also will need to set LLVM_MAIN_INCLUDE_DIR here or else TableGen won't include that path by default. Something like this should work:
set(LLVM_MAIN_INCLUDE_DIR ${LLVM_BUILD_MAIN_INCLUDE_DIR} CACHE PATH "Path to llvm/include")

33

find_program -> set

38

find_program -> set

[compnerd]

Fixes

[mgorny]

Seems not to break anything for me ;-).

[mgorny]

I'm sorry, I must've tested wrong before. It actually breaks the build for me:

[29/143] cd /var/tmp/portage/sys-devel/lld-9999/work/lld-9999_build && /usr/lib/llvm/9/bin/llvm-tblgen -gen-opt-parser-defs -I /var/tmp/portage/sys
-devel/lld-9999/work/lld-9999/lib/Driver /var/tmp/portage/sys-devel/lld-9999/work/lld-9999/lib/Driver/DarwinLdOptions.td -o lib/Driver/DarwinLdOpti
ons.inc -d lib/Driver/DarwinLdOptions.inc.d
FAILED: lib/Driver/DarwinLdOptions.inc
cd /var/tmp/portage/sys-devel/lld-9999/work/lld-9999_build && /usr/lib/llvm/9/bin/llvm-tblgen -gen-opt-parser-defs -I /var/tmp/portage/sys-devel/ll
d-9999/work/lld-9999/lib/Driver /var/tmp/portage/sys-devel/lld-9999/work/lld-9999/lib/Driver/DarwinLdOptions.td -o lib/Driver/DarwinLdOptions.inc -
d lib/Driver/DarwinLdOptions.inc.d
/var/tmp/portage/sys-devel/lld-9999/work/lld-9999/lib/Driver/DarwinLdOptions.td:1:9: error: Could not find include file 'llvm/Option/OptParser.td'
include "llvm/Option/OptParser.td"
        ^
/var/tmp/portage/sys-devel/lld-9999/work/lld-9999/lib/Driver/DarwinLdOptions.td:1:9: error: Unexpected input at top level
include "llvm/Option/OptParser.td"
        ^
ninja: build stopped: subcommand failed.

[compnerd]

Which version of the patch did you use? L13 in the current version sets LLVM_MAIN_INCLUDE_DIR specifically for that case.

[mgorny]

Yep, that one. However, something else might have changed in the background since LLDB doesn't want to build anymore either. I'll try a bit of pseudo-bisect just in case.

[mgorny]

Ok, I've researched this in detail and LLDB is broken by the same change. The concept used for tablegen here is apparently wrong, and causes it not to find installed .td files.

[mgorny]

CMakeLists.txt

11

So I've looked into the failures more closely, and it looks that ${LLVM_BUILD_MAIN_INCLUDE_DIR} is wrong here. It should be ${LLVM_INCLUDE_DIR}, i.e. the directory with installed headers. This is also what the old code was doing.

CMakeLists.txt

   set(CMAKE_INCLUDE_CURRENT_DIR ON)
   set(LLD_BUILT_STANDALONE TRUE)
 
-  find_program(LLVM_CONFIG_PATH "llvm-config" DOC "Path to llvm-config binary")
-  if(NOT LLVM_CONFIG_PATH)
-    message(FATAL_ERROR "llvm-config not found: specify LLVM_CONFIG_PATH")
-  endif()
-
-  execute_process(COMMAND "${LLVM_CONFIG_PATH}"
-                          "--obj-root"
-                          "--includedir"
-                          "--cmakedir"
-                          "--src-root"
-                  RESULT_VARIABLE HAD_ERROR
-                  OUTPUT_VARIABLE LLVM_CONFIG_OUTPUT
-                  OUTPUT_STRIP_TRAILING_WHITESPACE)
-  if(HAD_ERROR)
-    message(FATAL_ERROR "llvm-config failed with status ${HAD_ERROR}")
-  endif()
-
-  string(REGEX REPLACE "[ \t]*[\r\n]+[ \t]*" ";" LLVM_CONFIG_OUTPUT "${LLVM_CONFIG_OUTPUT}")
-
-  list(GET LLVM_CONFIG_OUTPUT 0 OBJ_ROOT)
-  list(GET LLVM_CONFIG_OUTPUT 1 MAIN_INCLUDE_DIR)
-  list(GET LLVM_CONFIG_OUTPUT 2 LLVM_CMAKE_PATH)
-  list(GET LLVM_CONFIG_OUTPUT 3 MAIN_SRC_DIR)
-
-  set(LLVM_OBJ_ROOT ${OBJ_ROOT} CACHE PATH "path to LLVM build tree")
-  set(LLVM_MAIN_INCLUDE_DIR ${MAIN_INCLUDE_DIR} CACHE PATH "path to llvm/include")
-  set(LLVM_MAIN_SRC_DIR ${MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
-
-  file(TO_CMAKE_PATH ${LLVM_OBJ_ROOT} LLVM_BINARY_DIR)
-
-  if(NOT EXISTS "${LLVM_CMAKE_PATH}/LLVMConfig.cmake")
-    message(FATAL_ERROR "LLVMConfig.cmake not found")
-  endif()
-  include("${LLVM_CMAKE_PATH}/LLVMConfig.cmake")
-
+  find_package(LLVM REQUIRED CONFIG NO_CMAKE_FIND_ROOT_PATH)
   list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_PATH}")
+  set(LLVM_MAIN_SRC_DIR ${LLVM_BUILD_MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
 
   set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
-  include_directories("${LLVM_BINARY_DIR}/include" ${LLVM_INCLUDE_DIRS})
+  include_directories(${LLVM_INCLUDE_DIRS})
   link_directories(${LLVM_LIBRARY_DIRS})
 
   set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin)
-  find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)
+
+  if(CMAKE_CROSSCOMPILING)
+    set(LLVM_NATIVE_BUILD "${LLVM_BINARY_DIR}/NATIVE")
+    if(NOT EXISTS "${LLVM_NATIVE_BUILD}")
+      message(FATAL_ERROR
+        "Attempting to cross-compiling lld standalone but no native LLVM build
+        found.  Please cross-compile LLVM as well.")
+    endif()
+
+    if(CMAKE_HOST_SYSTEM_NAME MATCHES Windows)
+      set(HOST_EXECUTABLE_SUFFIX ".exe")
+    endif()
+
+    if(NOT CMAKE_CONFIGURATION_TYPES)
+      find_program(LLVM_TABLEGEN_EXE
+        "${LLVM_NATIVE_BUILD_DIR}/bin/llvm-tblgen${HOST_EXECUTABLE_SUFFIX}")
+    else()
+      # NOTE: LLVM NATIVE build is always built Release, as is specified in
+      # CrossCompile.cmake
+      find_program(LLVM_TABLEGEN_EXE
+        "${LLVM_NATIVE_BUILD_DIR}/Release/bin/llvm-tblgen${HOST_EXECUTABLE_SUFFIX}")
+    endif()
+  else()
+    find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR}
+      NO_DEFAULT_PATH)
+  endif()
 
   include(AddLLVM)
   include(TableGen)