[PATCH] D118024: [sanitizer_common] Use atomic builtin in sanitizer_atomic_clang.h

[Rainer Orth via Phabricator via llvm-commits]

ro created this revision.
ro added a reviewer: vitalybuka.
ro added a project: Sanitizers.
Herald added subscribers: pengfei, cryptoad, fedor.sergeev, jyknight.
ro requested review of this revision.
Herald added a subscriber: Sanitizers.

As discussed in D118021 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118024

Files:
  compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h


Index: compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h
===================================================================
--- compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h
+++ compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h
@@ -74,13 +74,8 @@
 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>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D118024.402438.patch
Type: text/x-patch
Size: 852 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220124/d9f7d058/attachment.bin>