This is an archive of the discontinued LLVM Phabricator instance.

[test-suite] Use CMAKE_C_COMPILER as TEST_SUITE_HOST_CC when compiling test-suite tools.
ClosedPublic

Authored by amyk on Dec 3 2021, 7:41 AM.

Details

Summary

Originally the test-suite tools are configured to compile with cc by default.
This patch updates the the default tool to instead by ${CMAKE_C_COMPILER} that is detected.

Diff Detail

Repository
rT test-suite

Event Timeline

amyk created this revision.Dec 3 2021, 7:41 AM
amyk requested review of this revision.Dec 3 2021, 7:41 AM
amyk added a comment.Dec 3 2021, 7:44 AM

Hi @Meinersbur,

I put up this patch as when trying to configure and build test-suite on an AIX system, I was not able to use the default cc on the machine to compile the test-suite tools.
I figured the proper solution would probably be to use ${CMAKE_C_COMPILER}, and perhaps this was the original intention, but are you able to confirm if this is the proper thing to do? Was there any justification for doing just cc in the first place?

Thanks.

Meinersbur requested changes to this revision.Dec 7 2021, 7:47 AM

CMAKE_C_COMPILER does not work here in all cases. In particular, when cross-compiling using gcc. For instance, I would like to cross-compile to an embedded ARM board on my x86_64 desktop. I'd configure CMake to aarch64-linux-gnu-gcc as CMAKE_C_COMPILER. Hence that llvm_add_host_executable would compile to aarch64, not my x86_64 host.

TEST_SUITE_HOST_CC is a cmake configuration parameter, the idea is that you change it to something else if your host compiler is not cc. You could detect whether CMAKE_C_COMPILER is clang and if so, use that compiler has default host compiler since it uses -target to cross-compile.

This revision now requires changes to proceed.Dec 7 2021, 7:47 AM
amyk added a comment.Dec 8 2021, 6:51 AM

CMAKE_C_COMPILER does not work here in all cases. In particular, when cross-compiling using gcc. For instance, I would like to cross-compile to an embedded ARM board on my x86_64 desktop. I'd configure CMake to aarch64-linux-gnu-gcc as CMAKE_C_COMPILER. Hence that llvm_add_host_executable would compile to aarch64, not my x86_64 host.

TEST_SUITE_HOST_CC is a cmake configuration parameter, the idea is that you change it to something else if your host compiler is not cc. You could detect whether CMAKE_C_COMPILER is clang and if so, use that compiler has default host compiler since it uses -target to cross-compile.

I didn't realize that was an issue, so I appreciate it for elaborating on that.

I also wanted to check with you, but regarding the detection for clang, is the following what you meant by that? Here, we're just checking if we have clang, and we set it accordingly.

diff --git a/cmake/modules/Host.cmake b/cmake/modules/Host.cmake
index ef81deeb..a052f13e 100644
--- a/cmake/modules/Host.cmake
+++ b/cmake/modules/Host.cmake
@@ -1,6 +1,11 @@
 set(TEST_SUITE_HOST_CC "cc" CACHE STRING "C compiler targetting the host")
 mark_as_advanced(TEST_SUITE_HOST_CC)

+get_filename_component(C_COMPILER_NAME ${CMAKE_C_COMPILER} NAME)
+if(${C_COMPILER_NAME} STREQUAL "clang")
+  set(TEST_SUITE_HOST_CC ${CMAKE_C_COMPILER})
+endif()
+
 function(llvm_add_host_executable targetname exename)
   cmake_parse_arguments(_arg "" "" "LDFLAGS;CPPFLAGS" ${ARGN})

Use CMAKE_C_COMPILER_ID for detection.
The additional set(TEST_SUITE_HOST_CC ${CMAKE_C_COMPILER}) would ignore a user-configured host compiler. Instead, provide a default option like this (untested):

set(_host_cc_default "cc")
if (CMAKE_C_COMPILER_ID STREQUALS "Clang")
  # If using Clang we can use executable for the host even if CMake is configured to cross-compile.
  set(_host_cc_default "${CMAKE_C_COMPILER}")
endif ()
set(TEST_SUITE_HOST_CC "${_host_cc_default}" CACHE STRING "C compiler targeting the host")
amyk updated this revision to Diff 393005.Dec 9 2021, 6:28 AM

Updated the revision to use CMAKE_C_COMPILER_ID as suggested by @Meinersbur.
This approach will set TEST_SUITE_HOST_CC as clang is the CMAKE_C_COMPILER is also clang. It also respects if I set TEST_SUITE_HOST_CC in CMake.

This revision is now accepted and ready to land.Dec 9 2021, 8:54 AM
amyk added a comment.Dec 9 2021, 10:15 AM

LGTM

I also wanted to update that I've checked and configured/built with gcc, as well. I printed out ${TEST_SUITE_HOST_CC}, and the behaviour is what we expect: cc is still used when the compiler is gcc.
I'll commit this patch shortly today. Thanks for reviewing and suggesting the change.

Hi @amyk, this seems to have broken our public CTMark builders. Can you take a look? Thanks!

https://green.lab.llvm.org/green/job/lnt-ctmark-aarch64-O0-g/11207/

amyk added a comment.Dec 16 2021, 10:30 PM

Hi @amyk, this seems to have broken our public CTMark builders. Can you take a look? Thanks!

https://green.lab.llvm.org/green/job/lnt-ctmark-aarch64-O0-g/11207/

Hi @paquette,
Thank you for informing me of this. I did not realize this patch caused the CTMark builders to break, and I sincerely apologize for this.
I'm currently away for the holidays at the moment, and do not have the time currently to investigate in depth. I can revert the patch in the meantime, and resume the investigation when I am back.

There is probably something I am not considering with respect to this patch. Briefly scanning through the logs, I see missing headers from clang:

. . . 
[  5%] [TEST_SUITE_HOST_CC] Compiling host source timeit.c
cd /Users/buildslave/jenkins/workspace/lnt-ctmark-aarch64-O0-g/lnt-sandbox/build/tools && /Users/buildslave/jenkins/workspace/lnt-ctmark-aarch64-O0-g/compiler/bin/clang -c /Users/buildslave/jenkins/workspace/lnt-ctmark-aarch64-O0-g/test-suite/tools/timeit.c -o /Users/buildslave/jenkins/workspace/lnt-ctmark-aarch64-O0-g/lnt-sandbox/build/tools/timeit.c.o
/Users/buildslave/jenkins/workspace/lnt-ctmark-aarch64-O0-g/test-suite/tools/timeit.c:10:10: fatal error: 'stdio.h' file not found
#include <stdio.h>
         ^~~~~~~~~
1 error generated.

. . . 

[  5%] [TEST_SUITE_HOST_CC] Compiling host source fpcmp.c
cd /Users/buildslave/jenkins/workspace/lnt-ctmark-aarch64-O0-g/lnt-sandbox/build/tools && /Users/buildslave/jenkins/workspace/lnt-ctmark-aarch64-O0-g/compiler/bin/clang -c /Users/buildslave/jenkins/workspace/lnt-ctmark-aarch64-O0-g/test-suite/tools/fpcmp.c -o /Users/buildslave/jenkins/workspace/lnt-ctmark-aarch64-O0-g/lnt-sandbox/build/tools/fpcmp.c.o
/Users/buildslave/jenkins/workspace/lnt-ctmark-aarch64-O0-g/test-suite/tools/fpcmp.c:13:10: fatal error: 'ctype.h' file not found
#include <ctype.h>
         ^~~~~~~~~
1 error generated.

Above in the log, I see compilations with clang, but they include extra flags - I'm not sure if there is anything with respect to extra flags that I am missing for this patch at the moment.

/Users/buildslave/jenkins/workspace/lnt-ctmark-aarch64-O0-g/compiler/bin/clang   -B /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin    -O0 -g -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.5.sdk -Wl,-search_paths_first -Wl,-headerpad_max_install_names   -B /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin   CMakeFiles/fpcmp-target.dir/fpcmp.c.o  -o fpcmp-target

Thanks, @amyk!

I hit this on my local iMac as well. It looks like the host compiler needs to be pointed at the macOS or XcodeDefault SDK somehow.

For the target fpcmp build, everything works. The extra flags tell it where to look for headers + binaries.

-B /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin  <- look in the XcodeDefault toolchain for binaries

-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.5.sdk <- set the system root directory to the iPhoneOS13.5 SDK.

These flags come from a CMake cache in the test suite.

For the host build, something similar needs to be threaded through. Right now, it doesn't know where to look for the headers, resulting in the failure. I'm not sure of what the best way to do that is. In this specific case, I'd expect the host compiler to look at the macOS SDK.

@cmatthews do you have any thoughts on what the best approach here is?

amyk added a comment.Jan 6 2022, 5:54 AM

Thanks for the insight into the problem on the macOS side, @paquette.
@Meinersbur I was also wondering if you also have any thoughts regarding this issue?