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

[diamante0018]

Summary

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

[diamante0018]

I have undone an unrelated code formatting change.

[diamante0018]

I am learning how to use this differential tool and it appears the previous patch undid the changes I actually intended to commit instead of removing just the "one" offending line in regards to unrelated formatting changes.
I should have fixed the issue and now the actual diff file should be complete.
Sorry

[ldionne]

Thanks for the patch! If you upload the patch with arc, it should also include some context in the diff, which is useful. Let us know if you run into issues with that.

libcxx/include/latch

75

Not attached: Can you please add some tests for these assertions? To see how we do this for other assertions: libcxx/test/libcxx/containers/views/views.span/span.elem/assert.back.pass.cpp.

80–81

I think it is also UB if __expected > max(), right? If so, let's add an assertion.

91–92

I read:

If n is greater than the value of the internal counter or is negative, the behavior is undefined.

So we can actually check more than just __update >= 0.

112–113

Same here:

If n is greater than the value of the internal counter or is negative, the behavior is undefined.

[diamante0018]

libcxx/include/latch

80–81

You are right but in this implementation because max is equivalent to std::numeric_limits<ptrdiff_t>::max() it should not be possible

[ldionne]

libcxx/include/latch

80–81

Then the compiler can probably elide the check, but we should still have the check!

[diamante0018]

I addressed the review.

Please note the following:
Because in the llvm's implementation max() returns the max value of ptrdiff_t" (numeric_limits<ptrdiff_t>::max();) It should not be possible to check for that in an assertion.

Note that I used memory_order_relaxed in the assertions as that seems to be the memory order that has the potential to cause less collateral damage. Let me know if it's fine please.

Thanks

[diamante0018]

libcxx/include/latch

80–81

I see, I just saw your comment. I will add the check right away.

[diamante0018]

Added an assertions to check that when the latch object is constructed "n" is not greater than max

[diamante0018]

libcxx/include/latch

80–81

I added the check. That should be the last point to address from your previous review. I hope all is good now. Thanks 🐱

[diamante0018]

I added the check for "n is greater than max()". That should be the last point to address from your previous review. I hope all is good now. Thanks 🐱

[ldionne]

libcxx/include/latch

95–99

Could we instead move the _LIBCPP_ASSERT after the fetch_sub, i.e. do:

_LIBCPP_ASSERT_UNCATEGORIZED(
            __update >= 0, "latch::count_down called with a negative value");
auto const __old = __a_.fetch_sub(__update, memory_order_release);
_LIBCPP_ASSERT_UNCATEGORIZED(
            __update > __old,
            "__update is greater than the value of the internal counter");

This would not require an additional atomic load, yet we would catch that we just did something really really bad (i.e. that __a_ is now negative and who knows what will happen next).

libcxx/test/libcxx/thread/thread.latch/assert.arrive_and_wait.pass.cpp

1

Let's also add a assert.ctor.pass.cpp test to test the assertion you added in the constructor!

[diamante0018]

Added ctor unit test.
Correct assertion placement.
Remove one assertion as I realized that immediately after we are calling a function with the same kind of assertion. So perhaps it's not worth it to call .load() to check for the internal counter when we have a similar test right after that is checked without calling any extra operations

[diamante0018]

I am a bit concerned about these new tests apparently failing because I, unfortunately, I don't know why they are failing despite me checking them over and over again for mistakes. I need to learn all of this.

[diamante0018]

I realized by looking at already existing tests that my ctor.pass is incorrect. I will try to fix it in a few hours

[diamante0018]

Correct the tests files (hopefully)

[diamante0018]

Correct syntax in a test cpp file

[diamante0018]

Make sure the tests messages match the assertions messages from libcxx (should fix the tests)

[diamante0018]

Fix logic in the assertions I added

[diamante0018]

Add // class latch comment to all test files

[diamante0018]

Fix the test

[ldionne]

libcxx/include/latch

117–118

To avoid being inconsistent, I think I would remove this assertion and add a comment saying // preconditions on __update are checked in count_down().

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

33 ↗

(On Diff #537303)

You should do this instead:

TEST_LIBCPP_ASSERT_FAILURE([]{ std::latch foo(-1); }(), "assertion message");
// and add a comment that we can't check the precondition for max() because there's no value that would violate the precondition (in our implementation)

[diamante0018]

Address review

[diamante0018]

Hopefully, that should do it. I am happy I made progress with these tests earlier today. Was a bit hard to figure out but now they should turn green.

[diamante0018]

Apply clang-format on the lambda function

[diamante0018]

Fix format of my patch

[diamante0018]

Update tests

[diamante0018]

Fix logic in my assertions

[diamante0018]

Change "<" to "<=" in assert call

[diamante0018]

Test CI finally turned green woot woot. Thanks @Idionne 🥳

[ldionne]

LGTM, thanks! If you need someone to commit this on your behalf, please let us know Author Name <email@domain> to use for attribution.

[diamante0018]

Yes please:

Edoardo Sanguineti [e-mail]

Thanks again

[ldionne]

Thank *you* for the patch!

libcxx/include/latch

     }
 
     inline _LIBCPP_INLINE_VISIBILITY
-    constexpr explicit latch(ptrdiff_t __expected) : __a_(__expected) { }
+    constexpr explicit latch(ptrdiff_t __expected) : __a_(__expected)
+    {
+        _LIBCPP_ASSERT_UNCATEGORIZED(__expected >= 0,
+                                     "__expected cannot be negative");
+        _LIBCPP_ASSERT_UNCATEGORIZED(__expected > max(),
+                                     "__expected cannot be greater than max()");
+    }
 
     _LIBCPP_HIDE_FROM_ABI ~latch() = default;
     latch(const latch&) = delete;

     inline _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     void count_down(ptrdiff_t __update = 1)
     {
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            __update >= 0, "latch::count_down called with a negative value");
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            __update > __a_.load(memory_order_relaxed),
+            "__update is greater than the value of the internal counter");
+
         auto const __old = __a_.fetch_sub(__update, memory_order_release);
         if(__old == __update)
             __a_.notify_all();

     inline _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     void arrive_and_wait(ptrdiff_t __update = 1)
     {
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            __update >= 0, "latch::arrive_and_wait called with a negative value");
+        _LIBCPP_ASSERT_UNCATEGORIZED(
+            __update > __a_.load(memory_order_relaxed),
+            "__update is greater than the value of the internal counter");
+
         count_down(__update);
         wait();
     }

libcxx/test/libcxx/thread/thread.latch/assert.arrive_and_wait.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: c++03, c++11, c++14, c++17
+
+// <latch>
+//
+// void arrive_and_wait(ptrdiff_t __update = 1);
+
+// Make sure that calling arrive_and_wait with a negative value or a value
+// higher than the internal counter triggers an assertion.
+
+// REQUIRES: has-unix-headers
+// XFAIL: availability-verbose_abort-missing
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_ASSERTIONS=1
+
+#include <latch>
+
+#include "check_assertion.h"
+
+int main(int, char **) {
+  {
+    std::latch l(5);
+
+    TEST_LIBCPP_ASSERT_FAILURE(
+        l.arrive_and_wait(-10),
+        "latch::arrive_and_wait() called with a negative value");
+  }
+
+  {
+    std::latch l(5);
+
+    TEST_LIBCPP_ASSERT_FAILURE(
+        l.arrive_and_wait(10),
+        "latch::arrive_and_wait() called with value greater "
+        "than the internal counter");
+  }
+
+  return 0;
+}

libcxx/test/libcxx/thread/thread.latch/assert.count_down.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: c++03, c++11, c++14, c++17
+
+// <latch>
+//
+// void count_down(ptrdiff_t __update = 1);
+
+// Make sure that calling arrive_and_wait with a negative value or a value
+// higher than the internal counter triggers an assertion.
+
+// REQUIRES: has-unix-headers
+// XFAIL: availability-verbose_abort-missing
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_ASSERTIONS=1
+
+#include <latch>
+
+#include "check_assertion.h"
+
+int main(int, char **) {
+  {
+    std::latch l(5);
+
+    TEST_LIBCPP_ASSERT_FAILURE(
+        l.count_down(-10), "latch::count_down() called with a negative value");
+  }
+
+  {
+    std::latch l(5);
+
+    TEST_LIBCPP_ASSERT_FAILURE(l.count_down(10),
+                               "latch::count_down() called with value greater "
+                               "than the internal counter");
+  }
+
+  return 0;
+}