[clang][modules] Fix filesystem races in ModuleManager

The `ModuleManager` uses `FileEntry` objects to uniquely identify module files. This requires first consulting the `FileManager` (and therefore the file system) when loading PCM files. This is problematic, as this may load a different PCM file to what's already in the `InMemoryModuleCache` and fail the size and mtime checks.

This PR changes things so that module files are identified by their file system path. This removes the need of knowing the file entry at the start and allows us to fix the race. The downside is that we no longer get the `FileManager` inode-based uniquing, meaning symlinks in the module cache no longer work. I think this is fine, since Clang itself never creates symlinks in that directory. Moreover, we already had to work around filesystems recycling inode numbers, so this actually seems like a win to me (and resolves a long-standing FIXME-like comment).

Note that this change also requires that all places calling into `ModuleManager` from within a single `CompilerInstance` use consistent paths to refer to module files. This might be problematic if there are concurrent Clang processes operating on the same module cache directory but with different spellings - the `IMPORT` records in PCM files will be valid and consistent within single process, but may break uniquing in another process. We might need to do some canonicalization of the module cache paths to avoid this issue.

Author: jansvoboda11

clang/include/clang/Serialization/ModuleManager.h

@@ -22,6 +22,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/ADT/iterator_range.h"
@@ -58,7 +59,7 @@ class ModuleManager {
   SmallVector<ModuleFile *, 2> Roots;
 
   /// All loaded modules, indexed by name.
-  llvm::DenseMap<const FileEntry *, ModuleFile *> Modules;
+  llvm::StringMap<ModuleFile *> Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.
@@ -179,7 +180,7 @@ class ModuleManager {
   ModuleFile *lookupByModuleName(StringRef ModName) const;
 
   /// Returns the module associated with the given module file.
-  ModuleFile *lookup(const FileEntry *File) const;
+  ModuleFile *lookup(FileEntryRef File) const;
 
   /// Returns the in-memory (virtual file) buffer with the given name
   std::unique_ptr<llvm::MemoryBuffer> lookupBuffer(StringRef Name);
@@ -283,26 +284,6 @@ class ModuleManager {
   void visit(llvm::function_ref<bool(ModuleFile &M)> Visitor,
              llvm::SmallPtrSetImpl<ModuleFile *> *ModuleFilesHit = nullptr);
 
-  /// Attempt to resolve the given module file name to a file entry.
-  ///
-  /// \param FileName The name of the module file.
-  ///
-  /// \param ExpectedSize The size that the module file is expected to have.
-  /// If the actual size differs, the resolver should return \c true.
-  ///
-  /// \param ExpectedModTime The modification time that the module file is
-  /// expected to have. If the actual modification time differs, the resolver
-  /// should return \c true.
-  ///
-  /// \param File Will be set to the file if there is one, or null
-  /// otherwise.
-  ///
-  /// \returns True if a file exists but does not meet the size/
-  /// modification time criteria, false if the file is either available and
-  /// suitable, or is missing.
-  bool lookupModuleFile(StringRef FileName, off_t ExpectedSize,
-                        time_t ExpectedModTime, OptionalFileEntryRef &File);
-
   /// View the graphviz representation of the module graph.
   void viewGraph();
 

clang/lib/Serialization/ModuleManager.cpp

@@ -58,8 +58,8 @@ ModuleFile *ModuleManager::lookupByModuleName(StringRef Name) const {
   return nullptr;
 }
 
-ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
-  return Modules.lookup(File);
+ModuleFile *ModuleManager::lookup(FileEntryRef File) const {
+  return Modules.lookup(File.getName());
 }
 
 std::unique_ptr<llvm::MemoryBuffer>
@@ -106,105 +106,48 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
                          std::string &ErrorStr) {
   Module = nullptr;
 
-  // Look for the file entry. This only fails if the expected size or
-  // modification time differ.
-  OptionalFileEntryRef Entry;
-  if (Type == MK_ExplicitModule || Type == MK_PrebuiltModule) {
-    // If we're not expecting to pull this file out of the module cache, it
-    // might have a different mtime due to being moved across filesystems in
-    // a distributed build. The size must still match, though. (As must the
-    // contents, but we can't check that.)
-    ExpectedModTime = 0;
-  }
-  // Note: ExpectedSize and ExpectedModTime will be 0 for MK_ImplicitModule
-  // when using an ASTFileSignature.
-  if (lookupModuleFile(FileName, ExpectedSize, ExpectedModTime, Entry)) {
-    ErrorStr = "module file has a different size or mtime than expected";
-    return OutOfDate;
-  }
-
-  if (!Entry) {
-    ErrorStr = "module file not found";
-    return Missing;
-  }
-
-  // The ModuleManager's use of FileEntry nodes as the keys for its map of
-  // loaded modules is less than ideal. Uniqueness for FileEntry nodes is
-  // maintained by FileManager, which in turn uses inode numbers on hosts
-  // that support that. When coupled with the module cache's proclivity for
-  // turning over and deleting stale PCMs, this means entries for different
-  // module files can wind up reusing the same underlying inode. When this
-  // happens, subsequent accesses to the Modules map will disagree on the
-  // ModuleFile associated with a given file. In general, it is not sufficient
-  // to resolve this conundrum with a type like FileEntryRef that stores the
-  // name of the FileEntry node on first access because of path canonicalization
-  // issues. However, the paths constructed for implicit module builds are
-  // fully under Clang's control. We *can*, therefore, rely on their structure
-  // being consistent across operating systems and across subsequent accesses
-  // to the Modules map.
-  auto implicitModuleNamesMatch = [](ModuleKind Kind, const ModuleFile *MF,
-                                     FileEntryRef Entry) -> bool {
-    if (Kind != MK_ImplicitModule)
-      return true;
-    return Entry.getName() == MF->FileName;
-  };
-
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(*Entry)) {
-    if (implicitModuleNamesMatch(Type, ModuleEntry, *Entry)) {
-      // Check the stored signature.
-      if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
-        return OutOfDate;
-
-      Module = ModuleEntry;
-      updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc);
-      return AlreadyLoaded;
-    }
+  if (ModuleFile *ModuleEntry = Modules.lookup(FileName)) {
+    // Check the stored signature.
+    if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
+      return OutOfDate;
+
+    Module = ModuleEntry;
+    updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc);
+    return AlreadyLoaded;
   }
 
-  // Allocate a new module.
-  auto NewModule = std::make_unique<ModuleFile>(Type, *Entry, Generation);
-  NewModule->Index = Chain.size();
-  NewModule->FileName = FileName.str();
-  NewModule->ImportLoc = ImportLoc;
-  NewModule->InputFilesValidationTimestamp = 0;
-
-  if (NewModule->Kind == MK_ImplicitModule) {
-    std::string TimestampFilename =
-        ModuleFile::getTimestampFilename(NewModule->FileName);
-    llvm::vfs::Status Status;
-    // A cached stat value would be fine as well.
-    if (!FileMgr.getNoncachedStatValue(TimestampFilename, Status))
-      NewModule->InputFilesValidationTimestamp =
-          llvm::sys::toTimeT(Status.getLastModificationTime());
-  }
+  OptionalFileEntryRef Entry;
+  llvm::MemoryBuffer *ModuleBuffer = nullptr;
 
   // Load the contents of the module
   if (std::unique_ptr<llvm::MemoryBuffer> Buffer = lookupBuffer(FileName)) {
     // The buffer was already provided for us.
-    NewModule->Buffer = &ModuleCache->addBuiltPCM(FileName, std::move(Buffer));
-    // Since the cached buffer is reused, it is safe to close the file
-    // descriptor that was opened while stat()ing the PCM in
-    // lookupModuleFile() above, it won't be needed any longer.
-    Entry->closeFile();
+    ModuleBuffer = &ModuleCache->addBuiltPCM(FileName, std::move(Buffer));
   } else if (llvm::MemoryBuffer *Buffer =
                  getModuleCache().lookupPCM(FileName)) {
-    NewModule->Buffer = Buffer;
-    // As above, the file descriptor is no longer needed.
-    Entry->closeFile();
+    ModuleBuffer = Buffer;
   } else if (getModuleCache().shouldBuildPCM(FileName)) {
     // Report that the module is out of date, since we tried (and failed) to
     // import it earlier.
-    Entry->closeFile();
     return OutOfDate;
   } else {
+    Entry = FileName == "-"
+                ? expectedToOptional(FileMgr.getSTDIN())
+                : FileMgr.getOptionalFileRef(FileName, /*OpenFile=*/true,
+                                             /*CacheFailure=*/false);
+    if (!Entry) {
+      ErrorStr = "module file not found";
+      return Missing;
+    }
+
     // Get a buffer of the file and close the file descriptor when done.
     // The file is volatile because in a parallel build we expect multiple
     // compiler processes to use the same module file rebuilding it if needed.
     //
     // RequiresNullTerminator is false because module files don't need it, and
     // this allows the file to still be mmapped.
-    auto Buf = FileMgr.getBufferForFile(NewModule->File,
+    auto Buf = FileMgr.getBufferForFile(*Entry,
                                         /*IsVolatile=*/true,
                                         /*RequiresNullTerminator=*/false);
 
@@ -213,9 +156,39 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
       return Missing;
     }
 
-    NewModule->Buffer = &getModuleCache().addPCM(FileName, std::move(*Buf));
+    if ((ExpectedSize && ExpectedSize != Entry->getSize()) ||
+        (ExpectedModTime && ExpectedModTime != Entry->getModificationTime())) {
+      ErrorStr = "module file has a different size or mtime than expected";
+      return OutOfDate;
+    }
+
+    ModuleBuffer = &getModuleCache().addPCM(FileName, std::move(*Buf));
+  }
+
+  // TODO: Make it so that ModuleFile is not tied to a FileEntry.
+  if (!Entry && !((Entry = FileMgr.getVirtualFileRef(FileName, ExpectedSize,
+                                                     ExpectedModTime))))
+    return Missing;
+
+  // Allocate a new module.
+  auto NewModule = std::make_unique<ModuleFile>(Type, *Entry, Generation);
+  NewModule->Index = Chain.size();
+  NewModule->FileName = FileName.str();
+  NewModule->ImportLoc = ImportLoc;
+  NewModule->InputFilesValidationTimestamp = 0;
+
+  if (NewModule->Kind == MK_ImplicitModule) {
+    std::string TimestampFilename =
+        ModuleFile::getTimestampFilename(NewModule->FileName);
+    llvm::vfs::Status Status;
+    // A cached stat value would be fine as well.
+    if (!FileMgr.getNoncachedStatValue(TimestampFilename, Status))
+      NewModule->InputFilesValidationTimestamp =
+          llvm::sys::toTimeT(Status.getLastModificationTime());
   }
 
+  NewModule->Buffer = ModuleBuffer;
+
   // Initialize the stream.
   NewModule->Data = PCHContainerRdr.ExtractPCH(*NewModule->Buffer);
 
@@ -226,7 +199,7 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
     return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[*Entry] = NewModule.get();
+  Module = Modules[FileName] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -272,7 +245,7 @@ void ModuleManager::removeModules(ModuleIterator First) {
 
   // Delete the modules.
   for (ModuleIterator victim = First; victim != Last; ++victim)
-    Modules.erase(victim->File);
+    Modules.erase(victim->File.getName());
 
   Chain.erase(Chain.begin() + (First - begin()), Chain.end());
 }
@@ -432,29 +405,6 @@ void ModuleManager::visit(llvm::function_ref<bool(ModuleFile &M)> Visitor,
   returnVisitState(std::move(State));
 }
 
-bool ModuleManager::lookupModuleFile(StringRef FileName, off_t ExpectedSize,
-                                     time_t ExpectedModTime,
-                                     OptionalFileEntryRef &File) {
-  if (FileName == "-") {
-    File = expectedToOptional(FileMgr.getSTDIN());
-    return false;
-  }
-
-  // Open the file immediately to ensure there is no race between stat'ing and
-  // opening the file.
-  File = FileMgr.getOptionalFileRef(FileName, /*OpenFile=*/true,
-                                    /*CacheFailure=*/false);
-
-  if (File &&
-      ((ExpectedSize && ExpectedSize != File->getSize()) ||
-       (ExpectedModTime && ExpectedModTime != File->getModificationTime())))
-    // Do not destroy File, as it may be referenced. If we need to rebuild it,
-    // it will be destroyed by removeModules.
-    return true;
-
-  return false;
-}
-
 #ifndef NDEBUG
 namespace llvm {