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

[Martin Jambor]

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.


> 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.

Thanks,

Martin