aeubanks created this revision. aeubanks added a reviewer: dblaikie. aeubanks requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits.
Previously without -disable-free, -clear-ast-before-backend would crash in ~ASTContext() due to various reasons. This works around that by doing a lot of the cleanup ahead of the destructor so that the destructor doesn't actually do any manual cleanup if we've already cleaned up beforehand. This actually does save a measurable amount of memory with -clear-ast-before-backend, although at an almost unnoticeable runtime cost: https://llvm-compile-time-tracker.com/compare.php?from=5d755b32f2775b9219f6d6e2feda5e1417dc993b&to=58ef1c7ad7e2ad45f9c97597905a8cf05a26258c&stat=max-rss Previously we weren't doing any cleanup with -disable-free, so I tried measuring the impact of always doing the cleanup and didn't measure anything noticeable on llvm-compile-time-tracker. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D111767 Files: clang/include/clang/AST/ASTContext.h clang/lib/AST/ASTContext.cpp clang/lib/AST/DeclBase.cpp clang/lib/CodeGen/CodeGenAction.cpp Index: clang/lib/CodeGen/CodeGenAction.cpp =================================================================== --- clang/lib/CodeGen/CodeGenAction.cpp +++ clang/lib/CodeGen/CodeGenAction.cpp @@ -351,10 +351,11 @@ } } - // FIXME: Fix cleanup issues with clearing the AST when we properly free - // things. - if (CodeGenOpts.DisableFree && CodeGenOpts.ClearASTBeforeBackend) + if (CodeGenOpts.ClearASTBeforeBackend) { + // The ASTContext may be unusable after this. + C.cleanup(); C.getAllocator().Reset(); + } EmbedBitcode(getModule(), CodeGenOpts, llvm::MemoryBufferRef()); Index: clang/lib/AST/DeclBase.cpp =================================================================== --- clang/lib/AST/DeclBase.cpp +++ clang/lib/AST/DeclBase.cpp @@ -1962,6 +1962,7 @@ // pointer because the subclass doesn't add anything that needs to // be deleted. StoredDeclsMap::DestroyAll(LastSDM.getPointer(), LastSDM.getInt()); + LastSDM.setPointer(nullptr); } void StoredDeclsMap::DestroyAll(StoredDeclsMap *Map, bool Dependent) { Index: clang/lib/AST/ASTContext.cpp =================================================================== --- clang/lib/AST/ASTContext.cpp +++ clang/lib/AST/ASTContext.cpp @@ -990,7 +990,7 @@ addTranslationUnitDecl(); } -ASTContext::~ASTContext() { +void ASTContext::cleanup() { // Release the DenseMaps associated with DeclContext objects. // FIXME: Is this the ideal solution? ReleaseDeclContextMaps(); @@ -998,6 +998,7 @@ // Call all of the deallocation functions on all of their targets. for (auto &Pair : Deallocations) (Pair.first)(Pair.second); + Deallocations.clear(); // ASTRecordLayout objects in ASTRecordLayouts must always be destroyed // because they can contain DenseMaps. @@ -1007,6 +1008,7 @@ // Increment in loop to prevent using deallocated memory. if (auto *R = const_cast<ASTRecordLayout *>((I++)->second)) R->Destroy(*this); + ObjCLayouts.clear(); for (llvm::DenseMap<const RecordDecl*, const ASTRecordLayout*>::iterator I = ASTRecordLayouts.begin(), E = ASTRecordLayouts.end(); I != E; ) { @@ -1014,16 +1016,21 @@ if (auto *R = const_cast<ASTRecordLayout *>((I++)->second)) R->Destroy(*this); } + ASTRecordLayouts.clear(); for (llvm::DenseMap<const Decl*, AttrVec*>::iterator A = DeclAttrs.begin(), AEnd = DeclAttrs.end(); A != AEnd; ++A) A->second->~AttrVec(); + DeclAttrs.clear(); for (const auto &Value : ModuleInitializers) Value.second->~PerModuleInitializers(); + ModuleInitializers.clear(); } +ASTContext::~ASTContext() { cleanup(); } + void ASTContext::setTraversalScope(const std::vector<Decl *> &TopLevelDecls) { TraversalScope = TopLevelDecls; getParentMapContext().clear(); Index: clang/include/clang/AST/ASTContext.h =================================================================== --- clang/include/clang/AST/ASTContext.h +++ clang/include/clang/AST/ASTContext.h @@ -695,6 +695,12 @@ SourceManager& getSourceManager() { return SourceMgr; } const SourceManager& getSourceManager() const { return SourceMgr; } + // Cleans up some of the data structures. This allows us to do cleanup + // normally done in the constructor earlier. May render much of the + // ASTContext unusable so should be called when ASTContext will no longer be + // used. + void cleanup(); + llvm::BumpPtrAllocator &getAllocator() const { return BumpAlloc; }
Index: clang/lib/CodeGen/CodeGenAction.cpp =================================================================== --- clang/lib/CodeGen/CodeGenAction.cpp +++ clang/lib/CodeGen/CodeGenAction.cpp @@ -351,10 +351,11 @@ } } - // FIXME: Fix cleanup issues with clearing the AST when we properly free - // things. - if (CodeGenOpts.DisableFree && CodeGenOpts.ClearASTBeforeBackend) + if (CodeGenOpts.ClearASTBeforeBackend) { + // The ASTContext may be unusable after this. + C.cleanup(); C.getAllocator().Reset(); + } EmbedBitcode(getModule(), CodeGenOpts, llvm::MemoryBufferRef()); Index: clang/lib/AST/DeclBase.cpp =================================================================== --- clang/lib/AST/DeclBase.cpp +++ clang/lib/AST/DeclBase.cpp @@ -1962,6 +1962,7 @@ // pointer because the subclass doesn't add anything that needs to // be deleted. StoredDeclsMap::DestroyAll(LastSDM.getPointer(), LastSDM.getInt()); + LastSDM.setPointer(nullptr); } void StoredDeclsMap::DestroyAll(StoredDeclsMap *Map, bool Dependent) { Index: clang/lib/AST/ASTContext.cpp =================================================================== --- clang/lib/AST/ASTContext.cpp +++ clang/lib/AST/ASTContext.cpp @@ -990,7 +990,7 @@ addTranslationUnitDecl(); } -ASTContext::~ASTContext() { +void ASTContext::cleanup() { // Release the DenseMaps associated with DeclContext objects. // FIXME: Is this the ideal solution? ReleaseDeclContextMaps(); @@ -998,6 +998,7 @@ // Call all of the deallocation functions on all of their targets. for (auto &Pair : Deallocations) (Pair.first)(Pair.second); + Deallocations.clear(); // ASTRecordLayout objects in ASTRecordLayouts must always be destroyed // because they can contain DenseMaps. @@ -1007,6 +1008,7 @@ // Increment in loop to prevent using deallocated memory. if (auto *R = const_cast<ASTRecordLayout *>((I++)->second)) R->Destroy(*this); + ObjCLayouts.clear(); for (llvm::DenseMap<const RecordDecl*, const ASTRecordLayout*>::iterator I = ASTRecordLayouts.begin(), E = ASTRecordLayouts.end(); I != E; ) { @@ -1014,16 +1016,21 @@ if (auto *R = const_cast<ASTRecordLayout *>((I++)->second)) R->Destroy(*this); } + ASTRecordLayouts.clear(); for (llvm::DenseMap<const Decl*, AttrVec*>::iterator A = DeclAttrs.begin(), AEnd = DeclAttrs.end(); A != AEnd; ++A) A->second->~AttrVec(); + DeclAttrs.clear(); for (const auto &Value : ModuleInitializers) Value.second->~PerModuleInitializers(); + ModuleInitializers.clear(); } +ASTContext::~ASTContext() { cleanup(); } + void ASTContext::setTraversalScope(const std::vector<Decl *> &TopLevelDecls) { TraversalScope = TopLevelDecls; getParentMapContext().clear(); Index: clang/include/clang/AST/ASTContext.h =================================================================== --- clang/include/clang/AST/ASTContext.h +++ clang/include/clang/AST/ASTContext.h @@ -695,6 +695,12 @@ SourceManager& getSourceManager() { return SourceMgr; } const SourceManager& getSourceManager() const { return SourceMgr; } + // Cleans up some of the data structures. This allows us to do cleanup + // normally done in the constructor earlier. May render much of the + // ASTContext unusable so should be called when ASTContext will no longer be + // used. + void cleanup(); + llvm::BumpPtrAllocator &getAllocator() const { return BumpAlloc; }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits