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. > > It would also be good to add a test to the testsuite for this. > > Do you have git write access, or do you need me to push it once it's approved? > > > > > > Following is a simple testcase: > > ``` > > #include <chrono> > > #include <future> > > #include <iostream> > > > > using namespace std; > > > > int main() { > > promise<void> p; > > future<void> f = p.get_future(); > > chrono::steady_clock::time_point tp(chrono::milliseconds{-10}); > > future_status status = f.wait_until(tp); > > if (status == future_status::timeout) { > > cout << "Timed out" << endl; > > } > > return 0; > > } > > ``` > > > > libstdc++-v3/ChangeLog: > > > > * include/bits/atomic_futex.h: Check if __s and __ns is valid before > > calling before calling _M_load_and_test_until* > > > > Signed-off-by: William Tsai <william.t...@sifive.com> > > --- > > libstdc++-v3/include/bits/atomic_futex.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/libstdc++-v3/include/bits/atomic_futex.h > > b/libstdc++-v3/include/bits/atomic_futex.h > > index dd654174873..4c31946a97f 100644 > > --- a/libstdc++-v3/include/bits/atomic_futex.h > > +++ b/libstdc++-v3/include/bits/atomic_futex.h > > @@ -173,6 +173,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > auto __s = chrono::time_point_cast<chrono::seconds>(__atime); > > auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - > > __s); > > // XXX correct? > > + if ((__s.time_since_epoch().count() < 0) || (__ns.count() < 0)) > > + return false; > > return _M_load_and_test_until(__assumed, __operand, __equal, __mo, > > true, __s.time_since_epoch(), __ns); > > } > > @@ -186,6 +188,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > auto __s = chrono::time_point_cast<chrono::seconds>(__atime); > > auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - > > __s); > > // XXX correct? > > + if ((__s.time_since_epoch().count() < 0) || (__ns.count() < 0)) > > + return false; > > return _M_load_and_test_until_steady(__assumed, __operand, __equal, > > __mo, > > true, __s.time_since_epoch(), __ns); > > } > > -- > > 2.37.1 > >