[PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 06/29/2017 04:34 PM, Jan Hubicka wrote:
>>
>> The warning in the original code could have been suppressed (by
>> casting the pointer to char*), but it was valid so I opted not
>> to.  I'd expect it to be possible to work around the bug but
>> I don't have easy access to GCC 4.2 to reproduce it or verify
>> the fix.
>>
>> FWIW, after looking at the function again, I wondered if zeroing
>> out the elements (either way) was the right thing to do and if
>> they shouldn't be cleared by calling Descriptor::mark_empty()
>> instead, like in alloc_entries(), but making that change broke
>> a bunch of ipa/ipa-pta-*.c tests.  It's not really clear to me
>> what this code is supposed to do.
>
> Well, it is a standard hash table.

What I mean is that while the true branch of the if (nsize != size)
statement in hash_table::empty_slow () frees the table and then
allocates a new one which marks each element in the newly allocated
storage as empty by calling mark_empty() (i.e., Descriptor::
mark_empty()), while the false branch bypasses mark_empty() and
calls memset(entries, 0, ...) instead.  The two are equivalent
only when Descriptor::mark_empty(value_type &e) sets e to all
zeros.  They are not equivalent when Descriptor::mark_empty()
does something else.  When value_type is a pointer they should
be the equivalent (so long as a null pointer is all zeros), but
it's not obvious to me what value_type is in your case (tree?)

In any case, calling memset bypasses the (undocumented)
customization point (Descriptor::mark_empty).  Calling it
conditionally depending on the size of the hash table seems
suspicious (as does setting *entry = value_type();).  If there
is a hash table instance where mark_empty is not equivalent to
memset(entries, 0, ...) it could mean a bug.

Looking at some other definitions of mark_empty I see int_hash
defines it this way:

   template <typename Type, Type Empty, Type Deleted>
   inline void
   int_hash <Type, Empty, Deleted>::mark_empty (Type &x)
   {
     x = Empty;
   }

So for hash_tables with int value_types the memset is only
equivalent to calling mark_empty when Empty is zero.  There
are instances of hash tables in GCC like alias_set_hash and
uid_hash where Empty is non-zero.  I wonder what happens when
hash_table::empty_slow() is called on one of these instances
with a small number of elements and that instance is used
again.

> The problem I hit was
> lookup_with_hash walking infinitly around the hash table because
> all elements seemed used.
> is_empty is defined as:
> template <typename Type>
> inline bool
> pointer_hash <Type>::is_empty (Type *e)
> {
>   return e == NULL;
> }
>
> and mark_empty as
> template <typename Type>
> inline void
> pointer_hash <Type>::mark_empty (Type *&e)
> {
>   e = NULL;
> }

It sounds like value_type is a pointer in your case and
the assignment '*entry = value_type();' isn't clearing the *entry.
I vaguely remember coming across a bug like that years ago but I
don't know if that was GCC or some other compiler.  I tried
compiling a small test case with a few GCC revisions close to
4.2 (r117926 and some others) but couldn't reproduce anything
unexpected.

> I guess they are supposed to be definable to other implementations
> but then the former memset code would break.

Right, that's my concern.

Martin

PS the below is just a shot in the dark. I'd be surprised if it
actually did something for you, but with little else to go on
it's worth a try.

>> Martin
>>
>> PS Does this help at all?
>>
>> @@ -804,8 +804,8 @@ hash_table<Descriptor, Allocator>::empty_slow ()
>>      }
>>    else
>>      {
>> -      for ( ; size; ++entries, --size)
>> -       *entries = value_type ();
>> +      for (size_t i = 0; i != size; ++i)
>> +       entries[i] = value_type ();
>
> I can give it a try tomorrow.  Still wonder what goes wrong with ctors with 4.2 (and 4.3 as well apparently)
>
> Honza
>>      }
>>    m_n_deleted = 0;
>>    m_n_elements = 0;