llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) <details> <summary>Changes</summary> When we use `SemaSourceWithPriorities` as the `ASTContext`s ExternalASTSource, we allocate a `ClangASTSourceProxy` (via `CreateProxy`) and two `ExternalASTSourceWrapper`. Then we push these sources into a vector in `SemaSourceWithPriorities`. The allocated `SemaSourceWithPriorities` itself will get properly deallocated because the `ASTContext` wraps it in an `IntrusiveRefCntPtr`. But the three sources we allocated earlier will never get released. This patch fixes this by mimicking what `MultiplexExternalSemaSource` does (which is what `SemaSourceWithPriorities` is based on anyway). I.e., when `SemaSourceWithPriorities` gets constructed, it increments the use count of its sources. And on destruction it decrements them. Similarly, to make sure we dealloacted the `ClangASTProxy` properly, the `ExternalASTSourceWrapper` now optionally owns the source that it wraps. We might be able to get away with having `ExternalASTSourceWrapper` increment/decrement the use-count of the source, but it's not entirely clear to me why this class wanted to avoid sharing ownership of the source, as a first measure, I opted for adding an `owns_source` flag. --- Full diff: https://github.com/llvm/llvm-project/pull/104799.diff 3 Files Affected: - (modified) lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp (+8-2) - (modified) lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h (+23-8) - (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+9-7) ``````````diff diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp index a95fce1c5aa96..c1d34c1d519a8 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp @@ -8,7 +8,10 @@ #include "ASTUtils.h" -lldb_private::ExternalASTSourceWrapper::~ExternalASTSourceWrapper() = default; +lldb_private::ExternalASTSourceWrapper::~ExternalASTSourceWrapper() { + if (m_owns_source) + m_Source->Release(); +} void lldb_private::ExternalASTSourceWrapper::PrintStats() { m_Source->PrintStats(); @@ -18,7 +21,10 @@ lldb_private::ASTConsumerForwarder::~ASTConsumerForwarder() = default; void lldb_private::ASTConsumerForwarder::PrintStats() { m_c->PrintStats(); } -lldb_private::SemaSourceWithPriorities::~SemaSourceWithPriorities() = default; +lldb_private::SemaSourceWithPriorities::~SemaSourceWithPriorities() { + for (auto * Source : Sources) + Source->Release(); +} void lldb_private::SemaSourceWithPriorities::PrintStats() { for (size_t i = 0; i < Sources.size(); ++i) diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h index 79ed74b79f04f..31345fe35e862 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h +++ b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h @@ -14,6 +14,7 @@ #include "clang/Sema/MultiplexExternalSemaSource.h" #include "clang/Sema/Sema.h" #include "clang/Sema/SemaConsumer.h" +#include "llvm/ADT/IntrusiveRefCntPtr.h" #include <optional> namespace clang { @@ -24,13 +25,17 @@ class Module; namespace lldb_private { -/// Wraps an ExternalASTSource into an ExternalSemaSource. Doesn't take -/// ownership of the provided source. +/// Wraps an ExternalASTSource into an ExternalSemaSource. class ExternalASTSourceWrapper : public clang::ExternalSemaSource { ExternalASTSource *m_Source; + + ///< If true, means that this class is responsible for + ///< decrementing the use count of m_Source. + bool m_owns_source = false; public: - ExternalASTSourceWrapper(ExternalASTSource *Source) : m_Source(Source) { + ExternalASTSourceWrapper(ExternalASTSource *Source, bool owns_source = false) + : m_Source(Source), m_owns_source(owns_source) { assert(m_Source && "Can't wrap nullptr ExternalASTSource"); } @@ -250,16 +255,26 @@ class SemaSourceWithPriorities : public clang::ExternalSemaSource { private: /// The sources ordered in decreasing priority. - llvm::SmallVector<clang::ExternalSemaSource *, 2> Sources; + llvm::SmallVector<clang::ExternalSemaSource*, 2> + Sources; public: /// Construct a SemaSourceWithPriorities with a 'high quality' source that /// has the higher priority and a 'low quality' source that will be used /// as a fallback. - SemaSourceWithPriorities(clang::ExternalSemaSource &high_quality_source, - clang::ExternalSemaSource &low_quality_source) { - Sources.push_back(&high_quality_source); - Sources.push_back(&low_quality_source); + /// + /// This class assumes shared ownership of the sources provided to it. + SemaSourceWithPriorities( + clang::ExternalSemaSource * high_quality_source, + clang::ExternalSemaSource * low_quality_source) { + assert(high_quality_source); + assert(low_quality_source); + + high_quality_source->Retain(); + low_quality_source->Retain(); + + Sources.push_back(high_quality_source); + Sources.push_back(low_quality_source); } ~SemaSourceWithPriorities() override; diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp index c441153d1efb0..b64613d214e15 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp @@ -1221,18 +1221,20 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager, decl_map->InstallCodeGenerator(&m_compiler->getASTConsumer()); decl_map->InstallDiagnosticManager(diagnostic_manager); - clang::ExternalASTSource *ast_source = decl_map->CreateProxy(); + IntrusiveRefCntPtr<clang::ExternalASTSource> ast_source = + decl_map->CreateProxy(); if (ast_context.getExternalSource()) { - auto module_wrapper = + auto *module_wrapper = new ExternalASTSourceWrapper(ast_context.getExternalSource()); - auto ast_source_wrapper = new ExternalASTSourceWrapper(ast_source); + auto *ast_source_wrapper = + new ExternalASTSourceWrapper(ast_source.get(), true); - auto multiplexer = - new SemaSourceWithPriorities(*module_wrapper, *ast_source_wrapper); - IntrusiveRefCntPtr<ExternalASTSource> Source(multiplexer); - ast_context.setExternalSource(Source); + auto *multiplexer = + new SemaSourceWithPriorities(module_wrapper, ast_source_wrapper); + + ast_context.setExternalSource(multiplexer); } else { ast_context.setExternalSource(ast_source); } `````````` </details> https://github.com/llvm/llvm-project/pull/104799 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
