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

[diamante0018]

Summary

Adding assertions will aid users that have bugs or logic mistakes in their code to receive error messages when debugging.

[diamante0018]

Fix typo in my code

[diamante0018]

Remove static_assert. Do not want to add chrono include unless asked to.

[diamante0018]

Fix logic error in test file

[diamante0018]

Use std::counting_semaphore<>::max()

[diamante0018]

Undo last change to test file

[diamante0018]

Use new define for assertions

[diamante0018]

remove xfail flag and see if tests pass on mac

[diamante0018]

Add XFAILS flag for macos runners

[diamante0018]

Use std::counting_semaphore<>::max()

[EricWF]

LGTM, though I spot 4 new checks and 3 new tests. Is there one missing?

libcxx/include/semaphore

97–98

Please put an empty { } block here to make it more clear that this is intentional. Maybe a comment as well.

98

Shouldn't this check be using ::max() rather than the default _LIBPCP_SEMAPHORE_MAX?

143

We should probably add a static assertion that __least_max_value is positive.

[diamante0018]

You are correct in observing there are four new assertion but only three checks. There are three checks because one cannot be tested for. max::() is equivalent to the max value of ptrdiff_t so there is no number that can violate this precondition.
I was asked by @ldionne in a previous review to leave the assertion anyway for an identical situation in D154425.
I will see if I can address your other comments with a diff. Thanks for the review.

libcxx/include/semaphore

98

Unfortunately I don't think I can. ::max() is a definition that is not part of this class and is defined down in this file so it should not be an available definition right here.

[diamante0018]

Address review comments.

[diamante0018]

Add test for new static_assert

[diamante0018]

Add xfail flags that should be required

[diamante0018]

Address review comments from Discord

[diamante0018]

maybe unused requires c++17, used void

[ldionne]

libcxx/include/semaphore

102

I suggest we move to __old > 0 as a drive-by fix too, since it's more consistent with the rest of the code (and it's easier to read, although that is subjective).

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

11

This REQUIRES shouldn't be there, otherwise we're basically requiring an old system that didn't have the synchronization library. Along with the XFAIL below, this means that this test was never run.

libcxx/test/libcxx/thread/thread.semaphore/assert.release.pass.cpp

11–12

Same, both these lines should be removed.

libcxx/test/std/thread/thread.semaphore/ctor.compile.pass.cpp

33–36

This test is a .compile.pass.cpp, so your expected-error comment is never picked up. You want to move this to a separate .verify.cpp test instead.

[diamante0018]

Address review comments

[diamante0018]

I hope the tests succeed on macosx because they always seem to fail there and that's why I thought about adding the xfail flag. Let's see if removing both does the trick.

[ldionne]

LGTM assuming CI is green. Thanks!

[diamante0018]

fix warnings in test file

[diamante0018]

Use better macro for enabling assertions

[diamante0018]

FIx verify test

[diamante0018]

Attempt to fix test

[diamante0018]

Try to fix verify.cpp test file

[diamante0018]

Fix test finally

[diamante0018]

Added new UNSUPPORTED flag, fixing tests.

[diamante0018]

Since CI could be considered green since only one runner caused unrelated issues I'm gonna leave my details here for the landing of this revision since I don't have commit access. Thanks to the reviewers.

Edoardo Sanguineti <[e-mail]>

libcxx/include/semaphore

 /*
 
 __atomic_semaphore_base is the general-case implementation.
 It is a typical Dijkstra semaphore algorithm over atomics, wait and notify
 functions. It avoids contention against users' own use of those facilities.
 
 */
 
+#define _LIBCPP_SEMAPHORE_MAX (numeric_limits<ptrdiff_t>::max())
+
 class __atomic_semaphore_base
 {
     __atomic_base<ptrdiff_t> __a_;
 
 public:
     _LIBCPP_INLINE_VISIBILITY
     constexpr explicit __atomic_semaphore_base(ptrdiff_t __count) : __a_(__count)
     {
     }
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     void release(ptrdiff_t __update = 1)
     {
-        if(0 < __a_.fetch_add(__update, memory_order_release))
-            ;
-        else if(__update > 1)
+        auto __old = __a_.fetch_add(__update, memory_order_release);
+        _LIBCPP_ASSERT_UNCATEGORIZED(__update <= _LIBCPP_SEMAPHORE_MAX - __old, "update is greater than the expected value");
+
+        if (0 < __old)
+        {
+            // Nothing to do
+        }
+        else if (__update > 1)
             __a_.notify_all();
         else
             __a_.notify_one();
     }
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     void acquire()
     {
         auto const __test_fn = [this]() -> bool {
bool try_acquire()
             if (__old == 0)
                 return false;
             if (__a_.compare_exchange_strong(__old, __old - 1, memory_order_acquire, memory_order_relaxed))
                 return true;
         }
     }
 };
 
-#define _LIBCPP_SEMAPHORE_MAX (numeric_limits<ptrdiff_t>::max())
-
 template<ptrdiff_t __least_max_value = _LIBCPP_SEMAPHORE_MAX>
 class counting_semaphore
 {
     __atomic_semaphore_base __semaphore_;
 
 public:
+    static_assert(__least_max_value >= 0, "The least maximum value must be a positive number");
+
     static constexpr ptrdiff_t max() noexcept {
         return __least_max_value;
     }
 
     _LIBCPP_INLINE_VISIBILITY
-    constexpr explicit counting_semaphore(ptrdiff_t __count) : __semaphore_(__count) { }
+    constexpr explicit counting_semaphore(ptrdiff_t __count) : __semaphore_(__count)
+    {
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            __count >= 0,
+            "counting_semaphore::counting_semaphore(ptrdiff_t): counting_semaphore cannot be "
+            "initialized with a negative value");
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            __count <= max(),
+            "counting_semaphore::counting_semaphore(ptrdiff_t): counting_semaphore cannot be "
+            "initialized with a value greater than max()");
+    }
     ~counting_semaphore() = default;
 
     counting_semaphore(const counting_semaphore&) = delete;
     counting_semaphore& operator=(const counting_semaphore&) = delete;
 
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     void release(ptrdiff_t __update = 1)
     {
+        _LIBCPP_ASSERT_UNCATEGORIZED(__update >= 0, "counting_semaphore:release called with a negative value");
         __semaphore_.release(__update);
     }
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     void acquire()
     {
         __semaphore_.acquire();
     }
     template<class Rep, class Period>

libcxx/test/libcxx/thread/thread.semaphore/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
+
+// <semaphore>
+
+// constexpr explicit counting_semaphore(ptrdiff_t __count);
+
+// Make sure that constructing counting_semaphore with a negative value triggers an assertion
+
+#include <semaphore>
+
+#include "check_assertion.h"
+
+int main(int, char**) {
+  {
+    TEST_LIBCPP_ASSERT_FAILURE(
+        [] { std::counting_semaphore<> s(-1); }(),
+        "counting_semaphore::counting_semaphore(ptrdiff_t): counting_semaphore 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;
+}

libcxx/test/libcxx/thread/thread.semaphore/assert.release.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
+
+// <semaphore>
+
+// void release(ptrdiff_t __update = 1);
+
+// Make sure that calling release with a negative value triggers or with a value
+// greater than expected triggers an assertion
+
+#include <semaphore>
+
+#include "check_assertion.h"
+
+int main(int, char**) {
+  {
+    std::counting_semaphore<> s(2);
+    TEST_LIBCPP_ASSERT_FAILURE(s.release(-1), "counting_semaphore:release called with a negative value");
+  }
+
+  {
+    // Call release with an arbitrary larger than expected value
+    std::counting_semaphore<> s(2);
+    TEST_LIBCPP_ASSERT_FAILURE(
+        s.release(std::counting_semaphore<>::max()), "update is greater than the expected value");
+  }
+
+  return 0;
+}

libcxx/test/std/thread/thread.semaphore/ctor.compile.pass.cpp

 static_assert(!std::is_convertible<int, std::binary_semaphore>::value, "");
 static_assert(!std::is_convertible<int, std::counting_semaphore<>>::value, "");
 
 #if TEST_STD_VER > 17
 // Test constexpr-constructibility. (But not destructibility.)
 constinit std::binary_semaphore bs(1);
 constinit std::counting_semaphore<> cs(1);
 #endif
+
+void not_positive() {
+  // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}}The least maximum value must be a positive number}}
+  std::counting_semaphore<-1> s(2);
+  (void)s;
+}