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

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Jun 17, 2015 at 11:13 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Tue, Jun 16, 2015 at 05:31:57PM +0200, Martin Liska wrote:
>> On 06/16/2015 04:02 PM, Richard Biener wrote:
>> > On Tue, Jun 16, 2015 at 3:38 PM, Martin LiÅka <mliska@suse.cz> 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?)
>> >
>> > Richard.
>>
>> Hio.
>>
>> I've added calling of placement new operators and memset a memory, look the patch
>> works for me.
>>
>> If it's the right way, I'll write Changelog and run testsuite.
>
> So do I understand this thread correctly that any clas that overrides
> its new operator to allocate from a pool also has to override its
> placement new operator (to just return the argument)?  (I'm thinking
> of doing the same in the HSA branch.) If so, I think it would warrant
> at least a simple comment before the pool allocator class in
> alloc-pool.h.

Well, it applies to all classes overriding new that eventually get placement new
called on them.

And using the class-specific new is always correct (even though in the cselib.h
case it's somewhat weird).  For cselib.h the placement new operator can just
call placement ::new

>
>
>> From d60bc8fa02161df64ddbb6bdb35c733af5e073c6 Mon Sep 17 00:00:00 2001
>> From: mliska <mliska@suse.cz>
>> Date: Tue, 16 Jun 2015 17:28:27 +0200
>> Subject: [PATCH] Add placement new for classes in pool allocator.
>>
>> ---
>>  gcc/alloc-pool.h   | 10 +++++++++-
>>  gcc/asan.c         |  6 ++++++
>>  gcc/cselib.c       |  6 ++++++
>>  gcc/cselib.h       | 15 +++++++++++++++
>>  gcc/dse.c          | 30 ++++++++++++++++++++++++++++++
>>  gcc/et-forest.c    |  6 ++++++
>>  gcc/et-forest.h    |  8 ++++++++
>>  gcc/ira-color.c    |  6 ++++++
>>  gcc/lra-int.h      | 18 ++++++++++++++++++
>>  gcc/regcprop.c     |  6 ++++++
>>  gcc/tree-sra.c     | 12 ++++++++++++
>>  gcc/var-tracking.c | 21 +++++++++++++++++++++
>>  12 files changed, 143 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
>> index 1785df5..237ece3 100644
>> --- a/gcc/alloc-pool.h
>> +++ b/gcc/alloc-pool.h
>> @@ -413,7 +413,15 @@ pool_allocator<T>::allocate ()
>>    VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size));
>>
>>    /* Call default constructor.  */
>> -  return (T *)(header);
>> +  T *ptr = (T *)header;
>> +
>> +  if (!m_ignore_type_size)
>> +    {
>> +      memset (header, 0, sizeof (T));
>> +      return new (ptr) T ();
>> +    }
>> +  else
>> +    return ptr;
>>  }
>
> Interesting, does this mean that the allocator will clear the memory
> for all types?  I thought the initialization of data should be left to
> the class itself and its constructor.  On the other hand, I suppose
> that memset(this, 0, sizeof(*this)) is a very bad idea in C++, so the
> simplicity of this might actually justify the overhead in cases where
> we don't need the zeroing.

It really depends on what kind of memory is allocated by alloc-pool.
I see it has this strange ignore_type_size and extra_size so I suppose
just memset (header, 0, size) would be appropriate.  Not for class
types though where you really need to call placement new on.

Did you audit alloc-pool users whether they set ignore_type_size?
I suppose we'd like to do

   memset (header + sizeof (T), 0, m_extra_size);
   return new (ptr) T ();

but of course that only works when ignore_type_size is never true
and m_extra_size only accounts for the extra trailing array space.
The value_pool allocator in cselib.c is the only offender here it seems.

And it looks like callers are not expecting any initialization from the
allocator (looking at the old implementation).  So I think you
want even

   return new (ptr) T;

so not value-initialize to preserve that old behavior.

Richard.

>
> Thanks,
>
> Martin