[compiler-rt] [cmake] Remove i686 target that is duplicate to i386

[mgorny]

Summary

Remove the explicit i686 target that is completely duplicate to
the i386 target, with the latter being used more commonly.

1. The runtime built for i686 will be identical to the one built for

i386.

2. Supporting both -i386 and -i686 suffixes causes unnecessary confusion

on the clang end which has to expect either of them.

3. The checks are based on wrong assumption that i686 is defined for

all newer x86 CPUs. In fact, it is only declared when -march=i686 is
explicitly used. It is not available when a more specific (or newer)
-march is used.

Curious enough, if CFLAGS contain -march=i686, the runtime will be built
both for i386 and i686. For any other value, only i386 variant will be
built.

[mgorny]

Ping.

[beanz]

Looping in @eugenis, who added i686 support in r218761.

eugenis, since i686 is identical to i386 generating it as a separate target is undesirable. Can you help advise as to how we can better meet your needs and not duplicate an effectively identical target?

[mgorny]

In D26764#606638, @beanz wrote:

Looping in @eugenis, who added i686 support in r218761.

eugenis, since i686 is identical to i386 generating it as a separate target is undesirable. Can you help advise as to how we can better meet your needs and not duplicate an effectively identical target?

I suspect D26796 attempts to solve the same problem.

[eugenis]

I think this is the right move together with D26796.
Android build system will need to support both names for some time, depending on the toolchain version - looks doable. Technically, it can stick with the old name forever.

[mgorny]

Ping. @eugenis, do you consider it good to go then? Or do you want me to change something specifically?

[eugenis]

LGTM

[mgorny]

I'm going to commit this now to see if it helps with the Android buildbot. Updating to the rebased patch that's going to be committed.

[mgorny]

I have reverted the changes for now to fix Android.

[hans]

compiler-rt/trunk/lib/asan/scripts/asan_device_setup

98 ↗

(On Diff #112955)

There are lots of copies of this script in various project that now all need to be updated (searching the web for asan_device_setup shows some).

For me personally, this means updating Chromium (also the build system needs an update) to handle different names before and after this revision.

Do you have any suggestions for how we should handle this rename? Is there some backwards-compatibility fix we could do?

As you noticed this broke some LLVM buildbots. It also broke Chromium buildbots, and I suppose other projects which use asan. I'm wondering if the fallout is worth the benefits here.

[eugenis]

I may have underestimated the number of places that will need to be patched because of this change. Perhaps we should restore the special case in the android library name?

[mgorny]

I'm happy to help with whatever needs to be done to keep breakage to the minimum, provided that we determine some clear path forward that doesn't involve regressing to the half-broken state for non-Android Linux systems and it's doable in the free time I can spare. However, I should point out that it was a very bad idea to hardcode paths all over the projects in the first place.

[eugenis]

IMHO it was a very bad idea to include the "architecture name" (not even a target triple!) in the library path in the first place. No one else does that. GCC made the right choice and called theirs "libasan.so".

My plan is to tweak the code that sets the library name to use i686 when the target is i386 and the os is android, both in compiler-rt and in clang. There will be no duplicate targets.

[hans]

In D26764#855791, @eugenis wrote:

IMHO it was a very bad idea to include the "architecture name" (not even a target triple!) in the library path in the first place. No one else does that. GCC made the right choice and called theirs "libasan.so".

My plan is to tweak the code that sets the library name to use i686 when the target is i386 and the os is android, both in compiler-rt and in clang. There will be no duplicate targets.

Sounds good to me. Let me know if you think this will take a while and we should revert to green in the meantime.

[eugenis]

I've landed the android special case in r312048

[rnk]

In D26764#855791, @eugenis wrote:

IMHO it was a very bad idea to include the "architecture name" (not even a target triple!) in the library path in the first place. No one else does that. GCC made the right choice and called theirs "libasan.so".

+1, one day we should untangle the compiler-rt library names.

My plan is to tweak the code that sets the library name to use i686 when the target is i386 and the os is android, both in compiler-rt and in clang. There will be no duplicate targets.

[elram]

This patch breaks cross-compiling the builtins for i686, with COMPILER_RT_DEFAULT_TARGET_ONLY=YES.
CC is set to

clang -target i686-linux-musl -m32 -march=i686

But no supported target is detected by cmake and nothing gets built.

-- Builtin supported architectures:

cmake/Modules/CompilerRTUtils.cmake

elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" STREQUAL "${arch}" AND
     message(FATAL_ERROR "Cannot compile for ${arch}:\n${TARGET_${arch}_OUTPUT}")
   endif()
 endmacro()
 
 macro(detect_target_arch)
   check_symbol_exists(__arm__ "" __ARM)
   check_symbol_exists(__aarch64__ "" __AARCH64)
   check_symbol_exists(__x86_64__ "" __X86_64)
-  check_symbol_exists(__i686__ "" __I686)
   check_symbol_exists(__i386__ "" __I386)
   check_symbol_exists(__mips__ "" __MIPS)
   check_symbol_exists(__mips64__ "" __MIPS64)
   check_symbol_exists(__s390x__ "" __S390X)
   check_symbol_exists(__wasm32__ "" __WEBASSEMBLY32)
   check_symbol_exists(__wasm64__ "" __WEBASSEMBLY64)
   if(__ARM)
     add_default_target_arch(arm)
   elseif(__AARCH64)
     add_default_target_arch(aarch64)
   elseif(__X86_64)
     add_default_target_arch(x86_64)
-  elseif(__I686)
-    add_default_target_arch(i686)
   elseif(__I386)
     add_default_target_arch(i386)
   elseif(__MIPS64) # must be checked before __MIPS
     add_default_target_arch(mips64)
   elseif(__MIPS)
     add_default_target_arch(mips)
   elseif(__S390X)
     add_default_target_arch(s390x)

cmake/base-config-ix.cmake

macro(test_targets)
   if(ANDROID)
     # Examine compiler output to determine target architecture.
     detect_target_arch()
     set(COMPILER_RT_OS_SUFFIX "-android")
   elseif(NOT APPLE) # Supported archs for Apple platforms are generated later
     if("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "i[2-6]86|x86|amd64")
       if(NOT MSVC)
         test_target_arch(x86_64 "" "-m64")
-        # FIXME: We build runtimes for both i686 and i386, as "clang -m32" may
-        # target different variant than "$CMAKE_C_COMPILER -m32". This part should
-        # be gone after we resolve PR14109.
-        test_target_arch(i686 __i686__ "-m32")
         test_target_arch(i386 __i386__ "-m32")
       else()
         if (CMAKE_SIZEOF_VOID_P EQUAL 4)
           test_target_arch(i386 "" "")
         else()
           test_target_arch(x86_64 "" "")
         endif()
       endif()

cmake/builtin-config-ix.cmake

  _Atomic int result = x * y;
  return result;
 }
 ")
 
 
 set(ARM64 aarch64)
 set(ARM32 arm armhf)
-set(X86 i386 i686)
+set(X86 i386)
 set(X86_64 x86_64)
 set(MIPS32 mips mipsel)
 set(MIPS64 mips64 mips64el)
 set(PPC64 powerpc64 powerpc64le)
 set(WASM32 wasm32)
 set(WASM64 wasm64)
 
 if(APPLE)

cmake/config-ix.cmake

else()
       # for cross-targeted hardware (i.e. iOS).
       set(${out_var} -arch ${arch} PARENT_SCOPE)
     endif()
   endif()
 endfunction()
 
 set(ARM64 aarch64)
 set(ARM32 arm armhf)
-set(X86 i386 i686)
+set(X86 i386)
 set(X86_64 x86_64)
 set(MIPS32 mips mipsel)
 set(MIPS64 mips64 mips64el)
 set(PPC64 powerpc64 powerpc64le)
 set(S390X s390x)
 set(WASM32 wasm32)
 set(WASM64 wasm64)
 

lib/asan/CMakeLists.txt

add_compiler_rt_runtime(clang_rt.asan-preinit
     STATIC
     ARCHS ${ASAN_SUPPORTED_ARCH}
     OBJECT_LIBS RTAsan_preinit
     CFLAGS ${ASAN_CFLAGS}
     DEFS ${ASAN_COMMON_DEFINITIONS}
     PARENT_TARGET asan)
 
   foreach(arch ${ASAN_SUPPORTED_ARCH})
-    if (UNIX AND NOT ${arch} MATCHES "i386|i686")
+    if (UNIX AND NOT ${arch} STREQUAL "i386")
       add_sanitizer_rt_version_list(clang_rt.asan-dynamic-${arch}
                                     LIBS clang_rt.asan-${arch} clang_rt.asan_cxx-${arch}
                                     EXTRA asan.syms.extra)
       set(VERSION_SCRIPT_FLAG
            -Wl,--version-script,${CMAKE_CURRENT_BINARY_DIR}/clang_rt.asan-dynamic-${arch}.vers)
       set_source_files_properties(
         ${CMAKE_CURRENT_BINARY_DIR}/dummy.cc
         PROPERTIES
add_compiler_rt_runtime(clang_rt.asan
               RTUbsan_cxx
       CFLAGS ${ASAN_DYNAMIC_CFLAGS}
       LINKFLAGS ${ASAN_DYNAMIC_LINK_FLAGS}
                 ${VERSION_SCRIPT_FLAG}
       LINK_LIBS ${ASAN_DYNAMIC_LIBS}
       DEFS ${ASAN_DYNAMIC_DEFINITIONS}
       PARENT_TARGET asan)
 
-    if (UNIX AND NOT ${arch} MATCHES "i386|i686")
+    if (UNIX AND NOT ${arch} STREQUAL "i386")
       add_sanitizer_rt_symbols(clang_rt.asan_cxx
         ARCHS ${arch})
       add_dependencies(asan clang_rt.asan_cxx-${arch}-symbols)
       add_sanitizer_rt_symbols(clang_rt.asan
         ARCHS ${arch}
         EXTRA asan.syms.extra)
       add_dependencies(asan clang_rt.asan-${arch}-symbols)
     endif()

lib/asan/scripts/asan_device_setup

 
 function get_device_arch { # OUT OUT64
     local _outvar=$1
     local _outvar64=$2
     local _ABI=$(adb_shell getprop ro.product.cpu.abi)
     local _ARCH=
     local _ARCH64=
     if [[ $_ABI == x86* ]]; then
-        _ARCH=i686
+        _ARCH=i386
     elif [[ $_ABI == armeabi* ]]; then
         _ARCH=arm
     elif [[ $_ABI == arm64-v8a* ]]; then
         _ARCH=arm
         _ARCH64=aarch64
     else
         echo "Unrecognized device ABI: $_ABI"
         exit 1

lib/builtins/CMakeLists.txt

set(i386_SOURCES
       ${GENERIC_SOURCES})
 
   if (WIN32)
     set(i386_SOURCES
         ${i386_SOURCES}
         i386/chkstk.S
         i386/chkstk2.S)
   endif()
-
-  set(i686_SOURCES
-      ${i386_SOURCES})
 else () # MSVC
   # Use C versions of functions when building on MSVC
   # MSVC's assembler takes Intel syntax, not AT&T syntax.
   # Also use only MSVC compilable builtin implementations.
   set(x86_64_SOURCES
       x86_64/floatdidf.c
       x86_64/floatdisf.c
       x86_64/floatdixf.c
       ${MSVC_SOURCES})
   set(x86_64h_SOURCES ${x86_64_SOURCES})
   set(i386_SOURCES ${MSVC_SOURCES})
-  set(i686_SOURCES ${i386_SOURCES})
 endif () # if (NOT MSVC)
 
 set(arm_SOURCES
   arm/bswapdi2.S
   arm/bswapsi2.S
   arm/clzdi2.S
   arm/clzsi2.S
   arm/comparesf2.S

lib/ubsan/CMakeLists.txt

add_compiler_rt_runtime(clang_rt.ubsan_standalone_cxx
       STATIC
       ARCHS ${UBSAN_SUPPORTED_ARCH}
       OBJECT_LIBS RTUbsan_cxx
       CFLAGS ${UBSAN_CXXFLAGS}
       PARENT_TARGET ubsan)
 
     if (UNIX)
       set(ARCHS_FOR_SYMBOLS ${UBSAN_SUPPORTED_ARCH})
-      list(REMOVE_ITEM ARCHS_FOR_SYMBOLS i386 i686)
+      list(REMOVE_ITEM ARCHS_FOR_SYMBOLS i386)
       add_sanitizer_rt_symbols(clang_rt.ubsan_standalone
         ARCHS ${ARCHS_FOR_SYMBOLS}
         PARENT_TARGET ubsan
         EXTRA ubsan.syms.extra)
       add_sanitizer_rt_symbols(clang_rt.ubsan_standalone_cxx
         ARCHS ${ARCHS_FOR_SYMBOLS}
         PARENT_TARGET ubsan
         EXTRA ubsan.syms.extra)
     endif()
   endif()
 endif()

test/asan/CMakeLists.txt

 set(ASAN_LIT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
 
 set(ASAN_TESTSUITES)
 set(ASAN_DYNAMIC_TESTSUITES)
 
 # TODO(wwchrome): Re-enable Win64 asan tests when ready.
 # Disable tests for asan Win64 temporarily.
 if(OS_NAME MATCHES "Windows" AND CMAKE_SIZEOF_VOID_P EQUAL 8)
   set(EXCLUDE_FROM_ALL TRUE)
 endif()
 
 macro(get_bits_for_arch arch bits)
-  if (${arch} MATCHES "i386|i686|arm|mips|mipsel")
+  if (${arch} MATCHES "i386|arm|mips|mipsel")
     set(${bits} 32)
   elseif (${arch} MATCHES "x86_64|powerpc64|powerpc64le|aarch64|mips64|mips64el|s390x")
     set(${bits} 64)
   else()
     message(FATAL_ERROR "Unknown target architecture: ${arch}")
   endif()
 endmacro()
 

test/asan/lit.cfg

clang_wrapper = os.path.join(asan_lit_source_dir,
                                "android_commands", "android_compile.py") + " "
 else:
   config.available_features.add('not-android')
   clang_wrapper = ""
 
 def build_invocation(compile_flags):
   return " " + " ".join([clang_wrapper, config.clang] + compile_flags) + " "
 
-# Clang driver link 'x86' (i686) architecture to 'i386'.
-target_arch = config.target_arch
-if (target_arch == "i686"):
-  target_arch = "i386"
-
 config.substitutions.append( ("%clang ", build_invocation(target_cflags)) )
 config.substitutions.append( ("%clangxx ", build_invocation(target_cxxflags)) )
 config.substitutions.append( ("%clang_asan ", build_invocation(clang_asan_cflags)) )
 config.substitutions.append( ("%clangxx_asan ", build_invocation(clang_asan_cxxflags)) )
-config.substitutions.append( ("%shared_libasan", "libclang_rt.asan-%s.so" % target_arch))
+config.substitutions.append( ("%shared_libasan", "libclang_rt.asan-%s.so" % config.target_arch))
 if config.asan_dynamic:
   config.substitutions.append( ("%clang_asan_static ", build_invocation(clang_asan_static_cflags)) )
   config.substitutions.append( ("%clangxx_asan_static ", build_invocation(clang_asan_static_cxxflags)) )
 
 # Windows-specific tests might also use the clang-cl.exe driver.
 if platform.system() == 'Windows':
   clang_cl_asan_cxxflags = ["-fsanitize=address",
                             "-Wno-deprecated-declarations",

test/lit.common.cfg

 else:
   config.expect_crash = "not --crash "
 
 config.substitutions.append( ("%expect_crash ", config.expect_crash) )
 
 target_arch = getattr(config, 'target_arch', None)
 if target_arch:
   config.available_features.add(target_arch + '-target-arch')
-  if target_arch in ['x86_64', 'i386', 'i686']:
+  if target_arch in ['x86_64', 'i386']:
     config.available_features.add('x86-target-arch')
   config.available_features.add(target_arch + '-' + config.host_os.lower())
 
 compiler_rt_debug = getattr(config, 'compiler_rt_debug', False)
 if not compiler_rt_debug:
   config.available_features.add('compiler-rt-optimized')
 
 sanitizer_can_use_cxxabi = getattr(config, 'sanitizer_can_use_cxxabi', True)