[v2,libstdc++,testsuite] avoid async.cc loss of precision [PR91486]

When we get to test_pr91486_wait_until(), we're about 10s past the
float_steady_clock epoch.  This is enough for the 1s delta for the
timeout to come out slightly lower when the futex-less wait_until
converts the deadline from float_steady_clock to __clock_t.  So we may
wake up a little too early, and end up looping one extra time to sleep
for e.g. another 954ns until we hit the deadline.

Each iteration calls float_steady_clock::now(), bumping the call_count
that we VERIFY() at the end of the subtest.  Since we expect at most 3
calls, and we're going to have at the very least 3 on futex-less
targets (one in the test proper, one before wait_until_impl to compute
the deadline, and one after wait_until_impl to check whether the
deadline was hit), any such imprecision that causes an extra iteration
will reach 5 and cause the test to fail.

Initializing the epoch in the beginning of the test makes such
spurious fails due to loss of precision far less likely.  I don't
suppose allowing for an extra couple of calls would be desirable.

While at that, I'm annotating unused status variables as such.

Regstrapped on x86_64-linux-gnu, also tested on arm-vx7r2 with gcc-13.
Ok to install?

(Do we really want to use floats, that even with this tweak have
borderline precision for sub-µs vs 1s deltas?  Do we want to make sure
the wait time computation ensures we'll get past the deadline when the
time is converted back to the given clock?)


for  libstdc++-v3/ChangeLog

	PR libstdc++/91486
	* testsuite/30_threads/async/async.cc
	(test_pr91486_wait_for): Mark status as unused.
	(test_pr91486_wait_until): Likewise.  Initialize epoch later.
---
 libstdc++-v3/testsuite/30_threads/async/async.cc |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

On Aug  1, 2024, Alexandre Oliva <oliva@adacore.com> wrote:

> Each iteration calls float_steady_clock::now() [...] an extra iteration
> will reach 5 and cause the test to fail.

> (Do we really want to use floats, that even with this tweak have
> borderline precision for sub-µs vs 1s deltas?  Do we want to make sure
> the wait time computation ensures we'll get past the deadline when the
> time is converted back to the given clock?)

Ping?
https://gcc.gnu.org/pipermail/gcc-patches/2024-August/658977.html

> for  libstdc++-v3/ChangeLog

> 	PR libstdc++/91486
> 	* testsuite/30_threads/async/async.cc
> 	(test_pr91486_wait_for): Mark status as unused.
> 	(test_pr91486_wait_until): Likewise.  Initialize epoch later.

On Aug  7, 2024, Alexandre Oliva <oliva@adacore.com> wrote:

> On Aug  1, 2024, Alexandre Oliva <oliva@adacore.com> wrote:
>> Each iteration calls float_steady_clock::now() [...] an extra iteration
>> will reach 5 and cause the test to fail.

>> (Do we really want to use floats, that even with this tweak have
>> borderline precision for sub-µs vs 1s deltas?  Do we want to make sure
>> the wait time computation ensures we'll get past the deadline when the
>> time is converted back to the given clock?)

> Ping?
> https://gcc.gnu.org/pipermail/gcc-patches/2024-August/658977.html

Ping?

>> for  libstdc++-v3/ChangeLog

>> PR libstdc++/91486
>> * testsuite/30_threads/async/async.cc
>> (test_pr91486_wait_for): Mark status as unused.
>> (test_pr91486_wait_until): Likewise.  Initialize epoch later.

On Thu, 1 Aug 2024 at 11:44, Alexandre Oliva <oliva@adacore.com> wrote:
>
> When we get to test_pr91486_wait_until(), we're about 10s past the
> float_steady_clock epoch.  This is enough for the 1s delta for the
> timeout to come out slightly lower when the futex-less wait_until
> converts the deadline from float_steady_clock to __clock_t.  So we may
> wake up a little too early, and end up looping one extra time to sleep
> for e.g. another 954ns until we hit the deadline.
>
> Each iteration calls float_steady_clock::now(), bumping the call_count
> that we VERIFY() at the end of the subtest.  Since we expect at most 3
> calls, and we're going to have at the very least 3 on futex-less
> targets (one in the test proper, one before wait_until_impl to compute
> the deadline, and one after wait_until_impl to check whether the
> deadline was hit), any such imprecision that causes an extra iteration
> will reach 5 and cause the test to fail.
>
> Initializing the epoch in the beginning of the test makes such
> spurious fails due to loss of precision far less likely.  I don't
> suppose allowing for an extra couple of calls would be desirable.
>
> While at that, I'm annotating unused status variables as such.
>
> Regstrapped on x86_64-linux-gnu, also tested on arm-vx7r2 with gcc-13.
> Ok to install?

OK, thanks.


>
> (Do we really want to use floats, that even with this tweak have
> borderline precision for sub-µs vs 1s deltas?  Do we want to make sure
> the wait time computation ensures we'll get past the deadline when the
> time is converted back to the given clock?)

I think the bug that test is verifying is only seen with float
precision. If we used double, the bug wouldn't be seen anyway, so we
might as well drop the test. I hope your epoch tweak will make it
reliable enough. Let's see.

>
>
> for  libstdc++-v3/ChangeLog
>
>         PR libstdc++/91486
>         * testsuite/30_threads/async/async.cc
>         (test_pr91486_wait_for): Mark status as unused.
>         (test_pr91486_wait_until): Likewise.  Initialize epoch later.
> ---
>  libstdc++-v3/testsuite/30_threads/async/async.cc |   19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/libstdc++-v3/testsuite/30_threads/async/async.cc b/libstdc++-v3/testsuite/30_threads/async/async.cc
> index 3b157ed9c5680..2474d318d7b1b 100644
> --- a/libstdc++-v3/testsuite/30_threads/async/async.cc
> +++ b/libstdc++-v3/testsuite/30_threads/async/async.cc
> @@ -173,7 +173,7 @@ void test_pr91486_wait_for()
>
>    std::chrono::duration<float> const wait_time = std::chrono::seconds(1);
>    auto const start_steady = chrono::steady_clock::now();
> -  auto status = f1.wait_for(wait_time);
> +  auto status __attribute__ ((__unused__)) = f1.wait_for(wait_time);
>    auto const elapsed_steady = chrono::steady_clock::now() - start_steady;
>
>    VERIFY( elapsed_steady >= std::chrono::seconds(1) );
> @@ -209,7 +209,7 @@ struct float_steady_clock
>    }
>  };
>
> -chrono::steady_clock::time_point float_steady_clock::epoch = chrono::steady_clock::now();
> +chrono::steady_clock::time_point float_steady_clock::epoch;
>  int float_steady_clock::call_count = 0;
>
>  void test_pr91486_wait_until()
> @@ -218,6 +218,19 @@ void test_pr91486_wait_until()
>        std::this_thread::sleep_for(std::chrono::seconds(1));
>      });
>
> +  // When we don't _GLIBCXX_HAVE_LINUX_FUTEX, we use
> +  // condition_variables, whose wait_until converts times using
> +  // deltas, and if too much time has elapsed since we set the epoch
> +  // during program initialization, say if the other tests took over
> +  // 8s and we're unlucky with the numbers, we may lose enough
> +  // precision from the 1s delta that we don't sleep until the
> +  // deadline, and then we may loop more times than expected.  Each
> +  // iteration will recompute the wait time from deadline -
> +  // float_steady_clock::now(), and each such computation will bump
> +  // float_steady_clock::call_count, so the call_count check below
> +  // will fail spuriously.  Setting the epoch just before running this
> +  // test makes this failure mode far less likely.
> +  float_steady_clock::epoch = chrono::steady_clock::now();
>    float_steady_clock::time_point const now = float_steady_clock::now();
>
>    std::chrono::duration<float> const wait_time = std::chrono::seconds(1);
> @@ -225,7 +238,7 @@ void test_pr91486_wait_until()
>    VERIFY( expire > now );
>
>    auto const start_steady = chrono::steady_clock::now();
> -  auto status = f1.wait_until(expire);
> +  auto status __attribute__ ((__unused__)) = f1.wait_until(expire);
>    auto const elapsed_steady = chrono::steady_clock::now() - start_steady;
>
>    // This checks that we didn't come back too soon
>
>
> --
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
>