[clang] f90aa41 - [Support] Remove output file checks from `LockFileManager` (#130395)

[via cfe-commits]

Author: Jan Svoboda
Date: 2025-03-11T08:51:28-07:00
New Revision: f90aa418978748c0613036b0285bb80af40570b7

URL: https://github.com/llvm/llvm-project/commit/f90aa418978748c0613036b0285bb80af40570b7
DIFF: https://github.com/llvm/llvm-project/commit/f90aa418978748c0613036b0285bb80af40570b7.diff

LOG: [Support] Remove output file checks from `LockFileManager` (#130395)

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.

Added: 
    

Modified: 
    clang/lib/Frontend/CompilerInstance.cpp
    llvm/include/llvm/Support/LockFileManager.h
    llvm/lib/Support/LockFileManager.cpp

Removed: 
    


################################################################################
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..4bb8919a0ab31 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 
diff erent processes.
+/// Class that manages the creation of a lock file to aid implicit coordination
+/// between 
diff erent 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.

diff  --git a/llvm/lib/Support/LockFileManager.cpp b/llvm/lib/Support/LockFileManager.cpp
index 9a45a9966458e..ac26646525666 100644
--- a/llvm/lib/Support/LockFileManager.cpp
+++ b/llvm/lib/Support/LockFileManager.cpp
@@ -307,12 +307,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))