On Tue, 17 Dec 2024 at 19:38, Jonathan Wakely <jwak...@redhat.com> wrote:
>
> On Wed, 31 Jul 2024 at 22:39, Jonathan Wakely <jwak...@redhat.com> wrote:
> >
> > On Wed, 24 Jul 2024 at 14:14, William Tsai <william.t...@sifive.com> wrote:
> > >
> > > The template `future.wait_until` will expand to
> > > `_M_load_and_test_until_impl` where it will call
> > > `_M_load_and_test_until*` with given time_point casted into second and
> > > nanosecond. The callee expects the caller to provide the values
> > > correctly from caller while the caller did not make check with those
> > > values. One possible error is that if `future.wait_until` is given with
> > > a negative time_point, the underlying system call will raise an error as
> > > the system call does not accept second < 0 and nanosecond < 1.
> >
> > Thanks for the patch, it looks correct. The futex syscall returns
> > EINVAL in this case, which we don't handle, so the caller loops and
> > keeps calling the syscall again, which fails again the same way.
> >
> > I think it would be good to mention EINVAL, e.g. "will raise an EINVAL
> > error" instead of just "will raise an error".
>
> I was finally getting around to merging this patch and realised the
> fix is actually much simpler. We do already check for negative
> timeouts, we just didn't handle values between -1s and 0s. We can fix
> it in the library, without changing the headers:
>
> --- a/libstdc++-v3/src/c++11/futex.cc
> +++ b/libstdc++-v3/src/c++11/futex.cc
> @@ -128,7 +128,7 @@ namespace
> if (!futex_clock_realtime_unavailable.load(std::memory_order_relaxed))
> {
> // futex sets errno=EINVAL for absolute timeouts before the epoch.
> - if (__s.count() < 0)
> + if (__s.count() < 0 || __ns.count() < 0) [[unlikely]]
> return false;
>
> syscall_timespec rt;
> @@ -204,7 +204,7 @@ namespace
> if (!futex_clock_monotonic_unavailable.load(std::memory_order_relaxed))
> {
> // futex sets errno=EINVAL for absolute timeouts before the epoch.
> - if (__s.count() < 0) [[unlikely]]
> + if (__s.count() < 0 || __ns.count() < 0) [[unlikely]]
> return false;
>
> syscall_timespec rt;
>
>
> A timeout of 10ms before the epoch gets broken down into 0s and -10ms
> and so the __s.count() < 0 check is false, but the futex syscall still
> returns EINVAL.
>
> So I'll fix this that way instead.
Here's what I'm testing.
commit a3f4ab3c7bfda80b5dd3ffe56b2accad15de3d71
Author: Jonathan Wakely <jwak...@redhat.com>
AuthorDate: Tue Dec 17 21:32:19 2024
Commit: Jonathan Wakely <r...@gcc.gnu.org>
CommitDate: Wed Dec 18 00:35:16 2024
libstdc++: Fix std::future::wait_until for subsecond negative times
[PR118093]
The current check for negative times (i.e. before the epoch) only checks
for a negative number of seconds. For a time 1ms before the epoch the
seconds part will be zero, but the futex syscall will still fail with an
EINVAL error. Extend the check to handle this case.
This change adds a redundant check in the headers too, so that we avoid
even calling into the library for negative times. Both checks can be
marked [[unlikely]].
libstdc++-v3/ChangeLog:
PR libstdc++/118093
* include/bits/atomic_futex.h (_M_load_and_test_until_impl):
Return false for times before the epoch.
* src/c++11/futex.cc (_M_futex_wait_until): Extend check for
negative times to check for subsecond times. Add unlikely
attribute.
(_M_futex_wait_until_steady): Likewise.
* testsuite/30_threads/future/members/118093.cc: New test.
diff --git a/libstdc++-v3/include/bits/atomic_futex.h
b/libstdc++-v3/include/bits/atomic_futex.h
index c7d90466418..fe13508462a 100644
--- a/libstdc++-v3/include/bits/atomic_futex.h
+++ b/libstdc++-v3/include/bits/atomic_futex.h
@@ -172,11 +172,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
bool __equal, memory_order __mo,
const chrono::time_point<std::chrono::system_clock, _Dur>& __atime)
{
- auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
- auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
- // XXX correct?
+ auto __d = __atime.time_since_epoch();
+ if (__d < __d.zero()) [[__unlikely__]]
+ return false;
+ auto __s = chrono::duration_cast<chrono::seconds>(__d);
+ auto __ns = chrono::duration_cast<chrono::nanoseconds>(__d - __s);
return _M_load_and_test_until(__assumed, __operand, __equal, __mo,
- true, __s.time_since_epoch(), __ns);
+ true, __s, __ns);
}
template<typename _Dur>
@@ -185,11 +187,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
bool __equal, memory_order __mo,
const chrono::time_point<std::chrono::steady_clock, _Dur>& __atime)
{
- auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
- auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
- // XXX correct?
+ auto __d = __atime.time_since_epoch();
+ if (__d < __d.zero()) [[__unlikely__]]
+ return false;
+ auto __s = chrono::duration_cast<chrono::seconds>(__d);
+ auto __ns = chrono::duration_cast<chrono::nanoseconds>(__d - __s);
return _M_load_and_test_until_steady(__assumed, __operand, __equal, __mo,
- true, __s.time_since_epoch(), __ns);
+ true, __s, __ns);
}
public:
diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
index ada2fdf3c41..6139fc354c6 100644
--- a/libstdc++-v3/src/c++11/futex.cc
+++ b/libstdc++-v3/src/c++11/futex.cc
@@ -128,7 +128,7 @@ namespace
if (!futex_clock_realtime_unavailable.load(std::memory_order_relaxed))
{
// futex sets errno=EINVAL for absolute timeouts before the epoch.
- if (__s.count() < 0)
+ if (__s.count() < 0 || __ns.count() < 0) [[unlikely]]
return false;
syscall_timespec rt;
@@ -204,7 +204,7 @@ namespace
if (!futex_clock_monotonic_unavailable.load(std::memory_order_relaxed))
{
// futex sets errno=EINVAL for absolute timeouts before the epoch.
- if (__s.count() < 0) [[unlikely]]
+ if (__s.count() < 0 || __ns.count() < 0) [[unlikely]]
return false;
syscall_timespec rt;
diff --git a/libstdc++-v3/testsuite/30_threads/future/members/118093.cc
b/libstdc++-v3/testsuite/30_threads/future/members/118093.cc
new file mode 100644
index 00000000000..2bb1e1c6dad
--- /dev/null
+++ b/libstdc++-v3/testsuite/30_threads/future/members/118093.cc
@@ -0,0 +1,26 @@
+// { dg-do run { target c++11 } }
+
+#include <chrono>
+#include <future>
+
+void
+test_sys()
+{
+ std::promise<void> p;
+ std::chrono::system_clock::time_point tp(std::chrono::milliseconds{-10});
+ (void) p.get_future().wait_until(tp);
+}
+
+void
+test_steady()
+{
+ std::promise<void> p;
+ std::chrono::steady_clock::time_point tp(std::chrono::milliseconds{-10});
+ (void) p.get_future().wait_until(tp);
+}
+
+int main()
+{
+ test_sys();
+ test_steady();
+}
