[Support] Remove output file checks from LockFileManager

[jansvoboda11]

Currently, LockFileManager assumes the owner of the lock file creates an output file. This is problematic for at least three reasons:

1. It is orthogonal to the main purpose of this class - mutual exclusion. This makes creating an alternative implementation more complicated than it needs to be.

2. Some clients (like the upstream AMDGPUSplitModule.cpp file) assume the output file is not necessary. The owner of the lock file does not write the file expected by LockFileManager and the processes waiting for the non-owned lock file to be unlocked therefore assume the owner has died. This means that the work gets repeated by each waiting process, serially.

3. The documentation makes it sound like successfully waiting for a non-owned lock file guarantees the output file to be present on the file system. Implicitly-built modules rely on this. However, the module file may disappear between LockFileManager performing the check and the compiler loading the module (for example due to module cache pruning with short intervals, or intervention from outside of Clang). The compiler assumes this cannot happen, and fails the build if it does.

This PR solves this situation by removing the check, reflecting that in the LockFileManager documentation, and fixing the time-of-check time-of-use bug in implicit modules.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-llvm-support

Author: Jan Svoboda (jansvoboda11)

Changes

Currently, LockFileManager assumes the owner of the lock file creates an output file. This is problematic for at least three reasons:

1. It is orthogonal to the main purpose of this class - mutual exclusion. This makes creating an alternative implementation more complicated than it needs to be.

2. Some clients (like the upstream AMDGPUSplitModule.cpp file) assume the output file is not necessary. The owner of the lock file does not write the file expected by LockFileManager and the processes waiting for the non-owned lock file to be unlocked therefore assume the owner has died. This means that the work gets repeated by each waiting process, serially.

3. The documentation makes it sound like successfully waiting for a non-owned lock file guarantees the output file to be present on the file system. Implicitly-built modules rely on this. However, the module file may disappear between LockFileManager performing the check and the compiler loading the module (for example due to module cache pruning with short intervals, or intervention from outside of Clang). The compiler assumes this cannot happen, and fails the build if it does.

This PR solves this situation by removing the check, reflecting that in the LockFileManager documentation, and fixing the time-of-check time-of-use bug in implicit modules.

---

Full diff: https://github.com/llvm/llvm-project/pull/130395.diff

3 Files Affected:

• (modified) clang/lib/Frontend/CompilerInstance.cpp (+15-7)
• (modified) llvm/include/llvm/Support/LockFileManager.h (+6-8)
• (modified) llvm/lib/Support/LockFileManager.cpp (+6-11)

diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index c11c857ea0606..6098e2e30af9d 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1406,7 +1406,7 @@ static bool readASTAfterCompileModule(CompilerInstance &ImportingInstance,
                                       SourceLocation ImportLoc,
                                       SourceLocation ModuleNameLoc,
                                       Module *Module, StringRef ModuleFileName,
-                                      bool *OutOfDate) {
+                                      bool *OutOfDate, bool *Missing) {
   DiagnosticsEngine &Diags = ImportingInstance.getDiagnostics();
 
   unsigned ModuleLoadCapabilities = ASTReader::ARR_Missing;
@@ -1427,6 +1427,12 @@ static bool readASTAfterCompileModule(CompilerInstance &ImportingInstance,
     return false;
   }
 
+  // The caller wants to handle missing module files.
+  if (Missing && ReadResult == ASTReader::Missing) {
+    *Missing = 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)
@@ -1452,7 +1458,7 @@ static bool compileModuleAndReadASTImpl(CompilerInstance &ImportingInstance,
 
   return readASTAfterCompileModule(ImportingInstance, ImportLoc, ModuleNameLoc,
                                    Module, ModuleFileName,
-                                   /*OutOfDate=*/nullptr);
+                                   /*OutOfDate=*/nullptr, /*Missing=*/nullptr);
 }
 
 /// Compile a module in a separate compiler instance and read the AST,
@@ -1517,15 +1523,17 @@ static bool compileModuleAndReadASTBehindLock(
 
     // Read the module that was just written by someone else.
     bool OutOfDate = false;
+    bool Missing = false;
     if (readASTAfterCompileModule(ImportingInstance, ImportLoc, ModuleNameLoc,
-                                  Module, ModuleFileName, &OutOfDate))
+                                  Module, ModuleFileName, &OutOfDate, &Missing))
       return true;
-    if (!OutOfDate)
+    if (!OutOfDate && !Missing)
       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...
+    // The module may be missing or out of date in the presence of file system
+    // races. It may also be out of date if one of its imports depends on header
+    // search paths that are not consistent with this ImportingInstance.
+    // Try again...
   }
 }
 
diff --git a/llvm/include/llvm/Support/LockFileManager.h b/llvm/include/llvm/Support/LockFileManager.h
index 92c7ceed6a929..fc61ddbbe4730 100644
--- a/llvm/include/llvm/Support/LockFileManager.h
+++ b/llvm/include/llvm/Support/LockFileManager.h
@@ -16,14 +16,13 @@
 namespace llvm {
 class StringRef;
 
-/// Class that manages the creation of a lock file to aid
-/// implicit coordination between different processes.
+/// Class that manages the creation of a lock file to aid implicit coordination
+/// between different processes.
 ///
-/// The implicit coordination works by creating a ".lock" file alongside
-/// the file that we're coordinating for, using the atomicity of the file
-/// system to ensure that only a single process can create that ".lock" file.
-/// When the lock file is removed, the owning process has finished the
-/// operation.
+/// The implicit coordination works by creating a ".lock" file, using the
+/// atomicity of the file system to ensure that only a single process can create
+/// that ".lock" file. When the lock file is removed, the owning process has
+/// finished the operation.
 class LockFileManager {
 public:
   /// Describes the state of a lock file.
@@ -50,7 +49,6 @@ class LockFileManager {
   };
 
 private:
-  SmallString<128> FileName;
   SmallString<128> LockFileName;
   SmallString<128> UniqueLockFileName;
 
diff --git a/llvm/lib/Support/LockFileManager.cpp b/llvm/lib/Support/LockFileManager.cpp
index 9a45a9966458e..d1497799dc05a 100644
--- a/llvm/lib/Support/LockFileManager.cpp
+++ b/llvm/lib/Support/LockFileManager.cpp
@@ -157,16 +157,15 @@ class RemoveUniqueLockFileOnSignal {
 
 } // end anonymous namespace
 
-LockFileManager::LockFileManager(StringRef FileName)
-{
-  this->FileName = FileName;
-  if (std::error_code EC = sys::fs::make_absolute(this->FileName)) {
+LockFileManager::LockFileManager(StringRef FileName) {
+  SmallString<128> AbsoluteFileName(FileName);
+  if (std::error_code EC = sys::fs::make_absolute(AbsoluteFileName)) {
     std::string S("failed to obtain absolute path for ");
-    S.append(std::string(this->FileName));
+    S.append(std::string(AbsoluteFileName));
     setError(EC, S);
     return;
   }
-  LockFileName = this->FileName;
+  LockFileName = AbsoluteFileName;
   LockFileName += ".lock";
 
   // If the lock file already exists, don't bother to try to create our own
@@ -307,12 +306,8 @@ LockFileManager::waitForUnlock(const unsigned MaxSeconds) {
   while (Backoff.waitForNextAttempt()) {
     // FIXME: implement event-based waiting
     if (sys::fs::access(LockFileName.c_str(), sys::fs::AccessMode::Exist) ==
-        errc::no_such_file_or_directory) {
-      // If the original file wasn't created, somone thought the lock was dead.
-      if (!sys::fs::exists(FileName))
-        return Res_OwnerDied;
+        errc::no_such_file_or_directory)
       return Res_Success;
-    }
 
     // If the process owning the lock died without cleaning up, just bail out.
     if (!processStillExecuting((*Owner).first, (*Owner).second))

[Bigcheese]

lgtm. This makes a lot more sense.