[LLVM] Fix GetErrcMessages.cmake module for WoA

[omjavaid]

Summary

GetErrcMessages.cmake module makes use of cmake's try_run which by
default builds its sources in debug mode unless configured with
CMAKE_TRY_COMPILE_CONFIGURATION. Debug builds on Windows sometimes fail
when appropraite DLLs are not included in path. Also on Windows on Arm
machines debug builds sometimes fail to link the correct debug DLLs.

To fix this I am setting CMAKE_TRY_COMPILE_CONFIGURATION to active build
configuration of currently configured LLVM project. This makes sure we
select same build type for try_run/try_compile cmake modules as
currently configured LLVM project.

[DavidSpickett]

I was curious about the reasoning from cmake here.

https://cmake.org/cmake/help/latest/variable/CMAKE_TRY_COMPILE_CONFIGURATION.html#variable:CMAKE_TRY_COMPILE_CONFIGURATION:

Projects built by try_compile() and try_run() are built synchronously during the CMake configuration step. Therefore a specific build configuration must be chosen even if the generated build system supports multiple configurations.

Which maybe implies that at the time they run cmake doesn't know what build type you want and goes to it's default which is debug (not the project's default, which may not be set yet). Though it does seem like a chicken and egg situation given that the way you set CMAKE_TRY_COMPILE_CONFIGURATION is itself in the project's cmake (or at least can be) but ok I'm missing something there I'm sure.

Is it possibly better to set this at a higher level? I know there is some existing logic to error if you don't choose a build type for llvm now, maybe there?

I think the overall idea of "you shouldn't need to be able to build debug if you chose to build release" is fine but there's probably more try_compiles.

[mstorsjo]

I think @zero9178 was involved in adding this runtime check, adding him as reviewer too.

In D133482#3776741, @DavidSpickett wrote:

I was curious about the reasoning from cmake here.

https://cmake.org/cmake/help/latest/variable/CMAKE_TRY_COMPILE_CONFIGURATION.html#variable:CMAKE_TRY_COMPILE_CONFIGURATION:

Projects built by try_compile() and try_run() are built synchronously during the CMake configuration step. Therefore a specific build configuration must be chosen even if the generated build system supports multiple configurations.

I guess that this also means, that if you're building with such a multi configuration generator (e.g. the Visual Studio project file generator), there simply is no single ${CMAKE_BUILD_TYPE} set at this point (or does LLVM's own logic set one anyway?). I think such build configurations are uncommon, but they probably still exist.

Is it possibly better to set this at a higher level? I know there is some existing logic to error if you don't choose a build type for llvm now, maybe there?

I think the overall idea of "you shouldn't need to be able to build debug if you chose to build release" is fine but there's probably more try_compiles.

Alternatively, maybe we should back up and reset this variable to what it was set to before? So that we only affect this particular one. Then again, (my cmake is a bit unclear on this point), does setting it here, within a function, mean it doesn't affect things outside of this function? Then it's probably safe as such...

[zero9178]

If multi project configurators are a concern then some additional logic could be added via checking for GENERATOR_IS_MULTI_CONFIG[0] and then choosing one of the build types listed in CMAKE_CONFIGURATION_TYPES[1]. It shouldn't matter which one of them is chosen.

In D133482#3776766, @mstorsjo wrote:

Alternatively, maybe we should back up and reset this variable to what it was set to before? So that we only affect this particular one. Then again, (my cmake is a bit unclear on this point), does setting it here, within a function, mean it doesn't affect things outside of this function? Then it's probably safe as such...

CMake functions create a separate scope that is initialized with the parent scope, so this would be safe I believe.

[0] https://cmake.org/cmake/help/latest/prop_gbl/GENERATOR_IS_MULTI_CONFIG.html
[1] https://cmake.org/cmake/help/latest/variable/CMAKE_CONFIGURATION_TYPES.html#variable:CMAKE_CONFIGURATION_TYPES

[omjavaid]

In D133482#3776778, @zero9178 wrote:

If multi project configurators are a concern then some additional logic could be added via checking for GENERATOR_IS_MULTI_CONFIG[0] and then choosing one of the build types listed in CMAKE_CONFIGURATION_TYPES[1]. It shouldn't matter which one of them is chosen.

This looks like a good plan but as far as GetErrcMessages.cmake module is concerned we can also mark it to always run in release mode rather than choosing a config. Any thoughts on always running this module in release mode?

In D133482#3776766, @mstorsjo wrote:

Alternatively, maybe we should back up and reset this variable to what it was set to before? So that we only affect this particular one. Then again, (my cmake is a bit unclear on this point), does setting it here, within a function, mean it doesn't affect things outside of this function? Then it's probably safe as such...

CMake functions create a separate scope that is initialized with the parent scope, so this would be safe I believe.

[0] https://cmake.org/cmake/help/latest/prop_gbl/GENERATOR_IS_MULTI_CONFIG.html
[1] https://cmake.org/cmake/help/latest/variable/CMAKE_CONFIGURATION_TYPES.html#variable:CMAKE_CONFIGURATION_TYPES

[zero9178]

In D133482#3776820, @omjavaid wrote:

In D133482#3776778, @zero9178 wrote:

If multi project configurators are a concern then some additional logic could be added via checking for GENERATOR_IS_MULTI_CONFIG[0] and then choosing one of the build types listed in CMAKE_CONFIGURATION_TYPES[1]. It shouldn't matter which one of them is chosen.

This looks like a good plan but as far as GetErrcMessages.cmake module is concerned we can also mark it to always run in release mode rather than choosing a config. Any thoughts on always running this module in release mode?

In D133482#3776766, @mstorsjo wrote:

Alternatively, maybe we should back up and reset this variable to what it was set to before? So that we only affect this particular one. Then again, (my cmake is a bit unclear on this point), does setting it here, within a function, mean it doesn't affect things outside of this function? Then it's probably safe as such...

CMake functions create a separate scope that is initialized with the parent scope, so this would be safe I believe.

[0] https://cmake.org/cmake/help/latest/prop_gbl/GENERATOR_IS_MULTI_CONFIG.html
[1] https://cmake.org/cmake/help/latest/variable/CMAKE_CONFIGURATION_TYPES.html#variable:CMAKE_CONFIGURATION_TYPES

I am not familiar with Windows on ARM, nor the linking issue you described with debug builds. Does this only affect debug builds and for what reason (eg. is there a distribution of build tools that doesn't ship the debug DLLs?). Can this also happen with release builds instead of debug builds? If no, then I suppose just always using the release config would be alright. Choosing one of the configs the user requested to build feels like its always the safe option however, given that specific configuration ought to work if the user requested to use it.

[omjavaid]

This update moves configuration of CMAKE_TRY_COMPILE_CONFIGURATION
to the top-level CMakeLists.txt file. Also setting is done based
on CMAKE_CONFIGURATION_TYPES or CMAKE_BUILD_TYPE unless there is
no CMAKE_DEFAULT_BUILD_TYPE already configured by user.

[zero9178]

LGTM % nit. Thanks!

llvm/CMakeLists.txt

109

nit: I think it might be great to add a comment that once LLVM uses cmake 3.17 as minimum, the code can be simplified to just always use CMAKE_DEFAULT_BUILD_TYPE in the CMAKE_CONFIGURATION_TYPES case (since that gets initialized with the first element in CMAKE_CONFIGURATION_TYPES). Until then I like this approach of already using it if a user were to define it, even prior to 3.17.

llvm/CMakeLists.txt

Available options are:
   * -DCMAKE_BUILD_TYPE=Release - For an optimized build with no assertions or debug info.
   * -DCMAKE_BUILD_TYPE=Debug - For an unoptimized build with assertions and debug info.
   * -DCMAKE_BUILD_TYPE=RelWithDebInfo - For an optimized build with no assertions but with debug info.
   * -DCMAKE_BUILD_TYPE=MinSizeRel - For a build optimized for size instead of speed.
 Learn more about these options in our documentation at https://llvm.org/docs/CMake.html#cmake-build-type
 ")
 endif()
 
+# Set default build type for cmake's try_compile module.
+# CMake 3.17 or newer sets CMAKE_DEFAULT_BUILD_TYPE to one of the
+# items from CMAKE_CONFIGURATION_TYPES. Logic below can be further
+# simplified once LLVM's minimum CMake version is updated to 3.17.
+if(CMAKE_DEFAULT_BUILD_TYPE)
+  set(CMAKE_TRY_COMPILE_CONFIGURATION ${CMAKE_DEFAULT_BUILD_TYPE})
+else()
+  if(CMAKE_CONFIGURATION_TYPES)
+    list(GET CMAKE_CONFIGURATION_TYPES 0 CMAKE_TRY_COMPILE_CONFIGURATION)
+  elseif(CMAKE_BUILD_TYPE)
+    set(CMAKE_TRY_COMPILE_CONFIGURATION ${CMAKE_BUILD_TYPE})
+  endif()
+endif()
+
 # Side-by-side subprojects layout: automatically set the
 # LLVM_EXTERNAL_${project}_SOURCE_DIR using LLVM_ALL_PROJECTS
 # This allows an easy way of setting up a build directory for llvm and another
 # one for llvm+clang+... using the same sources.
 set(LLVM_ALL_PROJECTS "bolt;clang;clang-tools-extra;compiler-rt;cross-project-tests;libc;libclc;libcxx;libcxxabi;libunwind;lld;lldb;mlir;openmp;polly;pstl")
 # The flang project is not yet part of "all" projects (see C++ requirements)
 set(LLVM_EXTRA_PROJECTS "flang")
 # List of all known projects in the mono repo