[Clang] Expose RequiresNullTerminator in FileManager.

This is needed to fix the reason
0a2be46 (Modules: Invalidate out-of-date PCMs as they're
discovered) and 5b44a4b ([modules] Do not cache invalid state for
modules that we attempted to load.) were reverted.

These patches changed Clang to use `isVolatile` when loading modules.
This had the side effect of not using mmap when loading modules, and
thus greatly increased memory usage.

The reason it wasn't using mmap is because `MemoryBuffer` plays some
games with file size when you request null termination, and it has to
disable these when `isVolatile` is set as the size may change by the
time it's mmapped. Clang by default passes
`RequiresNullTerminator = true`, and `shouldUseMmap` ignored if
`RequiresNullTerminator` was even requested.

This patch adds `RequiresNullTerminator` to the `FileManager` interface
so Clang can use it when loading modules, and changes `shouldUseMmap` to
only take volatility into account if `RequiresNullTerminator` is true.
This is fine as both `mmap` and a `read` loop are vulnerable to
modifying the file while reading, but are immune to the rename Clang
does when replacing a module file.

Differential Revision: https://reviews.llvm.org/D77772

Author: Bigcheese

clang/include/clang/Basic/FileManager.h

@@ -378,15 +378,19 @@ class FileManager : public RefCountedBase<FileManager> {
   /// Open the specified file as a MemoryBuffer, returning a new
   /// MemoryBuffer if successful, otherwise returning null.
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
-  getBufferForFile(const FileEntry *Entry, bool isVolatile = false);
+  getBufferForFile(const FileEntry *Entry, bool isVolatile = false,
+                   bool RequiresNullTerminator = true);
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
-  getBufferForFile(StringRef Filename, bool isVolatile = false) {
-    return getBufferForFileImpl(Filename, /*FileSize=*/-1, isVolatile);
+  getBufferForFile(StringRef Filename, bool isVolatile = false,
+                   bool RequiresNullTerminator = true) {
+    return getBufferForFileImpl(Filename, /*FileSize=*/-1, isVolatile,
+                                RequiresNullTerminator);
   }
 
 private:
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
-  getBufferForFileImpl(StringRef Filename, int64_t FileSize, bool isVolatile);
+  getBufferForFileImpl(StringRef Filename, int64_t FileSize, bool isVolatile,
+                       bool RequiresNullTerminator);
 
 public:
   /// Get the 'stat' information for the given \p Path.

clang/lib/Basic/FileManager.cpp

@@ -458,7 +458,8 @@ void FileManager::fillRealPathName(FileEntry *UFE, llvm::StringRef FileName) {
 }
 
 llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
-FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile) {
+FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
+                              bool RequiresNullTerminator) {
   uint64_t FileSize = Entry->getSize();
   // If there's a high enough chance that the file have changed since we
   // got its size, force a stat before opening it.
@@ -468,28 +469,29 @@ FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile) {
   StringRef Filename = Entry->getName();
   // If the file is already open, use the open file descriptor.
   if (Entry->File) {
-    auto Result =
-        Entry->File->getBuffer(Filename, FileSize,
-                               /*RequiresNullTerminator=*/true, isVolatile);
+    auto Result = Entry->File->getBuffer(Filename, FileSize,
+                                         RequiresNullTerminator, isVolatile);
     Entry->closeFile();
     return Result;
   }
 
   // Otherwise, open the file.
-  return getBufferForFileImpl(Filename, FileSize, isVolatile);
+  return getBufferForFileImpl(Filename, FileSize, isVolatile,
+                              RequiresNullTerminator);
 }
 
 llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
 FileManager::getBufferForFileImpl(StringRef Filename, int64_t FileSize,
-                                  bool isVolatile) {
+                                  bool isVolatile,
+                                  bool RequiresNullTerminator) {
   if (FileSystemOpts.WorkingDir.empty())
-    return FS->getBufferForFile(Filename, FileSize,
-                                /*RequiresNullTerminator=*/true, isVolatile);
+    return FS->getBufferForFile(Filename, FileSize, RequiresNullTerminator,
+                                isVolatile);
 
   SmallString<128> FilePath(Filename);
   FixupRelativePath(FilePath);
-  return FS->getBufferForFile(FilePath, FileSize,
-                              /*RequiresNullTerminator=*/true, isVolatile);
+  return FS->getBufferForFile(FilePath, FileSize, RequiresNullTerminator,
+                              isVolatile);
 }
 
 /// getStatValue - Get the 'stat' information for the specified path,

llvm/lib/Support/MemoryBuffer.cpp

@@ -329,7 +329,7 @@ static bool shouldUseMmap(sys::fs::file_t FD,
   // mmap may leave the buffer without null terminator if the file size changed
   // by the time the last page is mapped in, so avoid it if the file size is
   // likely to change.
-  if (IsVolatile)
+  if (IsVolatile && RequiresNullTerminator)
     return false;
 
   // We don't use mmap for small files because this can severely fragment our

llvm/unittests/Support/MemoryBufferTest.cpp

@@ -380,4 +380,32 @@ TEST_F(MemoryBufferTest, writeThroughFile) {
   ASSERT_EQ(16u, MB.getBufferSize());
   EXPECT_EQ("xxxxxxxxxxxxxxxx", MB.getBuffer());
 }
+
+TEST_F(MemoryBufferTest, mmapVolatileNoNull) {
+  // Verify that `MemoryBuffer::getOpenFile` will use mmap when
+  // `RequiresNullTerminator = false`, `IsVolatile = true`, and the file is
+  // large enough to use mmap.
+  //
+  // This is done because Clang should use this mode to open module files, and
+  // falling back to malloc for them causes a huge memory usage increase.
+
+  int FD;
+  SmallString<64> TestPath;
+  ASSERT_NO_ERROR(sys::fs::createTemporaryFile(
+      "MemoryBufferTest_mmapVolatileNoNull", "temp", FD, TestPath));
+  FileRemover Cleanup(TestPath);
+  raw_fd_ostream OF(FD, true);
+  // Create a file large enough to mmap. A 32KiB file should be enough.
+  for (unsigned i = 0; i < 0x1000; ++i)
+    OF << "01234567";
+  OF.flush();
+
+  auto MBOrError = MemoryBuffer::getOpenFile(FD, TestPath,
+      /*FileSize=*/-1, /*RequiresNullTerminator=*/false, /*IsVolatile=*/true);
+  ASSERT_NO_ERROR(MBOrError.getError())
+  OwningBuffer MB = std::move(*MBOrError);
+  EXPECT_EQ(MB->getBufferKind(), MemoryBuffer::MemoryBuffer_MMap);
+  EXPECT_EQ(MB->getBufferSize(), std::size_t(0x8000));
+  EXPECT_TRUE(MB->getBuffer().startswith("01234567"));
+}
 }