[clang] c130300 - Frontend: Refactor compileModuleAndReadAST, NFC
Duncan P. N. Exon Smith via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 12 15:16:39 PDT 2021
Author: Duncan P. N. Exon Smith
Date: 2021-08-12T15:16:08-07:00
New Revision: c130300f8ba0363749cf2490bbd43515fad8a759
URL: https://github.com/llvm/llvm-project/commit/c130300f8ba0363749cf2490bbd43515fad8a759
DIFF: https://github.com/llvm/llvm-project/commit/c130300f8ba0363749cf2490bbd43515fad8a759.diff
LOG: Frontend: Refactor compileModuleAndReadAST, NFC
This renames `compileModuleAndReadAST`, adding a `BehindLock` suffix,
and refactors it to significantly reduce nesting.
- Split out helpers `compileModuleAndReadASTImpl` and
`readASTAfterCompileModule` which have straight-line code that doesn't
worry about locks.
- Use `break` in the interesting cases of `switch` statements to reduce
nesting.
- Use early `return`s to reduce nesting.
Detangling the compile-and-read logic from the check-for-locks logic
should be a net win for readability, although I also have a side
motivation of making the locks optional in a follow-up.
No functionality change here.
Differential Revision: https://reviews.llvm.org/D95581
Added:
Modified:
clang/lib/Frontend/CompilerInstance.cpp
Removed:
################################################################################
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index c642af1849bc4..d90b292808f21 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1264,31 +1264,79 @@ static bool compileModule(CompilerInstance &ImportingInstance,
return Result;
}
+/// Read the AST right after compiling the module.
+static bool readASTAfterCompileModule(CompilerInstance &ImportingInstance,
+ SourceLocation ImportLoc,
+ SourceLocation ModuleNameLoc,
+ Module *Module, StringRef ModuleFileName,
+ bool *OutOfDate) {
+ DiagnosticsEngine &Diags = ImportingInstance.getDiagnostics();
+
+ unsigned ModuleLoadCapabilities = ASTReader::ARR_Missing;
+ if (OutOfDate)
+ ModuleLoadCapabilities |= ASTReader::ARR_OutOfDate;
+
+ // Try to read the module file, now that we've compiled it.
+ ASTReader::ASTReadResult ReadResult =
+ ImportingInstance.getASTReader()->ReadAST(
+ ModuleFileName, serialization::MK_ImplicitModule, ImportLoc,
+ ModuleLoadCapabilities);
+ if (ReadResult == ASTReader::Success)
+ return true;
+
+ // The caller wants to handle out-of-date failures.
+ if (OutOfDate && ReadResult == ASTReader::OutOfDate) {
+ *OutOfDate = true;
+ return false;
+ }
+
+ // The ASTReader didn't diagnose the error, so conservatively report it.
+ if (ReadResult == ASTReader::Missing || !Diags.hasErrorOccurred())
+ Diags.Report(ModuleNameLoc, diag::err_module_not_built)
+ << Module->Name << SourceRange(ImportLoc, ModuleNameLoc);
+
+ return false;
+}
+
/// Compile a module in a separate compiler instance and read the AST,
/// returning true if the module compiles without errors.
+static bool compileModuleAndReadASTImpl(CompilerInstance &ImportingInstance,
+ SourceLocation ImportLoc,
+ SourceLocation ModuleNameLoc,
+ Module *Module,
+ StringRef ModuleFileName) {
+ if (!compileModule(ImportingInstance, ModuleNameLoc, Module,
+ ModuleFileName)) {
+ ImportingInstance.getDiagnostics().Report(ModuleNameLoc,
+ diag::err_module_not_built)
+ << Module->Name << SourceRange(ImportLoc, ModuleNameLoc);
+ return false;
+ }
+
+ return readASTAfterCompileModule(ImportingInstance, ImportLoc, ModuleNameLoc,
+ Module, ModuleFileName,
+ /*OutOfDate=*/nullptr);
+}
+
+/// Compile a module in a separate compiler instance and read the AST,
+/// returning true if the module compiles without errors, using a lock manager
+/// to avoid building the same module in multiple compiler instances.
///
/// Uses a lock file manager and exponential backoff to reduce the chances that
/// multiple instances will compete to create the same module. On timeout,
/// deletes the lock file in order to avoid deadlock from crashing processes or
/// bugs in the lock file manager.
-static bool compileModuleAndReadAST(CompilerInstance &ImportingInstance,
- SourceLocation ImportLoc,
- SourceLocation ModuleNameLoc,
- Module *Module, StringRef ModuleFileName) {
+static bool compileModuleAndReadASTBehindLock(
+ CompilerInstance &ImportingInstance, SourceLocation ImportLoc,
+ SourceLocation ModuleNameLoc, Module *Module, StringRef ModuleFileName) {
DiagnosticsEngine &Diags = ImportingInstance.getDiagnostics();
- auto diagnoseBuildFailure = [&] {
- Diags.Report(ModuleNameLoc, diag::err_module_not_built)
- << Module->Name << SourceRange(ImportLoc, ModuleNameLoc);
- };
-
// FIXME: have LockFileManager return an error_code so that we can
// avoid the mkdir when the directory already exists.
StringRef Dir = llvm::sys::path::parent_path(ModuleFileName);
llvm::sys::fs::create_directories(Dir);
while (1) {
- unsigned ModuleLoadCapabilities = ASTReader::ARR_Missing;
llvm::LockFileManager Locked(ModuleFileName);
switch (Locked) {
case llvm::LockFileManager::LFS_Error:
@@ -1302,55 +1350,42 @@ static bool compileModuleAndReadAST(CompilerInstance &ImportingInstance,
LLVM_FALLTHROUGH;
case llvm::LockFileManager::LFS_Owned:
// We're responsible for building the module ourselves.
- if (!compileModule(ImportingInstance, ModuleNameLoc, Module,
- ModuleFileName)) {
- diagnoseBuildFailure();
- return false;
- }
- break;
+ return compileModuleAndReadASTImpl(ImportingInstance, ImportLoc,
+ ModuleNameLoc, Module, ModuleFileName);
case llvm::LockFileManager::LFS_Shared:
- // Someone else is responsible for building the module. Wait for them to
- // finish.
- switch (Locked.waitForUnlock()) {
- case llvm::LockFileManager::Res_Success:
- ModuleLoadCapabilities |= ASTReader::ARR_OutOfDate;
- break;
- case llvm::LockFileManager::Res_OwnerDied:
- continue; // try again to get the lock.
- case llvm::LockFileManager::Res_Timeout:
- // Since ModuleCache takes care of correctness, we try waiting for
- // another process to complete the build so clang does not do it done
- // twice. If case of timeout, build it ourselves.
- Diags.Report(ModuleNameLoc, diag::remark_module_lock_timeout)
- << Module->Name;
- // Clear the lock file so that future invocations can make progress.
- Locked.unsafeRemoveLockFile();
- continue;
- }
- break;
+ break; // The interesting case.
}
- // Try to read the module file, now that we've compiled it.
- ASTReader::ASTReadResult ReadResult =
- ImportingInstance.getASTReader()->ReadAST(
- ModuleFileName, serialization::MK_ImplicitModule, ImportLoc,
- ModuleLoadCapabilities);
-
- if (ReadResult == ASTReader::OutOfDate &&
- Locked == llvm::LockFileManager::LFS_Shared) {
- // The module may be out of date in the presence of file system races,
- // or if one of its imports depends on header search paths that are not
- // consistent with this ImportingInstance. Try again...
+ // Someone else is responsible for building the module. Wait for them to
+ // finish.
+ switch (Locked.waitForUnlock()) {
+ case llvm::LockFileManager::Res_Success:
+ break; // The interesting case.
+ case llvm::LockFileManager::Res_OwnerDied:
+ continue; // try again to get the lock.
+ case llvm::LockFileManager::Res_Timeout:
+ // Since ModuleCache takes care of correctness, we try waiting for
+ // another process to complete the build so clang does not do it done
+ // twice. If case of timeout, build it ourselves.
+ Diags.Report(ModuleNameLoc, diag::remark_module_lock_timeout)
+ << Module->Name;
+ // Clear the lock file so that future invocations can make progress.
+ Locked.unsafeRemoveLockFile();
continue;
- } else if (ReadResult == ASTReader::Missing) {
- diagnoseBuildFailure();
- } else if (ReadResult != ASTReader::Success &&
- !Diags.hasErrorOccurred()) {
- // The ASTReader didn't diagnose the error, so conservatively report it.
- diagnoseBuildFailure();
}
- return ReadResult == ASTReader::Success;
+
+ // Read the module that was just written by someone else.
+ bool OutOfDate = false;
+ if (readASTAfterCompileModule(ImportingInstance, ImportLoc, ModuleNameLoc,
+ Module, ModuleFileName, &OutOfDate))
+ return true;
+ if (!OutOfDate)
+ return false;
+
+ // The module may be out of date in the presence of file system races,
+ // or if one of its imports depends on header search paths that are not
+ // consistent with this ImportingInstance. Try again...
}
}
@@ -1831,8 +1866,8 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
}
// Try to compile and then read the AST.
- if (!compileModuleAndReadAST(*this, ImportLoc, ModuleNameLoc, M,
- ModuleFilename)) {
+ if (!compileModuleAndReadASTBehindLock(*this, ImportLoc, ModuleNameLoc, M,
+ ModuleFilename)) {
assert(getDiagnostics().hasErrorOccurred() &&
"undiagnosed error in compileModuleAndReadAST");
if (getPreprocessorOpts().FailedModules)
More information about the cfe-commits
mailing list