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

[Richard Biener]

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