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

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

[Ulrich Weigand]

Hello,

since the new type-based pool allocator was merged, the SPU toolchain
automated build / regression test has been failing due to crashes of
the compiled GCC due to random memory corruption.

Debugging this, it seems this is caused by GCC being miscompiled by
the host compiler.  As one example, var-tracking.c contains this
code in vt_initialize:

  empty_shared_hash = new shared_hash_def;
  empty_shared_hash->refcount = 1;

Via operator new, these bits of alloc-pool.h are inlined:

template <typename T>
inline T *
pool_allocator<T>::allocate ()
{
[...]
  /* Pull the first free element from the free list, and return it.  */
  header = m_returned_free_list;
  VALGRIND_DISCARD (VALGRIND_MAKE_MEM_DEFINED (header, sizeof (*header)));
  m_returned_free_list = header->next;
  m_elts_free--;
[...]
  /* Call default constructor.  */
  return (T *)(header);
}

Note that the read from "header->next" accesses the same memory location
as the write to "empty_shared_hash->refcount" above.

Type-based aliasing now assumes that those accesses cannot alias, since
they use incompatible types.  The scheduler then moves the write to
before the read, which causes m_returned_free_list to be set to 1,
causing a crash later on.

Now -- I'm only seeing this effect on my SPU toolchain regression test
machine, which uses a RHEL 5 distribution with its default GCC 4.1.2
system compiler.  (Using -fno-strict-aliasing fixes the symptom.)

Maybe this old compiler simply has a bug in type-based aliasing.  But
on the other hand, I'm not completely sure whether this *is* a bug --
is the code above supposed to be valid C++, or does it in fact violate
the aliasing rules after all?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

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

[pinskia]

> On Jun 12, 2015, at 12:51 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> 
> Hello,
> 
> since the new type-based pool allocator was merged, the SPU toolchain
> automated build / regression test has been failing due to crashes of
> the compiled GCC due to random memory corruption.
> 
> Debugging this, it seems this is caused by GCC being miscompiled by
> the host compiler.  As one example, var-tracking.c contains this
> code in vt_initialize:
> 
>  empty_shared_hash = new shared_hash_def;
>  empty_shared_hash->refcount = 1;
> 
> Via operator new, these bits of alloc-pool.h are inlined:
> 
> template <typename T>
> inline T *
> pool_allocator<T>::allocate ()
> {
> [...]
>  /* Pull the first free element from the free list, and return it.  */
>  header = m_returned_free_list;
>  VALGRIND_DISCARD (VALGRIND_MAKE_MEM_DEFINED (header, sizeof (*header)));
>  m_returned_free_list = header->next;
>  m_elts_free--;
> [...]
>  /* Call default constructor.  */
>  return (T *)(header);
> }
> 
> Note that the read from "header->next" accesses the same memory location
> as the write to "empty_shared_hash->refcount" above.
> 
> Type-based aliasing now assumes that those accesses cannot alias, since
> they use incompatible types.  The scheduler then moves the write to
> before the read, which causes m_returned_free_list to be set to 1,
> causing a crash later on.
> 
> Now -- I'm only seeing this effect on my SPU toolchain regression test
> machine, which uses a RHEL 5 distribution with its default GCC 4.1.2
> system compiler.  (Using -fno-strict-aliasing fixes the symptom.)
> 
> Maybe this old compiler simply has a bug in type-based aliasing.  But
> on the other hand, I'm not completely sure whether this *is* a bug --
> is the code above supposed to be valid C++, or does it in fact violate
> the aliasing rules after all?

This is just a bug in the older compiler. There was a change to fix in placement new operator. I can't find the reference right now but this is the same issue as that. 

Thanks,
Andrew


> 
> Bye,
> Ulrich
> 
> -- 
>  Dr. Ulrich Weigand
>  GNU/Linux compilers and toolchain
>  Ulrich.Weigand@de.ibm.com
> 

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

[Jakub Jelinek]

On Fri, Jun 12, 2015 at 12:58:12AM +0800, pinskia@gmail.com wrote:
> This is just a bug in the older compiler. There was a change to fix in
> placement new operator.  I can't find the reference right now but this is
> the same issue as that.

I'm not claiming 4.1 is aliasing bug free, there are various known issues in
it.  But, is that the case here?

  empty_var = onepart_pool (onepart).allocate ();
  empty_var->dv = dv;
  empty_var->refcount = 1;
  empty_var->n_var_parts = 0;

doesn't really seem to use operator new at all, so I'd say the bug is in
all the spots that call allocate () method of the pool, but don't really
use operator new.

	Jakub

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

[Richard Biener]

On June 11, 2015 7:50:36 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Fri, Jun 12, 2015 at 12:58:12AM +0800, pinskia@gmail.com wrote:
>> This is just a bug in the older compiler. There was a change to fix
>in
>> placement new operator.  I can't find the reference right now but
>this is
>> the same issue as that.
>
>I'm not claiming 4.1 is aliasing bug free, there are various known
>issues in
>it.  But, is that the case here?
>
>  empty_var = onepart_pool (onepart).allocate ();
>  empty_var->dv = dv;
>  empty_var->refcount = 1;
>  empty_var->n_var_parts = 0;
>
>doesn't really seem to use operator new at all, so I'd say the bug is
>in
>all the spots that call allocate () method of the pool, but don't
>really
>use operator new.

Yeah.  BTW, I see the same issue on x86_64 and on ia64 with a gcc 4.1 host compiler.  I think allocate itself should use placement new, not just a static pointer conversion.

Richard.

>	Jakub

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

[Martin Liška]

On 06/11/2015 08:19 PM, Richard Biener wrote:
> On June 11, 2015 7:50:36 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Fri, Jun 12, 2015 at 12:58:12AM +0800, pinskia@gmail.com wrote:
>>> This is just a bug in the older compiler. There was a change to fix
>> in
>>> placement new operator.  I can't find the reference right now but
>> this is
>>> the same issue as that.
>>
>> I'm not claiming 4.1 is aliasing bug free, there are various known
>> issues in
>> it.  But, is that the case here?
>>
>>  empty_var = onepart_pool (onepart).allocate ();
>>  empty_var->dv = dv;
>>  empty_var->refcount = 1;
>>  empty_var->n_var_parts = 0;
>>
>> doesn't really seem to use operator new at all, so I'd say the bug is
>> in
>> all the spots that call allocate () method of the pool, but don't
>> really
>> use operator new.
> 
> Yeah.  BTW, I see the same issue on x86_64 and on ia64 with a gcc 4.1 host compiler.  I think allocate itself should use placement new, not just a static pointer conversion.
> 
> Richard.

Hi.

What do you mean by calling placement new? Currently pool_allocator<T>::allocate calls placement new as a last statement in the function:

  return (T *)(header);

Martin

> 
>> 	Jakub
> 
> 

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

[Andrew Pinski]

On Mon, Jun 15, 2015 at 2:09 AM, Martin Liška <mliska@suse.cz> wrote:
> On 06/11/2015 08:19 PM, Richard Biener wrote:
>> On June 11, 2015 7:50:36 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>>> On Fri, Jun 12, 2015 at 12:58:12AM +0800, pinskia@gmail.com wrote:
>>>> This is just a bug in the older compiler. There was a change to fix
>>> in
>>>> placement new operator.  I can't find the reference right now but
>>> this is
>>>> the same issue as that.
>>>
>>> I'm not claiming 4.1 is aliasing bug free, there are various known
>>> issues in
>>> it.  But, is that the case here?
>>>
>>>  empty_var = onepart_pool (onepart).allocate ();
>>>  empty_var->dv = dv;
>>>  empty_var->refcount = 1;
>>>  empty_var->n_var_parts = 0;
>>>
>>> doesn't really seem to use operator new at all, so I'd say the bug is
>>> in
>>> all the spots that call allocate () method of the pool, but don't
>>> really
>>> use operator new.
>>
>> Yeah.  BTW, I see the same issue on x86_64 and on ia64 with a gcc 4.1 host compiler.  I think allocate itself should use placement new, not just a static pointer conversion.
>>
>> Richard.
>
> Hi.
>
> What do you mean by calling placement new? Currently pool_allocator<T>::allocate calls placement new as a last statement in the function:
>
>   return (T *)(header);

That is only a cast and not a placement new.
Try this instead:
  return new(header) T();

Thanks,
Andrew

>
> Martin
>
>>
>>>      Jakub
>>
>>
>

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

[Martin Liška]

On 06/15/2015 11:13 AM, Andrew Pinski wrote:
> On Mon, Jun 15, 2015 at 2:09 AM, Martin Liška <mliska@suse.cz> wrote:
>> On 06/11/2015 08:19 PM, Richard Biener wrote:
>>> On June 11, 2015 7:50:36 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>>>> On Fri, Jun 12, 2015 at 12:58:12AM +0800, pinskia@gmail.com wrote:
>>>>> This is just a bug in the older compiler. There was a change to fix
>>>> in
>>>>> placement new operator.  I can't find the reference right now but
>>>> this is
>>>>> the same issue as that.
>>>>
>>>> I'm not claiming 4.1 is aliasing bug free, there are various known
>>>> issues in
>>>> it.  But, is that the case here?
>>>>
>>>>  empty_var = onepart_pool (onepart).allocate ();
>>>>  empty_var->dv = dv;
>>>>  empty_var->refcount = 1;
>>>>  empty_var->n_var_parts = 0;
>>>>
>>>> doesn't really seem to use operator new at all, so I'd say the bug is
>>>> in
>>>> all the spots that call allocate () method of the pool, but don't
>>>> really
>>>> use operator new.
>>>
>>> Yeah.  BTW, I see the same issue on x86_64 and on ia64 with a gcc 4.1 host compiler.  I think allocate itself should use placement new, not just a static pointer conversion.
>>>
>>> Richard.
>>
>> Hi.
>>
>> What do you mean by calling placement new? Currently pool_allocator<T>::allocate calls placement new as a last statement in the function:
>>
>>   return (T *)(header);
> 
> That is only a cast and not a placement new.
> Try this instead:
>   return new(header) T();

Ah, I overlooked that it's not a placement new, but just static casting.
Anyway, if I added:

cselib_val () {}

to struct cselib_val and changed the cast to placement new:
  char *ptr = (char *) header;
  return new (ptr) T ();

I got following compilation error:

In file included from ../../gcc/alias.c:46:0:
../../gcc/alloc-pool.h: In instantiation of ‘T* pool_allocator<T>::allocate() [with T = cselib_val]’:
../../gcc/cselib.h:51:27:   required from here
../../gcc/alloc-pool.h:416:23: error: no matching function for call to ‘cselib_val::operator new(sizetype, char*&)’
   return new (ptr) T ();
                       ^
In file included from ../../gcc/alias.c:47:0:
../../gcc/cselib.h:49:16: note: candidate: static void* cselib_val::operator new(size_t)
   inline void *operator new (size_t)
                ^
../../gcc/cselib.h:49:16: note:   candidate expects 1 argument, 2 provided

I am wondering if I can combine overwritten new operator, which is going to internally use placement new with a default
ctor?

Martin



> 
> Thanks,
> Andrew
> 
>>
>> Martin
>>
>>>
>>>>      Jakub
>>>
>>>
>>

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

[Marc Glisse]

On Mon, 15 Jun 2015, Martin Liška wrote:

> Ah, I overlooked that it's not a placement new, but just static casting.
> Anyway, if I added:
>
> cselib_val () {}
>
> to struct cselib_val and changed the cast to placement new:
>  char *ptr = (char *) header;
>  return new (ptr) T ();
>
> I got following compilation error:
>
> In file included from ../../gcc/alias.c:46:0:
> ../../gcc/alloc-pool.h: In instantiation of ‘T* pool_allocator<T>::allocate() [with T = cselib_val]’:
> ../../gcc/cselib.h:51:27:   required from here
> ../../gcc/alloc-pool.h:416:23: error: no matching function for call to ‘cselib_val::operator new(sizetype, char*&)’
>   return new (ptr) T ();
>                       ^
> In file included from ../../gcc/alias.c:47:0:
> ../../gcc/cselib.h:49:16: note: candidate: static void* cselib_val::operator new(size_t)
>   inline void *operator new (size_t)
>                ^
> ../../gcc/cselib.h:49:16: note:   candidate expects 1 argument, 2 provided

#include <new>

-- 
Marc Glisse

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

[Richard Biener]

On June 15, 2015 7:31:37 PM GMT+02:00, Marc Glisse <marc.glisse@inria.fr> wrote:
>On Mon, 15 Jun 2015, Martin Liška wrote:
>
>> Ah, I overlooked that it's not a placement new, but just static
>casting.
>> Anyway, if I added:
>>
>> cselib_val () {}
>>
>> to struct cselib_val and changed the cast to placement new:
>>  char *ptr = (char *) header;
>>  return new (ptr) T ();
>>
>> I got following compilation error:
>>
>> In file included from ../../gcc/alias.c:46:0:
>> ../../gcc/alloc-pool.h: In instantiation of ‘T*
>pool_allocator<T>::allocate() [with T = cselib_val]’:
>> ../../gcc/cselib.h:51:27:   required from here
>> ../../gcc/alloc-pool.h:416:23: error: no matching function for call
>to ‘cselib_val::operator new(sizetype, char*&)’
>>   return new (ptr) T ();
>>                       ^
>> In file included from ../../gcc/alias.c:47:0:
>> ../../gcc/cselib.h:49:16: note: candidate: static void*
>cselib_val::operator new(size_t)
>>   inline void *operator new (size_t)
>>                ^
>> ../../gcc/cselib.h:49:16: note:   candidate expects 1 argument, 2
>provided
>
>#include <new>

Please include that from system.h

Richard.

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

On 06/15/2015 07:31 PM, Marc Glisse wrote:
> On Mon, 15 Jun 2015, Martin Liška wrote:
> 
>> Ah, I overlooked that it's not a placement new, but just static casting.
>> Anyway, if I added:
>>
>> cselib_val () {}
>>
>> to struct cselib_val and changed the cast to placement new:
>>  char *ptr = (char *) header;
>>  return new (ptr) T ();
>>
>> I got following compilation error:
>>
>> In file included from ../../gcc/alias.c:46:0:
>> ../../gcc/alloc-pool.h: In instantiation of ‘T* pool_allocator<T>::allocate() [with T = cselib_val]’:
>> ../../gcc/cselib.h:51:27:   required from here
>> ../../gcc/alloc-pool.h:416:23: error: no matching function for call to ‘cselib_val::operator new(sizetype, char*&)’
>>   return new (ptr) T ();
>>                       ^
>> In file included from ../../gcc/alias.c:47:0:
>> ../../gcc/cselib.h:49:16: note: candidate: static void* cselib_val::operator new(size_t)
>>   inline void *operator new (size_t)
>>                ^
>> ../../gcc/cselib.h:49:16: note:   candidate expects 1 argument, 2 provided
> 
> #include <new>
> 

Hi.

<new> header file is not missing (explicit addition of the file does not help).
Feel free to play with following patch which should fix cselib.h compilation error.

Thanks,
Martin

[-- Attachment #2: pp.patch --]
[-- Type: text/x-patch, Size: 992 bytes --]

diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
index 1785df5..4b4bc56 100644
--- a/gcc/alloc-pool.h
+++ b/gcc/alloc-pool.h
@@ -413,7 +413,8 @@ pool_allocator<T>::allocate ()
   VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size));
 
   /* Call default constructor.  */
-  return (T *)(header);
+  char *ptr = (char *) header;
+  return new (ptr) T ();
 }
 
 /* Puts PTR back on POOL's free list.  */
diff --git a/gcc/cselib.h b/gcc/cselib.h
index cdd06ad..a0da27f 100644
--- a/gcc/cselib.h
+++ b/gcc/cselib.h
@@ -42,6 +42,9 @@ struct cselib_val
 
   struct cselib_val *next_containing_mem;
 
+  /* Default constructor.  */
+  cselib_val () {}
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
@@ -67,6 +70,9 @@ struct elt_loc_list {
   /* The insn that made the equivalence.  */
   rtx_insn *setting_insn;
 
+  /* Default constructor.  */
+  elt_loc_list () {}
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {

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

[Richard Biener]

On Tue, Jun 16, 2015 at 11:26 AM, Martin Liška <mliska@suse.cz> wrote:
> On 06/15/2015 07:31 PM, Marc Glisse wrote:
>> On Mon, 15 Jun 2015, Martin Liška wrote:
>>
>>> Ah, I overlooked that it's not a placement new, but just static casting.
>>> Anyway, if I added:
>>>
>>> cselib_val () {}
>>>
>>> to struct cselib_val and changed the cast to placement new:
>>>  char *ptr = (char *) header;
>>>  return new (ptr) T ();
>>>
>>> I got following compilation error:
>>>
>>> In file included from ../../gcc/alias.c:46:0:
>>> ../../gcc/alloc-pool.h: In instantiation of ‘T* pool_allocator<T>::allocate() [with T = cselib_val]’:
>>> ../../gcc/cselib.h:51:27:   required from here
>>> ../../gcc/alloc-pool.h:416:23: error: no matching function for call to ‘cselib_val::operator new(sizetype, char*&)’
>>>   return new (ptr) T ();
>>>                       ^
>>> In file included from ../../gcc/alias.c:47:0:
>>> ../../gcc/cselib.h:49:16: note: candidate: static void* cselib_val::operator new(size_t)
>>>   inline void *operator new (size_t)
>>>                ^
>>> ../../gcc/cselib.h:49:16: note:   candidate expects 1 argument, 2 provided
>>
>> #include <new>
>>
>
> Hi.
>
> <new> header file is not missing (explicit addition of the file does not help).
> Feel free to play with following patch which should fix cselib.h compilation error.

cselib_val overrides the new operator but fails to provide an overload
for the placement new
form.  Fix that and it should work (of course it gets quite awkward
with its 'new' calling
pool.allocate and its placement new doing value-construction then...)

Richard.

> Thanks,
> Martin

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

[Martin Liška]

On 06/16/2015 03:17 PM, Richard Biener wrote:
> On Tue, Jun 16, 2015 at 11:26 AM, Martin Liška <mliska@suse.cz> wrote:
>> On 06/15/2015 07:31 PM, Marc Glisse wrote:
>>> On Mon, 15 Jun 2015, Martin Liška wrote:
>>>
>>>> Ah, I overlooked that it's not a placement new, but just static casting.
>>>> Anyway, if I added:
>>>>
>>>> cselib_val () {}
>>>>
>>>> to struct cselib_val and changed the cast to placement new:
>>>>  char *ptr = (char *) header;
>>>>  return new (ptr) T ();
>>>>
>>>> I got following compilation error:
>>>>
>>>> In file included from ../../gcc/alias.c:46:0:
>>>> ../../gcc/alloc-pool.h: In instantiation of ‘T* pool_allocator<T>::allocate() [with T = cselib_val]’:
>>>> ../../gcc/cselib.h:51:27:   required from here
>>>> ../../gcc/alloc-pool.h:416:23: error: no matching function for call to ‘cselib_val::operator new(sizetype, char*&)’
>>>>   return new (ptr) T ();
>>>>                       ^
>>>> In file included from ../../gcc/alias.c:47:0:
>>>> ../../gcc/cselib.h:49:16: note: candidate: static void* cselib_val::operator new(size_t)
>>>>   inline void *operator new (size_t)
>>>>                ^
>>>> ../../gcc/cselib.h:49:16: note:   candidate expects 1 argument, 2 provided
>>>
>>> #include <new>
>>>
>>
>> Hi.
>>
>> <new> header file is not missing (explicit addition of the file does not help).
>> Feel free to play with following patch which should fix cselib.h compilation error.
> 
> cselib_val overrides the new operator but fails to provide an overload
> for the placement new
> form.  Fix that and it should work (of course it gets quite awkward
> with its 'new' calling
> pool.allocate and its placement new doing value-construction then...)
> 
> Richard.

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;
+  }

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;
  }
}

Thanks,
Martin



> 
>> Thanks,
>> Martin

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

[Richard Biener]

On Tue, Jun 16, 2015 at 3:38 PM, Martin Liška <mliska@suse.cz> wrote:
> On 06/16/2015 03:17 PM, Richard Biener wrote:
>> On Tue, Jun 16, 2015 at 11:26 AM, Martin Liška <mliska@suse.cz> wrote:
>>> On 06/15/2015 07:31 PM, Marc Glisse wrote:
>>>> On Mon, 15 Jun 2015, Martin Liška wrote:
>>>>
>>>>> Ah, I overlooked that it's not a placement new, but just static casting.
>>>>> Anyway, if I added:
>>>>>
>>>>> cselib_val () {}
>>>>>
>>>>> to struct cselib_val and changed the cast to placement new:
>>>>>  char *ptr = (char *) header;
>>>>>  return new (ptr) T ();
>>>>>
>>>>> I got following compilation error:
>>>>>
>>>>> In file included from ../../gcc/alias.c:46:0:
>>>>> ../../gcc/alloc-pool.h: In instantiation of ‘T* pool_allocator<T>::allocate() [with T = cselib_val]’:
>>>>> ../../gcc/cselib.h:51:27:   required from here
>>>>> ../../gcc/alloc-pool.h:416:23: error: no matching function for call to ‘cselib_val::operator new(sizetype, char*&)’
>>>>>   return new (ptr) T ();
>>>>>                       ^
>>>>> In file included from ../../gcc/alias.c:47:0:
>>>>> ../../gcc/cselib.h:49:16: note: candidate: static void* cselib_val::operator new(size_t)
>>>>>   inline void *operator new (size_t)
>>>>>                ^
>>>>> ../../gcc/cselib.h:49:16: note:   candidate expects 1 argument, 2 provided
>>>>
>>>> #include <new>
>>>>
>>>
>>> Hi.
>>>
>>> <new> header file is not missing (explicit addition of the file does not help).
>>> Feel free to play with following patch which should fix cselib.h compilation error.
>>
>> cselib_val overrides the new operator but fails to provide an overload
>> for the placement new
>> form.  Fix that and it should work (of course it gets quite awkward
>> with its 'new' calling
>> pool.allocate and its placement new doing value-construction then...)
>>
>> Richard.
>
> 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.

> }
>
> Thanks,
> Martin
>
>
>
>>
>>> Thanks,
>>> Martin
>

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2931 bytes --]

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:
>> On 06/16/2015 03:17 PM, Richard Biener wrote:
>>> On Tue, Jun 16, 2015 at 11:26 AM, Martin Liška <mliska@suse.cz> wrote:
>>>> On 06/15/2015 07:31 PM, Marc Glisse wrote:
>>>>> On Mon, 15 Jun 2015, Martin Liška wrote:
>>>>>
>>>>>> Ah, I overlooked that it's not a placement new, but just static casting.
>>>>>> Anyway, if I added:
>>>>>>
>>>>>> cselib_val () {}
>>>>>>
>>>>>> to struct cselib_val and changed the cast to placement new:
>>>>>>  char *ptr = (char *) header;
>>>>>>  return new (ptr) T ();
>>>>>>
>>>>>> I got following compilation error:
>>>>>>
>>>>>> In file included from ../../gcc/alias.c:46:0:
>>>>>> ../../gcc/alloc-pool.h: In instantiation of ‘T* pool_allocator<T>::allocate() [with T = cselib_val]’:
>>>>>> ../../gcc/cselib.h:51:27:   required from here
>>>>>> ../../gcc/alloc-pool.h:416:23: error: no matching function for call to ‘cselib_val::operator new(sizetype, char*&)’
>>>>>>   return new (ptr) T ();
>>>>>>                       ^
>>>>>> In file included from ../../gcc/alias.c:47:0:
>>>>>> ../../gcc/cselib.h:49:16: note: candidate: static void* cselib_val::operator new(size_t)
>>>>>>   inline void *operator new (size_t)
>>>>>>                ^
>>>>>> ../../gcc/cselib.h:49:16: note:   candidate expects 1 argument, 2 provided
>>>>>
>>>>> #include <new>
>>>>>
>>>>
>>>> Hi.
>>>>
>>>> <new> header file is not missing (explicit addition of the file does not help).
>>>> Feel free to play with following patch which should fix cselib.h compilation error.
>>>
>>> cselib_val overrides the new operator but fails to provide an overload
>>> for the placement new
>>> form.  Fix that and it should work (of course it gets quite awkward
>>> with its 'new' calling
>>> pool.allocate and its placement new doing value-construction then...)
>>>
>>> Richard.
>>
>> 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.

Thanks,
Martin

> 
>> }
>>
>> Thanks,
>> Martin
>>
>>
>>
>>>
>>>> Thanks,
>>>> Martin
>>


[-- Attachment #2: 0001-Add-placement-new-for-classes-in-pool-allocator.patch --]
[-- Type: text/x-patch, Size: 9773 bytes --]

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;
 }
 
 /* Puts PTR back on POOL's free list.  */
diff --git a/gcc/asan.c b/gcc/asan.c
index 599c822..a6160f7 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -361,6 +361,12 @@ struct asan_mem_ref
   /* The size of the access.  */
   HOST_WIDE_INT access_size;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, asan_mem_ref *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
diff --git a/gcc/cselib.c b/gcc/cselib.c
index 7ccaab4..9873aa0 100644
--- a/gcc/cselib.c
+++ b/gcc/cselib.c
@@ -53,6 +53,12 @@ struct elt_list
   struct elt_list *next;
   cselib_val *elt;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, elt_list *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
diff --git a/gcc/cselib.h b/gcc/cselib.h
index cdd06ad..9ef95e3 100644
--- a/gcc/cselib.h
+++ b/gcc/cselib.h
@@ -48,6 +48,12 @@ struct cselib_val
     return pool.allocate ();
   }
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, cselib_val *&ptr)
+  {
+    return ptr;
+  }
+
   /* Delete operator utilizing pool allocation.  */
   inline void operator delete (void *ptr)
   {
@@ -67,12 +73,21 @@ struct elt_loc_list {
   /* The insn that made the equivalence.  */
   rtx_insn *setting_insn;
 
+  /* Default constructor.  */
+  elt_loc_list () {}
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
     return pool.allocate ();
   }
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, elt_loc_list *&ptr)
+  {
+    return ptr;
+  }
+
   /* Delete operator utilizing pool allocation.  */
   inline void operator delete (void *ptr)
   {
diff --git a/gcc/dse.c b/gcc/dse.c
index d7d4ba6..9a6137a 100644
--- a/gcc/dse.c
+++ b/gcc/dse.c
@@ -345,6 +345,12 @@ struct read_info_type
   /* The next read_info for this insn.  */
   struct read_info_type *next;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, read_info_type *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
@@ -449,6 +455,12 @@ struct insn_info_type
      at active_local_stores.  */
   struct insn_info_type * next_local_store;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, insn_info_type *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
@@ -529,6 +541,12 @@ struct dse_bb_info_type
      accidentally clobber live hard regs.  */
   bitmap regs_live;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, dse_bb_info_type *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
@@ -615,6 +633,12 @@ struct group_info
   int *offset_map_n, *offset_map_p;
   int offset_map_size_n, offset_map_size_p;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, group_info *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
@@ -655,6 +679,12 @@ struct deferred_change
 
   struct deferred_change *next;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, deferred_change *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
diff --git a/gcc/et-forest.c b/gcc/et-forest.c
index 57c9916..d33ceb2 100644
--- a/gcc/et-forest.c
+++ b/gcc/et-forest.c
@@ -56,6 +56,12 @@ struct et_occ
   struct et_occ *min_occ;	/* The occurrence in the subtree with the minimal
 				   depth.  */
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, et_occ *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
diff --git a/gcc/et-forest.h b/gcc/et-forest.h
index 15c582d..4b2fb97 100644
--- a/gcc/et-forest.h
+++ b/gcc/et-forest.h
@@ -67,6 +67,14 @@ struct et_node
   struct et_occ *rightmost_occ;	/* The rightmost occurrence.  */
   struct et_occ *parent_occ;	/* The occurrence of the parent node.  */
 
+  inline et_node () {}
+
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, et_node *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
diff --git a/gcc/ira-color.c b/gcc/ira-color.c
index b9e1bda..2265caf 100644
--- a/gcc/ira-color.c
+++ b/gcc/ira-color.c
@@ -113,6 +113,12 @@ struct update_cost_record
   /* Next record for given allocno.  */
   struct update_cost_record *next;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, update_cost_record *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
diff --git a/gcc/lra-int.h b/gcc/lra-int.h
index 25bd3ce..800e0bb 100644
--- a/gcc/lra-int.h
+++ b/gcc/lra-int.h
@@ -55,6 +55,12 @@ struct lra_live_range
   /* Pointer to structures with the same start.	 */
   lra_live_range_t start_next;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, lra_live_range *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
@@ -85,6 +91,12 @@ struct lra_copy
   /* Next copy with correspondingly REGNO1 and REGNO2.	*/
   lra_copy_t regno1_next, regno2_next;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, lra_copy *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
@@ -208,6 +220,12 @@ struct lra_insn_reg
   /* Next reg info of the same insn.  */
   struct lra_insn_reg *next;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, lra_insn_reg *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
diff --git a/gcc/regcprop.c b/gcc/regcprop.c
index 8e6452e..3126547 100644
--- a/gcc/regcprop.c
+++ b/gcc/regcprop.c
@@ -59,6 +59,12 @@ struct queued_debug_insn_change
   rtx *loc;
   rtx new_rtx;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, queued_debug_insn_change *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 9bfcd98..a14a572 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -289,6 +289,12 @@ struct access
      caller.  */
   unsigned grp_not_necessarilly_dereferenced : 1;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, access *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
@@ -319,6 +325,12 @@ struct assign_link
   struct access *lacc, *racc;
   struct assign_link *next;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, assign_link *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
index c3adf51..4227624 100644
--- a/gcc/var-tracking.c
+++ b/gcc/var-tracking.c
@@ -271,6 +271,12 @@ typedef struct attrs_def
   /* Offset from start of DECL.  */
   HOST_WIDE_INT offset;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, attrs_def *&ptr)
+  {
+    return ptr;
+  }
+
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
@@ -302,6 +308,11 @@ typedef struct location_chain_def
   /* Initialized? */
   enum var_init_status init;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, location_chain_def *&ptr)
+  {
+    return ptr;
+  }
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
@@ -334,6 +345,11 @@ typedef struct loc_exp_dep_s
      the doubly-linked list.  */
   struct loc_exp_dep_s **pprev;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, loc_exp_dep_s *&ptr)
+  {
+    return ptr;
+  }
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
@@ -588,6 +604,11 @@ typedef struct shared_hash_def
   /* Actual hash table.  */
   variable_table_type *htab;
 
+  /* Placement new contructor.  */
+  inline void *operator new (size_t, shared_hash_def *&ptr)
+  {
+    return ptr;
+  }
   /* Pool allocation new operator.  */
   inline void *operator new (size_t)
   {
-- 
2.1.4

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

[Richard Biener]

On June 16, 2015 5:31:57 PM GMT+02:00, "Martin Liška" <mliska@suse.cz> 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:
>>> On 06/16/2015 03:17 PM, Richard Biener wrote:
>>>> On Tue, Jun 16, 2015 at 11:26 AM, Martin Liška <mliska@suse.cz>
>wrote:
>>>>> On 06/15/2015 07:31 PM, Marc Glisse wrote:
>>>>>> On Mon, 15 Jun 2015, Martin Liška wrote:
>>>>>>
>>>>>>> Ah, I overlooked that it's not a placement new, but just static
>casting.
>>>>>>> Anyway, if I added:
>>>>>>>
>>>>>>> cselib_val () {}
>>>>>>>
>>>>>>> to struct cselib_val and changed the cast to placement new:
>>>>>>>  char *ptr = (char *) header;
>>>>>>>  return new (ptr) T ();
>>>>>>>
>>>>>>> I got following compilation error:
>>>>>>>
>>>>>>> In file included from ../../gcc/alias.c:46:0:
>>>>>>> ../../gcc/alloc-pool.h: In instantiation of ‘T*
>pool_allocator<T>::allocate() [with T = cselib_val]’:
>>>>>>> ../../gcc/cselib.h:51:27:   required from here
>>>>>>> ../../gcc/alloc-pool.h:416:23: error: no matching function for
>call to ‘cselib_val::operator new(sizetype, char*&)’
>>>>>>>   return new (ptr) T ();
>>>>>>>                       ^
>>>>>>> In file included from ../../gcc/alias.c:47:0:
>>>>>>> ../../gcc/cselib.h:49:16: note: candidate: static void*
>cselib_val::operator new(size_t)
>>>>>>>   inline void *operator new (size_t)
>>>>>>>                ^
>>>>>>> ../../gcc/cselib.h:49:16: note:   candidate expects 1 argument,
>2 provided
>>>>>>
>>>>>> #include <new>
>>>>>>
>>>>>
>>>>> Hi.
>>>>>
>>>>> <new> header file is not missing (explicit addition of the file
>does not help).
>>>>> Feel free to play with following patch which should fix cselib.h
>compilation error.
>>>>
>>>> cselib_val overrides the new operator but fails to provide an
>overload
>>>> for the placement new
>>>> form.  Fix that and it should work (of course it gets quite awkward
>>>> with its 'new' calling
>>>> pool.allocate and its placement new doing value-construction
>then...)
>>>>
>>>> Richard.
>>>
>>> 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.

It might be better to just dispatch to
::new?

Richard.

>
>If it's the right way, I'll write Changelog and run testsuite.
>
>Thanks,
>Martin
>
>> 
>>> }
>>>
>>> Thanks,
>>> Martin
>>>
>>>
>>>
>>>>
>>>>> Thanks,
>>>>> Martin
>>>

Re: 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

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

[Richard Biener]

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

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

[Jakub Jelinek]

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

	Jakub

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

[Richard Biener]

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

Re: 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

Re: 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

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

[Pat Haugen]

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

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

[Martin Liška]

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

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

[Marc Glisse]

On Tue, 16 Jun 2015, Martin Liška wrote:

> On 06/15/2015 07:31 PM, Marc Glisse wrote:
>> On Mon, 15 Jun 2015, Martin Liška wrote:
>>
>>> Ah, I overlooked that it's not a placement new, but just static casting.
>>> Anyway, if I added:
>>>
>>> cselib_val () {}
>>>
>>> to struct cselib_val and changed the cast to placement new:
>>>  char *ptr = (char *) header;
>>>  return new (ptr) T ();
>>>
>>> I got following compilation error:
>>>
>>> In file included from ../../gcc/alias.c:46:0:
>>> ../../gcc/alloc-pool.h: In instantiation of ‘T* pool_allocator<T>::allocate() [with T = cselib_val]’:
>>> ../../gcc/cselib.h:51:27:   required from here
>>> ../../gcc/alloc-pool.h:416:23: error: no matching function for call to ‘cselib_val::operator new(sizetype, char*&)’
>>>   return new (ptr) T ();
>>>                       ^
>>> In file included from ../../gcc/alias.c:47:0:
>>> ../../gcc/cselib.h:49:16: note: candidate: static void* cselib_val::operator new(size_t)
>>>   inline void *operator new (size_t)
>>>                ^
>>> ../../gcc/cselib.h:49:16: note:   candidate expects 1 argument, 2 provided
>>
>> #include <new>
>
> Hi.
>
> <new> header file is not missing (explicit addition of the file does not help).
> Feel free to play with following patch which should fix cselib.h compilation error.

-  return (T *)(header);
+  return ::new (header) T ();

compiles just fine for me (without touching cselib.h).
(the discussion has moved on so this might not be relevant anymore)

-- 
Marc Glisse