Skip to content

Commit 1538d04

Browse files
committed
[lldb][ClangExpressionParser] Don't leak memory when multiplexing ExternalASTSources (llvm#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. (cherry picked from commit 770cd24)
1 parent 7e39cd5 commit 1538d04

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
@@ -15,6 +15,7 @@
1515
#include "clang/Sema/MultiplexExternalSemaSource.h"
1616
#include "clang/Sema/Sema.h"
1717
#include "clang/Sema/SemaConsumer.h"
18+
#include "llvm/ADT/IntrusiveRefCntPtr.h"
1819
#include <optional>
1920

2021
namespace clang {
@@ -25,13 +26,15 @@ class Module;
2526

2627
namespace lldb_private {
2728

28-
/// Wraps an ExternalASTSource into an ExternalSemaSource. Doesn't take
29-
/// ownership of the provided source.
29+
/// Wraps an ExternalASTSource into an ExternalSemaSource.
30+
///
31+
/// Assumes shared ownership of the underlying source.
3032
class ExternalASTSourceWrapper : public ImporterBackedASTSource {
31-
ExternalASTSource *m_Source;
33+
llvm::IntrusiveRefCntPtr<ExternalASTSource> m_Source;
3234

3335
public:
34-
ExternalASTSourceWrapper(ExternalASTSource *Source) : m_Source(Source) {
36+
explicit ExternalASTSourceWrapper(ExternalASTSource *Source)
37+
: m_Source(Source) {
3538
assert(m_Source && "Can't wrap nullptr ExternalASTSource");
3639
}
3740

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

266277
~SemaSourceWithPriorities() override;

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

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

11241124
if (ast_context.getExternalSource()) {
1125-
auto module_wrapper =
1125+
auto *module_wrapper =
11261126
new ExternalASTSourceWrapper(ast_context.getExternalSource());
11271127

1128-
auto ast_source_wrapper = new ExternalASTSourceWrapper(ast_source);
1128+
auto *ast_source_wrapper = new ExternalASTSourceWrapper(ast_source);
11291129

1130-
auto multiplexer =
1131-
new SemaSourceWithPriorities(*module_wrapper, *ast_source_wrapper);
1132-
IntrusiveRefCntPtr<ExternalASTSource> Source(multiplexer);
1133-
ast_context.setExternalSource(Source);
1130+
auto *multiplexer =
1131+
new SemaSourceWithPriorities(module_wrapper, ast_source_wrapper);
1132+
1133+
ast_context.setExternalSource(multiplexer);
11341134
} else {
11351135
ast_context.setExternalSource(ast_source);
11361136
}

0 commit comments

Comments
 (0)