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".
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
>
