Patch Out a Source of Iterator Invalidation #59144
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
@swift-ci smoke test |
beccadax
approved these changes
May 31, 2022
CodaFi
added a commit
to CodaFi/swift
that referenced
this pull request
Jun 11, 2022
See swiftlang#59144 for more on why this is a bad idea. Patch out the synthesized file unit accessor to only clear the source cache, then patch up all the places that were assuming they could iterate over the module's file list and see synthesized files. rdar://94164512
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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