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_allocator<asan_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_allocator<T> that will return an >>>> allocated memory (void*) >>>> (w/o calling any construction) >>>> + object_allocator<T>::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_allocator<T> &) >>> >>> I was thinking we'd simply use allocate () for cases where we don't >>> need to pass arguments to the constructor. It looks like et_forest >>> comes into that category. The operator new would be a single function >>> defined in pool-allocator.h for cases where explicit construction >>> is needed. >>> >>> In fact, it looks from a quick grep like all current uses of pool operator >>> new/delete are in POD types, so there are no special constructors. >>> The best example I could come up with was the copy constructor in: >>> >>> return new lra_live_range (*r); >>> >>> which would become: >>> >>> return new (*live_range_pool) lra_live_range (*r); >>> >>> but perhaps we should have an object_allocator copy (T *) routine: >>> >>> return live_range_pool->copy (*r); >>> >>>> + Pool allocators connected to these classes will be back transformed to >>>> static variables and >>>> one would call new et_forest (my_et_forest_allocator) >>> >>> Thanks, this sounds really good to me. Please make sure I'm not the >>> only one who thinks so though :-) >>> >>> I think the "normal" remove () method should then also call the destructor. >>> >>> Thanks, >>> Richard >>> >> >> Hello. >> >> 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. >> >> Thanks, >> Martin > > Hello. > > I would like to ping the patch as it's a blocker for some ppc64 machines.
Ok. Thanks, Richard. > Thanks, > Martin > >