r183101 - Fix memory leak for APValues that do memory allocation.
Manuel Klimek
klimek at google.com
Mon Jun 3 06:51:33 PDT 2013
Author: klimek
Date: Mon Jun 3 08:51:33 2013
New Revision: 183101
URL: http://llvm.org/viewvc/llvm-project?rev=183101&view=rev
Log:
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.
Added:
cfe/trunk/unittests/AST/DeclTest.cpp
Modified:
cfe/trunk/include/clang/AST/APValue.h
cfe/trunk/include/clang/AST/ASTContext.h
cfe/trunk/lib/AST/APValue.cpp
cfe/trunk/lib/AST/ASTContext.cpp
cfe/trunk/lib/AST/Decl.cpp
cfe/trunk/unittests/AST/CMakeLists.txt
Modified: cfe/trunk/include/clang/AST/APValue.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/APValue.h?rev=183101&r1=183100&r2=183101&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/APValue.h (original)
+++ cfe/trunk/include/clang/AST/APValue.h Mon Jun 3 08:51:33 2013
@@ -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);
Modified: cfe/trunk/include/clang/AST/ASTContext.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=183101&r1=183100&r2=183101&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/ASTContext.h (original)
+++ cfe/trunk/include/clang/AST/ASTContext.h Mon Jun 3 08:51:33 2013
@@ -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.
Modified: cfe/trunk/lib/AST/APValue.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/APValue.cpp?rev=183101&r1=183100&r2=183101&view=diff
==============================================================================
--- cfe/trunk/lib/AST/APValue.cpp (original)
+++ cfe/trunk/lib/AST/APValue.cpp Mon Jun 3 08:51:33 2013
@@ -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];
Modified: cfe/trunk/lib/AST/ASTContext.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=183101&r1=183100&r2=183101&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ASTContext.cpp (original)
+++ cfe/trunk/lib/AST/ASTContext.cpp Mon Jun 3 08:51:33 2013
@@ -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
Modified: cfe/trunk/lib/AST/Decl.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=183101&r1=183100&r2=183101&view=diff
==============================================================================
--- cfe/trunk/lib/AST/Decl.cpp (original)
+++ cfe/trunk/lib/AST/Decl.cpp Mon Jun 3 08:51:33 2013
@@ -1827,6 +1827,10 @@ EvaluatedStmt *VarDecl::ensureEvaluatedS
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;
Modified: cfe/trunk/unittests/AST/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/CMakeLists.txt?rev=183101&r1=183100&r2=183101&view=diff
==============================================================================
--- cfe/trunk/unittests/AST/CMakeLists.txt (original)
+++ cfe/trunk/unittests/AST/CMakeLists.txt Mon Jun 3 08:51:33 2013
@@ -3,6 +3,7 @@ add_clang_unittest(ASTTests
CommentLexer.cpp
CommentParser.cpp
DeclPrinterTest.cpp
+ DeclTest.cpp
SourceLocationTest.cpp
StmtPrinterTest.cpp
)
Added: cfe/trunk/unittests/AST/DeclTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/DeclTest.cpp?rev=183101&view=auto
==============================================================================
--- cfe/trunk/unittests/AST/DeclTest.cpp (added)
+++ cfe/trunk/unittests/AST/DeclTest.cpp Mon Jun 3 08:51:33 2013
@@ -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));
+}
More information about the cfe-commits
mailing list