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

[Martin Sebor <msebor@gmail.com>]

On 06/30/2017 02:34 AM, Richard Biener wrote:
> On Thu, Jun 29, 2017 at 10:23 PM, Martin Sebor <msebor@gmail.com> wrote:
>> On 06/29/2017 10:15 AM, Jan Hubicka wrote:
>>>
>>> Hello,
>>>>
>>>> diff --git a/gcc/hash-table.h b/gcc/hash-table.h
>>>> index 0f7e21a..443d16c 100644
>>>> --- a/gcc/hash-table.h
>>>> +++ b/gcc/hash-table.h
>>>> @@ -803,7 +803,10 @@ hash_table<Descriptor, Allocator>::empty_slow ()
>>>>        m_size_prime_index = nindex;
>>>>      }
>>>>    else
>>>> -    memset (entries, 0, size * sizeof (value_type));
>>>> +    {
>>>> +      for ( ; size; ++entries, --size)
>>>> +       *entries = value_type ();
>>>> +    }
>>>>    m_n_deleted = 0;
>>>>    m_n_elements = 0;
>>>>  }
>>>
>>>
>>> This change sends our periodic testers into an infinite loop.  It is fault
>>> of gcc 4.2 being used
>>> as bootstrap compiler, but perhaps that can be worked around?
>>
>>
>> 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.
>>
>> 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 ();
>>
>>      }
>>    m_n_deleted = 0;
>>    m_n_elements = 0;
>
> alloc_entries uses mark_empty.  untested:

Right.  That was my initial thought as well but it broke a number
of ipa/ipa-pta-*.c tests.  I didn't have time to investigate why.

Martin

>
> Index: gcc/hash-table.h
> ===================================================================
> --- gcc/hash-table.h    (revision 249780)
> +++ gcc/hash-table.h    (working copy)
> @@ -804,8 +804,8 @@ hash_table<Descriptor, Allocator>::empty
>      }
>    else
>      {
> -      for ( ; size; ++entries, --size)
> -       *entries = value_type ();
> +      for (size_t i = 0; i < size; ++i)
> +       mark_empty (entries[i]);
>      }
>    m_n_deleted = 0;
>    m_n_elements = 0;
>