On Thu, Oct 9, 2025 at 4:26 PM Jonathan Wakely <[email protected]> wrote:
> 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. > > To fix this everywhere that uses timeouts, new functions are introduced > for converting from a chrono::duration or chrono::time_point to a > timespec (or __gthread_time_t which is just a timespec on Linux). These > functions provide one central place where we can avoid overflow and also > handle negative timeouts (as these produce errors when passed to OS > functions that do not accept absolute times before the epoch). All > negative durations are converted to zero, and negative time_points are > converted to the epoch. > libstdc++-v3/ChangeLog: > > PR libstdc++/113327 > PR libstdc++/116586 > PR libstdc++/119258 > PR libstdc++/58931 > * include/bits/chrono.h (__to_timeout_timespec): New overloaded > function templates for converting chrono types to timespec. > (__to_timeout_gthread_time_t): New function template for > converting time_point to __gthread_time_t. > * include/bits/this_thread_sleep.h (sleep_for): Use > __to_timeout_timespec. > (__sleep_for): Remove namespace-scope declaration. > * 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. > * src/c++20/atomic.cc: Use __to_timeout_timespec and > __to_timeout_gthread_time_t for timeouts. > * testsuite/30_threads/this_thread/113327.cc: New test. > > Reviewed-by: Mike Crowe <[email protected]> > --- > > v2: followed Mike's suggestion to rename the functions from > __to_timespec and __to_gthread_time_t to __to_timeout_timespec and > __to_timeout_gthread_time_t. > Some comments below, I think I found an issue in __sleep_for implementation. > > Tested x86_64-linux. > > libstdc++-v3/include/bits/chrono.h | 95 +++++++++++++++++++ > libstdc++-v3/include/bits/this_thread_sleep.h | 20 ++-- > libstdc++-v3/include/std/condition_variable | 20 +--- > libstdc++-v3/include/std/mutex | 18 +--- > libstdc++-v3/include/std/shared_mutex | 39 +------- > libstdc++-v3/src/c++11/thread.cc | 32 ++++++- > libstdc++-v3/src/c++20/atomic.cc | 18 +--- > .../30_threads/this_thread/113327.cc | 29 ++++++ > 8 files changed, 172 insertions(+), 99 deletions(-) > create mode 100644 libstdc++-v3/testsuite/30_threads/this_thread/113327.cc > > diff --git a/libstdc++-v3/include/bits/chrono.h > b/libstdc++-v3/include/bits/chrono.h > index 8de8e756c714..c20b68140192 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 > @@ -1515,6 +1518,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_timeout_timespec(const duration<_Rep, _Period>& __d) > + { > + 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) I think simple || here would be fine, they treate_as_floting_point does not seem to be that expensive to instantiate, to outweight instantiation __or. > + { > + // 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_timeout_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 seemed strange at first, but I guess this is the result of using floating point arithmetic. > + > + __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_timeout_timespec(const time_point<_Clock, _Dur>& __t) > + { > + return chrono::__to_timeout_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_timeout_gthread_time_t(const time_point<_Clock, _Dur>& __t) > + { > + auto __ts = chrono::__to_timeout_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) > Could you add some comment here, tha __gthread_time_it is count of second in scalar, in contrast to being milliseconds for example. > + 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..01f25dda2af0 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_timeout_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); > + __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..dcf0b9235cf3 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_timeout_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_timeout_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 631c38069d4a..d4fc4c646488 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_timeout_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_timeout_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..a267ad7c4f66 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_timeout_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_timeout_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_timeout_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_timeout_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. > + auto max_ms = chrono::milliseconds::max() / 1000; > This does not seem right to me. millseconds::max() returns milliseconds (duration object) and not rep. Then we divide it by 1000 to get the number of seconds, but we still have milliseconds. + auto max_ms_in_s = chrono::duration_cast<chrono::seconds>(max_ms); So the conversions to seconds, will divide it by 1000 again, should this be: auto max_ms_in_s = chrono::duration_cast<chrono::seconds>(std::chrono::milliseconds::max); Or at least chrono::duration_cast<chrono::seconds>(max_ms.count()) I prefer the former. > + 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); > #endif > } > diff --git a/libstdc++-v3/src/c++20/atomic.cc > b/libstdc++-v3/src/c++20/atomic.cc > index 4120e1a0817f..7978809cd223 100644 > --- a/libstdc++-v3/src/c++20/atomic.cc > +++ b/libstdc++-v3/src/c++20/atomic.cc > @@ -350,14 +350,7 @@ __platform_wait_until(const __platform_wait_t* __addr, > __platform_wait_t __old, > const __wait_clock_t::time_point& __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_timeout_timespec(__atime); > > if (syscall (SYS_futex, __addr, > static_cast<int>(__futex_wait_flags::__wait_bitset_private), > @@ -378,14 +371,7 @@ bool > __cond_wait_until(__condvar& __cv, mutex& __mx, > const __wait_clock_t::time_point& __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_timeout_gthread_time_t(__atime); > > #ifdef _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT > if constexpr (is_same_v<chrono::steady_clock, __wait_clock_t>) > 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); > +} > -- > 2.51.0 > >
