Close FileEntries of cached files in ModuleManager::addModule().

[aprantl]

Summary

Close FileEntries of cached files in ModuleManager::addModule().

While investigating why LLDB (which can build hundreds of clang
modules during one debug session) was getting "too many open files"
errors, I found that most of them are .pcm files that are kept open by
ModuleManager. Pretty much all of the open file dscriptors are
FileEntries that are refering to .pcm files for which a buffer
already exists in a CompilerInstance's PCMCache.

Before PCMCache was added it was necessary to hold on to open file
descriptors to ensure that all ModuleManagers using the same
FileManager read the a consistent version of a given .pcm file on
disk, even when a concurrent clang process overwrites the file halfway
through. The PCMCache makes this practice unnecessary, since it caches
the entire contents of a .pcm file, while the FileManager caches all
the stat() information.

This patch adds a call to FileEntry::closeFile() to the path where a
Buffer has already been created. This is necessary because even for a
freshly written .pcm file the file is stat()ed once immediately
after writing to generate a FileEntry in the FileManager. Because a
freshly-generated file's contents is stored in the PCMCache, it is
fine to close the file immediately thereafter. The second change this
patch makes is to set the ShouldClose flag to true when reading a
.pcm file into the PCMCache for the first time.

[For reference, in 1 Clang instance there is

• 1 FileManager and
• n ModuleManagers with
• n PCMCaches.]

rdar://problem/40906753

[aprantl]

@bruno: When we last discussed this my plan was to avoid the stat() in lookupModuleFile() for files that were just added to the PCMCache by WriteAST() entirely, but ModuleManager::Modules is a DenseMap<FileEntry, ModuleFile *> and lookupModuleFile() is the easiest way to create a new FileEntry. It would be nice to find a way to avoid the stat() for a file that we just wrote, but it wasn't immediately obvious to me how to do that.

[bruno]

Thanks for working on this Adrian, LGTM.

When we last discussed this my plan was to avoid the stat() in lookupModuleFile() for files that were just added to the PCMCache by WriteAST() entirely, but ModuleManager::Modules is a DenseMap<FileEntry, ModuleFile *> and lookupModuleFile() is the easiest way to create a new FileEntry. It would be nice to find a way to avoid the stat() for a file that we just wrote, but it wasn't immediately obvious to me how to do that.

Makes sense, I don't know offhand either. It's an extra optimization step we want to do, but it shouldn't block this change.

lib/Serialization/ModuleManager.cpp

if (!FileMgr.getNoncachedStatValue(TimestampFilename, Status))
       NewModule->InputFilesValidationTimestamp =
           llvm::sys::toTimeT(Status.getLastModificationTime());
   }
 
   // Load the contents of the module
   if (std::unique_ptr<llvm::MemoryBuffer> Buffer = lookupBuffer(FileName)) {
     // The buffer was already provided for us.
     NewModule->Buffer = &PCMCache->addBuffer(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();
   } else if (llvm::MemoryBuffer *Buffer = PCMCache->lookupBuffer(FileName)) {
     NewModule->Buffer = Buffer;
+    // As above, the file descriptor is no longer needed.
+    Entry->closeFile();
   } else {
     // Open the AST file.
     llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Buf((std::error_code()));
     if (FileName == "-") {
       Buf = llvm::MemoryBuffer::getSTDIN();
     } else {
-      // Leave the FileEntry open so if it gets read again by another
-      // ModuleManager it must be the same underlying file.
-      // FIXME: Because FileManager::getFile() doesn't guarantee that it will
-      // give us an open file, this may not be 100% reliable.
+      // Get a buffer of the file and close the file descriptor when done.
       Buf = FileMgr.getBufferForFile(NewModule->File,
                                      /*IsVolatile=*/false,
-                                     /*ShouldClose=*/false);
+                                     /*ShouldClose=*/true);
     }
 
     if (!Buf) {
       ErrorStr = Buf.getError().message();
       return Missing;
     }
 
     NewModule->Buffer = &PCMCache->addBuffer(FileName, std::move(*Buf));