[sanitizer_common] Use atomic builtin in sanitizer_atomic_clang.h

[ro]

Summary

As discussed in D118021, clang -m32 on Solaris/sparcv9 currently incorrectly doesn't
inline atomics on 8-byte operands, unlike gcc. With the workaround in that patch in place,
we're left with may undefined references to __sync_val_compare_and_swap_8, which
isn't provided by libatomic. This reference is due to the use of __sync_val_compare_and_swap in sanitizer_atomic_clang.h's atomic_compare_exchange_strong.
As is already done in scudo/standalone/atomic_helpers.h, using __atomic_compare_exchange instead avoids this problem.

Tested on sparcv9-sun-solaris2.11, amd64-pc-solaris2.11, and x86_64-pc-linux-gnu.

However, there are two issues here:

• In a 2-stage build with GCC 11 as stage 1 compiler, it warns

/vol/llvm/src/llvm-project/local/compiler-rt/lib/asan/../sanitizer_common/sanitizer_atomic_clang.h:86:35: warning: invalid memory model argument to builtin [-Winvalid-memory-model]
   86 |   return __atomic_compare_exchange(&a->val_dont_use, cmp, &xchg, false, mo,
      |          ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   87 |                                    __ATOMIC_RELAXED);
      |                                    ~~~~~~~~~~~~~~~~~

According to [[ https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html#g_t_005f_005fatomic-Builtins | Built-in Functions for Memory Model Aware Atomic Operations ]], the `__atomic_*` builtins map runtime-variable memory order parameters to `__ATOMIC_SEQ_CST`.  However, with `mo, __ATOMIC_SEQ_CST`, `gcc` still warns

/vol/llvm/src/llvm-project/local/compiler-rt/lib/asan/../sanitizer_common/sanitizer_atomic_clang.h:86:35: warning: failure memory model cannot be stronger than success memory model for ‘__atomic_compare_exchange’ [-Winvalid-memory-model]
   86 |   return __atomic_compare_exchange(&a->val_dont_use, cmp, &xchg, false, mo,
      |          ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   87 |                                    __ATOMIC_SEQ_CST);
      |                                    ~~~~~~~~~~~~~~~~~

Only with ` __ATOMIC_SEQ_CST,  __ATOMIC_SEQ_CST` the warning vanishes.

• With any of the three variants above, on Solaris/sparcv9 three tests regress with this patch:

  SanitizerCommon-Unit :: ./Sanitizer-sparc-Test/DenseMapCustomTest.DefaultMinRe
servedSizeTest
  SanitizerCommon-Unit :: ./Sanitizer-sparcv9-Test/CompactRingBuffer.int64
  SanitizerCommon-Unit :: ./Sanitizer-sparcv9-Test/DenseMapCustomTest.DefaultMin
ReservedSizeTest

There's no such regression on x86, though.

[glaubitz]

Thanks, that's actually a workaround I have also used in the past!

[ro]

In D118024#3265602, @glaubitz wrote:

Thanks, that's actually a workaround I have also used in the past!

I saw after I'd developed it myself. Could you please check if you see the same testsuite regressions/failures on Linux/sparc? I'd like to know
if this is a Solaris-only issue.

[efriedma]

compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h

78

Please don't change the memory ordering in this patch. (__sync_* is sequentially consistent.) If you really want to change it, please post as a separate patch with an appropriate title.

94

Have you looked at extending this code to SPARC? I'm a little concerned that the dependency on libatomic might cause issues (however we write it).

[ro]

compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h

78

It wasn't clear to me from GCC's __sync builtin documentation that they are __ATOMIC_SEQ_CST. I just assumed that scudo/standalone/atomic_helpers.h really implemented atomic_compare_exchange_strong.

As I mentioned, both __ATOMIC_RELAXED and __ATOMIC_SEQ_CST cause 3 regressions on Solaris/sparcv9, so this patch certainly cannot go in as is, despite fixing hundreds of link failures.

94

Not yet: I'd hoped to be able to come up with a minimal workaround for LLVM not fully implementing atomics on SPARC v8+ (or even better, fix that problem instead).

Switching from __sync_val_compare_and_swap to __atomic_compare_exchange shouldn't make a difference for any target inlining atomics IIUC. Those that don't would just trade link failures for one function for another.

[ro]

Use __ATOMIC_SEQ_CST.

[ro]

I've meanwhile found that the 3 regressions mentioned in the summary are unrelated: I had updated versions of D91607 and D91608 in my
tree (still unreviewed after more than a year): the latter of those enables sanitizer_common testing on sparc. The DenseMapCustomTest.DefaultMinReservedSizeTest is new since the patches were originally submitted and AFAICS the failure happens because sparc uses 8k pages instead of 4k e.g. on x86. The CompactRingBuffer.int64 failures is worked around in D91617.

compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h

94

I've now tried, but results aren't encouraging: the code isn't
target-specific AFAICS, but on sparc I get two regressions compared to the __atomic_compare_exchange variant:

+  SanitizerCommon-Unit :: ./Sanitizer-sparc-Test/SanitizerCommon.Mutex
+  SanitizerCommon-Unit :: ./Sanitizer-sparc-Test/SanitizerCommon.ThreadRegistryThreadedTest

At least one of those never times out and hangs ninja check-all indefinitively. FWIW, I've repeated the same experiment on amd64-pc-solaris2.11 with the added change of disabling sanitizer_atomic_clang_mips.h. I get the same failures there, and also

  SanitizerCommon-Unit :: ./Sanitizer-x86_64-Test/SanitizerCommon.AtomicCompareE
xchangeTest

[efriedma]

compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h

78

Still not changed completely to use SEQ_CST. (__atomic_compare_exchange has two memory order operands.)

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h

78

I am not sure our memory_order enum matches ATOMIC_* consts, at least there never was an intention to make them compatible.
If you are going to use ATOMIC_SEQ_CST for both args, please add a comment saying that this is only due to transition from __sync_val_compare_and_swap, but we should use the specified mo.

94

I get two regressions compared to the __atomic_compare_exchange variant:
SanitizerCommon-Unit :: ./Sanitizer-sparc-Test/SanitizerCommon.Mutex

If Mutex is broken, everything is broken.

[ro]

compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h

78

I'd done this based on GCC's __atomic builtin description:

Note that the C++11 standard allows for the memory order parameter to be determined at run time rather than at compile time. These built-in functions map any run-time value to __ATOMIC_SEQ_CST rather than invoke a runtime library call or inline a switch statement. This is standard compliant, safe, and the simplest approach for now.

mo would thus be mapped to __ATOMIC_SEQ_CST. I don't know if this is just a GCC implementation detail, though, especially given that I've no idea what's the formal specification of the __atomic builtins, if any.

[ro]

Add comment.

[efriedma]

LGTM

compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h

if (mo == memory_order_seq_cst)
     __sync_synchronize();
   return v;
 }
 
 template <typename T>
 inline bool atomic_compare_exchange_strong(volatile T *a, typename T::Type *cmp,
                                            typename T::Type xchg,
                                            memory_order mo) {
-  typedef typename T::Type Type;
-  Type cmpv = *cmp;
-  Type prev;
-  prev = __sync_val_compare_and_swap(&a->val_dont_use, cmpv, xchg);
-  if (prev == cmpv) return true;
-  *cmp = prev;
-  return false;
+  return __atomic_compare_exchange(&a->val_dont_use, cmp, &xchg, false, mo,
+                                   __ATOMIC_RELAXED);
 }
 
 template<typename T>
 inline bool atomic_compare_exchange_weak(volatile T *a,
                                          typename T::Type *cmp,
                                          typename T::Type xchg,
                                          memory_order mo) {
   return atomic_compare_exchange_strong(a, cmp, xchg, mo);
 }
 
 }  // namespace __sanitizer
 
 // This include provides explicit template instantiations for atomic_uint64_t
 // on MIPS32, which does not directly support 8 byte atomics. It has to
 // proceed the template definitions above.
 #if defined(_MIPS_SIM) && defined(_ABIO32)
   #include "sanitizer_atomic_clang_mips.h"
 #endif
 
 #undef ATOMIC_ORDER
 
 #endif  // SANITIZER_ATOMIC_CLANG_H