diff mbox series

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

Message ID orplqviyis.fsf@lxoliva.fsfla.org
State New
Headers show
Series [libstdc++,testsuite] avoid async.cc loss of precision [PR91486] | expand

Commit Message

Alexandre Oliva July 29, 2024, 11:11 p.m. UTC 
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.

Regstrapping on x86_64-linux-gnu, also testing 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?)


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(-)

Comments

Alexandre Oliva July 30, 2024, 12:30 a.m. UTC | #1 
On Jul 29, 2024, Alexandre Oliva <oliva@adacore.com> wrote:

> -  auto status = f1.wait_for(wait_time);
> +  auto status __attribute__ (__unused__) = f1.wait_for(wait_time);

Sorry, it looks like I posted the patch before refreshing it.  Make it:

  +  auto status __attribute__ ((__unused__)) = f1.wait_for(wait_time);

(both occurrences were affected)
diff mbox series

Patch

diff --git a/libstdc++-v3/testsuite/30_threads/async/async.cc b/libstdc++-v3/testsuite/30_threads/async/async.cc
index 3b157ed9c5680..c68bed1689a94 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