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 <jwak...@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.