On Fri, 10 Oct 2025 at 12:39, Mike Crowe <[email protected]> wrote:
> 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.
>
Gah, that's what I missed then, that DWORD is 32-bits. I think I assumed it
was a "double word", which is true, but it's Intel 16-bit words. Sigh.
>
> 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!
>
>
It's used in exactly one place, and it's Windows, so I find it hard to care
enough to factor it out.
