[PATCH] Inconsistency in Preprocessor::ReleaseMacroInfo(MacroInfo *MI)
Vassil Vassilev
vasil.georgiev.vasilev at cern.ch
Mon Aug 4 02:26:31 PDT 2014
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 <mailto: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 <mailto: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 <mailto: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 <mailto: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
>>>>> <mailto:cfe-commits at cs.uiuc.edu>
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu <mailto: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/14f95989/attachment.html>
More information about the cfe-commits
mailing list