[committed] libstdc++: Optimise std::future::wait_for and fix futex polling

[Mike Crowe]

On Saturday 14 November 2020 at 00:17:22 +0000, Jonathan Wakely wrote:
> On 13/11/20 22:45 +0000, Jonathan Wakely wrote:
> > On 13/11/20 21:12 +0000, Jonathan Wakely wrote:
> > > On 13/11/20 20:29 +0000, Mike Crowe via Libstdc++ wrote:
> > > > On Friday 13 November 2020 at 17:25:22 +0000, Jonathan Wakely wrote:
> > > > > +  // Return the relative duration from (now_s + now_ns) to (abs_s + abs_ns)
> > > > > +  // as a timespec.
> > > > > +  struct timespec
> > > > > +  relative_timespec(chrono::seconds abs_s, chrono::nanoseconds abs_ns,
> > > > > +		    time_t now_s, long now_ns)
> > > > > +  {
> > > > > +    struct timespec rt;
> > > > > +
> > > > > +    // Did we already time out?
> > > > > +    if (now_s > abs_s.count())
> > > > > +      {
> > > > > +	rt.tv_sec = -1;
> > > > > +	return rt;
> > > > > +      }
> > > > > +
> > > > > +    auto rel_s = abs_s.count() - now_s;
> > > > > +
> > > > > +    // Avoid overflows
> > > > > +    if (rel_s > __gnu_cxx::__int_traits<time_t>::__max)
> > > > > +      rel_s = __gnu_cxx::__int_traits<time_t>::__max;
> > > > > +    else if (rel_s < __gnu_cxx::__int_traits<time_t>::__min)
> > > > > +      rel_s = __gnu_cxx::__int_traits<time_t>::__min;
> > > > 
> > > > I may be missing something, but if the line above executes...
> > > > 
> > > > > +
> > > > > +    // Convert the absolute timeout value to a relative timeout
> > > > > +    rt.tv_sec = rel_s;
> > > > > +    rt.tv_nsec = abs_ns.count() - now_ns;
> > > > > +    if (rt.tv_nsec < 0)
> > > > > +      {
> > > > > +	rt.tv_nsec += 1000000000;
> > > > > +	--rt.tv_sec;
> > > > 
> > > > ...and so does this line above, then I think that we'll end up
> > > > underflowing. (Presumably rt.tv_sec will wrap round to being some time in
> > > > 2038 on most 32-bit targets.)
> > > 
> > > Ugh.
> > > 
> > > > I'm currently trying to persuade myself that this can actually happen and
> > > > if so work out how to come up with a test case for it.
> > > 
> > > Maybe something like:
> > > 
> > > auto d = chrono::floor<chrono::seconds>(system_clock::now().time_since_epoch() - seconds(INT_MAX + 2LL));
> > > fut.wait_until(system_clock::time_point(d));
> > > 
> > > This will create a sys_time with a value that is slightly more than
> > > INT_MAX seconds before the current time, with a zero nanoseconds
> > 
> > Ah, but such a time will never reach the overflow because the first
> > thing that the new relative_timespec function does is:
> > 
> >     if (now_s > abs_s.count())
> >       {
> >         rt.tv_sec = -1;
> >         return rt;
> >       }
> > 
> > So in fact we can never have a negative rel_s anyway.
> 
> Here's what I've pushed now, after testing on x86_64-linux.
> 
> 

> commit b8d36dcc917e8a06d8c20b9f5ecc920ed2b9e947
> Author: Jonathan Wakely <jwakely@redhat.com>
> Date:   Fri Nov 13 20:57:15 2020
> 
>     libstdc++: Remove redundant overflow check for futex timeout [PR 93456]
>     
>     The relative_timespec function already checks for the case where the
>     specified timeout is in the past, so the difference can never be
>     negative. That means we dn't need to check if it's more negative than
>     the minimum time_t value.
>     
>     libstdc++-v3/ChangeLog:
>     
>             PR libstdc++/93456
>             * src/c++11/futex.cc (relative_timespec): Remove redundant check
>             negative values.
>             * testsuite/30_threads/future/members/wait_until_overflow.cc: Moved to...
>             * testsuite/30_threads/future/members/93456.cc: ...here.
> 
> diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
> index 15959cebee57..c008798318c9 100644
> --- a/libstdc++-v3/src/c++11/futex.cc
> +++ b/libstdc++-v3/src/c++11/futex.cc
> @@ -73,21 +73,23 @@ namespace
>  	return rt;
>        }
>  
> -    auto rel_s = abs_s.count() - now_s;
> +    const auto rel_s = abs_s.count() - now_s;
>  
> -    // Avoid overflows
> +    // Convert the absolute timeout to a relative timeout, without overflow.
>      if (rel_s > __int_traits<time_t>::__max) [[unlikely]]
> -      rel_s = __int_traits<time_t>::__max;
> -    else if (rel_s < __int_traits<time_t>::__min) [[unlikely]]
> -      rel_s = __int_traits<time_t>::__min;
> -
> -    // Convert the absolute timeout value to a relative timeout
> -    rt.tv_sec = rel_s;
> -    rt.tv_nsec = abs_ns.count() - now_ns;
> -    if (rt.tv_nsec < 0)
>        {
> -	rt.tv_nsec += 1000000000;
> -	--rt.tv_sec;
> +	rt.tv_sec = __int_traits<time_t>::__max;
> +	rt.tv_nsec = 999999999;
> +      }
> +    else
> +      {
> +	rt.tv_sec = rel_s;
> +	rt.tv_nsec = abs_ns.count() - now_ns;
> +	if (rt.tv_nsec < 0)
> +	  {
> +	    rt.tv_nsec += 1000000000;
> +	    --rt.tv_sec;
> +	  }
>        }
>  
>      return rt;

LGTM. Thanks.

Mike.