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().
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
}
