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

[Manuel Klimek]

  - removed unnecessary todo
  - added a test that fails under valgrind

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

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D736?vs=1810&id=1837#toc

Files:
  include/clang/AST/ASTContext.h
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  unittests/AST/CMakeLists.txt
  unittests/AST/DeclTest.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,7 @@
 }
 
 void ASTContext::AddDeallocation(void (*Callback)(void*), void *Data) {
-  Deallocations.push_back(std::make_pair(Callback, 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;
Index: unittests/AST/CMakeLists.txt
===================================================================
--- unittests/AST/CMakeLists.txt
+++ unittests/AST/CMakeLists.txt
@@ -3,6 +3,7 @@
   CommentLexer.cpp
   CommentParser.cpp
   DeclPrinterTest.cpp
+  DeclTest.cpp
   SourceLocationTest.cpp
   StmtPrinterTest.cpp
   )
Index: unittests/AST/DeclTest.cpp
===================================================================
--- /dev/null
+++ unittests/AST/DeclTest.cpp
@@ -0,0 +1,33 @@
+//===- unittests/AST/DeclTest.cpp --- Declaration tests -------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Unit tests for Decl nodes in the AST.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+using namespace clang::ast_matchers;
+using namespace clang::tooling;
+
+TEST(Decl, CleansUpAPValues) {
+  MatchFinder Finder;
+  llvm::OwningPtr<FrontendActionFactory> Factory(
+      newFrontendActionFactory(&Finder));
+
+  // This is a regression test for a memory leak in APValues for structs that
+  // allocate memory. This test only fails if run under valgrind with full leak
+  // checking enabled.
+  std::vector<std::string> Args(1, "-std=c++11");
+  ASSERT_TRUE(runToolOnCodeWithArgs(
+      Factory->create(), "struct X { int a; }; constexpr X x = { 42 }; ",
+      Args));
+}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D736.2.patch
Type: text/x-patch
Size: 5278 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130506/49db88b8/attachment.bin>