averello/swift-mirror
1
0
Fork 0
mirror of https://github.com/apple/swift.git synced 2025-12-14 20:36:38 +01:00
Code Issues Packages Projects Releases Wiki Activity

Patch Out a Source of Iterator Invalidation

Browse Source 
Synthesized file units were designed for autodiff to emit synthesized declarations, and also to sidestep the design implications of doing so late in the compiler pipeline.

A call to materialize synthesized file units was added to the GetImplicitSendable request. This introduced a source of iterator invalidation into forEachFileToTypeCheck in whole-module builds. Any call to insert a new file into the module has the potential to cause the underlying SmallVector to reallocate.

This patch provides a narrow workaround that stops using iterators altogether in forEachFileToTypeCheck. However, this bug reveals a severe architectural flaw in the concept of a synthesized file unit. Iterating over the files in a module is an extremely common operation, and there now are myriad ways we could wind up calling a function that mutates the module's list of files in the process. This also means the number and kind of files being visited by compiler analyses is dependent upon whether a request that inserts these files has or has not been called.

This suggests the call to ModuleDecl::addFile in FileUnit::getOrCreateSynthesizedFile is deleterious and should be removed. Doing so will come as part of a larger refactoring.

rdar://94043340
This commit is contained in:
Robert Widmann
2022-05-28 12:06:42 -07:00
parent e9fb9091b8
commit bbe4608fa4
3 changed files with 22 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
include/swift/AST/FileUnit.h
View File

@@ -55,6 +55,12 @@ public:
/// Returns the synthesized file for this source file, if it exists.
SynthesizedFileUnit *getSynthesizedFile() const;
/// Returns the synthesized file for this source file, creating one and
/// inserting it into the module if it does not exist.
///
/// \warning Because this function mutates the parent module's list of files,
/// it will invalidate the iterators of any upstream callers of
/// \c ModuleDecl::getFiles().
SynthesizedFileUnit &getOrCreateSynthesizedFile();
/// Look up a (possibly overloaded) value set at top-level scope

9
lib/AST/Module.cpp
View File

@@ -3064,6 +3064,15 @@ SynthesizedFileUnit &FileUnit::getOrCreateSynthesizedFile() {
return *thisSynth;
SynthesizedFile = new (getASTContext()) SynthesizedFileUnit(*this);
SynthesizedFileAndKind.setPointer(SynthesizedFile);
// FIXME: Mutating the module in-flight is not a good idea. Any
// callers above us in the stack that are iterating over
// the module's files will have their iterators invalidated. There's
// a strong chance that whatever analysis led to this function being
// called is doing just that!
//
// Instead we ought to just call ModuleDecl::clearLookupCache() here
// and patch out the places looking for synthesized files hanging off of
// source files.
getParentModule()->addFile(*SynthesizedFile);
}
return *SynthesizedFile;

9
lib/Frontend/Frontend.cpp
View File

@@ -1168,8 +1168,13 @@ bool CompilerInstance::loadPartialModulesAndImplicitImports(
bool CompilerInstance::forEachFileToTypeCheck(
llvm::function_ref<bool(SourceFile &)> fn) {
if (isWholeModuleCompilation()) {
for (auto fileName : getMainModule()->getFiles()) {
auto *SF = dyn_cast<SourceFile>(fileName);
// FIXME: Do not refactor this to use an iterator as long as
// ModuleDecl::addFile is called during Sema. Synthesized files pushed
// during semantic analysis will cause iterator invalidation here.
// See notes in SourceFile::getOrCreateSynthesizedFile() for more.
unsigned i = 0;
while (i < getMainModule()->getFiles().size()) {
auto *SF = dyn_cast<SourceFile>(getMainModule()->getFiles()[i++]);
if (!SF) {
continue;
}