[PATCH] D46597: [sanitizer] Correct 64-bit atomic_store on 32-bit "other" platforms

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_atomic_clang_other.h:89
       cur = __sync_val_compare_and_swap(&a->val_dont_use, cmp, v);
-      if (cmp == v)
+      if (cur == v)
         break;
----------------
dvyukov wrote:
> The current is not just slow, it looks broken to me. Is it even used anywhere? Maybe we should just drop it. I think we generally require C++11 for building compiler-rt.
> 
> The canonical way to do this is `cur == cmp` which should accomplish the operation with 1 CAS in common case. The old check was probably an attempt to optimize for the case when the variable already contains the value we want to store (and we obtained an atomic snapshot of the current value with CAS). So perhaps this could be `cur == cmp || cur == v`?
Unfortunately yes this is used for ARM32, when doing a 64-bit atomic store.
I noticed that on some platforms this was particularly slow hence me chasing down what could go wrong.
I'd like to keep using this to not have a libc++ (or equivalent) dependency (for Scudo at least).
I'll apply the suggested change, thanks!


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D46597