diff mbox

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

Message ID CAFiYyc1qMQd4PPU+-v+XMpH9iDOF_oJj=bX6Ln3+SaOkjhYVsA@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener June 30, 2017, 8:34 a.m. UTC
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:

Comments

Martin Sebor June 30, 2017, 2:29 p.m. UTC | #1
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;
>
diff mbox

Patch

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;