On Wed, 18 Dec 2024 at 00:35, Jonathan Wakely <jwak...@redhat.com> wrote:
>
> 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.
Pushed to trunk now.
