Skip to content

Commit cbcf17f

Browse files
committed
Stop leaking memory from Module and FileUnit.
Also, disallow creating Modules and FileUnits on the stack. They must always live as long as the ASTContext. <rdar://problem/15596964> Swift SVN r13671
1 parent 1e3fd1a commit cbcf17f

File tree

11 files changed

+37
-14
lines changed

11 files changed

+37
-14
lines changed

Diff for: include/swift/AST/ASTContext.h

+7
Original file line numberDiff line numberDiff line change
@@ -392,6 +392,13 @@ class ASTContext {
392392
/// Add a cleanup function to be called when the ASTContext is deallocated.
393393
void addCleanup(std::function<void(void)> cleanup);
394394

395+
/// Add a cleanup to run the given object's destructor when the ASTContext is
396+
/// deallocated.
397+
template<typename T>
398+
void addDestructorCleanup(T &object) {
399+
addCleanup([&object]{ object.~T(); });
400+
}
401+
395402
/// Create a context for the initializer of a non-local variable,
396403
/// like a global or a field. To reduce memory usage, if the
397404
/// context goes unused, it should be returned to the ASTContext

Diff for: include/swift/AST/Module.h

+10-6
Original file line numberDiff line numberDiff line change
@@ -172,13 +172,12 @@ class Module : public DeclContext {
172172
// FIXME: Do we really need to bloat all modules with this?
173173
DebuggerClient *DebugClient = nullptr;
174174

175-
// FIXME: This storage is never freed, because Modules are allocated on the
176-
// ASTContext.
177175
TinyPtrVector<FileUnit *> Files;
178176

177+
Module(Identifier name, ASTContext &ctx);
179178
public:
180-
Module(Identifier Name, ASTContext &C)
181-
: DeclContext(DeclContextKind::Module, nullptr), Ctx(C), Name(Name) {
179+
static Module *create(Identifier name, ASTContext &ctx) {
180+
return new (ctx) Module(name, ctx);
182181
}
183182

184183
ArrayRef<FileUnit *> getFiles() {
@@ -413,9 +412,9 @@ class FileUnit : public DeclContext {
413412
: DeclContext(DeclContextKind::FileUnit, &M), Kind(kind) {
414413
}
415414

416-
public:
417415
virtual ~FileUnit() = default;
418416

417+
public:
419418
FileUnitKind getKind() const {
420419
return Kind;
421420
}
@@ -571,6 +570,8 @@ class SourceFile final : public FileUnit {
571570
/// May be -1, to indicate no association with a buffer.
572571
int BufferID;
573572

573+
friend ASTContext;
574+
~SourceFile();
574575
public:
575576
/// The list of top-level declarations in the source file.
576577
std::vector<Decl*> Decls;
@@ -606,7 +607,6 @@ class SourceFile final : public FileUnit {
606607

607608
SourceFile(Module &M, SourceFileKind K, Optional<unsigned> bufferID,
608609
bool hasBuiltinModuleAccess = false);
609-
~SourceFile();
610610

611611
ArrayRef<std::pair<Module::ImportedModule, bool>> getImports() const {
612612
assert(ASTStage >= Parsed || Kind == SourceFileKind::SIL);
@@ -714,6 +714,9 @@ class BuiltinUnit final : public FileUnit {
714714
std::unique_ptr<LookupCache> Cache;
715715
LookupCache &getCache() const;
716716

717+
friend ASTContext;
718+
~BuiltinUnit() = default;
719+
717720
public:
718721
explicit BuiltinUnit(Module &M);
719722

@@ -733,6 +736,7 @@ class BuiltinUnit final : public FileUnit {
733736
/// Represents an externally-loaded file of some kind.
734737
class LoadedFile : public FileUnit {
735738
protected:
739+
~LoadedFile() = default;
736740
LoadedFile(FileUnitKind Kind, Module &M) noexcept
737741
: FileUnit(Kind, M) {
738742
assert(classof(this) && "invalid kind");

Diff for: include/swift/ClangImporter/ClangModule.h

+2
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ class ClangModuleUnit final : public LoadedFile {
3737

3838
Module *getAdapterModule() const;
3939

40+
~ClangModuleUnit() = default;
41+
4042
public:
4143
ClangModuleUnit(Module &M, ClangImporter &owner,
4244
clang::Module *clangModule);

Diff for: include/swift/Serialization/SerializedModuleLoader.h

+2
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,8 @@ class SerializedASTFile final : public LoadedFile {
132132

133133
ModuleFile &File;
134134

135+
~SerializedASTFile() = default;
136+
135137
SerializedASTFile(Module &M, ModuleFile &file)
136138
: LoadedFile(FileUnitKind::SerializedAST, M), File(file) {}
137139

Diff for: lib/AST/ASTContext.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ ConstraintCheckerArenaRAII::~ConstraintCheckerArenaRAII() {
244244
}
245245

246246
static Module *createBuiltinModule(ASTContext &ctx) {
247-
auto M = new (ctx) Module(ctx.getIdentifier("Builtin"), ctx);
247+
auto M = Module::create(ctx.getIdentifier("Builtin"), ctx);
248248
M->addFile(*new (ctx) BuiltinUnit(*M));
249249
return M;
250250
}

Diff for: lib/AST/Module.cpp

+8
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ void BuiltinUnit::LookupCache::lookupValue(Identifier Name, NLKind LookupKind,
8484
// Out-of-line because std::unique_ptr wants LookupCache to be complete.
8585
BuiltinUnit::BuiltinUnit(Module &M)
8686
: FileUnit(FileUnitKind::Builtin, M) {
87+
M.Ctx.addDestructorCleanup(*this);
8788
}
8889

8990
//===----------------------------------------------------------------------===//
@@ -269,6 +270,12 @@ void SourceLookupCache::lookupClassMember(AccessPathTy accessPath,
269270
// Module Implementation
270271
//===----------------------------------------------------------------------===//
271272

273+
Module::Module(Identifier name, ASTContext &ctx)
274+
: DeclContext(DeclContextKind::Module, nullptr), Ctx(ctx), Name(name) {
275+
ctx.addDestructorCleanup(*this);
276+
}
277+
278+
272279
void Module::addFile(FileUnit &newFile) {
273280
// Require Main and REPL files to be the first file added.
274281
assert(Files.empty() ||
@@ -1099,6 +1106,7 @@ SourceFile::SourceFile(Module &M, SourceFileKind K,
10991106
Optional<unsigned> bufferID, bool hasBuiltinModuleAccess)
11001107
: FileUnit(FileUnitKind::Source, M),
11011108
BufferID(bufferID ? *bufferID : -1), Kind(K) {
1109+
M.Ctx.addDestructorCleanup(*this);
11021110
performAutoImport(*this, hasBuiltinModuleAccess);
11031111
}
11041112

Diff for: lib/ClangImporter/ClangImporter.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ Module *ClangImporter::loadModule(
304304
// FIXME: The name of this module could end up as a key in the ASTContext,
305305
// but that's not correct for submodules.
306306
Identifier name = Impl.SwiftContext.getIdentifier((*clangModule).Name);
307-
auto result = new (Impl.SwiftContext) Module(name, Impl.SwiftContext);
307+
auto result = Module::create(name, Impl.SwiftContext);
308308

309309
auto file = new (Impl.SwiftContext) ClangModuleUnit(*result, *this,
310310
clangModule);
@@ -332,7 +332,7 @@ ClangImporter::Implementation::getWrapperForModule(ClangImporter &importer,
332332

333333
// FIXME: Handle hierarchical names better.
334334
Identifier name = SwiftContext.getIdentifier(underlying->Name);
335-
auto wrapper = new (SwiftContext) Module(name, SwiftContext);
335+
auto wrapper = Module::create(name, SwiftContext);
336336

337337
auto file = new (SwiftContext) ClangModuleUnit(*wrapper, importer,
338338
underlying);

Diff for: lib/Frontend/Frontend.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ bool swift::CompilerInstance::setup(const CompilerInvocation &Invok) {
161161
void CompilerInstance::performParse() {
162162
const SourceFileKind Kind = Invocation.getInputKind();
163163
Identifier ID = Context->getIdentifier(Invocation.getModuleName());
164-
MainModule = new (*Context) Module(ID, *Context);
164+
MainModule = Module::create(ID, *Context);
165165
Context->LoadedModules[ID.str()] = MainModule;
166166

167167
if (Kind == SourceFileKind::SIL) {

Diff for: lib/IDE/Utils.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ bool ide::isSourceInputComplete(std::unique_ptr<llvm::MemoryBuffer> MemBuf) {
3333
ASTContext Ctx(LangOpts, SearchPathOpts, SM, Diags);
3434
auto ModName = Ctx.getIdentifier("input");
3535

36-
Module Mod(ModName, Ctx);
37-
SourceFile SF(Mod, SourceFileKind::Main, BufferID);
36+
Module &Mod = *Module::create(ModName, Ctx);
37+
SourceFile &SF = *new (Ctx) SourceFile(Mod, SourceFileKind::Main, BufferID);
3838

3939
PersistentParserState PersistentState;
4040
Parser P(BufferID, SF, /*SIL=*/nullptr, &PersistentState);

Diff for: lib/Sema/SourceLoader.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ Module *SourceLoader::loadModule(SourceLoc importLoc,
9393
else
9494
bufferID = Ctx.SourceMgr.addNewSourceBuffer(inputFile.take());
9595

96-
auto *importMod = new (Ctx) Module(moduleID.first, Ctx);
96+
auto *importMod = Module::create(moduleID.first, Ctx);
9797
Ctx.LoadedModules[moduleID.first.str()] = importMod;
9898

9999
auto *importFile = new (Ctx) SourceFile(*importMod, SourceFileKind::Library,

Diff for: lib/Serialization/SerializedModuleLoader.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ Module *SerializedModuleLoader::loadModule(SourceLoc importLoc,
218218

219219
assert(inputFile);
220220

221-
auto M = new (Ctx) Module(moduleID.first, Ctx);
221+
auto M = Module::create(moduleID.first, Ctx);
222222
Ctx.LoadedModules[moduleID.first.str()] = M;
223223

224224
(void)loadAST(*M, moduleID.second, std::move(inputFile));

0 commit comments

Comments
 (0)