[PATCH] Inconsistency in Preprocessor::ReleaseMacroInfo(MacroInfo *MI)

[Yaron Keren]

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.

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.

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.

cling memory use should going up every iteration due to this startegy, no?

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/3ecb5cf3/attachment.html>