On Wed, Jun 17, 2015 at 11:13 AM, Martin Jambor <mjam...@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 <mli...@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 <mli...@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