This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Use COMPILER_RT_BUILD_CRT in the condition for test

Authored by phosek on Jul 12 2023, 3:04 PM.



Unlike COMPILER_RT_HAS_CRT this handles the case where CRT is available
but has been disabled by setting COMPILER_RT_BUILD_CRT. This addresses
an issue reported on D153989.

Diff Detail

Event Timeline

phosek created this revision.Jul 12 2023, 3:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2023, 3:04 PM
Herald added subscribers: ekilmer, Enna1. · View Herald Transcript
phosek requested review of this revision.Jul 12 2023, 3:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2023, 3:04 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
smeenai accepted this revision.Jul 12 2023, 3:10 PM
smeenai added inline comments.

HAS_CRT is the internal check and BUILD_CRT is user-configurable, right? Could we simplify this by ensuring that BUILD_CRT is always false is HAS_CRT is false (perhaps erroring out if the user explicitly asked it to be built when it's unavailable)?

This revision is now accepted and ready to land.Jul 12 2023, 3:10 PM
phosek added inline comments.Jul 12 2023, 3:15 PM

That's what cmake_dependent_option on the line above was intended achieve. The problem is that there are some LLVM bots that use incremental builds and have COMPILER_RT_BUILD_CRT=ON in their CMake cache. I was hoping to force a clean build on these bots, but it looks like the Buildbot login is currently broken :(

smeenai added inline comments.Jul 12 2023, 3:20 PM

Ah, that's subtle. If I'm reading correctly though, we'd overwrite the cache value in this file by creating a local variable. That wouldn't help test/builtins/CMakeLists.txt though.

phosek added inline comments.Jul 12 2023, 3:25 PM

A potential alternative would be to add unset(COMPILER_RT_BUILD_CRT CACHE) above cmake_dependent_option with TODO and then delete in in a follow up change. WDYT?

smeenai added inline comments.Jul 12 2023, 3:27 PM

Yeah, I think that's cleaner and conveys the issue better.

phosek updated this revision to Diff 539768.Jul 12 2023, 3:30 PM
phosek marked 3 inline comments as done.
This revision was landed with ongoing or failed builds.Jul 12 2023, 3:30 PM
This revision was automatically updated to reflect the committed changes.
arichardson added inline comments.

This also appears to unset COMPILER_RT_BUILD_CRT when set on the command line. I was surprised to see the CRT builds being run for my builtins-only build config (I noticed because one of the tests was failing despite having REQUIRES: crt.

I pass -DCOMPILER_RT_BUILD_CRT=OFF to cmake. Adding some prints around this statement shows the following:

-- before unset(): COMPILER_RT_BUILD_CRT=OFF
-- after unset: COMPILER_RT_BUILD_CRT=
-- after cmake_dependent_option COMPILER_RT_BUILD_CRT=ON