[libc++] add basic runtime assertions to <barrier>

[diamante0018]

Summary

Adding assertions will aid users that have bugs in their code to receive better error messages.

[diamante0018]

Remove reduntant function declaration

[ldionne]

libcxx/include/barrier

318

I see this:

The behavior is undefined if n is less than or equal to 0 or greater than the expected count for the current barrier phase.

It seems that we're not checking the second condition here. I think that's something we could check from __barrier_base::arrive.

[diamante0018]

Address review

[diamante0018]

I think the up-to-date code should capture and assert that as well. I added the test file to confirm the correctness of the assertion

[diamante0018]

Fix logic in the asserts

[diamante0018]

Note: the tests appear to have failed but in reality it's just two sole time outs out of the entire CI run.

[diamante0018]

add test for both ctors of barrier, remove extra compiler flag from test file that might be causing issues (fails) on the mac runners.

[diamante0018]

Push patch with context

[diamante0018]

Fix CI runner not being able to create branch (rebase)

[diamante0018]

Add xfail flag present in other barrier test files

[ldionne]

LGTM with comments applied and green CI. Thanks!

libcxx/test/libcxx/thread/thread.barrier/assert.arrive.pass.cpp

22–24

We generally try to group Lit annotations like this. Please move those above right under the XFAIL. Here and elsewhere.

libcxx/test/libcxx/thread/thread.barrier/assert.ctor.pass.cpp

39

constexpr and noexcept are not necessary here, and they make the reader wonder whether they are relevant to the test. I'd remove them.

[diamante0018]

Address review comments

[diamante0018]

Fix compilation error in my patch I missed

[diamante0018]

Change flag for tests file in attempt to fix CI on apply-backdeployment

[diamante0018]

Add xfail flags for macos builds specifically

[diamante0018]

As this CI turned green and should be approved, please use the following details when landing it. However, if the XFAILs flags needs to be modified please let me know.

Edoardo Sanguineti <[e-mail]>

Thanks to the reviewers here and on the Discord server

libcxx/include/barrier

__barrier_base(ptrdiff_t __expected, _CompletionF __completion = _CompletionF())
             : __expected_(__expected), __base_(std::__construct_barrier_algorithm_base(this->__expected_),
                                                &__destroy_barrier_algorithm_base),
               __expected_adjustment_(0), __completion_(std::move(__completion)), __phase_(0)
     {
     }
     [[__nodiscard__]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     arrival_token arrive(ptrdiff_t __update)
     {
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            __update <= __expected_, "update is greater than the expected count for the current barrier phase");
+
         auto const __old_phase = __phase_.load(memory_order_relaxed);
         for(; __update; --__update)
             if(__arrive_barrier_algorithm_base(__base_.get(), __old_phase)) {
                 __completion_();
                 __expected_ += __expected_adjustment_.load(memory_order_relaxed);
                 __expected_adjustment_.store(0, memory_order_relaxed);
                 __phase_.store(__old_phase + 2, memory_order_release);
                 __phase_.notify_all();
public:
     {
     }
     [[nodiscard]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     arrival_token arrive(ptrdiff_t update)
     {
         auto const __old_phase = __phase.load(memory_order_relaxed);
         auto const __result = __arrived.fetch_sub(update, memory_order_acq_rel) - update;
         auto const new_expected = __expected.load(memory_order_relaxed);
-        if(0 == __result) {
+
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            update <= new_expected, "update is greater than the expected count for the current barrier phase");
+
+        if (0 == __result) {
             __completion();
             __arrived.store(new_expected, memory_order_relaxed);
             __phase.store(!__old_phase, memory_order_release);
             __phase.notify_all();
         }
         return __old_phase;
     }
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
explicit inline __barrier_base(ptrdiff_t __count, __empty_completion = __empty_completion())
         : __phase_arrived_expected(__init(__count))
     {
     }
     [[nodiscard]] inline _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     arrival_token arrive(ptrdiff_t update)
     {
         auto const __inc = __arrived_unit * update;
         auto const __old = __phase_arrived_expected.fetch_add(__inc, memory_order_acq_rel);
-        if((__old ^ (__old + __inc)) & __phase_bit) {
+
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            update <= __old, "update is greater than the expected count for the current barrier phase");
+
+        if ((__old ^ (__old + __inc)) & __phase_bit) {
             __phase_arrived_expected.fetch_add((__old & __expected_mask) << 32, memory_order_relaxed);
             __phase_arrived_expected.notify_all();
         }
         return __old & __phase_bit;
     }
     inline _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     void wait(arrival_token&& __phase) const
     {
public:
 
     static _LIBCPP_HIDE_FROM_ABI constexpr ptrdiff_t max() noexcept {
         return __barrier_base<_CompletionF>::max();
     }
 
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     explicit barrier(ptrdiff_t __count, _CompletionF __completion = _CompletionF())
         : __b_(__count, _VSTD::move(__completion)) {
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            __count >= 0,
+            "barrier::barrier(ptrdiff_t, CompletionFunction): barrier cannot be initialized with a negative value");
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            __count <= max(),
+            "barrier::barrier(ptrdiff_t, CompletionFunction): barrier cannot be initialized with "
+            "a value greater than max()");
     }
 
     barrier(barrier const&) = delete;
     barrier& operator=(barrier const&) = delete;
 
     [[__nodiscard__]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     arrival_token arrive(ptrdiff_t __update = 1)
     {
+        _LIBCPP_ASSERT_UNCATEGORIZED(__update > 0, "barrier:arrive must be called with a value greater than 0");
         return __b_.arrive(__update);
     }
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     void wait(arrival_token&& __phase) const
     {
         __b_.wait(_VSTD::move(__phase));
     }
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY

libcxx/test/libcxx/thread/thread.barrier/assert.arrive.pass.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
+//
+//===----------------------------------------------------------------------===//
+// UNSUPPORTED: no-threads
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// REQUIRES: availability-synchronization_library-missing
+// XFAIL: target={{.+}}-apple-macosx10.{{13|15}}
+
+// REQUIRES: has-unix-headers
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_ASSERTIONS=1
+
+// <barrier>
+
+// class barrier;
+
+// void arrive(ptrdiff_t __update = 1);
+
+// Make sure that calling arrive with a negative value or with a value greater than 'expected' triggers an assertion
+
+#include <barrier>
+
+#include "check_assertion.h"
+
+int main(int, char**) {
+  {
+    std::barrier<> b(5);
+    TEST_LIBCPP_ASSERT_FAILURE(b.arrive(0), "barrier:arrive must be called with a value greater than 0");
+  }
+
+  {
+    std::barrier<> b(5);
+    TEST_LIBCPP_ASSERT_FAILURE(b.arrive(10), "update is greater than the expected count for the current barrier phase");
+  }
+
+  return 0;
+}

libcxx/test/libcxx/thread/thread.barrier/assert.ctor.pass.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
+//
+//===----------------------------------------------------------------------===//
+// UNSUPPORTED: no-threads
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// REQUIRES: availability-synchronization_library-missing
+// XFAIL: target={{.+}}-apple-macosx10.{{13|15}}
+
+// REQUIRES: has-unix-headers
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_ASSERTIONS=1
+
+// <barrier>
+
+// class barrier;
+
+// barrier(ptrdiff_t __count, _CompletionF __completion = _CompletionF());
+
+// Make sure that constructing barrier with a negative value triggers an assertion
+
+#include <barrier>
+
+#include "check_assertion.h"
+
+int main(int, char**) {
+  {
+    TEST_LIBCPP_ASSERT_FAILURE(
+        [] { std::barrier<> b(-1); }(),
+        "barrier::barrier(ptrdiff_t, CompletionFunction): barrier cannot be initialized with a negative value");
+  }
+
+  {
+    TEST_LIBCPP_ASSERT_FAILURE(
+        [] {
+          auto completion = []() {};
+          std::barrier<decltype(completion)> b(-1, completion);
+        }(),
+        "barrier::barrier(ptrdiff_t, CompletionFunction): barrier cannot be initialized with a negative value");
+  }
+
+  // We can't check the precondition for max() because there's no value
+  // that would violate the precondition (in our implementation)
+
+  return 0;
+}