On Tue, Oct 14, 2025 at 12:44 PM Jonathan Wakely <[email protected]> wrote:
> On Tue, 14 Oct 2025 at 11:57 +0200, Tomasz Kaminski wrote: > >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. > > I don't think it can. > > milliseconds::max() is 9223372036854775807ms > > duration_cast<seconds>(milliseconds::max()) is 9223372036854775s > > We know that the caller ensures that __ns <= 999'999'999ns, so > chrono::__detail::ceil<chrono::milliseconds>(__ns) <= 1000ms > > The check is that __s is less than that (not less than or equal), so > we know that __s is at most 9223372036854774s. We convert that to > milliseconds and add 1000ms. That gives 9223372036854775s, which is > the same value as duration_cast<seconds>(milliseconds::max()) and is > 807ms less than milliseconds::max(). > Ah, we check lower than chrono::duration_cast<chrono::seconds>(ms), so we already have extra seconds to max. It either s < ms_max or s <= ms_max - 1, and I somehow hold on to it latter. > > > > >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 > >>>> } > >>>> > >>>> > >
