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

Michael Buch via lldb-commits lldb-commits at lists.llvm.org
Mon Aug 19 08:44:39 PDT 2024


https://github.com/Michael137 updated https://github.com/llvm/llvm-project/pull/104799

>From 0c23c427f7154dabadbca65f64973ec2dbe365d9 Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Mon, 19 Aug 2024 15:40:07 +0100
Subject: [PATCH 1/5] [lldb][ClangExpressionParser] Don't leak memory when
 multiplexing ExternalASTSources

---
 .../ExpressionParser/Clang/ASTUtils.cpp       | 10 ++++--
 .../Plugins/ExpressionParser/Clang/ASTUtils.h | 31 ++++++++++++++-----
 .../Clang/ClangExpressionParser.cpp           | 16 +++++-----
 3 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp
index a95fce1c5aa966..c1d34c1d519a85 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 79ed74b79f04fd..31345fe35e8621 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 c441153d1efb05..b64613d214e157 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);
     }

>From 9ca4ebc63c723ef670698468095fbf53cb97d0f6 Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Mon, 19 Aug 2024 16:31:52 +0100
Subject: [PATCH 2/5] fixup! remove redundant header

---
 lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
index 31345fe35e8621..46ac5ee42a6dab 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
@@ -14,7 +14,6 @@
 #include "clang/Sema/MultiplexExternalSemaSource.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Sema/SemaConsumer.h"
-#include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include <optional>
 
 namespace clang {

>From da394a71eb7ce9ab05a4adc763d28d1e49c07eac Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Mon, 19 Aug 2024 16:41:16 +0100
Subject: [PATCH 3/5] fixup! make ExternalASTSourceWrapper owning

---
 .../Plugins/ExpressionParser/Clang/ASTUtils.cpp       |  5 -----
 lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h | 11 ++++-------
 .../ExpressionParser/Clang/ClangExpressionParser.cpp  |  4 ++--
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp
index c1d34c1d519a85..9619001be0d9af 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp
@@ -8,11 +8,6 @@
 
 #include "ASTUtils.h"
 
-lldb_private::ExternalASTSourceWrapper::~ExternalASTSourceWrapper() {
-  if (m_owns_source)
-    m_Source->Release();
-}
-
 void lldb_private::ExternalASTSourceWrapper::PrintStats() {
   m_Source->PrintStats();
 }
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
index 46ac5ee42a6dab..4bad9ec8a5baf8 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 {
@@ -26,15 +27,11 @@ namespace lldb_private {
 
 /// 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;
+  llvm::IntrusiveRefCntPtr<ExternalASTSource> m_Source;
 
 public:
-  ExternalASTSourceWrapper(ExternalASTSource *Source, bool owns_source = false)
-      : m_Source(Source), m_owns_source(owns_source) {
+  explicit ExternalASTSourceWrapper(ExternalASTSource *Source)
+      : m_Source(Source) {
     assert(m_Source && "Can't wrap nullptr ExternalASTSource");
   }
 
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index b64613d214e157..ef8f64431f7300 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1221,7 +1221,7 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
     decl_map->InstallCodeGenerator(&m_compiler->getASTConsumer());
     decl_map->InstallDiagnosticManager(diagnostic_manager);
 
-    IntrusiveRefCntPtr<clang::ExternalASTSource> ast_source =
+    clang::ExternalASTSource * ast_source =
         decl_map->CreateProxy();
 
     if (ast_context.getExternalSource()) {
@@ -1229,7 +1229,7 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
           new ExternalASTSourceWrapper(ast_context.getExternalSource());
 
       auto *ast_source_wrapper =
-          new ExternalASTSourceWrapper(ast_source.get(), true);
+          new ExternalASTSourceWrapper(ast_source);
 
       auto *multiplexer =
           new SemaSourceWithPriorities(module_wrapper, ast_source_wrapper);

>From 856e4ae2a101dd4b0141413ce98f9f5481985c64 Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Mon, 19 Aug 2024 16:41:26 +0100
Subject: [PATCH 4/5] fixup! clang-format

---
 .../ExpressionParser/Clang/ClangExpressionParser.cpp        | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index ef8f64431f7300..d37787ebae26c3 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1221,15 +1221,13 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
     decl_map->InstallCodeGenerator(&m_compiler->getASTConsumer());
     decl_map->InstallDiagnosticManager(diagnostic_manager);
 
-    clang::ExternalASTSource * ast_source =
-        decl_map->CreateProxy();
+    clang::ExternalASTSource *ast_source = decl_map->CreateProxy();
 
     if (ast_context.getExternalSource()) {
       auto *module_wrapper =
           new ExternalASTSourceWrapper(ast_context.getExternalSource());
 
-      auto *ast_source_wrapper =
-          new ExternalASTSourceWrapper(ast_source);
+      auto *ast_source_wrapper = new ExternalASTSourceWrapper(ast_source);
 
       auto *multiplexer =
           new SemaSourceWithPriorities(module_wrapper, ast_source_wrapper);

>From 86efdc138c8ac2db91a4a5a322b4fce5a5c9721d Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Mon, 19 Aug 2024 16:43:59 +0100
Subject: [PATCH 5/5] fixup! clang-format and add back destructor

---
 lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp | 2 ++
 lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h   | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp
index 9619001be0d9af..edfea9776cfdbd 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp
@@ -8,6 +8,8 @@
 
 #include "ASTUtils.h"
 
+lldb_private::ExternalASTSourceWrapper::~ExternalASTSourceWrapper() = default;
+
 void lldb_private::ExternalASTSourceWrapper::PrintStats() {
   m_Source->PrintStats();
 }
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
index 4bad9ec8a5baf8..412cb194cc98a3 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
@@ -26,6 +26,8 @@ class Module;
 namespace lldb_private {
 
 /// Wraps an ExternalASTSource into an ExternalSemaSource.
+///
+/// Assumes shared ownership of the underlying source.
 class ExternalASTSourceWrapper : public clang::ExternalSemaSource {
   llvm::IntrusiveRefCntPtr<ExternalASTSource> m_Source;
 
@@ -251,8 +253,7 @@ 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



More information about the lldb-commits mailing list