[CMake] Respect variables for specifying host tools even without LLVM_USE_HOST_TOOLS set

[mstorsjo]

Summary

When LLVM_NATIVE_TOOL_DIR was introduced in
d3da9067d143f3d4ce59b6d9ab4606a8ef1dc937 / D131052, it consisted
of refactoring a couple cases of manual logic for tools in
clang-tools-extra/clang-tidy, clang-tools-extra/pseudo/include
and mlir/tools/mlir-linalg-ods-gen. The former two had the same
consistent behaviour while the latter was slightly different, so
the refactoring would end up slightly adjusting one or the other.

The difference was that the clang-tools-extra tools respected the
external variable for setting the tool name, regardless of the
LLVM_USE_HOST_TOOLS variable, while mlir-linalg-ods-gen tool
only checked its external variable if LLVM_USE_HOST_TOOLS was set.

LLVM_USE_HOST_TOOLS is supposed to be enabled automatically whenever
cross compiling, so this shouldn't have been an issue.

In https://github.com/llvm/llvm-project/issues/60784, it seems like
some users do cross compile LLVM, without CMake knowing about it
(without CMAKE_CROSSCOMPILING being set). In these cases, their
build broke, as the variables for pointing to external host tools
no longer were being respected.

Skip the checks for LLVM_USE_HOST_TOOLS and always respect the
variables for pointing to external tools (both the old tool specific
variables, and the new LLVM_NATIVE_TOOL_DIR), if they're set. This
makes the logic within setup_host_tool more exactly match the
logic for the clang-tools-extra tools from before the refactoring
in d3da9067d143f3d4ce59b6d9ab4606a8ef1dc937.

This should hopefully fix
https://github.com/llvm/llvm-project/issues/60784 (and should
probably be backported to the 16.x release branch in the end).

[smeenai]

I'd need to learn more about the use case (I see you're awaiting a response on the issue), but I'm not a huge fan personally; it seems really weird to not be going through the standard CMake mechanisms for cross-compilation (setting a CMAKE_SYSTEM_NAME), and if you do that then this should work as expected. We should understand why LLVM_USE_HOST_TOOLS isn't being set automatically.

[mstorsjo]

In D146666#4214769, @smeenai wrote:

I'd need to learn more about the use case (I see you're awaiting a response on the issue), but I'm not a huge fan personally; it seems really weird to not be going through the standard CMake mechanisms for cross-compilation (setting a CMAKE_SYSTEM_NAME), and if you do that then this should work as expected. We should understand why LLVM_USE_HOST_TOOLS isn't being set automatically.

Yeah, in one sense, that’s the correct thing to do. But on the other hand, even if I’m doing a simple native build, if I’m going out of my way to specify the path to these tools, perhaps we should respect it. We already do it somewhat inconsistently - AFAIK the variables do take effect for the tablegen family of tools without checking the LLVM_USE_HOST_TOOLS variables.

[smeenai]

In D146666#4214806, @mstorsjo wrote:

In D146666#4214769, @smeenai wrote:

I'd need to learn more about the use case (I see you're awaiting a response on the issue), but I'm not a huge fan personally; it seems really weird to not be going through the standard CMake mechanisms for cross-compilation (setting a CMAKE_SYSTEM_NAME), and if you do that then this should work as expected. We should understand why LLVM_USE_HOST_TOOLS isn't being set automatically.

Yeah, in one sense, that’s the correct thing to do. But on the other hand, even if I’m doing a simple native build, if I’m going out of my way to specify the path to these tools, perhaps we should respect it. We already do it somewhat inconsistently - AFAIK the variables do take effect for the tablegen family of tools without checking the LLVM_USE_HOST_TOOLS variables.

The flip side could be that you have a shared configuration for all your builds that just sets that flag in case it's needed, and that shared config is used by both native and cross-compilation. Not sure how realistic that is either, but I can see someone being surprised the other way as well.

[mstorsjo]

In D146666#4214857, @smeenai wrote:

In D146666#4214806, @mstorsjo wrote:

In D146666#4214769, @smeenai wrote:

I'd need to learn more about the use case (I see you're awaiting a response on the issue), but I'm not a huge fan personally; it seems really weird to not be going through the standard CMake mechanisms for cross-compilation (setting a CMAKE_SYSTEM_NAME), and if you do that then this should work as expected. We should understand why LLVM_USE_HOST_TOOLS isn't being set automatically.

Yeah, in one sense, that’s the correct thing to do. But on the other hand, even if I’m doing a simple native build, if I’m going out of my way to specify the path to these tools, perhaps we should respect it. We already do it somewhat inconsistently - AFAIK the variables do take effect for the tablegen family of tools without checking the LLVM_USE_HOST_TOOLS variables.

The flip side could be that you have a shared configuration for all your builds that just sets that flag in case it's needed, and that shared config is used by both native and cross-compilation. Not sure how realistic that is either, but I can see someone being surprised the other way as well.

That's true - although if the provided tools are valid (up to date), it should be harmless to use them. It could possibly even speed up the build, if it could use the preexisting tools for generating headers without first needing to build the new tool (but it's possible that there are dependencies in place that force the new tools to be built first anyway).

In any case, if looking at historical behaviour, the tablegen tools have respected the setting without looking at LLVM_USE_HOST_TOOLS since forever - and still do, that part wasn't refactored in my commit, and the clang-tools-extra tools also used their individual settings without looking at LLVM_USE_HOST_TOOLS in 15.x. Previously it was only mlir-linalg-ods-gen which required LLVM_USE_HOST_TOOLS to be set.

So with long term behaviour vs surprises, my refactoring was the one that brought in the surprise change.

It's still of course important to use cmake correctly and make sure it's aware of the fact that the user is cross compiling - but I think I needlessly caused surprises/breakage for users here.

[smeenai]

Fair enough; I'm convinced :) LGTM

llvm/cmake/modules/AddLLVM.cmake

if(git_result EQUAL 0)
       set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
     endif()
   endif()
 endfunction()
 
 function(setup_host_tool tool_name setting_name exe_var_name target_var_name)
   set(${setting_name}_DEFAULT "${tool_name}")
 
-  if(LLVM_USE_HOST_TOOLS AND LLVM_NATIVE_TOOL_DIR)
+  if(LLVM_NATIVE_TOOL_DIR)
     if(EXISTS "${LLVM_NATIVE_TOOL_DIR}/${tool_name}${LLVM_HOST_EXECUTABLE_SUFFIX}")
       set(${setting_name}_DEFAULT "${LLVM_NATIVE_TOOL_DIR}/${tool_name}${LLVM_HOST_EXECUTABLE_SUFFIX}")
     endif()
   endif()
 
   set(${setting_name} "${${setting_name}_DEFAULT}" CACHE
     STRING "Host ${tool_name} executable. Saves building if cross-compiling.")
 
-  if(LLVM_USE_HOST_TOOLS)
-    if(NOT ${setting_name} STREQUAL "${tool_name}")
-      set(exe_name ${${setting_name}})
-      set(target_name ${${setting_name}})
-    else()
-      build_native_tool(${tool_name} exe_name DEPENDS ${tool_name})
-      set(target_name ${exe_name})
-    endif()
+  elseif(LLVM_USE_HOST_TOOLS)
   else()
     set(exe_name $<TARGET_FILE:${tool_name}>)
     set(target_name ${tool_name})
   endif()
   set(${exe_var_name} "${exe_name}" CACHE STRING "")
   set(${target_var_name} "${target_name}" CACHE STRING "")
 endfunction()