Skip to content

[5.7][SourceKit] Guard DependencyStamps with a lock #58850

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 13, 2022
Merged

[5.7][SourceKit] Guard DependencyStamps with a lock #58850

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,12 +308,20 @@ class ASTBuildOperation
/// can be determined at construction time of the \c ASTBuildOperation.
const std::vector<FileContent> FileContents;

/// Guards \c DependencyStamps. This prevents reading from \c DependencyStamps
/// while it is being modified. It does not provide any ordering gurantees
/// that \c DependencyStamps have been computed in \c buildASTUnit before they
/// are accessed in \c matchesSourceState but that's fine (see comment on
/// \c DependencyStamps).
llvm::sys::Mutex DependencyStampsMtx;

/// \c DependencyStamps contains the stamps of all module depenecies needed
/// for the AST build. These stamps are only known after the AST is built.
/// Before the AST has been built, we thus assume that all dependency stamps
/// match. This seems to be a reasonable assumption since the dependencies
/// shouldn't change (much) in the time between an \c ASTBuildOperation is
/// created and until it produced an AST.
/// Must only be accessed if \c DependencyStampsMtx has been claimed.
SmallVector<std::pair<std::string, BufferStamp>, 8> DependencyStamps = {};

/// The ASTManager from which this operation got scheduled. Used to update
Expand Down Expand Up @@ -898,6 +906,8 @@ bool ASTBuildOperation::matchesSourceState(
}
}

llvm::sys::ScopedLock L(DependencyStampsMtx);

for (auto &Dependency : DependencyStamps) {
if (Dependency.second !=
ASTManager->Impl.getBufferStamp(Dependency.first, OtherFileSystem))
Expand Down Expand Up @@ -1069,9 +1079,12 @@ ASTUnitRef ASTBuildOperation::buildASTUnit(std::string &Error) {
collectModuleDependencies(CompIns.getMainModule(), Visited, Filenames);
// FIXME: There exists a small window where the module file may have been
// modified after compilation finished and before we get its stamp.
for (auto &Filename : Filenames) {
DependencyStamps.push_back(std::make_pair(
Filename, ASTManager->Impl.getBufferStamp(Filename, FileSystem)));
{
llvm::sys::ScopedLock L(DependencyStampsMtx);
for (auto &Filename : Filenames) {
DependencyStamps.push_back(std::make_pair(
Filename, ASTManager->Impl.getBufferStamp(Filename, FileSystem)));
}
}

// Since we only typecheck the primary file (plus referenced constructs
Expand Down