On Fri, 10 Oct 2025 at 09:19, Tomasz Kaminski <[email protected]> wrote:
> > > 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. > Yes, it might not be necessary, but I'd rather just have this conditional branch (which is discarded for non-floating-point reps) than get EINVAL from some pthreads API for using ts.tv_nsec == 1'000'000'000. > + >> + __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. > Done. > + 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/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. > Yes, this code is nonsense. > + 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. > OK, how's this: #elif defined(_GLIBCXX_USE_WIN32_SLEEP) // Can't use limit<chrono::milliseconds>(__s) here because it would // multiply __s by 1000 which could overflow. // Limit to milliseconds::max() and truncate to seconds: chrono::milliseconds ms = chrono::milliseconds::max(); if (__s < chrono::duration_cast<chrono::seconds>(ms)) { ms = __s; ms += chrono::__detail::ceil<chrono::milliseconds>(__ns); } // Use Sleep(DWORD millis) where DWORD is uint32_t. constexpr chrono::milliseconds max_sleep(INFINITE - 1u); while (ms > max_sleep) { ::Sleep(max_sleep.count()); ms -= max_sleep; } ::Sleep(ms.count()); #endif This fixes the duplicated division by 1000, and adds a loop to sleep longer than INFINITE milliseconds (which is approx. 49 days). We could use duration<uint64_t, milli> instead of milliseconds, which would double the upper limit by using an unsigned type, but milliseconds::max() is already huge. + 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 >> } >> >>
