[libcxx-commits] [PATCH] D68480: Implementation of C++20's P1135R6 for libcxx

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Here are some comments. More to come. A few things that I noticed more broadly:

- Make sure things are correctly enabled for C++ versions and tests are marked as unsupported before C++2a.
- A lot of stuff here won't work in C++03 mode. Either wrap it in version `#if`s or use C++03 supported versions.



================
Comment at: include/__threading_support:265
 
+__libcpp_timespec_t __libcpp_to_timespec(const chrono::nanoseconds& __ns) {
+
----------------
Make sure this is wrapped in an `#if !defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)` (It might already be but, I can't see the context). 


================
Comment at: include/__threading_support:509
+   auto const step = __ns.count();
+   assert(step < numeric_limits<unsigned>::max());
+   asm volatile("nanosleep.u32 %0;"::"r"((unsigned)step):);
----------------
I think runtime assertions aren't great in the standard library. 

If we keep this make sure you include `cassert`.


================
Comment at: include/__threading_support:522
+    for(int count = 0;;) {
+      if(__builtin_expect(__f(),1))
+        return true;
----------------
Should we put some ifdefs around `__builtin_expect`?


================
Comment at: include/__threading_support:566
+
+using __libcpp_platform_wait_t = int;
+
----------------
If this file is used before C++11, this should be a typedef.


================
Comment at: include/__threading_support:573
+
+template <class _Tp, typename std::enable_if<__libcpp_platform_wait_uses_type<_Tp>::value, int>::type = 1>
+void __libcpp_platform_wait(_Tp const* ptr, _Tp val, void const* timeout) {
----------------
Remove the `std::`. Also, you can use `_EnableIf`.


================
Comment at: include/__threading_support:587
+
+struct alignas(64) __libcpp_contention_t {
+#if defined(_LIBCPP_HAS_PLATFORM_WAIT)
----------------
Are you sure that `64` is always the correct alignment here? What if `__libcpp_platform_wait_t` is only 8 bits, or what if its 128 bits? The same could probably go for `ptrdiff_t` (but, I'm not sure about the exact requirements for it). 


================
Comment at: include/__threading_support:587
+
+struct alignas(64) __libcpp_contention_t {
+#if defined(_LIBCPP_HAS_PLATFORM_WAIT)
----------------
zoecarver wrote:
> Are you sure that `64` is always the correct alignment here? What if `__libcpp_platform_wait_t` is only 8 bits, or what if its 128 bits? The same could probably go for `ptrdiff_t` (but, I'm not sure about the exact requirements for it). 
Additionally, `alignas` won't work in C++03. You can use `aligned_storage` if you need. 


================
Comment at: include/atomic:633
 
+extern "C" int memcmp( const void * ptr1, const void * ptr2, size_t num );
+
----------------
Why can't you just use `std::memcmp`?


================
Comment at: include/barrier:94
+    inline _LIBCPP_INLINE_VISIBILITY
+    bool __arrive(__phase_t const old_phase)
+    {
----------------
__simt__ wrote:
> EricWF wrote:
> > `old_phase`, `half`, and friends all need to be reserved names.
> Yes to all these, but let's focus on the design for now.
I think there are race conditions in this function. Every so often, I will run a test and get a segfault (sometimes it passes and sometimes an assertion fails). 


================
Comment at: include/barrier:134
+                }
+                assert(round == 0 && expect == full);
+            }
----------------
I haven't looked into it enough to understand how big of an issue this is, but, the following program fails this assertion nearly every time: 

```
void test() {
  int n_threads = 100;
  std::vector<std::thread*> workers;

  std::barrier task_barrier(n_threads);

  for (int i = 0; i < n_threads; ++i) {
    workers.push_back(new std::thread([&] {
      while(true) {
        task_barrier.arrive_and_wait();
       }
     }));
  }
}
```


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68480/new/

https://reviews.llvm.org/D68480