[PATCH] Inconsistency in Preprocessor::ReleaseMacroInfo(MacroInfo *MI)
Yaron Keren
yaron.keren at gmail.com
Mon Aug 4 03:12:30 PDT 2014
Hi,
You should not destruct SmallVector explicitly as its destructor will be
called from ~MacroInfo from ~MacroInfoChain from ~Preprocessor. If you want
to reclaim the Tokens memory use in MacroInfo use you need to do as before,
call ~MacroInfo and remove it from the chain.
The source of the memory usage comes much more from AST memory "leakage"
(leakage in the sense the memory is not freed until destruction which does
not happen in cling) and other allocations all around the code rather than
the bit of memory lost to the macros.
I have looked into this issue for Ceemple which has similar need as cling
for memory reclaim and gave it up for now.
It's actually quite hard to make clang reclaim memory before destruction,
since
1) BumpPtrAllocator does not reuse free memory. Could be replaced by a
MallocAllocator or other custom allocation but this would reduce
compilation performance. It's hard to compete with BumpPtrAllocator
performance.
2) Freeing the memory slows down performance even more. BumpPtrAllocator
free is a no-op.
3) Actually releasing the memory may cause use-after-free bugs which are
not seen now since the AST memory is never really released.
4) BumpPtrAllocator is used everywhere, sometimes without calling the
no-op free, so even with a real allocator there would still be leaks
(meaning non-reused memory, in destruction all is freed to the system)
unless every allocation is matched with a free.
Yaron
2014-08-04 12:26 GMT+03:00 Vassil Vassilev <vasil.georgiev.vasilev at cern.ch>:
> Hi Yaron,
>
> On 08/04/2014 10:31 AM, Yaron Keren wrote:
>
> Hi Vassil,
>
> If you decide to keep the last code posted as a cling patch, it could do
> with 'I' only instead of 'I' and 'current', and when MI is the first node
> the code should set MIChainHead but not set its Next.
>
> Thanks for pointing out, will do.
>
>
> To the point, ReleaseMacroInfo just releases the SmallVector Tokens
> memory if it wasn't small.
> It did not modify anything else. You could still removeMacro without
> ReleaseMacroInfo.
>
> Thanks for explaining. My code looks like this:
>
> void Preprocessor::removeMacro(IdentifierInfo *II, const MacroDirective
> *MD) {
> assert(II && MD);
> assert(!MD->getPrevious() && "Already attached to a MacroDirective
> history.");
>
> //Release the MacroInfo allocated space so it can be reused.
> MacroInfo* MI = MD->getMacroInfo();
> if (MI) {
> ReleaseMacroInfo(MI);
> }
> Macros.erase(II);
> }
>
> IIUC I need to check if the small vector isSmall and if not then do a
> ReleaseMacro, or even this is redundant?
>
>
> There's lots of places in clang where memory is allocated and not
> released until destruction for performance.
> The whole AST for starters...
>
>
> It would be nice to early release the Tokens but In this context it
> would hardly move the needle.
>
> I agree. So I need to somehow implement it.
>
>
> cling memory use should going up every iteration due to this startegy,
> no?
>
> Yes, it grows. The context I want things removed is support of 'code
> unloading'. Say:
> [cling] #include "MyFile.h"
> [cling] MyClass m; m.do();
> // Figure out that do is not what I want. I edit the file and do:
> [cling] #include "MyFile.h" // It would undo everything up to #include
> "MyFile.h" (inclusively). I want the memory to be reduced also. This is why
> I need to delete the macros and not only undef them. (The same holds for
> the AST)
> [cling] MyClass m; m.do(); // Here do and MyClass may have completely
> different implementation.
>
> Vassil
>
>
> Yaron
>
>
>
>
>
> 2014-08-04 10:47 GMT+03:00 Vassil Vassilev <
> vasil.georgiev.vasilev at cern.ch>:
>
>> Hi Richard,
>> Thanks for the fix!
>>
>> Unfortunately it doesn't help for cling case. I implement a removeMacro
>> routine using ReleaseMacroInfo. ReleaseMacroInfo allowed me to implement
>> efficiently the removal of a macro instead of dragging a long def undef
>> chains, for example.
>> IIUC it allowed some memory reduction in some cases for clang, too. Is
>> there any chance to keep the ReleaseMacroInfo upstream?
>> Vassil
>>
>> On 08/04/2014 01:50 AM, Richard Smith wrote:
>>
>> Fixed in a much more simple way in r214675. Thanks for reporting!
>>
>>
>> On Sun, Aug 3, 2014 at 11:52 AM, Vassil Vassilev <vvasilev at cern.ch>
>> wrote:
>>
>>> I will try just one more time and then shut up :)
>>>
>>>
>>> diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp
>>> index 5f38387..000ea7a 100644
>>> --- a/lib/Lex/PPDirectives.cpp
>>> +++ b/lib/Lex/PPDirectives.cpp
>>> @@ -94,6 +94,19 @@
>>> Preprocessor::AllocateVisibilityMacroDirective(SourceLocation Loc,
>>> /// error in the macro definition.
>>> void Preprocessor::ReleaseMacroInfo(MacroInfo *MI) {
>>> // Don't try to reuse the storage; this only happens on error paths.
>>> +
>>> + // If this is on the macro info chain, avoid double deletion on
>>> teardown.
>>> + MacroInfoChain *current = MIChainHead;
>>> + while (MacroInfoChain *I = current) {
>>> + if (&(I->MI) == MI) {
>>> + I->Next = (I->Next) ? I->Next->Next : 0;
>>> + if (I == MIChainHead)
>>> + MIChainHead = I->Next;
>>>
>>> + break;
>>> + }
>>> + current = I->Next;
>>> + }
>>> +
>>> MI->~MacroInfo();
>>> }
>>>
>>>
>>> On 03/08/14 20:47, Vassil Vassilev wrote:
>>>
>>> Hi,
>>> Sorry overlooked, thanks for pointing it out!
>>> I hope this is what we want.
>>> Vassil
>>>
>>> diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp
>>> index 5f38387..000ea7a 100644
>>> --- a/lib/Lex/PPDirectives.cpp
>>> +++ b/lib/Lex/PPDirectives.cpp
>>> @@ -94,6 +94,19 @@
>>> Preprocessor::AllocateVisibilityMacroDirective(SourceLocation Loc,
>>> /// error in the macro definition.
>>> void Preprocessor::ReleaseMacroInfo(MacroInfo *MI) {
>>> // Don't try to reuse the storage; this only happens on error paths.
>>> +
>>> + // If this is on the macro info chain, avoid double deletion on
>>> teardown.
>>> + MacroInfoChain *current = MIChainHead;
>>> + while (MacroInfoChain *I = current) {
>>> + if (&(I->MI) == MI) {
>>> + I->Next = (I->Next) ? I->Next->Next : 0;
>>> + if (I == MIChainHead)
>>> + MIChainHead = I;
>>> + break;
>>> + }
>>> + current = I->Next;
>>> + }
>>> +
>>> MI->~MacroInfo();
>>> }
>>>
>>> On 03/08/14 20:28, Yaron Keren wrote:
>>>
>>> Hi,
>>>
>>> MIChainHead is a pointer to the head of a linked list
>>> of MacroInfoChain nodes, each containing a MacroInfo and MacroInfoChain*.
>>>
>>> Why does the while loop modify MIChainHead on every iteration?
>>> MIChainHead should be modified only if it points to the node containing
>>> the removed MacroInfo MI. In all other cases it should not change.
>>>
>>> As it is now, the loop will always terminate with MIChainHead ==
>>> nullptr.
>>>
>>> Yaron
>>>
>>>
>>>
>>> 2014-08-03 21:10 GMT+03:00 Vassil Vassilev <vvasilev at cern.ch>:
>>>
>>>> Hi Yaron,
>>>> Yes I meant double destruction.
>>>> Vassil
>>>>
>>>> On 03/08/14 20:08, Yaron Keren wrote:
>>>>
>>>> Hi Vassil,
>>>>
>>>> Do you mean double destruction (not deletion) of MacroInfo first time
>>>> in ReleaseMacroInfo and the second time in ~Preprocessor via
>>>> ~MacroInfoChain?
>>>>
>>>> while (MacroInfoChain *I = MIChainHead) {
>>>> MIChainHead = I->Next;
>>>> I->~MacroInfoChain();
>>>> }
>>>>
>>>> or something else?
>>>>
>>>> Yaron
>>>>
>>>>
>>>>
>>>> 2014-08-02 23:05 GMT+03:00 Vassil Vassilev <vvasilev at cern.ch>:
>>>>
>>>>> Hi,
>>>>> In cases where ReleaseMacroInfo gets called and it doesn't cleanup
>>>>> the Preprocessor's MIChainHead can lead to double deletion. I am sending
>>>>> the patch that fixes the problem for me.
>>>>> Vassil
>>>>>
>>>>>
>>>>> diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp
>>>>> index 5f38387..1a9b5eb 100644
>>>>> --- a/lib/Lex/PPDirectives.cpp
>>>>> +++ b/lib/Lex/PPDirectives.cpp
>>>>> @@ -94,6 +94,14 @@
>>>>> Preprocessor::AllocateVisibilityMacroDirective(SourceLocation Loc,
>>>>> /// error in the macro definition.
>>>>> void Preprocessor::ReleaseMacroInfo(MacroInfo *MI) {
>>>>> // Don't try to reuse the storage; this only happens on error paths.
>>>>> +
>>>>> + // If this is on the macro info chain, avoid double deletion on
>>>>> teardown.
>>>>> + while (MacroInfoChain *I = MIChainHead) {
>>>>> + if (&(I->MI) == MI)
>>>>> + I->Next = (I->Next) ? I->Next->Next : 0;
>>>>> + MIChainHead = I->Next;
>>>>> + }
>>>>> +
>>>>> MI->~MacroInfo();
>>>>> }
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing listcfe-commits at cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140804/7e4221b6/attachment.html>
More information about the cfe-commits
mailing list