On Mon, Oct 13, 2025 at 11:00 PM Jonathan Wakely <[email protected]> wrote:
> > > 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); > Couldn't this still overflow? __ns may be really close to 1 second, and thus larger than the difference between chrono::duration_cast<chrono::seconds>(ms) - s. We could stay on safe side, and remove one second from years of ms, i.e. check: if (__s < chrono::duration_cast<chrono::seconds>(ms) - chrono::seconds(1)) This wii guarantee that adding __ns will never overflow. > } > > // 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 >>> } >>> >>>
