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

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h:78
+  return __atomic_compare_exchange(&a->val_dont_use, cmp, &xchg, false, mo,
+                                   __ATOMIC_SEQ_CST);
 }
----------------
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.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h:94
 // proceed the template definitions above.
 #if defined(_MIPS_SIM) && defined(_ABIO32)
   #include "sanitizer_atomic_clang_mips.h"
----------------
ro wrote:
> ro wrote:
> > efriedma wrote:
> > > 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).
> > 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.
> 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
> ```
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118024/new/

https://reviews.llvm.org/D118024