Thanks for the patches, Mike, I've finally managed to do the first set of reviews ...
On Sun, 14 Sept 2025 at 21:30, Mike Crowe wrote: > > Passing a timeout from before the epoch to __platform_wait_until() > currently results in an exception being thrown because futex(2) doesn't > support negative timeouts. Let's just immediately return indicating a > timeout has happened. > > Add test cases to prove that this bug is fixed for std::binary_semaphore > (which is just an alias for std::counting_semaphore<1>). The tests > exercise cases that aren't problematic with the current code since > system_clock is converted to steady_clock before calling > __platform_wait_until() is called but they will protect against changes > in the implementation reintroducing this bug. > > PR libstdc++/116586 > > libstdc++-v3/ChangeLog: > * src/c++20/atomic.cc (__platform_wait_until): Return false on > timeout before epoch. > * testsuite/30_threads/semaphore/try_acquire_for.cc: Add tests. > * testsuite/30_threads/semaphore/try_acquire_until.cc: Add tests. > > Signed-off-by: Mike Crowe <[email protected]> > --- > libstdc++-v3/src/c++20/atomic.cc | 4 ++++ > .../30_threads/semaphore/try_acquire_for.cc | 20 +++++++++++++++++ > .../30_threads/semaphore/try_acquire_until.cc | 22 +++++++++++++++++++ > 3 files changed, 46 insertions(+) > > diff --git a/libstdc++-v3/src/c++20/atomic.cc > b/libstdc++-v3/src/c++20/atomic.cc > index 4120e1a0817..1e987213d80 100644 > --- a/libstdc++-v3/src/c++20/atomic.cc > +++ b/libstdc++-v3/src/c++20/atomic.cc > @@ -359,6 +359,10 @@ __platform_wait_until(const __platform_wait_t* __addr, > static_cast<long>(__ns.count()) > }; > > + // SYS_futex returns EINVAL if timeout is negative, act as if we timed out > immediately > + if (__rt.tv_sec < 0 || __rt.tv_nsec < 0) We could put [[unlikely]] here. I'm not sure how much difference it will make, but I don't think it can hurt. > + return false; > + > if (syscall (SYS_futex, __addr, > static_cast<int>(__futex_wait_flags::__wait_bitset_private), > __old, &__rt, nullptr, > diff --git a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc > b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc > index 39681c7ee56..fe0d704baf8 100644 > --- a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc > +++ b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc > @@ -90,9 +90,29 @@ test03() > s.try_acquire_for(timeout); > } > > +// Prove semaphore doesn't suffer from PR116586 > +template <typename Clock> > +void > +test_relative(std::chrono::nanoseconds offset) > +{ > + std::binary_semaphore sem(1); > + VERIFY(sem.try_acquire_for(offset)); > + VERIFY(!sem.try_acquire_for(offset)); > +} > + > int main() > { > test01(); > test02(); > test03(); > + for (const std::chrono::nanoseconds offset : { > + std::chrono::nanoseconds{0}, > + > std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::milliseconds{-10}), With a 'using namespace std::chrono;' above, this could be just nanoseconds{-10ms} > + > std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::seconds{-10}) And this could be just nanoseconds{-10s} Which seems more readable to me. I can push this if you're happy with those changes. > + }) { > + test_relative<std::chrono::system_clock>(offset); > + test_relative<std::chrono::system_clock>(offset - > std::chrono::system_clock::now().time_since_epoch()); > + test_relative<std::chrono::steady_clock>(offset); > + test_relative<std::chrono::steady_clock>(offset - > std::chrono::steady_clock::now().time_since_epoch()); > + } > } > diff --git a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_until.cc > b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_until.cc > index de0068d670a..a00f124b147 100644 > --- a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_until.cc > +++ b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_until.cc > @@ -87,8 +87,30 @@ void test02() > b.wait(1); > } > > +// Prove semaphore doesn't suffer from PR116586 > +template <typename Clock> > +void > +test_absolute(std::chrono::nanoseconds offset) > +{ > + std::binary_semaphore sem(1); > + std::chrono::time_point<Clock> tp(offset); > + VERIFY(sem.try_acquire_until(tp)); > + VERIFY(!sem.try_acquire_until(tp)); > +} > + > int main() > { > test01(); > test02(); > + for (const std::chrono::nanoseconds offset : { > + // tv_sec == 0, tv_nsec == 0 > + std::chrono::nanoseconds{0}, > + // tv_sec == 0, tv_nsec < 0 > + > std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::milliseconds{-10}), > + // tv_sec < 0 > + > std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::seconds{-10}) > + }) { > + test_absolute<std::chrono::system_clock>(offset); > + test_absolute<std::chrono::steady_clock>(offset); > + } > } > -- > 2.39.5 >
