[libcxx] Fix hang in try_acquire with __atomic_semaphore_base

[• Quuxplusone]

Summary

On platforms with _LIBCPP_NO_NATIVE_SEMAPHORES, try_acquire blocks instead of returning false.

This is because libcpp_thread_poll_with_backoff interprets zero as meaning infinite, causing try_acquire to wait indefinitely when atomic_semaphore_base is used.

[• Quuxplusone]

libcxx/include/semaphore

111

So __libcpp_thread_poll_with_backoff treats 0=infinite, but __libcpp_semaphore_wait_timed (line 167) treats 0=0? That sounds like a recipe for bugs. Is there any appetite to just make libc++ treat 0=0 uniformly across the board?
(Then, if someone wants to wait for a billion seconds, they just pass 1000000000 as the timeout — I don't see any practical need to overload 0 with that extra meaning.)

[pabusse]

libcxx/include/semaphore

111

That's correct.

I didn't modify the semantics of __libcpp_thread_poll_with_backoff straight away as it is also used in some other contexts with zero-intending-infinite. I agree however that this might be the right change to make.

[__simt__]

libcxx/include/semaphore

111

A large timeout would still mean you have to call it in a loop, whereas the current arrangement avoids that.

But we could make the interface more clear to avoid this kind of bug, I agree we should try.

[• Quuxplusone]

@ldionne, now that we're using __atomic_semaphore_base unconditionally, I think this patch has become super important. I confirm that the new test really does hang forever on my Macbook, and that this change to <semaphore> fixes the hang.

I think we should land this ASAP in both main and 13.x (assuming you're still planning to cherry-pick D110110). Having D110110 without this patch too, is a pretty bad state to be in.

[ldionne]

In D98334#3110625, @Quuxplusone wrote:

@ldionne, now that we're using __atomic_semaphore_base unconditionally, I think this patch has become super important. I confirm that the new test really does hang forever on my Macbook, and that this change to <semaphore> fixes the hang.

I think we should land this ASAP in both main and 13.x (assuming you're still planning to cherry-pick D110110). Having D110110 without this patch too, is a pretty bad state to be in.

Agreed. Thanks for rebasing, Arthur. Let's ship this and cherry-pick to 13.x.

libcxx/include/semaphore

void acquire()
             return (__old != 0) && __a.compare_exchange_strong(__old, __old - 1, memory_order_acquire, memory_order_relaxed);
         };
         __cxx_atomic_wait(&__a.__a_, __test_fn);
     }
     template <class Rep, class Period>
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     bool try_acquire_for(chrono::duration<Rep, Period> const& __rel_time)
     {
-        auto const __test_fn = [this]() -> bool {
-            auto __old = __a.load(memory_order_acquire);
-            while(1) {
-                if (__old == 0)
-                    return false;
-                if(__a.compare_exchange_strong(__old, __old - 1, memory_order_acquire, memory_order_relaxed))
-                    return true;
-            }
-        };
-        return __libcpp_thread_poll_with_backoff(__test_fn, __libcpp_timed_backoff_policy(), __rel_time);
+        if (__rel_time == chrono::duration<Rep, Period>::zero())
+            return try_acquire();
+        auto const __test_fn = [this]() { return try_acquire(); };
+        return __libcpp_thread_poll_with_backoff(__test_fn, __libcpp_timed_backoff_policy(), __rel_time);
+    }
+    _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
+    bool try_acquire()
+    {
+        while (true) {
+            if (__a.compare_exchange_strong(__old, __old - 1, memory_order_acquire, memory_order_relaxed))
     }
 };
 
 #define _LIBCPP_SEMAPHORE_MAX (numeric_limits<ptrdiff_t>::max())
 
 template<ptrdiff_t __least_max_value = _LIBCPP_SEMAPHORE_MAX>
 class counting_semaphore
 {
public:
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     bool try_acquire_for(chrono::duration<Rep, Period> const& __rel_time)
     {
         return __semaphore.try_acquire_for(chrono::duration_cast<chrono::nanoseconds>(__rel_time));
     }
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     bool try_acquire()
     {
-        return try_acquire_for(chrono::nanoseconds::zero());
+        return __semaphore.try_acquire();
     }
     template <class Clock, class Duration>
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     bool try_acquire_until(chrono::time_point<Clock, Duration> const& __abs_time)
     {
         auto const current = Clock::now();
-        if(current >= __abs_time)
+        if (current >= __abs_time)
             return try_acquire();
         else
             return try_acquire_for(__abs_time - current);
     }
 };
 
 using binary_semaphore = counting_semaphore<1>;
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP_STD_VER >= 14
 
 _LIBCPP_POP_MACROS
 
 #endif //_LIBCPP_SEMAPHORE

libcxx/test/std/thread/thread.semaphore/try_acquire.pass.cpp

 #include "make_test_thread.h"
 #include "test_macros.h"
 
 int main(int, char**)
 {
   std::counting_semaphore<> s(1);
 
   assert(s.try_acquire());
+  assert(!s.try_acquire());
   s.release();
   assert(s.try_acquire());
+  assert(!s.try_acquire());
   s.release(2);
   std::thread t = support::make_test_thread([&](){
     assert(s.try_acquire());
   });
   t.join();
   assert(s.try_acquire());
+  assert(!s.try_acquire());
 
   return 0;
 }