New type-based pool allocator code miscompiled due to aliasing issue?

On 06/17/2015 01:29 PM, Richard Biener wrote:
> On Wed, Jun 17, 2015 at 1:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Wed, Jun 17, 2015 at 11:13:58AM +0200, Martin Jambor wrote:
>>>>>> Do you mean Richard following changes:
>>>>>>
>>>>>> alloc-pool.h (allocate):
>>>>>> ...
>>>>>> +  /* Placement new contructor.  */
>>>>>> +  inline void *operator new (size_t, elt_loc_list *&ptr)
>>>>>> +  {
>>>>>> +    return ptr;
>>>>>> +  }
>>>>>
>>>>> That should be there with including <new>
>>>>>
>>>>>> and e.g. cselib.h:
>>>>>>
>>>>>> struct cselib_val
>>>>>> {
>>>>>>   /* Pool allocation new operator.  */
>>>>>>   inline void *operator new (size_t)
>>>>>>   {
>>>>>>     return pool.allocate ();
>>>>>>   }
>>>>>>
>>>>>>   /* Placement new contructor.  */
>>>>>>   inline void *operator new (size_t, char *&ptr)
>>>>>>   {
>>>>>>     return ptr;
>>>>>>   }
>>>>>
>>>>> Yes, though I wonder whether cselib_val should really have undefined
>>>>> contents after
>>>>> allocating it?  (or does the pool allocator zero the memory?)
>>
>> IMHO if you want to put placement new into allocate, then you probably need
>> two different methods, one that will return you just void * to the
>> allocated memory chunk that you should use
>> inline void *operator new (size_t) { return pool.allocate (); } on,
>> and another method that will use that and additionally invoke a placement
>> new on it and return you the constructed pointer, which you'd use elsewhere.
>> Otherwise you'd construct the object twice...
> 
> Note that in the case of cselib_val a
> 
>   new cselib_val;
> 
> will already invoke the constructor.  I think it's just too much
> (partly) C++-ification here.
> Either all alloc-pool users have to work that way or none.
> 
> Richard.
> 
>>         Jakub

Hello.

You are right that we should call ::new just for classes that have m_ignore_type_size == false.
I've come up with following patch, that I tested slightly:


Would it be suitable?
Martin

On Wed, Jun 17, 2015 at 4:44 PM, Martin Liška <mliska@suse.cz> wrote:
> On 06/17/2015 01:29 PM, Richard Biener wrote:
>> On Wed, Jun 17, 2015 at 1:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> On Wed, Jun 17, 2015 at 11:13:58AM +0200, Martin Jambor wrote:
>>>>>>> Do you mean Richard following changes:
>>>>>>>
>>>>>>> alloc-pool.h (allocate):
>>>>>>> ...
>>>>>>> +  /* Placement new contructor.  */
>>>>>>> +  inline void *operator new (size_t, elt_loc_list *&ptr)
>>>>>>> +  {
>>>>>>> +    return ptr;
>>>>>>> +  }
>>>>>>
>>>>>> That should be there with including <new>
>>>>>>
>>>>>>> and e.g. cselib.h:
>>>>>>>
>>>>>>> struct cselib_val
>>>>>>> {
>>>>>>>   /* Pool allocation new operator.  */
>>>>>>>   inline void *operator new (size_t)
>>>>>>>   {
>>>>>>>     return pool.allocate ();
>>>>>>>   }
>>>>>>>
>>>>>>>   /* Placement new contructor.  */
>>>>>>>   inline void *operator new (size_t, char *&ptr)
>>>>>>>   {
>>>>>>>     return ptr;
>>>>>>>   }
>>>>>>
>>>>>> Yes, though I wonder whether cselib_val should really have undefined
>>>>>> contents after
>>>>>> allocating it?  (or does the pool allocator zero the memory?)
>>>
>>> IMHO if you want to put placement new into allocate, then you probably need
>>> two different methods, one that will return you just void * to the
>>> allocated memory chunk that you should use
>>> inline void *operator new (size_t) { return pool.allocate (); } on,
>>> and another method that will use that and additionally invoke a placement
>>> new on it and return you the constructed pointer, which you'd use elsewhere.
>>> Otherwise you'd construct the object twice...
>>
>> Note that in the case of cselib_val a
>>
>>   new cselib_val;
>>
>> will already invoke the constructor.  I think it's just too much
>> (partly) C++-ification here.
>> Either all alloc-pool users have to work that way or none.
>>
>> Richard.
>>
>>>         Jakub
>
> Hello.
>
> You are right that we should call ::new just for classes that have m_ignore_type_size == false.
> I've come up with following patch, that I tested slightly:
>
> diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
> index 1785df5..7da5f7a 100644
> --- a/gcc/alloc-pool.h
> +++ b/gcc/alloc-pool.h
> @@ -412,8 +412,16 @@ pool_allocator<T>::allocate ()
>  #endif
>    VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size));
>
> +  T *ptr = (T *)header;
> +
>    /* Call default constructor.  */
> -  return (T *)(header);
> +  if (!m_ignore_type_size)
> +    {
> +      memset (header + sizeof (T), 0, m_extra_size);
> +      return ::new (ptr) T;
> +    }
> +  else
> +    return ptr;
>  }
>
>  /* Puts PTR back on POOL's free list.  */
>
> Would it be suitable?

Suitable with the memset removed, yes.

Thanks,
Richard.

> Martin

On 06/18/2015 06:10 AM, Richard Biener wrote:
>> You are right that we should call ::new just for classes that have m_ignore_type_size == false.
>> >I've come up with following patch, that I tested slightly:
>> >
>> >diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
>> >index 1785df5..7da5f7a 100644
>> >--- a/gcc/alloc-pool.h
>> >+++ b/gcc/alloc-pool.h
>> >@@ -412,8 +412,16 @@ pool_allocator<T>::allocate ()
>> >  #endif
>> >    VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size));
>> >
>> >+  T *ptr = (T *)header;
>> >+
>> >    /* Call default constructor.  */
>> >-  return (T *)(header);
>> >+  if (!m_ignore_type_size)
>> >+    {
>> >+      memset (header + sizeof (T), 0, m_extra_size);
>> >+      return ::new (ptr) T;
>> >+    }
>> >+  else
>> >+    return ptr;
>> >  }
>> >
>> >  /* Puts PTR back on POOL's free list.  */
>> >
>> >Would it be suitable?
> Suitable with the memset removed, yes.
What's the status of this patch? I have a couple spec regression testers 
that have been unable to build GCC due to this issue, specifically the 
sched-deps.c change. The above patch (with memset removed) does result 
in a successful build.

Thanks,
Pat

On 06/23/2015 09:44 PM, Pat Haugen wrote:
> On 06/18/2015 06:10 AM, Richard Biener wrote:
>>> You are right that we should call ::new just for classes that have m_ignore_type_size == false.
>>> >I've come up with following patch, that I tested slightly:
>>> >
>>> >diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
>>> >index 1785df5..7da5f7a 100644
>>> >--- a/gcc/alloc-pool.h
>>> >+++ b/gcc/alloc-pool.h
>>> >@@ -412,8 +412,16 @@ pool_allocator<T>::allocate ()
>>> >  #endif
>>> >    VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size));
>>> >
>>> >+  T *ptr = (T *)header;
>>> >+
>>> >    /* Call default constructor.  */
>>> >-  return (T *)(header);
>>> >+  if (!m_ignore_type_size)
>>> >+    {
>>> >+      memset (header + sizeof (T), 0, m_extra_size);
>>> >+      return ::new (ptr) T;
>>> >+    }
>>> >+  else
>>> >+    return ptr;
>>> >  }
>>> >
>>> >  /* Puts PTR back on POOL's free list.  */
>>> >
>>> >Would it be suitable?
>> Suitable with the memset removed, yes.
> What's the status of this patch? I have a couple spec regression testers that have been unable to build GCC due to this issue, specifically the sched-deps.c change. The above patch (with memset removed) does result in a successful build.
>
> Thanks,
> Pat
>

Hello.

I've finishing a new patch that will do the job in more suitable way.

Martin