Re: Still crashes due to aliasing violation (Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator)
On July 17, 2015 11:28:28 PM GMT+02:00, Ulrich Weigand uweig...@de.ibm.com wrote: On July 17, 2015 6:54:32 PM GMT+02:00, Ulrich Weigand uweig...@de.ibm.com wrote: So do we now consider host compilers 4.3 (4?) unsupported for building mainline GCC, or should we try to work around the issue (e.g. by moving the allocator out-of-line or using some other aliasing barrier)? Why is this an issue for stage1 which runs w/o optimization? Well, this is the SPU compiler on a Cell system, which is technically a cross compiler from PowerPC (even though the resulting binaries run natively on the machine). For cross compiling we already suggest using known good compilers. The documentation says: To build a cross compiler, we recommend first building and installing a native compiler. You can then use the native GCC compiler to build the cross compiler. The installed native compiler needs to be GCC version 2.95 or later. I think that needs updating anyway since even for crosses we now require a C++04 conforming host compiler. So building with a native GCC 4.1 seems to have been officially supported until now as far as I can tell (unless you're building Ada). Now, I could certainly live with a statement that cross compilers can only be build with a native GCC 4.3 or newer; but that should be IMO a deliberate decision and be widely announced (maybe even verified by a configure check?), so that others don't run into the problem; the nature of its symptoms make the problem difficult to diagnose. The requirement is to have a bug-free host compiler or use flags that make it appear bug-free. Which is why we use -O0 when bootstrapping... Yes, we could detect appropriate host gcc versions at configure time and apply a workaround (use -fno-strict-aliasing) for too old GCC. Richard. Bye, Ulrich
Re: Still crashes due to aliasing violation (Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator)
Richard Biener wrote:
On July 17, 2015 3:11:51 PM GMT+02:00, Ulrich Weigand uweig...@de.ibm.com
wrote:
(Since there is no C++ operator new involved at all anymore,
this clearly violates even the C aliasing rules ...)
I really think the allocate routine needs to be more careful to
avoid violating aliasing, e.g. by using memcpy or union-based
type-punning to access its free list info.
As far as I understand the object allocator delegates construction to callers
and thus in the above case cselib
Would be responsible for calling placement new on the return value from
Allocate.
Ah, it looks like I was wrong above: the code uses the *object*
allocator, so it should go through a placement new here:
inline T *
allocate () ATTRIBUTE_MALLOC
{
return ::new (m_allocator.allocate ()) T ();
}
It's still being miscompiled at least by my GCC 4.1 host compiler ...
Bye,
Ulrich
--
Dr. Ulrich Weigand
GNU/Linux compilers and toolchain
ulrich.weig...@de.ibm.com
Re: Still crashes due to aliasing violation (Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator)
On 07/17/2015 03:44 PM, Ulrich Weigand wrote:
Richard Biener wrote:
On July 17, 2015 3:11:51 PM GMT+02:00, Ulrich Weigand uweig...@de.ibm.com
wrote:
(Since there is no C++ operator new involved at all anymore,
this clearly violates even the C aliasing rules ...)
I really think the allocate routine needs to be more careful to
avoid violating aliasing, e.g. by using memcpy or union-based
type-punning to access its free list info.
As far as I understand the object allocator delegates construction to
callers and thus in the above case cselib
Would be responsible for calling placement new on the return value from
Allocate.
Ah, it looks like I was wrong above: the code uses the *object*
allocator, so it should go through a placement new here:
inline T *
allocate () ATTRIBUTE_MALLOC
{
return ::new (m_allocator.allocate ()) T ();
}
It's still being miscompiled at least by my GCC 4.1 host compiler ...
Bye,
Ulrich
Hi.
I've just wanted to write you that it really utilizes a placement new :)
The first example that bypasses pool allocator is of course a bug, I'll fix.
Question is why aliasing oracle still wrongly aliases these pointers?
Another option (suggested by Martin Jambor) would be to place ::allocate
implementation
to alloc-pool.c file.
Thoughts?
Martin
Re: Still crashes due to aliasing violation (Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator)
On July 17, 2015 3:11:51 PM GMT+02:00, Ulrich Weigand uweig...@de.ibm.com wrote: On 07/09/2015 11:43 PM, Martin Liška wrote: This final version which I agreed with Richard Sandiford. Hope this can be finally installed to trunk? Patch can bootstrap and survive regression tests on x86_64-linux-gnu. Unfortunately, this still crashes on my SPU toolchain build machine, for pretty much the same reason outlined here: https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00868.html However, the host compiler no longer miscompiles these lines: empty_shared_hash = new shared_hash_def; empty_shared_hash-refcount = 1; But that is simply because that new now goes to the default heap-based allocator from the standard library. There is a shared_hash_def_pool, but that's apparently not being used for anything -- this probably was not intended? But now the following lines are miscompiled: elt_list *el = elt_list_pool.allocate (); el-next = next; el-elt = elt; from new_elt_list in cselib.c. Again, the allocate call ends simply with a cast: header = m_returned_free_list; m_returned_free_list = header-next; return (void *)(header); and type-based aliasing now states the access to header-next in allocate must not alias the access to el-next in new_elt_list, but clearly it does. (Since there is no C++ operator new involved at all anymore, this clearly violates even the C aliasing rules ...) I really think the allocate routine needs to be more careful to avoid violating aliasing, e.g. by using memcpy or union-based type-punning to access its free list info. As far as I understand the object allocator delegates construction to callers and thus in the above case cselib Would be responsible for calling placement new on the return value from Allocate. Richard. Bye, Ulrich
Still crashes due to aliasing violation (Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator)
On 07/09/2015 11:43 PM, Martin Liška wrote: This final version which I agreed with Richard Sandiford. Hope this can be finally installed to trunk? Patch can bootstrap and survive regression tests on x86_64-linux-gnu. Unfortunately, this still crashes on my SPU toolchain build machine, for pretty much the same reason outlined here: https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00868.html However, the host compiler no longer miscompiles these lines: empty_shared_hash = new shared_hash_def; empty_shared_hash-refcount = 1; But that is simply because that new now goes to the default heap-based allocator from the standard library. There is a shared_hash_def_pool, but that's apparently not being used for anything -- this probably was not intended? But now the following lines are miscompiled: elt_list *el = elt_list_pool.allocate (); el-next = next; el-elt = elt; from new_elt_list in cselib.c. Again, the allocate call ends simply with a cast: header = m_returned_free_list; m_returned_free_list = header-next; return (void *)(header); and type-based aliasing now states the access to header-next in allocate must not alias the access to el-next in new_elt_list, but clearly it does. (Since there is no C++ operator new involved at all anymore, this clearly violates even the C aliasing rules ...) I really think the allocate routine needs to be more careful to avoid violating aliasing, e.g. by using memcpy or union-based type-punning to access its free list info. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain ulrich.weig...@de.ibm.com
Re: Still crashes due to aliasing violation (Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator)
On July 17, 2015 3:50:19 PM GMT+02:00, Martin Liška mli...@suse.cz wrote:
On 07/17/2015 03:44 PM, Ulrich Weigand wrote:
Richard Biener wrote:
On July 17, 2015 3:11:51 PM GMT+02:00, Ulrich Weigand
uweig...@de.ibm.com wrote:
(Since there is no C++ operator new involved at all anymore,
this clearly violates even the C aliasing rules ...)
I really think the allocate routine needs to be more careful to
avoid violating aliasing, e.g. by using memcpy or union-based
type-punning to access its free list info.
As far as I understand the object allocator delegates construction
to callers and thus in the above case cselib
Would be responsible for calling placement new on the return value
from
Allocate.
Ah, it looks like I was wrong above: the code uses the *object*
allocator, so it should go through a placement new here:
inline T *
allocate () ATTRIBUTE_MALLOC
{
return ::new (m_allocator.allocate ()) T ();
}
It's still being miscompiled at least by my GCC 4.1 host compiler ...
Bye,
Ulrich
Hi.
I've just wanted to write you that it really utilizes a placement new
:)
The first example that bypasses pool allocator is of course a bug, I'll
fix.
Question is why aliasing oracle still wrongly aliases these pointers?
Another option (suggested by Martin Jambor) would be to place
::allocate implementation
to alloc-pool.c file.
Note that all compilers up to 4.4 have aliasing issues with placement new.
A fix is to move the placement new out-of-line.
Richard.
Thoughts?
Martin
Re: Still crashes due to aliasing violation (Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator)
Richard Biener wrote: On July 17, 2015 3:50:19 PM GMT+02:00, Martin Liška mli...@suse.cz wrote: Question is why aliasing oracle still wrongly aliases these pointers? Another option (suggested by Martin Jambor) would be to place ::allocate implementation to alloc-pool.c file. Note that all compilers up to 4.4 have aliasing issues with placement new. A fix is to move the placement new out-of-line. Yes, that's what I just noticed as well. In fact, my particular problem already disappears with 4.3, presumably due to the fix for PR 29286. So do we now consider host compilers 4.3 (4?) unsupported for building mainline GCC, or should we try to work around the issue (e.g. by moving the allocator out-of-line or using some other aliasing barrier)? Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain ulrich.weig...@de.ibm.com
Re: Still crashes due to aliasing violation (Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator)
On July 17, 2015 6:54:32 PM GMT+02:00, Ulrich Weigand uweig...@de.ibm.com wrote: Richard Biener wrote: On July 17, 2015 3:50:19 PM GMT+02:00, Martin Liška mli...@suse.cz wrote: Question is why aliasing oracle still wrongly aliases these pointers? Another option (suggested by Martin Jambor) would be to place ::allocate implementation to alloc-pool.c file. Note that all compilers up to 4.4 have aliasing issues with placement new. A fix is to move the placement new out-of-line. Yes, that's what I just noticed as well. In fact, my particular problem already disappears with 4.3, presumably due to the fix for PR 29286. So do we now consider host compilers 4.3 (4?) unsupported for building mainline GCC, or should we try to work around the issue (e.g. by moving the allocator out-of-line or using some other aliasing barrier)? Why is this an issue for stage1 which runs w/o optimization? For cross compiling we already suggest using known good compilers. Bye, Ulrich
Re: Still crashes due to aliasing violation (Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator)
On 07/17/2015 05:03 PM, Richard Biener wrote:
On July 17, 2015 3:50:19 PM GMT+02:00, Martin Liška mli...@suse.cz wrote:
On 07/17/2015 03:44 PM, Ulrich Weigand wrote:
Richard Biener wrote:
On July 17, 2015 3:11:51 PM GMT+02:00, Ulrich Weigand
uweig...@de.ibm.com wrote:
(Since there is no C++ operator new involved at all anymore,
this clearly violates even the C aliasing rules ...)
I really think the allocate routine needs to be more careful to
avoid violating aliasing, e.g. by using memcpy or union-based
type-punning to access its free list info.
As far as I understand the object allocator delegates construction
to callers and thus in the above case cselib
Would be responsible for calling placement new on the return value
from
Allocate.
Ah, it looks like I was wrong above: the code uses the *object*
allocator, so it should go through a placement new here:
inline T *
allocate () ATTRIBUTE_MALLOC
{
return ::new (m_allocator.allocate ()) T ();
}
It's still being miscompiled at least by my GCC 4.1 host compiler ...
Bye,
Ulrich
Hi.
I've just wanted to write you that it really utilizes a placement new
:)
The first example that bypasses pool allocator is of course a bug, I'll
fix.
Question is why aliasing oracle still wrongly aliases these pointers?
Another option (suggested by Martin Jambor) would be to place
::allocate implementation
to alloc-pool.c file.
Note that all compilers up to 4.4 have aliasing issues with placement new.
A fix is to move the placement new out-of-line.
Richard.
Hi Richi.
Should I place the placement new to alloc-pool.c or should I leave it as it is?
Thanks for decision,
Martin
Thoughts?
Martin
Re: Still crashes due to aliasing violation (Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator)
On July 17, 2015 6:54:32 PM GMT+02:00, Ulrich Weigand uweig...@de.ibm.com wrote: So do we now consider host compilers 4.3 (4?) unsupported for building mainline GCC, or should we try to work around the issue (e.g. by moving the allocator out-of-line or using some other aliasing barrier)? Why is this an issue for stage1 which runs w/o optimization? Well, this is the SPU compiler on a Cell system, which is technically a cross compiler from PowerPC (even though the resulting binaries run natively on the machine). For cross compiling we already suggest using known good compilers. The documentation says: To build a cross compiler, we recommend first building and installing a native compiler. You can then use the native GCC compiler to build the cross compiler. The installed native compiler needs to be GCC version 2.95 or later. So building with a native GCC 4.1 seems to have been officially supported until now as far as I can tell (unless you're building Ada). Now, I could certainly live with a statement that cross compilers can only be build with a native GCC 4.3 or newer; but that should be IMO a deliberate decision and be widely announced (maybe even verified by a configure check?), so that others don't run into the problem; the nature of its symptoms make the problem difficult to diagnose. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain ulrich.weig...@de.ibm.com
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
On Thu, Jul 16, 2015 at 12:47 PM, Martin Liška mli...@suse.cz wrote:
On 07/09/2015 11:43 PM, Martin Liška wrote:
On 07/03/2015 06:18 PM, Richard Sandiford wrote:
Hi Martin,
Martin Liška mli...@suse.cz writes:
On 07/03/2015 03:07 PM, Richard Sandiford wrote:
Martin Jambor mjam...@suse.cz writes:
On Fri, Jul 03, 2015 at 09:55:58AM +0100, Richard Sandiford wrote:
Trevor Saunders tbsau...@tbsaunde.org writes:
On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote:
Martin Liška mli...@suse.cz writes:
diff --git a/gcc/asan.c b/gcc/asan.c
index e89817e..dabd6f1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -362,20 +362,20 @@ struct asan_mem_ref
/* Pool allocation new operator. */
inline void *operator new (size_t)
{
-return pool.allocate ();
+return ::new (pool.allocate ()) asan_mem_ref ();
}
/* Delete operator utilizing pool allocation. */
inline void operator delete (void *ptr)
{
-pool.remove ((asan_mem_ref *) ptr);
+pool.remove (ptr);
}
/* Memory allocation pool. */
- static pool_allocatorasan_mem_ref pool;
+ static pool_allocator pool;
};
I'm probably going over old ground/wounds, sorry, but what's the
benefit
of having this sort of pattern? Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with
pool.remove
calling the destructor) instead of new and delete? It feels wrong to
me
to tie the data type to a particular allocation object like this.
Well the big question is what does allocate() do about construction?
if
it seems wierd for it to not call the ctor, but I'm not sure we can do
a
good job of forwarding args to allocate() with C++98.
If you need non-default constructors then:
new (pool) type (aaa, bbb)...;
doesn't seem too bad. I agree object_allocator's allocate () should
call
the constructor.
but then the pool allocator must not call placement new on the
allocated memory itself because that would result in double
construction.
But we're talking about two different methods. The normal allocator
object_allocator T::allocate () would use placement new and return a
pointer to the new object while operator new (size_t, object_allocator
T )
wouldn't call placement new and would just return a pointer to the memory.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the
pool
itself is being cleared.
Well, all these cases involve a pool with static storage lifetime
right?
so actually if you don't delete things in these pool they are
effectively leaked.
They might have a static storage lifetime now, but it doesn't seem like
a good idea to hard-bake that into the interface
Does that mean that operators new and delete are considered evil?
Not IMO. Just that static load-time-initialized caches are not
necessarily a good thing. That's effectively what the pool
allocator is.
(by saying that for
these types you should use new and delete, but for other pool-allocated
types you should use object_allocators).
Depending on what kind of pool allocator you use, you will be forced
to either call placement new or not, so the inconsistency will be
there anyway.
But how we handle argument-taking constructors is a problem that needs
to be solved for the pool-allocated objects that don't use a single
static type-specific pool. And once we solve that, we get consistency
across all pools:
- if you want a new object and argumentless construction is OK,
use pool.allocate ()
- if you want a new object and need to pass arguments to the constructor,
use new (pool) some_type (arg1, arg2, ...)
Maybe I just have bad memories
from doing the SWITCHABLE_TARGET stuff, but there I was changing a lot
of state that was obviously static in the old days, but that needed
to become non-static to support vaguely-efficient switching between
different subtargets. The same kind of thing is likely to happen again.
I assume things like the jit would prefer not to have new global state
with load-time construction.
I'm not sure I follow this branch of the discussion, the allocators of
any kind surely can dynamically allocated themselves?
Sure, but either (a) you keep the pools as a static part of the class
and some initialisation and finalisation code that has tendrils into
all such classes or (b) you move the static pool outside of the
class to some new (still global) state. Explicit pool allocation,
like in the C days, gives you the option of putting the pool whereever
it needs to go without relying on the principle that you can get to
it from global state.
Thanks,
Richard
Ok Richard.
I've just finally understood your suggestions and I would suggest
following:
+ I will add a new method to object_allocatorT that will return an
allocated memory (void*)
(w/o calling any construction)
+
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
On 07/09/2015 11:43 PM, Martin Liška wrote:
On 07/03/2015 06:18 PM, Richard Sandiford wrote:
Hi Martin,
Martin Liška mli...@suse.cz writes:
On 07/03/2015 03:07 PM, Richard Sandiford wrote:
Martin Jambor mjam...@suse.cz writes:
On Fri, Jul 03, 2015 at 09:55:58AM +0100, Richard Sandiford wrote:
Trevor Saunders tbsau...@tbsaunde.org writes:
On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote:
Martin Liška mli...@suse.cz writes:
diff --git a/gcc/asan.c b/gcc/asan.c
index e89817e..dabd6f1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -362,20 +362,20 @@ struct asan_mem_ref
/* Pool allocation new operator. */
inline void *operator new (size_t)
{
-return pool.allocate ();
+return ::new (pool.allocate ()) asan_mem_ref ();
}
/* Delete operator utilizing pool allocation. */
inline void operator delete (void *ptr)
{
-pool.remove ((asan_mem_ref *) ptr);
+pool.remove (ptr);
}
/* Memory allocation pool. */
- static pool_allocatorasan_mem_ref pool;
+ static pool_allocator pool;
};
I'm probably going over old ground/wounds, sorry, but what's the
benefit
of having this sort of pattern? Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with
pool.remove
calling the destructor) instead of new and delete? It feels wrong to
me
to tie the data type to a particular allocation object like this.
Well the big question is what does allocate() do about construction? if
it seems wierd for it to not call the ctor, but I'm not sure we can do a
good job of forwarding args to allocate() with C++98.
If you need non-default constructors then:
new (pool) type (aaa, bbb)...;
doesn't seem too bad. I agree object_allocator's allocate () should call
the constructor.
but then the pool allocator must not call placement new on the
allocated memory itself because that would result in double
construction.
But we're talking about two different methods. The normal allocator
object_allocator T::allocate () would use placement new and return a
pointer to the new object while operator new (size_t, object_allocator T
)
wouldn't call placement new and would just return a pointer to the memory.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the pool
itself is being cleared.
Well, all these cases involve a pool with static storage lifetime right?
so actually if you don't delete things in these pool they are
effectively leaked.
They might have a static storage lifetime now, but it doesn't seem like
a good idea to hard-bake that into the interface
Does that mean that operators new and delete are considered evil?
Not IMO. Just that static load-time-initialized caches are not
necessarily a good thing. That's effectively what the pool
allocator is.
(by saying that for
these types you should use new and delete, but for other pool-allocated
types you should use object_allocators).
Depending on what kind of pool allocator you use, you will be forced
to either call placement new or not, so the inconsistency will be
there anyway.
But how we handle argument-taking constructors is a problem that needs
to be solved for the pool-allocated objects that don't use a single
static type-specific pool. And once we solve that, we get consistency
across all pools:
- if you want a new object and argumentless construction is OK,
use pool.allocate ()
- if you want a new object and need to pass arguments to the constructor,
use new (pool) some_type (arg1, arg2, ...)
Maybe I just have bad memories
from doing the SWITCHABLE_TARGET stuff, but there I was changing a lot
of state that was obviously static in the old days, but that needed
to become non-static to support vaguely-efficient switching between
different subtargets. The same kind of thing is likely to happen again.
I assume things like the jit would prefer not to have new global state
with load-time construction.
I'm not sure I follow this branch of the discussion, the allocators of
any kind surely can dynamically allocated themselves?
Sure, but either (a) you keep the pools as a static part of the class
and some initialisation and finalisation code that has tendrils into
all such classes or (b) you move the static pool outside of the
class to some new (still global) state. Explicit pool allocation,
like in the C days, gives you the option of putting the pool whereever
it needs to go without relying on the principle that you can get to
it from global state.
Thanks,
Richard
Ok Richard.
I've just finally understood your suggestions and I would suggest following:
+ I will add a new method to object_allocatorT that will return an
allocated memory (void*)
(w/o calling any construction)
+ object_allocatorT::allocate will call placement new with for a
parameterless ctor
+ I will
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
On 07/09/2015 04:43 PM, Martin Liška wrote: This final version which I agreed with Richard Sandiford. Hope this can be finally installed to trunk? Patch can bootstrap and survive regression tests on x86_64-linux-gnu. FWIW, I confirmed this version of the patch fixes the build issues on powerpc64 that I have been seeing. -Pat
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
On 07/03/2015 06:18 PM, Richard Sandiford wrote:
Hi Martin,
Martin Liška mli...@suse.cz writes:
On 07/03/2015 03:07 PM, Richard Sandiford wrote:
Martin Jambor mjam...@suse.cz writes:
On Fri, Jul 03, 2015 at 09:55:58AM +0100, Richard Sandiford wrote:
Trevor Saunders tbsau...@tbsaunde.org writes:
On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote:
Martin Liška mli...@suse.cz writes:
diff --git a/gcc/asan.c b/gcc/asan.c
index e89817e..dabd6f1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -362,20 +362,20 @@ struct asan_mem_ref
/* Pool allocation new operator. */
inline void *operator new (size_t)
{
-return pool.allocate ();
+return ::new (pool.allocate ()) asan_mem_ref ();
}
/* Delete operator utilizing pool allocation. */
inline void operator delete (void *ptr)
{
-pool.remove ((asan_mem_ref *) ptr);
+pool.remove (ptr);
}
/* Memory allocation pool. */
- static pool_allocatorasan_mem_ref pool;
+ static pool_allocator pool;
};
I'm probably going over old ground/wounds, sorry, but what's the benefit
of having this sort of pattern? Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with
pool.remove
calling the destructor) instead of new and delete? It feels wrong to me
to tie the data type to a particular allocation object like this.
Well the big question is what does allocate() do about construction? if
it seems wierd for it to not call the ctor, but I'm not sure we can do a
good job of forwarding args to allocate() with C++98.
If you need non-default constructors then:
new (pool) type (aaa, bbb)...;
doesn't seem too bad. I agree object_allocator's allocate () should call
the constructor.
but then the pool allocator must not call placement new on the
allocated memory itself because that would result in double
construction.
But we're talking about two different methods. The normal allocator
object_allocator T::allocate () would use placement new and return a
pointer to the new object while operator new (size_t, object_allocator T )
wouldn't call placement new and would just return a pointer to the memory.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the pool
itself is being cleared.
Well, all these cases involve a pool with static storage lifetime right?
so actually if you don't delete things in these pool they are
effectively leaked.
They might have a static storage lifetime now, but it doesn't seem like
a good idea to hard-bake that into the interface
Does that mean that operators new and delete are considered evil?
Not IMO. Just that static load-time-initialized caches are not
necessarily a good thing. That's effectively what the pool
allocator is.
(by saying that for
these types you should use new and delete, but for other pool-allocated
types you should use object_allocators).
Depending on what kind of pool allocator you use, you will be forced
to either call placement new or not, so the inconsistency will be
there anyway.
But how we handle argument-taking constructors is a problem that needs
to be solved for the pool-allocated objects that don't use a single
static type-specific pool. And once we solve that, we get consistency
across all pools:
- if you want a new object and argumentless construction is OK,
use pool.allocate ()
- if you want a new object and need to pass arguments to the constructor,
use new (pool) some_type (arg1, arg2, ...)
Maybe I just have bad memories
from doing the SWITCHABLE_TARGET stuff, but there I was changing a lot
of state that was obviously static in the old days, but that needed
to become non-static to support vaguely-efficient switching between
different subtargets. The same kind of thing is likely to happen again.
I assume things like the jit would prefer not to have new global state
with load-time construction.
I'm not sure I follow this branch of the discussion, the allocators of
any kind surely can dynamically allocated themselves?
Sure, but either (a) you keep the pools as a static part of the class
and some initialisation and finalisation code that has tendrils into
all such classes or (b) you move the static pool outside of the
class to some new (still global) state. Explicit pool allocation,
like in the C days, gives you the option of putting the pool whereever
it needs to go without relying on the principle that you can get to
it from global state.
Thanks,
Richard
Ok Richard.
I've just finally understood your suggestions and I would suggest following:
+ I will add a new method to object_allocatorT that will return an
allocated memory (void*)
(w/o calling any construction)
+ object_allocatorT::allocate will call placement new with for a
parameterless ctor
+ I will remove all overwritten operators new/delete on e.g. et_forest, ...
+ For these classes, I will add void* operator new (size_t,
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
On 07/03/2015 03:07 PM, Richard Sandiford wrote:
Martin Jambor mjam...@suse.cz writes:
On Fri, Jul 03, 2015 at 09:55:58AM +0100, Richard Sandiford wrote:
Trevor Saunders tbsau...@tbsaunde.org writes:
On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote:
Martin Liška mli...@suse.cz writes:
diff --git a/gcc/asan.c b/gcc/asan.c
index e89817e..dabd6f1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -362,20 +362,20 @@ struct asan_mem_ref
/* Pool allocation new operator. */
inline void *operator new (size_t)
{
-return pool.allocate ();
+return ::new (pool.allocate ()) asan_mem_ref ();
}
/* Delete operator utilizing pool allocation. */
inline void operator delete (void *ptr)
{
-pool.remove ((asan_mem_ref *) ptr);
+pool.remove (ptr);
}
/* Memory allocation pool. */
- static pool_allocatorasan_mem_ref pool;
+ static pool_allocator pool;
};
I'm probably going over old ground/wounds, sorry, but what's the benefit
of having this sort of pattern? Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with
pool.remove
calling the destructor) instead of new and delete? It feels wrong to me
to tie the data type to a particular allocation object like this.
Well the big question is what does allocate() do about construction? if
it seems wierd for it to not call the ctor, but I'm not sure we can do a
good job of forwarding args to allocate() with C++98.
If you need non-default constructors then:
new (pool) type (aaa, bbb)...;
doesn't seem too bad. I agree object_allocator's allocate () should call
the constructor.
but then the pool allocator must not call placement new on the
allocated memory itself because that would result in double
construction.
But we're talking about two different methods. The normal allocator
object_allocator T::allocate () would use placement new and return a
pointer to the new object while operator new (size_t, object_allocator T )
wouldn't call placement new and would just return a pointer to the memory.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the pool
itself is being cleared.
Well, all these cases involve a pool with static storage lifetime right?
so actually if you don't delete things in these pool they are
effectively leaked.
They might have a static storage lifetime now, but it doesn't seem like
a good idea to hard-bake that into the interface
Does that mean that operators new and delete are considered evil?
Not IMO. Just that static load-time-initialized caches are not
necessarily a good thing. That's effectively what the pool
allocator is.
(by saying that for
these types you should use new and delete, but for other pool-allocated
types you should use object_allocators).
Depending on what kind of pool allocator you use, you will be forced
to either call placement new or not, so the inconsistency will be
there anyway.
But how we handle argument-taking constructors is a problem that needs
to be solved for the pool-allocated objects that don't use a single
static type-specific pool. And once we solve that, we get consistency
across all pools:
- if you want a new object and argumentless construction is OK,
use pool.allocate ()
- if you want a new object and need to pass arguments to the constructor,
use new (pool) some_type (arg1, arg2, ...)
Maybe I just have bad memories
from doing the SWITCHABLE_TARGET stuff, but there I was changing a lot
of state that was obviously static in the old days, but that needed
to become non-static to support vaguely-efficient switching between
different subtargets. The same kind of thing is likely to happen again.
I assume things like the jit would prefer not to have new global state
with load-time construction.
I'm not sure I follow this branch of the discussion, the allocators of
any kind surely can dynamically allocated themselves?
Sure, but either (a) you keep the pools as a static part of the class
and some initialisation and finalisation code that has tendrils into
all such classes or (b) you move the static pool outside of the
class to some new (still global) state. Explicit pool allocation,
like in the C days, gives you the option of putting the pool whereever
it needs to go without relying on the principle that you can get to
it from global state.
Thanks,
Richard
Ok Richard.
I've just finally understood your suggestions and I would suggest following:
+ I will add a new method to object_allocatorT that will return an allocated
memory (void*)
(w/o calling any construction)
+ object_allocatorT::allocate will call placement new with for a
parameterless ctor
+ I will remove all overwritten operators new/delete on e.g. et_forest, ...
+ For these classes, I will add void* operator new (size_t, object_allocatorT
)
+ Pool
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
Martin Jambor mjam...@suse.cz writes:
On Fri, Jul 03, 2015 at 09:55:58AM +0100, Richard Sandiford wrote:
Trevor Saunders tbsau...@tbsaunde.org writes:
On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote:
Martin Liška mli...@suse.cz writes:
diff --git a/gcc/asan.c b/gcc/asan.c
index e89817e..dabd6f1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -362,20 +362,20 @@ struct asan_mem_ref
/* Pool allocation new operator. */
inline void *operator new (size_t)
{
-return pool.allocate ();
+return ::new (pool.allocate ()) asan_mem_ref ();
}
/* Delete operator utilizing pool allocation. */
inline void operator delete (void *ptr)
{
-pool.remove ((asan_mem_ref *) ptr);
+pool.remove (ptr);
}
/* Memory allocation pool. */
- static pool_allocatorasan_mem_ref pool;
+ static pool_allocator pool;
};
I'm probably going over old ground/wounds, sorry, but what's the benefit
of having this sort of pattern? Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with
pool.remove
calling the destructor) instead of new and delete? It feels wrong to me
to tie the data type to a particular allocation object like this.
Well the big question is what does allocate() do about construction? if
it seems wierd for it to not call the ctor, but I'm not sure we can do a
good job of forwarding args to allocate() with C++98.
If you need non-default constructors then:
new (pool) type (aaa, bbb)...;
doesn't seem too bad. I agree object_allocator's allocate () should call
the constructor.
but then the pool allocator must not call placement new on the
allocated memory itself because that would result in double
construction.
But we're talking about two different methods. The normal allocator
object_allocator T::allocate () would use placement new and return a
pointer to the new object while operator new (size_t, object_allocator T )
wouldn't call placement new and would just return a pointer to the memory.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the pool
itself is being cleared.
Well, all these cases involve a pool with static storage lifetime right?
so actually if you don't delete things in these pool they are
effectively leaked.
They might have a static storage lifetime now, but it doesn't seem like
a good idea to hard-bake that into the interface
Does that mean that operators new and delete are considered evil?
Not IMO. Just that static load-time-initialized caches are not
necessarily a good thing. That's effectively what the pool
allocator is.
(by saying that for
these types you should use new and delete, but for other pool-allocated
types you should use object_allocators).
Depending on what kind of pool allocator you use, you will be forced
to either call placement new or not, so the inconsistency will be
there anyway.
But how we handle argument-taking constructors is a problem that needs
to be solved for the pool-allocated objects that don't use a single
static type-specific pool. And once we solve that, we get consistency
across all pools:
- if you want a new object and argumentless construction is OK,
use pool.allocate ()
- if you want a new object and need to pass arguments to the constructor,
use new (pool) some_type (arg1, arg2, ...)
Maybe I just have bad memories
from doing the SWITCHABLE_TARGET stuff, but there I was changing a lot
of state that was obviously static in the old days, but that needed
to become non-static to support vaguely-efficient switching between
different subtargets. The same kind of thing is likely to happen again.
I assume things like the jit would prefer not to have new global state
with load-time construction.
I'm not sure I follow this branch of the discussion, the allocators of
any kind surely can dynamically allocated themselves?
Sure, but either (a) you keep the pools as a static part of the class
and some initialisation and finalisation code that has tendrils into
all such classes or (b) you move the static pool outside of the
class to some new (still global) state. Explicit pool allocation,
like in the C days, gives you the option of putting the pool whereever
it needs to go without relying on the principle that you can get to
it from global state.
Thanks,
Richard
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
Hi,
On Fri, Jul 03, 2015 at 09:55:58AM +0100, Richard Sandiford wrote:
Trevor Saunders tbsau...@tbsaunde.org writes:
On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote:
Martin Liška mli...@suse.cz writes:
diff --git a/gcc/asan.c b/gcc/asan.c
index e89817e..dabd6f1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -362,20 +362,20 @@ struct asan_mem_ref
/* Pool allocation new operator. */
inline void *operator new (size_t)
{
-return pool.allocate ();
+return ::new (pool.allocate ()) asan_mem_ref ();
}
/* Delete operator utilizing pool allocation. */
inline void operator delete (void *ptr)
{
-pool.remove ((asan_mem_ref *) ptr);
+pool.remove (ptr);
}
/* Memory allocation pool. */
- static pool_allocatorasan_mem_ref pool;
+ static pool_allocator pool;
};
I'm probably going over old ground/wounds, sorry, but what's the benefit
of having this sort of pattern? Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with pool.remove
calling the destructor) instead of new and delete? It feels wrong to me
to tie the data type to a particular allocation object like this.
Well the big question is what does allocate() do about construction? if
it seems wierd for it to not call the ctor, but I'm not sure we can do a
good job of forwarding args to allocate() with C++98.
If you need non-default constructors then:
new (pool) type (aaa, bbb)...;
doesn't seem too bad. I agree object_allocator's allocate () should call
the constructor.
but then the pool allocator must not call placement new on the
allocated memory itself because that would result in double
construction. And calling placement new was explicitely requested in
the previous thread about allocators, so we still need two kinds of
allocators, typed and untyped. Or just the untyped allocators and
requiring that users construct their objects via placement new. In
fact, they might have to call placement new even if there is no
constructor because of the weird aliasing issue. Two kinds of
pool-allocators seem the lesser evil to me.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the pool
itself is being cleared.
Well, all these cases involve a pool with static storage lifetime right?
so actually if you don't delete things in these pool they are
effectively leaked.
They might have a static storage lifetime now, but it doesn't seem like
a good idea to hard-bake that into the interface
Does that mean that operators new and delete are considered evil?
(by saying that for
these types you should use new and delete, but for other pool-allocated
types you should use object_allocators).
Depending on what kind of pool allocator you use, you will be forced
to either call placement new or not, so the inconsistency will be
there anyway.
I'm using pool allocators for classes with non-default constructors a
lot in the HSA branch so I'd appreciate an early settlement of this
issue. I think I slightly prefer overloading new and delete to using
placement new (at least in new code) because then users just allocate
stuff as usual and there is one central point where thing can be
changed. But I do not have strong feelings and will comply with
whatever we can agree on.
Maybe I just have bad memories
from doing the SWITCHABLE_TARGET stuff, but there I was changing a lot
of state that was obviously static in the old days, but that needed
to become non-static to support vaguely-efficient switching between
different subtargets. The same kind of thing is likely to happen again.
I assume things like the jit would prefer not to have new global state
with load-time construction.
I'm not sure I follow this branch of the discussion, the allocators of
any kind surely can dynamically allocated themselves?
Thanks,
Martin
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
Hi Martin,
Martin Liška mli...@suse.cz writes:
On 07/03/2015 03:07 PM, Richard Sandiford wrote:
Martin Jambor mjam...@suse.cz writes:
On Fri, Jul 03, 2015 at 09:55:58AM +0100, Richard Sandiford wrote:
Trevor Saunders tbsau...@tbsaunde.org writes:
On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote:
Martin Liška mli...@suse.cz writes:
diff --git a/gcc/asan.c b/gcc/asan.c
index e89817e..dabd6f1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -362,20 +362,20 @@ struct asan_mem_ref
/* Pool allocation new operator. */
inline void *operator new (size_t)
{
-return pool.allocate ();
+return ::new (pool.allocate ()) asan_mem_ref ();
}
/* Delete operator utilizing pool allocation. */
inline void operator delete (void *ptr)
{
-pool.remove ((asan_mem_ref *) ptr);
+pool.remove (ptr);
}
/* Memory allocation pool. */
- static pool_allocatorasan_mem_ref pool;
+ static pool_allocator pool;
};
I'm probably going over old ground/wounds, sorry, but what's the benefit
of having this sort of pattern? Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with
pool.remove
calling the destructor) instead of new and delete? It feels wrong to me
to tie the data type to a particular allocation object like this.
Well the big question is what does allocate() do about construction? if
it seems wierd for it to not call the ctor, but I'm not sure we can do a
good job of forwarding args to allocate() with C++98.
If you need non-default constructors then:
new (pool) type (aaa, bbb)...;
doesn't seem too bad. I agree object_allocator's allocate () should call
the constructor.
but then the pool allocator must not call placement new on the
allocated memory itself because that would result in double
construction.
But we're talking about two different methods. The normal allocator
object_allocator T::allocate () would use placement new and return a
pointer to the new object while operator new (size_t, object_allocator T )
wouldn't call placement new and would just return a pointer to the memory.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the pool
itself is being cleared.
Well, all these cases involve a pool with static storage lifetime right?
so actually if you don't delete things in these pool they are
effectively leaked.
They might have a static storage lifetime now, but it doesn't seem like
a good idea to hard-bake that into the interface
Does that mean that operators new and delete are considered evil?
Not IMO. Just that static load-time-initialized caches are not
necessarily a good thing. That's effectively what the pool
allocator is.
(by saying that for
these types you should use new and delete, but for other pool-allocated
types you should use object_allocators).
Depending on what kind of pool allocator you use, you will be forced
to either call placement new or not, so the inconsistency will be
there anyway.
But how we handle argument-taking constructors is a problem that needs
to be solved for the pool-allocated objects that don't use a single
static type-specific pool. And once we solve that, we get consistency
across all pools:
- if you want a new object and argumentless construction is OK,
use pool.allocate ()
- if you want a new object and need to pass arguments to the constructor,
use new (pool) some_type (arg1, arg2, ...)
Maybe I just have bad memories
from doing the SWITCHABLE_TARGET stuff, but there I was changing a lot
of state that was obviously static in the old days, but that needed
to become non-static to support vaguely-efficient switching between
different subtargets. The same kind of thing is likely to happen again.
I assume things like the jit would prefer not to have new global state
with load-time construction.
I'm not sure I follow this branch of the discussion, the allocators of
any kind surely can dynamically allocated themselves?
Sure, but either (a) you keep the pools as a static part of the class
and some initialisation and finalisation code that has tendrils into
all such classes or (b) you move the static pool outside of the
class to some new (still global) state. Explicit pool allocation,
like in the C days, gives you the option of putting the pool whereever
it needs to go without relying on the principle that you can get to
it from global state.
Thanks,
Richard
Ok Richard.
I've just finally understood your suggestions and I would suggest following:
+ I will add a new method to object_allocatorT that will return an
allocated memory (void*)
(w/o calling any construction)
+ object_allocatorT::allocate will call placement new with for a
parameterless ctor
+ I will remove all overwritten operators new/delete on e.g. et_forest, ...
+ For these classes, I will add void*
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
Trevor Saunders tbsau...@tbsaunde.org writes:
On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote:
Martin Liška mli...@suse.cz writes:
diff --git a/gcc/asan.c b/gcc/asan.c
index e89817e..dabd6f1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -362,20 +362,20 @@ struct asan_mem_ref
/* Pool allocation new operator. */
inline void *operator new (size_t)
{
-return pool.allocate ();
+return ::new (pool.allocate ()) asan_mem_ref ();
}
/* Delete operator utilizing pool allocation. */
inline void operator delete (void *ptr)
{
-pool.remove ((asan_mem_ref *) ptr);
+pool.remove (ptr);
}
/* Memory allocation pool. */
- static pool_allocatorasan_mem_ref pool;
+ static pool_allocator pool;
};
I'm probably going over old ground/wounds, sorry, but what's the benefit
of having this sort of pattern? Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with pool.remove
calling the destructor) instead of new and delete? It feels wrong to me
to tie the data type to a particular allocation object like this.
Well the big question is what does allocate() do about construction? if
it seems wierd for it to not call the ctor, but I'm not sure we can do a
good job of forwarding args to allocate() with C++98.
If you need non-default constructors then:
new (pool) type (aaa, bbb)...;
doesn't seem too bad. I agree object_allocator's allocate () should call
the constructor.
However it seems kind of wierd the operator new here is calling the
placement new on the object it allocates.
Yeah.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the pool
itself is being cleared.
Well, all these cases involve a pool with static storage lifetime right?
so actually if you don't delete things in these pool they are
effectively leaked.
They might have a static storage lifetime now, but it doesn't seem like
a good idea to hard-bake that into the interface (by saying that for
these types you should use new and delete, but for other pool-allocated
types you should use object_allocators). Maybe I just have bad memories
from doing the SWITCHABLE_TARGET stuff, but there I was changing a lot
of state that was obviously static in the old days, but that needed
to become non-static to support vaguely-efficient switching between
different subtargets. The same kind of thing is likely to happen again.
I assume things like the jit would prefer not to have new global state
with load-time construction.
Thanks,
Richard
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
On 07/03/2015 10:55 AM, Richard Sandiford wrote:
Trevor Saunders tbsau...@tbsaunde.org writes:
On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote:
Martin Liška mli...@suse.cz writes:
diff --git a/gcc/asan.c b/gcc/asan.c
index e89817e..dabd6f1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -362,20 +362,20 @@ struct asan_mem_ref
/* Pool allocation new operator. */
inline void *operator new (size_t)
{
-return pool.allocate ();
+return ::new (pool.allocate ()) asan_mem_ref ();
}
/* Delete operator utilizing pool allocation. */
inline void operator delete (void *ptr)
{
-pool.remove ((asan_mem_ref *) ptr);
+pool.remove (ptr);
}
/* Memory allocation pool. */
- static pool_allocatorasan_mem_ref pool;
+ static pool_allocator pool;
};
I'm probably going over old ground/wounds, sorry, but what's the benefit
of having this sort of pattern? Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with pool.remove
calling the destructor) instead of new and delete? It feels wrong to me
to tie the data type to a particular allocation object like this.
Well the big question is what does allocate() do about construction? if
it seems wierd for it to not call the ctor, but I'm not sure we can do a
good job of forwarding args to allocate() with C++98.
If you need non-default constructors then:
new (pool) type (aaa, bbb)...;
doesn't seem too bad. I agree object_allocator's allocate () should call
the constructor.
Hello.
I do not insist on having a new/delete operator for aforementioned class.
However, I don't know a different approach that will do an object construction
in the allocate method w/o utilizing placement new?
However it seems kind of wierd the operator new here is calling the
placement new on the object it allocates.
Yeah.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the pool
itself is being cleared.
Well, all these cases involve a pool with static storage lifetime right?
so actually if you don't delete things in these pool they are
effectively leaked.
They might have a static storage lifetime now, but it doesn't seem like
a good idea to hard-bake that into the interface (by saying that for
these types you should use new and delete, but for other pool-allocated
types you should use object_allocators). Maybe I just have bad memories
from doing the SWITCHABLE_TARGET stuff, but there I was changing a lot
of state that was obviously static in the old days, but that needed
to become non-static to support vaguely-efficient switching between
different subtargets. The same kind of thing is likely to happen again.
I assume things like the jit would prefer not to have new global state
with load-time construction.
Agree with that it's a global state. But even before my transformation the code
utilized static variables that are similar problem from e.g. JIT perspective.
Best
approach would be to encapsulate these static allocators to a class (a pass?).
It's
quite a lot of work.
Thanks,
Martin
Thanks,
Richard
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
Martin Liška mli...@suse.cz writes:
@@ -136,18 +135,18 @@ private:
int64_t align_i;
} u;
-static inline allocation_objectU *
+static inline allocation_object*
space before *
get_instance (void *data_ptr)
{
- return (allocation_objectU *)(((char *)(data_ptr))
- - offsetof (allocation_objectU,
+ return (allocation_object *)(((char *)(data_ptr))
+ - offsetof (allocation_object,
u.data));
space between ) and (.
}
-static inline U *
+static inline void*
get_data (void *instance_ptr)
{
- return (U*)(((allocation_objectU *) instance_ptr)-u.data);
+ return (void*)(((allocation_object *) instance_ptr)-u.data);
same 2 comments here, although maybe dropping the cast would be better?
@@ -387,11 +349,11 @@ pool_allocatorT::allocate ()
/* We now know that we can take the first elt off the virgin list and
put it on the returned list. */
block = m_virgin_free_list;
- header = (allocation_pool_list*) allocation_objectT::get_data
(block);
+ header = (allocation_pool_list*) allocation_object::get_data (block);
Space before *. I'll not list out the others :-)
@@ -408,36 +370,34 @@ pool_allocatorT::allocate ()
#ifdef ENABLE_CHECKING
/* Set the ID for element. */
- allocation_objectT::get_instance (header)-id = m_id;
+ allocation_object::get_instance (header)-id = m_id;
#endif
VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size));
- /* Call default constructor. */
- return (T *)(header);
+ return (void *)(header);
Same comment about cast to void *.
diff --git a/gcc/asan.c b/gcc/asan.c
index e89817e..dabd6f1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -362,20 +362,20 @@ struct asan_mem_ref
/* Pool allocation new operator. */
inline void *operator new (size_t)
{
-return pool.allocate ();
+return ::new (pool.allocate ()) asan_mem_ref ();
}
/* Delete operator utilizing pool allocation. */
inline void operator delete (void *ptr)
{
-pool.remove ((asan_mem_ref *) ptr);
+pool.remove (ptr);
}
/* Memory allocation pool. */
- static pool_allocatorasan_mem_ref pool;
+ static pool_allocator pool;
};
I'm probably going over old ground/wounds, sorry, but what's the benefit
of having this sort of pattern? Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with pool.remove
calling the destructor) instead of new and delete? It feels wrong to me
to tie the data type to a particular allocation object like this.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the pool
itself is being cleared.
Thanks,
Richard
Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator
On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote:
Martin Liška mli...@suse.cz writes:
diff --git a/gcc/asan.c b/gcc/asan.c
index e89817e..dabd6f1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -362,20 +362,20 @@ struct asan_mem_ref
/* Pool allocation new operator. */
inline void *operator new (size_t)
{
-return pool.allocate ();
+return ::new (pool.allocate ()) asan_mem_ref ();
}
/* Delete operator utilizing pool allocation. */
inline void operator delete (void *ptr)
{
-pool.remove ((asan_mem_ref *) ptr);
+pool.remove (ptr);
}
/* Memory allocation pool. */
- static pool_allocatorasan_mem_ref pool;
+ static pool_allocator pool;
};
I'm probably going over old ground/wounds, sorry, but what's the benefit
of having this sort of pattern? Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with pool.remove
calling the destructor) instead of new and delete? It feels wrong to me
to tie the data type to a particular allocation object like this.
Well the big question is what does allocate() do about construction? if
it seems wierd for it to not call the ctor, but I'm not sure we can do a
good job of forwarding args to allocate() with C++98.
However it seems kind of wierd the operator new here is calling the
placement new on the object it allocates.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the pool
itself is being cleared.
Well, all these cases involve a pool with static storage lifetime right?
so actually if you don't delete things in these pool they are
effectively leaked.
Trev
Thanks,
Richard
[RFC, PATCH] Split pool_allocator and create a new object_allocator
Hello. This mail thread is follow-up from the previous thread, where Ulrich Weigand spotted a miscompilation: https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00868.html Following patch changes behavior of the pool_allocator: 1) pool_allocator is typeless and does not call any ctor 2) pool_allocator returns void* as a result of allocate function 3) all classes (e.g. et_occ) which use an operator new utilize the typeless allocator 4) object_allocator is a new type-based allocator which is used for allocation of objects 5) for a type T, the allocator calls placement new constructor and T* is return type of the allocate function 6) the object_allocator internally utilizes pool_allocator with m_size == sizeof(T) The patch can bootstrap on x86_64-unknown-linux-gnu and ppc64le-unknown-linux-gnu and regression tests have been running. Thoughts? Thanks, Martin From 9702a6ea0b60985f08ff28b0977c1dc46c25f27b Mon Sep 17 00:00:00 2001 From: mliska mli...@suse.cz Date: Wed, 24 Jun 2015 13:42:52 +0200 Subject: [PATCH] Add new object_allocator. gcc/c-family/ChangeLog: 2015-06-24 Martin Liska mli...@suse.cz * c-format.c (static void check_format_info_main): Use object_allocator instead of pool_allocator. (check_format_arg): Likewise. (check_format_info_main): Likewise. gcc/ChangeLog: 2015-06-24 Martin Liska mli...@suse.cz * alloc-pool.h (pool_allocator::initialize): Change to type-less pool_allocator. (pool_allocator::allocate): Likewise. (pool_allocator::remove): Likewise. * asan.c (struct asan_mem_ref): Likewise. * cfg.c (initialize_original_copy_tables): Use object_allocator instead of pool_allocator. * cselib.c (struct elt_list): Change declaration of used pool_allocator. (new_cselib_val): Likewise. * cselib.h (struct cselib_val): Change to type-less pool_allocator.. (struct elt_loc_list): Likewise. * df-problems.c (df_chain_alloc): Use object_allocator instead of pool_allocator.. * df-scan.c (struct df_scan_problem_data): Likewise. (df_scan_alloc): Likewise. * df.h (struct dataflow): Likewise. * dse.c (struct read_info_type): Change to type-less pool_allocator. (struct insn_info_type): Likewise. (struct dse_bb_info_type): Likewise. (struct group_info): Likewise. (struct deferred_change): Likewise. * et-forest.c (struct et_occ): Likewise. * et-forest.h (struct et_node): Likewise. * ipa-cp.c: Use object_allocator instead of pool_allocator. * ipa-inline-analysis.c: Likewise. * ipa-profile.c: Likewise. * ipa-prop.c: Likewise. * ipa-prop.h: Likewise. * ira-build.c (initiate_cost_vectors): Change to type-less pool_allocator. (ira_allocate_cost_vector): Likewise. * ira-color.c (struct update_cost_record): Use object_allocator instead of pool_allocator. * lra-int.h (struct lra_live_range): Change to type-less pool_allocator. (struct lra_copy): Likewise. (struct lra_insn_reg): Likewise. * lra-lives.c: Likewise. * lra.c: Likewise. * regcprop.c (struct queued_debug_insn_change): Use object_allocator instead of pool_allocator. * sched-deps.c (sched_deps_init): Likewise. * sel-sched-ir.c: Likewise. * sel-sched-ir.h: Likewise. * stmt.c (expand_case): Likewise. (expand_sjlj_dispatch_table): Likewise. * tree-sra.c (struct access): Change to type-less pool_allocator. (struct assign_link): Likewise. * tree-ssa-math-opts.c (pass_cse_reciprocals::execute): Use object_allocator instead of pool_allocator. * tree-ssa-pre.c: Likewise. * tree-ssa-reassoc.c: Likewise. * tree-ssa-sccvn.c (allocate_vn_table): Likewise. * tree-ssa-strlen.c: Likewise. * tree-ssa-structalias.c: Likewise. * var-tracking.c (onepart_pool_allocate): Change to type-less pool_allocator. (unshare_variable): Likewise. (variable_merge_over_cur): Likewise. (variable_from_dropped): Likewise. (variable_was_changed): Likewise. (set_slot_part): Likewise. (emit_notes_for_differences_1): Likewise. --- gcc/alloc-pool.h | 176 + gcc/asan.c | 8 +-- gcc/c-family/c-format.c| 7 +- gcc/cfg.c | 5 +- gcc/cselib.c | 17 +++-- gcc/cselib.h | 12 ++-- gcc/df-problems.c | 2 +- gcc/df-scan.c | 24 +++ gcc/df.h | 2 +- gcc/dse.c | 50 +++-- gcc/et-forest.c| 11 ++- gcc/et-forest.h| 6 +- gcc/ipa-cp.c | 8 +-- gcc/ipa-inline-analysis.c | 2 +- gcc/ipa-profile.c | 2 +- gcc/ipa-prop.c | 2 +- gcc/ipa-prop.h | 8 +-- gcc/ira-build.c| 18 ++--- gcc/ira-color.c| 8 +-- gcc/lra-int.h | 19 +++-- gcc/lra-lives.c| 3 +- gcc/lra.c | 4 +- gcc/regcprop.c | 4 +- gcc/sched-deps.c | 8 +-- gcc/sel-sched-ir.c | 2 +- gcc/sel-sched-ir.h | 2 +- gcc/stmt.c | 7 +- gcc/tree-sra.c | 16 ++---
