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

[Martin Liška]

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?
Martin