Re: [PATCH] Complete __gnu_debug::basic_string

[Jonathan Wakely <jwakely@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]

On 25/03/21 18:45 +0100, François Dumont via Libstdc++ wrote:
>On 25/03/21 4:29 pm, Jonathan Wakely wrote:
>>On 25/03/21 14:48 +0000, Jonathan Wakely wrote:
>>>On 25/03/21 13:05 +0000, Jonathan Wakely wrote:
>>>>On 24/03/21 22:48 +0100, François Dumont wrote:
>>>>>I still need to find out why, when running test on 
>>>>>__gnu_debug::basic_string after the std::basic_string one, the 
>>>>>generate(sz) call always returns sz.
>>>>
>>>>The "random" generator will always return the same sequence of numbers
>>>>every time you run the test. It uses a default-constructed
>>>>std::mt19937 without a seed, so the sequence of random numbers is 100%
>>>>reproducable.
>>>
>>>This patch allows those random engines to be seeded, so that we can test
>>>with different random numbers.
>>>
>>>It's already found a bug:
>>>
>>>GLIBCXX_SEED_TEST_RNG=-941908610 make check RUNTESTFLAGS=conformance.exp=23_containers/forward_list/requirements/exception/generation_prohibited.cc
>>>
>>>Using random seed 3353058686
>>>FAIL: 23_containers/forward_list/requirements/exception/generation_prohibited.cc 
>>>execution test
>>>
>>>We need to investigate that.
>>
>>Oh, it's the same generate(sz) bug as you already found. But I've
>>found other bugs, e.g. with GLIBCXX_SEED_TEST_RNG=1908970375).
>>
>>I think we should also add a check for non-empty containers to those
>>test functions, and ensure we don't try to erase from empty
>>containers (see attached).
>>
>>
>Yes, I also realized this empty container potential issue.
>
>Please go ahead with any of your patches, I'll just adapt if you push first.

OK, thanks. I've pushed the attached patch. You should only need to
undo the generate(sz) changes in your patch.

>I will commit in a couple of hours.

Great, thanks.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 7946 bytes --]

commit c7fc73ee459045edabb99816658f14f32d23bf92
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Mar 25 13:51:08 2021

    libstdc++: Allow seeding random engines in testsuite
    
    The testsuite utilities that use random numbers use a
    default-constructed mersenne_twister_engine, meaning the values are
    reproducable. This adds support for seeding them, controlledby an
    environment variable. Defining GLIBCXX_SEED_TEST_RNG=val in the
    environment will cause the engines to be seeded with atoi(val) if that
    is non-zero, or with a value read from std::random_device otherwise.
    
    Running with different seeds revealed some bugs in the tests, where a
    randomly selected iterator was past-the-end (which can't be erased), or
    where the randomly populated container was empty, and then we tried to
    remove elements from it unconditionally.
    
    libstdc++-v3/ChangeLog:
    
            * testsuite/util/exception/safety.h (setup_base::generate):
            Support seeding random engine.
            (erase_point, erase_range): Adjust range of random numbers to
            ensure dereferenceable iterators are used where required.
            (generation_prohibited::run): Do not try to erase from empty
            containers.
            * testsuite/util/testsuite_containergen.h (test_containers):
            Support seeding random engine.

diff --git a/libstdc++-v3/testsuite/util/exception/safety.h b/libstdc++-v3/testsuite/util/exception/safety.h
index 6c91e740e0d..2e5d8acae00 100644
--- a/libstdc++-v3/testsuite/util/exception/safety.h
+++ b/libstdc++-v3/testsuite/util/exception/safety.h
@@ -22,6 +22,8 @@
 
 #include <testsuite_container_traits.h>
 #include <ext/throw_allocator.h>
+#include <cstdlib> // getenv, atoi
+#include <cstdio>  // printf, fflush
 
 // Container requirement testing.
 namespace __gnu_test
@@ -33,27 +35,34 @@ namespace __gnu_test
     typedef std::uniform_int_distribution<size_type> 	distribution_type;
     typedef std::mt19937 				engine_type;
 
+    static engine_type
+    get_engine()
+    {
+      engine_type engine;
+      if (const char* v = std::getenv("GLIBCXX_SEED_TEST_RNG"))
+	{
+	  // A single seed value is much smaller than the mt19937 state size,
+	  // but we're not trying to be cryptographically secure here.
+	  int s = std::atoi(v);
+	  if (s == 0)
+	    s = (int)std::random_device{}();
+	  std::printf("Using random seed %d\n", s);
+	  std::fflush(stdout);
+	  engine.seed((unsigned)s);
+	}
+      return engine;
+    }
+
     // Return randomly generated integer on range [0, __max_size].
     static size_type
     generate(size_type __max_size)
     {
-      // Make the generator static...
-      const engine_type engine;
-      const distribution_type distribution;
-      static auto generator = std::bind(distribution, engine,
-					std::placeholders::_1);
+      using param_type = typename distribution_type::param_type;
 
-      // ... but set the range for this particular invocation here.
-      const typename distribution_type::param_type p(0, __max_size);
-      size_type random = generator(p);
-      if (random < distribution.min() || random > distribution.max())
-	std::__throw_out_of_range_fmt(__N("setup_base::generate\n"
-					  "random number generated is: %zu "
-					  "out of range [%zu, %zu]\n"),
-				      (size_t)random,
-				      (size_t)distribution.min(),
-				      (size_t)distribution.max());
-      return random;
+      // Make the engine and distribution static...
+      static engine_type engine = get_engine();
+      static distribution_type distribution;
+      return distribution(engine, param_type{0, __max_size});
     }
 
     // Given an instantiating type, return a unique value.
@@ -309,10 +318,13 @@ namespace __gnu_test
 	      // computed with begin() and end().
 	      const size_type sz = std::distance(__container.begin(),
 						 __container.end());
+	      // Container::erase(pos) requires dereferenceable pos.
+	      if (sz == 0)
+		throw std::logic_error("erase_point: empty container");
 
 	      // NB: Lowest common denominator: use forward iterator operations.
 	      auto i = __container.begin();
-	      std::advance(i, generate(sz));
+	      std::advance(i, generate(sz - 1));
 
 	      // Makes it easier to think of this as __container.erase(i)
 	      (__container.*_F_erase_point)(i);
@@ -337,12 +349,15 @@ namespace __gnu_test
 	      // computed with begin() and end().
 	      const size_type sz = std::distance(__container.begin(),
 						 __container.end());
+	      // forward_list::erase_after(pos) requires dereferenceable pos.
+	      if (sz == 0)
+		throw std::logic_error("erase_point: empty container");
 
 	      // NB: Lowest common denominator: use forward iterator operations.
 	      auto i = __container.before_begin();
-	      std::advance(i, generate(sz));
+	      std::advance(i, generate(sz - 1));
 
-	      // Makes it easier to think of this as __container.erase(i)
+	      // Makes it easier to think of this as __container.erase_after(i)
 	      (__container.*_F_erase_point)(i);
 	    }
 	  catch(const __gnu_cxx::forced_error&)
@@ -405,14 +420,19 @@ namespace __gnu_test
 	    {
 	      const size_type sz = std::distance(__container.begin(),
 						 __container.end());
-	      size_type s1 = generate(sz);
-	      size_type s2 = generate(sz);
+	      // forward_list::erase_after(pos, last) requires a pos != last
+	      if (sz == 0)
+		return; // Caller doesn't check for this, not a logic error.
+
+	      size_type s1 = generate(sz - 1);
+	      size_type s2 = generate(sz - 1);
 	      auto i1 = __container.before_begin();
 	      auto i2 = __container.before_begin();
 	      std::advance(i1, std::min(s1, s2));
-	      std::advance(i2, std::max(s1, s2));
+	      std::advance(i2, std::max(s1, s2) + 1);
 
-	      // Makes it easier to think of this as __container.erase(i1, i2).
+	      // Makes it easier to think of this as
+	      // __container.erase_after(i1, i2).
 	      (__container.*_F_erase_range)(i1, i2);
 	    }
 	  catch(const __gnu_cxx::forced_error&)
@@ -1454,16 +1474,25 @@ namespace __gnu_test
 	  // constructor or assignment operator of value_type throws.
 	  if (!traits<container_type>::has_throwing_erase::value)
 	    {
-	      typename base_type::erase_point erasep;
-	      erasep(container);
+	      if (!container.empty())
+		{
+		  typename base_type::erase_point erasep;
+		  erasep(container);
+		}
 	      typename base_type::erase_range eraser;
 	      eraser(container);
 	    }
 
-	  typename base_type::pop_front popf;
-	  popf(container);
-	  typename base_type::pop_back popb;
-	  popb(container);
+	  if (!container.empty())
+	    {
+	      typename base_type::pop_front popf;
+	      popf(container);
+	    }
+	  if (!container.empty())
+	    {
+	      typename base_type::pop_back popb;
+	      popb(container);
+	    }
 
 	  typename base_type::iterator_ops iops;
 	  iops(container);
diff --git a/libstdc++-v3/testsuite/util/testsuite_containergen.h b/libstdc++-v3/testsuite/util/testsuite_containergen.h
index a2156733ec6..c468c7f4415 100644
--- a/libstdc++-v3/testsuite/util/testsuite_containergen.h
+++ b/libstdc++-v3/testsuite/util/testsuite_containergen.h
@@ -20,6 +20,8 @@
 
 #include <testsuite_container_traits.h>
 #include <random>
+#include <cstdlib> // getenv, atoi
+#include <cstdio>  // printf, fflush
 
 namespace __gnu_test
 {
@@ -63,6 +65,18 @@ namespace __gnu_test
     {
       std::mt19937_64 random_gen;
 
+      if (const char* v = std::getenv("GLIBCXX_SEED_TEST_RNG"))
+	{
+	  // A single seed value is much smaller than the mt19937 state size,
+	  // but we're not trying to be cryptographically secure here.
+	  int s = std::atoi(v);
+	  if (s == 0)
+	    s = (int)std::random_device{}();
+	  std::printf("Using random seed %d\n", s);
+	  std::fflush(stdout);
+	  random_gen.seed((unsigned)s);
+	}
+
 #ifdef SIMULATOR_TEST
       int loops = 10;
 #else