Parse the triple in base-config-ix to propagate ANDROID variable correctly

[sgundapa]

Summary

The triple must be parsed in base-config-ix.cmake.
Otherwise, the cmake variable ANDROID won't be set and this
will confuse cmake to build unsupported targets targeted for
android.

[sgundapa]

For most of the targets android and linux variants have the same components supported. For aarch64-android this is not the case

[apazos]

LGTM, can this be merged to unblock work?

[rengolin]

We need someone with a little more build experience in RT... I'll try to add a few folks.

[rengolin]

This seems like just a code move.

Can you quickly explain what was before and what it now, and why the move "fixes" whatever was broken?

[sgundapa]

The cmake variable ANDROID is set based on triple specified for build. With the base-config-ix restructure, the usage of ANDROID is before we parse the triple. With this problem, even for an android triple all the components that are available for linux are built for android too. This holds well for matured targets like X64, ARM but for AArch64 fails.

[beanz]

This code may need to go into the base-config-ix so that it is exposed to the builtin build too.

I don't really have a problem with this approach, and I don't really understand the Android build path well enough to be an authority here, but I'm a little skeptical about Android needing a build path that is separate from other Linux based platforms.

Darwin is separate largely because we generate multi-architecture builds, and that special snowflake kinda needs to die. I really hope we can find ways to reduce the number of different ways we configure compiler-rt.

[sgundapa]

I agree. I will move the code to base-config (not going to change the behavior but will keep the flow intact)

I don't really have a problem with this approach, and I don't really understand the Android build path well enough to be an authority here, but I'm a little skeptical about Android needing a build path that is separate from other Linux based platforms.

Here is the existing code in compiler-rt that demonstrates the differentiation between linux and android runtime components
if(ANDROID)

set(OS_NAME "Android")

else()

set(OS_NAME "${CMAKE_SYSTEM_NAME}")

endif()

if (COMPILER_RT_HAS_SANITIZER_COMMON AND DFSAN_SUPPORTED_ARCH AND

  OS_NAME MATCHES "Linux")
set(COMPILER_RT_HAS_DFSAN TRUE)

else()

set(COMPILER_RT_HAS_DFSAN FALSE)

endif()

[compnerd]

I believe that the comments from @beanz have been addressed. The change itself looks fine.

cmake/base-config-ix.cmake

75 ↗

(On Diff #61974)

This isn't really your fault, but this is really rather fragile. It looks like it would break if I do something like:

armv7-unknown-linux-androideabi

It would parse linux as the ABI rather than eabi. Also, the last component androideabi is the environment, not the ABI.

[beanz]

There is actually a problem with this patch that I didn't spot earlier. It depends on TARGET_TRIPLE being set. That comes from LLVM, so this actually can't be in base-config-ix.cmake because that file needs to work without dependencies on the LLVM build tree.

I'm a little unsure about this patch in general though. The premise seems to be that ANDROID needs to be defined before base-config-ix because it is used there, but it isn't. The only reference to ANDROID in base-config-ix is in the test_targets macro, which shouldn't be evaluated at include time, so I don't understand the problem.

I also still don't fundamentally understand why Android's target detection is different from Linux's. @sgundapa, your comment with the listed differences between Android and Linux is incomplete. The root of this patch is based around the fact that Android and Linux's target detection is different. I still don't fundamentally understand that. Simply put, I don't understand why base-config.ix has any use of the ANDROID variable.

[compnerd]

In D21474#468340, @beanz wrote:

There is actually a problem with this patch that I didn't spot earlier. It depends on TARGET_TRIPLE being set. That comes from LLVM, so this actually can't be in base-config-ix.cmake because that file needs to work without dependencies on the LLVM build tree.

I'm a little unsure about this patch in general though. The premise seems to be that ANDROID needs to be defined before base-config-ix because it is used there, but it isn't. The only reference to ANDROID in base-config-ix is in the test_targets macro, which shouldn't be evaluated at include time, so I don't understand the problem.

I also still don't fundamentally understand why Android's target detection is different from Linux's. @sgundapa, your comment with the listed differences between Android and Linux is incomplete. The root of this patch is based around the fact that Android and Linux's target detection is different. I still don't fundamentally understand that. Simply put, I don't understand why base-config.ix has any use of the ANDROID variable.

I think I can explain the bit on why Android is a special snowflake here. Android isn't an OS, but an environment. The OS is Linux, the environment is Android rather than GNU or musl or uclibc (yes, you can have armv7-unknown-linux-uclibceabihf!) So, the target needs to be processed specially to determine if the environment is android and if so, behave differently. Im not saying that this is pretty or good, merely what the current state of affairs are.

[sgundapa]

I think @compnerd explained the oddity with android builds. @beanz , agreed things are not pretty in this zone. My patch is to just restore the earlier state.

[sgundapa]

FYI, I committed this change some time back.

[beanz]

I have reverted your patch in r273956, and put in the fix I suggested in r273957. Please ensure this meets your needs.

-Chris

[sgundapa]

@beanz The change is very disruptive I believe.

set(COMPILER_RT_OS_SUFFIX "-android")
Right now I am building both linux and android components and one way to differentiate them is through "-android" prefix.

Also, there is the usage of
if(ANDROID)
in base-config-ix.cmake and ANDROID variable is set later.

[compnerd]

@sgundapa unfortunately, your patch broke the buildbots. If the change that @beanz graciously put in is insufficient, perhaps you can come up with an alternative patch that can be merged to address the issue that you are seeing. BTW, how are you building the library?

[sgundapa]

I can build with -DANROID=1, but I need the "-android" OS suffix for libraries

[sgundapa]

Realized that ANDROID usage is inside a macro in base-config. Thanks to @beanz
r274030 fixed the OS suffix change.

CMakeLists.txt

 cmake_policy(SET CMP0056 OLD)
 
 # Add path for custom compiler-rt modules.
 list(INSERT CMAKE_MODULE_PATH 0
   "${CMAKE_CURRENT_SOURCE_DIR}/cmake"
   "${CMAKE_CURRENT_SOURCE_DIR}/cmake/Modules"
   )
 
+set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${TARGET_TRIPLE} CACHE STRING
+    "Default triple for which compiler-rt runtimes will be built.")
+if(DEFINED COMPILER_RT_TEST_TARGET_TRIPLE)
+  # Backwards compatibility: this variable used to be called
+  # COMPILER_RT_TEST_TARGET_TRIPLE.
+  set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${COMPILER_RT_TEST_TARGET_TRIPLE})
+endif()
+
+string(REPLACE "-" ";" TARGET_TRIPLE_LIST ${COMPILER_RT_DEFAULT_TARGET_TRIPLE})
+list(GET TARGET_TRIPLE_LIST 0 COMPILER_RT_DEFAULT_TARGET_ARCH)
+list(GET TARGET_TRIPLE_LIST 1 COMPILER_RT_DEFAULT_TARGET_OS)
+list(GET TARGET_TRIPLE_LIST 2 COMPILER_RT_DEFAULT_TARGET_ABI)
 include(base-config-ix)
 
 option(COMPILER_RT_BUILD_BUILTINS "Build builtins" ON)
 mark_as_advanced(COMPILER_RT_BUILD_BUILTINS)
 option(COMPILER_RT_BUILD_SANITIZERS "Build sanitizers" ON)
 mark_as_advanced(COMPILER_RT_BUILD_SANITIZERS)
 
 if (COMPILER_RT_STANDALONE_BUILD)
if (COMPILER_RT_STANDALONE_BUILD)
   # Define default arguments to lit.
   set(LIT_ARGS_DEFAULT "-sv")
   if (MSVC OR XCODE)
     set(LIT_ARGS_DEFAULT "${LIT_ARGS_DEFAULT} --no-progress-bar")
   endif()
   set(LLVM_LIT_ARGS "${LIT_ARGS_DEFAULT}" CACHE STRING "Default options for lit")
 endif()
 
-set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${TARGET_TRIPLE} CACHE STRING
-    "Default triple for which compiler-rt runtimes will be built.")
-if(DEFINED COMPILER_RT_TEST_TARGET_TRIPLE)
-  # Backwards compatibility: this variable used to be called
-  # COMPILER_RT_TEST_TARGET_TRIPLE.
-  set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${COMPILER_RT_TEST_TARGET_TRIPLE})
-endif()
-
-string(REPLACE "-" ";" TARGET_TRIPLE_LIST ${COMPILER_RT_DEFAULT_TARGET_TRIPLE})
-list(GET TARGET_TRIPLE_LIST 0 COMPILER_RT_DEFAULT_TARGET_ARCH)
-list(GET TARGET_TRIPLE_LIST 1 COMPILER_RT_DEFAULT_TARGET_OS)
-list(GET TARGET_TRIPLE_LIST 2 COMPILER_RT_DEFAULT_TARGET_ABI)
 # Determine if test target triple is specified explicitly, and doesn't match the
 # default.
 if(NOT COMPILER_RT_DEFAULT_TARGET_TRIPLE STREQUAL TARGET_TRIPLE)
   set(COMPILER_RT_HAS_EXPLICIT_DEFAULT_TARGET_TRIPLE TRUE)
 else()
   set(COMPILER_RT_HAS_EXPLICIT_DEFAULT_TARGET_TRIPLE FALSE)
 endif()
 include(CompilerRTUtils)