On Tue, Oct 14, 2025 at 11:57 AM Tomasz Kaminski <[email protected]> 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. > 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. > LGTM with above change. > } >> >> // 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 >>>> } >>>> >>>>
