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

[Vassil Vassilev]

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/666b97a4/attachment.html>