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

[Hubert Tong via Phabricator via llvm-commits]

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

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

> If I've understood this correctly the patch introduces COMPILER_RT_TOOLCHAIN_CFLAGS which we don't expect users to set directly, but instead set -gcc-toolchain if the compiler is clang and CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN is set?


Yes.

> My understanding is that CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN is not respected when cross-compiling compiler-rt for Arm and AArch64. I've always had to pass in --gcc-toolchain manually in the C flags. Is there any reason why we should make a distinction between a native and a cross-compile here?

I am only able to speak to my own use case. There is existing logic for "cross-compile" cases that I cannot speak to (and therefore cannot change with confidence).

> I'd say we were more likely to want --gcc-toolchain when cross-compiling. If that is the case it would be better to find a way of using the existing COMPILER_RT_TEST_CFLAGS? This would also make it a bit less confusing as COMPILER_RT_TOOLCHAIN_CFLAGS is only used in the context of tests and COMPILER_RT_TEST_CFLAGS is where I would expect to see that being used.

`COMPILER_RT_TEST_CFLAGS` is not showing up in my tree; perhaps you meant `COMPILER_RT_UNITTEST_CFLAGS`? As it is, the patch //does// feed the option into `COMPILER_RT_UNITTEST_CFLAGS` (through `COMPILER_RT_TOOLCHAIN_CFLAGS`). Note that `COMPILER_RT_UNITTEST_CFLAGS` does not factor into the various `*_TEST_TARGET_CFLAGS` used for the lit tests; so, adding the option directly into `COMPILER_RT_UNITTEST_CFLAGS` does not produce the desired effect.

>> An issue encountered when preparing this change was that COMPILER_RT_UNITTEST_LINK_FLAGS is dropped in many places, unlike COMPILER_RT_UNITTEST_CFLAGS. This patch also attempts to remove that inconsistency.
> 
> Personally I'd recommend splitting that part out into a separate patch, it will make it a bit easier to untangle which parts of the patch are concerned with the toolchain changes.

Sure, I'll post that as a separate patch that is a parent of this one and refresh this patch to contain only the toolchain changes.



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


================
Comment at: cmake/config-ix.cmake:196
     set(${cc_out} ${COMPILER_RT_TEST_COMPILER})
     set(${cflags_out} ${COMPILER_RT_TEST_COMPILER_CFLAGS})
   else()
----------------
peter.smith wrote:
> At this stage are COMPILER_RT_TEST_COMPILER_CFLAGS the same as COMPILER_RT_TOOLCHAIN_FLAGS?  As Line 222 cmake/base-config-ix.cmake looks to be doing
> 
> ```
> if(COMPILER_RT_TEST_COMPILER_CFLAGS)
>     # COMPILER_RT_TEST_COMPILER_CFLAGS is used for cross-compiling.
>     set(COMPILER_RT_TOOLCHAIN_CFLAGS ${COMPILER_RT_TEST_COMPILER_CFLAGS})
> ```
> If that is the case then append to flags_out is the same on both branches.
Yes, that's right; however, using `COMPILER_RT_TEST_COMPILER_CFLAGS` alongside the similarly named `COMPILER_RT_TEST_COMPILER` is perhaps beneficial in terms of not requiring extra analysis for the casual consumer of the code. Also, the block of code here was otherwise untouched.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D58951