[CMake] llvm/runtimes: Do not override LLVM_* variables with just-built LLVM configurations

[JamesNagurne]

Summary

In the projects generated by llvm/runtimes/CMakeLists.txt, using that same
file, there is a find_package for LLVM. This imports the just-built
toolchain's configurations. Among these configurations is LLVM_ENABLE_PIC.

A runtimes configuration might want to build the toolchain with PIC and
build the runtimes without PIC, but the aforementioned behavior will always
prefer the LLVM's configuration of using PIC. I suggest that this is
undesirable behavior, and the overrides sent into the runtime configuration
via 'RUNTIMES_<target>_LLVM_ENABLE_PIC' should take precedence.

To that end, I've added a loop after the find_package which reinforces
the definition of variables found in the cache. Should LLVM_ENABLE_PIC
be in that cache, its value will take precedence over the just-build
toolchain.

[phosek]

This looks reasonable to me, @smeenai & @beanz do you see any problem with this?

[JamesNagurne]

In D72947#1850273, @phosek wrote:

This looks reasonable to me, @smeenai & @beanz do you see any problem with this?

I'm not really married to the solution, to be honest. CACHE_VARIABLES is not intended to be used by an end-user.
However, this is the easiest/least verbose way to guarantee that LLVM_* configurations make it through to the runtimes build.

A different solution would be to generalize HandleLLVMOptions to take an alternate namespace to search for variables in, such that LLVM_ENABLE_PIC may be ON, but perhaps RUNTIMES_ENABLE_PIC is OFF, indicating correctly that LLVM was built with PIC and the runtimes will not be. That's a much more serious undertaking that I don't believe I have time for at this point.

[smeenai]

Seems reasonable to me.

An alternative solution would be to change the generated LLVMConfig such that it checks if certain variables are already set before overriding them, but I'm not completely sure if that makes sense to do.

[smeenai]

Anything holding this up?

[smeenai]

Giving an explicit accept to remove this from my review queue, since @phosek also expressed approval above.

[JamesNagurne]

When @phosek gets back to it, one of you would need to commit it on my behalf.

We've been using this downstream with no issues (thus the lack of enthusiasm to get this upstreamed on my end), but our use-case is small since we have a single target and a narrow breadth of build environments.

[JamesNagurne]

Actually, it's very fortuitous that you reminded of this review today.

There's an issue here, and I don't think I'd commit this upstream without understanding more.

LLVM recently moved towards Python3 and cmake updates. This means using find_package(Python) in some places
To save time, the python package search sets _Python_INTERPRETER_PROPERTIES, which is a list of information found about an interpreter. This ends up being a cache variable.

It's causing problems on our end when working with Windows builds with Python 3.8.2 and cmake 3.17

Do not commit this unless I can get a patch up.

[smeenai]

Thanks for the info. Requesting changes so that it doesn't get accidentally committed.

[JamesNagurne]

The problem here is the method we reset the variables in the cache back to their cached value.

set(VAR1 "A;;B;;C")
message(STATUS "VAR1 ${VAR1}")
set(VAR2 ${VAR1})
message(STATUS "VAR2 ${VAR2}")
set(VAR3 "${VAR1})
message(STATUS "VAR3 ${VAR3}")

Emits the following:
VAR1 A;;B;;C
VAR2 A;B;C
VAR3 A;;B;;C

This confuses me, but it seems to be the way cmake does things. A;;B;;C is a 5 element list, and when used as a raw list, empty elements are deleted.
The reason there is an issue here is because in some work that has happened since I've uploaded this changeset, we now utilize find_package(Python) where possible instead of include(FindPythonInterpreter).
The former sets an internal cache variable, _{PYTHON_PREFIX}_INTERPRETER_PROPERTIES, to a list of properties as a cache, and then indexes into it (https://github.com/Kitware/CMake/blob/v3.17.2/Modules/FindPython/Support.cmake#L1299), when called a second time.

My code as writtten will delete empty elements in this list, and cause cmake to error out.
The solution on our end is to set the variable as you see above for VAR3. This will preserve the empty elements, and doesn't seem to change the behavior of special values like ON/OFF or integers found in other cache variables.

However, I feel uncomfortable with having this particular solution committed to the upstream due to the dubious nature of using CACHE_VARIABLES and with my relative uncertainty of cmake semantics.
I'll retract this for now.

[beanz]

Sorry for being late to the party on this.

There's really only a handful of variables in LLVMConfig.cmake that can actually be different between LLVM and the project using LLVM. Might it make sense to just handle those as special? We already do handle LLVM_PTHREAD_LIB specially for exactly the same reason you describe in the description.

[JamesNagurne]

In D72947#2047140, @beanz wrote:

Sorry for being late to the party on this.

There's really only a handful of variables in LLVMConfig.cmake that can actually be different between LLVM and the project using LLVM. Might it make sense to just handle those as special? We already do handle LLVM_PTHREAD_LIB specially for exactly the same reason you describe in the description.

I agree that it's probably the best general solution. I missed LLVM_PTHREAD_LIB, but are you talking about LLVMConfig.cmake.in? That's where I can see the variable being set differently than the others.

if (NOT "@LLVM_PTHREAD_LIB@" STREQUAL "")
  set(LLVM_PTHREAD_LIB "@LLVM_PTHREAD_LIB@")
endif()

However, if LLVM_PTHREAD_LIB is set by the LLVM configuration (the just-built compiler), then the 'if' condition will always be true and we'd always override whatever configuration the runtimes build would want to do.
Indeed, if I set my runtimes cache to have "RUNTIMES_LLVM_PTHREAD_LIB=THISISANERROR", then it gets overridden by find_package(LLVM ...) in llvm/runtimes/CMakeLists.txt to be '-lpthread'.

llvm/runtimes/CMakeLists.txt

function(runtime_register_component name)
     set_property(GLOBAL APPEND PROPERTY SUB_COMPONENTS ${name})
   endfunction()
 
   cmake_minimum_required(VERSION 3.4.3)
   project(Runtimes C CXX ASM)
 
   find_package(LLVM PATHS "${LLVM_BINARY_DIR}" NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH)
 
+  # find_package may have overridden some cache variables which ought not be
+  # overridden for a build using the just-built toolchain. Revert them.
+  get_cmake_property(_variableNames CACHE_VARIABLES)
+  foreach (_variableName ${_variableNames})
+      get_property(original_value CACHE ${_variableName} PROPERTY VALUE)
+      set(${_variableName} ${original_value})
+  endforeach()
+
   # Add the root project's CMake modules, and the LLVM build's modules to the
   # CMake module path.
   list(INSERT CMAKE_MODULE_PATH 0
     "${CMAKE_CURRENT_SOURCE_DIR}/../cmake"
     "${CMAKE_CURRENT_SOURCE_DIR}/../cmake/modules"
   )
 
   # Some of the runtimes will conditionally use the compiler-rt sanitizers