[Lldb-commits] [lldb] [lldb][ClangExpressionParser] Don't leak memory when multiplexing ExternalASTSources (PR #104799)

[via lldb-commits]

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