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:
diff --git a/gcc/hash-table.h b/gcc/hash-table.h
index dc24fea6405..57564914e31 100644
--- a/gcc/hash-table.h
+++ b/gcc/hash-table.h
@@ -1027,7 +1027,7 @@ void
hash_table<Descriptor, Lazy, Allocator>
::verify (const compare_type &comparable, hashval_t hash)
{
- for (size_t i = 0; i < m_size; i++)
+ for (size_t i = 0; i < MIN (m_size, 1000); i++)
{
value_type *entry = &m_entries[i];
if (!is_empty (*entry) && !is_deleted (*entry)
Without that it would be probably terribly slow. Moreover, one probably does
not want
that with an extra checking, but with an extra-extra checking. Ideas about
where to enable
it?
Would it be possible to add the sanitization with the aforementioned
disablement?
Thanks,
Martin
>
> I'm fine with having the patch in in next stage1 after the problems will
> be fixed.
>
> Martin
>
>>
>>> +
>>> +/* Verify that all existing elements in th hash table which are
>> s/th/the/
>>
>>
>> Jeff
>>
>
>From fee775b5f4443e6eaeb4028e9a8922ea4ec8703f Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Mon, 13 May 2019 07:16:22 +0200
Subject: [PATCH] Enable sanitization for hash tables.
---
gcc/cp/pt.c | 4 ++++
gcc/cselib.c | 2 ++
gcc/hash-table.h | 53 ++++++++++++++++++++++++++++++++++++++++--
gcc/tree-ssa-loop-im.c | 2 ++
4 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index d6976e08690..db9c953e3dd 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -28320,7 +28320,11 @@ void
init_template_processing (void)
{
decl_specializations = hash_table<spec_hasher>::create_ggc (37);
+ /* FIXME: enable sanitization */
+ decl_specializations->disable_sanitize_eq_and_hash ();
type_specializations = hash_table<spec_hasher>::create_ggc (37);
+ /* FIXME: enable sanitization */
+ type_specializations->disable_sanitize_eq_and_hash ();
if (cxx_dialect >= cxx11)
declare_integer_pack ();
diff --git a/gcc/cselib.c b/gcc/cselib.c
index 84c17c23f6d..a60865c810a 100644
--- a/gcc/cselib.c
+++ b/gcc/cselib.c
@@ -2859,6 +2859,8 @@ cselib_init (int record_what)
used_regs = XNEWVEC (unsigned int, cselib_nregs);
n_used_regs = 0;
cselib_hash_table = new hash_table<cselib_hasher> (31);
+ /* FIXME: enable sanitization */
+ cselib_hash_table->disable_sanitize_eq_and_hash ();
if (cselib_preserve_constants)
cselib_preserved_hash_table = new hash_table<cselib_hasher> (31);
next_uid = 1;
diff --git a/gcc/hash-table.h b/gcc/hash-table.h
index 4178616478e..dc24fea6405 100644
--- a/gcc/hash-table.h
+++ b/gcc/hash-table.h
@@ -405,6 +405,9 @@ public:
/* Return true when there are no elements in this hash table. */
bool is_empty () const { return elements () == 0; }
+ /* Disable equal and hash function sanitization. */
+ void disable_sanitize_eq_and_hash (void) { m_sanitize_eq_and_hash = false; }
+
/* This function clears a specified SLOT in a hash table. It is
useful when you've already done the lookup and don't want to do it
again. */
@@ -516,6 +519,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)
@@ -564,6 +568,9 @@ private:
/* if m_entries is stored in ggc memory. */
bool m_ggc;
+ /* True if the table should be sanitized for equal and hash functions. */
+ bool m_sanitize_eq_and_hash;
+
/* If we should gather memory statistics for the table. */
#if GATHER_STATISTICS
bool m_gather_mem_stats;
@@ -591,7 +598,7 @@ hash_table<Descriptor, Lazy, Allocator>::hash_table (size_t size, bool ggc,
mem_alloc_origin origin
MEM_STAT_DECL) :
m_n_elements (0), m_n_deleted (0), m_searches (0), m_collisions (0),
- m_ggc (ggc)
+ m_ggc (ggc), m_sanitize_eq_and_hash (true)
#if GATHER_STATISTICS
, m_gather_mem_stats (gather_mem_stats)
#endif
@@ -941,8 +948,12 @@ hash_table<Descriptor, Lazy, Allocator>
if (insert == INSERT && m_size * 3 <= m_n_elements * 4)
expand ();
- m_searches++;
+#if ENABLE_EXTRA_CHECKING
+ if (m_sanitize_eq_and_hash && 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);
@@ -989,6 +1000,44 @@ hash_table<Descriptor, Lazy, 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");
+#ifndef GENERATOR_FILE
+ gcc_unreachable ();
+#else
+ exit (1);
+#endif
+}
+
+/* Verify that all existing elements in th hash table which are
+ equal to COMPARABLE have an equal HASH value provided as argument. */
+
+template<typename Descriptor, bool Lazy,
+ template<typename Type> class Allocator>
+void
+hash_table<Descriptor, Lazy, Allocator>
+::verify (const compare_type &comparable, hashval_t hash)
+{
+ for (size_t i = 0; i < m_size; i++)
+ {
+ value_type *entry = &m_entries[i];
+ if (!is_empty (*entry) && !is_deleted (*entry)
+ && hash != Descriptor::hash (*entry)
+ && Descriptor::equal (*entry, comparable))
+ hashtab_chk_error ();
+ }
+}
+#endif
+
/* This function deletes an element with the given COMPARABLE value
from hash table starting with the given HASH. If there is no
matching element in the hash table, this function does nothing. */
diff --git a/gcc/tree-ssa-loop-im.c b/gcc/tree-ssa-loop-im.c
index 56d8e8e4330..1bef286d46b 100644
--- a/gcc/tree-ssa-loop-im.c
+++ b/gcc/tree-ssa-loop-im.c
@@ -2565,6 +2565,8 @@ tree_ssa_lim_initialize (void)
alloc_aux_for_edges (0);
memory_accesses.refs = new hash_table<mem_ref_hasher> (100);
+ /* FIXME: enable sanitization */
+ memory_accesses.refs->disable_sanitize_eq_and_hash ();
memory_accesses.refs_list.create (100);
/* Allocate a special, unanalyzable mem-ref with ID zero. */
memory_accesses.refs_list.quick_push
--
2.21.0
