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

Vassil Vassilev vasil.georgiev.vasilev at cern.ch
Mon Aug 4 03:42:39 PDT 2014


Hi Yaron,
   Thanks for raising those very interesting points!
On 08/04/2014 12:12 PM, Yaron Keren wrote:
> 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.
Understood.
>
> 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.
Yes, I was planning to replace the bump alloc with a slab alloc and 
measure the performance penalty. This is on my todo list since some 
time. What would be your guess?
>
> 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
I couldn't agree more.
>
> 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.
I think using a slab alloc may be not that bad for us.
>
> 2) Freeing the memory slows down performance even more. 
> BumpPtrAllocator free is a no-op.
For our use-cases this is not an issue. This is on the error path, there 
we can be slower. Memory is much much more important.
>
> 3) Actually releasing the memory may cause use-after-free bugs which 
> are not seen now since the AST memory is never really released.
Another issue to tackle. I was thinking to borrow some ideas from llvm 
like AssertingVH to track use-after-delete. Again I need to measure the 
penalties. And codegen cleanup is a monster in this respect. I saw some 
light in the tunnel with the recent changes (haven't updated clang since 
a while).
>
> 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.
This should be fixable, but I agree not easy to debug. AFAIK for AST 
nodes it happens only in the ASTContext.
Vassil

>
> Yaron
>
>
>
>
> 2014-08-04 12:26 GMT+03:00 Vassil Vassilev 
> <vasil.georgiev.vasilev at cern.ch <mailto: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
>>     <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/db75a8ec/attachment.html>


More information about the cfe-commits mailing list