[PATCH] Fix memory leak for APValues that do memory allocation.

[Manuel Klimek]

This patch ensures that APValues are deallocated with the ASTContext
by registering a deallocation function for APValues to the ASTContext.

This patch was written by James Dennett, and I'm taking it over
as this currently provides problems for tools.

http://llvm-reviews.chandlerc.com/D736

Files:
  include/clang/AST/ASTContext.h
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp

Index: include/clang/AST/ASTContext.h
===================================================================
--- include/clang/AST/ASTContext.h
+++ include/clang/AST/ASTContext.h
@@ -2186,10 +2186,12 @@
                 const ObjCImplementationDecl *Impl) const;
 
 private:
-  /// \brief A set of deallocations that should be performed when the 
+  /// \brief A set of deallocations that should be performed when the
   /// ASTContext is destroyed.
-  SmallVector<std::pair<void (*)(void*), void *>, 16> Deallocations;
-                                       
+  typedef llvm::SmallDenseMap<void(*)(void*), llvm::SmallVector<void*, 16> >
+    DeallocationMap;
+  DeallocationMap Deallocations;
+
   // FIXME: This currently contains the set of StoredDeclMaps used
   // by DeclContext objects.  This probably should not be in ASTContext,
   // but we include it here so that ASTContext can quickly deallocate them.
Index: lib/AST/ASTContext.cpp
===================================================================
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -726,10 +726,12 @@
   // FIXME: Is this the ideal solution?
   ReleaseDeclContextMaps();
 
-  // Call all of the deallocation functions.
-  for (unsigned I = 0, N = Deallocations.size(); I != N; ++I)
-    Deallocations[I].first(Deallocations[I].second);
-  
+  // Call all of the deallocation functions on all of their targets.
+  for (DeallocationMap::const_iterator I = Deallocations.begin(),
+           E = Deallocations.end(); I != E; ++I)
+    for (unsigned J = 0, N = I->second.size(); J != N; ++J)
+      (I->first)((I->second)[J]);
+
   // ASTRecordLayout objects in ASTRecordLayouts must always be destroyed
   // because they can contain DenseMaps.
   for (llvm::DenseMap<const ObjCContainerDecl*,
@@ -753,7 +755,16 @@
 }
 
 void ASTContext::AddDeallocation(void (*Callback)(void*), void *Data) {
-  Deallocations.push_back(std::make_pair(Callback, Data));
+  // TODO(jdennett): Optimize the naive code to avoid temporary vectors
+  // for the common case where the Callback is already present.  The
+  // assumption is that few distinct callbacks are used.
+  // SmallDenseMap<void (*)(void*), void*>::const_iterator I =
+  //   Deallocations.find(Callback);
+  // if (I == Deallocations.end())
+  //   Deallocations[Callback].push_back(Data);
+  // else
+  //   I->second.push_back(Data);
+  Deallocations[Callback].push_back(Data);
 }
 
 void
Index: lib/AST/Decl.cpp
===================================================================
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -1742,6 +1742,10 @@
   EvaluatedStmt *Eval = Init.dyn_cast<EvaluatedStmt *>();
   if (!Eval) {
     Stmt *S = Init.get<Stmt *>();
+    // Note: EvaluatedStmt contains an APValue, which usually holds
+    // resources not allocated from the ASTContext.  We need to do some
+    // work to avoid leaking those, but we do so in VarDecl::evaluateValue
+    // where we can detect whether there's anything to clean up or not.
     Eval = new (getASTContext()) EvaluatedStmt;
     Eval->Value = S;
     Init = Eval;
@@ -1754,6 +1758,13 @@
   return evaluateValue(Notes);
 }
 
+namespace {
+// Destroy an APValue that was allocated in an ASTContext.
+void DestroyAPValue(void* UntypedValue) {
+  static_cast<APValue*>(UntypedValue)->~APValue();
+}
+} // namespace
+
 APValue *VarDecl::evaluateValue(
     SmallVectorImpl<PartialDiagnosticAt> &Notes) const {
   EvaluatedStmt *Eval = ensureEvaluatedStmt();
@@ -1779,8 +1790,12 @@
   bool Result = Init->EvaluateAsInitializer(Eval->Evaluated, getASTContext(),
                                             this, Notes);
 
-  // Ensure the result is an uninitialized APValue if evaluation fails.
-  if (!Result)
+  // Ensure the computed APValue is cleaned up later if evaluation succeeded,
+  // or that it's empty (so that there's nothing to clean up) if evaluation
+  // failed.
+  if (Result)
+    getASTContext().AddDeallocation(DestroyAPValue, &Eval->Evaluated);
+  else
     Eval->Evaluated = APValue();
 
   Eval->IsEvaluating = false;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D736.1.patch
Type: text/x-patch
Size: 4045 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130503/3c809c2f/attachment.bin>