[PATCH 2/6] libstdc++: Fix shared_timed_mutex for negative timeouts [PR116586]
Jonathan Wakely
jwakely@redhat.com
Wed Oct 8 20:49:16 GMT 2025
On Wed, 08 Oct 2025 at 21:21 +0100, Mike Crowe wrote:
>On Wednesday 08 October 2025 at 12:27:06 +0100, Jonathan Wakely wrote:
>> A few months ago I was testing the patch below, which replaces all the
>> repeated code that creates timespec objects with a call to a single
>> function. That function avoids overflows, e.g. if a timeout of
>> chrono::years::max() is used, and also turns negative timeouts into
>> the epoch.
>>
>> This avoids solving the same problems in several places across the
>> library, like the four places above in <shared_mutex>.
>>
>> I got distracted by other fixes and never submitted this patch to the
>> mailing list (we were in the final stages of gcc 15 when I last worked
>> on it). I think it's a good approach but would appreciate your review.
>
>In general I think that this is a great improvement. Consolidating this
>kind of code that needs extreme care to get right makes a lot of sense.
>
>> I think what I'll do is apply your patches locally on top of this one
>> (resolving the conflicts) and test everything again.
>
>Please let me know if you'd like me to have a go at doing that.
Don't worry, I can do that. I think only the patch for shared_mutex
will cause any conflicts, and they'll be trivial to resolve.
>Do you know of any platforms that actually have a floating-point time_t? I
>didn't realise that was possible until today and it looks like it was only
>really permitted in the hope of being useful and never really materialised
>according to https://www.austingroupbugs.net/view.php?id=327 .
Yeah, it's hypothetical as far as I know. But we're not just a POSIX
library, so it's possible that somebody will try to port libstdc++ to
an OS with a floating-point time_t one day. Unlikely though, so we
could just drop that part of the patch and maybe just leave a comment
saying "if your OS is weird, you need to make changes here".
>I've made a few minor comments on the patch itself below.
Thanks for the review, replies inline below.
>> commit 27a12d66c96ebc107cc997faa63372392436de33
>> Author: Jonathan Wakely <jwakely@redhat.com>
>> AuthorDate: Wed Apr 30 10:58:51 2025
>> Commit: Jonathan Wakely <redi@gcc.gnu.org>
>> CommitDate: Mon May 19 14:25:49 2025
>>
>> libstdc++: Avoid overflow in timeout conversions [PR113327]
>> When converting from a coarse duration with a very large value, the
>> existing code scales that up to chrono::seconds which overflows the
>> chrono::seconds::rep type. For example, sleep_for(chrono::hours::max())
>> tries to calculate LLONG_MAX * 3600, which overflows to -3600 and so the
>> sleep returns immediately.
>> The solution in this commit is inspired by this_thread::sleep_for in
>> libc++ which compares the duration argument to
>> chrono::duration<long double>(nanoseconds::max()) and limits the
>> duration to nanoseconds::max(). Because we split the duration into
>> seconds and nanoseconds, we can use seconds::max() as our upper limit.
>> We might need to limit further if seconds::max() doesn't fit in the
>> type used for sleeping, which is one of std::time_t, unsigned int, or
>> chrono::milliseconds.
>> libstdc++-v3/ChangeLog:
>> PR libstdc++/113327
>> PR libstdc++/119258
>> PR libstdc++/58931
>> * include/bits/chrono.h (__to_timespec): New overloaded function
>> templates for converting chrono types to timespec.
>> (__to_gthread_time_t): New function template for converting
>> time_point to __gthread_time_t.
>> * include/bits/this_thread_sleep.h (sleep_for): Use
>> __to_timespec.
>> (__sleep_for): Remove namespace-scope declaration.
>> * include/bits/atomic_timed_wait.h: Use __to_timespec and
>> __to_gthread_time_t for timeouts.
>> * include/std/condition_variable: Likewise.
>> * include/std/mutex: Likewise.
>> * include/std/shared_mutex: Likewise.
>> * src/c++11/thread.cc (limit): New helper function.
>> (__sleep_for): Use limit to prevent overflow when converting
>> chrono::seconds to time_t, unsigned, or chrono::milliseconds.
>> * testsuite/30_threads/this_thread/113327.cc: New test.
>>
>> diff --git a/libstdc++-v3/include/bits/atomic_timed_wait.h b/libstdc++-v3/include/bits/atomic_timed_wait.h
>> index 9a6ac95b7d0e..9b915d11f672 100644
>> --- a/libstdc++-v3/include/bits/atomic_timed_wait.h
>> +++ b/libstdc++-v3/include/bits/atomic_timed_wait.h
>> @@ -83,14 +83,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> const chrono::time_point<__wait_clock_t, _Dur>&
>> __atime) noexcept
>> {
>> - auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
>> - auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
>> -
>> - struct timespec __rt =
>> - {
>> - static_cast<std::time_t>(__s.time_since_epoch().count()),
>> - static_cast<long>(__ns.count())
>> - };
>> + struct timespec __rt = chrono::__to_timespec(__atime);
>> auto __e = syscall (SYS_futex, __addr,
>> static_cast<int>(__futex_wait_flags::
>> @@ -150,14 +143,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> static_assert(std::__is_one_of<_Clock, chrono::steady_clock,
>> chrono::system_clock>::value);
>> - auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
>> - auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
>> -
>> - __gthread_time_t __ts =
>> - {
>> - static_cast<std::time_t>(__s.time_since_epoch().count()),
>> - static_cast<long>(__ns.count())
>> - };
>> + __gthread_time_t __ts = chrono::__to_gthread_time_t(__atime);
>> #ifdef _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT
>> if constexpr (is_same_v<chrono::steady_clock, _Clock>)
>> diff --git a/libstdc++-v3/include/bits/chrono.h b/libstdc++-v3/include/bits/chrono.h
>> index fad216203d2f..55becc4d9aef 100644
>> --- a/libstdc++-v3/include/bits/chrono.h
>> +++ b/libstdc++-v3/include/bits/chrono.h
>> @@ -50,6 +50,9 @@
>> #include <bits/version.h>
>> +// TODO move __to_gthread_time_t to a better place
>> +#include <bits/gthr.h> // for __gthread_time_t +
>> namespace std _GLIBCXX_VISIBILITY(default)
>> {
>> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> @@ -1513,6 +1516,98 @@ _GLIBCXX_END_INLINE_ABI_NAMESPACE(_V2)
>> } // namespace filesystem
>> #endif // C++17 && HOSTED
>> +#if defined _GLIBCXX_USE_NANOSLEEP || defined _GLIBCXX_USE_CLOCK_REALTIME \
>> + || defined _GLIBCXX_HAS_GTHREADS
>> +#pragma GCC diagnostic push
>> +#pragma GCC diagnostic ignored "-Wc++17-extensions"
>> +namespace chrono
>> +{
>> +/// @cond undocumented
>> +
>> + // Convert a chrono::duration to a relative time represented as timespec
>> + // (e.g. for use with nanosleep).
>> + template<typename _Rep, typename _Period>
>> + [[__nodiscard__]] _GLIBCXX14_CONSTEXPR inline
>> + struct ::timespec
>> + __to_timespec(const duration<_Rep, _Period>& __d)
>
>IMO this function is really only useful for timeouts - the guaranteed
>non-negative behaviour is required for that use case. Calling the function
>__to_timeout_timespec() or __to_timespec_for_timeout() would make that
>clearer and stop someone expecting the function to be more general.
Good idea.
>> + {
>> + struct ::timespec __ts{};
>> +
>> + if (__d < __d.zero()) // Negative timeouts don't make sense.
>> + return __ts;
>> +
>> + if constexpr (__or_<ratio_greater<_Period, ratio<1>>,
>> + treat_as_floating_point<_Rep>>::value)
>> + {
>> + // Converting from e.g. chrono::hours::max() to chrono::seconds
>> + // would evaluate LLONG_MAX * 3600 which would overflow.
>> + // Limit to chrono::seconds::max().
>> + chrono::duration<double> __fmax(chrono::seconds::max());
>> + if (__d > __fmax) [[__unlikely__]]
>> + return chrono::__to_timespec(chrono::seconds::max());
>> + }
>> +
>> + auto __s = chrono::duration_cast<chrono::seconds>(__d);
>> +
>> + if constexpr (is_integral<time_t>::value) // POSIX.1-2001 allows floating
>> + {
>> + // Also limit to time_t maximum (only relevant for 32-bit time_t).
>> + constexpr auto __tmax = numeric_limits<time_t>::max();
>> + if (__s.count() > __tmax) [[__unlikely__]]
>> + {
>> + __ts.tv_sec = __tmax;
>> + return __ts;
>> + }
>> + }
>> +
>> + auto __ns = chrono::duration_cast<chrono::nanoseconds>(__d - __s);
>> +
>> + if constexpr (treat_as_floating_point<_Rep>::value)
>> + if (__ns.count() > 999999999) [[__unlikely__]]
>> + __ns = chrono::nanoseconds(999999999);
>
>This probably doesn't make any real difference, but shouldn't this be:
>
> {
> ++__s;
> __ns = 0;
> }
>
>to ensure that the timespec is at least as long as the provided duration?
>(Though I suppose we then need to worry about __s potentially overflowing
>too which is a pain for little gain.)
It's been a few months now, but I think my thinking was that we could
only get a value larger than 999'999'999 from rounding, and that if it
was _really_ larger than that, then the duration_cast<seconds>(d)
should have already give us a larger value for s.
I don't think it's possible to get a value here that's significantly
larger than 999'999'999, only a few nanos larger, and so just
truncating to 999'999'999 should be fine (the kernel will return
EINVAL if you pass a tv_nsec larger than that. which is why I bothered
to clamp it here at all).
>> +
>> + __ts.tv_sec = static_cast<time_t>(__s.count());
>> + __ts.tv_nsec = static_cast<long>(__ns.count());
>> + return __ts;
>> + }
>> +
>> + // Convert a chrono::time_point to an absolute time represented as timespec.
>> + // All times before the epoch get converted to the epoch, so this assumes
>> + // that we only use it for clocks where that's true.
>> + // It should be safe to use this for system_clock and steady_clock.
>> + template<typename _Clock, typename _Dur>
>> + [[__nodiscard__]] _GLIBCXX14_CONSTEXPR inline
>> + struct ::timespec
>> + __to_timespec(const time_point<_Clock, _Dur>& __t)
>> + {
>> + return chrono::__to_timespec(__t.time_since_epoch());
>> + }
>> +
>> +#ifdef _GLIBCXX_HAS_GTHREADS
>> + // Convert a time_point to an absolute time represented as __gthread_time_t
>> + // (which is typically just a typedef for struct timespec).
>> + template<typename _Clock, typename _Dur>
>> + [[__nodiscard__]] _GLIBCXX14_CONSTEXPR inline
>> + __gthread_time_t
>> + __to_gthread_time_t(const time_point<_Clock, _Dur>& __t)
>
>Similar comment to above for __to_timespec() about this only being useful
>for timeouts and so naming it __to_gthread_timeout_time_t() or
>__to_gthread_time_t_for_timeout(), though I admit that these names are
>getting rather unwieldy.
We don't need to use them too often, and they're not meant to be
public APIs that users would call. I think clear names are better.
>> + {
>> + auto __ts = chrono::__to_timespec(__t.time_since_epoch());
>> + if constexpr (is_same<::timespec, __gthread_time_t>::value)
>> + return __ts;
>> + else if constexpr (is_convertible<::timespec, __gthread_time_t>::value)
>> + return __ts;
>> + else if constexpr (is_scalar<__gthread_time_t>::value)
>> + return static_cast<__gthread_time_t>(__ts.tv_sec);
>> + else // Assume this works:
>> + return __gthread_time_t{ __ts.tv_sec, __ts.tv_nsec };
>> + }
>> +#endif // HAS_GTHREADS
>> +
>> +/// @endcond
>> +} // namespace chrono
>> +#pragma GCC diagnostic pop
>> +#endif // USE_NANOSLEEP || USE_CLOCK_REALTIME || HAS_GTHREADS
>> +
>> _GLIBCXX_END_NAMESPACE_VERSION
>> } // namespace std
>> diff --git a/libstdc++-v3/include/bits/this_thread_sleep.h b/libstdc++-v3/include/bits/this_thread_sleep.h
>> index 57f89f858952..bf52aa9cd1c5 100644
>> --- a/libstdc++-v3/include/bits/this_thread_sleep.h
>> +++ b/libstdc++-v3/include/bits/this_thread_sleep.h
>> @@ -36,6 +36,7 @@
>> #if __cplusplus >= 201103L
>> #include <bits/chrono.h> // std::chrono::*
>> +#include <ext/numeric_traits.h> // __int_traits
>> #ifdef _GLIBCXX_USE_NANOSLEEP
>> # include <cerrno> // errno, EINTR
>> @@ -59,11 +60,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> {
>> #ifndef _GLIBCXX_NO_SLEEP
>> -#ifndef _GLIBCXX_USE_NANOSLEEP
>> - void
>> - __sleep_for(chrono::seconds, chrono::nanoseconds);
>> -#endif
>> -
>> /// this_thread::sleep_for
>> template<typename _Rep, typename _Period>
>> inline void
>> @@ -71,18 +67,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> {
>> if (__rtime <= __rtime.zero())
>> return;
>> - auto __s = chrono::duration_cast<chrono::seconds>(__rtime);
>> - auto __ns = chrono::duration_cast<chrono::nanoseconds>(__rtime - __s);
>> +
>> + struct timespec __ts = chrono::__to_timespec(__rtime);
>> #ifdef _GLIBCXX_USE_NANOSLEEP
>> - struct ::timespec __ts =
>> - {
>> - static_cast<std::time_t>(__s.count()),
>> - static_cast<long>(__ns.count())
>> - };
>> while (::nanosleep(&__ts, &__ts) == -1 && errno == EINTR)
>> { }
>> #else
>> - __sleep_for(__s, __ns);
>> + using chrono::seconds;
>> + using chrono::nanoseconds;
>> + void __sleep_for(seconds __s, nanoseconds __ns);
>
>Could we just make __sleep_for() take a single nanoseconds parameter so
>that its implementation can just call __to_timespec()? Perhaps you'd need
>to keep the old two-parameter version for ABI compatibility anyway so it's
>not worth it?
Yes, we'd need to keep it anyway. Since we can't remove it, I didn't
think about adding an alternative to use instead.
>> + __sleep_for(seconds(__ts.tv_sec), nanoseconds(__ts.tv_nsec));
>> #endif
>> }
>> diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
>> index 3525ff35ba31..cbc399493db3 100644
>> --- a/libstdc++-v3/include/std/condition_variable
>> +++ b/libstdc++-v3/include/std/condition_variable
>> @@ -193,15 +193,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> __wait_until_impl(unique_lock<mutex>& __lock,
>> const chrono::time_point<steady_clock, _Dur>& __atime)
>> {
>> - auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
>> - auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
>> -
>> - __gthread_time_t __ts =
>> - {
>> - static_cast<std::time_t>(__s.time_since_epoch().count()),
>> - static_cast<long>(__ns.count())
>> - };
>> -
>> + __gthread_time_t __ts = chrono::__to_gthread_time_t(__atime);
>> _M_cond.wait_until(*__lock.mutex(), CLOCK_MONOTONIC, __ts);
>> return (steady_clock::now() < __atime
>> @@ -214,15 +206,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> __wait_until_impl(unique_lock<mutex>& __lock,
>> const chrono::time_point<system_clock, _Dur>& __atime)
>> {
>> - auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
>> - auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
>> -
>> - __gthread_time_t __ts =
>> - {
>> - static_cast<std::time_t>(__s.time_since_epoch().count()),
>> - static_cast<long>(__ns.count())
>> - };
>> -
>> + __gthread_time_t __ts = chrono::__to_gthread_time_t(__atime);
>> _M_cond.wait_until(*__lock.mutex(), __ts);
>> return (system_clock::now() < __atime
>> diff --git a/libstdc++-v3/include/std/mutex b/libstdc++-v3/include/std/mutex
>> index e575a81c138e..a4624b475411 100644
>> --- a/libstdc++-v3/include/std/mutex
>> +++ b/libstdc++-v3/include/std/mutex
>> @@ -179,14 +179,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> _M_try_lock_until(const chrono::time_point<chrono::system_clock,
>> _Duration>& __atime)
>> {
>> - auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
>> - auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
>> -
>> - __gthread_time_t __ts = {
>> - static_cast<std::time_t>(__s.time_since_epoch().count()),
>> - static_cast<long>(__ns.count())
>> - };
>> -
>> + __gthread_time_t __ts = chrono::__to_gthread_time_t(__atime);
>> return static_cast<_Derived*>(this)->_M_timedlock(__ts);
>> }
>> @@ -196,14 +189,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> _M_try_lock_until(const chrono::time_point<chrono::steady_clock,
>> _Duration>& __atime)
>> {
>> - auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
>> - auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
>> -
>> - __gthread_time_t __ts = {
>> - static_cast<std::time_t>(__s.time_since_epoch().count()),
>> - static_cast<long>(__ns.count())
>> - };
>> -
>> + __gthread_time_t __ts = chrono::__to_gthread_time_t(__atime);
>> return static_cast<_Derived*>(this)->_M_clocklock(CLOCK_MONOTONIC,
>> __ts);
>> }
>> diff --git a/libstdc++-v3/include/std/shared_mutex b/libstdc++-v3/include/std/shared_mutex
>> index 94c8532399d9..5936f9347899 100644
>> --- a/libstdc++-v3/include/std/shared_mutex
>> +++ b/libstdc++-v3/include/std/shared_mutex
>> @@ -520,15 +520,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> try_lock_until(const chrono::time_point<chrono::system_clock,
>> _Duration>& __atime)
>> {
>> - auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
>> - auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
>> -
>> - __gthread_time_t __ts =
>> - {
>> - static_cast<std::time_t>(__s.time_since_epoch().count()),
>> - static_cast<long>(__ns.count())
>> - };
>> -
>> + struct timespec __ts = chrono::__to_timespec(__atime);
>> int __ret = __glibcxx_rwlock_timedwrlock(&_M_rwlock, &__ts);
>> // On self-deadlock, we just fail to acquire the lock. Technically,
>> // the program violated the precondition.
>> @@ -546,15 +538,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> try_lock_until(const chrono::time_point<chrono::steady_clock,
>> _Duration>& __atime)
>> {
>> - auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
>> - auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
>> -
>> - __gthread_time_t __ts =
>> - {
>> - static_cast<std::time_t>(__s.time_since_epoch().count()),
>> - static_cast<long>(__ns.count())
>> - };
>> -
>> + struct timespec __ts = chrono::__to_timespec(__atime);
>> int __ret = pthread_rwlock_clockwrlock(&_M_rwlock, CLOCK_MONOTONIC,
>> &__ts);
>> // On self-deadlock, we just fail to acquire the lock. Technically,
>> @@ -596,14 +580,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> try_lock_shared_until(const chrono::time_point<chrono::system_clock,
>> _Duration>& __atime)
>> {
>> - auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
>> - auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
>> -
>> - __gthread_time_t __ts =
>> - {
>> - static_cast<std::time_t>(__s.time_since_epoch().count()),
>> - static_cast<long>(__ns.count())
>> - };
>> + struct timespec __ts = chrono::__to_timespec(__atime);
>> int __ret;
>> // Unlike for lock(), we are not allowed to throw an exception so if
>> @@ -636,15 +613,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> try_lock_shared_until(const chrono::time_point<chrono::steady_clock,
>> _Duration>& __atime)
>> {
>> - auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
>> - auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
>> -
>> - __gthread_time_t __ts =
>> - {
>> - static_cast<std::time_t>(__s.time_since_epoch().count()),
>> - static_cast<long>(__ns.count())
>> - };
>> -
>> + struct timespec __ts = chrono::__to_timespec(__atime);
>> int __ret = pthread_rwlock_clockrdlock(&_M_rwlock, CLOCK_MONOTONIC,
>> &__ts);
>> // On self-deadlock, we just fail to acquire the lock. Technically,
>> diff --git a/libstdc++-v3/src/c++11/thread.cc b/libstdc++-v3/src/c++11/thread.cc
>> index 6c2ec2978f88..0768a99d6741 100644
>> --- a/libstdc++-v3/src/c++11/thread.cc
>> +++ b/libstdc++-v3/src/c++11/thread.cc
>> @@ -231,10 +231,30 @@ namespace std _GLIBCXX_VISIBILITY(default)
>> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> namespace this_thread
>> {
>> +namespace
>> +{
>> + // returns min(s, Dur::max())
>> + template<typename Dur>
>> + inline chrono::seconds
>> + limit(chrono::seconds s)
>> + {
>> + static_assert(ratio_equal<typename Dur::period, ratio<1>>::value,
>> + "period must be seconds to avoid potential overflow");
>> +
>> + if (s > Dur::max()) [[__unlikely__]]
>> + s = chrono::duration_cast<chrono::seconds>(Dur::max());
>> + return s;
>> + }
>> +}
>> +
>> void
>> __sleep_for(chrono::seconds __s, chrono::nanoseconds __ns)
>> {
>> #ifdef _GLIBCXX_USE_NANOSLEEP
>> +#pragma GCC diagnostic ignored "-Wc++17-extensions"
>> + if constexpr (is_integral<time_t>::value) // POSIX.1-2001 allows floating
>> + __s = limit<chrono::duration<time_t>>(__s);
>> +
>> struct ::timespec __ts =
>> {
>> static_cast<std::time_t>(__s.count()),
>> @@ -246,6 +266,8 @@ namespace this_thread
>> const auto target = chrono::steady_clock::now() + __s + __ns;
>> while (true)
>> {
>> + __s = limit<chrono::duration<unsigned>>(__s);
>> +
>> unsigned secs = __s.count();
>> if (__ns.count() > 0)
>> {
>> @@ -271,11 +293,19 @@ namespace this_thread
>> break;
>> __s = chrono::duration_cast<chrono::seconds>(target - now);
>> __ns = chrono::duration_cast<chrono::nanoseconds>(target - (now + __s));
>> - }
>> + }
>> #elif defined(_GLIBCXX_USE_WIN32_SLEEP)
>> + // Can't use limit<chrono::milliseconds>(__s) here because it would
>> + // multiply __s by 1000 which could overflow.
>
>My initial reaction is that it ought to be possible to make limit() cope
>with this but I haven't tried so I'm probably wrong. :)
I did consider that, then decided I didn't want to complicate the
limit function for the non-Windows systems.
>> + auto max_ms = chrono::milliseconds::max() / 1000;
>> + auto max_ms_in_s = chrono::duration_cast<chrono::seconds>(max_ms);
>> + if (__s > max_ms_in_s)
>> + __s = max_ms_in_s;
>> +
>> unsigned long ms = __ns.count() / 1000000;
>> if (__ns.count() > 0 && ms == 0)
>> ms = 1;
>> +
>> ::Sleep(chrono::milliseconds(__s).count() + ms);
>
>Shouldn't there be a loop around this so that we're guaranteed to wait for
>at least the requested duration even if __s is too large to be represented
>as milliseconds (at the cost of waking up every ~25 days)?
I think max_ms_in_s is 106751991 days, or 292277 years, so I felt we
didn't need to care about sleeping again if we'd woken up "early".
>> #endif
>> }
>> diff --git a/libstdc++-v3/testsuite/30_threads/this_thread/113327.cc b/libstdc++-v3/testsuite/30_threads/this_thread/113327.cc
>> new file mode 100644
>> index 000000000000..2daa2b0e46e2
>> --- /dev/null
>> +++ b/libstdc++-v3/testsuite/30_threads/this_thread/113327.cc
>> @@ -0,0 +1,29 @@
>> +// { dg-do run { target c++11 } }
>> +// { dg-additional-options "-pthread" { target pthread } }
>> +// { dg-require-gthreads "" }
>> +
>> +// PR libstdc++/113327
>> +// std::sleep_for(std::chrono::hours::max()) returns immediately
>> +
>> +#include <thread>
>> +#include <chrono>
>> +#include <cstdlib>
>> +#include <csignal>
>> +
>> +int main()
>> +{
>> + std::thread sleepy([] {
>> + // Rather than overflowing to a negative value, the timeout should be
>> + // truncated to seconds::max() and so sleep for 292 billion years.
>> + std::this_thread::sleep_for(std::chrono::minutes::max());
>> + // This should not happen:
>> + throw 1;
>> + });
>> + // Give the new thread a chance to start sleeping:
>> + std::this_thread::yield();
>> + std::this_thread::sleep_for(std::chrono::seconds(2));
>> + // If we get here without the other thread throwing an exception
>> + // then it should be sleeping peacefully, so the test passed.
>> + // pthread_kill(sleepy.native_handle(), SIGINT);
>> + std::_Exit(0);
>> +}
>
>Thanks again for looking at this.
>
>Mike.
>
More information about the Libstdc++
mailing list