Skip to content

[SourceKit] Guard DependencyStamps with a lock #58681

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 12, 2022
Merged

[SourceKit] Guard DependencyStamps with a lock #58681

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 = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's quite a bit of shared state in this class in general. Is this the only other one that needs the lock? Should we just merge this one and ConsumersAndResultMtx and make it a SharedStateMtx or something?


/// 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 @@ -1070,9 +1080,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