Skip to content

Commit 770cd24

Browse files
authored
[lldb][ClangExpressionParser] Don't leak memory when multiplexing ExternalASTSources (#104799)
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 assumes shared ownership of the underlying source.
1 parent 7aa22f0 commit 770cd24

File tree

3 files changed

+29
-15
lines changed

3 files changed

+29
-15
lines changed

lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@ lldb_private::ASTConsumerForwarder::~ASTConsumerForwarder() = default;
1818

1919
void lldb_private::ASTConsumerForwarder::PrintStats() { m_c->PrintStats(); }
2020

21-
lldb_private::SemaSourceWithPriorities::~SemaSourceWithPriorities() = default;
21+
lldb_private::SemaSourceWithPriorities::~SemaSourceWithPriorities() {
22+
for (auto *Source : Sources)
23+
Source->Release();
24+
}
2225

2326
void lldb_private::SemaSourceWithPriorities::PrintStats() {
2427
for (size_t i = 0; i < Sources.size(); ++i)

lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "clang/Sema/MultiplexExternalSemaSource.h"
1515
#include "clang/Sema/Sema.h"
1616
#include "clang/Sema/SemaConsumer.h"
17+
#include "llvm/ADT/IntrusiveRefCntPtr.h"
1718
#include <optional>
1819

1920
namespace clang {
@@ -24,13 +25,15 @@ class Module;
2425

2526
namespace lldb_private {
2627

27-
/// Wraps an ExternalASTSource into an ExternalSemaSource. Doesn't take
28-
/// ownership of the provided source.
28+
/// Wraps an ExternalASTSource into an ExternalSemaSource.
29+
///
30+
/// Assumes shared ownership of the underlying source.
2931
class ExternalASTSourceWrapper : public clang::ExternalSemaSource {
30-
ExternalASTSource *m_Source;
32+
llvm::IntrusiveRefCntPtr<ExternalASTSource> m_Source;
3133

3234
public:
33-
ExternalASTSourceWrapper(ExternalASTSource *Source) : m_Source(Source) {
35+
explicit ExternalASTSourceWrapper(ExternalASTSource *Source)
36+
: m_Source(Source) {
3437
assert(m_Source && "Can't wrap nullptr ExternalASTSource");
3538
}
3639

@@ -256,10 +259,18 @@ class SemaSourceWithPriorities : public clang::ExternalSemaSource {
256259
/// Construct a SemaSourceWithPriorities with a 'high quality' source that
257260
/// has the higher priority and a 'low quality' source that will be used
258261
/// as a fallback.
259-
SemaSourceWithPriorities(clang::ExternalSemaSource &high_quality_source,
260-
clang::ExternalSemaSource &low_quality_source) {
261-
Sources.push_back(&high_quality_source);
262-
Sources.push_back(&low_quality_source);
262+
///
263+
/// This class assumes shared ownership of the sources provided to it.
264+
SemaSourceWithPriorities(clang::ExternalSemaSource *high_quality_source,
265+
clang::ExternalSemaSource *low_quality_source) {
266+
assert(high_quality_source);
267+
assert(low_quality_source);
268+
269+
high_quality_source->Retain();
270+
low_quality_source->Retain();
271+
272+
Sources.push_back(high_quality_source);
273+
Sources.push_back(low_quality_source);
263274
}
264275

265276
~SemaSourceWithPriorities() override;

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1224,15 +1224,15 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
12241224
clang::ExternalASTSource *ast_source = decl_map->CreateProxy();
12251225

12261226
if (ast_context.getExternalSource()) {
1227-
auto module_wrapper =
1227+
auto *module_wrapper =
12281228
new ExternalASTSourceWrapper(ast_context.getExternalSource());
12291229

1230-
auto ast_source_wrapper = new ExternalASTSourceWrapper(ast_source);
1230+
auto *ast_source_wrapper = new ExternalASTSourceWrapper(ast_source);
12311231

1232-
auto multiplexer =
1233-
new SemaSourceWithPriorities(*module_wrapper, *ast_source_wrapper);
1234-
IntrusiveRefCntPtr<ExternalASTSource> Source(multiplexer);
1235-
ast_context.setExternalSource(Source);
1232+
auto *multiplexer =
1233+
new SemaSourceWithPriorities(module_wrapper, ast_source_wrapper);
1234+
1235+
ast_context.setExternalSource(multiplexer);
12361236
} else {
12371237
ast_context.setExternalSource(ast_source);
12381238
}

0 commit comments

Comments
 (0)