[huixie90]

Fixes #180334

Problem

The semaphore timed wait test is flaky on Windows. It hangs from time to time.

Some examples:

windows (clang-cl-no-vcruntime, false, clang-cl, clang-cl)
windows (mingw-static, true, cc, c++)
windows (clang-cl-static, false, clang-cl, clang-cl)
windows (clang-cl-dll, false, clang-cl, clang-cl)
windows (mingw-static, true, cc, c++)

Root Cause

The internal dylib function takes a timeout

static void __platform_wait_on_address(void const* __ptr, void const* __val, uint64_t __timeout_ns) 

We followed the same convention as __libcpp_thread_poll_with_backoff, where we used 0ns to indicate wait indefinitely until being notified.

_LIBCPP_HIDE_FROM_ABI __poll_with_backoff_results __libcpp_thread_poll_with_backoff(
    _Poll&& __poll, _Backoff&& __backoff, chrono::nanoseconds __max_elapsed = chrono::nanoseconds::zero())

This is problematic, if the caller indeed wants to wait 0ns and passes 0, the internal dylib function __platform_wait_on_address would wait indefinitely

__timeout_ns == 0 ? INFINITE : static_cast<DWORD>(__timeout_ns / 1'000'000)

This is what actually happened here. So the fix is to update internal dylib function to use optional and use nullopt to indicate wait indefinitely

static void __platform_wait_on_address(void const* __ptr, void const* __val, optional<uint64_t> __timeout_ns) 

Edit: after code review, the code is updated to use tag type NoTimeout to indicate "wait indefinitely". this is superior because it is coded into the type system instead of runtime check

Why __libcpp_thread_poll_with_backoff never suffered from this problem?

__libcpp_thread_poll_with_backoff has this " 0ns means wait indefinitely " semantic for years (it has always been like that), but it never causes issues. This is because, it has

chrono::nanoseconds const __elapsed = chrono::high_resolution_clock::now() - __start;
if (__max_elapsed != chrono::nanoseconds::zero() && __max_elapsed < __elapsed)
      return __poll_with_backoff_results::__timeout;

__max_elapsed is what user passed in, let's assume the user passed in 0ns, and __elapsed is certainly a positive number so it never goes to the backoff function and directly returned. No hanging possible

Why Windows and we passed in 0ns ?

So in the test, we passed wait some time, say 1ms, from the semaphore public API, which calls __libcpp_thread_poll_with_backoff with 1ms timeout. __libcpp_thread_poll_with_backoff will do some polling loops and then calling into the backoff function, platform timed wait in this case, with timeout of 1ms - elapsed , say 950us.
However, Windows platform wait has millisecond precision, so 950us is rounded down to 0ms (static_cast<DWORD>(__timeout_ns / 1'000'000)), so the function call almost immediately returns, and __libcpp_thread_poll_with_backoff will keep its polling loop like this. As time goes by in the polling loop, the timeout for platform wait will decrease from 950us to smaller and smaller number.

In the __libcpp_thread_poll_with_backoff

if (__max_elapsed != chrono::nanoseconds::zero() && __max_elapsed < __elapsed)
      return __poll_with_backoff_results::__timeout;
if (auto __backoff_res = __backoff(__elapsed); __backoff_res == __backoff_results::__continue_poll)

__max_elapsed is user requested timeout, which is 1ms in this case, and __elapsed gradually increases and eventually, if it becomes greater than 1ms, we have __max_elapsed < __elapsed, it will return and test passes. all Good. But there is a slim chance that on one loop, __elapsed is exactly the same number of the user requested __max_elapsed 1ms, so __max_elapsed == __elapsed, this will make the code path go to backoff platform wait, with the timeout __max_elapsed - __elapsed == 0ns. So now we are requesting __platform_wait_on_address to wait exactly 0ns, and due to our ambiguous API, 0ns means wait indefinitely

__timeout_ns == 0 ? INFINITE : static_cast<DWORD>(__timeout_ns / 1'000'000)

The test will just hang forever.

Why not __max_elapsed <= __elapsed ?

in __libcpp_thread_poll_with_backoff, If we check __max_elapsed <= __elapsed instead of __max_elapsed < __elapsed, we would avoid the call to platform wait with 0ns. But according to the standard, I think the current __max_elapsed < __elapsed is more correct

The timeout expires ([thread.req.timing]) when the current time is after abs_time (for try_acquire_until) or when at least rel_time has passed from the start of the function (for try_acquire_for).

https://eel.is/c++draft/thread.sema#cnt-18.2

So this after means that it needs strictly greater than i think.

The Fix

The fix is to update the internal dylib API to use nullopt to indicate wait indefinitely. So a call to platform wait with 0ns will not cause a hang.

Edit: after code review, the code is updated to use tag type NoTimeout to indicate "wait indefinitely". this is superior because it is coded into the type system instead of runtime check

I made a small change as well. it is very easy to get into a situation where the requested platform wait timeout is <1ms, we will be keep calling Windows platform wait with 0ns because of the rounding, and this is effectively a spin lock . I made a change such that if the requested timeout is between 100us to 1ms, just rounded up to 1ms to wait a bit longer (which is conforming IIUC) . let me know if this change is necessary, happy to take this part out if it is not considered good.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Hui (huixie90)

Changes

Fixes #180334

---

Full diff: https://github.com/llvm/llvm-project/pull/180398.diff

3 Files Affected:

• (modified) libcxx/src/atomic.cpp (+21-19)
• (added) libcxx/test/std/thread/thread.semaphore/timed.hang.pass.cpp (+45)
• (modified) libcxx/test/std/thread/thread.semaphore/timed.pass.cpp (+1-2)

diff --git a/libcxx/src/atomic.cpp b/libcxx/src/atomic.cpp
index 637c971ed8896..4d05815c76929 100644
--- a/libcxx/src/atomic.cpp
+++ b/libcxx/src/atomic.cpp
@@ -14,6 +14,7 @@
 #include <cstring>
 #include <functional>
 #include <new>
+#include <optional>
 #include <thread>
 #include <type_traits>
 
@@ -64,17 +65,17 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 #ifdef __linux__
 
 template <std::size_t _Size>
-static void __platform_wait_on_address(void const* __ptr, void const* __val, uint64_t __timeout_ns) {
+static void __platform_wait_on_address(void const* __ptr, void const* __val, optional<uint64_t> __timeout_ns) {
   static_assert(_Size == 4, "Can only wait on 4 bytes value");
   char buffer[_Size];
   std::memcpy(&buffer, const_cast<const void*>(__val), _Size);
   static constexpr timespec __default_timeout = {2, 0};
   timespec __timeout;
-  if (__timeout_ns == 0) {
+  if (!__timeout_ns.has_value()) {
     __timeout = __default_timeout;
   } else {
-    __timeout.tv_sec  = __timeout_ns / 1'000'000'000;
-    __timeout.tv_nsec = __timeout_ns % 1'000'000'000;
+    __timeout.tv_sec  = *__timeout_ns / 1'000'000'000;
+    __timeout.tv_nsec = *__timeout_ns % 1'000'000'000;
   }
   _LIBCPP_FUTEX(__ptr, FUTEX_WAIT_PRIVATE, *reinterpret_cast<__cxx_contention_t const*>(&buffer), &__timeout, 0, 0);
 }
@@ -97,11 +98,12 @@ extern "C" int __ulock_wake(uint32_t operation, void* addr, uint64_t wake_value)
 #  define ULF_WAKE_ALL 0x00000100
 
 template <std::size_t _Size>
-static void __platform_wait_on_address(void const* __ptr, void const* __val, uint64_t __timeout_ns) {
+static void __platform_wait_on_address(void const* __ptr, void const* __val, optional<uint64_t> __timeout_ns) {
   static_assert(_Size == 8 || _Size == 4, "Can only wait on 8 bytes or 4 bytes value");
   char buffer[_Size];
   std::memcpy(&buffer, const_cast<const void*>(__val), _Size);
-  auto __timeout_us = __timeout_ns == 0 ? 0 : static_cast<uint32_t>(__timeout_ns / 1000);
+  auto __timeout_us =
+      !__timeout_ns.has_value() ? uint32_t(0) : std::max(static_cast<uint32_t>(*__timeout_ns / 1000), uint32_t(1));
   if constexpr (_Size == 4)
     __ulock_wait(
         UL_COMPARE_AND_WAIT, const_cast<void*>(__ptr), *reinterpret_cast<uint32_t const*>(&buffer), __timeout_us);
@@ -128,16 +130,16 @@ static void __platform_wake_by_address(void const* __ptr, bool __notify_one) {
  */
 
 template <std::size_t _Size>
-static void __platform_wait_on_address(void const* __ptr, void const* __val, uint64_t __timeout_ns) {
+static void __platform_wait_on_address(void const* __ptr, void const* __val, optional<uint64_t> __timeout_ns) {
   static_assert(_Size == 8, "Can only wait on 8 bytes value");
   char buffer[_Size];
   std::memcpy(&buffer, const_cast<const void*>(__val), _Size);
-  if (__timeout_ns == 0) {
+  if (!__timeout_ns.has_value()) {
     _umtx_op(const_cast<void*>(__ptr), UMTX_OP_WAIT, *reinterpret_cast<__cxx_contention_t*>(&buffer), nullptr, nullptr);
   } else {
     _umtx_time ut;
-    ut._timeout.tv_sec  = __timeout_ns / 1'000'000'000;
-    ut._timeout.tv_nsec = __timeout_ns % 1'000'000'000;
+    ut._timeout.tv_sec  = *__timeout_ns / 1'000'000'000;
+    ut._timeout.tv_nsec = *__timeout_ns % 1'000'000'000;
     ut._flags           = 0;               // Relative time (not absolute)
     ut._clockid         = CLOCK_MONOTONIC; // Use monotonic clock
 
@@ -182,7 +184,7 @@ static void* win32_get_synch_api_function(const char* function_name) {
 }
 
 template <std::size_t _Size>
-static void __platform_wait_on_address(void const* __ptr, void const* __val, uint64_t __timeout_ns) {
+static void __platform_wait_on_address(void const* __ptr, void const* __val, optional<uint64_t> __timeout_ns) {
   static_assert(_Size == 8, "Can only wait on 8 bytes value");
   // WaitOnAddress was added in Windows 8 (build 9200)
   static auto wait_on_address =
@@ -191,12 +193,12 @@ static void __platform_wait_on_address(void const* __ptr, void const* __val, uin
     wait_on_address(const_cast<void*>(__ptr),
                     const_cast<void*>(__val),
                     _Size,
-                    __timeout_ns == 0 ? INFINITE : static_cast<DWORD>(__timeout_ns / 1'000'000));
+                    !__timeout_ns.has_value() ? INFINITE : static_cast<DWORD>(*__timeout_ns / 1'000'000));
   } else {
     __libcpp_thread_poll_with_backoff(
         [=]() -> bool { return std::memcmp(const_cast<const void*>(__ptr), __val, _Size) != 0; },
         __libcpp_timed_backoff_policy(),
-        std::chrono::nanoseconds(__timeout_ns));
+        std::chrono::nanoseconds(__timeout_ns.value_or(0)));
   }
 }
 
@@ -231,7 +233,7 @@ static void __platform_wake_by_address(void const* __ptr, bool __notify_one) {
 // Baseline is just a timed backoff
 
 template <std::size_t _Size>
-static void __platform_wait_on_address(void const* __ptr, void const* __val, uint64_t __timeout_ns) {
+static void __platform_wait_on_address(void const* __ptr, void const* __val, optional<uint64_t> __timeout_ns) {
   __libcpp_thread_poll_with_backoff(
       [=]() -> bool { return std::memcmp(const_cast<const void*>(__ptr), __val, _Size) != 0; },
       __libcpp_timed_backoff_policy(),
@@ -262,7 +264,7 @@ template <std::size_t _Size>
 static void __contention_wait(__cxx_atomic_contention_t* __waiter_count,
                               void const* __address_to_wait,
                               void const* __old_value,
-                              uint64_t __timeout_ns) {
+                              optional<uint64_t> __timeout_ns) {
   __cxx_atomic_fetch_add(__waiter_count, __cxx_contention_t(1), memory_order_relaxed);
   // https://llvm.org/PR109290
   // There are no platform guarantees of a memory barrier in the platform wait implementation
@@ -331,7 +333,7 @@ _LIBCPP_EXPORTED_FROM_ABI void
 __atomic_wait_global_table(void const* __location, __cxx_contention_t __old_value) noexcept {
   auto const __entry = __get_global_contention_state(__location);
   __contention_wait<sizeof(__cxx_atomic_contention_t)>(
-      &__entry->__waiter_count, &__entry->__platform_state, &__old_value, 0);
+      &__entry->__waiter_count, &__entry->__platform_state, &__old_value, nullopt);
 }
 
 _LIBCPP_EXPORTED_FROM_ABI void __atomic_wait_global_table_with_timeout(
@@ -353,7 +355,7 @@ _LIBCPP_EXPORTED_FROM_ABI void __atomic_notify_all_global_table(void const* __lo
 
 template <std::size_t _Size>
 _LIBCPP_EXPORTED_FROM_ABI void __atomic_wait_native(void const* __address, void const* __old_value) noexcept {
-  __contention_wait<_Size>(__get_native_waiter_count(__address), __address, __old_value, 0);
+  __contention_wait<_Size>(__get_native_waiter_count(__address), __address, __old_value, nullopt);
 }
 
 template <std::size_t _Size>
@@ -428,7 +430,7 @@ _LIBCPP_EXPORTED_FROM_ABI void
 __libcpp_atomic_wait(void const volatile* __location, __cxx_contention_t __old_value) noexcept {
   auto const __entry = __get_global_contention_state(const_cast<void const*>(__location));
   __contention_wait<sizeof(__cxx_atomic_contention_t)>(
-      &__entry->__waiter_count, &__entry->__platform_state, &__old_value, 0);
+      &__entry->__waiter_count, &__entry->__platform_state, &__old_value, nullopt);
 }
 
 _LIBCPP_EXPORTED_FROM_ABI void __cxx_atomic_notify_one(__cxx_atomic_contention_t const volatile* __location) noexcept {
@@ -447,7 +449,7 @@ _LIBCPP_EXPORTED_FROM_ABI void
 __libcpp_atomic_wait(__cxx_atomic_contention_t const volatile* __location, __cxx_contention_t __old_value) noexcept {
   auto __location_cast = const_cast<const void*>(static_cast<const volatile void*>(__location));
   __contention_wait<sizeof(__cxx_atomic_contention_t)>(
-      __get_native_waiter_count(__location_cast), __location_cast, &__old_value, 0);
+      __get_native_waiter_count(__location_cast), __location_cast, &__old_value, nullopt);
 }
 
 // this function is even unused in the old ABI
diff --git a/libcxx/test/std/thread/thread.semaphore/timed.hang.pass.cpp b/libcxx/test/std/thread/thread.semaphore/timed.hang.pass.cpp
new file mode 100644
index 0000000000000..75366bd4f5397
--- /dev/null
+++ b/libcxx/test/std/thread/thread.semaphore/timed.hang.pass.cpp
@@ -0,0 +1,45 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+
+// <semaphore>
+
+// This is a regression test for a bug in semaphore::try_acquire_for
+// where it can wait indefinitely
+// https://github.com/llvm/llvm-project/issues/180334
+
+#include <semaphore>
+#include <thread>
+#include <chrono>
+#include <cassert>
+
+#include "make_test_thread.h"
+#include "test_macros.h"
+
+void test() {
+  auto const start = std::chrono::steady_clock::now();
+  std::counting_semaphore<> s(0);
+
+  assert(!s.try_acquire_for(std::chrono::nanoseconds(1)));
+  assert(!s.try_acquire_for(std::chrono::microseconds(1)));
+  assert(!s.try_acquire_for(std::chrono::milliseconds(1)));
+  assert(!s.try_acquire_for(std::chrono::milliseconds(100)));
+
+  auto const end = std::chrono::steady_clock::now();
+  assert(end - start < std::chrono::seconds(10));
+}
+
+int main(int, char**) {
+  for (auto i = 0; i < 10; ++i) {
+    test();
+  }
+
+  return 0;
+}
diff --git a/libcxx/test/std/thread/thread.semaphore/timed.pass.cpp b/libcxx/test/std/thread/thread.semaphore/timed.pass.cpp
index d0aa1f46072b9..5b3aac6718075 100644
--- a/libcxx/test/std/thread/thread.semaphore/timed.pass.cpp
+++ b/libcxx/test/std/thread/thread.semaphore/timed.pass.cpp
@@ -19,8 +19,7 @@
 #include "make_test_thread.h"
 #include "test_macros.h"
 
-int main(int, char**)
-{
+int main(int, char**) {
   auto const start = std::chrono::steady_clock::now();
 
   std::counting_semaphore<> s(0);

[ldionne]

LGTM but I'd also like some folks who worked on the recent Windows patch related to this to have a look. Pinging some of them here @mstorsjo @RogerSanders @Andarwinux @Alcaro

Thanks for the fix!

[ldionne]

Once it lands, this should also be cherry-picked onto LLVM 22. Feel free to point the release manager to this comment for approval.

[Sir-Walrus]

The solution as described in the PR header does indeed sound like a good fix for the corresponding described problem. Good job debugging that, flaky tests are always a pain.

I (and the other people ldionne named) were indeed involved in a recent relevant PR (#163524), but I was more interested in DLL lifetime and win7 compat than timeouts. I am not familiar enough with libc++ to review the actual code changes.

[ldionne]

LGTM but I'd also like some folks who worked on the recent Windows patch related to this to have a look.

Just recording private communication: if we don't get any other reviews from Windows folks by end of this week, I think it's fine to merge. It would be nice, but it's more important to make the LLVM 22 release with this bug fix since we understand it and it fixes a (flaky) test failure in our CI.

[mstorsjo]

LGTM but I'd also like some folks who worked on the recent Windows patch related to this to have a look.

No objection from me; I'm not very familiar with these APIs involved, but the explanation of what is being done sounds reasonable.

[mstorsjo]

Sounds reasonable to me, no objections from me.

[huixie90]

/cherry-pick 8de1a07

[llvmbot]

Failed to cherry-pick: 8de1a07

https://github.com/llvm/llvm-project/actions/runs/21998883099

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

[huixie90]

Manually Created PR
#181415