[cfe-commits] r95861 - in /cfe/trunk: include/clang/AST/ASTContext.h lib/AST/ASTContext.cpp lib/AST/DeclBase.cpp

[Daniel Dunbar]

On Wed, Feb 10, 2010 at 11:12 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Author: kremenek
> Date: Thu Feb 11 01:12:28 2010
> New Revision: 95861
>
> URL: http://llvm.org/viewvc/llvm-project?rev=95861&view=rev
> Log:
> Have ~ASTContext() delete StoredDeclsMap (internal to DeclContext) by
> storing the set of StoredDeclsMaps in an internal vector of void*.
> This isn't an ideal solution, but for the time being this fixes a
> major memory leak with these DenseMaps not being freed.
>
> Fixes: <rdar://problem/7634755>
>
>
> Modified:
>    cfe/trunk/include/clang/AST/ASTContext.h
>    cfe/trunk/lib/AST/ASTContext.cpp
>    cfe/trunk/lib/AST/DeclBase.cpp
>
> Modified: cfe/trunk/include/clang/AST/ASTContext.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=95861&r1=95860&r2=95861&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/AST/ASTContext.h (original)
> +++ cfe/trunk/include/clang/AST/ASTContext.h Thu Feb 11 01:12:28 2010
> @@ -46,6 +46,7 @@
>   class SourceManager;
>   class TargetInfo;
>   // Decls
> +  class DeclContext;
>   class CXXMethodDecl;
>   class CXXRecordDecl;
>   class Decl;
> @@ -1212,6 +1213,15 @@
>
>   const ASTRecordLayout &getObjCLayout(const ObjCInterfaceDecl *D,
>                                        const ObjCImplementationDecl *Impl);
> +
> +private:
> +  // 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.
> +  std::vector<void*> SDMs;
> +  friend class DeclContext;
> +  void *CreateStoredDeclsMap();
> +  void ReleaseDeclContextMaps();
>  };
>
>  /// @brief Utility function for constructing a nullary selector.
>
> Modified: cfe/trunk/lib/AST/ASTContext.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=95861&r1=95860&r2=95861&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/AST/ASTContext.cpp (original)
> +++ cfe/trunk/lib/AST/ASTContext.cpp Thu Feb 11 01:12:28 2010
> @@ -56,6 +56,10 @@
>  }
>
>  ASTContext::~ASTContext() {
> +  // Release the DenseMaps associated with DeclContext objects.
> +  // FIXME: Is this the ideal solution?
> +  ReleaseDeclContextMaps();
> +
>   if (FreeMemory) {
>     // Deallocate all the types.
>     while (!Types.empty()) {
>
> Modified: cfe/trunk/lib/AST/DeclBase.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclBase.cpp?rev=95861&r1=95860&r2=95861&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/AST/DeclBase.cpp (original)
> +++ cfe/trunk/lib/AST/DeclBase.cpp Thu Feb 11 01:12:28 2010
> @@ -448,7 +448,10 @@
>  }
>
>  DeclContext::~DeclContext() {
> -  delete static_cast<StoredDeclsMap*>(LookupPtr);
> +  // FIXME: Currently ~ASTContext will delete the StoredDeclsMaps because
> +  // ~DeclContext() is not guaranteed to be called when ASTContext uses
> +  // a BumpPtrAllocator.
> +  // delete static_cast<StoredDeclsMap*>(LookupPtr);
>  }
>
>  void DeclContext::DestroyDecls(ASTContext &C) {
> @@ -622,7 +625,8 @@
>   // Load the declaration IDs for all of the names visible in this
>   // context.
>   assert(!LookupPtr && "Have a lookup map before de-serialization?");
> -  StoredDeclsMap *Map = new StoredDeclsMap;
> +  StoredDeclsMap *Map =
> +    (StoredDeclsMap*) getParentASTContext().CreateStoredDeclsMap();
>   LookupPtr = Map;
>   for (unsigned I = 0, N = Decls.size(); I != N; ++I) {
>     (*Map)[Decls[I].Name].setFromDeclIDs(Decls[I].Declarations);
> @@ -830,8 +834,11 @@
>   if (isa<ClassTemplateSpecializationDecl>(D))
>     return;
>
> -  if (!LookupPtr)
> -    LookupPtr = new StoredDeclsMap;
> +  ASTContext *C = 0;
> +  if (!LookupPtr) {
> +    C = &getParentASTContext();

Shouldn't this just be
> +  ASTContext &C = getParentASTContext();
and remove the duplicated condition below?

 - Daniel

> +    LookupPtr = (StoredDeclsMap*) C->CreateStoredDeclsMap();
> +  }
>
>   // Insert this declaration into the map.
>   StoredDeclsMap &Map = *static_cast<StoredDeclsMap*>(LookupPtr);
> @@ -844,7 +851,10 @@
>   // If it is possible that this is a redeclaration, check to see if there is
>   // already a decl for which declarationReplaces returns true.  If there is
>   // one, just replace it and return.
> -  if (DeclNameEntries.HandleRedeclaration(getParentASTContext(), D))
> +  if (!C)
> +    C = &getParentASTContext();
> +
> +  if (DeclNameEntries.HandleRedeclaration(*C, D))
>     return;
>
>   // Put this declaration into the appropriate slot.
> @@ -896,3 +906,18 @@
>   }
>   }
>  }
> +
> +//===----------------------------------------------------------------------===//
> +// Creation and Destruction of StoredDeclsMaps.                               //
> +//===----------------------------------------------------------------------===//
> +
> +void *ASTContext::CreateStoredDeclsMap() {
> +  StoredDeclsMap *M = new StoredDeclsMap();
> +  SDMs.push_back(M);
> +  return M;
> +}
> +
> +void ASTContext::ReleaseDeclContextMaps() {
> +  for (std::vector<void*>::iterator I = SDMs.begin(), E = SDMs.end(); I!=E; ++I)
> +    delete (StoredDeclsMap*) *I;
> +}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>