From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28493 invoked by alias); 12 Jun 2019 12:50:52 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 28479 invoked by uid 89); 12 Jun 2019 12:50:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-13.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,KAM_SHORT,RCVD_IN_DNSWL_NONE,SEM_FRESH,SEM_URIRED,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-lj1-f194.google.com Received: from mail-lj1-f194.google.com (HELO mail-lj1-f194.google.com) (209.85.208.194) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 12 Jun 2019 12:50:48 +0000 Received: by mail-lj1-f194.google.com with SMTP id v24so10639121ljg.13 for ; Wed, 12 Jun 2019 05:50:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=nj4p/b5uFAi1UR0gtL3qJf0ODhKD0HEjpsBoE9kicSs=; b=Nl3LJt9vCJBqFqqpdumts5fea5JU49VoJXx1DgSKqH5DdCkczG0MtqUUxRmJAa22H1 DSdWFK24cIycNiCJfdF9sntLV+kA1a+1qMNprrz5k4XM+h1DpZtRRpAk5vFW3pjbh/td Uncv/FpirzcPPHJumGDAmy6N/9KrLkZO9vfPiQS3IbcsnqgMOBJf8n1EJs62choRtdAk ghH3X+d/pSzX6B51inZm+z2+lCTQcpFDC62KOhkD1j10UYweOoLwdYYIKqePvXt2KTGE azVK3cQp++hgn8m/D5r27gEky2fK2gt3UBns8lZjwd0sNRGU0a9SMa1XOyE/E+2eY8KO T1bQ== MIME-Version: 1.0 References: <23ffca95-6492-e609-aebb-bbdd83b5185d@suse.cz> <999abc46-57c7-ccf9-b0c9-baf4c0686b16@suse.cz> <4faef430-49cf-13bc-4bb2-858a72668ae6@suse.cz> <243b87c2-91e0-063d-0682-de232656beaa@suse.cz> <96a94055-1f19-e76a-5753-ec72b088f363@suse.cz> <69fd71c8-b459-3922-9517-2364740e845a@suse.cz> <974d5683-57ee-8c42-af50-07d09803f085@redhat.com> <26ef6db6-a2fe-a232-0770-697e833b6542@suse.cz> <381562d2-cf65-8398-e03a-7eeb1f79ef98@suse.cz> In-Reply-To: <381562d2-cf65-8398-e03a-7eeb1f79ef98@suse.cz> From: Richard Biener Date: Wed, 12 Jun 2019 12:50:00 -0000 Message-ID: Subject: Re: [PATCH][RFC] Sanitize equals and hash functions in hash-tables. To: =?UTF-8?Q?Martin_Li=C5=A1ka?= Cc: Jason Merrill , Jeff Law , Jakub Jelinek , Alexander Monakov , GCC Patches , Nathan Sidwell , Paul Richard Thomas , Martin Jambor Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2019-06/txt/msg00701.txt.bz2 On Wed, Jun 12, 2019 at 1:45 PM Martin Li=C5=A1ka wrote: > > On 6/12/19 11:41 AM, Richard Biener wrote: > > On Wed, Jun 12, 2019 at 11:15 AM Martin Li=C5=A1ka wro= te: > >> > >> On 6/12/19 10:02 AM, Martin Li=C5=A1ka wrote: > >>> On 6/12/19 9:59 AM, Richard Biener wrote: > >>>> On Tue, Jun 11, 2019 at 9:02 PM Jason Merrill wro= te: > >>>>> > >>>>> On 6/11/19 9:16 AM, Martin Li=C5=A1ka wrote: > >>>>>> On 6/11/19 2:27 PM, Jason Merrill wrote: > >>>>>>> On 6/11/19 3:41 AM, Martin Li=C5=A1ka wrote: > >>>>>>>> On 6/10/19 8:21 PM, Jason Merrill wrote: > >>>>>>>>> On Mon, Jun 10, 2019 at 3:08 AM Martin Li=C5=A1ka wrote: > >>>>>>>>>> On 6/7/19 11:43 PM, Jason Merrill wrote: > >>>>>>>>>>> On Fri, Jun 7, 2019 at 8:14 AM Martin Li=C5=A1ka wrote: > >>>>>>>>>>>> On 6/7/19 2:09 PM, Richard Biener wrote: > >>>>>>>>>>>>> On Fri, Jun 7, 2019 at 2:03 PM Martin Li=C5=A1ka wrote: > >>>>>>>>>>>>>> On 6/7/19 10:57 AM, Richard Biener wrote: > >>>>>>>>>>>>>>> On Mon, Jun 3, 2019 at 3:35 PM Martin Li=C5=A1ka wrote: > >>>>>>>>>>>>>>>> On 6/1/19 12:06 AM, Jeff Law wrote: > >>>>>>>>>>>>>>>>> On 5/22/19 3:13 AM, Martin Li=C5=A1ka wrote: > >>>>>>>>>>>>>>>>>> On 5/21/19 1:51 PM, Richard Biener wrote: > >>>>>>>>>>>>>>>>>>> On Tue, May 21, 2019 at 1:02 PM Martin Li=C5=A1ka wrote: > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> On 5/21/19 11:38 AM, Richard Biener wrote: > >>>>>>>>>>>>>>>>>>>>> On Tue, May 21, 2019 at 12:07 AM Jeff Law wrote: > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> On 5/13/19 1:41 AM, Martin Li=C5=A1ka wrote: > >>>>>>>>>>>>>>>>>>>>>>> On 11/8/18 9:56 AM, Martin Li=C5=A1ka wrote: > >>>>>>>>>>>>>>>>>>>>>>>> On 11/7/18 11:23 PM, Jeff Law wrote: > >>>>>>>>>>>>>>>>>>>>>>>>> On 10/30/18 6:28 AM, Martin Li=C5=A1ka wrote: > >>>>>>>>>>>>>>>>>>>>>>>>>> On 10/30/18 11:03 AM, Jakub Jelinek wrote: > >>>>>>>>>>>>>>>>>>>>>>>>>>> On Mon, Oct 29, 2018 at 04:14:21PM +0100, Mar= tin Li=C5=A1ka wrote: > >>>>>>>>>>>>>>>>>>>>>>>>>>>> +hashtab_chk_error () > >>>>>>>>>>>>>>>>>>>>>>>>>>>> +{ > >>>>>>>>>>>>>>>>>>>>>>>>>>>> + fprintf (stderr, "hash table checking fai= led: " > >>>>>>>>>>>>>>>>>>>>>>>>>>>> + "equal operator returns true for= a pair " > >>>>>>>>>>>>>>>>>>>>>>>>>>>> + "of values with a different hash= value"); > >>>>>>>>>>>>>>>>>>>>>>>>>>> BTW, either use internal_error here, or at le= ast if using fprintf > >>>>>>>>>>>>>>>>>>>>>>>>>>> terminate with \n, in your recent mail I saw: > >>>>>>>>>>>>>>>>>>>>>>>>>>> ...different hash valueduring RTL pass: vartr= ack > >>>>>>>>>>>>>>>>>>>>>>>>>>> ^^^^^^ > >>>>>>>>>>>>>>>>>>>>>>>>>> Sure, fixed in attached patch. > >>>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>>>> Martin > >>>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>>>>>> + gcc_unreachable (); > >>>>>>>>>>>>>>>>>>>>>>>>>>>> +} > >>>>>>>>>>>>>>>>>>>>>>>>>>> Jakub > >>>>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>>>> 0001-Sanitize-equals-and-hash-functions-in-has= h-tables.patch > >>>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>>>> From 0d9c979c845580a98767b83c099053d36eb49bb= 9 Mon Sep 17 00:00:00 2001 > >>>>>>>>>>>>>>>>>>>>>>>>>> From: marxin > >>>>>>>>>>>>>>>>>>>>>>>>>> Date: Mon, 29 Oct 2018 09:38:21 +0100 > >>>>>>>>>>>>>>>>>>>>>>>>>> Subject: [PATCH] Sanitize equals and hash func= tions in hash-tables. > >>>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>>>> --- > >>>>>>>>>>>>>>>>>>>>>>>>>> gcc/hash-table.h | 40 +++++++++++++++++++++= ++++++++++++++++++- > >>>>>>>>>>>>>>>>>>>>>>>>>> 1 file changed, 39 insertions(+), 1 deletio= n(-) > >>>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>>>> diff --git a/gcc/hash-table.h b/gcc/hash-table= .h > >>>>>>>>>>>>>>>>>>>>>>>>>> index bd83345c7b8..694eedfc4be 100644 > >>>>>>>>>>>>>>>>>>>>>>>>>> --- a/gcc/hash-table.h > >>>>>>>>>>>>>>>>>>>>>>>>>> +++ b/gcc/hash-table.h > >>>>>>>>>>>>>>>>>>>>>>>>>> @@ -503,6 +503,7 @@ private: > >>>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>>>> value_type *alloc_entries (size_t n CXX_M= EM_STAT_INFO) const; > >>>>>>>>>>>>>>>>>>>>>>>>>> value_type *find_empty_slot_for_expand (h= ashval_t); > >>>>>>>>>>>>>>>>>>>>>>>>>> + void verify (const compare_type &comparable= , hashval_t hash); > >>>>>>>>>>>>>>>>>>>>>>>>>> bool too_empty_p (unsigned int); > >>>>>>>>>>>>>>>>>>>>>>>>>> void expand (); > >>>>>>>>>>>>>>>>>>>>>>>>>> static bool is_deleted (value_type &v) > >>>>>>>>>>>>>>>>>>>>>>>>>> @@ -882,8 +883,12 @@ hash_table > >>>>>>>>>>>>>>>>>>>>>>>>>> if (insert =3D=3D INSERT && m_size * 3 <= =3D m_n_elements * 4) > >>>>>>>>>>>>>>>>>>>>>>>>>> expand (); > >>>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>>>> - m_searches++; > >>>>>>>>>>>>>>>>>>>>>>>>>> +#if ENABLE_EXTRA_CHECKING > >>>>>>>>>>>>>>>>>>>>>>>>>> + if (insert =3D=3D INSERT) > >>>>>>>>>>>>>>>>>>>>>>>>>> + verify (comparable, hash); > >>>>>>>>>>>>>>>>>>>>>>>>>> +#endif > >>>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>>>> + m_searches++; > >>>>>>>>>>>>>>>>>>>>>>>>>> value_type *first_deleted_slot =3D NULL; > >>>>>>>>>>>>>>>>>>>>>>>>>> hashval_t index =3D hash_table_mod1 (hash= , m_size_prime_index); > >>>>>>>>>>>>>>>>>>>>>>>>>> hashval_t hash2 =3D hash_table_mod2 (hash= , m_size_prime_index); > >>>>>>>>>>>>>>>>>>>>>>>>>> @@ -930,6 +935,39 @@ hash_table > >>>>>>>>>>>>>>>>>>>>>>>>>> return &m_entries[index]; > >>>>>>>>>>>>>>>>>>>>>>>>>> } > >>>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>>>> +#if ENABLE_EXTRA_CHECKING > >>>>>>>>>>>>>>>>>>>>>>>>>> + > >>>>>>>>>>>>>>>>>>>>>>>>>> +/* Report a hash table checking error. */ > >>>>>>>>>>>>>>>>>>>>>>>>>> + > >>>>>>>>>>>>>>>>>>>>>>>>>> +ATTRIBUTE_NORETURN ATTRIBUTE_COLD > >>>>>>>>>>>>>>>>>>>>>>>>>> +static void > >>>>>>>>>>>>>>>>>>>>>>>>>> +hashtab_chk_error () > >>>>>>>>>>>>>>>>>>>>>>>>>> +{ > >>>>>>>>>>>>>>>>>>>>>>>>>> + fprintf (stderr, "hash table checking faile= d: " > >>>>>>>>>>>>>>>>>>>>>>>>>> + "equal operator returns true for a pair " > >>>>>>>>>>>>>>>>>>>>>>>>>> + "of values with a different hash value\n= "); > >>>>>>>>>>>>>>>>>>>>>>>>>> + gcc_unreachable (); > >>>>>>>>>>>>>>>>>>>>>>>>>> +} > >>>>>>>>>>>>>>>>>>>>>>>>> I think an internal_error here is probably stil= l better than a simple > >>>>>>>>>>>>>>>>>>>>>>>>> fprintf, even if the fprintf is terminated with= a \n :-) > >>>>>>>>>>>>>>>>>>>>>>>> Fully agree with that, but I see a lot of build = errors when using internal_error. > >>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>>> The question then becomes can we bootstrap with= this stuff enabled and > >>>>>>>>>>>>>>>>>>>>>>>>> if not, are we likely to soon? It'd be a shame= to put it into > >>>>>>>>>>>>>>>>>>>>>>>>> EXTRA_CHECKING, but then not be able to really = use EXTRA_CHECKING > >>>>>>>>>>>>>>>>>>>>>>>>> because we've got too many bugs to fix. > >>>>>>>>>>>>>>>>>>>>>>>> Unfortunately it's blocked with these 2 PRs: > >>>>>>>>>>>>>>>>>>>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D8= 7845 > >>>>>>>>>>>>>>>>>>>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D8= 7847 > >>>>>>>>>>>>>>>>>>>>>>> Hi. > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> I've just added one more PR: > >>>>>>>>>>>>>>>>>>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D90= 450 > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> I'm sending updated version of the patch that pro= vides a disablement for the 3 PRs > >>>>>>>>>>>>>>>>>>>>>>> with a new function disable_sanitize_eq_and_hash. > >>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>> With that I can bootstrap and finish tests. Howev= er, I've done that with a patch > >>>>>>>>>>>>>>>>>>>>>>> limits maximal number of checks: > >>>>>>>>>>>>>>>>>>>>>> So rather than call the disable_sanitize_eq_and_ha= sh, can you have its > >>>>>>>>>>>>>>>>>>>>>> state set up when you instantiate the object? It'= s not a huge deal, > >>>>>>>>>>>>>>>>>>>>>> just thinking about loud. > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> So how do we want to go forward, particularly the = EXTRA_EXTRA checking > >>>>>>>>>>>>>>>>>>>>>> issue :-) > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> There is at least one PR where we have a table wher= e elements _in_ the > >>>>>>>>>>>>>>>>>>>>> table are never compared against each other but alw= ays against another > >>>>>>>>>>>>>>>>>>>>> object (I guess that's usual even), but the setup i= s in a way that the > >>>>>>>>>>>>>>>>>>>>> comparison function only works with those. With th= e patch we verify > >>>>>>>>>>>>>>>>>>>>> hashing/comparison for something that is never used. > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> So - wouldn't it be more "correct" to only verify c= omparison/hashing > >>>>>>>>>>>>>>>>>>>>> at lookup time, using the object from the lookup an= d verify that against > >>>>>>>>>>>>>>>>>>>>> all other elements? > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> I don't a have problem with that. Apparently this ch= anges fixes > >>>>>>>>>>>>>>>>>>>> PR90450 and PR87847. > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> Changes from previous version: > >>>>>>>>>>>>>>>>>>>> - verification happens only when an element is searc= hed (not inserted) > >>>>>>>>>>>>>>>>>>>> - new argument 'sanitize_eq_and_hash' added for hash= _table::hash_table > >>>>>>>>>>>>>>>>>>>> - new param has been introduced hash-table-verificat= ion-limit in order > >>>>>>>>>>>>>>>>>>>> to limit number of elements that are compared wi= thin a table > >>>>>>>>>>>>>>>>>>>> - verification happens only with flag_checking >=3D 2 > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> I've been bootstrapping and testing the patch right = now. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> Looks like I misremembered the original patch. The i= ssue isn't > >>>>>>>>>>>>>>>>>>> comparing random two elements in the table. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> That it fixes PR90450 is because LIM never calls find= _slot_with_hash > >>>>>>>>>>>>>>>>>>> without INSERTing. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> There's updated version of the patch where I check all= find operations > >>>>>>>>>>>>>>>>>> (both w/ and w/o insertion). > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> Patch can bootstrap on x86_64-linux-gnu and survives r= egression tests > >>>>>>>>>>>>>>>>>> except for: > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> $ ./xgcc -B. /home/marxin/Programming/gcc/gcc/testsuit= e/gcc.dg/torture/pr63941.c -O2 -c > >>>>>>>>>>>>>>>>>> hash table checking failed: equal operator returns tru= e for a pair of values with a different hash value > >>>>>>>>>>>>>>>>>> during GIMPLE pass: lim > >>>>>>>>>>>>>>>>>> /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/tort= ure/pr63941.c: In function =E2=80=98fn1=E2=80=99: > >>>>>>>>>>>>>>>>>> /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/tort= ure/pr63941.c:6:1: internal compiler error: in hashtab_chk_error, at hash-t= able.h:1019 > >>>>>>>>>>>>>>>>>> 6 | fn1 () > >>>>>>>>>>>>>>>>>> | ^~~ > >>>>>>>>>>>>>>>>>> 0x6c5725 hashtab_chk_error > >>>>>>>>>>>>>>>>>> /home/marxin/Programming/gcc/gcc/hash-table.h:1= 019 > >>>>>>>>>>>>>>>>>> 0xe504ea hash_table::verify(ao_ref* const&, unsigned int) > >>>>>>>>>>>>>>>>>> /home/marxin/Programming/gcc/gcc/hash-table.h:1= 040 > >>>>>>>>>>>>>>>>>> 0xe504ea hash_table::find_slot_with_hash(ao_ref* const&, unsigned int, insert_option) > >>>>>>>>>>>>>>>>>> /home/marxin/Programming/gcc/gcc/hash-table.h:9= 60 > >>>>>>>>>>>>>>>>>> 0xe504ea gather_mem_refs_stmt > >>>>>>>>>>>>>>>>>> /home/marxin/Programming/gcc/gcc/tree-ssa-loop-= im.c:1501 > >>>>>>>>>>>>>>>>>> 0xe504ea analyze_memory_references > >>>>>>>>>>>>>>>>>> /home/marxin/Programming/gcc/gcc/tree-ssa-loop-= im.c:1625 > >>>>>>>>>>>>>>>>>> 0xe504ea tree_ssa_lim > >>>>>>>>>>>>>>>>>> /home/marxin/Programming/gcc/gcc/tree-ssa-loop-= im.c:2646 > >>>>>>>>>>>>>>>>>> 0xe504ea execute > >>>>>>>>>>>>>>>>>> /home/marxin/Programming/gcc/gcc/tree-ssa-loop-= im.c:2708 > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> Richi: it's after your recent patch. > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> For some reason I don't see PR87847 issue any longer. > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> May I install the patch with disabled sanitization in = tree-ssa-loop-im.c ? > >>>>>>>>>>>>>>>>> Don't we still need to deal with the naked fprintf when= there's a > >>>>>>>>>>>>>>>>> failure. ie, shouldn't we be raising it with a gcc_ass= ert or somesuch? > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Good point, I've just adjusted that. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Patch can bootstrap on x86_64-linux-gnu and survives reg= ression tests. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Ready to be installed? > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Ugh, the cselib one is really bad. But I don't hold my b= reath for anyone > >>>>>>>>>>>>>>> fixing it ... > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Yes :D It's been some time and there's no interest in the = PR. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> One question - there's unconditional > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> + if (m_sanitize_eq_and_hash) > >>>>>>>>>>>>>>> + verify (comparable, hash); > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> which will read a global variable and have (possibly not = inline) call > >>>>>>>>>>>>>>> to verify on a common path even with checking disabled. = So I think > >>>>>>>>>>>>>>> we want to compile this checking feature out for !CHECKIN= G_P > >>>>>>>>>>>>>>> or at least make the if __builtin_expect (..., 0), ::veri= fy not > >>>>>>>>>>>>>>> inlined and marked pure () (thus, !CHECKING_P is simplest= ;)). > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Fixed. May I install the patch? The cselib issue can be so= lved later.. > >>>>>>>>>>>>> > >>>>>>>>>>>>> You missed the second occurance > >>>>>>>>>>>>> > >>>>>>>>>>>>> - m_searches++; > >>>>>>>>>>>>> + if (m_sanitize_eq_and_hash) > >>>>>>>>>>>>> + verify (comparable, hash); > >>>>>>>>>>>> > >>>>>>>>>>>> Yep ;) I've just install the patch. > >>>>>>>>>>> > >>>>>>>>>>> This is breaking my build: > >>>>>>>>>>> > >>>>>>>>>>> /home/jason/gt/gcc/hash-map.h:123:71: error: no matching func= tion for > >>>>>>>>>>> call to =E2=80=98hash_table >>>>>>>>>>> escription::mem_location_hash, mem_usage*, > >>>>>>>>>>> simple_hashmap_traits >>>>>>>>>>> ription::mem_location_hash>, mem_usage*> >::hash_e= ntry, > >>>>>>>>>>> false, xcallocator>::hash_table(size_t&, bo\ > >>>>>>>>>>> ol&, bool&, mem_alloc_origin, const char*&, int&, const char*= &)=E2=80=99 > >>>>>>>>>>> : m_table (n, ggc, gather_mem_stats, HASH_MAP_ORIGIN P= ASS_MEM_STAT) {} > >>>>>>>>>>> > >>>>>>>>>>> Looks like this needs to be updated to pass an argument to th= e new > >>>>>>>>>>> sanitize_eq_and_hash parameter. > >>>>>>>>>>> > >>>>>>>>>>> Jason > >>>>>>>>>> > >>>>>>>>>> Sorry for the breakage, I've just fixed that in r272104. > >>>>>>>>> > >>>>>>>>> Thanks. I'm also seeing a massive compile time hit from this: = A > >>>>>>>>> constexpr testcase that I've been looking at went from compilin= g in 13 > >>>>>>>>> seconds to 78 seconds, 6 times as long. I would expect templat= e-heavy > >>>>>>>>> code to see similar problems when sanitization is enabled for t= hose > >>>>>>>>> hash tables. Could we keep the parameter low or 0 by default, = and > >>>>>>>>> just do occasional sanitize runs with it explicitly enabled? > >>>>>>>> > >>>>>>>> Makes sense to me. Can you please provide a test-case which I ca= n measure? > >>>>>>> > >>>>>>> This is the one I've been looking at: > >>>>>>> > >>>>>>> struct Int { > >>>>>>> constexpr Int(int v): v(v) {} > >>>>>>> constexpr Int& operator+=3D(Int b) { this->v +=3D b.v; retur= n *this; } > >>>>>>> constexpr Int& operator++() { ++this->v; return *this; } > >>>>>>> private: > >>>>>>> friend constexpr bool operator<(Int a, Int b) { return a.v <= b.v; } > >>>>>>> int v; > >>>>>>> }; > >>>>>>> constexpr int f(int n) { > >>>>>>> Int i =3D {0}; > >>>>>>> Int k =3D {0}; > >>>>>>> k =3D 0; > >>>>>>> for (; k<10000; ++k) { > >>>>>>> i +=3D k; > >>>>>>> } > >>>>>>> return n; > >>>>>>> } > >>>>>>> > >>>>>>> template struct S { > >>>>>>> static constexpr int sm =3D S::sm+f(N); > >>>>>>> }; > >>>>>>> template<> struct S<0> { > >>>>>>> static constexpr int sm =3D 0; > >>>>>>> }; > >>>>>>> constexpr int r =3D S<20>::sm; > >>>>>>> > >>>>>>> Jason > >>>>>> > >>>>>> For the test-case provided I see: > >>>>>> > >>>>>> $ time g++ time.cc -c --param hash-table-verification-limit=3D100 > >>>>>> > >>>>>> real 0m1.855s > >>>>>> user 0m1.829s > >>>>>> sys 0m0.025s > >>>>>> > >>>>>> $ time g++ time.cc -c --param hash-table-verification-limit=3D0 > >>>>>> > >>>>>> real 0m1.275s > >>>>>> user 0m1.219s > >>>>>> sys 0m0.052s > >>>>>> > >>>>>> $ time g++-9 time.cc -c > >>>>>> > >>>>>> real 0m0.939s > >>>>>> user 0m0.827s > >>>>>> sys 0m0.109s > >>>>>> > >>>>>> So it's slower, but I can't confirm the huge slowdown you see. > >>>>>> Is it due to r272144? > >>>>> > >>>>> Hmm, I wonder if this is because of the > >>>>> --enable-gather-detailed-mem-stats hash tables. > >>>> > >>>> I wonder if we can reduce the overhead by making > >>>> hash-tables/maps using predefined traits not perform > >>>> the checking? Thus make [the default] whether to check or not > >>>> to check part of the traits? Surely the basic pointer-hash/map > >>>> stuff is OK and needs no extra checking. > >>> > >>> Interesting idea! I can prepare a patch. Right now I'm testing a patch > >>> that removes sanitization for hash-tables (and maps) that track memory > >>> allocations. > >> > >> I've got 2 patch candidates that will make it happen. It survives build > >> on --enable-languages=3Dall, but one would need to build all cross-com= pilers > >> to make a proper testing. > >> > >> Do you like the idea of the patch before I'll write a changelog and te= st it? > > > > The disabling for mem-stats patch looks good to me. I don't like the > > implementation of the traits one, we don't want to have to put the > > default returning true everywhere. Instead I expected some > > enable_if <> magic to do select a default true if the member isn't > > present. The issue with the traits idea is also that people like > > to derive from one of the standard traits and thus might inherit > > 'false' even though they override hash/compare methods. That is, > > I expected > > > > +template > > +inline bool > > +pointer_hash ::sanitize () > > +{ > > + return true; > > +} > > > > to return false for example. Basically for the case we > > auto-detect the traits based on the key type I wanted to > > disable sanitizing and for custom users leave them to > > per-object decisions. > > > > Not sure if we have enough C++ power to make that happen easily. > > Huh, I've tried quite hard and I was unable to make it happen. Maybe > somebody more familiar with C++ can step in? > > > So let's go the route of disabling the "ovbiously" correct and performa= nce > > critical parts for now. > > I'm sending patch for it. explicit hash_map (size_t n =3D 13, bool ggc =3D false, + bool sanitize_eq_and_hash =3D true, bool gather_mem_stats =3D GATHER_STATISTICS CXX_MEM_STAT_INFO) maybe not an issue here but adding defaulted params into the middle is prone to change existing calls semantics. Consider hash_map (13, false, false); which was gather_mem_stats =3D=3D false before but now is sanitize_eq_and_hash. I realize the original patch had the same issue. Patch is still OK. Please grep for explicit constructor calls and double-check. Ick. I guess also hash_map baz (13, false, false); is affected so make sure to apply enough grep-fu. New params should always go to the end ;) Richard. > > Martin > > > > > Richard. > > > >> Thanks, > >> Martin > >> > >>> > >>> Martin > >>> > >>>> > >>>> Richard. > >>>> > >>>>> Jason > >>> > >> >