compiler-rt uses deprecated built-ins for atomic access

[glaubitz]

I have been trying to fix the failing builds on the buildbot clang-sparc64-linux-multistage [1] for quite some time now.

The reason for the failure is that the linker cannot find __sync_val_compare_and_swap_8 when building on 32-bit SPARC which is not available on this target:

/usr/bin/ld: warning: -z gnu-version-script-compat ignored
/usr/bin/ld: projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.sparc.dir/sanitizer_libignore.cpp.o: in function `bool __sanitizer::atomic_compare_exchange_strong<__sanitizer::atomic_uint64_t>(__sanitizer::atomic_uint64_t volatile*, __sanitizer::atomic_uint64_t::Type*, __sanitizer::atomic_uint64_t::Type, __sanitizer::memory_order)':
/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h:80: undefined reference to `__sync_val_compare_and_swap_8'

According to a bug report in GCC, the atomic built-ins __sync_(...) [3] are considered deprecated and have been superseded by the __atomic_(...) built-ins [4].

And, in fact, the following simple hack fixes the build of compiler-rt on 32-bit SPARC on Linux:

diff --git a/compiler-rt/cmake/base-config-ix.cmake b/compiler-rt/cmake/base-config-ix.cmake
index d7b0124f3546..266011ebdeee 100644
--- a/compiler-rt/cmake/base-config-ix.cmake
+++ b/compiler-rt/cmake/base-config-ix.cmake
@@ -206,7 +206,11 @@ macro(test_targets)
     elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "s390x")
       test_target_arch(s390x "" "")
     elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "sparc")
-      test_target_arch(sparc "" "-m32")
+      if(CMAKE_SYSTEM_NAME MATCHES "Linux")
+       test_target_arch(sparc "" "-m32 -latomic")
+      else()
+       test_target_arch(sparc "" "-m32")
+      endif()
       test_target_arch(sparcv9 "" "-m64")
     elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "mipsel|mips64el")
       # Gcc doesn't accept -m32/-m64 so we do the next best thing and use
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h b/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h
index fc13ca52dda7..bfe30c775663 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h
@@ -77,7 +77,7 @@ inline bool atomic_compare_exchange_strong(volatile T *a, typename T::Type *cmp,
   typedef typename T::Type Type;
   Type cmpv = *cmp;
   Type prev;
-  prev = __sync_val_compare_and_swap(&a->val_dont_use, cmpv, xchg);
+  prev = __atomic_compare_exchange(&a->val_dont_use, &cmpv, &xchg, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);
   if (prev == cmpv) return true;
   *cmp = prev;
   return false;

I would therefore suggest that compiler-rt switches to the new built-ins for atomic access.

[1] https://lab.llvm.org/staging/#/builders/113
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63368
[3] https://gcc.gnu.org/onlinedocs/gcc-4.5.3/gcc/Atomic-Builtins.html
[4] https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html

[efriedma-quic]

__sync_val_compare_and_swap may be "deprecated", but it should still work.

The SPARC backend should never generate calls to __sync_val_compare_and_swap_8; if it does, that's a bug. (See https://llvm.org/docs/Atomics.html#libcalls-sync .)

[efriedma-quic]

Specifically, see:

llvm-project/llvm/lib/Target/Sparc/SparcISelLowering.cpp

Line 1591 in 754d6af

if (Subtarget->isV9())

Looks like it's calling setMaxAtomicSizeInBitsSupported(64) on 32-bit targets. I'm pretty sure this is wrong: 32-bit SPARC doesn't have 64-bit atomics.

[glaubitz]

Hi Eli!

Thanks for looking into this! I have done more research and stumbled over an interesting fact.

On Solaris, 32-bit SPARC uses the V8+ baseline which includes 64-bit atomics.

Quoting from [1]:

With -mv8plus, GCC generates code for the SPARC-V8+ ABI. The difference from the V8 ABI is that the global and out registers are considered 64 bits wide. This is enabled by default on Solaris in 32-bit mode for all SPARC-V9 processors. 

However, for gcc, V8+ is not the same as 32-bit V9:

glaubitz@gcc202:~/llvm-project$ echo | gcc -mcpu=v9 -m32 -E -dM -|grep -i SWAP
#define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
#define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
#define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
glaubitz@gcc202:~/llvm-project$ echo | gcc -mv8plus -E -dM -|grep -i SWAP
#define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
#define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
#define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
#define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8 1
glaubitz@gcc202:~/llvm-project$

Thus, the following patch fixes the build with GCC:

diff --git a/compiler-rt/cmake/base-config-ix.cmake b/compiler-rt/cmake/base-config-ix.cmake
index d7b0124f3546..a1dd88da490b 100644
--- a/compiler-rt/cmake/base-config-ix.cmake
+++ b/compiler-rt/cmake/base-config-ix.cmake
@@ -206,7 +206,11 @@ macro(test_targets)
     elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "s390x")
       test_target_arch(s390x "" "")
     elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "sparc")
-      test_target_arch(sparc "" "-m32")
+      if(CMAKE_COMPILER_IS_GNUCXX)
+       test_target_arch(sparc "" "-m32 -mv8plus")
+      else()
+       test_target_arch(sparc "" "-m32")
+      endif()
       test_target_arch(sparcv9 "" "-m64")
     elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "mipsel|mips64el")
       # Gcc doesn't accept -m32/-m64 so we do the next best thing and use

For the stage2 build, one would have to default the 32-bit baseline to V9 as it has been done here [2].

[1] https://gcc.gnu.org/onlinedocs/gcc/SPARC-Options.html
[2] https://reviews.llvm.org/D86621

[efriedma-quic]

Interesting. Not really relevant, though, unless you're planning to add "-mv8plus" support to LLVM.

[efriedma-quic]

Err, wait, the bot is failing stage 1? I was assuming the issue was that clang was generating a call to __sync_val_compare_and_swap_8 . Not sure why gcc is generating calls to __sync_val_compare_and_swap_8, instead of using __atomic libcalls. If that's the issue, I guess it's okay to work around it in the source code...

Alternatively, maybe consider just disabling the code to generate 32-bit compiler-rt libraries on SPARC.

[glaubitz]

Err, wait, the bot is failing stage 1? I was assuming the issue was that clang was generating a call to __sync_val_compare_and_swap_8 . Not sure why gcc is generating calls to __sync_val_compare_and_swap_8, instead of using __atomic libcalls. If that's the issue, I guess it's okay to work around it in the source code...

Exactly. The problem is the bootstrap build with GCC. For the stage2 build, we can just use the approach taken on Solaris [1].

Alternatively, maybe consider just disabling the code to generate 32-bit compiler-rt libraries on SPARC.

I rather want to avoid that. Also, the Solaris maintainers won't like that since Solaris isn't fully 64-bit yet.

[1] https://reviews.llvm.org/D86621

[brad0]

What is the current status using 14.0.0?

[brad0]

Ping.