[libcxx] Diagnose invalid memory order arguments in <atomic>. Fixes PR21179.

[EricWF]

Summary

This patch uses the attribute((enable_if)) hack suggested by @rsmith to diagnose invalid arguments when possible.

In order to diagnose an invalid argument m to f(m) we provide an additional overload of f that is only enabled when m is invalid. When that function is enabled it uses attribute((unavailable)) to produce a diagnostic message.

[jfb]

Awesome, thanks for doing this!

Should this be a warning or an error?

include/atomic

581

This isn't checking for the following requirement from the standard:

The failure argument shall be no stronger than the success argument.

I think that's OK because I intend to remove that requirement from C++ :)

Should we nonetheless enforce the requirement until the standard drops it? If so, "stronger" isn't well defined by the standard, details here: https://github.com/jfbastien/papers/blob/master/source/D0418r1.bs

test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp

30

Could you test that the other memory orders don't have any diagnostic (here and below).

55

For cmpxchg (weak and strong), should you also test the version with only a success ordering? The failure one is auto-generated from success, but it would be good to know that it never generates a diagnostic.

[bcraig]

include/atomic

569

what about relaxed?

[jfb]

include/atomic

569

Load relaxed is valid.

[EricWF]

include/atomic

581

Because "stronger" lacks a solid definition I choose not to implement checks for that requirement. I believe Clang does as well.

I think that's OK because I intend to remove that requirement from C++ :)

Yet another reason to not implement it.

[EricWF]

Add tests for valid memory orderings as well. Originally I figured the rest of the test suite should cover them but there's no harm in more tests.

[bcraig]

include/atomic

569

Gah, read the logic backwards... and even backwards my comments are incomplete.

[jfb]

Two comments, then lgtm.

include/atomic

581

OK that's totally fine with me. Could you put this in a comment? Something like "not checking the 'stronger' requirement, see wg21.link/p0418" (it'll be published in the pre-Issaquah meeting, URL will work then).

test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp

87

Could you do the other 5 (same below).

[EricWF]

Address inline comments from @jfb and add him as a reviewer.

test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp

87

I could, but we only check the two argument version of the overload, so I *know* the other orderings will have the same result. I think the one test is sufficient because it will fail the second we start testing the one argument version.

Do you still want the additional tests?

[EricWF]

Add comment about checking "stronger" orderings.

[jfb]

test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp

88

That's not quite true: the failure ordering is auto-deduced from the success one, but it's not necessarily the same! The spec says all success is valid, so the auto-mapping has to ensure that all failures are also valid. That's what I'm trying to have you test: that the auto-mapping is always valid as well.

[EricWF]

test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp

88

Right, but the auto-mapping is done once we have entered the compare_exchange_weak function, These diagnostics are triggered within the function signature. So even if we got the auto mapping wrong this test would not be able to diagnose it.

I agree we should be testing the auto-mapping, but I don't think this test is the right place to do it.

include/atomic

 #error <atomic> is not implemented
 #endif
 
 #if _LIBCPP_STD_VER > 14
 // FIXME: use the right feature test macro value as chose by SG10.
 # define __cpp_lib_atomic_is_always_lock_free 201603L
 #endif
 
+
+#if __has_attribute(__enable_if__) && __has_attribute(__unavailable__)
+# define _LIBCPP_CHECK_ATOMIC_MEMORY_ORDER
+# define _LIBCPP_CHECK_LOAD_MEMORY_ORDER(__m)                                 \
+    __attribute__ ((__enable_if__(__m == memory_order_release                 \
+                               || __m == memory_order_acq_rel, "")))          \
+    __attribute__ ((__unavailable__("memory order argument to atomic operation is invalid")))
+
+# define _LIBCPP_CHECK_STORE_MEMORY_ORDER(__m)                                \
+    __attribute__ ((__enable_if__(__m == memory_order_consume                 \
+                               || __m == memory_order_acquire                 \
+                               || __m == memory_order_acq_rel, "")))          \
+    __attribute__ ((__unavailable__("memory order argument to atomic operation is invalid")))
+
+# define _LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__s, __f)                        \
+    __attribute__ ((__enable_if__(__f == memory_order_release                 \
+                               || __f == memory_order_acq_rel, "")))          \
+    __attribute__ ((__unavailable__("memory order argument to atomic operation is invalid")))
+#endif
+
+
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 typedef enum memory_order
 {
     memory_order_relaxed, memory_order_consume, memory_order_acquire,
     memory_order_release, memory_order_acq_rel, memory_order_seq_cst
 } memory_order;
 
#endif
     _LIBCPP_INLINE_VISIBILITY
     bool compare_exchange_strong(_Tp& __e, _Tp __d,
                               memory_order __m = memory_order_seq_cst) volatile _NOEXCEPT
         {return __c11_atomic_compare_exchange_strong(&__a_, &__e, __d, __m, __m);}
     _LIBCPP_INLINE_VISIBILITY
     bool compare_exchange_strong(_Tp& __e, _Tp __d,
                                  memory_order __m = memory_order_seq_cst) _NOEXCEPT
         {return __c11_atomic_compare_exchange_strong(&__a_, &__e, __d, __m, __m);}
+#if defined(_LIBCPP_CHECK_ATOMIC_MEMORY_ORDER)
+    void store(_Tp __d, memory_order __m) volatile _LIBCPP_CHECK_STORE_MEMORY_ORDER(__m);
+    void store(_Tp __d, memory_order __m) _LIBCPP_CHECK_STORE_MEMORY_ORDER(__m);
+
+    _Tp load(memory_order __m) const volatile _LIBCPP_CHECK_LOAD_MEMORY_ORDER(__m);
+    _Tp load(memory_order __m) const _LIBCPP_CHECK_LOAD_MEMORY_ORDER(__m);
+    bool compare_exchange_weak(_Tp&, _Tp, memory_order __s, memory_order __f) volatile
+        _LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__s, __f);
+    bool compare_exchange_weak(_Tp&, _Tp, memory_order __s, memory_order __f)
+        _LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__s, __f);
+    bool compare_exchange_strong(_Tp&, _Tp, memory_order __s, memory_order __f) volatile
+        _LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__s, __f);
+    bool compare_exchange_strong(_Tp&, _Tp, memory_order __s, memory_order __f)
+        _LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__s, __f);
+#endif
 
     _LIBCPP_INLINE_VISIBILITY
 #ifndef _LIBCPP_HAS_NO_DEFAULTED_FUNCTIONS
     __atomic_base() _NOEXCEPT = default;
 #else
     __atomic_base() _NOEXCEPT : __a_() {}
 #endif // _LIBCPP_HAS_NO_DEFAULTED_FUNCTIONS
 

 bool
 atomic_compare_exchange_strong_explicit(atomic<_Tp>* __o, _Tp* __e,
                                         _Tp __d,
                                         memory_order __s, memory_order __f) _NOEXCEPT
 {
     return __o->compare_exchange_strong(*__e, __d, __s, __f);
 }
 
+
+#ifdef _LIBCPP_CHECK_ATOMIC_MEMORY_ORDER
+template <class _Tp>
+void atomic_store_explicit(volatile atomic<_Tp>*, _Tp, memory_order __m)
+_LIBCPP_CHECK_STORE_MEMORY_ORDER(__m);
+
+template <class _Tp>
+void atomic_store_explicit(atomic<_Tp>*, _Tp, memory_order __m)
+_LIBCPP_CHECK_STORE_MEMORY_ORDER(__m);
+
+template <class _Tp>
+_Tp atomic_load_explicit(const volatile atomic<_Tp>*, memory_order __m)
+_LIBCPP_CHECK_LOAD_MEMORY_ORDER(__m);
+
+template <class _Tp>
+_Tp atomic_load_explicit(const atomic<_Tp>*, memory_order __m)
+_LIBCPP_CHECK_LOAD_MEMORY_ORDER(__m);
+
+template <class _Tp>
+bool atomic_compare_exchange_weak_explicit(volatile atomic<_Tp>*, _Tp*, _Tp,
+                                           memory_order __s, memory_order __f)
+_LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__s, __f);
+
+template <class _Tp>
+bool atomic_compare_exchange_weak_explicit(atomic<_Tp>*, _Tp*, _Tp,
+                                           memory_order __s, memory_order __f)
+_LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__s, __f);
+
+template <class _Tp>
+bool atomic_compare_exchange_strong_explicit(volatile atomic<_Tp>*, _Tp*, _Tp,
+                                             memory_order __s, memory_order __f)
+_LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__s, __f);
+
+template <class _Tp>
+bool atomic_compare_exchange_strong_explicit(atomic<_Tp>*, _Tp*, _Tp,
+                                             memory_order __s, memory_order __f)
+_LIBCPP_CHECK_EXCHANGE_MEMORY_ORDER(__s, __f);
+
+#endif
+
 // atomic_fetch_add
 
 template <class _Tp>
 inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
     is_integral<_Tp>::value && !is_same<_Tp, bool>::value,
     _Tp

test/libcxx/atomics/diagnose_invalid_memory_order.fail.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// The attributes used to diagnose incorrect memory order inputs is clang
+// specific.
+// UNSUPPORTED: gcc
+
+// <atomic>
+
+// Test that invalid memory order arguments are diagnosed where possible.
+
+#include <atomic>
+
+int main() {
+    std::atomic<int> x(42);
+    volatile std::atomic<int>& vx = x;
+    int val1 = 1;
+    int val2 = 2;
+    // load operations
+    {
+        x.load(std::memory_order_release); // expected-error {{operation is invalid}}
+        x.load(std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+        vx.load(std::memory_order_release); // expected-error {{operation is invalid}}
+        vx.load(std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+    }
+    {
+        std::atomic_load_explicit(&x, std::memory_order_release); // expected-error {{operation is invalid}}
+        std::atomic_load_explicit(&x, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+        std::atomic_load_explicit(&vx, std::memory_order_release); // expected-error {{operation is invalid}}
+        std::atomic_load_explicit(&vx, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+    }
+    // store operations
+    {
+        x.store(42, std::memory_order_consume); // expected-error {{operation is invalid}}
+        x.store(42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+        x.store(42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+        vx.store(42, std::memory_order_consume); // expected-error {{operation is invalid}}
+        vx.store(42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+        vx.store(42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+    }
+    {
+        std::atomic_store_explicit(&x, 42, std::memory_order_consume); // expected-error {{operation is invalid}}
+        std::atomic_store_explicit(&x, 42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+        std::atomic_store_explicit(&x, 42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+        std::atomic_store_explicit(&vx, 42, std::memory_order_consume); // expected-error {{operation is invalid}}
+        std::atomic_store_explicit(&vx, 42, std::memory_order_acquire); // expected-error {{operation is invalid}}
+        std::atomic_store_explicit(&vx, 42, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+    }
+    // compare exchange weak
+    {
+        x.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+        x.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+        vx.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+        vx.compare_exchange_weak(val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+    }
+    {
+        std::atomic_compare_exchange_weak_explicit(&x, &val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+        std::atomic_compare_exchange_weak_explicit(&x, &val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+        std::atomic_compare_exchange_weak_explicit(&vx, &val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+        std::atomic_compare_exchange_weak_explicit(&vx, &val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+    }
+    // compare exchange strong
+    {
+        x.compare_exchange_strong(val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+        x.compare_exchange_strong(val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+        vx.compare_exchange_strong(val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+        vx.compare_exchange_strong(val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+    }
+    {
+        std::atomic_compare_exchange_strong_explicit(&x, &val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+        std::atomic_compare_exchange_strong_explicit(&x, &val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+        std::atomic_compare_exchange_strong_explicit(&vx, &val1, val2, std::memory_order_seq_cst, std::memory_order_release); // expected-error {{operation is invalid}}
+        std::atomic_compare_exchange_strong_explicit(&vx, &val1, val2, std::memory_order_seq_cst, std::memory_order_acq_rel); // expected-error {{operation is invalid}}
+    }
+}