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

[Richard Biener]

On Tue, Jun 25, 2019 at 12:25 PM Martin Liška  wrote:
>
> On 6/24/19 4:09 PM, Richard Biener wrote:
> > You still get one instance in each TU ...
>
> Right, fixed in attached patch.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

Yes.

Thanks,
Richard.

> Thanks,
> Martin

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

[Martin Liška]

On 6/25/19 12:25 PM, Martin Liška wrote:
> On 6/24/19 4:09 PM, Richard Biener wrote:
>> You still get one instance in each TU ...
> 
> Right, fixed in attached patch.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin
> 

Btw. clang provides a reasonable warning for that:

/home/marxin/Programming/gcc/gcc/hash-table.h:1017:1: warning: 'static' 
function 'hashtab_chk_error' declared in header file should be declared 'static 
inline' [-Wunneeded-internal-declaration]

Martin

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

[Martin Liška]

On 6/24/19 4:09 PM, Richard Biener wrote:
> You still get one instance in each TU ...

Right, fixed in attached patch.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin
>From aa5ea14a8665b14aa60245c42bd4c9809d0bf81a Mon Sep 17 00:00:00 2001
From: Martin Liska 
Date: Tue, 25 Jun 2019 10:33:39 +0200
Subject: [PATCH] Put hashtab_chk_error into hash-table.c.

gcc/ChangeLog:

2019-06-25  Martin Liska  

	* hash-table.c (hashtab_chk_error): Move here from ...
	* hash-table.h (hashtab_chk_error): ... here.
---
 gcc/hash-table.c | 12 
 gcc/hash-table.h | 14 ++
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/gcc/hash-table.c b/gcc/hash-table.c
index 8e86fffa36f..e3b5d3da09e 100644
--- a/gcc/hash-table.c
+++ b/gcc/hash-table.c
@@ -124,3 +124,15 @@ void dump_hash_table_loc_statistics (void)
   hash_table_usage ().dump (origin);
 }
 }
+
+/* Report a hash table checking error.  */
+
+ATTRIBUTE_NORETURN ATTRIBUTE_COLD
+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 ();
+}
diff --git a/gcc/hash-table.h b/gcc/hash-table.h
index 4f5e150a0ac..a39fb942158 100644
--- a/gcc/hash-table.h
+++ b/gcc/hash-table.h
@@ -303,6 +303,8 @@ extern unsigned int hash_table_sanitize_eq_limit;
 extern unsigned int hash_table_higher_prime_index (unsigned long n)
ATTRIBUTE_PURE;
 
+extern ATTRIBUTE_NORETURN ATTRIBUTE_COLD void hashtab_chk_error ();
+
 /* Return X % Y using multiplicative inverse values INV and SHIFT.
 
The multiplicative inverses computed above are for 32-bit types,
@@ -1010,18 +1012,6 @@ hash_table
   return _entries[index];
 }
 
-/* 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 ();
-}
-
 /* Verify that all existing elements in th hash table which are
equal to COMPARABLE have an equal HASH value provided as argument.  */
 
-- 
2.21.0

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

[Richard Biener]

On Mon, Jun 24, 2019 at 3:51 PM Martin Liška  wrote:
>
> On 6/24/19 2:29 PM, Richard Biener wrote:
> > On Mon, Jun 24, 2019 at 1:08 AM Ian Lance Taylor  wrote:
> >>
> >> On Fri, Jun 7, 2019 at 5:04 AM Martin Liška  wrote:
> >>>
> >>> On 6/7/19 10:57 AM, Richard Biener wrote:
>  On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
> >>> 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 , hashval_t 
> >>> hash);
> >>>bool too_empty_p (unsigned int);
> >>>void expand ();
> >>>static bool is_deleted (value_type )
> >>> @@ -882,8 +883,12 @@ hash_table
> >>>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
> >>>return _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 

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

[Martin Liška]

On 6/24/19 2:29 PM, Richard Biener wrote:
> On Mon, Jun 24, 2019 at 1:08 AM Ian Lance Taylor  wrote:
>>
>> On Fri, Jun 7, 2019 at 5:04 AM Martin Liška  wrote:
>>>
>>> On 6/7/19 10:57 AM, Richard Biener wrote:
 On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
>>> 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 , hashval_t hash);
>>>bool too_empty_p (unsigned int);
>>>void expand ();
>>>static bool is_deleted (value_type )
>>> @@ -882,8 +883,12 @@ hash_table
>>>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
>>>return _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
> 

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

[Richard Biener]

On Mon, Jun 24, 2019 at 1:08 AM Ian Lance Taylor  wrote:
>
> On Fri, Jun 7, 2019 at 5:04 AM Martin Liška  wrote:
> >
> > On 6/7/19 10:57 AM, Richard Biener wrote:
> > > On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
> >  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 , hashval_t 
> >  hash);
> > bool too_empty_p (unsigned int);
> > void expand ();
> > static bool is_deleted (value_type )
> >  @@ -882,8 +883,12 @@ hash_table
> > 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
> > return _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 

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

[Ian Lance Taylor]

On Fri, Jun 7, 2019 at 5:04 AM Martin Liška  wrote:
>
> On 6/7/19 10:57 AM, Richard Biener wrote:
> > On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
>  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 , hashval_t hash);
> bool too_empty_p (unsigned int);
> void expand ();
> static bool is_deleted (value_type )
>  @@ -882,8 +883,12 @@ hash_table
> 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
> return _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 

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

[Martin Liška]

On 6/12/19 2:50 PM, Richard Biener wrote:
> New params should always go to the end 

Ah, sorry, I'll take of that new time.

I've just changed the function signature and I can verify there are no
other calls (tested for --enable-languages=all).

Thanks,
Martin

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

[Richard Biener]

On Wed, Jun 12, 2019 at 1:45 PM Martin Liška  wrote:
>
> On 6/12/19 11:41 AM, Richard Biener wrote:
> > On Wed, Jun 12, 2019 at 11:15 AM Martin Liška  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  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  wrote:
> >> On 6/7/19 11:43 PM, Jason Merrill wrote:
> >>> On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  
> >>> wrote:
>  On 6/7/19 2:09 PM, Richard Biener wrote:
> > On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  
> > wrote:
> >> On 6/7/19 10:57 AM, Richard Biener wrote:
> >>> On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  
> >>> 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 
> >>>  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š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 
> >> 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 , 
> >> hashval_t hash);
> >>  bool too_empty_p (unsigned int);
> >>  void expand ();
> >>  static bool is_deleted (value_type )
> >> @@ -882,8 +883,12 @@ hash_table >> Allocator>
> >>  if (insert == INSERT && m_size * 3 <= 
> >> m_n_elements * 4)
> >>expand ();
> 

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

[Martin Liška]

On 6/12/19 11:41 AM, Richard Biener wrote:
> On Wed, Jun 12, 2019 at 11:15 AM Martin Liška  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  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  wrote:
>> On 6/7/19 11:43 PM, Jason Merrill wrote:
>>> On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:
 On 6/7/19 2:09 PM, Richard Biener wrote:
> On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  
> wrote:
>> On 6/7/19 10:57 AM, Richard Biener wrote:
>>> On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  
>>> 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 
>>>  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š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 
>> 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 , 
>> hashval_t hash);
>>  bool too_empty_p (unsigned int);
>>  void expand ();
>>  static bool is_deleted (value_type )
>> @@ -882,8 +883,12 @@ hash_table> 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
>>

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

[Richard Biener]

On Wed, Jun 12, 2019 at 11:15 AM Martin Liška  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  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  wrote:
>  On 6/7/19 11:43 PM, Jason Merrill wrote:
> > On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:
> >> On 6/7/19 2:09 PM, Richard Biener wrote:
> >>> On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  
> >>> wrote:
>  On 6/7/19 10:57 AM, Richard Biener wrote:
> > On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  
> > 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 
> >  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š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 
>  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 , 
>  hashval_t hash);
>   bool too_empty_p (unsigned int);
>   void expand ();
>   static bool is_deleted (value_type )
>  @@ -882,8 +883,12 @@ hash_table  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++;
> 

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

[Martin Liška]

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  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  wrote:
 On 6/7/19 11:43 PM, Jason Merrill wrote:
> On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:
>> On 6/7/19 2:09 PM, Richard Biener wrote:
>>> On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:
 On 6/7/19 10:57 AM, Richard Biener wrote:
> On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  
> 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  
> 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š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 
 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 , 
 hashval_t hash);
  bool too_empty_p (unsigned int);
  void expand ();
  static bool is_deleted (value_type )
 @@ -882,8 +883,12 @@ hash_table
  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 @@ 

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

[Martin Liška]

On 6/12/19 9:59 AM, Richard Biener wrote:
> On Tue, Jun 11, 2019 at 9:02 PM Jason Merrill  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  wrote:
>>> On 6/7/19 11:43 PM, Jason Merrill wrote:
 On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:
> On 6/7/19 2:09 PM, Richard Biener wrote:
>> On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:
>>> On 6/7/19 10:57 AM, Richard Biener wrote:
 On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  
 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š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 
>>> 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 , 
>>> hashval_t hash);
>>>  bool too_empty_p (unsigned int);
>>>  void expand ();
>>>  static bool is_deleted (value_type )
>>> @@ -882,8 +883,12 @@ hash_table
>>>  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
>>>  return _entries[index];
>>>}
>>>
>>> +#if ENABLE_EXTRA_CHECKING

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

[Richard Biener]

On Tue, Jun 11, 2019 at 9:02 PM Jason Merrill  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  wrote:
> > On 6/7/19 11:43 PM, Jason Merrill wrote:
> >> On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:
> >>> On 6/7/19 2:09 PM, Richard Biener wrote:
>  On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:
> > On 6/7/19 10:57 AM, Richard Biener wrote:
> >> On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  
> >> 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š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 
> > 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 , 
> > hashval_t hash);
> >  bool too_empty_p (unsigned int);
> >  void expand ();
> >  static bool is_deleted (value_type )
> > @@ -882,8 +883,12 @@ hash_table
> >  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
> >  return _entries[index];
> >}
> >
> > +#if ENABLE_EXTRA_CHECKING
> > +
> > +/* 

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

[Jason Merrill]

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  wrote:

On 6/7/19 11:43 PM, Jason Merrill wrote:

On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:

On 6/7/19 2:09 PM, Richard Biener wrote:

On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:

On 6/7/19 10:57 AM, Richard Biener wrote:

On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
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 , hashval_t hash);
     bool too_empty_p (unsigned int);
     void expand ();
     static bool is_deleted (value_type )
@@ -882,8 +883,12 @@ hash_table
     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
     return _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 

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

[Martin Liška]

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  wrote:
 On 6/7/19 11:43 PM, Jason Merrill wrote:
> On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:
>> On 6/7/19 2:09 PM, Richard Biener wrote:
>>> On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:
 On 6/7/19 10:57 AM, Richard Biener wrote:
> On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  
> 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š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 
 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 , hashval_t 
 hash);
     bool too_empty_p (unsigned int);
     void expand ();
     static bool is_deleted (value_type )
 @@ -882,8 +883,12 @@ hash_table
     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
     return _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");

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

[Jason Merrill]

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  wrote:

On 6/7/19 11:43 PM, Jason Merrill wrote:

On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:

On 6/7/19 2:09 PM, Richard Biener wrote:

On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:

On 6/7/19 10:57 AM, Richard Biener wrote:

On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
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 , hashval_t hash);
bool too_empty_p (unsigned int);
void expand ();
static bool is_deleted (value_type )
@@ -882,8 +883,12 @@ hash_table
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
return _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 

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

[Martin Liška]

On 6/10/19 8:21 PM, Jason Merrill wrote:
> On Mon, Jun 10, 2019 at 3:08 AM Martin Liška  wrote:
>> On 6/7/19 11:43 PM, Jason Merrill wrote:
>>> On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:
 On 6/7/19 2:09 PM, Richard Biener wrote:
> On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:
>> On 6/7/19 10:57 AM, Richard Biener wrote:
>>> On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
>> 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 , hashval_t 
>> hash);
>>bool too_empty_p (unsigned int);
>>void expand ();
>>static bool is_deleted (value_type )
>> @@ -882,8 +883,12 @@ hash_table
>>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
>>return _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 

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

[Jason Merrill]

On Mon, Jun 10, 2019 at 3:08 AM Martin Liška  wrote:
> On 6/7/19 11:43 PM, Jason Merrill wrote:
> > On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:
> >> On 6/7/19 2:09 PM, Richard Biener wrote:
> >>> On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:
>  On 6/7/19 10:57 AM, Richard Biener wrote:
> > On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
>  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 , hashval_t 
>  hash);
> bool too_empty_p (unsigned int);
> void expand ();
> static bool is_deleted (value_type )
>  @@ -882,8 +883,12 @@ hash_table
> 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
> return _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.
> >>

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

[Martin Liška]

On 6/7/19 11:43 PM, Jason Merrill wrote:
> On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:
>>
>> On 6/7/19 2:09 PM, Richard Biener wrote:
>>> On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:

 On 6/7/19 10:57 AM, Richard Biener wrote:
> On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
 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 , hashval_t 
 hash);
bool too_empty_p (unsigned int);
void expand ();
static bool is_deleted (value_type )
 @@ -882,8 +883,12 @@ hash_table
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
return _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

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

[Jason Merrill]

On Fri, Jun 7, 2019 at 8:14 AM Martin Liška  wrote:
>
> On 6/7/19 2:09 PM, Richard Biener wrote:
> > On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:
> >>
> >> On 6/7/19 10:57 AM, Richard Biener wrote:
> >>> On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
> >> 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 , hashval_t 
> >> hash);
> >>bool too_empty_p (unsigned int);
> >>void expand ();
> >>static bool is_deleted (value_type )
> >> @@ -882,8 +883,12 @@ hash_table
> >>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
> >>return _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 

Re: [PATCH] Fix bootstrap (was: Re: [PATCH][RFC] Sanitize equals and hash functions in hash-tables.)

[Martin Liška]

On 6/7/19 5:13 PM, Jakub Jelinek wrote:

Ok for trunk?  Which one?


Thank you Jakub for the fix. I'm fine with both.

Martin

[PATCH] Fix bootstrap (was: Re: [PATCH][RFC] Sanitize equals and hash functions in hash-tables.)

[Jakub Jelinek]

Hi!

Attached are two different fixes, each of which fixes the build.
The first one arranges gencondmd to be linked against build/errors.o,
so that it links fine, the second one instead doesn't define the function
and method which requires that, as it is only called from #if CHECKING_P
guarded code.

Ok for trunk?  Which one?

Jakub
2019-06-07  Jakub Jelinek  

* Makefile.in (genprogerr): Add condmd.
(genprog): Remove it here.

--- gcc/Makefile.in.jj  2019-05-06 09:46:11.117792961 +0200
+++ gcc/Makefile.in 2019-06-07 16:59:21.248627607 +0200
@@ -2802,11 +2802,11 @@ genprogmd = $(genprogrtl) mddeps constan
 $(genprogmd:%=build/gen%$(build_exeext)): $(BUILD_MD)
 
 # All these programs need to report errors.
-genprogerr = $(genprogmd) genrtl modes gtype hooks cfn-macros
+genprogerr = $(genprogmd) genrtl modes gtype hooks cfn-macros condmd
 $(genprogerr:%=build/gen%$(build_exeext)): $(BUILD_ERRORS)
 
 # Remaining build programs.
-genprog = $(genprogerr) check checksum condmd match
+genprog = $(genprogerr) check checksum match
 
 # These programs need libs over and above what they get from the above list.
 build/genautomata$(build_exeext) : BUILD_LIBS += -lm
2019-06-07  Jakub Jelinek  

* hash-table.h (hashtab_chk_error, hash_table::verify): Only define
if CHECKING_P.

--- gcc/hash-table.h.jj 2019-06-07 16:55:07.675622566 +0200
+++ gcc/hash-table.h2019-06-07 17:05:36.212720156 +0200
@@ -1012,6 +1012,7 @@ hash_table
 
 /* Report a hash table checking error.  */
 
+#if CHECKING_P
 ATTRIBUTE_NORETURN ATTRIBUTE_COLD
 static void
 hashtab_chk_error ()
@@ -1040,6 +1041,7 @@ hash_table
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

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

[Martin Sebor]

On 6/7/19 6:13 AM, Martin Liška wrote:

On 6/7/19 2:09 PM, Richard Biener wrote:

On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:


On 6/7/19 10:57 AM, Richard Biener wrote:

On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
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 , hashval_t hash);
bool too_empty_p (unsigned int);
void expand ();
static bool is_deleted (value_type )
@@ -882,8 +883,12 @@ hash_table
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
return _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 

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

[Martin Liška]

On 6/7/19 2:09 PM, Richard Biener wrote:
> On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:
>>
>> On 6/7/19 10:57 AM, Richard Biener wrote:
>>> On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
>> 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 , hashval_t hash);
>>bool too_empty_p (unsigned int);
>>void expand ();
>>static bool is_deleted (value_type )
>> @@ -882,8 +883,12 @@ hash_table
>>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
>>return _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 

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

[Richard Biener]

On Fri, Jun 7, 2019 at 2:03 PM Martin Liška  wrote:
>
> On 6/7/19 10:57 AM, Richard Biener wrote:
> > On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
>  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 , hashval_t hash);
> bool too_empty_p (unsigned int);
> void expand ();
> static bool is_deleted (value_type )
>  @@ -882,8 +883,12 @@ hash_table
> 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
> return _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 

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

[Martin Liška]

On 6/7/19 10:57 AM, Richard Biener wrote:
> On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
 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 , hashval_t hash);
bool too_empty_p (unsigned int);
void expand ();
static bool is_deleted (value_type )
 @@ -882,8 +883,12 @@ hash_table
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
return _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 

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

[Richard Biener]

On Mon, Jun 3, 2019 at 3:35 PM Martin Liška  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  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š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 
> >> 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 , hashval_t hash);
> >>bool too_empty_p (unsigned int);
> >>void expand ();
> >>static bool is_deleted (value_type )
> >> @@ -882,8 +883,12 @@ hash_table
> >>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
> >>return _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
> >> 

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

[Martin Liška]

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  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š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 
>> 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 , hashval_t hash);
>>bool too_empty_p (unsigned int);
>>void expand ();
>>static bool is_deleted (value_type )
>> @@ -882,8 +883,12 @@ hash_table
>>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
>>return _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
> 

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

[Jeff Law]

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  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š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 
> 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 , hashval_t hash);
>bool too_empty_p (unsigned int);
>void expand ();
>static bool is_deleted (value_type )
> @@ -882,8 +883,12 @@ hash_table
>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
>return _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 

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

[Martin Liška]

On 5/31/19 2:50 PM, Richard Biener wrote:
> On Wed, May 22, 2019 at 11: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  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š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 
>> 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 , hashval_t hash);
>>bool too_empty_p (unsigned int);
>>void expand ();
>>static bool is_deleted (value_type )
>> @@ -882,8 +883,12 @@ hash_table
>>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
>>return _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 

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

[Richard Biener]

On Wed, May 22, 2019 at 11: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  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š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 
>  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 , hashval_t hash);
> bool too_empty_p (unsigned int);
> void expand ();
> static bool is_deleted (value_type )
>  @@ -882,8 +883,12 @@ hash_table
> 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
> return _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 

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

[Martin Liška]

On 5/21/19 1:51 PM, Richard Biener wrote:
> On Tue, May 21, 2019 at 1:02 PM Martin Liška  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š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 
 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 , hashval_t hash);
bool too_empty_p (unsigned int);
void expand ();
static bool is_deleted (value_type )
 @@ -882,8 +883,12 @@ hash_table
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
return _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 

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

[Richard Biener]

On Tue, May 21, 2019 at 1:02 PM Martin Liška  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š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 
> >> 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 , hashval_t hash);
> >>bool too_empty_p (unsigned int);
> >>void expand ();
> >>static bool is_deleted (value_type )
> >> @@ -882,8 +883,12 @@ hash_table
> >>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
> >>return _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, 

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

[Martin Liška]

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š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 
>> 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 , hashval_t hash);
>>bool too_empty_p (unsigned int);
>>void expand ();
>>static bool is_deleted (value_type )
>> @@ -882,8 +883,12 @@ hash_table
>>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
>>return _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 

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

[Richard Biener]

On Tue, May 21, 2019 at 12:07 AM Jeff Law  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 
>  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 , hashval_t hash);
> bool too_empty_p (unsigned int);
> void expand ();
> static bool is_deleted (value_type )
>  @@ -882,8 +883,12 @@ hash_table
> 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
> return _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?

Richard.

>
> Jeff

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

[Jeff Law]

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 
 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 , hashval_t hash);
bool too_empty_p (unsigned int);
void expand ();
static bool is_deleted (value_type )
 @@ -882,8 +883,12 @@ hash_table
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
return _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 :-)


Jeff

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

[Jason Merrill]

On 5/13/19 3: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 
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 , hashval_t hash);
bool too_empty_p (unsigned int);
void expand ();
static bool is_deleted (value_type )
@@ -882,8 +883,12 @@ hash_table
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
return _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
  ::verify (const compare_type , 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 = _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?


As a --param?  I use --param ggc-min-heapsize=0 --param ggc-min-expand=0 
to catch GC issues, and this seems similar.



+  /* FIXME: enable sanitization */


Please add PR numbers to these comments.


+#if ENABLE_EXTRA_CHECKING


The documentation describes this flag as


@samp{extra} adds for @samp{misc} checking extra checks that might affect
code generation and should therefore not differ between stage1 and later
stages.


which doesn't seem to apply here.

Jason

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

[Martin Liška]

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 
>>> 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 , hashval_t hash);
>>>bool too_empty_p (unsigned int);
>>>void expand ();
>>>static bool is_deleted (value_type )
>>> @@ -882,8 +883,12 @@ hash_table
>>>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
>>>return _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
 ::verify (const compare_type , 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 = _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 
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
@@ 

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

[Martin Liška]

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 
>> 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 , hashval_t hash);
>>bool too_empty_p (unsigned int);
>>void expand ();
>>static bool is_deleted (value_type )
>> @@ -882,8 +883,12 @@ hash_table
>>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
>>return _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

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
> 

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

[Jakub Jelinek]

On Wed, Nov 07, 2018 at 03:23:55PM -0700, Jeff Law wrote:
> > @@ -882,8 +883,12 @@ hash_table
> >if (insert == INSERT && m_size * 3 <= m_n_elements * 4)
> >  expand ();
> >  
> > -  m_searches++;
> > +#if ENABLE_EXTRA_CHECKING
> > +if (insert == INSERT)
> > +  verify (comparable, hash);
> > +#endif

Plus formatting, the above is indented too much.

Jakub

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

[Jeff Law]

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 
> 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 , hashval_t hash);
>bool too_empty_p (unsigned int);
>void expand ();
>static bool is_deleted (value_type )
> @@ -882,8 +883,12 @@ hash_table
>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
>return _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 :-)

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.

> +
> +/* Verify that all existing elements in th hash table which are
s/th/the/


Jeff

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

[Martin Liška]

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
> 

>From 0d9c979c845580a98767b83c099053d36eb49bb9 Mon Sep 17 00:00:00 2001
From: marxin 
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 , hashval_t hash);
   bool too_empty_p (unsigned int);
   void expand ();
   static bool is_deleted (value_type )
@@ -882,8 +883,12 @@ hash_table
   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
   return _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 ();
+}
+
+/* Verify that all existing elements in th hash table which are
+   equal to COMPARABLE have an equal HASH value provided as argument.  */
+
+template class Allocator>
+void
+hash_table
+::verify (const compare_type , hashval_t hash)
+{
+  for (size_t i = 0; i < m_size; i++)
+{
+  value_type *entry = _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. */
-- 
2.19.0

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

[Jakub Jelinek]

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
^^

> +  gcc_unreachable ();
> +}

Jakub

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

[Martin Liška]

On 10/29/18 2:53 PM, Alexander Monakov wrote:
> On Mon, 29 Oct 2018, Martin Liška wrote:
>> My question is whether we want to have in GCC 9 time frame or should I wait 
>> with that?
>> Does it worth implementing?
> 
> This is cool, thanks! A few questions/comments on the patch.

Hi.

Thanks for support.

> 
> I think there are places that use libiberty C-style hashtab (htab_t), would it
> make sense to have this kind of checking for them as well?  I think it's going
> to be more complicated though, no need to do both in one step.

Sure, that can be also added in the future!

> 
> I would recommend to factor out the error reporting path into a separate
> non-template function, e.g. hashtab_chk_error.  See how qsort_chk_error
> has "cold" and "noreturn" attributes and invokes problematic comparators:
> the idea was that a developer can 'break qsort_chk_error' in GDB and then
> easily step into broken comparators.

I did so.

> 
> Furthermore, it might be nice to investigate if the entire checking loop can
> be factored out somehow into a non-template function to avoid having multiple
> instantiations of it for different hashtable template parameters.

Note that Jakub is preferring to enable the checking only in non-release builds.
Thus I've guarded the checking with #if ENABLE_EXTRA_CHECKING. That said I won't
care much about saving a binary size.

The checking function is dependent on template arguments, so the avoidance is 
probably
not possible.

> 
> On my first attempt to submit qsort_chk, Richi asked how much it slows down
> stage2, do you have some data on the cost of this hashtable checking?

It does not survive bootstrap right now (too many cselib_find_slot failures).

> 
> I think it is possible to optimize this a bit: instead of checking on
> insertions, check on deletions and when destroying the table (with care to do
> n^2/2 rather than n^2 tests). Although, such approach would miss errors on
> hashtables that are never destroyed (leaked or deliberately not deleted).

Interesting idea. Benefit of the current approach is that the ICE is triggered
as soon as first bad element is inserted into a hast table. Which means it's 
easier
to debug. One problem with the destruction is that one equals function is 
defined on:
Descriptor::equal (Descriptor::value_type, Descriptor::compare_type). One can 
have
a table where these are not equal and so that equals can't be called.

Martin

> 
> Alexander
> 

>From 9afc42d324820cc0fc8a8a8b2d9cccd218734d10 Mon Sep 17 00:00:00 2001
From: marxin 
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..43adfac2dc0 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 , hashval_t hash);
   bool too_empty_p (unsigned int);
   void expand ();
   static bool is_deleted (value_type )
@@ -882,8 +883,12 @@ hash_table
   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
   return _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");
+  gcc_unreachable ();
+}
+
+/* Verify that all existing elements in th hash table which are
+   equal to COMPARABLE have an equal HASH value provided as argument.  */
+
+template class Allocator>
+void
+hash_table
+::verify (const compare_type , hashval_t hash)
+{
+  for (size_t i = 0; i < m_size; i++)
+{
+  value_type *entry = _entries[i];
+  if (!is_empty (*entry) && !is_deleted (*entry)
+	  && Descriptor::equal (*entry, comparable)
+	  && hash != Descriptor::hash (*entry))
+	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. */
-- 
2.19.0

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

[Alexander Monakov]

On Mon, 29 Oct 2018, Martin Liška wrote:
> My question is whether we want to have in GCC 9 time frame or should I wait 
> with that?
> Does it worth implementing?

This is cool, thanks! A few questions/comments on the patch.

I think there are places that use libiberty C-style hashtab (htab_t), would it
make sense to have this kind of checking for them as well?  I think it's going
to be more complicated though, no need to do both in one step.

I would recommend to factor out the error reporting path into a separate
non-template function, e.g. hashtab_chk_error.  See how qsort_chk_error
has "cold" and "noreturn" attributes and invokes problematic comparators:
the idea was that a developer can 'break qsort_chk_error' in GDB and then
easily step into broken comparators.

Furthermore, it might be nice to investigate if the entire checking loop can
be factored out somehow into a non-template function to avoid having multiple
instantiations of it for different hashtable template parameters.

On my first attempt to submit qsort_chk, Richi asked how much it slows down
stage2, do you have some data on the cost of this hashtable checking?

I think it is possible to optimize this a bit: instead of checking on
insertions, check on deletions and when destroying the table (with care to do
n^2/2 rather than n^2 tests). Although, such approach would miss errors on
hashtables that are never destroyed (leaked or deliberately not deleted).

Alexander

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

[Martin Liška]

Hi.

As slightly discussed here: 
https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01674.html I fixed
a situation where an equal operator of a hash table returns true, while 
corresponding hash
value of a pair of elements is different. That's inconsistent and can probably 
cause issues
in different areas of compiler.

I wrote a simple patch that verifies for a newly added element into a hash 
table that there's
no other equal element with a different hash. That's O(n) for each insert, so 
that it's only
enabled with -fchecking=3. Apart from that the way how it's enable is a bit 
cumbersome, but
that's caused by usage of hash-table also in generated files.

Anyway, there are first places where I see violation of the sanity check:

1) cselib_lookup_1:

$ cat ice.c
a() { b(); }

$ /dev/shm/objdir/gcc/xgcc -B/dev/shm/objdir/gcc/ ice.c -g -c -fchecking=3 -O
hash table checking failed: equal operator returns true for a pair of values 
with a different hash valueduring RTL pass: vartrack
ice.c:1:1: internal compiler error: in find_slot_with_hash, at hash-table.h:905
1 | a() { b(); }
  | ^
0x9680b5 hash_table::find_slot_with_hash(cselib_hasher::key* const&, unsigned int, 
insert_option)
/home/marxin/Programming/gcc/gcc/hash-table.h:905
0x962518 cselib_find_slot
/home/marxin/Programming/gcc/gcc/cselib.c:584
0x9625d4 cselib_lookup_1
/home/marxin/Programming/gcc/gcc/cselib.c:2097
0x9625d4 cselib_lookup(rtx_def*, machine_mode, int, machine_mode)
/home/marxin/Programming/gcc/gcc/cselib.c:2141
0x965ee7 cselib_record_sets
/home/marxin/Programming/gcc/gcc/cselib.c:2593
0x9670a9 cselib_process_insn(rtx_insn*)
/home/marxin/Programming/gcc/gcc/cselib.c:2790
0x1036b73 vt_initialize
/home/marxin/Programming/gcc/gcc/var-tracking.c:10231
0x103b98a variable_tracking_main_1
/home/marxin/Programming/gcc/gcc/var-tracking.c:10460
0x103b98a variable_tracking_main()
/home/marxin/Programming/gcc/gcc/var-tracking.c:10513

2) gfc_find_module

$ ./xgcc -B. 
/home/marxin/Programming/gcc/gcc/testsuite/gfortran.dg/coarray/alloc_comp_2.f90 
-fcoarray=single -fchecking=3
hash table checking failed: equal operator returns true for a pair of values 
with a different hash valuef951: internal compiler error: in 
find_slot_with_hash, at hash-table.h:905
0x8e5e86 hash_table::find_slot_with_hash(char 
const* const&, unsigned int, insert_option)
/home/marxin/Programming/gcc/gcc/hash-table.h:905
0x8e2c2c gfc_find_module(char const*)
/home/marxin/Programming/gcc/gcc/fortran/trans-decl.c:4865
0x8e4f42 gfc_generate_module_vars(gfc_namespace*)
/home/marxin/Programming/gcc/gcc/fortran/trans-decl.c:5475
0x8b8d7e gfc_generate_module_code(gfc_namespace*)
/home/marxin/Programming/gcc/gcc/fortran/trans.c:2190
0x868427 translate_all_program_units
/home/marxin/Programming/gcc/gcc/fortran/parse.c:6112
0x868427 gfc_parse_file()
/home/marxin/Programming/gcc/gcc/fortran/parse.c:6328
0x8b19cb gfc_be_parse_file
/home/marxin/Programming/gcc/gcc/fortran/f95-lang.c:204

3) lookup_template_class_1

$ ./xg++ -B. /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/template/ttp23.C 
-c -fchecking=3
hash table checking failed: equal operator returns true for a pair of values 
with a different hash 
value/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/template/ttp23.C: In 
instantiation of ‘struct B’:
/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/template/ttp23.C:15:8:   
required from here
/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/template/ttp23.C:8:17: 
internal compiler error: in find_slot_with_hash, at hash-table.h:905
8 | friend bool foo (const B& a);
  | ^~~
0xa265a4 hash_table::find_slot_with_hash(spec_entry* 
const&, unsigned int, insert_option)
/home/marxin/Programming/gcc/gcc/hash-table.h:905
0xa042ce lookup_template_class_1
/home/marxin/Programming/gcc/gcc/cp/pt.c:9629
0xa042ce lookup_template_class(tree_node*, tree_node*, tree_node*, tree_node*, 
int, int)
/home/marxin/Programming/gcc/gcc/cp/pt.c:9674
0xa03670 tsubst_aggr_type
/home/marxin/Programming/gcc/gcc/cp/pt.c:12679
0x9fefcd tsubst(tree_node*, tree_node*, int, tree_node*)
/home/marxin/Programming/gcc/gcc/cp/pt.c:14294
0x9fe1a9 tsubst(tree_node*, tree_node*, int, tree_node*)
/home/marxin/Programming/gcc/gcc/cp/pt.c:14285
0xa0d8bd tsubst_arg_types
/home/marxin/Programming/gcc/gcc/cp/pt.c:13891
0xa0dc24 tsubst_function_type
/home/marxin/Programming/gcc/gcc/cp/pt.c:14032
0x9fe790 tsubst(tree_node*, tree_node*, int, tree_node*)
/home/marxin/Programming/gcc/gcc/cp/pt.c:14769
0x9f2c7c tsubst_function_decl
/home/marxin/Programming/gcc/gcc/cp/pt.c:12921
0xa02d27 tsubst_template_decl
/home/marxin/Programming/gcc/gcc/cp/pt.c:13214
0x9f4416 tsubst_decl
/home/marxin/Programming/gcc/gcc/cp/pt.c:13316
0x9ff0ca tsubst(tree_node*, tree_node*, int, tree_node*)