Fix a pair of leaks related to FileUnit destructors not being run #26816
Conversation
I'm not sure why this wasn't already broken, but with the changes in the next commit the multifile module trace test fails complaining that it can't get a path for the main module. But we don't /need/ a path for the main module because it's not a dependency of itself.
...rather than the buffer, for a compiled module that came from a module interface. This was already happening at a higher level (ModuleDecl::getModuleFilename) so pushing it down to the low-level ModuleFile::getModuleFilename doesn't really change things much. The important fix that goes with this is that SerializedASTFile no longer leaks this name by storing it outside of ModuleFile. https://bugs.swift.org/browse/SR-11365
Implementing it in LoadedFile is nice in theory, but causes a leak in practice because that type is ASTContext-allocated and usually never destroyed. https://bugs.swift.org/browse/SR-11366
We already do this for other ASTContext-allocated types (see Decl.cpp). This will prevent the sort of mistakes in the previous two commits. Note that if any particular subclass of FileUnit wants to have its destructor run, it can opt into that manually using ASTContext::addDestructorCleanup. SourceFile and BuiltinUnit both do this. But we generally don't /want/ to do this if we can avoid it because it adds to compiler teardown time.
jrose-apple
requested review from
rintaro,
harlanhaskins,
adrian-prantl,
aschwaighofer and
varungandhi-apple
|
@swift-ci Please test |
|
W.r.t the module trace: perhaps I'm misunderstanding something -- the issue is that we're treating the main module as a dependency in the trace, correct? In that case, I'm surprised that a path for the main module is present in the dependency tracker, I would've expected that path to not be present at all. Is that a bug or is that intentional? |
|
(Also, I think you reordered the commits and perhaps didn't update the commit messages? The last commit message says "prevents the mistakes in the past two commits" but that doesn't have anything to do with the module trace commit which comes right before AFAICT. I also don't understand what you mean by "that's about to be exposed by…" -- exposed by ???) |
|
I blame GitHub for showing commits in chronological order instead of parent order. The real order is the one I wrote in the description. The issue isn't actually that we're treating the main module as a dependency, but that we look at all loaded modules to build a path-to-module map (which is used later), and we can only do that if a particular module has a unique path. The main module does not have such a thing if it's made up of multiple files! |
Uh oh!
There was an error while loading. Please reload this page.