Fix memory leak for APValues that do memory allocation.

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

Original version of the patch by James Dennett.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@183101 91177308-0d34-0410-b5e6-96231b3b80d8

7 files changed, 125 insertions, 9 deletions

diff --git a/include/clang/AST/APValue.h b/include/clang/AST/APValue.h
index ec8faa4e35..b4fd2affa6 100644
--- a/include/clang/AST/APValue.h
+++ b/include/clang/AST/APValue.h
@@ -168,6 +168,13 @@ public:
     MakeUninit();
   }
 
+  /// \brief Returns whether the object performed allocations.
+  ///
+  /// If APValues are constructed via placement new, \c needsCleanup()
+  /// indicates whether the destructor must be called in order to correctly
+  /// free all allocated memory.
+  bool needsCleanup() const;
+
   /// \brief Swaps the contents of this and the given APValue.
   void swap(APValue &RHS);
 
diff --git a/include/clang/AST/ASTContext.h b/include/clang/AST/ASTContext.h
index 88a6297775..0982e757ea 100644
--- a/include/clang/AST/ASTContext.h
+++ b/include/clang/AST/ASTContext.h
@@ -2209,10 +2209,12 @@ private:
                 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.
diff --git a/lib/AST/APValue.cpp b/lib/AST/APValue.cpp
index 98e825b3ba..5cf9942a84 100644
--- a/lib/AST/APValue.cpp
+++ b/lib/AST/APValue.cpp
@@ -212,6 +212,39 @@ void APValue::DestroyDataAndMakeUninit() {
   Kind = Uninitialized;
 }
 
+bool APValue::needsCleanup() const {
+  switch (getKind()) {
+  case Uninitialized:
+  case AddrLabelDiff:
+    return false;
+  case Struct:
+  case Union:
+  case Array:
+  case Vector:
+    return true;
+  case Int:
+    return getInt().needsCleanup();
+  case Float:
+    return getFloat().needsCleanup();
+  case ComplexFloat:
+    assert(getComplexFloatImag().needsCleanup() ==
+               getComplexFloatReal().needsCleanup() &&
+           "In _Complex float types, real and imaginary values always have the "
+           "same size.");
+    return getComplexFloatReal().needsCleanup();
+  case ComplexInt:
+    assert(getComplexIntImag().needsCleanup() ==
+               getComplexIntReal().needsCleanup() &&
+           "In _Complex int types, real and imaginary values must have the "
+           "same size.");
+    return getComplexIntReal().needsCleanup();
+  case LValue:
+    return reinterpret_cast<const LV *>(Data)->hasPathPtr();
+  case MemberPointer:
+    return reinterpret_cast<const MemberPointerData *>(Data)->hasPathPtr();
+  }
+}
+
 void APValue::swap(APValue &RHS) {
   std::swap(Kind, RHS.Kind);
   char TmpData[MaxSize];
diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
index c1f31529b3..333e80be30 100644
--- a/lib/AST/ASTContext.cpp
+++ b/lib/AST/ASTContext.cpp
@@ -732,10 +732,12 @@ ASTContext::~ASTContext() {
   // 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*,
@@ -759,7 +761,7 @@ ASTContext::~ASTContext() {
 }
 
 void ASTContext::AddDeallocation(void (*Callback)(void*), void *Data) {
-  Deallocations.push_back(std::make_pair(Callback, Data));
+  Deallocations[Callback].push_back(Data);
 }
 
 void
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
index 7ae7e8209d..80a32c5870 100644
--- a/lib/AST/Decl.cpp
+++ b/lib/AST/Decl.cpp
@@ -1827,6 +1827,10 @@ EvaluatedStmt *VarDecl::ensureEvaluatedStmt() const {
   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;
@@ -1839,6 +1843,13 @@ APValue *VarDecl::evaluateValue() const {
   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();
@@ -1864,9 +1875,13 @@ APValue *VarDecl::evaluateValue(
   bool Result = Init->EvaluateAsInitializer(Eval->Evaluated, getASTContext(),
                                             this, Notes);
 
-  // Ensure the result is an uninitialized APValue if evaluation fails.
+  // 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)
     Eval->Evaluated = APValue();
+  else if (Eval->Evaluated.needsCleanup())
+    getASTContext().AddDeallocation(DestroyAPValue, &Eval->Evaluated);
 
   Eval->IsEvaluating = false;
   Eval->WasEvaluated = true;
diff --git a/unittests/AST/CMakeLists.txt b/unittests/AST/CMakeLists.txt
index ad29428220..3ef2a5e153 100644
--- a/unittests/AST/CMakeLists.txt
+++ b/unittests/AST/CMakeLists.txt
@@ -3,6 +3,7 @@ add_clang_unittest(ASTTests
   CommentLexer.cpp
   CommentParser.cpp
   DeclPrinterTest.cpp
+  DeclTest.cpp
   SourceLocationTest.cpp
   StmtPrinterTest.cpp
   )
diff --git a/unittests/AST/DeclTest.cpp b/unittests/AST/DeclTest.cpp
new file mode 100644
index 0000000000..ff7c30cbee
--- /dev/null
+++ b/unittests/AST/DeclTest.cpp
@@ -0,0 +1,56 @@
+//===- 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 };"
+      "union Y { constexpr Y(int a) : a(a) {} int a; }; constexpr Y y = { 42 };"
+      "constexpr int z[2] = { 42, 43 };"
+      "constexpr int __attribute__((vector_size(16))) v1 = {};"
+      "constexpr __uint128_t large_int = 0xffffffffffffffff;"
+      "constexpr __uint128_t small_int = 1;"
+      "constexpr double d1 = 42.42;"
+      "constexpr long double d2 = 42.42;"
+      "constexpr _Complex long double c1 = 42.0i;"
+      "constexpr _Complex long double c2 = 42.0;"
+      "template<int N> struct A : A<N-1> {};"
+      "template<> struct A<0> { int n; }; A<50> a;"
+      "constexpr int &r = a.n;"
+      "constexpr int A<50>::*p = &A<50>::n;"
+      "void f() { foo: bar: constexpr int k = __builtin_constant_p(0) ?"
+      "                         (char*)&&foo - (char*)&&bar : 0; }",
+      Args));
+
+  // FIXME: Once this test starts breaking we can test APValue::needsCleanup
+  // for ComplexInt.
+  ASSERT_FALSE(runToolOnCodeWithArgs(
+      Factory->create(),
+      "constexpr _Complex __uint128_t c = 0xffffffffffffffff;",
+      Args));
+}