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.
Thanks,
Martin
