[PATCH] D58951: [compiler-rt][tests] Improve handling with non-default toolchains

Fri Apr 5 09:39:48 PDT 2019

peter.smith added a comment.

I've had a chance to look into this a bit more, although my understanding is far from complete. At the moment I think the patch is too specific to the use-case, I'm not an expert in this area so I welcome a second opinion. I don't want to block this, but I also not confident enough to approve either.

As I understand it compiler-rt is built by make/ninja from CMake derived files so CMake only features such as CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT will affect the make/ninja build files. The tests are run by llvm-lit so we wouldn't expect CMake only features to affect the output unless we convert them to something llvm-lit understands. We've also got a situation where cross-compiling and native tests have different options available to them:

- Cross compiled test runs can use COMPILER_RT_TEST_COMPILER_CFLAGS to pass in --gcc-toolchain=... --sysroot=... duplicating any use of  CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT, but at least it is possible.
- Native test runs always pick up the default host compiler, and the default architecture C-flags and don't have a way (at least as far as I can work out in time available) to pass through --gcc-toolchain.

Possible ways to fix this are:

- Convert the CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT to a form that llvm-lit understands, either by converting them to c-flags or adding support to them in llvm-lit so that they have their own line in lit.common.configured.in
  - Apply these to the cross compilation options as well.
- Allow COMPILER_RT_TEST_COMPILER_CFLAGS to be passed to native builds in addition to the exiting flags. This would mean duplication of CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT on the COMPILER_RT_TEST_COMPILER_CFLAGS but at least it would be consistent.

I would prefer if we could come up with a solution for both cross and native cases, as well as supporting CMAKE_SYSROOT as both this and CMAKE_C(XX)_EXTERNAL_TOOLCHAIN are used when cross-compiling the tests on linux platforms.

================
Comment at: cmake/base-config-ix.cmake:226
+    set(COMPILER_RT_TOOLCHAIN_CFLAGS ${COMPILER_RT_TEST_COMPILER_CFLAGS})
+  elseif(CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN AND COMPILER_RT_TEST_COMPILER_ID MATCHES "Clang")
+    # Use the non-default toolchain specified for the build when compiling tests.
----------------
What would happen if CMAKE_C_COMPILER_EXTERNAL_TOOLCHAIN were set? Is this guaranteed to also set CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN? The compiler-rt builtins don't have any C++ code so I could expect someone just compiling the builtins to naturally use the C version of the flag.

================
Comment at: cmake/config-ix.cmake:192
 macro(get_test_cc_for_arch arch cc_out cflags_out)
   if(ANDROID OR ${arch} MATCHES "arm|aarch64")
     # This is only true if we are cross-compiling.
----------------
hubert.reinterpretcast wrote:
> peter.smith wrote:
> > At a first glance this doesn't look right in the context of a native arm or aarch64 build of compiler-rt. I'll need to check to see what happens natively.
> Thanks; I am not familiar with the builds for those platforms. If there is something to be done that logically belongs with this patch, then we consider it.
I've tried building compiler-rt from a standalone directory on an AArch64 machine. It does indeed go down this path even though it is technically not cross compiling. It does happen to work because COMPILER_RT_TEST_COMPILER defaults to the compiler that built compiler-rt which is clang. However this is not right in general, for example I suspect RiscV32 might wish to be cross-compiled here. I suspect we'll need to test for CMAKE_CROSSCOMPILING here, which may cause some people's builds to fail at first as CMAKE_CROSSCOMPILING is only set when CMAKE_SYSTEM_NAME is set, but at least this is the documented CMake way to detect cross compilation.

I don't think that this needs to be fixed here. It would be best addressed with a separate patch if it became important.

One thing I think it does show is that there isn't any reason to not pass CMAKE_C(XX)_EXTERNAL_TOOLCHAIN through when cross-compiling and the compiler is clang as the expectation is that this is the same clang that built the builtins.

Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58951/new/

https://reviews.llvm.org/D58951