Re: [PATCH][RFC] Sanitize equals and hash functions in hash-tables.

["Martin Liška" <mliska@suse.cz>]

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

On 6/12/19 11:41 AM, Richard Biener wrote:
> On Wed, Jun 12, 2019 at 11:15 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 6/12/19 10:02 AM, Martin Liška wrote:
>>> On 6/12/19 9:59 AM, Richard Biener wrote:
>>>> On Tue, Jun 11, 2019 at 9:02 PM Jason Merrill <jason@redhat.com> wrote:
>>>>>
>>>>> On 6/11/19 9:16 AM, Martin Liška wrote:
>>>>>> On 6/11/19 2:27 PM, Jason Merrill wrote:
>>>>>>> On 6/11/19 3:41 AM, Martin Liška wrote:
>>>>>>>> On 6/10/19 8:21 PM, Jason Merrill wrote:
>>>>>>>>> On Mon, Jun 10, 2019 at 3:08 AM Martin Liška <mliska@suse.cz> wrote:
>>>>>>>>>> On 6/7/19 11:43 PM, Jason Merrill wrote:
>>>>>>>>>>> On Fri, Jun 7, 2019 at 8:14 AM Martin Liška <mliska@suse.cz> wrote:
>>>>>>>>>>>> On 6/7/19 2:09 PM, Richard Biener wrote:
>>>>>>>>>>>>> On Fri, Jun 7, 2019 at 2:03 PM Martin Liška <mliska@suse.cz> wrote:
>>>>>>>>>>>>>> On 6/7/19 10:57 AM, Richard Biener wrote:
>>>>>>>>>>>>>>> On Mon, Jun 3, 2019 at 3:35 PM Martin Liška <mliska@suse.cz> wrote:
>>>>>>>>>>>>>>>> On 6/1/19 12:06 AM, Jeff Law wrote:
>>>>>>>>>>>>>>>>> On 5/22/19 3:13 AM, Martin Liška wrote:
>>>>>>>>>>>>>>>>>> On 5/21/19 1:51 PM, Richard Biener wrote:
>>>>>>>>>>>>>>>>>>> On Tue, May 21, 2019 at 1:02 PM Martin Liška <mliska@suse.cz> wrote:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On 5/21/19 11:38 AM, Richard Biener wrote:
>>>>>>>>>>>>>>>>>>>>> On Tue, May 21, 2019 at 12:07 AM Jeff Law <law@redhat.com> wrote:
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> On 5/13/19 1:41 AM, Martin Liška wrote:
>>>>>>>>>>>>>>>>>>>>>>> On 11/8/18 9:56 AM, Martin Liška wrote:
>>>>>>>>>>>>>>>>>>>>>>>> On 11/7/18 11:23 PM, Jeff Law wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> On 10/30/18 6:28 AM, Martin Liška wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> On 10/30/18 11:03 AM, Jakub Jelinek wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> On Mon, Oct 29, 2018 at 04:14:21PM +0100, Martin Liška wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> +hashtab_chk_error ()
>>>>>>>>>>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>>>>>>>>>>> +  fprintf (stderr, "hash table checking failed: "
>>>>>>>>>>>>>>>>>>>>>>>>>>>> +           "equal operator returns true for a pair "
>>>>>>>>>>>>>>>>>>>>>>>>>>>> +           "of values with a different hash value");
>>>>>>>>>>>>>>>>>>>>>>>>>>> BTW, either use internal_error here, or at least if using fprintf
>>>>>>>>>>>>>>>>>>>>>>>>>>> terminate with \n, in your recent mail I saw:
>>>>>>>>>>>>>>>>>>>>>>>>>>> ...different hash valueduring RTL pass: vartrack
>>>>>>>>>>>>>>>>>>>>>>>>>>>                       ^^^^^^
>>>>>>>>>>>>>>>>>>>>>>>>>> Sure, fixed in attached patch.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Martin
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> +  gcc_unreachable ();
>>>>>>>>>>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>>>>>>>>>>     Jakub
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> 0001-Sanitize-equals-and-hash-functions-in-hash-tables.patch
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>   From 0d9c979c845580a98767b83c099053d36eb49bb9 Mon Sep 17 00:00:00 2001
>>>>>>>>>>>>>>>>>>>>>>>>>> From: marxin <mliska@suse.cz>
>>>>>>>>>>>>>>>>>>>>>>>>>> Date: Mon, 29 Oct 2018 09:38:21 +0100
>>>>>>>>>>>>>>>>>>>>>>>>>> Subject: [PATCH] Sanitize equals and hash functions in hash-tables.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>>>>>>>>    gcc/hash-table.h | 40 +++++++++++++++++++++++++++++++++++++++-
>>>>>>>>>>>>>>>>>>>>>>>>>>    1 file changed, 39 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> 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_MEM_STAT_INFO) const;
>>>>>>>>>>>>>>>>>>>>>>>>>>      value_type *find_empty_slot_for_expand (hashval_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<Descriptor, Allocator>
>>>>>>>>>>>>>>>>>>>>>>>>>>      if (insert == INSERT && m_size * 3 <= m_n_elements * 4)
>>>>>>>>>>>>>>>>>>>>>>>>>>        expand ();
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> -  m_searches++;
>>>>>>>>>>>>>>>>>>>>>>>>>> +#if ENABLE_EXTRA_CHECKING
>>>>>>>>>>>>>>>>>>>>>>>>>> +    if (insert == INSERT)
>>>>>>>>>>>>>>>>>>>>>>>>>> +      verify (comparable, hash);
>>>>>>>>>>>>>>>>>>>>>>>>>> +#endif
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> +  m_searches++;
>>>>>>>>>>>>>>>>>>>>>>>>>>      value_type *first_deleted_slot = NULL;
>>>>>>>>>>>>>>>>>>>>>>>>>>      hashval_t index = hash_table_mod1 (hash, m_size_prime_index);
>>>>>>>>>>>>>>>>>>>>>>>>>>      hashval_t hash2 = hash_table_mod2 (hash, m_size_prime_index);
>>>>>>>>>>>>>>>>>>>>>>>>>> @@ -930,6 +935,39 @@ hash_table<Descriptor, Allocator>
>>>>>>>>>>>>>>>>>>>>>>>>>>      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 failed: "
>>>>>>>>>>>>>>>>>>>>>>>>>> +     "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 still 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=87845
>>>>>>>>>>>>>>>>>>>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87847
>>>>>>>>>>>>>>>>>>>>>>> Hi.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> I've just added one more PR:
>>>>>>>>>>>>>>>>>>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90450
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> I'm sending updated version of the patch that provides a disablement for the 3 PRs
>>>>>>>>>>>>>>>>>>>>>>> with a new function disable_sanitize_eq_and_hash.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> With that I can bootstrap and finish tests. However, I've done that with a patch
>>>>>>>>>>>>>>>>>>>>>>> limits maximal number of checks:
>>>>>>>>>>>>>>>>>>>>>> So rather than call the disable_sanitize_eq_and_hash, 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 where elements _in_ the
>>>>>>>>>>>>>>>>>>>>> table are never compared against each other but always against another
>>>>>>>>>>>>>>>>>>>>> object (I guess that's usual even), but the setup is in a way that the
>>>>>>>>>>>>>>>>>>>>> comparison function only works with those.  With the patch we verify
>>>>>>>>>>>>>>>>>>>>> hashing/comparison for something that is never used.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> So - wouldn't it be more "correct" to only verify comparison/hashing
>>>>>>>>>>>>>>>>>>>>> at lookup time, using the object from the lookup and verify that against
>>>>>>>>>>>>>>>>>>>>> all other elements?
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> I don't a have problem with that. Apparently this changes fixes
>>>>>>>>>>>>>>>>>>>> PR90450 and PR87847.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Changes from previous version:
>>>>>>>>>>>>>>>>>>>> - verification happens only when an element is searched (not inserted)
>>>>>>>>>>>>>>>>>>>> - new argument 'sanitize_eq_and_hash' added for hash_table::hash_table
>>>>>>>>>>>>>>>>>>>> - new param has been introduced hash-table-verification-limit in order
>>>>>>>>>>>>>>>>>>>>     to limit number of elements that are compared within a table
>>>>>>>>>>>>>>>>>>>> - verification happens only with flag_checking >= 2
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> I've been bootstrapping and testing the patch right now.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Looks like I misremembered the original patch.  The issue 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 regression tests
>>>>>>>>>>>>>>>>>> except for:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> $ ./xgcc -B. /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/torture/pr63941.c -O2 -c
>>>>>>>>>>>>>>>>>> hash table checking failed: equal operator returns true for a pair of values with a different hash value
>>>>>>>>>>>>>>>>>> during GIMPLE pass: lim
>>>>>>>>>>>>>>>>>> /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/torture/pr63941.c: In function ‘fn1’:
>>>>>>>>>>>>>>>>>> /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/torture/pr63941.c:6:1: internal compiler error: in hashtab_chk_error, at hash-table.h:1019
>>>>>>>>>>>>>>>>>>       6 | fn1 ()
>>>>>>>>>>>>>>>>>>         | ^~~
>>>>>>>>>>>>>>>>>> 0x6c5725 hashtab_chk_error
>>>>>>>>>>>>>>>>>>        /home/marxin/Programming/gcc/gcc/hash-table.h:1019
>>>>>>>>>>>>>>>>>> 0xe504ea hash_table<mem_ref_hasher, false, xcallocator>::verify(ao_ref* const&, unsigned int)
>>>>>>>>>>>>>>>>>>        /home/marxin/Programming/gcc/gcc/hash-table.h:1040
>>>>>>>>>>>>>>>>>> 0xe504ea hash_table<mem_ref_hasher, false, xcallocator>::find_slot_with_hash(ao_ref* const&, unsigned int, insert_option)
>>>>>>>>>>>>>>>>>>        /home/marxin/Programming/gcc/gcc/hash-table.h:960
>>>>>>>>>>>>>>>>>> 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_assert or somesuch?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Good point, I've just adjusted that.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Ready to be installed?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Ugh, the cselib one is really bad.  But I don't hold my breath 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 !CHECKING_P
>>>>>>>>>>>>>>> or at least make the if __builtin_expect (..., 0), ::verify not
>>>>>>>>>>>>>>> inlined and marked pure () (thus, !CHECKING_P is simplest ;)).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Fixed. May I install the patch? The cselib issue can be solved 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 function for
>>>>>>>>>>> call to ‘hash_table<hash_map<mem_alloc_d\
>>>>>>>>>>> escription<mem_usage>::mem_location_hash, mem_usage*,
>>>>>>>>>>> simple_hashmap_traits<default_hash_traits<mem_alloc_desc\
>>>>>>>>>>> ription<mem_usage>::mem_location_hash>, mem_usage*> >::hash_entry,
>>>>>>>>>>> false, xcallocator>::hash_table(size_t&, bo\
>>>>>>>>>>> ol&, bool&, mem_alloc_origin, const char*&, int&, const char*&)’
>>>>>>>>>>>        : m_table (n, ggc, gather_mem_stats, HASH_MAP_ORIGIN PASS_MEM_STAT) {}
>>>>>>>>>>>
>>>>>>>>>>> Looks like this needs to be updated to pass an argument to the 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 compiling in 13
>>>>>>>>> seconds to 78 seconds, 6 times as long.  I would expect template-heavy
>>>>>>>>> code to see similar problems when sanitization is enabled for those
>>>>>>>>> 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 can measure?
>>>>>>>
>>>>>>> This is the one I've been looking at:
>>>>>>>
>>>>>>>    struct Int {
>>>>>>>      constexpr Int(int v): v(v) {}
>>>>>>>      constexpr Int& operator+=(Int b) { this->v += b.v; return *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 = {0};
>>>>>>>      Int k = {0};
>>>>>>>      k = 0;
>>>>>>>      for (; k<10000; ++k) {
>>>>>>>        i += k;
>>>>>>>      }
>>>>>>>      return n;
>>>>>>>    }
>>>>>>>
>>>>>>>    template<int N> struct S {
>>>>>>>      static constexpr int sm = S<N-1>::sm+f(N);
>>>>>>>    };
>>>>>>>    template<> struct S<0> {
>>>>>>>      static constexpr int sm = 0;
>>>>>>>    };
>>>>>>>    constexpr int r = S<20>::sm;
>>>>>>>
>>>>>>> Jason
>>>>>>
>>>>>> For the test-case provided I see:
>>>>>>
>>>>>> $ time g++ time.cc -c --param hash-table-verification-limit=100
>>>>>>
>>>>>> real  0m1.855s
>>>>>> user  0m1.829s
>>>>>> sys   0m0.025s
>>>>>>
>>>>>> $ time g++ time.cc -c --param hash-table-verification-limit=0
>>>>>>
>>>>>> 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=all, but one would need to build all cross-compilers
>> to make a proper testing.
>>
>> Do you like the idea of the patch before I'll write a changelog and test 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 <typename Type>
> +inline bool
> +pointer_hash <Type>::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 performance
> critical parts for now.

I'm sending patch for it.

Martin

> 
> Richard.
> 
>> Thanks,
>> Martin
>>
>>>
>>> Martin
>>>
>>>>
>>>> Richard.
>>>>
>>>>> Jason
>>>
>>


[-- Attachment #2: 0001-Disable-hash-table-sanitization-for-mem-stats-maps.patch --]
[-- Type: text/x-patch, Size: 2978 bytes --]

From 71668e0eaf983ff05dfd52a95827cefdf24350ec Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Wed, 12 Jun 2019 11:04:11 +0200
Subject: [PATCH] Disable hash-table sanitization for mem stats maps.

gcc/ChangeLog:

2019-06-12  Martin Liska  <mliska@suse.cz>

	* ggc-common.c (ggc_prune_overhead_list): Do not sanitize
	the created map.
	* hash-map.h: Add sanitize_eq_and_hash into ::hash_map.
	* mem-stats.h (mem_alloc_description::mem_alloc_description):
	Do not sanitize created maps.
---
 gcc/ggc-common.c | 2 +-
 gcc/hash-map.h   | 9 ++++++---
 gcc/mem-stats.h  | 6 +++---
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/gcc/ggc-common.c b/gcc/ggc-common.c
index 2acdb6dc60c..6fb5a3d5ceb 100644
--- a/gcc/ggc-common.c
+++ b/gcc/ggc-common.c
@@ -1014,5 +1014,5 @@ ggc_prune_overhead_list (void)
       (*it).second.first->m_collected += (*it).second.second;
 
   delete ggc_mem_desc.m_reverse_object_map;
-  ggc_mem_desc.m_reverse_object_map = new map_t (13, false, false);
+  ggc_mem_desc.m_reverse_object_map = new map_t (13, false, false, false);
 }
diff --git a/gcc/hash-map.h b/gcc/hash-map.h
index a8eb42d5a03..588dfda04fa 100644
--- a/gcc/hash-map.h
+++ b/gcc/hash-map.h
@@ -118,16 +118,19 @@ class GTY((user)) hash_map
 
 public:
   explicit hash_map (size_t n = 13, bool ggc = false,
+		     bool sanitize_eq_and_hash = true,
 		     bool gather_mem_stats = GATHER_STATISTICS
 		     CXX_MEM_STAT_INFO)
-    : m_table (n, ggc, true, gather_mem_stats, HASH_MAP_ORIGIN PASS_MEM_STAT)
+    : m_table (n, ggc, sanitize_eq_and_hash, gather_mem_stats,
+	       HASH_MAP_ORIGIN PASS_MEM_STAT)
   {
   }
 
   explicit hash_map (const hash_map &h, bool ggc = false,
+		     bool sanitize_eq_and_hash = true,
 		     bool gather_mem_stats = GATHER_STATISTICS
 		     CXX_MEM_STAT_INFO)
-    : m_table (h.m_table, ggc, true, gather_mem_stats,
+    : m_table (h.m_table, ggc, sanitize_eq_and_hash, gather_mem_stats,
 	       HASH_MAP_ORIGIN PASS_MEM_STAT) {}
 
   /* Create a hash_map in ggc memory.  */
@@ -136,7 +139,7 @@ public:
 			       CXX_MEM_STAT_INFO)
     {
       hash_map *map = ggc_alloc<hash_map> ();
-      new (map) hash_map (size, true, gather_mem_stats PASS_MEM_STAT);
+      new (map) hash_map (size, true, true, gather_mem_stats PASS_MEM_STAT);
       return map;
     }
 
diff --git a/gcc/mem-stats.h b/gcc/mem-stats.h
index 63ce8712e2b..77960595753 100644
--- a/gcc/mem-stats.h
+++ b/gcc/mem-stats.h
@@ -559,9 +559,9 @@ template <class T>
 inline
 mem_alloc_description<T>::mem_alloc_description ()
 {
-  m_map = new mem_map_t (13, false, false);
-  m_reverse_map = new reverse_mem_map_t (13, false, false);
-  m_reverse_object_map = new reverse_object_map_t (13, false, false);
+  m_map = new mem_map_t (13, false, false, false);
+  m_reverse_map = new reverse_mem_map_t (13, false, false, false);
+  m_reverse_object_map = new reverse_object_map_t (13, false, false, false);
 }
 
 /* Default destructor.  */
-- 
2.21.0