From f0f353b36f173ea282209070fcdbbedab84c19db Mon Sep 17 00:00:00 2001 From: Manuel Klimek Date: Mon, 3 Jun 2013 13:51:33 +0000 Subject: [PATCH] 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 --- include/clang/AST/APValue.h | 7 +++++ include/clang/AST/ASTContext.h | 8 +++-- lib/AST/APValue.cpp | 33 ++++++++++++++++++++ lib/AST/ASTContext.cpp | 12 +++++--- lib/AST/Decl.cpp | 17 ++++++++++- unittests/AST/CMakeLists.txt | 1 + unittests/AST/DeclTest.cpp | 56 ++++++++++++++++++++++++++++++++++ 7 files changed, 125 insertions(+), 9 deletions(-) create mode 100644 unittests/AST/DeclTest.cpp 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, 16> Deallocations; - + typedef llvm::SmallDenseMap > + 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(Data)->hasPathPtr(); + case MemberPointer: + return reinterpret_cast(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(); if (!Eval) { Stmt *S = Init.get(); + // 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(UntypedValue)->~APValue(); +} +} // namespace + APValue *VarDecl::evaluateValue( SmallVectorImpl &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 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 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 struct A : A {};" + "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)); +} -- 2.40.0