Page MenuHomePhabricator

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

Authored by JamesNagurne on Jan 17 2020, 12:38 PM.

Details

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.

Diff Detail

Event Timeline

JamesNagurne created this revision.Jan 17 2020, 12:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2020, 12:38 PM

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

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.

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.

Anything holding this up?

smeenai accepted this revision.May 13 2020, 2:55 PM

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

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.

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 requested changes to this revision.May 13 2020, 4:26 PM

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

This revision now requires changes to proceed.May 13 2020, 4:26 PM
JamesNagurne abandoned this revision.May 15 2020, 9:44 AM

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.

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.

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'.