Page MenuHomePhabricator

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

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


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

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.