On 6/12/19 11:41 AM, Richard Biener wrote:
> On Wed, Jun 12, 2019 at 11:15 AM Martin Liška <mli...@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 <ja...@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 <mli...@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 <mli...@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 <mli...@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 <mli...@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
>>>>>>>>>>>>>>>>>>> <mli...@suse.cz> wrote:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On 5/21/19 11:38 AM, Richard Biener wrote:
>>>>>>>>>>>>>>>>>>>>> On Tue, May 21, 2019 at 12:07 AM Jeff Law
>>>>>>>>>>>>>>>>>>>>> <l...@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 <mli...@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
>>>
>>
>From 71668e0eaf983ff05dfd52a95827cefdf24350ec Mon Sep 17 00:00:00 2001
From: Martin Liska <mli...@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 <mli...@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
