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. 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 . I've made a few minor comments on the patch itself below. > commit 27a12d66c96ebc107cc997faa63372392436de33 > Author: Jonathan Wakely <[email protected]> > AuthorDate: Wed Apr 30 10:58:51 2025 > Commit: Jonathan Wakely <[email protected]> > 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. > + { > + 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.) > + > + __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. > + { > + 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? > + __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. :) > + 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)? > #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.
