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

[Jonathan Wakely]

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
component. The difference between the gettimeofday result and this
time will be slightly more negative than INT_MIN and so will overflow
a 32-bit time_t, causing the code to use __int_traits<time_t>::__min.
As long as the gettimeofday call doesn't happen to also have a zero
nanoseconds component, the difference of the nanoseconds values will
be negative, and we will decrement the time_t value.

>I don't believe that this part changed in your later patch.

Right. Thanks for catching this.

The attached patch should fix it. There's no point clamping very
negative values to the minimum time_t value, since any relative
timeout less than zero has already passed. So we can just use -1
there (and not bother with the tv_nsec field at all):

     else if (rel_s <= 0) [[unlikely]]
       {
	rt.tv_sec = -1;
       }

But please check my working.



>Oh how I wish all these functions didn't expect already cracked seconds and
>nanoseconds. :(

Indeed.

At some point (probably not for GCC 11) I'm going to add a proper
wrapper class for all these futex calls, and make it robust, and then
replace the various hand-crafted uses of syscall(SYS_futex, ...) with
that type so we only need to fix all this in one place. And it will
just take a duration or time_point.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 1575 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20201113/04b72f98/attachment.bin>