From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 4B3813858001 for ; Thu, 25 Mar 2021 18:23:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4B3813858001 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-495-RHC2sKaLOVumSTIM0g4Gyw-1; Thu, 25 Mar 2021 14:23:23 -0400 X-MC-Unique: RHC2sKaLOVumSTIM0g4Gyw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1F0D385242A; Thu, 25 Mar 2021 18:22:43 +0000 (UTC) Received: from localhost (unknown [10.33.36.164]) by smtp.corp.redhat.com (Postfix) with ESMTP id A90D91037E81; Thu, 25 Mar 2021 18:22:42 +0000 (UTC) Date: Thu, 25 Mar 2021 18:22:41 +0000 From: Jonathan Wakely To: =?iso-8859-1?Q?Fran=E7ois?= Dumont Cc: "libstdc++@gcc.gnu.org" , gcc-patches Subject: Re: [PATCH] Complete __gnu_debug::basic_string Message-ID: <20210325182241.GT3008@redhat.com> References: <53722923-e31e-4311-27cb-c66d60b52b19@gmail.com> <20210319194115.GU3008@redhat.com> <648191d4-da3b-9e94-67a4-8121a2da489f@gmail.com> <20210323154236.GX3008@redhat.com> <20210325130507.GQ3008@redhat.com> <20210325144808.GR3008@redhat.com> <20210325152952.GS3008@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="im83/wVv0jiGQj4J" Content-Disposition: inline X-Spam-Status: No, score=-14.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Mar 2021 18:23:47 -0000 --im83/wVv0jiGQj4J Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit 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. --im83/wVv0jiGQj4J Content-Type: text/x-patch; charset=us-ascii Content-Disposition: attachment; filename="patch.txt" commit c7fc73ee459045edabb99816658f14f32d23bf92 Author: Jonathan Wakely 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 #include +#include // getenv, atoi +#include // printf, fflush // Container requirement testing. namespace __gnu_test @@ -33,27 +35,34 @@ namespace __gnu_test typedef std::uniform_int_distribution 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::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 #include +#include // getenv, atoi +#include // 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 --im83/wVv0jiGQj4J--