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

Mon Apr 8 05:41:59 PDT 2019

peter.smith added a comment.

In D58951#1456888 <https://reviews.llvm.org/D58951#1456888>, @hubert.reinterpretcast wrote:

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

I think once you know what to you need to do it suffices, however finding out how to do it is more difficult. I think people have largely been reliant on the mailing list for guidance.
...

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

In the suggestion I was thinking more along the lines of a separate toolchain and sysroot rather than passing them as C Flags, something like

  set_default("gcc_toolchain", "@GCC_TOOLCHAIN@")
  set_default("sysroot", "@SYSROOT") 

However I'm not really sure that adds much value other than matching CMake a bit more closely. I guess it would hand off the decision over what to do with the flags to lit, which may have more of an idea of what to do with them.

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

I agree with you that the native build is not supporting external_toolchain. It is also true that cross compiling can use an alternative method to work around it.

My main concerns are:

- We're adding another specific case to just the native part making the CMake files harder to understand, where there is an opportunity to support both native and cross. I may be alone here, I'd welcome alternative opinions from others to check?
- To me the name COMPILER_RT_TOOLCHAIN_CFLAGS matches the --gcctoolchain option well. However it doesn't intuitively say "merged flags from COMPILER_RT_TEST_COMPILER_FLAGS", the name is too specific. Having both native and cross use COMPILER_RT_TEST_COMPILER_FLAGS would be a way to fix this. Alternatives could be picking a better name that suites both, or perhaps adding both and not merging the flags.

  I care about the merging of the COMPILER_RT_TEST_COMPILER_CFLAGS into a name like COMPILER_RT_TOOLCHAIN flags more than I care about both cross and native being the same. If I'm the only one that cares about the equivalence of cross and native I'd like to see if we can resolve the naming of COMPILER_RT_TOOLCHAIN.

Apologies for rambling on. I'm at Eurollvm at the moment, if you happen to be here, I'm happy to talk about it.

Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D58951