This is an archive of the discontinued LLVM Phabricator instance.

[builtins] Make sure builtin compile tests respect CMAKE_C_COMPILER_TARGET
ClosedPublic

Authored by fjricci on Aug 23 2016, 5:50 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

fjricci updated this revision to Diff 69062.Aug 23 2016, 5:50 PM
fjricci updated this revision to Diff 69063.
fjricci retitled this revision from to [builtins] Make sure builtin compile tests respect CMAKE_C_COMPILER_TARGET.
fjricci updated this object.
fjricci added reviewers: beanz, compnerd.
fjricci added a subscriber: llvm-commits.

Improved cmake syntax

compnerd accepted this revision.Aug 23 2016, 9:54 PM
compnerd edited edge metadata.
compnerd added inline comments.
cmake/Modules/BuiltinTests.cmake
17 ↗(On Diff #69063)

I think I prefer the following syntax:

list(APPEND TRY_COMPILE_FLAGS -target;${CMAKE_C_COMPILER_TARGET})
This revision is now accepted and ready to land.Aug 23 2016, 9:54 PM
fjricci updated this revision to Diff 69134.Aug 24 2016, 10:13 AM
fjricci edited edge metadata.

Update cmake style and check compiler ID

beanz edited edge metadata.Aug 24 2016, 11:55 AM

CMAKE_C_COMPILER_TARGET is an oddly used option. CMake only uses it in two places, one is in the tests, and the other is in the logic for finding the compiler. Setting CMAKE_C_COMPILER_TARGET and not explicitly overriding your compiler to clang, could result in undesirable behavior on systems that have GCC cross-compilers installed, so I'm a little concerned about using this option. It might make more sense to use COMPILER_RT_DEFAULT_TARGET_TRIPLE instead.

cmake/Modules/BuiltinTests.cmake
17 ↗(On Diff #69134)

list(APPEND...) is better, but don't put a semi-colon between the elements, a space will suffice. Using a semi-colon is odd because instead of appending a list to a list you're appending a string-ified list.

In order for cmake to check whether Clang can cross-compile for a given target, -target <target> needs to be set. One could do this either by explicitly adding it to CMAKE_<LANG>_FLAGS or by setting CMAKE_<LANG>_COMPILER_TARGET. The problem with using COMPILER_RT_DEFAULT_TARGET_TRIPLE is that this won't set -target.

A further complication is that COMPILER_RT_DEFAULT_TARGET_TRIPLE and the desired -target option aren't always the same. For example, to build for linux-armhf, COMPILER_RT_DEFAULT_TARGET_TRIPLE expects armhf-linux-gnueabihf, but you need to build with -target armv7-linux-gnueabihf, so there isn't a 1:1 mapping.

Presumably this could be fixed by reworking COMPILER_RT_DEFAULT_TARGET_TRIPLE, but that's more involved, and this seemed like a simpler solution.

fjricci updated this revision to Diff 69179.Aug 24 2016, 3:41 PM
fjricci edited edge metadata.

Remove semi-colon in list append

beanz accepted this revision.Aug 24 2016, 3:50 PM
beanz edited edge metadata.

You’re right. My grep of CMake's sources missed the generator uses where it adds --target to the compiler invocations. I completely understand why you're using it this way.

Patch LGTM.

This revision was automatically updated to reflect the committed changes.