[CMake] Prefer multi-target variables over generic target variables in runtimes build

[JamesNagurne]

Summary

Runtimes variables in a multi-target environment are defined like:

RUNTIMES_target_VARIABLE_NAME
RUNTIMES_target+multi_VARIABLE_NAME

In my case, I have a downstream runtimes cache that does the following:

set(RUNTIMES_${target}+except_LIBCXXABI_ENABLE_EXCEPTIONS ON CACHE BOOL "")
set(RUNTIMES_${target}_LIBCXX_ENABLE_EXCEPTIONS OFF CACHE BOOL "")

I found that I was always getting the 'target' variable value (OFF) in my 'target+except' build, which was unexpected.
This behavior was caused by the loop in llvm/runtimes/CMakeLists.txt that runs through all variable names, adding '-DVARIABLE_NAME=' options to the subsequent external project's cmake command.

The issue is that the loop does a single pass, such that if the 'target' value appears in the cache after the 'target+except' value, the 'target' value will take precedence. I suggest in my change here that the more specific 'target+except' value should take precedence always, without relying on CMake cache ordering.

[phosek]

I'm fine with this change in general, but I'm concerned that no matter what order we choose, this might break users who rely on the opposite order. Maybe it'd be better to modify the logic to skip the second loop if the first loop matches, that way we wouldn't rely on any particular order.

[phosek]

In D71570#1792087, @phosek wrote:

I'm fine with this change in general, but I'm concerned that no matter what order we choose, this might break users who rely on the opposite order. Maybe it'd be better to modify the logic to skip the second loop if the first loop matches, that way we wouldn't rely on any particular order.

I've implemented the proposed idea in D71737.

[JamesNagurne]

In D71570#1792087, @phosek wrote:

I'm fine with this change in general, but I'm concerned that no matter what order we choose, this might break users who rely on the opposite order. Maybe it'd be better to modify the logic to skip the second loop if the first loop matches, that way we wouldn't rely on any particular order.

I think the problem is the behavior wasn't defined at all before, and relied on how cmake defined the variables in the LLVM project cmake cache file.

What I wrote in my runtimes cache file:

• RUNTIMES_target_VARIABLE=123
• RUNTIMES_target+multi_VARIABLE=456

What I got in LLVM's CMakeCache.txt

• RUNTIMES_target+multi_VARIABLE=456
• RUNTIMES_target=VARIABLE=123

What I expected in CMakeCache.txt:

• RUNTIMES_target=VARIABLE=123
• RUNTIMES_target+multi_VARIABLE=456

The result: VARIABLE in the target+multi runtime project was 123 instead of 456

While we might break projects, I believe that this solves the problem of relying on some internal cmake or system behavior (I don't know why the resulting CMakeCache.txt is out-of-order, haven't dug that deep), while allowing users to get whatever behavior they want.

[phosek]

LGTM

[JamesNagurne]

Sadly, I do not have commit access. Could you do the honors @phosek ?

[JamesNagurne]

@phosek or really any other committer, could we get this onto master?

We have the change locally, but think it'd be nice to have it upstreamed so it doesn't get stale.

Thanks!

llvm/runtimes/CMakeLists.txt

if(LLVM_INCLUDE_TESTS)
         list(APPEND ${name}_test_targets ${target_name}-${name})
         list(APPEND test_targets ${target_name}-${name})
       endforeach()
       set(test_targets "${test_targets}" PARENT_SCOPE)
     endif()
 
     get_cmake_property(variableNames VARIABLES)
     foreach(variableName ${variableNames})
-      string(FIND "${variableName}" "RUNTIMES_${name}_" out)
+      string(FIND "${variableName}" "RUNTIMES_${target}_" out)
       if("${out}" EQUAL 0)
-        string(REPLACE "RUNTIMES_${name}_" "" new_name ${variableName})
+        string(REPLACE "RUNTIMES_${target}_" "" new_name ${variableName})
         list(APPEND ${name}_extra_args "-D${new_name}=${${variableName}}")
       endif()
-      string(FIND "${variableName}" "RUNTIMES_${target}_" out)
+    endforeach()
+    foreach(variableName ${variableNames})
+      string(FIND "${variableName}" "RUNTIMES_${name}_" out)
       if("${out}" EQUAL 0)
-        string(REPLACE "RUNTIMES_${target}_" "" new_name ${variableName})
+        string(REPLACE "RUNTIMES_${name}_" "" new_name ${variableName})
         list(APPEND ${name}_extra_args "-D${new_name}=${${variableName}}")
       endif()
     endforeach()
 
     if(NOT RUNTIMES_${name}_LLVM_ENABLE_RUNTIMES)
       string(REPLACE ";" "|" LLVM_ENABLE_RUNTIMES_PASSTHROUGH "${LLVM_ENABLE_RUNTIMES}")
       list(APPEND ${name}_extra_args -DLLVM_ENABLE_RUNTIMES=${LLVM_ENABLE_RUNTIMES_PASSTHROUGH})
     endif()