On Wednesday 08 October 2025 at 21:49:16 +0100, Jonathan Wakely wrote:
> On Wed, 08 Oct 2025 at 21:21 +0100, Mike Crowe wrote:
> > On Wednesday 08 October 2025 at 12:27:06 +0100, Jonathan Wakely wrote:
> > > 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;
> > > + auto max_ms_in_s = chrono::duration_cast<chrono::seconds>(max_ms);
> > > + 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);
> >
> > Shouldn't there be a loop around this so that we're guaranteed to wait for
> > at least the requested duration even if __s is too large to be represented
> > as milliseconds (at the cost of waking up every ~25 days)?
>
> I think max_ms_in_s is 106751991 days, or 292277 years, so I felt we
> didn't need to care about sleeping again if we'd woken up "early".
It appears that Windows Sleep() takes a 32-bit unsigned DWORD, so I think
no matter what you pass you can't make it wait for more than just under 50
days. (The ~25 days in my original calculation was wrong because I hadn't
taken into account that the parameter is unsigned.)
I believe that the max_ms_in_s check is necessary to avoid overflow in the
milliseconds int64_t, but it's not sufficient to avoid truncation when that
int64_t is converted to a DWORD.
Some could argue that a wait of over fifty days is unreasonable, but I
think that's definitely more of a possibility in real code than a wait of
292277 years.
Nevertheless, I wonder whether this is now sufficiently complex that we
should factor it out to some sort of __duration_as_uint_ms() function or
similar so that the implementation can be tested (ideally not just on
Windows) with all the nasty corner cases?
Either that or invent a Sleep wrapper that takes the full resolution
timeout so we don't have to care about this part of the problem. Something
like:
void __win32_sleep(uint64_t ms)
{
const auto max_sleep = numeric_limits<DWORD>::max();
while (ms > max_sleep) {
::Sleep(max_sleep)
ms -= max_sleep;
}
::Sleep(ms);
}
or, an equivalent that uses steady_clock::now() and a timeout to account
for mounting errors from lazy wakeup but I doubt that callers care that
much about split-second accuracy when waiting for more than fifty days!
Thanks.
Mike.
