This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer_common] Use atomic builtin in sanitizer_atomic_clang.h
ClosedPublic

Authored by ro on Jan 24 2022, 2:34 AM.

Details

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.

Diff Detail

Event Timeline

ro created this revision.Jan 24 2022, 2:34 AM
ro requested review of this revision.Jan 24 2022, 2:34 AM
Herald added a subscriber: Restricted Project. · View Herald TranscriptJan 24 2022, 2:34 AM

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

ro added a comment.Jan 24 2022, 2:41 AM

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 added inline comments.
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.

98

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 added inline comments.Jan 25 2022, 5:27 AM
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.

98

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 updated this revision to Diff 403611.Jan 27 2022, 5:06 AM

Use __ATOMIC_SEQ_CST.

ro marked an inline comment as done.Jan 27 2022, 5:18 AM

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
98

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
lkail added a subscriber: lkail.Jan 27 2022, 5:33 AM
efriedma added inline comments.Jan 27 2022, 10:44 AM
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.)

vitalybuka added 1 blocking reviewer(s): dvyukov.
dvyukov added inline comments.Jan 27 2022, 11:04 PM
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.

98

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 added inline comments.Jan 28 2022, 2:37 AM
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 updated this revision to Diff 403940.Jan 28 2022, 2:45 AM

Add comment.

dvyukov accepted this revision.Jan 29 2022, 6:10 AM
This revision is now accepted and ready to land.Jan 29 2022, 6:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 5:46 PM