[clang] [libc++] Fix inconsistency between is_lock_free and is_always_lock_free (PR #68109)

Louis Dionne via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 17 17:06:00 PDT 2023


https://github.com/ldionne updated https://github.com/llvm/llvm-project/pull/68109

>From 84ae453ad32cad94038bdb1e91b4b5c422f3ceb3 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Tue, 3 Oct 2023 10:06:12 -0400
Subject: [PATCH 1/2] [libc++] Fix inconsistency between is_lock_free and
 is_always_lock_free

std::atomic is implemented with the following (confusing!) hierarchy of
types:

     std::atomic<T> : std::__atomic_base<T> { ... };
     std::__atomic_base<T> {
          std::__cxx_atomic_impl<T> __impl;
     };
     std::__cxx_atomic_impl<T> {
          _Atomic(T) __val;
     };

Inside std::__atomic_base, we implement the is_lock_free() and
is_always_lock_free() functions. However, we used to implement them
inconsistently:
- is_always_lock_free() is based on whether __cxx_atomic_impl<T> is always
  lock free (using the builtin), which means that we include any potential
  padding added by _Atomic(T) into the determination.
- is_lock_free() was based on whether T is lock free (using the builtin),
  which meant that we did not take into account any potential padding
  added by _Atomic(T).

It is important to note that the padding added by _Atomic(T) can turn a
type that wouldn't be lock free into a lock free type, for example by
making its size become a power of two.

The inconsistency of how the two functions were implemented could lead
to cases where is_always_lock_free() would return true, but is_lock_free()
would then return false. This is the case for example of the following
type, which is always lock free on arm64 but was incorrectly reported as
!is_lock_free() before this patch:

     struct Foo { float x[3]; };

This patch switches the determination of is_lock_free() to be based on
__cxx_atomic_impl<T> instead to match how we determine is_always_lock_free().

rdar://115324353
---
 libcxx/include/__atomic/atomic_base.h         |  2 +-
 .../atomics/atomics.align/align.pass.cpp      |  1 +
 .../isalwayslockfree.pass.cpp                 |  2 +-
 .../atomics.types.generic/address.pass.cpp    |  7 +++++--
 .../atomics.types.generic/bool.pass.cpp       | 21 +++++++++++++------
 .../atomics.types.generic/integral.pass.cpp   |  7 +++++--
 .../atomic_is_lock_free.pass.cpp              |  4 ++++
 7 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/libcxx/include/__atomic/atomic_base.h b/libcxx/include/__atomic/atomic_base.h
index 87100ba5d8a50db..775d06d75701833 100644
--- a/libcxx/include/__atomic/atomic_base.h
+++ b/libcxx/include/__atomic/atomic_base.h
@@ -39,7 +39,7 @@ struct __atomic_base  // false
 
     _LIBCPP_HIDE_FROM_ABI
     bool is_lock_free() const volatile _NOEXCEPT
-        {return __cxx_atomic_is_lock_free(sizeof(_Tp));}
+        {return __cxx_atomic_is_lock_free(sizeof(__cxx_atomic_impl<_Tp>));}
     _LIBCPP_HIDE_FROM_ABI
     bool is_lock_free() const _NOEXCEPT
         {return static_cast<__atomic_base const volatile*>(this)->is_lock_free();}
diff --git a/libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp b/libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp
index 495d02fbe5c8d44..f9e01bd5d032bd8 100644
--- a/libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp
+++ b/libcxx/test/libcxx/atomics/atomics.align/align.pass.cpp
@@ -100,6 +100,7 @@ int main(int, char**) {
   CHECK_ALIGNMENT(struct Empty {});
   CHECK_ALIGNMENT(struct OneInt { int i; });
   CHECK_ALIGNMENT(struct IntArr2 { int i[2]; });
+  CHECK_ALIGNMENT(struct FloatArr3 { float i[3]; });
   CHECK_ALIGNMENT(struct LLIArr2 { long long int i[2]; });
   CHECK_ALIGNMENT(struct LLIArr4 { long long int i[4]; });
   CHECK_ALIGNMENT(struct LLIArr8 { long long int i[8]; });
diff --git a/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp b/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp
index b2d83f0a6fe8814..6d6e6477bc2511e 100644
--- a/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.lockfree/isalwayslockfree.pass.cpp
@@ -21,7 +21,6 @@
 template <typename T>
 void checkAlwaysLockFree() {
   if (std::atomic<T>::is_always_lock_free) {
-    LIBCPP_ASSERT(sizeof(std::atomic<T>) == sizeof(T)); // technically not required, but libc++ does it that way
     assert(std::atomic<T>().is_lock_free());
   }
 }
@@ -79,6 +78,7 @@ void run()
     CHECK_ALWAYS_LOCK_FREE(struct Empty {});
     CHECK_ALWAYS_LOCK_FREE(struct OneInt { int i; });
     CHECK_ALWAYS_LOCK_FREE(struct IntArr2 { int i[2]; });
+    CHECK_ALWAYS_LOCK_FREE(struct FloatArr3 { float i[3]; });
     CHECK_ALWAYS_LOCK_FREE(struct LLIArr2 { long long int i[2]; });
     CHECK_ALWAYS_LOCK_FREE(struct LLIArr4 { long long int i[4]; });
     CHECK_ALWAYS_LOCK_FREE(struct LLIArr8 { long long int i[8]; });
diff --git a/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
index b3aa1fc47629a3b..f5119cc74821bf2 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
@@ -80,8 +80,11 @@ do_test()
     typedef typename std::remove_pointer<T>::type X;
     A obj(T(0));
     assert(obj == T(0));
-    bool b0 = obj.is_lock_free();
-    ((void)b0); // mark as unused
+    {
+        bool lockfree = obj.is_lock_free();
+        if (A::is_always_lock_free)
+            assert(lockfree);
+    }
     obj.store(T(0));
     assert(obj == T(0));
     obj.store(T(1), std::memory_order_release);
diff --git a/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
index 78234ae6d96305a..a7ee5d0b78325d4 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
@@ -61,8 +61,11 @@ int main(int, char**)
     {
         volatile std::atomic<bool> obj(true);
         assert(obj == true);
-        bool b0 = obj.is_lock_free();
-        (void)b0; // to placate scan-build
+        {
+            bool lockfree = obj.is_lock_free();
+            if (std::atomic<bool>::is_always_lock_free)
+                assert(lockfree);
+        }
         obj.store(false);
         assert(obj == false);
         obj.store(true, std::memory_order_release);
@@ -112,8 +115,11 @@ int main(int, char**)
     {
         std::atomic<bool> obj(true);
         assert(obj == true);
-        bool b0 = obj.is_lock_free();
-        (void)b0; // to placate scan-build
+        {
+            bool lockfree = obj.is_lock_free();
+            if (std::atomic<bool>::is_always_lock_free)
+                assert(lockfree);
+        }
         obj.store(false);
         assert(obj == false);
         obj.store(true, std::memory_order_release);
@@ -163,8 +169,11 @@ int main(int, char**)
     {
         std::atomic_bool obj(true);
         assert(obj == true);
-        bool b0 = obj.is_lock_free();
-        (void)b0; // to placate scan-build
+        {
+            bool lockfree = obj.is_lock_free();
+            if (std::atomic_bool::is_always_lock_free)
+                assert(lockfree);
+        }
         obj.store(false);
         assert(obj == false);
         obj.store(true, std::memory_order_release);
diff --git a/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
index 058db2dc3ab049f..1905b1b34071c03 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
@@ -98,8 +98,11 @@ do_test()
 {
     A obj(T(0));
     assert(obj == T(0));
-    bool b0 = obj.is_lock_free();
-    ((void)b0); // mark as unused
+    {
+        bool lockfree = obj.is_lock_free();
+        if (A::is_always_lock_free)
+            assert(lockfree);
+    }
     obj.store(T(0));
     assert(obj == T(0));
     obj.store(T(1), std::memory_order_release);
diff --git a/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp b/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp
index 8b838f62abb1d32..39fa837f4807bf6 100644
--- a/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp
@@ -27,8 +27,12 @@ struct TestFn {
   void operator()() const {
     typedef std::atomic<T> A;
     T t = T();
+
     A a(t);
     bool b1 = std::atomic_is_lock_free(static_cast<const A*>(&a));
+    if (A::is_always_lock_free)
+      assert(b1);
+
     volatile A va(t);
     bool b2 = std::atomic_is_lock_free(static_cast<const volatile A*>(&va));
     assert(b1 == b2);

>From 491a3a128f2e49a56895c62cf9d54f4ddac96cba Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Tue, 17 Oct 2023 17:05:27 -0700
Subject: [PATCH 2/2] Fix CI failures in C++11/03/14

---
 .../std/atomics/atomics.types.generic/address.pass.cpp   | 3 +++
 .../test/std/atomics/atomics.types.generic/bool.pass.cpp | 9 +++++++++
 .../std/atomics/atomics.types.generic/integral.pass.cpp  | 3 +++
 3 files changed, 15 insertions(+)

diff --git a/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
index f5119cc74821bf2..0926628a2e9a895 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/address.pass.cpp
@@ -82,8 +82,11 @@ do_test()
     assert(obj == T(0));
     {
         bool lockfree = obj.is_lock_free();
+        (void)lockfree;
+#if TEST_STD_VER >= 17
         if (A::is_always_lock_free)
             assert(lockfree);
+#endif
     }
     obj.store(T(0));
     assert(obj == T(0));
diff --git a/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
index a7ee5d0b78325d4..2609e5b56e798de 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/bool.pass.cpp
@@ -63,8 +63,11 @@ int main(int, char**)
         assert(obj == true);
         {
             bool lockfree = obj.is_lock_free();
+            (void)lockfree;
+#if TEST_STD_VER >= 17
             if (std::atomic<bool>::is_always_lock_free)
                 assert(lockfree);
+#endif
         }
         obj.store(false);
         assert(obj == false);
@@ -117,8 +120,11 @@ int main(int, char**)
         assert(obj == true);
         {
             bool lockfree = obj.is_lock_free();
+            (void)lockfree;
+#if TEST_STD_VER >= 17
             if (std::atomic<bool>::is_always_lock_free)
                 assert(lockfree);
+#endif
         }
         obj.store(false);
         assert(obj == false);
@@ -171,8 +177,11 @@ int main(int, char**)
         assert(obj == true);
         {
             bool lockfree = obj.is_lock_free();
+            (void)lockfree;
+#if TEST_STD_VER >= 17
             if (std::atomic_bool::is_always_lock_free)
                 assert(lockfree);
+#endif
         }
         obj.store(false);
         assert(obj == false);
diff --git a/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
index 1905b1b34071c03..2695ff94da30658 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
@@ -100,8 +100,11 @@ do_test()
     assert(obj == T(0));
     {
         bool lockfree = obj.is_lock_free();
+        (void)lockfree;
+#if TEST_STD_VER >= 17
         if (A::is_always_lock_free)
             assert(lockfree);
+#endif
     }
     obj.store(T(0));
     assert(obj == T(0));



More information about the cfe-commits mailing list