[libcxx][atomic] Fix failure mapping in compare_exchange_{strong,weak}.

[rupprecht]

Summary

https://eel.is/c++draft/atomics.types.operations#23 says: ... the value of failure is order except that a value of memory_order::acq_rel shall be replaced by the value memory_order::acquire and a value of memory_order::release shall be replaced by the value memory_order::relaxed.

This failure mapping is only handled for _LIBCPP_HAS_GCC_ATOMIC_IMP. We are seeing bad code generation for compare_exchange_strong(cmp, 1, std::memory_order_acq_rel) when using libc++ in place of libstdc++: https://godbolt.org/z/v3onrrq4G.

This was caught by tsan tests after D99434, [TSAN] Honor failure memory orders in AtomicCAS, but appears to be an issue in non-tsan code.

[ldionne]

Is it possible to write a test for that? When you say

This was caught by tsan tests after D99434 [...]

Do you mean that any Clang that includes the fix for D99434 will catch the error with the existing libc++ test suite when TSAN is enabled? If so, then I think it's fine not to add a test, since it's already tested. We could consider moving more CI configurations to Clang trunk, too.

Requesting changes until I have answers to my questions, but otherwise this LGTM.

[rupprecht]

In D103846#2805849, @ldionne wrote:

Is it possible to write a test for that? When you say

This was caught by tsan tests after D99434 [...]

Do you mean that any Clang that includes the fix for D99434 will catch the error with the existing libc++ test suite when TSAN is enabled? If so, then I think it's fine not to add a test, since it's already tested. We could consider moving more CI configurations to Clang trunk, too.

Requesting changes until I have answers to my questions, but otherwise this LGTM.

No, these are some tests we run internally with tsan enabled, and found bad codegen for this dependency: https://github.com/zeromq/libzmq/blob/b40a793142326edeec5c4dc63e6d8929e979a097/src/atomic_ptr.hpp#L211

There is libcxx/test/std/atomics/atomics.general/replace_failure_order.pass.cpp that already covers this issue. It only tests that the build is successful, and AFAICT doesn't verify correctness (e.g. the right codegen), so the test passes.

Here's a test that catches the issue (verified by locally reverting changes). Is is possible to put this somewhere in the libc++ test suite?

// RUN: %clangxx -O2 -c %s -stdlib=libc++ -fsanitize=thread -S -emit-llvm -o - | FileCheck %s
#include <atomic>

// CHECK: @_Z27strong_memory_order_acq_relPNSt3__16atomicIiEEi(
// CHECK:    {{.*}} = call i32 @__tsan_atomic32_compare_exchange_val({{.*}}, i32 1, i32 4, i32 2)
bool strong_memory_order_acq_rel(std::atomic<int> *a, int cmp) {
  return a->compare_exchange_strong(cmp, 1, std::memory_order_acq_rel);
}

(it generates ..., i32 1, i32 4, i32 0) w/o this patch)

[rupprecht]

• Add a codegen test

[rupprecht]

The test I added is not the greatest test, but it catches the issue. Lemme know if this works.

[ldionne]

Thanks a lot! Indeed, we don't have access to FileCheck in the libc++ tests. I had once looked into doing that but I ran into issues and we didn't have a clear use case so I decided not to spend more time on it. Looks like this will do for the time being.

Please merge this once/if CI is passing! Thanks!

[ldionne]

libcxx/test/std/atomics/atomics.general/replace_failure_order_codegen.sh.cpp

11

You can fix the CI failure by saying // REQUIRES: clang.

[rupprecht]

• Add requires for clang to avoid unknown gcc flag errors

[rupprecht]

Thanks! I'll wait for another CI notification to post before landing.

libcxx/test/std/atomics/atomics.general/replace_failure_order_codegen.sh.cpp

11

Ah, thanks for the tip. Done.

[rupprecht]

• Require tsan feature to avoid buildbot jobs that don't support -fsanitize=thread (asan, windows, 32bit, ARMv7/8)

libcxx/include/atomic

_Tp __cxx_atomic_exchange(__cxx_atomic_base_impl<_Tp> volatile* __a, _Tp __value, memory_order __order) _NOEXCEPT {
     return __c11_atomic_exchange(&__a->__a_value, __value, static_cast<__memory_order_underlying_t>(__order));
 }
 template<class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 _Tp __cxx_atomic_exchange(__cxx_atomic_base_impl<_Tp> * __a, _Tp __value, memory_order __order) _NOEXCEPT {
     return __c11_atomic_exchange(&__a->__a_value, __value, static_cast<__memory_order_underlying_t>(__order));
 }
 
+_LIBCPP_INLINE_VISIBILITY inline _LIBCPP_CONSTEXPR memory_order __to_failure_order(memory_order __order) {
+  // Avoid switch statement to make this a constexpr.
+  return __order == memory_order_release ? memory_order_relaxed:
+         (__order == memory_order_acq_rel ? memory_order_acquire:
+             __order);
+}
+
 template<class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 bool __cxx_atomic_compare_exchange_strong(__cxx_atomic_base_impl<_Tp> volatile* __a, _Tp* __expected, _Tp __value, memory_order __success, memory_order __failure) _NOEXCEPT {
-    return __c11_atomic_compare_exchange_strong(&__a->__a_value, __expected, __value, static_cast<__memory_order_underlying_t>(__success), static_cast<__memory_order_underlying_t>(__failure));
+    return __c11_atomic_compare_exchange_strong(&__a->__a_value, __expected, __value, static_cast<__memory_order_underlying_t>(__success), static_cast<__memory_order_underlying_t>(__to_failure_order(__failure)));
 }
 template<class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 bool __cxx_atomic_compare_exchange_strong(__cxx_atomic_base_impl<_Tp> * __a, _Tp* __expected, _Tp __value, memory_order __success, memory_order __failure) _NOEXCEPT {
-    return __c11_atomic_compare_exchange_strong(&__a->__a_value, __expected, __value, static_cast<__memory_order_underlying_t>(__success), static_cast<__memory_order_underlying_t>(__failure));
+    return __c11_atomic_compare_exchange_strong(&__a->__a_value, __expected, __value, static_cast<__memory_order_underlying_t>(__success), static_cast<__memory_order_underlying_t>(__to_failure_order(__failure)));
 }
 
 template<class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 bool __cxx_atomic_compare_exchange_weak(__cxx_atomic_base_impl<_Tp> volatile* __a, _Tp* __expected, _Tp __value, memory_order __success, memory_order __failure) _NOEXCEPT {
-    return __c11_atomic_compare_exchange_weak(&__a->__a_value, __expected, __value, static_cast<__memory_order_underlying_t>(__success), static_cast<__memory_order_underlying_t>(__failure));
+    return __c11_atomic_compare_exchange_weak(&__a->__a_value, __expected, __value, static_cast<__memory_order_underlying_t>(__success), static_cast<__memory_order_underlying_t>(__to_failure_order(__failure)));
 }
 template<class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 bool __cxx_atomic_compare_exchange_weak(__cxx_atomic_base_impl<_Tp> * __a, _Tp* __expected, _Tp __value, memory_order __success, memory_order __failure) _NOEXCEPT {
-    return __c11_atomic_compare_exchange_weak(&__a->__a_value, __expected, __value,  static_cast<__memory_order_underlying_t>(__success), static_cast<__memory_order_underlying_t>(__failure));
+    return __c11_atomic_compare_exchange_weak(&__a->__a_value, __expected, __value,  static_cast<__memory_order_underlying_t>(__success), static_cast<__memory_order_underlying_t>(__to_failure_order(__failure)));
 }
 
 template<class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 _Tp __cxx_atomic_fetch_add(__cxx_atomic_base_impl<_Tp> volatile* __a, _Tp __delta, memory_order __order) _NOEXCEPT {
     return __c11_atomic_fetch_add(&__a->__a_value, __delta, static_cast<__memory_order_underlying_t>(__order));
 }
 template<class _Tp>

libcxx/test/std/atomics/atomics.general/replace_failure_order_codegen.sh.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: clang
+// UNSUPPORTED: libcpp-has-no-threads
+
+// Adding "-fsanitize=thread" directly causes many platforms to fail (because
+// they don't support tsan), and causes other sanitizer builds to fail (e.g.
+// asan and tsan don't mix). Instead, require the tsan feature.
+// REQUIRES: tsan
+
+// This test verifies behavior specified by [atomics.types.operations.req]/21:
+//
+//     When only one memory_order argument is supplied, the value of success is
+//     order, and the value of failure is order except that a value of
+//     memory_order_acq_rel shall be replaced by the value memory_order_acquire
+//     and a value of memory_order_release shall be replaced by the value
+//     memory_order_relaxed.
+//
+// This test mirrors replace_failure_order.pass.cpp. However, we also want to
+// verify the codegen is correct. This verifies a bug where memory_order_acq_rel
+// was not being replaced with memory_order_acquire in external
+// TSAN-instrumented tests.
+
+// RUN: %{cxx} -c %s %{flags} %{compile_flags} -O2 -stdlib=libc++ -S -emit-llvm -o %t.ll
+
+#include <atomic>
+
+// Note: libc++ tests do not use on FileCheck.
+// RUN: grep -E "call i32 @__tsan_atomic32_compare_exchange_val\(.*, i32 1, i32 4, i32 2\)" %t.ll
+bool strong_memory_order_acq_rel(std::atomic<int>* a, int cmp) {
+  return a->compare_exchange_strong(cmp, 1, std::memory_order_acq_rel);
+}