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;
