[PATCH] D118024: [sanitizer_common] Use atomic builtin in sanitizer_atomic_clang.h
Rainer Orth via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 25 05:27:13 PST 2022
ro 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_RELAXED);
}
----------------
efriedma wrote:
> 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.
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.
================
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"
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118024/new/
https://reviews.llvm.org/D118024
More information about the llvm-commits
mailing list