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

Fri Apr 5 14:11:30 PDT 2019

hubert.reinterpretcast marked 3 inline comments as done.
hubert.reinterpretcast added a comment.

In D58951#1456409 <https://reviews.llvm.org/D58951#1456409>, @peter.smith wrote:

> 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.

That is also my understanding. I also believe that it is working sufficiently today for people using such configurations, or at least sufficiently so that improving the situation was not seen as being urgent.

> - 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.

This is the reason why I produced the patch under review here.

> 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

This patch implements the above approach for `CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN`. The various `*_TEST_TARGET_CFLAGS` variables are picked up by their respective `lit.site.cfg.in` files.

>   - Apply these to the cross compilation options as well.
> - [ ... ]
> 
>   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.

Not having this patch is actively blocking the native build cases that use `CMAKE_C(XX)_COMPILER_EXTERNAL_TOOLCHAIN`. While improving the cross-compiling cases would be nice, I don't think those cases are currently broken.

================
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.
----------------
peter.smith wrote:
> 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.
I'll switch this to `CMAKE_C_COMPILER_EXTERNAL_TOOLCHAIN` since there is such a concern.

================
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.
----------------
peter.smith wrote:
> 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.
> 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.
I concerned with moving forward with such a change, because `COMPILER_RT_TEST_COMPILER` is available as a customization point.

Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D58951